Re: Running pgindent - Mailing list pgsql-hackers

On Wed, May 29, 2013 at 10:08:10PM -0400, Stephen Frost wrote:
> * Bruce Momjian (bruce@momjian.us) wrote:
> > Wow, uh, yeah, I guess we could do that.  I will await more feedback.
> 
> Please don't.  I'm already rather concerned by this one.  It looks like
> there's a rule to pull a line in to meet the max-column requirement even
> when that makes things line up 'funny', eg:

I did a comparison of the parameters passed to BSD indent, and Andrew
did accurately transfer all the flags, so I am a little confused why
there is such a difference, as BSD indent has not changed.

> *** a/contrib/hstore/hstore_io.c
> --- b/contrib/hstore/hstore_io.c
> *************** hstore_to_json_loose(PG_FUNCTION_ARGS)
> *** 1300,1306 ****
>              * digit as numeric - could be a zip code or similar
>              */
>             if (src->len > 0 &&
> !               !(src->data[0] == '0' && isdigit((unsigned char) src->data[1])) &&
>                 strspn(src->data, "+-0123456789Ee.") == src->len)
>             {
>                 /*
> --- 1300,1306 ----
>              * digit as numeric - could be a zip code or similar
>              */
>             if (src->len > 0 &&
> !           !(src->data[0] == '0' && isdigit((unsigned char) src->data[1])) &&
>                 strspn(src->data, "+-0123456789Ee.") == src->len)
>             {
>                 /*
> 
> For this case, perhaps we should just split it on to multiple lines, but
> when there's not much on the line beyond a long string, I thought our
> policy was to allow the line to go beyond the column limit?  That's
> certainly how section 49.1 reads to me; any chance we can get indent to
> understand that?

I thought we used to do that too.  However, I don't see this line in the
9.2 hstore source, so I assume it is new and that pgindent is doing what
it used to do.

> We also claim that our indent runs won't screw around with comment
> blocks, but it looks to pretty clearly be doing exactly that..

We only promise that if you use /* ---- markers.

> Another interesting case is this:
> 
> *************** start_postmaster(ClusterInfo *cluster, b
> *** 229,235 ****
>      * it might supply a reason for the failure.
>      */
>     pg_ctl_return = exec_prog(SERVER_START_LOG_FILE,
> !                             /* pass both file names if they differ */
>                               (strcmp(SERVER_LOG_FILE,
>                                       SERVER_START_LOG_FILE) != 0) ?
>                               SERVER_LOG_FILE : NULL,
> --- 229,235 ----
>      * it might supply a reason for the failure.
>      */
>     pg_ctl_return = exec_prog(SERVER_START_LOG_FILE,
> !   /* pass both file names if they differ */
>                               (strcmp(SERVER_LOG_FILE,
>                                       SERVER_START_LOG_FILE) != 0) ?
>                               SERVER_LOG_FILE : NULL,
> 
> It seems that a comment inside of parameters passed to a function isn't
> indented to the same depth of the other arguments by this run, but I'm
> guessing it was by prior versions.

Uh, this comment was definitely in 9.2, but was added as a backpatch by
Tom in September 2012, after pg_upgrade was run on the 9.2 code.

> Also doesn't seem to indent operations the same way that function
> parameters are indented, eg:
> 
> *************** pgrowlocks(PG_FUNCTION_ARGS)
> *** 166,172 ****
>                 values[Atnum_ismulti] = pstrdup("true");
>   
>                 allow_old = !(infomask & HEAP_LOCK_MASK) &&
> !                            (infomask & HEAP_XMAX_LOCK_ONLY);
>                 nmembers = GetMultiXactIdMembers(xmax, &members, allow_old);
>                 if (nmembers == -1)
>                 {
> --- 166,172 ----
>                 values[Atnum_ismulti] = pstrdup("true");
>   
>                 allow_old = !(infomask & HEAP_LOCK_MASK) &&
> !                   (infomask & HEAP_XMAX_LOCK_ONLY);
>                 nmembers = GetMultiXactIdMembers(xmax, &members, allow_old);
>                 if (nmembers == -1)
>                 {
> 
> Which seems to also be 'wrong' to my eyes.

Yes, it certainly does, but this line also does not exist in 9.2 source
tree.

> In general, there are a lot of improvements being made, but I don't like
> what appear to be regressions. :)

I agree.  Can someone fine a case that was run through 9.2 pgindent that
is worse now?  Every case I could find either was the same in 9.2 or was
changed after the 9.2 pgindent run.

--  Bruce Momjian  <bruce@momjian.us>        http://momjian.us EnterpriseDB
http://enterprisedb.com
 + It's impossible for everything to be true. +