pgindent run?

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

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

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

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

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

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

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

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

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

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

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 +

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

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

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 is maintained. 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.