Thread: Running pgindent
Do we want to run pgindent soon? -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. +
On Wed, May 22, 2013 at 1:52 PM, Bruce Momjian <bruce@momjian.us> wrote: > Do we want to run pgindent soon? +1. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Wed, May 22, 2013 at 01:52:28PM -0400, Bruce Momjian wrote: > Do we want to run pgindent soon? OK, should I run it this week? Wednesday, 1800 GMT? -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. +
On Tue, May 28, 2013 at 9:40 AM, Bruce Momjian <bruce@momjian.us> wrote: > On Wed, May 22, 2013 at 01:52:28PM -0400, Bruce Momjian wrote: >> Do we want to run pgindent soon? > > OK, should I run it this week? Wednesday, 1800 GMT? wfm. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Tue, May 28, 2013 at 9:48 AM, Robert Haas <robertmhaas@gmail.com> wrote: > On Tue, May 28, 2013 at 9:40 AM, Bruce Momjian <bruce@momjian.us> wrote: >> On Wed, May 22, 2013 at 01:52:28PM -0400, Bruce Momjian wrote: >>> Do we want to run pgindent soon? >> >> OK, should I run it this week? Wednesday, 1800 GMT? > > wfm. +1. --Magnus HaganderMe: http://www.hagander.net/Work: http://www.redpill-linpro.com/
On Tue, May 28, 2013 at 09:49:32AM -0400, Magnus Hagander wrote: > On Tue, May 28, 2013 at 9:48 AM, Robert Haas <robertmhaas@gmail.com> wrote: > > On Tue, May 28, 2013 at 9:40 AM, Bruce Momjian <bruce@momjian.us> wrote: > >> On Wed, May 22, 2013 at 01:52:28PM -0400, Bruce Momjian wrote: > >>> Do we want to run pgindent soon? > >> > >> OK, should I run it this week? Wednesday, 1800 GMT? > > > > wfm. > > +1. OK, consider it scheduled, 2013-05-29, 1400 ET, 1800 GMT. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. +
On Tue, May 28, 2013 at 09:56:03AM -0400, Bruce Momjian wrote: > On Tue, May 28, 2013 at 09:49:32AM -0400, Magnus Hagander wrote: > > On Tue, May 28, 2013 at 9:48 AM, Robert Haas <robertmhaas@gmail.com> wrote: > > > On Tue, May 28, 2013 at 9:40 AM, Bruce Momjian <bruce@momjian.us> wrote: > > >> On Wed, May 22, 2013 at 01:52:28PM -0400, Bruce Momjian wrote: > > >>> Do we want to run pgindent soon? > > >> > > >> OK, should I run it this week? Wednesday, 1800 GMT? > > > > > > wfm. > > > > +1. > > OK, consider it scheduled, 2013-05-29, 1400 ET, 1800 GMT. Done. This was the first run of the Perl-based pgindent script. There was a lot of code churn in this run as the paragraphs are slightly wider. Also, I saw some outdenting of long lines, rather than allowing them to go past 80 characters, but it seemed minimal. If others see problems, we can adjust the script and run it again. All tests passed. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. +
Bruce Momjian escribió: > Done. This was the first run of the Perl-based pgindent script. There > was a lot of code churn in this run as the paragraphs are slightly > wider. Also, I saw some outdenting of long lines, rather than allowing > them to go past 80 characters, but it seemed minimal. If others see > problems, we can adjust the script and run it again. It seems a bit troubling to me that the underlying indent binary seems to have behaved slightly different. I think this will cause problems down the road when we try to backpatch stuff that has been indented differently on the older branches. I wonder if it would be best to inflict pgindent with the new script upon the old active branches as well, to avoid this problem. (It might cause other problems, of course.) -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
On Wed, May 29, 2013 at 05:56:32PM -0400, Alvaro Herrera wrote: > Bruce Momjian escribió: > > > Done. This was the first run of the Perl-based pgindent script. There > > was a lot of code churn in this run as the paragraphs are slightly > > wider. Also, I saw some outdenting of long lines, rather than allowing > > them to go past 80 characters, but it seemed minimal. If others see > > problems, we can adjust the script and run it again. > > It seems a bit troubling to me that the underlying indent binary seems > to have behaved slightly different. I think this will cause problems > down the road when we try to backpatch stuff that has been indented > differently on the older branches. Well, we have to figure out why it is behaving differently. If I had to guess it would be that the parameters to BSD indent were somehow different, though I thought Andrew copied them directly. > I wonder if it would be best to inflict pgindent with the new script upon > the old active branches as well, to avoid this problem. (It might cause > other problems, of course.) Wow, uh, yeah, I guess we could do that. I will await more feedback. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. +
* 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:
*** 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?
We also claim that our indent runs won't screw around with comment
blocks, but it looks to pretty clearly be doing exactly that..
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.
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.
In general, there are a lot of improvements being made, but I don't like
what appear to be regressions. :)
Thanks,
Stephen
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. +
On 05/29/2013 11:41 PM, Bruce Momjian wrote: > 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. I spent quite a lot of time trying to make the tool behave the same as the old script. cheers andrew
On Fri, May 31, 2013 at 03:51:30PM -0400, Andrew Dunstan wrote: > > On 05/29/2013 11:41 PM, Bruce Momjian wrote: > >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. > > I spent quite a lot of time trying to make the tool behave the same > as the old script. Yes, and I believe we tested running the Perl version to make sure it was the same, so the changes we are seeing are just normal (unfortunate) adjustments by pgindent. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. +
* Bruce Momjian (bruce@momjian.us) wrote:
> On Fri, May 31, 2013 at 03:51:30PM -0400, Andrew Dunstan wrote:
> > I spent quite a lot of time trying to make the tool behave the same
> > as the old script.
>
> Yes, and I believe we tested running the Perl version to make sure it
> was the same, so the changes we are seeing are just normal (unfortunate)
> adjustments by pgindent.
Fair enough, thanks for the efforts and for looking into it, apologies
for my complaining.
If no one objects, I'm going to review the conditional statements and
similar lines which could be split out on to multiple lines instead of
being squeezed closer to the left edge and not lined up properly. Then
I'll try and get pgindent running locally. If I can get that happening
and it doesn't want to further change the modifications that I make,
then I'll commit those cleanups. This will only be for changes made
during this latest pgindent run and only against master- I don't want to
create any more code churn beyond what's already being impacted.
Thanks,
Stephen
On Fri, May 31, 2013 at 04:57:20PM -0400, Stephen Frost wrote: > * Bruce Momjian (bruce@momjian.us) wrote: > > On Fri, May 31, 2013 at 03:51:30PM -0400, Andrew Dunstan wrote: > > > I spent quite a lot of time trying to make the tool behave the same > > > as the old script. > > > > Yes, and I believe we tested running the Perl version to make sure it > > was the same, so the changes we are seeing are just normal (unfortunate) > > adjustments by pgindent. > > Fair enough, thanks for the efforts and for looking into it, apologies > for my complaining. > > If no one objects, I'm going to review the conditional statements and > similar lines which could be split out on to multiple lines instead of > being squeezed closer to the left edge and not lined up properly. Then > I'll try and get pgindent running locally. If I can get that happening > and it doesn't want to further change the modifications that I make, > then I'll commit those cleanups. This will only be for changes made > during this latest pgindent run and only against master- I don't want to > create any more code churn beyond what's already being impacted. OK. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. +
* Bruce Momjian (bruce@momjian.us) wrote:
> OK.
Done.
Thanks,
Stephen
* Bruce Momjian (bruce@momjian.us) wrote:
> On Fri, May 31, 2013 at 03:51:30PM -0400, Andrew Dunstan wrote:
> > I spent quite a lot of time trying to make the tool behave the same
> > as the old script.
>
> Yes, and I believe we tested running the Perl version to make sure it
> was the same, so the changes we are seeing are just normal (unfortunate)
> adjustments by pgindent.
Just to wrap this up- I wanted to say thanks to both you (Bruce) and
to Andrew for making pgindent work and the documentation / instructions
easy to follow. In the past, pgindent has always seemed to be a black
art, where it was difficult to get consistent results across
architectures due to different typedef lists, etc. Now, at least for
me, it 'just worked'.
Thanks!
Stephen
Stephen Frost escribió: > Just to wrap this up- I wanted to say thanks to both you (Bruce) and > to Andrew for making pgindent work and the documentation / instructions > easy to follow. In the past, pgindent has always seemed to be a black > art, where it was difficult to get consistent results across > architectures due to different typedef lists, etc. Now, at least for > me, it 'just worked'. Yes, agreed. I tend to add a pgindent step to my patch submissions (even though my editor does a pretty good job), and this has been made possible by this work. Thanks guys. -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services