Re: add line number as prompt option to psql

On Fri, Jul 11, 2014 at 4:23 PM, Jeevan Chalke
<jeevan(dot)chalke(at)enterprisedb(dot)com> wrote:
> Hi,
>
> A.
>
> However, this introduced new bug. As I told, when editor number of lines
> reaches INT_MAX it starts giving negative number. You tried overcoming this
> issue by adding "< 0" check. But I guess you again fumbled in setting that
> correctly. You are setting it to INT_MAX - 1. This enforces each new line to
> show line number as INT_MAX - 1 which is incorrect.
>
> postgres=# \set PROMPT1 '%/[%l]%R%# '
> postgres[1]=# \set PROMPT2 '%/[%l]%R%# '
> postgres[1]=# \e
> postgres[2147483646]-# limit
> postgres[2147483646]-# 1;
>
> relname
> --------------
> pg_statistic
> (1 row)
>
> postgres[1]=# \e
> postgres[2147483646]-# =
> postgres[2147483646]-# '
> postgres[2147483646]'# abc
> postgres[2147483646]'# '
> postgres[2147483646]-# ;
> relname
> ---------
> (0 rows)
>
>
> postgres[1]=# select
> relname
> from
> pg_class
> where
> relname
> =
> '
> abc
> '
> ;
>
>
> Again to mimic that, I have simply intialized newline to INT_MAX - 2.
> Please don't take me wrong, but it seems that your unit testing is not
> enough. Above issue I discovered by doing exactly same steps I did in
> reviewing previous patch. If you had tested your new patch with those steps
> I guess you have caught that yourself.
>

To my understating cleanly, you means that line number is not changed
when newline has reached to INT_MAX, is incorrect?
And the line number should be switched to 1 when line number has
reached to INT_MAX?

>
> B.
>
> + /* Calculate the line number */
> + if (scan_result != PSCAN_INCOMPLETE)
> + {
> + /* The one new line is always added to tail of query_buf
> */
> + newline = (newline != 0) ? (newline + 1) : 1;
> + cur_line += newline;
> + }
>
> Here in above code changes, in any case you are adding 1 to newline. i.e.
> when it is 0 you are setting it 1 (+1) and when > 0 you are setting nl + 1
> (again +1).
> So why can't you simply use"
> if (scan_result != PSCAN_INCOMPLETE)
> cur_line += (newline + 1);
>
> Or better, why can't you initialize newline with 1 itself and then directly
> assign cur_line with newline. That will eliminate above entire code block,
> isn't it?
> Or much better, simply get rid of newline, and use cur_line itself, will
> this work well for you?

this is better. I will change code to this.

>
> C. Typos:
> 1.
> /* Count the number of new line for calculate ofline number */
>
> Missing space between 'of' and 'line'.
> However better improve that to something like (just suggestion):
> "Count the number of new lines to correctly adjust current line number"
>
> 2.
> /* Avoid cur_line and newline exceeds the INT_MAX */
>
> You are saying avoid cur_line AND newline, but there is no adjustment for
> newline in the code following the comment.
>
> Thanks
> --
> Jeevan B Chalke
> Principal Software Engineer, Product Development
> EnterpriseDB Corporation
> The Enterprise PostgreSQL Company
>

Thanks.
I will fix it.

--
Regards,

-------
Sawada Masahiko