Thread: pgindent run?

pgindent run?

From
Robert Haas
Date:
If nobody minds too much, I'd like to update typedefs.list and
pgindent the whole tree again.  We've generally done a pretty good job
with pgindenting patches as they are committed this cycle, but we're
starting to accumulate things here and there that are not indented
according to what pgindent likes -- and that makes it harder to keep
later patches indented properly.  You can reindent the files they
touch, but then you have to strip back out the changes that are
unrelated to the patch you're working on.  There are only four source
files where  more than a dozen lines will be touched, so I don't think
this will inconvenience anyone too much:
src/backend/access/transam/xlog.c               | 14 +++---src/backend/tcop/postgres.c                     | 18
+++----src/backend/optimizer/util/relnode.c           | 34 +++++++-------src/backend/catalog/partition.c
| 39 ++++++++--------src/backend/utils/mmgr/generation.c             | 62 ++++++++++++-------------
 

And the changes to typedefs.list would be, as of now, like this:
src/tools/pgindent/typedefs.list                | 53 ++++++++++++++-------

Obviously, one of my motivations here is that I keep ending up
commiting patches to partition.c to fix bugs and add features, and the
fact I've missed that a few times is now coming back to haunt me.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


Re: pgindent run?

From
Andres Freund
Date:
Hi,

On 2017-11-28 14:51:06 -0500, Robert Haas wrote:
> If nobody minds too much, I'd like to update typedefs.list and
> pgindent the whole tree again.

+1.

Greetings,

Andres Freund


Re: pgindent run?

From
Robert Haas
Date:
On Tue, Nov 28, 2017 at 2:51 PM, Robert Haas <robertmhaas@gmail.com> wrote:
> There are only four source files where more than a dozen lines will be touched...

...and of course by "four" I mean "five".

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


Re: pgindent run?

From
Mark Dilger
Date:
> On Nov 28, 2017, at 11:51 AM, Robert Haas <robertmhaas@gmail.com> wrote:
>
> If nobody minds too much, I'd like to update typedefs.list and
> pgindent the whole tree again.  We've generally done a pretty good job
> with pgindenting patches as they are committed this cycle, but we're
> starting to accumulate things here and there that are not indented
> according to what pgindent likes -- and that makes it harder to keep
> later patches indented properly.  You can reindent the files they
> touch, but then you have to strip back out the changes that are
> unrelated to the patch you're working on.  There are only four source
> files where  more than a dozen lines will be touched, so I don't think
> this will inconvenience anyone too much:
>
> src/backend/access/transam/xlog.c               | 14 +++---
> src/backend/tcop/postgres.c                     | 18 +++----
> src/backend/optimizer/util/relnode.c            | 34 +++++++-------
> src/backend/catalog/partition.c                 | 39 ++++++++--------
> src/backend/utils/mmgr/generation.c             | 62 ++++++++++++-------------
>
> And the changes to typedefs.list would be, as of now, like this:
>
> src/tools/pgindent/typedefs.list                | 53 ++++++++++++++-------
>
> Obviously, one of my motivations here is that I keep ending up
> commiting patches to partition.c to fix bugs and add features, and the
> fact I've missed that a few times is now coming back to haunt me.

I have no objection, but if the community intends to keep everything
indented per project standards, why is there no git hook to reject
improperly indented code at commit time?  I've suffered some pain
trying to merge code pre-global-indent-run into a branch
post-global-indent-run and would rather this not keep happening.

Sorry if this has been debated before.  A quick search did not turn up
any previous conversation, but that doesn't mean it has never been
discussed.

mark



Re: pgindent run?

From
Tom Lane
Date:
Andres Freund <andres@anarazel.de> writes:
> On 2017-11-28 14:51:06 -0500, Robert Haas wrote:
>> If nobody minds too much, I'd like to update typedefs.list and
>> pgindent the whole tree again.

> +1.

OK by me --- I've several times restrained myself from just doing
an ad-hoc reindent on some of these files.
        regards, tom lane


Re: pgindent run?

From
Tom Lane
Date:
Mark Dilger <hornschnorter@gmail.com> writes:
> I have no objection, but if the community intends to keep everything
> indented per project standards, why is there no git hook to reject
> improperly indented code at commit time?  I've suffered some pain
> trying to merge code pre-global-indent-run into a branch
> post-global-indent-run and would rather this not keep happening.

I think that'd be taking it too far, especially given that the dependency
on a typedefs list means that the git hook might have a different idea
of what's correctly indented than the committer does.  It'd be very hard
to debug such discrepancies and figure out what would satisfy the hook.

In the end we're trying to minimize the net amount of pain involved.
I doubt that mechanized enforcement would fall at the global minimum.
        regards, tom lane


Re: pgindent run?

From
Thomas Munro
Date:
On Wed, Nov 29, 2017 at 9:47 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Mark Dilger <hornschnorter@gmail.com> writes:
>> I have no objection, but if the community intends to keep everything
>> indented per project standards, why is there no git hook to reject
>> improperly indented code at commit time?  I've suffered some pain
>> trying to merge code pre-global-indent-run into a branch
>> post-global-indent-run and would rather this not keep happening.
>
> I think that'd be taking it too far, especially given that the dependency
> on a typedefs list means that the git hook might have a different idea
> of what's correctly indented than the committer does.  It'd be very hard
> to debug such discrepancies and figure out what would satisfy the hook.
>
> In the end we're trying to minimize the net amount of pain involved.
> I doubt that mechanized enforcement would fall at the global minimum.

Is there no way to get reasonable indentation that doesn't depend on
that typedefs list?

-- 
Thomas Munro
http://www.enterprisedb.com


Re: pgindent run?

From
Tom Lane
Date:
Thomas Munro <thomas.munro@enterprisedb.com> writes:
> On Wed, Nov 29, 2017 at 9:47 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> I think that'd be taking it too far, especially given that the dependency
>> on a typedefs list means that the git hook might have a different idea
>> of what's correctly indented than the committer does.  It'd be very hard
>> to debug such discrepancies and figure out what would satisfy the hook.

> Is there no way to get reasonable indentation that doesn't depend on
> that typedefs list?

Perhaps, but not with the tool we've got.

It's well known that C is unparseable without knowing which identifiers
are typedefs, so it doesn't exactly surprise me that it might not be
sanely indentable without knowing that.  But I've not thought hard about
it, nor looked for alternate tools.
        regards, tom lane


Re: pgindent run?

From
Mark Dilger
Date:
> On Nov 28, 2017, at 12:47 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> 
> Mark Dilger <hornschnorter@gmail.com> writes:
>> I have no objection, but if the community intends to keep everything
>> indented per project standards, why is there no git hook to reject
>> improperly indented code at commit time?  I've suffered some pain
>> trying to merge code pre-global-indent-run into a branch
>> post-global-indent-run and would rather this not keep happening.
> 
> I think that'd be taking it too far, especially given that the dependency
> on a typedefs list means that the git hook might have a different idea
> of what's correctly indented than the committer does.  It'd be very hard
> to debug such discrepancies and figure out what would satisfy the hook.

It sounds like it just requires that the committer also commit any changes
to the typedefs list, such that the indenter run by the git hook can use the
same list the committer is using.  For many commits, the typedefs list won't
change, and the hook would just use the most recent one from the repository.

Barring any objections, I'll see if I can make that work on my local git repo
and post a patch if so.

mark


Re: pgindent run?

From
Tom Lane
Date:
Mark Dilger <hornschnorter@gmail.com> writes:
>> On Nov 28, 2017, at 12:47 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> I think that'd be taking it too far, especially given that the dependency
>> on a typedefs list means that the git hook might have a different idea
>> of what's correctly indented than the committer does.  It'd be very hard
>> to debug such discrepancies and figure out what would satisfy the hook.

> It sounds like it just requires that the committer also commit any changes
> to the typedefs list, such that the indenter run by the git hook can use the
> same list the committer is using.  For many commits, the typedefs list won't
> change, and the hook would just use the most recent one from the repository.

> Barring any objections, I'll see if I can make that work on my local git repo
> and post a patch if so.

The other problem that would have to be considered is cross-branch
variation in the indent rules.  We've generally been in the habit of
back-patching HEAD diffs without worrying about whether they meet
back-branch rules; certainly nobody maintains typedefs.list in the
back branches.  Maybe the most expedient answer for that is to only
enforce indentation in HEAD.

I'm still not really on board with this though.  I can definitely
see the day coming when it would block a security patch and somebody
would be scrambling desperately to fix their indentation under time
pressure, even though perhaps the patch had been fine when created.
        regards, tom lane


Re: pgindent run?

From
Mark Dilger
Date:
> On Nov 28, 2017, at 2:57 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Mark Dilger <hornschnorter@gmail.com> writes:
>>> On Nov 28, 2017, at 12:47 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> I think that'd be taking it too far, especially given that the dependency
>>> on a typedefs list means that the git hook might have a different idea
>>> of what's correctly indented than the committer does.  It'd be very hard
>>> to debug such discrepancies and figure out what would satisfy the hook.
>
>> It sounds like it just requires that the committer also commit any changes
>> to the typedefs list, such that the indenter run by the git hook can use the
>> same list the committer is using.  For many commits, the typedefs list won't
>> change, and the hook would just use the most recent one from the repository.
>
>> Barring any objections, I'll see if I can make that work on my local git repo
>> and post a patch if so.
>
> The other problem that would have to be considered is cross-branch
> variation in the indent rules.  We've generally been in the habit of
> back-patching HEAD diffs without worrying about whether they meet
> back-branch rules; certainly nobody maintains typedefs.list in the
> back branches.  Maybe the most expedient answer for that is to only
> enforce indentation in HEAD.
>
> I'm still not really on board with this though.  I can definitely
> see the day coming when it would block a security patch and somebody
> would be scrambling desperately to fix their indentation under time
> pressure, even though perhaps the patch had been fine when created.

Ok, I'll consider the idea dead.  I don't see any solution to that.

mark

Re: pgindent run?

From
Bruce Momjian
Date:
On Tue, Nov 28, 2017 at 04:38:12PM -0500, Tom Lane wrote:
> Thomas Munro <thomas.munro@enterprisedb.com> writes:
> > On Wed, Nov 29, 2017 at 9:47 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >> I think that'd be taking it too far, especially given that the dependency
> >> on a typedefs list means that the git hook might have a different idea
> >> of what's correctly indented than the committer does.  It'd be very hard
> >> to debug such discrepancies and figure out what would satisfy the hook.
> 
> > Is there no way to get reasonable indentation that doesn't depend on
> > that typedefs list?
> 
> Perhaps, but not with the tool we've got.
> 
> It's well known that C is unparseable without knowing which identifiers
> are typedefs, so it doesn't exactly surprise me that it might not be
> sanely indentable without knowing that.  But I've not thought hard about
> it, nor looked for alternate tools.

For people curious about the C typedef parsing details:

    http://calculist.blogspot.com/2009/02/c-typedef-parsing-problem.html
    https://stackoverflow.com/questions/243383/why-cant-c-be-parsed-with-a-lr1-parser

-- 
  Bruce Momjian  <bruce@momjian.us>        http://momjian.us
  EnterpriseDB                             http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+                      Ancient Roman grave inscription +


Re: pgindent run?

From
Andres Freund
Date:
On 2018-01-23 22:22:47 -0500, Bruce Momjian wrote:
> On Tue, Nov 28, 2017 at 04:38:12PM -0500, Tom Lane wrote:
> > Thomas Munro <thomas.munro@enterprisedb.com> writes:
> > > On Wed, Nov 29, 2017 at 9:47 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > >> I think that'd be taking it too far, especially given that the dependency
> > >> on a typedefs list means that the git hook might have a different idea
> > >> of what's correctly indented than the committer does.  It'd be very hard
> > >> to debug such discrepancies and figure out what would satisfy the hook.
> > 
> > > Is there no way to get reasonable indentation that doesn't depend on
> > > that typedefs list?
> > 
> > Perhaps, but not with the tool we've got.
> > 
> > It's well known that C is unparseable without knowing which identifiers
> > are typedefs, so it doesn't exactly surprise me that it might not be
> > sanely indentable without knowing that.  But I've not thought hard about
> > it, nor looked for alternate tools.
> 
> For people curious about the C typedef parsing details:
> 
>     http://calculist.blogspot.com/2009/02/c-typedef-parsing-problem.html
>     https://stackoverflow.com/questions/243383/why-cant-c-be-parsed-with-a-lr1-parser

FWIW, I think this problem could just as well be addressed with a few
printing heuristics instead of actually needing an actual list of
typedefs. There'd be one or two edge cases of bad formatting, but the
end result would be far less painful than what we have today, were
basically nobody can format their patches without a lot of manual
cherry-picking or large scale unrelated whitespace changes.

Greetings,

Andres Freund


Re: pgindent run?

From
Tom Lane
Date:
Andres Freund <andres@anarazel.de> writes:
> FWIW, I think this problem could just as well be addressed with a few
> printing heuristics instead of actually needing an actual list of
> typedefs.

Step right up and implement that, and we'd all be happier.  Certainly
the typedefs list is a pain in the rear.

> There'd be one or two edge cases of bad formatting, but the
> end result would be far less painful than what we have today, were
> basically nobody can format their patches without a lot of manual
> cherry-picking or large scale unrelated whitespace changes.

IMO, the big problem here is that people commit large changes without
having pgindent'd them first.

            regards, tom lane


Re: pgindent run?

From
Andres Freund
Date:

On January 24, 2018 11:34:07 AM PST, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>Andres Freund <andres@anarazel.de> writes:
>> There'd be one or two edge cases of bad formatting, but the
>> end result would be far less painful than what we have today, were
>> basically nobody can format their patches without a lot of manual
>> cherry-picking or large scale unrelated whitespace changes.
>
>IMO, the big problem here is that people commit large changes without
>having pgindent'd them first.

Well, I think it'd really have to be every patch that's indented properly. And that's hard given the way typedefs.list
ismaintained.  Without most new typedefs included one continually reindents with a lot of damage. 

It'd be less bad is we automated the maintenance of the lists so
a) patch authors can automatically add to the list and include that in commits
b) the committed list gets updated automatically every few days based on bf results
c) there's automated whole tree pgindent runs every few days.

Andres
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.