Обсуждение: pgindent run?
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
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 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.