run pgindent on a regular basis / scripted manner

On 2020-Aug-12, Andres Freund wrote:

Is there any reason we don't just automatically run pgindent regularly?
Like once a week? And also update typedefs.list automatically, while
we're at it?

Seconded.

--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Hi Andres,

On Wed, Aug 12, 2020 at 3:34 PM Andres Freund wrote:

Hi,

When developing patches I find it fairly painful that I cannot re-indent
patches with pgindent without also seeing a lot of indentation changes
in unmodified parts of files. It is easy enough ([1]) to only re-indent
files that I have modified, but there's often a lot of independent
indentation changes in the files that I did modified.

I e.g. just re-indented patch 0001 of my GetSnapshotData() series and
most of the hunks were entirely unrelated. Despite the development
window for 14 having only relatively recently opened. Based on my
experience it tends to get worse over time.

How bad was it right after branching 13? I wonder if we have any
empirical measure of badness over time -- assuming there was a point in
the recent past where everything was good, and the bad just crept in.

Is there any reason we don't just automatically run pgindent regularly?
Like once a week? And also update typedefs.list automatically, while
we're at it?

You know what's better than weekly? Every check-in. I for one would love
it if we can just format the entire codebase, and ensure that new
check-ins are also formatted. We _do_ need some form of continuous
integration to catch us when we have fallen short (again, once HEAD
reaches a "known good" state, it's conceivably cheap to keep it in the
good state.

Cheers,
Jesse

Hi,

On 2020-08-12 16:08:50 -0700, Jesse Zhang wrote:

On Wed, Aug 12, 2020 at 3:34 PM Andres Freund wrote:

Hi,

When developing patches I find it fairly painful that I cannot re-indent
patches with pgindent without also seeing a lot of indentation changes
in unmodified parts of files. It is easy enough ([1]) to only re-indent
files that I have modified, but there's often a lot of independent
indentation changes in the files that I did modified.

I e.g. just re-indented patch 0001 of my GetSnapshotData() series and
most of the hunks were entirely unrelated. Despite the development
window for 14 having only relatively recently opened. Based on my
experience it tends to get worse over time.

How bad was it right after branching 13? I wonder if we have any
empirical measure of badness over time -- assuming there was a point in
the recent past where everything was good, and the bad just crept in.

Well, just after branching it was perfect, because pgindent was
customarily is run just before branching. After that it incrementally
gets worse.

Is there any reason we don't just automatically run pgindent regularly?
Like once a week? And also update typedefs.list automatically, while
we're at it?

You know what's better than weekly? Every check-in. I for one would love
it if we can just format the entire codebase, and ensure that new
check-ins are also formatted. We _do_ need some form of continuous
integration to catch us when we have fallen short (again, once HEAD
reaches a "known good" state, it's conceivably cheap to keep it in the
good state.

Unfortunately that is, with the current tooling, not entirely trivial to
do so completely. The way we generate the list of known typedefs
unfortunately depends on the platform a build is run on. Therefore the
buildfarm collects a number of the generated list of typedefs from
different platforms, and then we use that combined list to run pgindent.

We surely can improve further, but I think having any automation around
this already would be a huge step.

Greetings,

Andres Freund

Jesse Zhang <sbjesse@gmail.com> writes:

On Wed, Aug 12, 2020 at 3:34 PM Andres Freund wrote:

Is there any reason we don't just automatically run pgindent regularly?
Like once a week? And also update typedefs.list automatically, while
we're at it?

You know what's better than weekly? Every check-in.

I'm not in favor of unsupervised pgindent runs, really. It can do a lot
of damage to code that was written without thinking about it --- in
particular, it'll make a hash of comment blocks that were manually
formatted and not protected with dashes.

If the workflow is commit first and re-indent later, then we can always
revert the pgindent commit and clean things up manually; but an auto
re-indent during commit wouldn't provide that history.

I do like the idea of more frequent, smaller pgindent runs instead of
doing just one giant run per year. With the giant runs it's necessary
to invest a fair amount of time eyeballing all the changes; if we did it
every couple weeks then the pain would be a lot less.

Another idea would be to have a bot that runs pgindent *without*
committing the results, and emails the people who have made commits
into files that changed, saying "if you don't like these diffs then
you'd better clean up your commit before it happens for real". With
some warning like that, it might be okay to do automatic reindenting
a little bit later. Plus, nagging committers who habitually commit
improperly-indented code might persuade them to clean up their acts ;-)

regards, tom lane

Andres Freund <andres@anarazel.de> writes:

Unfortunately that is, with the current tooling, not entirely trivial to
do so completely. The way we generate the list of known typedefs
unfortunately depends on the platform a build is run on. Therefore the
buildfarm collects a number of the generated list of typedefs from
different platforms, and then we use that combined list to run pgindent.

Yeah, it's hard to see how to avoid that given that the set of typedefs
provided by system headers is not fixed. (Windows vs not Windows is the
worst case of course, but Unixen aren't all alike either.)

Another gotcha that we have to keep our eyes on is that sometimes the
process finds spurious names that we don't want to treat as typedefs
because it results in messing up too much code. There's a reject list
in pgindent that takes care of those, but it has to be maintained
manually. So I'm not sure how that could work in conjunction with
unsupervised reindents --- by the time you notice a problem, the git
history will already be cluttered with bogus reindentations.

Maybe the secret is to not allow automated adoption of new typedefs.list
entries, but to require somebody to add entries to that file by hand,
even if they're basing it on the buildfarm results. (This would
encourage the habit some people have adopted of updating typedefs.list
along with commits that add typedefs. I've never done that, but would
be willing to change if there's good motivation to.)

regards, tom lane

On Wed, Aug 12, 2020 at 06:53:25PM -0400, Alvaro Herrera wrote:

On 2020-Aug-12, Andres Freund wrote:

Is there any reason we don't just automatically run pgindent regularly?
Like once a week? And also update typedefs.list automatically, while
we're at it?

Seconded.

Thirded.
--
Michael

On Wed, Aug 12, 2020 at 07:47:01PM -0400, Tom Lane wrote:

Jesse Zhang <sbjesse@gmail.com> writes:

On Wed, Aug 12, 2020 at 3:34 PM Andres Freund wrote:

Is there any reason we don't just automatically run pgindent regularly?
Like once a week? And also update typedefs.list automatically, while
we're at it?

You know what's better than weekly? Every check-in.

I'm not in favor of unsupervised pgindent runs, really. It can do a lot
of damage to code that was written without thinking about it --- in
particular, it'll make a hash of comment blocks that were manually
formatted and not protected with dashes.

If the workflow is commit first and re-indent later, then we can always
revert the pgindent commit and clean things up manually; but an auto
re-indent during commit wouldn't provide that history.

There are competing implementations of assuring pgindent-cleanliness at every
check-in:

1. After each push, an automated followup commit appears, restoring
pgindent-cleanliness.
2. "git push" results in a commit that melds pgindent changes into what the
committer tried to push.
3. "git push" fails, for the master branch, if the pushed commit disrupts
pgindent-cleanliness.

Were you thinking of (2)? (1) doesn't have the lack-of-history problem, but
it does have the unexpected-damage problem, and it makes gitweb noisier. (3)
has neither problem, and I'd prefer it over (1), (2), or $SUBJECT.

Regarding typedefs.list, I would use the in-tree one, like you discussed here:

Noah Misch <noah@leadboat.com> writes:

On Wed, Aug 12, 2020 at 07:47:01PM -0400, Tom Lane wrote:

If the workflow is commit first and re-indent later, then we can always
revert the pgindent commit and clean things up manually; but an auto
re-indent during commit wouldn't provide that history.

There are competing implementations of assuring pgindent-cleanliness at every
check-in:

1. After each push, an automated followup commit appears, restoring
pgindent-cleanliness.
2. "git push" results in a commit that melds pgindent changes into what the
committer tried to push.
3. "git push" fails, for the master branch, if the pushed commit disrupts
pgindent-cleanliness.

Were you thinking of (2)?

I was objecting to (2). (1) would perhaps work. (3) could be pretty
darn annoying, especially if it blocks a time-critical security patch.

Regarding typedefs.list, I would use the in-tree one, like you discussed here:

Yeah, after thinking about that more, it seems like automated
typedefs.list updates would be far riskier than automated reindent
based on the existing typedefs.list. The latter could at least be
expected not to change code unrelated to the immediate commit.
typedefs.list updates need some amount of adult supervision.

(I'd still vote for nag-mail to the committer whose work got reindented,
in case the bot made things a lot worse.)

I hadn't thought about the angle of HEAD versus back-branch patches,
but that does seem like something to ponder. The back branches don't
have the same pgindent rules necessarily, plus the patch versions
might be different in more than just whitespace. My own habit when
back-patching has been to indent the HEAD patch per-current-rules and
then preserve that layout as much as possible in the back branches,
but I doubt we could get a tool to do that with any reliability.

Of course, there's also the possibility of forcibly reindenting
all the active back branches to current rules. But I think we've
rejected that idea already, because it would cause so much pain
for forks that are following a back branch.

regards, tom lane

On Thu, Aug 13, 2020 at 12:08:36AM -0400, Tom Lane wrote:

Noah Misch <noah@leadboat.com> writes:

On Wed, Aug 12, 2020 at 07:47:01PM -0400, Tom Lane wrote:

If the workflow is commit first and re-indent later, then we can always
revert the pgindent commit and clean things up manually; but an auto
re-indent during commit wouldn't provide that history.

There are competing implementations of assuring pgindent-cleanliness at every
check-in:

1. After each push, an automated followup commit appears, restoring
pgindent-cleanliness.
2. "git push" results in a commit that melds pgindent changes into what the
committer tried to push.
3. "git push" fails, for the master branch, if the pushed commit disrupts
pgindent-cleanliness.

Were you thinking of (2)?

I was objecting to (2). (1) would perhaps work. (3) could be pretty
darn annoying,

Right. I think of three use cases here:

a) I'm a committer who wants to push clean code. I want (3).
b) I'm a committer who wants to ignore pgindent. I get some email complaints
under (1), which I ignore. Under (3), I'm forced to become (a).
c) I'm reading the history. I want (3).

I hadn't thought about the angle of HEAD versus back-branch patches,
but that does seem like something to ponder. The back branches don't
have the same pgindent rules necessarily, plus the patch versions
might be different in more than just whitespace. My own habit when
back-patching has been to indent the HEAD patch per-current-rules and
then preserve that layout as much as possible in the back branches,
but I doubt we could get a tool to do that with any reliability.

Similar habit here. Another advantage of master-only is a guarantee against
disrupting time-critical patches. (It would be ugly to push back branches and
sort out the master push later, but it doesn't obstruct the mission.)

Noah Misch <noah@leadboat.com> writes:

... Another advantage of master-only is a guarantee against
disrupting time-critical patches. (It would be ugly to push back branches and
sort out the master push later, but it doesn't obstruct the mission.)

Hm, doesn't it? I had the idea that "git push" is atomic --- either all
the per-branch commits succeed, or they all fail. I might be wrong.

regards, tom lane

On Thu, Aug 13, 2020 at 01:14:33AM -0400, Tom Lane wrote:

Noah Misch <noah@leadboat.com> writes:

... Another advantage of master-only is a guarantee against
disrupting time-critical patches. (It would be ugly to push back branches and
sort out the master push later, but it doesn't obstruct the mission.)

Hm, doesn't it? I had the idea that "git push" is atomic --- either all
the per-branch commits succeed, or they all fail. I might be wrong.

Atomicity is good. I just meant that you could issue something like "git push
origin $(cd .git/refs/heads && ls REL*)" to defer the complaint about master.

On Wed, Aug 12, 2020 at 10:14 PM Tom Lane wrote:

Noah Misch <noah@leadboat.com> writes:

... Another advantage of master-only is a guarantee against
disrupting time-critical patches. (It would be ugly to push back branches and
sort out the master push later, but it doesn't obstruct the mission.)

Hm, doesn't it? I had the idea that "git push" is atomic --- either all
the per-branch commits succeed, or they all fail. I might be wrong.

As of Git 2.28, atomic pushes are not turned on by default. That means
"git push my-remote foo bar" _can_ result in partial success. I'm that
paranoid who does "git push --atomic my-remote foo bar fizz".

Cheers,
Jesse

On Thu, Aug 13, 2020 at 6:08 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Noah Misch <noah@leadboat.com> writes:

On Wed, Aug 12, 2020 at 07:47:01PM -0400, Tom Lane wrote:

If the workflow is commit first and re-indent later, then we can always
revert the pgindent commit and clean things up manually; but an auto
re-indent during commit wouldn't provide that history.

There are competing implementations of assuring pgindent-cleanliness at

every

check-in:

1. After each push, an automated followup commit appears, restoring
pgindent-cleanliness.
2. "git push" results in a commit that melds pgindent changes into what

the

committer tried to push.
3. "git push" fails, for the master branch, if the pushed commit disrupts
pgindent-cleanliness.

There's another option here as well, that is a bit "softer", is to use a
pre-commit hook.

That is, it's a hook that runs on the committers machine prior to the
commit. This hook can then yell "hey, you need to run pgindent before
committing this", but it gives the committer the ability to do --no-verify
and commit anyway (thus won't block things that are urgent).

Since it allows a simple bypass, and very much relies on the committer to
remember to install the hook in their local repository, this is not a
guarantee in any way. So it might need to be done together with something
else in the background doing like a daily job or so, but it might make that
background work be smaller and fewer changes.

This obviously only works in the case where we can rely on the committers
to remember to install such a hook, but given the few committers that we do
have, I think we can certainly get that up to an "acceptable rate" fairly
easily. FWIW, this is similar to what we do in the pgweb, pgeu and a few
other repositories, to ensure python styleguides are followed.

Were you thinking of (2)?

I was objecting to (2). (1) would perhaps work. (3) could be pretty
darn annoying, especially if it blocks a time-critical security patch.

FWIW, I agree that (2) seems like a really bad option. In that suddenly a
committer has committed something they were not aware of.

Regarding typedefs.list, I would use the in-tree one, like you discussed

here:

Yeah, after thinking about that more, it seems like automated
typedefs.list updates would be far riskier than automated reindent
based on the existing typedefs.list. The latter could at least be
expected not to change code unrelated to the immediate commit.
typedefs.list updates need some amount of adult supervision.

(I'd still vote for nag-mail to the committer whose work got reindented,
in case the bot made things a lot worse.)

Yeah, I'm definitely not a big fan of automated commits, regardless of if
it's just indent or both indent+typedef. It's happened at least once, and I
think more than once, that we've had to basically hard reset the upstream
repository and clean things up after automated commits have gone bonkers
(hi, Bruce!). Having an automated system do the whole flow of
change->commit->push definitely invites this type of problem.

There are many solutions that do such things but that do it in the "github
workflow" way, which is they do change -> commit -> create pull request,
and then somebody eyeballs that pullrequest and approves it. We don't have
PRs, but we could either have a script that simply sends out a patch to a
mailinglist, or we could have a script that maintains a separate branch
which is auto-pgindented, and then have a committer squash-merge that
branch after having reviewed it.

Maybe something like that in combination with a pre-commit hook per above.

--
Magnus Hagander
Me: https://www.hagander.net/ <http://www.hagander.net/&gt;
Work: https://www.redpill-linpro.com/ <http://www.redpill-linpro.com/&gt;

Greetings,

* Magnus Hagander (magnus@hagander.net) wrote:

There's another option here as well, that is a bit "softer", is to use a
pre-commit hook.

Yeah, +1 on a pre-commit idea to help address this. I certainly agree
with Andres that it's quite annoying to deal with commits coming in that
aren't indented properly but are in some file that I'm working on.

This obviously only works in the case where we can rely on the committers
to remember to install such a hook, but given the few committers that we do
have, I think we can certainly get that up to an "acceptable rate" fairly
easily. FWIW, this is similar to what we do in the pgweb, pgeu and a few
other repositories, to ensure python styleguides are followed.

Yeah, no guarantee, but definitely seems like it'd be a good
improvement.

I was objecting to (2). (1) would perhaps work. (3) could be pretty
darn annoying, especially if it blocks a time-critical security patch.

FWIW, I agree that (2) seems like a really bad option. In that suddenly a
committer has committed something they were not aware of.

Yeah, I dislike (2) a lot too.

Yeah, I'm definitely not a big fan of automated commits, regardless of if
it's just indent or both indent+typedef. It's happened at least once, and I
think more than once, that we've had to basically hard reset the upstream
repository and clean things up after automated commits have gone bonkers
(hi, Bruce!). Having an automated system do the whole flow of
change->commit->push definitely invites this type of problem.

Agreed, automated commits seems terribly risky.

There are many solutions that do such things but that do it in the "github
workflow" way, which is they do change -> commit -> create pull request,
and then somebody eyeballs that pullrequest and approves it. We don't have
PRs, but we could either have a script that simply sends out a patch to a
mailinglist, or we could have a script that maintains a separate branch
which is auto-pgindented, and then have a committer squash-merge that
branch after having reviewed it.

Maybe something like that in combination with a pre-commit hook per above.

So, in our world, wouldn't this translate to 'make cfbot complain'?

I'm definitely a fan of the idea of having cfbot flag these and then we
maybe get to a point where it's not the committers dealing with fixing
patches that weren't pgindent'd properly, it's the actual patch
submitters being nagged about it...

Thanks,

Stephen

Stephen Frost <sfrost@snowman.net> writes:

So, in our world, wouldn't this translate to 'make cfbot complain'?

I'm definitely a fan of the idea of having cfbot flag these and then we
maybe get to a point where it's not the committers dealing with fixing
patches that weren't pgindent'd properly, it's the actual patch
submitters being nagged about it...

Meh. Asking all submitters to install pgindent is a bit of a burden.
Moreover, sometimes it's better to provide a patch that deliberately
hasn't reindented existing code, for ease of review (say, when you're
adding if() { ... } around some big hunk of code). I think getting
committers to do this as part of commit is a better workflow.

(Admittedly, since I've been doing that for a long time, I don't
find it to be a burden. I suppose some committers do.)

regards, tom lane

On Thu, Aug 13, 2020 at 6:30 PM Stephen Frost <sfrost@snowman.net> wrote:

* Magnus Hagander (magnus@hagander.net) wrote:

There are many solutions that do such things but that do it in the

"github

workflow" way, which is they do change -> commit -> create pull request,
and then somebody eyeballs that pullrequest and approves it. We don't

have

PRs, but we could either have a script that simply sends out a patch to a
mailinglist, or we could have a script that maintains a separate branch
which is auto-pgindented, and then have a committer squash-merge that
branch after having reviewed it.

Maybe something like that in combination with a pre-commit hook per

above.

So, in our world, wouldn't this translate to 'make cfbot complain'?

I'm definitely a fan of the idea of having cfbot flag these and then we
maybe get to a point where it's not the committers dealing with fixing
patches that weren't pgindent'd properly, it's the actual patch
submitters being nagged about it...

Werll, that's one thing, but what I was thinking of here was more of an
automated branch maintained for the committers, not for the individual
patch owners.

That is:
1. Whenever a patch is pushed on master on the main repo a process kicked
off (or maybe wait 5 minutes to coalesce multiple patches if there are)
2. This process checks out master, and runs pgindent on it
3. When done, this gets committed to a new branch (or just overwrites an
existing branch of course, we don't need to maintain history here) like
"master-indented". This branch can be in a different repo, but one that
starts out as a clone of the main one
4. A committer (any committer) can then on regular basis examine the
differences by fetch + diff. If they're happy with it, cherry pick it in.
If not, figure out what needs to be done to adjust it.

Step 4 can be done at whatever interval we prefer, and we can have
something to nag us if head has been "off-indent" for too long.

This would be the backup for things that aren't indented during patch
commit, not other things.

--
Magnus Hagander
Me: https://www.hagander.net/ <http://www.hagander.net/&gt;
Work: https://www.redpill-linpro.com/ <http://www.redpill-linpro.com/&gt;

On Wed, Aug 12, 2020 at 7:47 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

I'm not in favor of unsupervised pgindent runs, really. It can do a lot
of damage to code that was written without thinking about it --- in
particular, it'll make a hash of comment blocks that were manually
formatted and not protected with dashes.

No committer should be committing code without thinking about
pgindent. If some are, they need to up their game.

I am not sure whether weekly or after-every-commit pgindent runs is a
good idea, but I think we should try to do it once a month or so. It's
too annoying otherwise. I could go either way on the question of
automation.

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

On Thu, Aug 13, 2020 at 12:30 PM Stephen Frost <sfrost@snowman.net> wrote:

So, in our world, wouldn't this translate to 'make cfbot complain'?

This seems like it would be useful, but we'd have to figure out what
to do about typedefs.list. If the patch is indented with the current
one (which is auto-generated by the entire build farm, remember) it's
likely to mess up a patch that's otherwise properly formatted. We'd
either need to insist that people include updates to typedefs.list in
the patch, or else have the cfbot take a stab at doing those updates
itself.

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

Robert Haas <robertmhaas@gmail.com> writes:

I am not sure whether weekly or after-every-commit pgindent runs is a
good idea, but I think we should try to do it once a month or so. It's
too annoying otherwise. I could go either way on the question of
automation.

The traditional reason for not doing pgindent too often has been that
it'd cause more work for people who have to rebase their patches over
pgindent's results. If we want to do it more often, then in order to
respond to that concern, I think we need to do it really often ---
not necessarily quite continuously, but often enough that pgindent
is only changing recently-committed code. In this way, it'd be likely
that anyone with a patch touching that same code would only need to
rebase once not twice. The approaches involving an automated run
give a guarantee of that, otherwise we don't have a guarantee; but
as long as it's not many days delay I think it wouldn't be bad.

Intervals on the order of a month seem likely to be the worst of
both worlds from this standpoint --- too long for people to wait
before rebasing their patch, yet short enough that they'd have
to do so repeatedly.

regards, tom lane

On Thu, Aug 13, 2020 at 3:47 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

The traditional reason for not doing pgindent too often has been that
it'd cause more work for people who have to rebase their patches over
pgindent's results. If we want to do it more often, then in order to
respond to that concern, I think we need to do it really often ---
not necessarily quite continuously, but often enough that pgindent
is only changing recently-committed code. In this way, it'd be likely
that anyone with a patch touching that same code would only need to
rebase once not twice. The approaches involving an automated run
give a guarantee of that, otherwise we don't have a guarantee; but
as long as it's not many days delay I think it wouldn't be bad.

Intervals on the order of a month seem likely to be the worst of
both worlds from this standpoint --- too long for people to wait
before rebasing their patch, yet short enough that they'd have
to do so repeatedly.

Yeah, I get the point. It depends somewhat on how often you think
people will rebase. The main thing against more frequent pgindent runs
is that it clutters the history. If done manually, it's also a lot of
work.

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

Hi,

On 2020-08-13 12:50:16 -0400, Tom Lane wrote:

Stephen Frost <sfrost@snowman.net> writes:

So, in our world, wouldn't this translate to 'make cfbot complain'?

I'm definitely a fan of the idea of having cfbot flag these and then we
maybe get to a point where it's not the committers dealing with fixing
patches that weren't pgindent'd properly, it's the actual patch
submitters being nagged about it...

Meh. Asking all submitters to install pgindent is a bit of a burden.

+1. We could improve on that by slurping it into src/tools though. If
there were a 'make patchprep' target, it'd be a lot more realistic.

But even so, it'd robably further inrease the rate of needing to
constantly rebase lingering patches if cfbot considered indentation
issues failures. E.g. because of typedefs.list updates etc. So I'm
against that, even if we had a patchprep target that didn't need
external tools.

Greetings,

Andres Freund

Greetings,

* Robert Haas (robertmhaas@gmail.com) wrote:

On Thu, Aug 13, 2020 at 12:30 PM Stephen Frost <sfrost@snowman.net> wrote:

So, in our world, wouldn't this translate to 'make cfbot complain'?

This seems like it would be useful, but we'd have to figure out what
to do about typedefs.list. If the patch is indented with the current
one (which is auto-generated by the entire build farm, remember) it's
likely to mess up a patch that's otherwise properly formatted. We'd
either need to insist that people include updates to typedefs.list in
the patch, or else have the cfbot take a stab at doing those updates
itself.

For my 2c, anyway, I like the idea of having folks update the typedefs
themselves when they've got a patch that needs a new typedef to be
indented correctly. Having cfbot try to do that seems unlikely to work
well.

I also didn't mean to imply that we'd push back and ask for a rebase due
to indentation changes, but at the same time, I question if it's really
that realistic a concern- either whomever posted the patch ran pgindent
on it, or they didn't, and I doubt cfbot's check of that would change
without there being a conflict between the patch and something that got
committed anyway.

I also disagree that it's that much of a burden to ask people who are
already hacking on PG to install pgindent.

All that said, seems that others feel differently and while I still
think it's a pretty reasonable idea to have cfbot check, if no one
agrees with me, that's fine too. Having the pre-commit hook would help
with the downstream issue of pgindent pain from unrelated incorrect
indentation, so at least dealing with the patch author not properly
indenting to start with would be just on the bits the patch is already
modifying, which is a lot better.

Thanks,

Stephen

On 2020-Aug-13, Stephen Frost wrote:

For my 2c, anyway, I like the idea of having folks update the typedefs
themselves when they've got a patch that needs a new typedef to be
indented correctly.

Well, let's for starters encourage committers to update typedefs.
Personally I've stayed away from it for each commit just because we've
historically not done it, but I can easily change that. Plus, it cannot
harm.

--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 2020-Aug-13, Magnus Hagander wrote:

That is:
1. Whenever a patch is pushed on master on the main repo a process kicked
off (or maybe wait 5 minutes to coalesce multiple patches if there are)
2. This process checks out master, and runs pgindent on it
3. When done, this gets committed to a new branch (or just overwrites an
existing branch of course, we don't need to maintain history here) like
"master-indented". This branch can be in a different repo, but one that
starts out as a clone of the main one
4. A committer (any committer) can then on regular basis examine the
differences by fetch + diff. If they're happy with it, cherry pick it in.
If not, figure out what needs to be done to adjust it.

Sounds good -- for branch master.

Yesterday I tried to indent some patch across all branches, only to
discover that I'm lacking the pg_bsd_indent necessary for the older
ones. I already have two, but apparently I'd need *four* different
versions with current branches (1.3, 2.0, 2.1, 2.1.1)

--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Sat, Aug 15, 2020 at 1:57 AM Alvaro Herrera <alvherre@2ndquadrant.com> wrote:

On 2020-Aug-13, Magnus Hagander wrote:

That is:
1. Whenever a patch is pushed on master on the main repo a process kicked
off (or maybe wait 5 minutes to coalesce multiple patches if there are)
2. This process checks out master, and runs pgindent on it
3. When done, this gets committed to a new branch (or just overwrites an
existing branch of course, we don't need to maintain history here) like
"master-indented". This branch can be in a different repo, but one that
starts out as a clone of the main one
4. A committer (any committer) can then on regular basis examine the
differences by fetch + diff. If they're happy with it, cherry pick it in.
If not, figure out what needs to be done to adjust it.

Sounds good -- for branch master.

Yesterday I tried to indent some patch across all branches, only to
discover that I'm lacking the pg_bsd_indent necessary for the older
ones. I already have two, but apparently I'd need *four* different
versions with current branches (1.3, 2.0, 2.1, 2.1.1)

FWIW, for back-branches, I just do similar to what Tom said above [1]/messages/by-id/397020.1597291716@sss.pgh.pa.us
("My own habit when back-patching has been to indent the HEAD patch
per-current-rules and then preserve that layout as much as possible in
the back branches"). If we want we can maintain all the required
versions of pg_bsd_indent but as of now, I am not doing so and thought
that following some approximation rule (do it for HEAD and try my best
to maintain the layout for back-branches) is good enough.

[1]: /messages/by-id/397020.1597291716@sss.pgh.pa.us

--
With Regards,
Amit Kapila.

On 2020-08-13 00:34, Andres Freund wrote:

I e.g. just re-indented patch 0001 of my GetSnapshotData() series and
most of the hunks were entirely unrelated. Despite the development
window for 14 having only relatively recently opened. Based on my
experience it tends to get worse over time.

Do we have a sense of why poorly-indented code gets committed? I think
some of the indentation rules are hard to follow manually. (pgperltidy
is worse.)

Also, since pgindent gets run eventually anyway, it's not really that
important to get the indentation right the first time. I suspect the
goal of most authors and committers is to write readable code rather
than to divine the exact pgindent output.

I think as a start, we could just issue a guidelines that all committed
code should follow pgindent. That has never really been a guideline, so
it's not surprising that it's not followed.

--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Hi,

On 2020-08-15 13:47:41 +0200, Peter Eisentraut wrote:

On 2020-08-13 00:34, Andres Freund wrote:

I e.g. just re-indented patch 0001 of my GetSnapshotData() series and
most of the hunks were entirely unrelated. Despite the development
window for 14 having only relatively recently opened. Based on my
experience it tends to get worse over time.

Do we have a sense of why poorly-indented code gets committed? I think some
of the indentation rules are hard to follow manually. (pgperltidy is
worse.)

Also, since pgindent gets run eventually anyway, it's not really that
important to get the indentation right the first time. I suspect the goal
of most authors and committers is to write readable code rather than to
divine the exact pgindent output.

One thing is that some here are actively against manually adding entries
to typedefs.list. Which then means that code gets oddly indented if you
use pgindent. I personally try to make the predictable updates to
typedefs.list, which then at least allows halfway sensibly indenting my
own changes.

I think as a start, we could just issue a guidelines that all committed code
should follow pgindent. That has never really been a guideline, so it's not
surprising that it's not followed.

Without a properly indented baseline that's hard to do, because it'll
cause damage all over. So I don't think we easily can start just there -
we'd first need to re-indent everything.

Greetings,

Andres Freund

Andres Freund <andres@anarazel.de> writes:

One thing is that some here are actively against manually adding entries
to typedefs.list.

I've been of the opinion that it's pointless to do so under the current
regime where (a) only a few people do that and (b) we only officially
re-indent once a year anyway. When I want to manually run pgindent,
I always pull down a fresh typedefs.list from the buildfarm, which is
reasonably up-to-date regardless of what people added or didn't add,
and then add any new typedefs from my current patch to that out-of-tree
copy.

Now, if we switch to a regime where we're trying to keep the tree in
more nearly correctly-indented shape all the time, it would make sense
to revisit that. I'm not saying that it's unreasonable to want to have
the in-tree typedefs.list track reality more closely --- only that doing
so in a half-baked way won't be very helpful.

I think as a start, we could just issue a guidelines that all committed code
should follow pgindent. That has never really been a guideline, so it's not
surprising that it's not followed.

Without a properly indented baseline that's hard to do, because it'll
cause damage all over. So I don't think we easily can start just there -
we'd first need to re-indent everything.

Well, we can certainly do a tree-wide re-indent anytime we're ready.
I doubt it would be very painful right now, with so little new work
since the last run.

regards, tom lane

On Sat, Aug 15, 2020 at 01:44:34PM -0400, Tom Lane wrote:

Andres Freund <andres@anarazel.de> writes:

Without a properly indented baseline that's hard to do, because it'll
cause damage all over. So I don't think we easily can start just there -
we'd first need to re-indent everything.

Well, we can certainly do a tree-wide re-indent anytime we're ready.
I doubt it would be very painful right now, with so little new work
since the last run.

Uh, I thought Tom was saying we need to reindent all branches, which
would be a big change for those tracking forks.

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

The usefulness of a cup is in its emptiness, Bruce Lee

Hi,

On 2020-08-17 13:54:15 -0400, Bruce Momjian wrote:

On Sat, Aug 15, 2020 at 01:44:34PM -0400, Tom Lane wrote:

Andres Freund <andres@anarazel.de> writes:

Without a properly indented baseline that's hard to do, because it'll
cause damage all over. So I don't think we easily can start just there -
we'd first need to re-indent everything.

Well, we can certainly do a tree-wide re-indent anytime we're ready.
I doubt it would be very painful right now, with so little new work
since the last run.

Uh, I thought Tom was saying we need to reindent all branches, which
would be a big change for those tracking forks.

I don't think he said that? He said *if* we were to reindent all
branches, forks would probably have an issue. We're already reindenting
HEAD on a regular basis (just very infrequent), so it can't be a
fundamental issue.

Greetings,

Andres Freund

Bruce Momjian <bruce@momjian.us> writes:

On Sat, Aug 15, 2020 at 01:44:34PM -0400, Tom Lane wrote:

Well, we can certainly do a tree-wide re-indent anytime we're ready.
I doubt it would be very painful right now, with so little new work
since the last run.

Uh, I thought Tom was saying we need to reindent all branches, which
would be a big change for those tracking forks.

No, I'm not for reindenting the back branches in general. There was
some discussion about whether to try to indent back-patched patches
to meet the conventions of the specific back branch, but I doubt
that that's worth troubling over. I think we really only care
about whether HEAD is fully consistently indented.

regards, tom lane

Hi,

On 2020-08-15 13:44:34 -0400, Tom Lane wrote:

Andres Freund <andres@anarazel.de> writes:

One thing is that some here are actively against manually adding entries
to typedefs.list.

I've been of the opinion that it's pointless to do so under the current
regime where (a) only a few people do that and (b) we only officially
re-indent once a year anyway. When I want to manually run pgindent,
I always pull down a fresh typedefs.list from the buildfarm, which is
reasonably up-to-date regardless of what people added or didn't add,
and then add any new typedefs from my current patch to that out-of-tree
copy.

Well, properly indenting new code still is worthwhile. And once you go
through the trouble of adding the typedefs locally, I don't really see
the reason not to also include them in the commit. Sure it'll not help
much with tree-wide re-indents, but with individual files it can still
make life less painful.

Greetings,

Andres Freund

Andres Freund <andres@anarazel.de> writes:

On 2020-08-15 13:44:34 -0400, Tom Lane wrote:

Andres Freund <andres@anarazel.de> writes:

One thing is that some here are actively against manually adding entries
to typedefs.list.

I've been of the opinion that it's pointless to do so under the current
regime where (a) only a few people do that and (b) we only officially
re-indent once a year anyway. When I want to manually run pgindent,
I always pull down a fresh typedefs.list from the buildfarm, which is
reasonably up-to-date regardless of what people added or didn't add,
and then add any new typedefs from my current patch to that out-of-tree
copy.

Well, properly indenting new code still is worthwhile. And once you go
through the trouble of adding the typedefs locally, I don't really see
the reason not to also include them in the commit.

Yeah, I'm quite religious about making sure my commits have been through
pgindent already (mainly to reduce subsequent "git blame" noise).
However, relying on manual updates to the in-tree typedefs.list only
works if every committer is equally religious about it. They're not;
else we'd not be having this discussion. The workflow I describe above
is not dependent on how careful everybody else is, which is why I
prefer it.

I think that the main new idea that's come out of this thread so far
is that very frequent reindents would be as good, maybe better, as
once-a-year reindents in terms of the amount of rebasing pain they
cause for not-yet-committed patches. If we can fix it so that any
mis-indented commit is quickly corrected, then rebasing would only be
needed in places that were changed anyway. So it seems like that
would be OK as a new project policy if we can make it happen.

However, I don't see any way to make it happen like that without
more-or-less automated reindents and typedefs.list updates,
and that remains a bit scary.

I did just have an idea that seems to ameliorate the scariness
a good bit: allow the reindent bot to auto-commit its results
only if the only lines it's changing are ones that were touched
by commits since the last auto-reindent. Otherwise punt and ask
a human to review the results. Not sure how hard that is to
implement, though.

Another good safety check would be to not proceed unless the latest
typedefs list available from the buildfarm is newer than the last
commit --- then we won't mess up recent commits whose typedefs are
not in the list yet.

regards, tom lane

On Fri, Aug 14, 2020 at 10:26 PM Alvaro Herrera <alvherre@2ndquadrant.com>
wrote:

On 2020-Aug-13, Magnus Hagander wrote:

That is:
1. Whenever a patch is pushed on master on the main repo a process kicked
off (or maybe wait 5 minutes to coalesce multiple patches if there are)
2. This process checks out master, and runs pgindent on it
3. When done, this gets committed to a new branch (or just overwrites an
existing branch of course, we don't need to maintain history here) like
"master-indented". This branch can be in a different repo, but one that
starts out as a clone of the main one
4. A committer (any committer) can then on regular basis examine the
differences by fetch + diff. If they're happy with it, cherry pick it in.
If not, figure out what needs to be done to adjust it.

Sounds good -- for branch master.

So mostly for testing, I've set up a job that does this.

Basically it runs every 15 minutes and if there is a new commit on master
it will rebase onto the latest master and run pgindent on it. This then
gets pushed up to a separate repo (postgresql-pgindent.git on
git.postgresql.org), and can be viewed there.

To see the current state of pgindent, view:
https://git.postgresql.org/gitweb/?p=postgresql-pgindent.git;a=commitdiff;h=master-pgindent

(or decorate as wanted to see for example a raw patch format)

If a committer wants to use it directly, just "git remote add" the
postgresql-pgindent.git and then cherry-pick the branch tip into your own
repository, and push. Well, actually that will right now fail because the
commit is made by "Automatic pgindent" which is not an approved committer,
but if we want to do this as a more regular thing, we can certainly fix
that.

Note that the only thing the job does is run pgindent. It does not attempt
to do anything with the typedef list at this point.

--
Magnus Hagander
Me: https://www.hagander.net/ <http://www.hagander.net/&gt;
Work: https://www.redpill-linpro.com/ <http://www.redpill-linpro.com/&gt;

As a not very frequent submitter, on all the patches that I submit I
keep running into this problem. I have two open patchsets for
libpq[1]https://commitfest.postgresql.org/41/3511/[2]https://commitfest.postgresql.org/41/3679/ both of which currently include the same initial "run
pgindent" patch in addition to the actual patch, just so I can
actually run it on my own patch because a9e9a9f caused formatting to
totally be off in the libpq directory. And there's lots of other
changes that pgindent wants to make, which are visible on the job that
Magnus has set up.

To me a master branch that pgindent never complains about sounds
amazing! And I personally think rejection of unindented pushes and
cfbot complaining about unindented patches would be a very good thing,
because that seems to be the only solution that could achieve that.

Having cfbot complain also doesn't sound like a crazy burden for
submitters. Many open-source projects have CI complaining if code
formatting does not pass automatic formatting tools. As long as there
is good documentation on how to install and run pgindent I don't think
it should be a big problem. A link to those docs could even be
included in the failing CI job its error message. A pre-commit hook
that submitters/committers could install would be super useful too.
Since right now I sometimes forget to run pgindent, especially since
there's no editor integration (that I know of) for pgindent.

Side-question: What's the reason why pgindent is used instead of some
more "modern" code formatter that doesn't require keeping
typedefs.list up to date for good looking output? (e.g. uncrustify or
clang-format) Because that would also allow for easy editor
integration.

[1]: https://commitfest.postgresql.org/41/3511/
[2]: https://commitfest.postgresql.org/41/3679/

On Fri, Jan 20, 2023 at 10:43:50AM +0100, Jelte Fennema wrote:

Side-question: What's the reason why pgindent is used instead of some
more "modern" code formatter that doesn't require keeping
typedefs.list up to date for good looking output? (e.g. uncrustify or
clang-format) Because that would also allow for easy editor
integration.

Good question. Our last big pgindent dicussion was in 2017, where I
said:

/messages/by-id/20170612213525.GA4074@momjian.us

Understood. You would think that with the number of open-source
programs written in C that there would be more interest in C formatting
tools. Is the Postgres community the only ones with specific
requirements, or is it just that we settled on an older tool and can't
easily change? I have reviewed the C formatting options a few times
over the years and every time the other options were worse than what we
had.

We also discussed it in 2011, and this email was key for me:

/messages/by-id/201106220218.p5M2InB08144@momjian.us

I am excited Andrew has done this. It has been on my TODO list for a
while --- I was hoping someday we could switch to GNU indent but gave up
after the GNU indent report from Greg Stark that exactly matched my
experience years ago:

http://archives.postgresql.org/pgsql-hackers/2011-04/msg01436.php

Basically, GNU indent has new bugs, but bugs that are harder to work
around than the BSD indent bugs.

--
Bruce Momjian <bruce@momjian.us> https://momjian.us
EDB https://enterprisedb.com

Embrace your flaws. They make you human, rather than perfect,
which you will never be.

Jelte Fennema <postgres@jeltef.nl> writes:

To me a master branch that pgindent never complains about sounds
amazing! And I personally think rejection of unindented pushes and
cfbot complaining about unindented patches would be a very good thing,
because that seems to be the only solution that could achieve that.

The core problem here is that requiring that would translate to
requiring every code contributor to have a working copy of pg_bsd_indent.
Maybe that's not a huge lift, but it'd be YA obstacle to new contributors,
and we don't need any more of those.

Yeah, if we switched to some other tool maybe we could reduce the size
of that problem. But as Bruce replied, we've not found another one that
(a) can be coaxed to make output comparable to what we're accustomed to
and (b) seems decently well maintained.

regards, tom lane

Hi,

On 2023-01-20 12:09:05 -0500, Tom Lane wrote:

Jelte Fennema <postgres@jeltef.nl> writes:

To me a master branch that pgindent never complains about sounds
amazing! And I personally think rejection of unindented pushes and
cfbot complaining about unindented patches would be a very good thing,
because that seems to be the only solution that could achieve that.

The core problem here is that requiring that would translate to
requiring every code contributor to have a working copy of pg_bsd_indent.

Wouldn't just every committer suffice?

Maybe that's not a huge lift, but it'd be YA obstacle to new contributors,
and we don't need any more of those.

Yeah, if we switched to some other tool maybe we could reduce the size
of that problem. But as Bruce replied, we've not found another one that
(a) can be coaxed to make output comparable to what we're accustomed to
and (b) seems decently well maintained.

One question around this is how much change we'd accept. clang-format for
example is well maintained and can get somewhat close to our style - but
there are things that can't easily be approximated.

Greetings,

Andres Freund

Andres Freund <andres@anarazel.de> writes:

On 2023-01-20 12:09:05 -0500, Tom Lane wrote:

The core problem here is that requiring that would translate to
requiring every code contributor to have a working copy of pg_bsd_indent.

Wouldn't just every committer suffice?

Not if we have cfbot complaining about it.

(Another problem here is that there's a sizable subset of committers
who clearly just don't care, and I'm not sure we can convince them to.)

Yeah, if we switched to some other tool maybe we could reduce the size
of that problem. But as Bruce replied, we've not found another one that
(a) can be coaxed to make output comparable to what we're accustomed to
and (b) seems decently well maintained.

One question around this is how much change we'd accept. clang-format for
example is well maintained and can get somewhat close to our style - but
there are things that can't easily be approximated.

If somebody wants to invest the effort in seeing how close we can get
and what the remaining delta would look like, we could have a discussion
about whether it's an acceptable change. I don't think anyone has
tried with clang-format.

regards, tom lane

On 2023-01-20 Fr 13:19, Tom Lane wrote:

Andres Freund <andres@anarazel.de> writes:

On 2023-01-20 12:09:05 -0500, Tom Lane wrote:

The core problem here is that requiring that would translate to
requiring every code contributor to have a working copy of pg_bsd_indent.

Wouldn't just every committer suffice?

Not if we have cfbot complaining about it.

(Another problem here is that there's a sizable subset of committers
who clearly just don't care, and I'm not sure we can convince them to.)

I think we could do better with some automation tooling for committers
here. One low-risk and simple change would be to provide a
non-destructive mode for pgindent that would show you the changes if any
it would make. That could be worked into a git pre-commit hook that
committers could deploy. I can testify to the usefulness of such hooks -
I have one that while not perfect has saved me on at least two occasions
from forgetting to bump the catalog version.

I'll take a look at fleshing this out, for my own if no-one else's use.

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

On 2023-01-21 Sa 08:26, Andrew Dunstan wrote:

On 2023-01-20 Fr 13:19, Tom Lane wrote:

Andres Freund <andres@anarazel.de> writes:

On 2023-01-20 12:09:05 -0500, Tom Lane wrote:

The core problem here is that requiring that would translate to
requiring every code contributor to have a working copy of pg_bsd_indent.

Wouldn't just every committer suffice?

Not if we have cfbot complaining about it.

(Another problem here is that there's a sizable subset of committers
who clearly just don't care, and I'm not sure we can convince them to.)

I think we could do better with some automation tooling for committers
here. One low-risk and simple change would be to provide a
non-destructive mode for pgindent that would show you the changes if any
it would make. That could be worked into a git pre-commit hook that
committers could deploy. I can testify to the usefulness of such hooks -
I have one that while not perfect has saved me on at least two occasions
from forgetting to bump the catalog version.

I'll take a look at fleshing this out, for my own if no-one else's use.

Here's a quick patch for this. I have it in mind to use like this in a
pre-commit hook:

# only do this on master
test `git rev-parse --abbrev-ref HEAD` = "master" || exit 0

src/tools/pgindent/pg_indent --silent `git diff --cached --name-only` || \

  { echo "Need a pgindent run" >&2 ; exit 1; }

The committer could then run

src/tools/pgindent/pg_indent --show-diff `git diff --cached --name-only`

to see what changes it thinks are needed.

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

On 2023-01-21 Sa 10:00, Andrew Dunstan wrote:

On 2023-01-21 Sa 08:26, Andrew Dunstan wrote:

On 2023-01-20 Fr 13:19, Tom Lane wrote:

Andres Freund <andres@anarazel.de> writes:

On 2023-01-20 12:09:05 -0500, Tom Lane wrote:

The core problem here is that requiring that would translate to
requiring every code contributor to have a working copy of pg_bsd_indent.

Wouldn't just every committer suffice?

Not if we have cfbot complaining about it.

(Another problem here is that there's a sizable subset of committers
who clearly just don't care, and I'm not sure we can convince them to.)

I think we could do better with some automation tooling for committers
here. One low-risk and simple change would be to provide a
non-destructive mode for pgindent that would show you the changes if any
it would make. That could be worked into a git pre-commit hook that
committers could deploy. I can testify to the usefulness of such hooks -
I have one that while not perfect has saved me on at least two occasions
from forgetting to bump the catalog version.

I'll take a look at fleshing this out, for my own if no-one else's use.

Here's a quick patch for this. I have it in mind to use like this in a
pre-commit hook:

# only do this on master
test `git rev-parse --abbrev-ref HEAD` = "master" || exit 0

src/tools/pgindent/pg_indent --silent `git diff --cached --name-only` || \

  { echo "Need a pgindent run" >&2 ; exit 1; }

The committer could then run

src/tools/pgindent/pg_indent --show-diff `git diff --cached --name-only`

to see what changes it thinks are needed.

This time with patch.

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

On 2023-01-21 Sa 10:02, Andrew Dunstan wrote:

On 2023-01-21 Sa 10:00, Andrew Dunstan wrote:

On 2023-01-21 Sa 08:26, Andrew Dunstan wrote:

On 2023-01-20 Fr 13:19, Tom Lane wrote:

Andres Freund <andres@anarazel.de> writes:

On 2023-01-20 12:09:05 -0500, Tom Lane wrote:

The core problem here is that requiring that would translate to
requiring every code contributor to have a working copy of pg_bsd_indent.

Wouldn't just every committer suffice?

Not if we have cfbot complaining about it.

(Another problem here is that there's a sizable subset of committers
who clearly just don't care, and I'm not sure we can convince them to.)

I think we could do better with some automation tooling for committers
here. One low-risk and simple change would be to provide a
non-destructive mode for pgindent that would show you the changes if any
it would make. That could be worked into a git pre-commit hook that
committers could deploy. I can testify to the usefulness of such hooks -
I have one that while not perfect has saved me on at least two occasions
from forgetting to bump the catalog version.

I'll take a look at fleshing this out, for my own if no-one else's use.

Here's a quick patch for this. I have it in mind to use like this in a
pre-commit hook:

# only do this on master
test `git rev-parse --abbrev-ref HEAD` = "master" || exit 0

src/tools/pgindent/pg_indent --silent `git diff --cached --name-only` || \

  { echo "Need a pgindent run" >&2 ; exit 1; }

The committer could then run

src/tools/pgindent/pg_indent --show-diff `git diff --cached --name-only`

to see what changes it thinks are needed.

This time with patch.

... with typo fixed.

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Andrew Dunstan <andrew@dunslane.net> writes:

I think we could do better with some automation tooling for committers
here. One low-risk and simple change would be to provide a
non-destructive mode for pgindent that would show you the changes if any
it would make. That could be worked into a git pre-commit hook that
committers could deploy. I can testify to the usefulness of such hooks -
I have one that while not perfect has saved me on at least two occasions
from forgetting to bump the catalog version.

That sounds like a good idea from here. I do not think we want a
mandatory commit filter, but if individual committers care to work
this into their process in some optional way, great! I can think
of ways I'd use it during patch development, too.

(One reason not to want a mandatory filter is that you might wish
to apply pgindent as a separate commit, so that you can then
put that commit into .git-blame-ignore-revs. This could be handy
for example when a patch needs to change the nesting level of a lot
of pre-existing code, without making changes in it otherwise.)

This time with patch.

... with typo fixed.

Looks reasonable, but you should also update
src/tools/pgindent/pgindent.man, which AFAICT is our only
documentation for pgindent switches. (Is it time for a
--help option in pgindent?)

regards, tom lane

... btw, can we get away with making the diff run be "diff -upd"
not just "diff -u"? I find diff output for C files noticeably
more useful with those options, but I'm unsure about their
portability.

regards, tom lane

On Fri, Jan 20, 2023 at 10:43:50AM +0100, Jelte Fennema wrote:

Side-question: What's the reason why pgindent is used instead of some
more "modern" code formatter that doesn't require keeping
typedefs.list up to date for good looking output? (e.g. uncrustify or
clang-format) Because that would also allow for easy editor
integration.

One reason the typedef list is required is a quirk of the C syntax.
Most languages have a lexer/scanner, which tokenizes, and a parser,
which parses. The communication is usually one-way, lexer to parser.
For C, typedefs require the parser to feed new typedefs back into the
lexer:

http://calculist.blogspot.com/2009/02/c-typedef-parsing-problem.html

BSD indent doesn't have that feedback mechanism, probably because it
doesn't fully parse the C file. Therefore, we have to supply typedefs
manually, and for Postgres we pull them from debug-enabled binaries in
our buildfarm. The problem with that is you often import typedefs from
system headers, and the typedefs apply to all C files, not just the ones
were the typdefs are visible.

I don't see uncrustify or clang-format supporting typedef lists so maybe
they implemented this feedback loop. It would be good to see if we can
get either of these tools to match our formatting.

--
Bruce Momjian <bruce@momjian.us> https://momjian.us
EDB https://enterprisedb.com

Embrace your flaws. They make you human, rather than perfect,
which you will never be.

On Sat, Jan 21, 2023 at 9:30 AM Bruce Momjian <bruce@momjian.us> wrote:

I don't see uncrustify or clang-format supporting typedef lists so maybe
they implemented this feedback loop. It would be good to see if we can
get either of these tools to match our formatting.

I personally use clang-format for Postgres development work, since it
integrates nicely with my text editor, and can be configured to
produce approximately the same result as pgindent (certainly good
enough when working on a patch that's still far from a committable
state). I'm fairly sure that clang-format has access to a full AST
from the clang compiler, which is the ideal approach - at least in
theory.

In practice this approach tends to run into problems when the relevant
AST isn't available. For example, if there's code that only builds on
Windows, maybe it won't work at all (at least on my Linux system).
This doesn't really bother me currently, since I only rely on
clang-format as a first pass sort of thing. Maybe I could figure out a
better way to deal with such issues, but right now I don't have much
incentive to.

Another advantage of clang-format is that it's a known quantity. For
example there is direct support for it built into meson, with bells
and whistles such as CI support:

https://mesonbuild.com/Code-formatting.html

My guess is that moving to clang-format would require giving up some
flexibility, but getting much better integration with text editors and
tools like meson in return. It would probably make it practical to
have much stronger rules about how committed code must be indented --
rules that are practical, and can actually be enforced. That trade-off
seems likely to be worth it in my view, though it's not something that
I feel too strongly about.

--
Peter Geoghegan

Peter Geoghegan <pg@bowt.ie> writes:

In practice this approach tends to run into problems when the relevant
AST isn't available. For example, if there's code that only builds on
Windows, maybe it won't work at all (at least on my Linux system).

Hmm, that could be a deal-breaker. It's not going to be acceptable
to have to pgindent different parts of the system on different platforms
... at least not unless we can segregate them on the file level, and
even that would have a large PITA factor.

Still, we won't know unless someone makes a serious experiment with it.

regards, tom lane

On Sat, Jan 21, 2023 at 12:59 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Hmm, that could be a deal-breaker. It's not going to be acceptable
to have to pgindent different parts of the system on different platforms
... at least not unless we can segregate them on the file level, and
even that would have a large PITA factor.

It's probably something that could be worked around. My remarks are
based on some dim memories of dealing with the tool before I arrived
at a configuration that works well enough for me. Importantly,
clang-format doesn't require you to futz around with Makefiles or
objdump or anything like that -- that's a huge plus. It doesn't seem
to impose any requirements on how I build Postgres at all (I generally
use gcc, not clang).

Even if these kinds of issues proved to be a problem for the person
tasked with running clang-format against the whole tree periodically,
they still likely wouldn't affect most of us. It's quite convenient to
use clang-format from an editor -- it can be invoked very
incrementally, against a small range of lines at a time. It's pretty
much something that I can treat like the built-in indent for my
editor. It's vastly different to the typical pgindent workflow.

Still, we won't know unless someone makes a serious experiment with it.

There is one thing about clang-format that I find mildly infuriating:
it can indent function declarations in the way that I want it to, and
it can indent variable declarations in the way that I want it to. It
just can't do both at the same time, because they're both controlled
by AlignConsecutiveDeclarations.

Of course the way that I want to do things is (almost by definition)
the pgindent way, at least right now -- it's not necessarily about my
fixed preferences (though it can be hard to tell!). It's really not
surprising that clang-format cannot quite perfectly simulate pgindent.
How flexible can we be about stuff like that? Obviously there is no
clear answer right now.

--
Peter Geoghegan

Peter Geoghegan <pg@bowt.ie> writes:

Of course the way that I want to do things is (almost by definition)
the pgindent way, at least right now -- it's not necessarily about my
fixed preferences (though it can be hard to tell!). It's really not
surprising that clang-format cannot quite perfectly simulate pgindent.
How flexible can we be about stuff like that? Obviously there is no
clear answer right now.

I don't feel wedded to every last detail of what pgindent does (and
especially not the bugs). But I think if the new tool is not a pretty
close match we'll be in for years of back-patching pain. We have made
changes in pgindent itself in the past, and the patching consequences
weren't *too* awful, but the changes weren't very big either.

As I said upthread, this is really impossible to answer without a
concrete proposal of how to configure clang-format and a survey of
what diffs we'd wind up with.

regards, tom lane

Hi,

On 2023-01-21 14:05:41 -0800, Peter Geoghegan wrote:

On Sat, Jan 21, 2023 at 12:59 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Hmm, that could be a deal-breaker. It's not going to be acceptable
to have to pgindent different parts of the system on different platforms
... at least not unless we can segregate them on the file level, and
even that would have a large PITA factor.

Unless I miss something, I don't think clang-format actually does that level
of C parsing - you can't pass include paths etc, so it really can't.

It's probably something that could be worked around. My remarks are
based on some dim memories of dealing with the tool before I arrived
at a configuration that works well enough for me.

Could you share your .clang-format?

Still, we won't know unless someone makes a serious experiment with it.

There is one thing about clang-format that I find mildly infuriating:
it can indent function declarations in the way that I want it to, and
it can indent variable declarations in the way that I want it to. It
just can't do both at the same time, because they're both controlled
by AlignConsecutiveDeclarations.

Of course the way that I want to do things is (almost by definition)
the pgindent way, at least right now -- it's not necessarily about my
fixed preferences (though it can be hard to tell!). It's really not
surprising that clang-format cannot quite perfectly simulate pgindent.
How flexible can we be about stuff like that? Obviously there is no
clear answer right now.

I personally find the current indentation of variables assignment deeply
unhelpful - but changing it would be a very noisy change.

Greetings,

Andres Freund

Hi,

On 2023-01-21 17:20:35 -0500, Tom Lane wrote:

I don't feel wedded to every last detail of what pgindent does (and
especially not the bugs). But I think if the new tool is not a pretty
close match we'll be in for years of back-patching pain. We have made
changes in pgindent itself in the past, and the patching consequences
weren't *too* awful, but the changes weren't very big either.

Perhaps we could backpatch formatting changes in a way that doesn't
inconvenience forks too much. One way to deal with such changes is to

1) revert the re-indent commits in $backbranch
2) merge $backbranch-with-revert into $forkbranch
3) re-indent $forkbranch

After that future changes should be mergable again.

Certainly doesn't do away with the pain entirely, but it does make it perhaps
bearable

Greetings,

Andres Freund

On Sat, Jan 21, 2023 at 2:43 PM Andres Freund <andres@anarazel.de> wrote:

Unless I miss something, I don't think clang-format actually does that level
of C parsing - you can't pass include paths etc, so it really can't.

It's hard to keep track of, since I also use clangd, which is
influenced by .clang-format for certain completions. It clearly does
plenty of stuff that requires an AST, since it requires a
compile_commands.json. You're the LLVM committer, not me.

Attached is my .clang-format, since you asked for it. It was
originally based on stuff that both you and Peter E posted several
years back, I believe. Plus the timescaledb one in one or two places.
I worked a couple of things out through trial and error. It's
relatively hard to follow the documentation, and there have been
features added to newer LLVM versions.

--
Peter Geoghegan

Attached are a .clang-format file and an uncrustify.cfg file that I
whipped up to match the current style at least somewhat.

I was unable to get either of them to produce the weird alignment of
declarations that pgindent outputs. Maybe that's just because I don't
understand what this alignment is supposed to do. Because to me the
current alignment seems completely random most of the time (and like
Andres said also not very unhelpful). For clang-format you should use
at least clang-format 15, otherwise it has some bugs in the alignment
logic.

One thing that clang-format really really wants to change in the
codebase, is making it consistent on what to do when
arguments/parameters don't fit on a single line: You can either choose
to minimally pack everything on the minimum number of lines, or to put
all arguments on their own separate line. Uncrustify is a lot less
strict about that and will leave most things as they currently are.

Overall it seems that uncrustify is a lot more customizable, whether
that's a good thing is debatable. Personally I think the fewer
possible debates I have about codestyle the better, so I usually like
the tools with fewer options better. But if the customizability allows
for closer matching of existing codestyle then it might be worth the
extra debates and effort in customization in this case.

1) revert the re-indent commits in $backbranch
2) merge $backbranch-with-revert into $forkbranch
3) re-indent $forkbranch

The git commands to achieve this are something like the following.
I've used such git commands in the past to make big automatic
refactors much easier to get in and it has worked quite well so far.

git checkout forkbranch
git rebase {commit-before-re-indent}
# now, get many merge conflicts
git rebase {re-indent-commit}
# keep your own changes (its --theirs instead of --ours because rebase
flips it around)
git checkout --theirs .
run-new-reindent
git add .
git rebase --continue

On Sat, Jan 21, 2023 at 3:39 PM Jelte Fennema <postgres@jeltef.nl> wrote:

I was unable to get either of them to produce the weird alignment of
declarations that pgindent outputs. Maybe that's just because I don't
understand what this alignment is supposed to do. Because to me the
current alignment seems completely random most of the time (and like
Andres said also not very unhelpful). For clang-format you should use
at least clang-format 15, otherwise it has some bugs in the alignment
logic.

Really? I have been using 14, which is quite recent. Did you just
figure this out recently? If this is true, then it's certainly
discouraging.

I don't have a problem with the current pgindent alignment of function
parameters, so not sure what you mean about that. It *was* terrible
prior to commit e3860ffa, but that was back in 2017 (pg_bsd_indent 2.0
fixed that problem).

--
Peter Geoghegan

On Sat, Jan 21, 2023 at 2:05 PM Peter Geoghegan <pg@bowt.ie> wrote:

There is one thing about clang-format that I find mildly infuriating:
it can indent function declarations in the way that I want it to, and
it can indent variable declarations in the way that I want it to. It
just can't do both at the same time, because they're both controlled
by AlignConsecutiveDeclarations.

Looks like I'm not the only one that doesn't like this behavior:

https://github.com/llvm/llvm-project/issues/55605

--
Peter Geoghegan

On Sat, Jan 21, 2023 at 08:08:34PM -0800, Peter Geoghegan wrote:

On Sat, Jan 21, 2023 at 2:05 PM Peter Geoghegan <pg@bowt.ie> wrote:

There is one thing about clang-format that I find mildly infuriating:
it can indent function declarations in the way that I want it to, and
it can indent variable declarations in the way that I want it to. It
just can't do both at the same time, because they're both controlled
by AlignConsecutiveDeclarations.

Looks like I'm not the only one that doesn't like this behavior:

https://github.com/llvm/llvm-project/issues/55605

Wow, that is very weird. When I work with other open source projects, I
am regularly surprised by their low quality requirements.

--
Bruce Momjian <bruce@momjian.us> https://momjian.us
EDB https://enterprisedb.com

Embrace your flaws. They make you human, rather than perfect,
which you will never be.

Hi,

On 2023-01-21 23:21:22 -0500, Bruce Momjian wrote:

On Sat, Jan 21, 2023 at 08:08:34PM -0800, Peter Geoghegan wrote:

On Sat, Jan 21, 2023 at 2:05 PM Peter Geoghegan <pg@bowt.ie> wrote:

There is one thing about clang-format that I find mildly infuriating:
it can indent function declarations in the way that I want it to, and
it can indent variable declarations in the way that I want it to. It
just can't do both at the same time, because they're both controlled
by AlignConsecutiveDeclarations.

Looks like I'm not the only one that doesn't like this behavior:

https://github.com/llvm/llvm-project/issues/55605

Wow, that is very weird. When I work with other open source projects, I
am regularly surprised by their low quality requirements.

I don't think not fulfulling precisely our needs is the same as low quality
requirements.

Greetings,

Andres Freund

Hi,

On 2023-01-21 15:32:45 -0800, Peter Geoghegan wrote:

Attached is my .clang-format, since you asked for it. It was
originally based on stuff that both you and Peter E posted several
years back, I believe. Plus the timescaledb one in one or two places.
I worked a couple of things out through trial and error. It's
relatively hard to follow the documentation, and there have been
features added to newer LLVM versions.

Reformatting with your clang-format end up with something like:

Peter's:
2234 files changed, 334753 insertions(+), 289772 deletions(-)

Jelte's:
2236 files changed, 357500 insertions(+), 306815 deletions(-)

Mine (modified to reduce this):
2226 files changed, 261538 insertions(+), 256039 deletions(-)

Which is all at least an order of magnitude too much.

Jelte's uncrustify:
1773 files changed, 121722 insertions(+), 125369 deletions(-)

better, but still not great. Also had to prevent a file files it choked on
from getting reindented.

I think the main issue with either is that our variable definition indentation
just can't be emulated by the tools as-is.

Some tools can indent variable definitions so that the variable name starts on
the same column. Some can limit that for too long type names. But so far I
haven't seen one that cn make that column be column +12. They all look to
other surrounding types.

I hate that variable name indentation with a fiery passion. But switching away
from that intermixed with a lot of other changes isn't going to be fun.

Greetings,

Andres Freund

On 2023-01-21 Sa 11:10, Tom Lane wrote:

Andrew Dunstan <andrew@dunslane.net> writes:

I think we could do better with some automation tooling for committers
here. One low-risk and simple change would be to provide a
non-destructive mode for pgindent that would show you the changes if any
it would make. That could be worked into a git pre-commit hook that
committers could deploy. I can testify to the usefulness of such hooks -
I have one that while not perfect has saved me on at least two occasions
from forgetting to bump the catalog version.

That sounds like a good idea from here. I do not think we want a
mandatory commit filter, but if individual committers care to work
this into their process in some optional way, great! I can think
of ways I'd use it during patch development, too.

Yes, it's intended for use at committers' discretion. We have no way of
forcing use of a git hook on committers, although we could reject pushes
that offend against certain rules. For the reasons you give below that's
not a good idea. A pre-commit hook can be avoided by using `git commit
-n` and there's are similar option/hook for `git merge`.

(One reason not to want a mandatory filter is that you might wish
to apply pgindent as a separate commit, so that you can then
put that commit into .git-blame-ignore-revs. This could be handy
for example when a patch needs to change the nesting level of a lot
of pre-existing code, without making changes in it otherwise.)

Agreed.

Looks reasonable, but you should also update
src/tools/pgindent/pgindent.man, which AFAICT is our only
documentation for pgindent switches. (Is it time for a
--help option in pgindent?)

Yes, see revised patch.

... btw, can we get away with making the diff run be "diff -upd"
not just "diff -u"? I find diff output for C files noticeably
more useful with those options, but I'm unsure about their
portability.

I think they are available on Linux, MacOS and FBSD, and on Windows (if
anyone's actually using it for this) it's likely to be Gnu diff. So I
think that's probably enough coverage.

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Andrew Dunstan <andrew@dunslane.net> writes:

... btw, can we get away with making the diff run be "diff -upd"
not just "diff -u"? I find diff output for C files noticeably
more useful with those options, but I'm unsure about their
portability.

I think they are available on Linux, MacOS and FBSD, and on Windows (if
anyone's actually using it for this) it's likely to be Gnu diff. So I
think that's probably enough coverage.

I checked NetBSD as well, and it has all three too.

Patch looks good to me.

regards, tom lane

Peter Geoghegan <pg@bowt.ie> writes:

On Sat, Jan 21, 2023 at 3:39 PM Jelte Fennema <postgres@jeltef.nl> wrote:

... For clang-format you should use
at least clang-format 15, otherwise it has some bugs in the alignment
logic.

Really? I have been using 14, which is quite recent. Did you just
figure this out recently? If this is true, then it's certainly
discouraging.

Indeed. What that points to is a future where different contributors
get different results depending on what clang version they have
installed --- and it's not going to be practical to insist that
everybody have the same version, because AFAICT clang-format is tied
to clang itself. So that sounds a bit unappetizing.

One of the few advantages of the current tool situation is that at any
time there's just one agreed-on version of pgindent and pgperltidy.
I've not heard push-back about our policy that you should use
perltidy version 20170521, because that's not especially connected
to any other part of one's system. Maybe the same would hold for
uncrustify, but it's never going to work for pieces of the clang
ecosystem.

regards, tom lane

But so far I haven't seen one that can make that
column be column +12.

Thanks for clarifying what the current variable declaration indention
rule is. Indeed neither uncrustify or clang-format seem to support
that. Getting uncrustify to support it might not be too difficult, but
the question remains if we even want that.

But switching away from that intermixed with a lot of other changes isn't going to be fun.

I don't think the amount of pain is really much lower if we reformat
10,000 or 300,000 lines of code, without automation both would be
quite painful. But the git commands I shared in my previous email
should alleviate most of that pain.

I don't have a problem with the current pgindent alignment of function
parameters, so not sure what you mean about that.

Function parameter alignment is fine with pgindent imho, but this +12
column variable declaration thing I personally think is quite weird.

Really? I have been using 14, which is quite recent. Did you just
figure this out recently? If this is true, then it's certainly
discouraging.

It seems this was due to my Ubuntu 22.04 install having clang-format
14.0.0. After
updating it to 14.0.6 by using the official llvm provided packages, I
don't have this
issue on clang-format-14 anymore. To be clear this was an issue in alignment of
variable declarations not function parameters.

But I agree with Tom Lane that this makes clear that whatever tool we
pick we'll need
to pick a specific version, just like we do now with perltidy. And
indeed I'm not sure
how easy that is with clang. Installing a specific uncrustify version
is pretty easy btw,
the compilation from source is quite quick.

But I do think this discussion about other formatting tools
is distracting from the main pain point I wanted to discuss:
our current formatting tool is not run consistently enough.
The only thing that another tool will change in this
regard is that there is no need to update typedefs.list.
It doesn't seem like that's such a significant difference
that it would change the solution to the first problem.

When reading the emails in this discussion from 2 years ago
it seems like the respondents wouldn't mind updating the
typedefs.list manually. And proposed approach number 3
seemed to have support overall, i.e. fail a push to master
when pgindent was not run on the new commit. Would
it make sense to simply try that approach and see if
there's any big issues with it?

(Another problem here is that there's a sizable subset of committers
who clearly just don't care, and I'm not sure we can convince them to.)

My guess would be that the main reason is simply
because committers forget it sometimes because
there's no automation complaining about it.

Jelte Fennema <postgres@jeltef.nl> writes:

When reading the emails in this discussion from 2 years ago
it seems like the respondents wouldn't mind updating the
typedefs.list manually. And proposed approach number 3
seemed to have support overall, i.e. fail a push to master
when pgindent was not run on the new commit. Would
it make sense to simply try that approach and see if
there's any big issues with it?

I will absolutely not accept putting in something that fails pushes
on this basis. There are too many cases where pgindent purity is
not an overriding issue. I mentioned a counterexample just upthread:
even if you are as anal as you could be about indentation, you might
prefer to separate a logic-changing patch from the ensuing mechanical
reindentation.

regards, tom lane

Maybe I'm not understanding your issue correctly, but for such
a case you could push two commits at the same time. Apart
from that "git diff -w" will hide any whitespace changes so I'm
not I personally wouldn't consider it important to split such
changes across commits.

Hi,

On 2023-01-22 22:19:10 +0100, Jelte Fennema wrote:

Maybe I'm not understanding your issue correctly, but for such
a case you could push two commits at the same time.

Right.

Apart from that "git diff -w" will hide any whitespace changes so I'm not I
personally wouldn't consider it important to split such changes across
commits.

I do think it's important. For one, the changes made by pgindent et al aren't
just whitespace ones. But I think it's also important to be able to see the
actual changes made in a patch precisely - lots of spurious whitespace changes
could indicate a problem.

Greetings,

Andres Freund

Hi,

On 2023-01-22 18:20:49 +0100, Jelte Fennema wrote:

But switching away from that intermixed with a lot of other changes isn't going to be fun.

I don't think the amount of pain is really much lower if we reformat
10,000 or 300,000 lines of code, without automation both would be
quite painful. But the git commands I shared in my previous email
should alleviate most of that pain.

It's practically not possible to review a 300k line change. And perhaps I'm
paranoid, but I would have a problem with a commit in the history that's
practically not reviewable.

I don't have a problem with the current pgindent alignment of function
parameters, so not sure what you mean about that.

Function parameter alignment is fine with pgindent imho, but this +12
column variable declaration thing I personally think is quite weird.

I strongly dislike it, I rarely get it right by hand - but it does have some
benefit over aligning variable names based on the length of the type names as
uncrustify/clang-format: In their approach an added local variable can cause
all the other variables to be re-indented (and their initial value possibly
wrapped). The fixed alignment doesn't have that issue.

Personally I think the cost of trying to align local variable names is way way
higher than the benefit.

Greetings,

Andres Freund

Andres Freund <andres@anarazel.de> writes:

I strongly dislike it, I rarely get it right by hand - but it does have some
benefit over aligning variable names based on the length of the type names as
uncrustify/clang-format: In their approach an added local variable can cause
all the other variables to be re-indented (and their initial value possibly
wrapped). The fixed alignment doesn't have that issue.

Yeah. That's one of my biggest gripes about pgperltidy: if you insert
another assignment in a series of assignments, it is very likely to
reformat all the adjacent assignments because it thinks it's cool to
make all the equal signs line up. That's just awful. You can either
run pgperltidy on new code before committing, and accept that the feature
patch will touch a lot of lines it's not making real changes to (thereby
dirtying the "git blame" history) or not do so and thereby commit code
that's not passing tidiness checks. Let's *not* adopt any style that
causes similar things to start happening in our C code.

regards, tom lane

Jelte Fennema <postgres@jeltef.nl> writes:

Maybe I'm not understanding your issue correctly, but for such
a case you could push two commits at the same time.

I don't know that much about git commit hooks, but do they really
only check the final state of a series of commits?

In any case, I'm still down on the idea of checking this in a
commit hook because of the complexity and lack of transparency
of such a check. If you think your commit is correctly indented,
but the hook (running on somebody else's machine) disagrees,
how are you going to debug that? I don't want to get into such
a situation, especially since Murphy's law guarantees that it
would mainly bite people under time pressure, like when pushing
a security fix.

regards, tom lane

Andres Freund <andres@anarazel.de> writes:

On 2023-01-22 18:20:49 +0100, Jelte Fennema wrote:

I don't think the amount of pain is really much lower if we reformat
10,000 or 300,000 lines of code, without automation both would be
quite painful. But the git commands I shared in my previous email
should alleviate most of that pain.

It's practically not possible to review a 300k line change. And perhaps I'm
paranoid, but I would have a problem with a commit in the history that's
practically not reviewable.

As far as that goes, if you had concern then you could run the indentation
tool locally and confirm you got matching results. But this does point up
that the processes Jelte suggested all depend critically on indentation
results being 100% reproducible by anybody.

So the more I think about it the less excited I am about depending on
clang-format, because version skew in peoples' clang installations seems
inevitable, and there's good reason to fear that that would show up
as varying indentation results.

regards, tom lane

On Sun, Jan 22, 2023 at 3:28 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

So the more I think about it the less excited I am about depending on
clang-format, because version skew in peoples' clang installations seems
inevitable, and there's good reason to fear that that would show up
as varying indentation results.

I have to admit that the way that I was thinking about this was
colored by the way that I use clang-format today. I only now realize
how different my requirements are to the requirements that we'd have
for any tool that gets run against the tree in bulk. In particular, I
didn't realize how annoying the non-additive nature of certain
variable alignment rules might be until you pointed it out today
(seems obvious now!).

In my experience clang-format really shines when you need to quickly
indent code that is indented in some way that looks completely wrong
-- it does quite a lot better than pgindent when that's your starting
point. It has a reasonable way of balancing competing goals like
maximum number of columns (a soft maximum) and how function parameters
are displayed, which pgindent can't do. It also avoids allowing a
function parameter from a function declaration with its type name on
its own line, before the variable name -- also beyond the capabilities
of pgindent IIRC.

Features like that make it very useful as a first pass thing, where
all the bells and whistles have little real downside. Running
clang-format and then running pgindent tends to produce better results
than just running pgindent, at least when working on a new patch.

--
Peter Geoghegan

Hi,

On 2023-01-22 18:28:27 -0500, Tom Lane wrote:

Andres Freund <andres@anarazel.de> writes:

On 2023-01-22 18:20:49 +0100, Jelte Fennema wrote:

I don't think the amount of pain is really much lower if we reformat
10,000 or 300,000 lines of code, without automation both would be
quite painful. But the git commands I shared in my previous email
should alleviate most of that pain.

It's practically not possible to review a 300k line change. And perhaps I'm
paranoid, but I would have a problem with a commit in the history that's
practically not reviewable.

As far as that goes, if you had concern then you could run the indentation
tool locally and confirm you got matching results.

Of course, but I somehow feel a change of formatting should be reviewable to
at least some degree. Even if it's just to make sure that the tool didn't have
a bug and cause some subtle behavioural change.

So the more I think about it the less excited I am about depending on
clang-format, because version skew in peoples' clang installations seems
inevitable, and there's good reason to fear that that would show up
as varying indentation results.

One thing that I like about clang-format is that it's possible to treat it
about our include order rules (which does find some "irregularities). But of
course that's not enough.

If we decide to move to another tool, I think it might be worth to remove a
few of the pg_bsd_indent options, that other tools won't be able to emulate,
first. E.g. -di12 -> -di4 would remove a *lot* of the noise from a move to
another tool. And be much easier to write manually, but ... :)

I think I've proposed this before, but I still think that as long as we rely
on pg_bsd_indent, we should have it be part of our source tree and
automatically built. It's no wonder that barely anybody indents their
patches, given that it requires building pg_bsd_ident in a separate repo (but
referencing our sourc etree), putting the binary in path, etc.

Greetings,

Andres Freund

Andres Freund <andres@anarazel.de> writes:

I think I've proposed this before, but I still think that as long as we rely
on pg_bsd_indent, we should have it be part of our source tree and
automatically built. It's no wonder that barely anybody indents their
patches, given that it requires building pg_bsd_ident in a separate repo (but
referencing our sourc etree), putting the binary in path, etc.

Hmm ... right offhand, the only objection I can see is that the
pg_bsd_indent files use the BSD 4-clause license, which is not ours.
However, didn't UCB grant a blanket exception years ago that said
that people could treat that as the 3-clause license? If we could
get past the license question, I agree that doing what you suggest
would be superior to the current situation.

regards, tom lane

On Sun, Jan 22, 2023 at 07:28:42PM -0500, Tom Lane wrote:

Andres Freund <andres@anarazel.de> writes:

I think I've proposed this before, but I still think that as long as we rely
on pg_bsd_indent, we should have it be part of our source tree and
automatically built. It's no wonder that barely anybody indents their
patches, given that it requires building pg_bsd_ident in a separate repo (but
referencing our sourc etree), putting the binary in path, etc.

Hmm ... right offhand, the only objection I can see is that the
pg_bsd_indent files use the BSD 4-clause license, which is not ours.
However, didn't UCB grant a blanket exception years ago that said
that people could treat that as the 3-clause license? If we could
get past the license question, I agree that doing what you suggest
would be superior to the current situation.

+1.
--
Michael

On 2023-01-22 Su 18:14, Tom Lane wrote:

Jelte Fennema <postgres@jeltef.nl> writes:

Maybe I'm not understanding your issue correctly, but for such
a case you could push two commits at the same time.

I don't know that much about git commit hooks, but do they really
only check the final state of a series of commits?

The pre-commit hook is literally run every time you do `git commit`. But
it's only run on your local instance and only if you have enabled it.
It's not project-wide.

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Hi,

On 2023-01-22 19:50:10 -0500, Andrew Dunstan wrote:

On 2023-01-22 Su 18:14, Tom Lane wrote:

Jelte Fennema <postgres@jeltef.nl> writes:

Maybe I'm not understanding your issue correctly, but for such
a case you could push two commits at the same time.

I don't know that much about git commit hooks, but do they really
only check the final state of a series of commits?

The pre-commit hook is literally run every time you do `git commit`. But
it's only run on your local instance and only if you have enabled it.
It's not project-wide.

There's different hooks. Locally, I think pre-push would be better suited to
this than pre-commit (I often save WIP work in local branches, it'd be pretty
annoying if some indentation thing swore at me).

But there's also hooks like pre-receive, that allow doing validation on the
server side. Which obviously would be project wide...

Greetings,

Andres Freund

Hi,

On 2023-01-22 19:28:42 -0500, Tom Lane wrote:

Andres Freund <andres@anarazel.de> writes:

I think I've proposed this before, but I still think that as long as we rely
on pg_bsd_indent, we should have it be part of our source tree and
automatically built. It's no wonder that barely anybody indents their
patches, given that it requires building pg_bsd_ident in a separate repo (but
referencing our sourc etree), putting the binary in path, etc.

Hmm ... right offhand, the only objection I can see is that the
pg_bsd_indent files use the BSD 4-clause license, which is not ours.
However, didn't UCB grant a blanket exception years ago that said
that people could treat that as the 3-clause license?

Yep:
https://www.freebsd.org/copyright/license/

NOTE: The copyright of UC Berkeley’s Berkeley Software Distribution ("BSD") source has been updated. The copyright addendum may be found at ftp://ftp.cs.berkeley.edu/pub/4bsd/README.Impt.License.Change and is included below.

July 22, 1999

To All Licensees, Distributors of Any Version of BSD:

As you know, certain of the Berkeley Software Distribution ("BSD") source code files require that further distributions of products containing all or portions of the software, acknowledge within their advertising materials that such products contain software developed by UC Berkeley and its contributors.

Specifically, the provision reads:

* 3. All advertising materials mentioning features or use of this software
* must display the following acknowledgement:
* This product includes software developed by the University of
* California, Berkeley and its contributors."

Effective immediately, licensees and distributors are no longer required to include the acknowledgement within advertising materials. Accordingly, the foregoing paragraph of those BSD Unix files containing it is hereby deleted in its entirety.

William Hoskins
Director, Office of Technology Licensing
University of California, Berkeley

I did check, and the FTP bit is still downloadable. A bit awkward though, now
that browsers have/are removing ftp support.

Greetings,

Andres Freund

Andres Freund <andres@anarazel.de> writes:

On 2023-01-22 19:28:42 -0500, Tom Lane wrote:

Hmm ... right offhand, the only objection I can see is that the
pg_bsd_indent files use the BSD 4-clause license, which is not ours.
However, didn't UCB grant a blanket exception years ago that said
that people could treat that as the 3-clause license?

Yep:
https://www.freebsd.org/copyright/license/

Cool. I'll take a look at doing this later (probably after the current
CF) unless somebody beats me to it.

regards, tom lane

I whipped up a pre-commit hook which automatically runs pgindent on the
changed files in the commit. It won't add any changes automatically, but
instead it fails the commit if it made any changes. That way you can add
them manually if you want. Or if you don't, you can simply run git commit
again without adding the changes. (or you can use the --no-verify flag of
git commit to skip the hook completely)

It did require adding some extra flags to pgindent. While it only required
the --staged-only and --fail-on-changed flags, the --changed-only flag
was easy to add and seemed generally useful.

I also attached a patch which adds the rules for formatting pgindent
itself to the .editorconfig file.

Locally, I think pre-push would be better suited to
this than pre-commit (I often save WIP work in local branches, it'd be pretty
annoying if some indentation thing swore at me).

I personally prefer pre-commit hooks, since then I don't have to
go back and change some commit I made some time ago. And I
think with the easy opt-out that this hook has it would work fine
for my own workflow. But it shouldn't be hard to also include a
pre-push hook too if you want that after all. Then people can
choose to install the hook that they prefer.

On 2023-01-22 Su 20:03, Andres Freund wrote:

Hi,

On 2023-01-22 19:50:10 -0500, Andrew Dunstan wrote:

On 2023-01-22 Su 18:14, Tom Lane wrote:

Jelte Fennema <postgres@jeltef.nl> writes:

Maybe I'm not understanding your issue correctly, but for such
a case you could push two commits at the same time.

I don't know that much about git commit hooks, but do they really
only check the final state of a series of commits?

The pre-commit hook is literally run every time you do `git commit`. But
it's only run on your local instance and only if you have enabled it.
It's not project-wide.

There's different hooks. Locally, I think pre-push would be better suited to
this than pre-commit (I often save WIP work in local branches, it'd be pretty
annoying if some indentation thing swore at me).

Yes, me too, so I currently have a filter in my hook that ignores local
WIP branches. The problem with pre-push is that by the time you're
pushing you have already committed and you would have to go back and
undo some stuff to fix it. Probably 99 times out of 100 I'd prefer to
commit indented code off the bat rather than make a separate indentation
commit. But this really illustrates my point: how you do it is up to you.

But there's also hooks like pre-receive, that allow doing validation on the
server side. Which obviously would be project wide...

Yes, but I think it's been demonstrated (again) that there's no
consensus in using those for this purpose.

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

On 2023-01-22 Su 11:18, Tom Lane wrote:

Andrew Dunstan <andrew@dunslane.net> writes:

... btw, can we get away with making the diff run be "diff -upd"
not just "diff -u"? I find diff output for C files noticeably
more useful with those options, but I'm unsure about their
portability.

I think they are available on Linux, MacOS and FBSD, and on Windows (if
anyone's actually using it for this) it's likely to be Gnu diff. So I
think that's probably enough coverage.

I checked NetBSD as well, and it has all three too.

Patch looks good to me.

Thanks, pushed.

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

On 2023-01-23 Mo 05:44, Jelte Fennema wrote:

I whipped up a pre-commit hook which automatically runs pgindent on the
changed files in the commit. It won't add any changes automatically, but
instead it fails the commit if it made any changes. That way you can add
them manually if you want. Or if you don't, you can simply run git commit
again without adding the changes. (or you can use the --no-verify flag of
git commit to skip the hook completely)

It did require adding some extra flags to pgindent. While it only required
the --staged-only and --fail-on-changed flags, the --changed-only flag
was easy to add and seemed generally useful.

Please see the changes to pgindent I committed about the same time I got
your email. I don't think we need your new flags, as it's possible (and
always has been) to provide pgindent with a list of files to be
indented. Instead of having pgindent run `git diff --name-only ...` the
git hook can do it and pass the results to pgindent in its command line.

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Indeed the flags you added are enough. Attached is a patch
that adds an updated pre-commit hook with the same behaviour
as the one before. I definitely think having a pre-commit hook
in the repo is beneficial, since writing one that works in all
cases definitely takes some time.

as it's possible (and
always has been) to provide pgindent with a list of files to be
indented.

I guess I didn't realise this was a feature that existed, because
none of the documentation mentioned it.

I wrote:

Cool. I'll take a look at doing this later (probably after the current
CF) unless somebody beats me to it.

Thinking about that (importing pg_bsd_indent into our main source
tree) a bit harder:

1. I'd originally thought vaguely that we could teach pgindent
how to build pg_bsd_indent automatically. But with a little
more consideration, I doubt that would work transparently.
It's common (at least for me) to run pgindent in a distclean'd
tree, where configure results wouldn't be available. It's even
worse if you habitually use VPATH builds, so that those files
*never* exist in your source tree. So now I think that we should
stick to the convention that it's on the user to install
pg_bsd_indent somewhere in their PATH; all we'll be doing with
this change is eliminating the step of fetching pg_bsd_indent's
source files from somewhere else.

2. Given #1, it'll be prudent to continue having pgindent
double-check that pg_bsd_indent reports a specific version
number. We could imagine starting to use the main Postgres
version number for that, but I'm inclined to continue with
its existing numbering series. One argument for that is
that we generally change pg_bsd_indent less often than annually,
so having it track the main version would end up forcing
make-work builds of your installed pg_bsd_indent at least
once a year. Also, when we do change pg_bsd_indent, it's
typically right before a mass reindentation commit, and those
do not happen at the same time as forking a new PG version.

3. If we do nothing special, the first mass reindentation is
going to reformat the pg_bsd_indent sources per PG style,
which is ... er ... not the way they look now. Do we want
to accept that outcome, or take steps to prevent pgindent
from processing pg_bsd_indent? I have a feeling that manual
cleanup would be necessary if we let such reindentation
happen, but I haven't experimented.

regards, tom lane

Hi,

On 2023-01-23 10:09:06 -0500, Tom Lane wrote:

1. I'd originally thought vaguely that we could teach pgindent
how to build pg_bsd_indent automatically. But with a little
more consideration, I doubt that would work transparently.
It's common (at least for me) to run pgindent in a distclean'd
tree, where configure results wouldn't be available. It's even
worse if you habitually use VPATH builds, so that those files
*never* exist in your source tree. So now I think that we should
stick to the convention that it's on the user to install
pg_bsd_indent somewhere in their PATH; all we'll be doing with
this change is eliminating the step of fetching pg_bsd_indent's
source files from somewhere else.

I think it'd be better to build pg_bsd_indent automatically as you planned
earlier - most others don't run pgindent from a distcleaned source tree. And
it shouldn't be hard to teach pgindent to run from a vpath build directory.

I'd like to get to the point where we can have simple build target for
a) re-indenting the whole tree
b) re-indenting the files touched in changes compared to master

If we add that to the list of things to do before sending a patch upstream,
we're a heck of a lot more likely to get decently formatted patches compared
to today.

As long as we need typedefs.list, I think it'd be good for such a target to
add new typedefs found in the local build to typedefs.list (but *not* remove
old ones, due to platform dependent code). But that's a separate enough
topic...

2. Given #1, it'll be prudent to continue having pgindent
double-check that pg_bsd_indent reports a specific version
number.

+1

3. If we do nothing special, the first mass reindentation is
going to reformat the pg_bsd_indent sources per PG style,
which is ... er ... not the way they look now. Do we want
to accept that outcome, or take steps to prevent pgindent
from processing pg_bsd_indent? I have a feeling that manual
cleanup would be necessary if we let such reindentation
happen, but I haven't experimented.

I think we should exempt it, initially at least. If somebody decides to invest
a substantial amount of time in pgindent, let's change it, but I'm somewhat
doubtful that'll happen anytime soon.

Greetings,

Andres Freund

On 2023-Jan-23, Tom Lane wrote:

1. [...] So now I think that we should
stick to the convention that it's on the user to install
pg_bsd_indent somewhere in their PATH; all we'll be doing with
this change is eliminating the step of fetching pg_bsd_indent's
source files from somewhere else.

+1

2. Given #1, it'll be prudent to continue having pgindent
double-check that pg_bsd_indent reports a specific version
number. We could imagine starting to use the main Postgres
version number for that, but I'm inclined to continue with
its existing numbering series.

+1

3. If we do nothing special, the first mass reindentation is
going to reformat the pg_bsd_indent sources per PG style,
which is ... er ... not the way they look now. Do we want
to accept that outcome, or take steps to prevent pgindent
from processing pg_bsd_indent? I have a feeling that manual
cleanup would be necessary if we let such reindentation
happen, but I haven't experimented.

Hmm, initially it must just be easier to have an exception so that
pg_bsd_indent itself isn't indented.

--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
#error "Operator lives in the wrong universe"
("Use of cookies in real-time system development", M. Gleixner, M. Mc Guire)

On 2023-01-23 Mo 09:49, Jelte Fennema wrote:

Attached is a patch
that adds an updated pre-commit hook with the same behaviour
as the one before. I definitely think having a pre-commit hook
in the repo is beneficial, since writing one that works in all
cases definitely takes some time.

Not sure if this should go in the git repo or in the developer wiki.

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Hi!

I've ran pdindent on the whole Postgres and it'd changed
an awful lot of source files. Won't it create a lot of merge conflicts?

On Mon, Jan 23, 2023 at 8:48 PM Alvaro Herrera <alvherre@alvh.no-ip.org>
wrote:

On 2023-Jan-23, Tom Lane wrote:

1. [...] So now I think that we should
stick to the convention that it's on the user to install
pg_bsd_indent somewhere in their PATH; all we'll be doing with
this change is eliminating the step of fetching pg_bsd_indent's
source files from somewhere else.

+1

2. Given #1, it'll be prudent to continue having pgindent
double-check that pg_bsd_indent reports a specific version
number. We could imagine starting to use the main Postgres
version number for that, but I'm inclined to continue with
its existing numbering series.

+1

3. If we do nothing special, the first mass reindentation is
going to reformat the pg_bsd_indent sources per PG style,
which is ... er ... not the way they look now. Do we want
to accept that outcome, or take steps to prevent pgindent
from processing pg_bsd_indent? I have a feeling that manual
cleanup would be necessary if we let such reindentation
happen, but I haven't experimented.

Hmm, initially it must just be easier to have an exception so that
pg_bsd_indent itself isn't indented.

--
Álvaro Herrera 48°01'N 7°57'E —
https://www.EnterpriseDB.com/
#error <https://www.EnterpriseDB.com/#error&gt; "Operator lives in the wrong
universe"
("Use of cookies in real-time system development", M. Gleixner, M. Mc
Guire)

--
Regards,
Nikita Malakhov
Postgres Professional
https://postgrespro.ru/

Nikita Malakhov <hukutoc@gmail.com> writes:

I've ran pdindent on the whole Postgres and it'd changed
an awful lot of source files. Won't it create a lot of merge conflicts?

Well, yeah, you've rediscovered the fact that a lot of commits are sloppy
about this, and it's been awhile since our last tree-wide pgindent.

Our normal process has been to do a cleanup pgindent run annually or so,
usually after the end of the last commitfest of a cycle when there's
plenty of time for people to deal with the ensuing merge conflicts.

If we could get to "commits are pgindent clean to begin with", we
could avoid the merge problems from one-big-reindent. I'd still
be inclined to do an annual run as a backup, but hopefully it would
find few problems.

regards, tom lane

Not sure if this should go in the git repo or in the developer wiki.

I would say the git repo is currently the most fitting place, since it
has all the existing docs for pgindent. The wiki even links to the
pgindent source directory:
https://wiki.postgresql.org/wiki/Developer_FAQ#What.27s_the_formatting_style_used_in_PostgreSQL_source_code.3F

On Mon, Jan 23, 2023 at 09:31:36AM -0800, Andres Freund wrote:

As long as we need typedefs.list, I think it'd be good for such a target to
add new typedefs found in the local build to typedefs.list (but *not* remove
old ones, due to platform dependent code). But that's a separate enough
topic...

One issue on requiring patches to have run pgindent previously is
actually the typedef list. If someone adds a typedef in a commit, they
will see different pgident output in the committed files, and perhaps
others, and the new typedefs might only appear after the commit, causing
later commits to not match.

--
Bruce Momjian <bruce@momjian.us> https://momjian.us
EDB https://enterprisedb.com

Embrace your flaws. They make you human, rather than perfect,
which you will never be.

One issue on requiring patches to have run pgindent previously is
actually the typedef list. If someone adds a typedef in a commit, they
will see different pgident output in the committed files, and perhaps
others, and the new typedefs might only appear after the commit, causing
later commits to not match.

I'm not sure I understand the issue you're pointing out. If someone
changes the typedef list, imho they want the formatting to change
because of that. So requiring an addition to the typedef list to also
commit reindentation to all files that this typedef indirectly impacts
seems pretty reasonable to me.

Jelte Fennema <postgres@jeltef.nl> writes:

One issue on requiring patches to have run pgindent previously is
actually the typedef list. If someone adds a typedef in a commit, they
will see different pgident output in the committed files, and perhaps
others, and the new typedefs might only appear after the commit, causing
later commits to not match.

I'm not sure I understand the issue you're pointing out. If someone
changes the typedef list, imho they want the formatting to change
because of that. So requiring an addition to the typedef list to also
commit reindentation to all files that this typedef indirectly impacts
seems pretty reasonable to me.

I think the issue Bruce is pointing out is that this is another mechanism
whereby different people could get different indentation results.
I fear any policy that is based on an assumption that indentation has
One True Result is going to fail.

As a concrete example, suppose Alice commits some code that uses "foo"
as a variable name, and more or less concurrently, Bob commits something
that defines "foo" as a typedef. Bob's change is likely to have
side-effects on the formatting of Alice's code. If they're working in
well-separated parts of the source tree, nobody is likely to notice
that for awhile --- but whoever next touches the files Alice touched
will be in for a surprise, which will be more or less painful depending
on whether we've installed brittle processes.

As another example, the mechanisms we use to create the typedefs list
in the first place are pretty squishy/leaky: they depend on which
buildfarm animals are running the typedef-generation step, and on
whether anything's broken lately in that code --- which happens on
a fairly regular basis (eg [1]https://github.com/PGBuildFarm/client-code/commit/dcca861554e90d6395c3c153317b0b0e3841f103). Maybe that could be improved,
but I don't see an easy way to capture the set of system-defined
typedefs that are in use on platforms other than your own. I
definitely do not want to go over to hand maintenance of that list.

I think we need to be content with a "soft", it's more-or-less-right
approach to indentation. As I explained to somebody upthread, the
main benefit of this for most people is avoiding the need for a massive
once-a-year reindent run that causes merge failures for many pending
patches. But we don't need to completely eliminate such runs to get
99.9% of that benefit; we only need to reduce the number of places
they're likely to touch.

regards, tom lane

[1]: https://github.com/PGBuildFarm/client-code/commit/dcca861554e90d6395c3c153317b0b0e3841f103

Andres> Of course, but I somehow feel a change of formatting should be
reviewable to
Andres> at least some degree

One way of reviewing the formatting changes is to compare the compiled
binaries.

If the binaries before and after formatting are the same, then there's a
high chance the behaviour is the same.

Vladimir

As a concrete example, suppose Alice commits some code that uses "foo"
as a variable name, and more or less concurrently, Bob commits something
that defines "foo" as a typedef. Bob's change is likely to have
side-effects on the formatting of Alice's code. If they're working in
well-separated parts of the source tree, nobody is likely to notice
that for awhile --- but whoever next touches the files Alice touched
will be in for a surprise, which will be more or less painful depending
on whether we've installed brittle processes.

Sounds like this conflict could be handled fairly easily by
having a local git hook rerunning pgindent whenever
you rebase a commit:
1. if you changed typedefs.list the hook would format all files
2. if you didn't it only formats the files that you changed

As another example, the mechanisms we use to create the typedefs list
in the first place are pretty squishy/leaky: they depend on which
buildfarm animals are running the typedef-generation step, and on
whether anything's broken lately in that code --- which happens on
a fairly regular basis (eg [1]). Maybe that could be improved,
but I don't see an easy way to capture the set of system-defined
typedefs that are in use on platforms other than your own. I
definitely do not want to go over to hand maintenance of that list.

Wouldn't the automatic addition-only solution that Andres suggested
solve this issue? Build farms could still remove unused typedefs
on a regular basis, but commits would at least add typedefs for the
platform that the comitter uses.

I think we need to be content with a "soft", it's more-or-less-right
approach to indentation.

I think that this would already be a significant improvement over the
current situation. My experience with the current situation is that
indentation is more-or-less-wrong.

As I explained to somebody upthread, the
main benefit of this for most people is avoiding the need for a massive
once-a-year reindent run that causes merge failures for many pending
patches.

Merge failures are one issue. But personally the main benefit that
I would be getting is being able to run pgindent on the files
I'm editing and get this weird +12 columns formatting correct
without having to manually type it. Without pgindent also
changing random parts of the files that someone else touched
a few commits before me.

Jelte Fennema <postgres@jeltef.nl> writes:

Sounds like this conflict could be handled fairly easily by
having a local git hook rerunning pgindent whenever
you rebase a commit:
1. if you changed typedefs.list the hook would format all files
2. if you didn't it only formats the files that you changed

I think that would be undesirable, because then reindentation noise
in completely-unrelated files would get baked into feature commits,
complicating review and messing up "git blame" history.
The approach we currently have allows reindent effects to be
separated into ignorable labeled commits, which is a nice property.

Merge failures are one issue. But personally the main benefit that
I would be getting is being able to run pgindent on the files
I'm editing and get this weird +12 columns formatting correct
without having to manually type it. Without pgindent also
changing random parts of the files that someone else touched
a few commits before me.

Yeah, that always annoys me too, but I've always considered that
it's my problem not something I can externalize onto other people.
The real bottom line here is that AFAICT, there are fewer committers
who care about indent cleanliness than committers who do not, so
I do not think that the former group get to impose strict rules
on the latter, much as I might wish otherwise.

FWIW, Andrew's recent --show-diff feature for pgindent has
already improved my workflow for that. I can do
"pgindent --show-diff >fixindent.patch", manually remove any hunks
in fixindent.patch that don't pertain to the code I'm working on,
and apply what remains to fix up my new code. (I had been doing
something basically like this, but with more file-copying steps
to undo pgindent's edit-in-place behavior.)

regards, tom lane

On 2023-01-24 Tu 11:43, Tom Lane wrote:

Jelte Fennema <postgres@jeltef.nl> writes:

Sounds like this conflict could be handled fairly easily by
having a local git hook rerunning pgindent whenever
you rebase a commit:
1. if you changed typedefs.list the hook would format all files
2. if you didn't it only formats the files that you changed

I think that would be undesirable, because then reindentation noise
in completely-unrelated files would get baked into feature commits,
complicating review and messing up "git blame" history.
The approach we currently have allows reindent effects to be
separated into ignorable labeled commits, which is a nice property.

Merge failures are one issue. But personally the main benefit that
I would be getting is being able to run pgindent on the files
I'm editing and get this weird +12 columns formatting correct
without having to manually type it. Without pgindent also
changing random parts of the files that someone else touched
a few commits before me.

Yeah, that always annoys me too, but I've always considered that
it's my problem not something I can externalize onto other people.
The real bottom line here is that AFAICT, there are fewer committers
who care about indent cleanliness than committers who do not, so
I do not think that the former group get to impose strict rules
on the latter, much as I might wish otherwise.

FWIW, Andrew's recent --show-diff feature for pgindent has
already improved my workflow for that. I can do
"pgindent --show-diff >fixindent.patch", manually remove any hunks
in fixindent.patch that don't pertain to the code I'm working on,
and apply what remains to fix up my new code. (I had been doing
something basically like this, but with more file-copying steps
to undo pgindent's edit-in-place behavior.)

I'm glad it's helpful.

Here's another improvement I think will be useful when the new gadgets
are used in a git hook: first, look for the excludes file under the
current directory if we aren't setting $code_base (e.g if we have files
given on the command line), and second apply the exclude patterns to the
command line files as well as to files found using File::Find.

I propose to apply this fairly soon.

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

I think that would be undesirable, because then reindentation noise
in completely-unrelated files would get baked into feature commits,
complicating review and messing up "git blame" history.

With a rebase hook similar to the the pre-commit hook that I shared
upthread, your files will be changed accordingly, but you don't need
to commit those changes in the same commit as the one that you're
rebasing. You could append another commit after it. Another option
would be to move the typedefs.list change to a separate commit
together with all project wide indentation changes.

The real bottom line here is that AFAICT, there are fewer committers
who care about indent cleanliness than committers who do not, so
I do not think that the former group get to impose strict rules
on the latter, much as I might wish otherwise.

Is this actually the case? I haven't seen anyone in this thread say
they don't care. From my perspective it seems like the unclean
indents simply come from forgetting to run pgindent once in
a while. And those few forgetful moments add up over a year.
of commits. That's why to me tooling seems the answer here.
If the tooling makes it easy not to forget then the problem
goes away.

FWIW, Andrew's recent --show-diff feature for pgindent has
already improved my workflow for that. I can do
"pgindent --show-diff >fixindent.patch", manually remove any hunks
in fixindent.patch that don't pertain to the code I'm working on,
and apply what remains to fix up my new code. (I had been doing
something basically like this, but with more file-copying steps
to undo pgindent's edit-in-place behavior.)

Yeah, I have a similar workflow with the pre-commit hook that
I shared. By using "git checkout -p" I can remove hunks that
don't pertain to my code. Still it would be really nice not
to have to go through that effort (which is significant for the
libpq code that I've been workin on, since there's ~50
incorrectly indented hunks).

Here's another improvement I think will be useful when the new gadgets
are used in a git hook: first, look for the excludes file under the
current directory if we aren't setting $code_base (e.g if we have files
given on the command line), and second apply the exclude patterns to the
command line files as well as to files found using File::Find.

Change looks good to me.

On Tue, Jan 24, 2023 at 09:54:57AM -0500, Tom Lane wrote:

As another example, the mechanisms we use to create the typedefs list
in the first place are pretty squishy/leaky: they depend on which
buildfarm animals are running the typedef-generation step, and on
whether anything's broken lately in that code --- which happens on
a fairly regular basis (eg [1]). Maybe that could be improved,
but I don't see an easy way to capture the set of system-defined
typedefs that are in use on platforms other than your own. I
definitely do not want to go over to hand maintenance of that list.

As I now understand it, we would need to standardize on a typedef list
at the beginning of each major development cycle, and then only allow
additions, and the addition would have to include any pgindent affects
of the addition.

--
Bruce Momjian <bruce@momjian.us> https://momjian.us
EDB https://enterprisedb.com

Embrace your flaws. They make you human, rather than perfect,
which you will never be.

On 2023-01-24 Tu 13:42, Jelte Fennema wrote:

Here's another improvement I think will be useful when the new gadgets
are used in a git hook: first, look for the excludes file under the
current directory if we aren't setting $code_base (e.g if we have files
given on the command line), and second apply the exclude patterns to the
command line files as well as to files found using File::Find.

Change looks good to me.

Thanks, pushed

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

On 2023-01-23 Mo 09:49, Jelte Fennema wrote:

Indeed the flags you added are enough. Attached is a patch
that adds an updated pre-commit hook with the same behaviour
as the one before. I definitely think having a pre-commit hook
in the repo is beneficial, since writing one that works in all
cases definitely takes some time.

I didn't really like your hook, as it forces a reindent, and many people
won't want that (for reasons given elsewhere in this thread).

Here's an extract from my pre-commit hook that does that if PGAUTOINDENT
is set to "yes", and otherwise just warns you that you need to run pgindent.

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

On Thu, 26 Jan 2023 at 15:40, Andrew Dunstan <andrew@dunslane.net> wrote:

I didn't really like your hook, as it forces a reindent, and many people
won't want that (for reasons given elsewhere in this thread).

I'm not sure what you mean by "forces a reindent". Like I explained
you can simply run "git commit" again to ignore the changes and
commit anyway. As long as the files are indented on your filesystem
the hook doesn't care if you actually included the indentation changes
in the changes that you're currently committing.

So to be completely clear you can do the following with my hook:
git commit # runs pgindent and fails
git commit # commits changes anyway
git commit -am 'Run pgindent' # commit indentation changes separately

Or what I usually do:
git commit # runs pgindent and fails
git add --patch # choose relevant changes to add to commit
git commit # commit the changes
git checkout -- . # undo irrelevant changes on filesystem

Honestly PGAUTOINDENT=no seems stricter, since the only
way to bypass the failure is now to run manually run pgindent
or git commit with the --no-verify flag.

files=$(git diff --cached --name-only --diff-filter=ACMR)
src/tools/pgindent/pgindent $files

That seems like it would fail if there's any files or directories with
spaces in them. Maybe this isn't something we care about though.

# no need to filter files - pgindent ignores everything that isn't a
# .c or .h file

If the first argument is a non .c or .h file, then pgindent interprets
it as the typedefs file. So it's definitely important to filter non .c
and .h files out. Because now if you commit a single
non .c or .h file this hook messes up the indentation in all of
your files. You can reproduce by running:
src/tools/pgindent/pgindent README

# only do this on master
test "$branch" = "master" || return 0

I would definitely want a way to disable this check. As a normal
submitter I never work directly on master.

On 2023-01-26 Th 11:16, Jelte Fennema wrote:

On Thu, 26 Jan 2023 at 15:40, Andrew Dunstan <andrew@dunslane.net> wrote:

I didn't really like your hook, as it forces a reindent, and many people
won't want that (for reasons given elsewhere in this thread).

I'm not sure what you mean by "forces a reindent". Like I explained
you can simply run "git commit" again to ignore the changes and
commit anyway. As long as the files are indented on your filesystem
the hook doesn't care if you actually included the indentation changes
in the changes that you're currently committing.

Your hook does this:

+git diff --cached --name-only --diff-filter=ACMR | grep '\.[ch]$' |\
+    xargs src/tools/pgindent/pgindent --silent-diff \
+    || {
+        echo ERROR: Aborting commit because pgindent was not run
+        git diff --cached --name-only --diff-filter=ACMR | grep
'\.[ch]$' | xargs src/tools/pgindent/pgindent
+        exit 1
+    }

At this stage the files are now indented, so if it failed and you run
`git commit` again it will commit with the indention changes.

So to be completely clear you can do the following with my hook:
git commit # runs pgindent and fails
git commit # commits changes anyway
git commit -am 'Run pgindent' # commit indentation changes separately

Or what I usually do:
git commit # runs pgindent and fails
git add --patch # choose relevant changes to add to commit
git commit # commit the changes
git checkout -- . # undo irrelevant changes on filesystem

Honestly PGAUTOINDENT=no seems stricter, since the only
way to bypass the failure is now to run manually run pgindent
or git commit with the --no-verify flag.

files=$(git diff --cached --name-only --diff-filter=ACMR)
src/tools/pgindent/pgindent $files

That seems like it would fail if there's any files or directories with
spaces in them. Maybe this isn't something we care about though.

We don't have any, and the filenames git produces are relative to the
git root. I don't think this is an issue.

# no need to filter files - pgindent ignores everything that isn't a
# .c or .h file

If the first argument is a non .c or .h file, then pgindent interprets
it as the typedefs file. So it's definitely important to filter non .c
and .h files out. Because now if you commit a single
non .c or .h file this hook messes up the indentation in all of
your files. You can reproduce by running:
src/tools/pgindent/pgindent README

I have a patch at [1]  /messages/by-id/21bb8573-9e56-812b-84cf-1e4f3c4c2a7b@dunslane.net to remove this misfeature.

# only do this on master
test "$branch" = "master" || return 0

I would definitely want a way to disable this check. As a normal
submitter I never work directly on master.

Sure, that's your choice. My intended audience here is committers, who
of course do work on master.

cheers

andrew

[1]:   /messages/by-id/21bb8573-9e56-812b-84cf-1e4f3c4c2a7b@dunslane.net

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

At this stage the files are now indented, so if it failed and you run
`git commit` again it will commit with the indention changes.

No, because at no point a "git add" is happening, so the changes
made by pgindent are not staged. As long as you don't run the
second "git commit" with the -a flag the commit will be exactly
the same as you prepared it before.

Sure, that's your choice. My intended audience here is committers, who
of course do work on master.

Yes I understand, I meant it would be nice if the script had a environment
variable like PG_COMMIT_HOOK_ALL_BRANCHES (bad name)
for this purpose.

On 2023-01-26 Th 12:05, Jelte Fennema wrote:

At this stage the files are now indented, so if it failed and you run
`git commit` again it will commit with the indention changes.

No, because at no point a "git add" is happening, so the changes
made by pgindent are not staged. As long as you don't run the
second "git commit" with the -a flag the commit will be exactly
the same as you prepared it before.

Hmm, but I usually run with -a, I even have a git alias for it. I guess
what this discussion illustrates is that there are various patters for
using git, and we shouldn't assume that everyone else is using the same
patterns we are.

I'm still  mildly inclined to say this material would be better placed
in the developer wiki. After all, this isn't the only thing a postgres
developer might use a git hook for (mine has more material in it than in
what I posted).

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

On Thu, 26 Jan 2023 at 22:46, Andrew Dunstan <andrew@dunslane.net> wrote:

Hmm, but I usually run with -a, I even have a git alias for it. I guess
what this discussion illustrates is that there are various patters for
using git, and we shouldn't assume that everyone else is using the same
patterns we are.

I definitely agree that there are lots of ways to use git. And I now
understand why my hook didn't work well for your existing workflow.

I've pretty much unlearned the -a flag. Because the easiest way I've
been able to split up changes into different commits is using "git add
-p", which adds partial pieces of files to the staging area. And that
workflow combines terribly with "git commit -a" because -a adds all
the things that I specifically didn't put in the staging area into the
final commit anyway.

I'm still mildly inclined to say this material would be better placed
in the developer wiki. After all, this isn't the only thing a postgres
developer might use a git hook for

I think it should definitely be somewhere. I have a preference for the
repo, since I think the docs on codestyle are already in too many
different places. But the wiki is already much better than having no
shared hook at all. I mainly think we should try to make it as easy as
possible for people to commit well indented code.

(mine has more material in it than in what I posted).

Anything that is useful for the wider community and could be part of
this example/template git hook? (e.g. some perltidy automation)

On 2023-01-26 Th 17:54, Jelte Fennema wrote:

I'm still mildly inclined to say this material would be better placed
in the developer wiki. After all, this isn't the only thing a postgres
developer might use a git hook for

I think it should definitely be somewhere. I have a preference for the
repo, since I think the docs on codestyle are already in too many
different places. But the wiki is already much better than having no
shared hook at all. I mainly think we should try to make it as easy as
possible for people to commit well indented code.

I've added a section to the wiki at
<https://wiki.postgresql.org/wiki/Working_with_Git#Using_git_hooks&gt; and
put a reference to that in the pgindent docco. You can of course add
some more info to the wiki if you feel it's necessary.

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

On Tue, Jan 24, 2023 at 02:04:02PM -0500, Bruce Momjian wrote:

On Tue, Jan 24, 2023 at 09:54:57AM -0500, Tom Lane wrote:

As another example, the mechanisms we use to create the typedefs list
in the first place are pretty squishy/leaky: they depend on which
buildfarm animals are running the typedef-generation step, and on
whether anything's broken lately in that code --- which happens on
a fairly regular basis (eg [1]). Maybe that could be improved,
but I don't see an easy way to capture the set of system-defined
typedefs that are in use on platforms other than your own. I
definitely do not want to go over to hand maintenance of that list.

As I now understand it, we would need to standardize on a typedef list
at the beginning of each major development cycle, and then only allow
additions,

Not to my knowledge. There's no particular obstacle to updating the list more
frequently or removing entries.

and the addition would have to include any pgindent affects
of the addition.

Yes, a hook intended to enforce pgindent cleanliness should run tree-wide
pgindent when the given commit(s) change the typedef list. typedef list
changes essentially become another kind of refactoring that can yield merge
conflicts. If your commit passed the pgindent check, rebasing it onto a new
typedefs list may require further indentation changes. New typedefs don't
tend to change a lot of old code, so I would expect this sort of conflict to
be minor, compared to all the other sources of conflicts.

Noah Misch <noah@leadboat.com> writes:

Yes, a hook intended to enforce pgindent cleanliness should run tree-wide
pgindent when the given commit(s) change the typedef list. typedef list
changes essentially become another kind of refactoring that can yield merge
conflicts. If your commit passed the pgindent check, rebasing it onto a new
typedefs list may require further indentation changes. New typedefs don't
tend to change a lot of old code, so I would expect this sort of conflict to
be minor, compared to all the other sources of conflicts.

In fact, if a typedef addition *does* affect a lot of old code,
that's a good sign that the choice of typedef name ought to be
rethought: it's evidently conflicting with existing names.

I'm not sure what that observation implies for our standard
practices here. But it does suggest that "let pgindent do what
it wants without human oversight" probably isn't a good plan.
We've seen that to be true for other reasons as well, notably that
it can destroy the readability of carefully-laid-out comments.

regards, tom lane

On Sat, Jan 28, 2023 at 05:06:03PM -0800, Noah Misch wrote:

On Tue, Jan 24, 2023 at 02:04:02PM -0500, Bruce Momjian wrote:

On Tue, Jan 24, 2023 at 09:54:57AM -0500, Tom Lane wrote:

As another example, the mechanisms we use to create the typedefs list
in the first place are pretty squishy/leaky: they depend on which
buildfarm animals are running the typedef-generation step, and on
whether anything's broken lately in that code --- which happens on
a fairly regular basis (eg [1]). Maybe that could be improved,
but I don't see an easy way to capture the set of system-defined
typedefs that are in use on platforms other than your own. I
definitely do not want to go over to hand maintenance of that list.

As I now understand it, we would need to standardize on a typedef list
at the beginning of each major development cycle, and then only allow
additions,

Not to my knowledge. There's no particular obstacle to updating the list more
frequently or removing entries.

We would need to re-pgindent the tree each time, I think, which would
cause disruption if we did it too frequently.

and the addition would have to include any pgindent affects
of the addition.

Yes, a hook intended to enforce pgindent cleanliness should run tree-wide
pgindent when the given commit(s) change the typedef list. typedef list
changes essentially become another kind of refactoring that can yield merge
conflicts. If your commit passed the pgindent check, rebasing it onto a new
typedefs list may require further indentation changes. New typedefs don't
tend to change a lot of old code, so I would expect this sort of conflict to
be minor, compared to all the other sources of conflicts.

Agreed.

--
Bruce Momjian <bruce@momjian.us> https://momjian.us
EDB https://enterprisedb.com

Embrace your flaws. They make you human, rather than perfect,
which you will never be.

On Mon, Jan 30, 2023 at 03:42:09PM -0500, Bruce Momjian wrote:

On Sat, Jan 28, 2023 at 05:06:03PM -0800, Noah Misch wrote:

On Tue, Jan 24, 2023 at 02:04:02PM -0500, Bruce Momjian wrote:

On Tue, Jan 24, 2023 at 09:54:57AM -0500, Tom Lane wrote:

As another example, the mechanisms we use to create the typedefs list
in the first place are pretty squishy/leaky: they depend on which
buildfarm animals are running the typedef-generation step, and on
whether anything's broken lately in that code --- which happens on
a fairly regular basis (eg [1]). Maybe that could be improved,
but I don't see an easy way to capture the set of system-defined
typedefs that are in use on platforms other than your own. I
definitely do not want to go over to hand maintenance of that list.

As I now understand it, we would need to standardize on a typedef list
at the beginning of each major development cycle, and then only allow
additions,

Not to my knowledge. There's no particular obstacle to updating the list more
frequently or removing entries.

We would need to re-pgindent the tree each time, I think, which would
cause disruption if we did it too frequently.

More important than frequency is how much old code changes. A new typedef
typically is an identifier not already appearing in the tree, so no old code
changes. A removed typedef typically no longer appears in the tree, so again
no old code changes. The tree can get those daily; they're harmless.

The push that adds or removes FooTypedef from the source code is in the best
position to react to any surprising indentation consequences of adding or
removing FooTypedef from typedefs.list. (Reactions could include choosing a
different typedef name or renaming incidental matches in older code.) Hence,
changing typedefs.list as frequently as it affects the code is less disruptive
than changing it once a year. The same applies to challenges like pgindent
wrecking a non-"/*----------" comment. Such breakage is hard to miss when
it's part of the push that crafts the comment; it's easier to miss in a bulk,
end-of-cycle pgindent.

Regarding the concern about a pre-receive hook blocking an emergency push, the
hook could approve every push where a string like "pgindent: no" appears in a
commit message within the push. You'd still want to make the tree clean
sometime the same week or so. It's cheap to provide a break-glass like that.

Noah Misch <noah@leadboat.com> writes:

Regarding the concern about a pre-receive hook blocking an emergency push, the
hook could approve every push where a string like "pgindent: no" appears in a
commit message within the push. You'd still want to make the tree clean
sometime the same week or so. It's cheap to provide a break-glass like that.

I think the real question here is whether we can get all (or at least
a solid majority of) committers to accept such draconian constraints.
I'd buy into it, and evidently so would you, but I can't help noting
that less than a quarter of active committers have bothered to
comment on this thread. I suspect the other three-quarters would
be quite annoyed if we tried to institute such requirements. That's
not manpower we can afford to drive away.

Maybe this should get taken up at the this-time-for-sure developer
meeting at PGCon?

regards, tom lane

On Thu, 2 Feb 2023 at 06:40, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Noah Misch <noah@leadboat.com> writes:

Regarding the concern about a pre-receive hook blocking an emergency push, the
hook could approve every push where a string like "pgindent: no" appears in a
commit message within the push. You'd still want to make the tree clean
sometime the same week or so. It's cheap to provide a break-glass like that.

I think the real question here is whether we can get all (or at least
a solid majority of) committers to accept such draconian constraints.
I'd buy into it, and evidently so would you, but I can't help noting
that less than a quarter of active committers have bothered to
comment on this thread. I suspect the other three-quarters would
be quite annoyed if we tried to institute such requirements.

I didn't reply until now, but I'm solidly in the camp of committers
who care about keeping the tree properly indented, and I wouldn't have
any problem with such a check being imposed.

I regularly run pgindent locally, and if I ever commit without
indenting, it's either intentional, or because I forgot, so the
reminder would be useful.

And as someone who runs pgindent regularly, I think this will be a net
time saver, since I won't have to skip over other unrelated indent
changes all the time.

Regards,
Dean

On Fri, Feb 3, 2023 at 12:35 AM Dean Rasheed <dean.a.rasheed@gmail.com> wrote:

And as someone who runs pgindent regularly, I think this will be a net
time saver, since I won't have to skip over other unrelated indent
changes all the time.

+1

On Thu, Feb 2, 2023 at 5:05 PM Dean Rasheed <dean.a.rasheed@gmail.com> wrote:

On Thu, 2 Feb 2023 at 06:40, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Noah Misch <noah@leadboat.com> writes:

Regarding the concern about a pre-receive hook blocking an emergency push, the
hook could approve every push where a string like "pgindent: no" appears in a
commit message within the push. You'd still want to make the tree clean
sometime the same week or so. It's cheap to provide a break-glass like that.

I think the real question here is whether we can get all (or at least
a solid majority of) committers to accept such draconian constraints.
I'd buy into it, and evidently so would you, but I can't help noting
that less than a quarter of active committers have bothered to
comment on this thread. I suspect the other three-quarters would
be quite annoyed if we tried to institute such requirements.

I didn't reply until now, but I'm solidly in the camp of committers
who care about keeping the tree properly indented, and I wouldn't have
any problem with such a check being imposed.

I regularly run pgindent locally, and if I ever commit without
indenting, it's either intentional, or because I forgot, so the
reminder would be useful.

And as someone who runs pgindent regularly, I think this will be a net
time saver, since I won't have to skip over other unrelated indent
changes all the time.

+1.

--
With Regards,
Amit Kapila.

On 2023-02-02 Th 01:40, Tom Lane wrote:

Noah Misch <noah@leadboat.com> writes:

Regarding the concern about a pre-receive hook blocking an emergency push, the
hook could approve every push where a string like "pgindent: no" appears in a
commit message within the push. You'd still want to make the tree clean
sometime the same week or so. It's cheap to provide a break-glass like that.

I think the real question here is whether we can get all (or at least
a solid majority of) committers to accept such draconian constraints.
I'd buy into it, and evidently so would you, but I can't help noting
that less than a quarter of active committers have bothered to
comment on this thread. I suspect the other three-quarters would
be quite annoyed if we tried to institute such requirements. That's
not manpower we can afford to drive away.

I'd be very surprised if this caused any active committer to walk away
from the project. Many will probably appreciate the nudge. But maybe I'm
overoptimistic.

Maybe this should get taken up at the this-time-for-sure developer
meeting at PGCon?

Sure. There's probably some work that could still be done in this area
too, such as making pgperltidy work similarly to how we've now make
pgindent work.

There's also a question of timing. Possibly the best time would be when
we next fork the tree.

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Andrew Dunstan <andrew@dunslane.net> writes:

Sure. There's probably some work that could still be done in this area
too, such as making pgperltidy work similarly to how we've now make
pgindent work.

Perhaps. But before we commit to that, I'd like to see some tweaks to the
pgperltidy rules to make it less eager to revisit the formatting of lines
close to a change. Its current behavior will induce a lot of "git blame"
noise if we apply these same procedures to Perl code.

(Should I mention reformat-dat-files?)

There's also a question of timing. Possibly the best time would be when
we next fork the tree.

Yeah. We have generally not wanted to do a mass indent except
when there's a minimum amount of pending patches, ie after the last
CF of a cycle. What I'd suggest is that we plan on doing a mass
indent and then switch over to the new rules, right after the March
CF closes. That gives us a couple months to nail down and test out
the new procedures before they go live.

regards, tom lane

On 2023-02-02 Th 10:00, Tom Lane wrote:

Andrew Dunstan <andrew@dunslane.net> writes:

Sure. There's probably some work that could still be done in this area
too, such as making pgperltidy work similarly to how we've now make
pgindent work.

Perhaps. But before we commit to that, I'd like to see some tweaks to the
pgperltidy rules to make it less eager to revisit the formatting of lines
close to a change. Its current behavior will induce a lot of "git blame"
noise if we apply these same procedures to Perl code.

I haven't done anything about that yet, but I have reworked the script
so it's a lot more like pgindent, with --show-diff and --silent-diff
modes, and allowing a list of files to be indented on the command line.
Non-perl files are filtered out from such a list.

(Should I mention reformat-dat-files?)

If you want I can add those flags there too.

There's also a question of timing. Possibly the best time would be when
we next fork the tree.

Yeah. We have generally not wanted to do a mass indent except
when there's a minimum amount of pending patches, ie after the last
CF of a cycle. What I'd suggest is that we plan on doing a mass
indent and then switch over to the new rules, right after the March
CF closes. That gives us a couple months to nail down and test out
the new procedures before they go live.

WFM. Of course then we're not waiting for the developer meeting.

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

On 2023-01-22 Su 17:47, Tom Lane wrote:

Andres Freund<andres@anarazel.de> writes:

I strongly dislike it, I rarely get it right by hand - but it does have some
benefit over aligning variable names based on the length of the type names as
uncrustify/clang-format: In their approach an added local variable can cause
all the other variables to be re-indented (and their initial value possibly
wrapped). The fixed alignment doesn't have that issue.

Yeah. That's one of my biggest gripes about pgperltidy: if you insert
another assignment in a series of assignments, it is very likely to
reformat all the adjacent assignments because it thinks it's cool to
make all the equal signs line up. That's just awful. You can either
run pgperltidy on new code before committing, and accept that the feature
patch will touch a lot of lines it's not making real changes to (thereby
dirtying the "git blame" history) or not do so and thereby commit code
that's not passing tidiness checks. Let's *not* adopt any style that
causes similar things to start happening in our C code.

Modern versions of perltidy give you much more control over this, so
maybe we need to investigate the possibility of updating. See the latest
docco at
<*https://metacpan.org/dist/Perl-Tidy/view/bin/perltidy#Completely-turning-off-vertical-alignment-with-novalign&gt;
*

Probably we'd want to use something like

|--valign-exclusion-list=||'= => ,'|
||
||
|cheers|
||
|andrew|

--
Andrew Dunstan
EDB:https://www.enterprisedb.com

Andrew Dunstan <andrew@dunslane.net> writes:

On 2023-01-22 Su 17:47, Tom Lane wrote:

Yeah. That's one of my biggest gripes about pgperltidy: if you insert
another assignment in a series of assignments, it is very likely to
reformat all the adjacent assignments because it thinks it's cool to
make all the equal signs line up. That's just awful.

Modern versions of perltidy give you much more control over this, so
maybe we need to investigate the possibility of updating.

I have no objection to updating perltidy from time to time. I think the
idea is just to make sure that we have an agreed-on version for everyone
to use.

regards, tom lane

On Thu, Feb 02, 2023 at 11:34:37AM +0000, Dean Rasheed wrote:

I didn't reply until now, but I'm solidly in the camp of committers
who care about keeping the tree properly indented, and I wouldn't have
any problem with such a check being imposed.

So do I. pgindent is part of my routine when it comes to all the
patches I merge on HEAD, and having to clean up unrelated diffs in
the files touched after an indentation is always annoying.

FWIW, I just use a script that does pgindent, pgperltidy, pgperlcritic
and `make reformat-dat-files` in src/include/catalog.

I regularly run pgindent locally, and if I ever commit without
indenting, it's either intentional, or because I forgot, so the
reminder would be useful.

And as someone who runs pgindent regularly, I think this will be a net
time saver, since I won't have to skip over other unrelated indent
changes all the time.

+1.
--
Michael

On Fri, Feb 03, 2023 at 12:52:50PM -0500, Tom Lane wrote:

Andrew Dunstan <andrew@dunslane.net> writes:

On 2023-01-22 Su 17:47, Tom Lane wrote:

Yeah. That's one of my biggest gripes about pgperltidy: if you insert
another assignment in a series of assignments, it is very likely to
reformat all the adjacent assignments because it thinks it's cool to
make all the equal signs line up. That's just awful.

Modern versions of perltidy give you much more control over this, so
maybe we need to investigate the possibility of updating.

I have no objection to updating perltidy from time to time. I think the
idea is just to make sure that we have an agreed-on version for everyone
to use.

Agreed. If we're changing the indentation of assignments, that's a
considerable diff already. It would be a good time to absorb other diffs
we'll want eventually, including diffs from a perltidy version upgrade.

Hi,

On 2023-02-03 20:18:48 -0800, Noah Misch wrote:

On Fri, Feb 03, 2023 at 12:52:50PM -0500, Tom Lane wrote:

Andrew Dunstan <andrew@dunslane.net> writes:

On 2023-01-22 Su 17:47, Tom Lane wrote:

Yeah. That's one of my biggest gripes about pgperltidy: if you insert
another assignment in a series of assignments, it is very likely to
reformat all the adjacent assignments because it thinks it's cool to
make all the equal signs line up. That's just awful.

Modern versions of perltidy give you much more control over this, so
maybe we need to investigate the possibility of updating.

I have no objection to updating perltidy from time to time. I think the
idea is just to make sure that we have an agreed-on version for everyone
to use.

Agreed. If we're changing the indentation of assignments, that's a
considerable diff already. It would be a good time to absorb other diffs
we'll want eventually, including diffs from a perltidy version upgrade.

ISTM that we're closer to being able to enforce pgindent than
perltidy. At the same time, I think the issue of C code in HEAD not
being indented is more pressing - IME it's much more common to have to
touch a lot of C code than to have to touch a lot fo perl files. So
perhaps we should just start with being more stringent with C code, and
once we made perltidy less noisy, add that?

Greetings,

Andres Freund

On 2023-02-04 Sa 06:34, Andres Freund wrote:

ISTM that we're closer to being able to enforce pgindent than
perltidy. At the same time, I think the issue of C code in HEAD not
being indented is more pressing - IME it's much more common to have to
touch a lot of C code than to have to touch a lot fo perl files. So
perhaps we should just start with being more stringent with C code, and
once we made perltidy less noisy, add that?

Sure, we don't have to tie them together.

I'm currently experimenting with settings on the buildfarm code, trying
to get it both stable and looking nice. Then I'll try on the Postgres
core code and post some results.

cheers

andrew

--
Andrew Dunstan
EDB:https://www.enterprisedb.com

Andres Freund <andres@anarazel.de> writes:

ISTM that we're closer to being able to enforce pgindent than
perltidy. At the same time, I think the issue of C code in HEAD not
being indented is more pressing - IME it's much more common to have to
touch a lot of C code than to have to touch a lot fo perl files. So
perhaps we should just start with being more stringent with C code, and
once we made perltidy less noisy, add that?

Agreed, we should move more slowly with perltidy. Aside from the
points you raise, I bet fewer committers have it installed at all.

(I haven't forgotten that I'm on the hook to import pg_bsd_indent
into our tree. Will get to that soon.)

regards, tom lane

On Sat, Feb 04, 2023 at 11:07:59AM -0500, Tom Lane wrote:

(I haven't forgotten that I'm on the hook to import pg_bsd_indent
into our tree. Will get to that soon.)

+1 for that - it's no surprise that you have trouble convincing people
to follow the current process:

1) requires using a hacked copy of BSD indent; 2) which is stored
outside the main repo; 3) is run via a perl script that itself mungles
the source code (because the only indent tool that can support the
project's style doesn't actually support what's needed); 4) and wants to
retrieve a remote copy of typedefs.list (?).

The only thing that makes this scheme even remotely viable is that
apt.postgresql.org includes a package for pg-bsd-indent. I've used it
only a handful of times by running:
pg_bsd_indent -bad -bap -bbb -bc -bl -cli1 -cp33 -cdb -nce -d0 -di12 -nfc1 -i4 -l79 -lp -lpl -nip -npro -sac -tpg -ts4 -U .../typedefs.list

The perl wrapper is still a step too far for me (maybe it'd be tolerable
if available as a build target).

Would you want to make those the default options of the in-tree indent ?
Or provide a shortcut like --postgresql ?

--
Justin

Justin Pryzby <pryzby@telsasoft.com> writes:

Would you want to make those the default options of the in-tree indent ?
Or provide a shortcut like --postgresql ?

Hmmm ... inserting all of those as the default options would likely
make it impossible to update pg_bsd_indent itself with anything like
its current indent style (not that it's terribly consistent about
that). I could see inventing a --postgresql shortcut switch perhaps.

But it's not clear to me why you're allergic to the perl wrapper?
It's not like that's the only perl infrastructure in our build process.
Also, whether or not we could push some of what it does into pg_bsd_indent
proper, I can't see pushing all of it (for instance, the very PG-specific
list of typedef exclusions).

regards, tom lane

On 2023-02-04 Sa 09:20, Andrew Dunstan wrote:

On 2023-02-04 Sa 06:34, Andres Freund wrote:

ISTM that we're closer to being able to enforce pgindent than
perltidy. At the same time, I think the issue of C code in HEAD not
being indented is more pressing - IME it's much more common to have to
touch a lot of C code than to have to touch a lot fo perl files. So
perhaps we should just start with being more stringent with C code, and
once we made perltidy less noisy, add that?

Sure, we don't have to tie them together.

I'm currently experimenting with settings on the buildfarm code,
trying to get it both stable and looking nice. Then I'll try on the
Postgres core code and post some results.

So here's a diff made from running perltidy v20221112 with the
additional setting --valign-exclusion-list=", = => || && if unless"

Essentially this abandons those bits of vertical alignment that tend to
catch us out when additions are made to the code.

I think this will make the code much more maintainable and result in
much less perltidy churn. It would also mean that it's far more likely
that what we would naturally code can be undisturbed by perltidy.

cheers

andrew

--
Andrew Dunstan
EDB:https://www.enterprisedb.com

On Sat, Feb 4, 2023 at 12:37 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

But it's not clear to me why you're allergic to the perl wrapper?
It's not like that's the only perl infrastructure in our build process.
Also, whether or not we could push some of what it does into pg_bsd_indent
proper, I can't see pushing all of it (for instance, the very PG-specific
list of typedef exclusions).

I don't mind that there is a script. I do mind that it's not that good
of a script. There have been some improvements for which I am
grateful, like removing the thing where the first argument was taken
as a typedefs file under some circumstances. But there are still some
things that I would like:

1. I'd like to be able to run pgindent src/include and have it indent
everything relevant under src/include. Right now that silently does
nothing.

2. I'd like an easy way to indent the unstaged files in the current
directory (e.g. pgindent --dirty) or the files that have been queued
up for commit (e.g. pgindent --cached).

3. I'd also like an easy way to indent every file touched by a recent
commit, e.g. pgindent --commit HEAD, pgindent --commit HEAD~2,
pgindent --commit 62e1e28bf7.

It'd be much less annoying to include this in my workflow with these
kinds of options. For instance:

patch -p1 < ~/Downloads/some_stuff_v94.patch
# committer adjustments as desired
git add -u
pgindent --cached
git diff # did pgindent change anything? does it look ok?
git commit -a

For a while I, like some others here, was trying to be religious about
pgindenting at least the bigger commits that I pushed. But I fear I've
grown slack. I don't mind if we tighten up the process, but the better
we make the tools, the less friction it will cause.

--
Robert Haas
EDB: http://www.enterprisedb.com

Robert Haas <robertmhaas@gmail.com> writes:

I don't mind that there is a script. I do mind that it's not that good
of a script. There have been some improvements for which I am
grateful, like removing the thing where the first argument was taken
as a typedefs file under some circumstances. But there are still some
things that I would like:

I have no objection to someone coding those things up ;-).
I'll just note that adding features like those to a Perl script
is going to be a ton easier than doing it inside pg_bsd_indent.

regards, tom lane

On 2023-02-06 Mo 09:40, Robert Haas wrote:

On Sat, Feb 4, 2023 at 12:37 PM Tom Lane<tgl@sss.pgh.pa.us> wrote:

But it's not clear to me why you're allergic to the perl wrapper?
It's not like that's the only perl infrastructure in our build process.
Also, whether or not we could push some of what it does into pg_bsd_indent
proper, I can't see pushing all of it (for instance, the very PG-specific
list of typedef exclusions).

I don't mind that there is a script. I do mind that it's not that good
of a script. There have been some improvements for which I am
grateful, like removing the thing where the first argument was taken
as a typedefs file under some circumstances. But there are still some
things that I would like:

1. I'd like to be able to run pgindent src/include and have it indent
everything relevant under src/include. Right now that silently does
nothing.

2. I'd like an easy way to indent the unstaged files in the current
directory (e.g. pgindent --dirty) or the files that have been queued
up for commit (e.g. pgindent --cached).

3. I'd also like an easy way to indent every file touched by a recent
commit, e.g. pgindent --commit HEAD, pgindent --commit HEAD~2,
pgindent --commit 62e1e28bf7.

Good suggestions. 1 and 3 seem fairly straightforward. I'll start on
those, and look into 2.

cheers

andrew

--
Andrew Dunstan
EDB:https://www.enterprisedb.com

On Mon, Feb 6, 2023 at 10:16 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

I'll just note that adding features like those to a Perl script
is going to be a ton easier than doing it inside pg_bsd_indent.

+1.

--
Robert Haas
EDB: http://www.enterprisedb.com

On Mon, Feb 6, 2023 at 10:21 AM Andrew Dunstan <andrew@dunslane.net> wrote:

Good suggestions. 1 and 3 seem fairly straightforward. I'll start on those, and look into 2.

Thanks!

--
Robert Haas
EDB: http://www.enterprisedb.com

On 2023-02-06 Mo 10:36, Robert Haas wrote:

On Mon, Feb 6, 2023 at 10:21 AM Andrew Dunstan<andrew@dunslane.net> wrote:

Good suggestions. 1 and 3 seem fairly straightforward. I'll start on those, and look into 2.

Thanks!

Here's a quick patch for 1 and 3. Would also need to adjust the docco.

cheers

andrew

--
Andrew Dunstan
EDB:https://www.enterprisedb.com

On 02.02.23 07:40, Tom Lane wrote:

Noah Misch <noah@leadboat.com> writes:

Regarding the concern about a pre-receive hook blocking an emergency push, the
hook could approve every push where a string like "pgindent: no" appears in a
commit message within the push. You'd still want to make the tree clean
sometime the same week or so. It's cheap to provide a break-glass like that.

I think the real question here is whether we can get all (or at least
a solid majority of) committers to accept such draconian constraints.
I'd buy into it, and evidently so would you, but I can't help noting
that less than a quarter of active committers have bothered to
comment on this thread. I suspect the other three-quarters would
be quite annoyed if we tried to institute such requirements. That's
not manpower we can afford to drive away.

I have some concerns about this.

First, as a matter of principle, it would introduce another level of
gatekeeping power. Right now, the committers are as a group in charge
of what gets into the tree. Adding commit hooks that are installed
somewhere(?) by someone(?) and can only be seen by some(?) would upset
that. If we were using something like github or gitlab (not suggesting
that, but for illustration), then you could put this kind of thing under
.github/ or similar and then it would be under the same control as the
source code itself.

Also, pgindent takes tens of seconds to run, so hooking that into the
git push process would slow this down quite a bit. And maybe we want to
add pgperltidy and so on, where would this lead? If somehow your local
indenting doesn't give you the "correct" result for some reason, you
might sit there for minutes and minutes trying to fix and push and fix
and push.

Then, consider the typedefs issue. If you add a typedef but don't add
it to the typedefs list but otherwise pgindent your code perfectly, the
push would be accepted. If then later someone updates the typedefs
list, perhaps from the build farm, it would then reject the indentation
of your previously committed code, thus making it their problem.

I think a better way to address these issues would be making this into a
test suite, so that you can run some command that checks "is everything
indented correctly". Then you can run this locally, on the build farm,
in the cfbot etc. in a uniform way and apply the existing
blaming/encouragement processes like for any other test failure.

On 2023-02-06 Mo 12:03, Andrew Dunstan wrote:

On 2023-02-06 Mo 10:36, Robert Haas wrote:

On Mon, Feb 6, 2023 at 10:21 AM Andrew Dunstan<andrew@dunslane.net> wrote:

Good suggestions. 1 and 3 seem fairly straightforward. I'll start on those, and look into 2.

Thanks!

Here's a quick patch for 1 and 3. Would also need to adjust the docco.

This time with patch.

cheers

andrew

--
Andrew Dunstan
EDB:https://www.enterprisedb.com

On 2023-02-06 Mo 12:17, Peter Eisentraut wrote:

On 02.02.23 07:40, Tom Lane wrote:

Noah Misch <noah@leadboat.com> writes:

Regarding the concern about a pre-receive hook blocking an emergency
push, the
hook could approve every push where a string like "pgindent: no"
appears in a
commit message within the push.  You'd still want to make the tree
clean
sometime the same week or so.  It's cheap to provide a break-glass
like that.

I think the real question here is whether we can get all (or at least
a solid majority of) committers to accept such draconian constraints.
I'd buy into it, and evidently so would you, but I can't help noting
that less than a quarter of active committers have bothered to
comment on this thread.  I suspect the other three-quarters would
be quite annoyed if we tried to institute such requirements. That's
not manpower we can afford to drive away.

I have some concerns about this.

First, as a matter of principle, it would introduce another level of
gatekeeping power.  Right now, the committers are as a group in charge
of what gets into the tree.  Adding commit hooks that are installed
somewhere(?) by someone(?) and can only be seen by some(?) would upset
that.  If we were using something like github or gitlab (not
suggesting that, but for illustration), then you could put this kind
of thing under .github/ or similar and then it would be under the same
control as the source code itself.

Also, pgindent takes tens of seconds to run, so hooking that into the
git push process would slow this down quite a bit.  And maybe we want
to add pgperltidy and so on, where would this lead?  If somehow your
local indenting doesn't give you the "correct" result for some reason,
you might sit there for minutes and minutes trying to fix and push and
fix and push.

Well, pgindent should produce canonical results or we're surely doing it
wrong. Regarding the time it takes, if we are only indenting the changed
files that time will be vastly reduced for most cases.

But I take your point to some extent. I think we should start by making
it easier and quicker to run pgindent locally, both by hand and in local
git hooks, for ordinary developers and for committers, and we should
encourage committers to be stricter in their use of pgindent. If there
are features we need to make this possible, speak up (c.f. Robert's
email earlier today). I'm committed to making this as easy as possible
for people.

Once we get over those hurdles we can possibly revisit automation.

Then, consider the typedefs issue.  If you add a typedef but don't add
it to the typedefs list but otherwise pgindent your code perfectly,
the push would be accepted.  If then later someone updates the
typedefs list, perhaps from the build farm, it would then reject the
indentation of your previously committed code, thus making it their
problem.

It would be nice if there were a gadget that would find new typedefs and
warn you about them. Unfortunately our current code to find typedefs
isn't all that fast either. Nicer still would be a way of not needing
the typedefs list, but I don't think anyone has come up with one yet
that meets our other requirements.

I think a better way to address these issues would be making this into
a test suite, so that you can run some command that checks "is
everything indented correctly".  Then you can run this locally, on the
build farm, in the cfbot etc. in a uniform way and apply the existing
blaming/encouragement processes like for any other test failure.

Well arguably the new --silent-diff and --show-diff modes are such tests :-)

cheers

andrew

--
Andrew Dunstan
EDB:https://www.enterprisedb.com

Hi,

On 2023-02-06 18:17:02 +0100, Peter Eisentraut wrote:

First, as a matter of principle, it would introduce another level of
gatekeeping power. Right now, the committers are as a group in charge of
what gets into the tree. Adding commit hooks that are installed
somewhere(?) by someone(?) and can only be seen by some(?) would upset that.
If we were using something like github or gitlab (not suggesting that, but
for illustration), then you could put this kind of thing under .github/ or
similar and then it would be under the same control as the source code
itself.

Well, we did talk about adding a pre-commit hook to the repository, with
instructions for how to enable it. And I don't see a problem with adding the
pre-receive we're discussing here to src/tools/something.

Also, pgindent takes tens of seconds to run, so hooking that into the git
push process would slow this down quite a bit. And maybe we want to add
pgperltidy and so on, where would this lead?

Yes, I've been annoyed by this as well. This is painful, even without a push
hook. Not just for pgindent, headerscheck/cpluspluscheck are quite painful as
well. I came to the conclusion that we ought to integrate pgindent etc into
the buildsystem properly. Instead of running such targets serially across all
files, without logic to prevent re-processing files, the relevant targets
ought to be run once for each process, and create a stamp file.

If somehow your local indenting doesn't give you the "correct" result for
some reason, you might sit there for minutes and minutes trying to fix and
push and fix and push.

I was imagining that such a pre-receive hook would spit out the target that
you'd need to run locally to verify that the issue is resolved.

Then, consider the typedefs issue. If you add a typedef but don't add it to
the typedefs list but otherwise pgindent your code perfectly, the push would
be accepted. If then later someone updates the typedefs list, perhaps from
the build farm, it would then reject the indentation of your previously
committed code, thus making it their problem.

I'd like to address this one via the buildsystem as well. We can do the
trickery that the buildfarm uses to extract typedefs as part of the build, and
update typedefs.list with the additional types. There's really no need to
force us to do this manually.

Greetings,

Andres Freund

Andres Freund <andres@anarazel.de> writes:

On 2023-02-06 18:17:02 +0100, Peter Eisentraut wrote:

First, as a matter of principle, it would introduce another level of
gatekeeping power. Right now, the committers are as a group in charge of
what gets into the tree. Adding commit hooks that are installed
somewhere(?) by someone(?) and can only be seen by some(?) would upset that.
If we were using something like github or gitlab (not suggesting that, but
for illustration), then you could put this kind of thing under .github/ or
similar and then it would be under the same control as the source code
itself.

Well, we did talk about adding a pre-commit hook to the repository, with
instructions for how to enable it. And I don't see a problem with adding the
pre-receive we're discussing here to src/tools/something.

Yeah. I don't think we are seriously considering putting any restrictions
in place on gitmaster --- the idea is to offer better tools to committers
to let them check/fix the indentation of what they are working on. If
somebody wants to run that as a local pre-commit hook, that's their choice.

regards, tom lane

On Mon, Feb 06, 2023 at 06:17:02PM +0100, Peter Eisentraut wrote:

Also, pgindent takes tens of seconds to run, so hooking that into the git
push process would slow this down quite a bit.

The pre-receive hook would do a full pgindent when you change typedefs.list.
Otherwise, it would reindent only the files being changed. The average push
need not take tens of seconds.

If somehow your local
indenting doesn't give you the "correct" result for some reason, you might
sit there for minutes and minutes trying to fix and push and fix and push.

As Andres mentioned, the hook could print the command it used. It could even
print the diff it found, for you to apply.

On Mon, Feb 06, 2023 at 04:36:07PM -0500, Tom Lane wrote:

Andres Freund <andres@anarazel.de> writes:

On 2023-02-06 18:17:02 +0100, Peter Eisentraut wrote:

First, as a matter of principle, it would introduce another level of
gatekeeping power. Right now, the committers are as a group in charge of
what gets into the tree. Adding commit hooks that are installed
somewhere(?) by someone(?) and can only be seen by some(?) would upset that.
If we were using something like github or gitlab (not suggesting that, but
for illustration), then you could put this kind of thing under .github/ or
similar and then it would be under the same control as the source code
itself.

Well, we did talk about adding a pre-commit hook to the repository, with
instructions for how to enable it. And I don't see a problem with adding the
pre-receive we're discussing here to src/tools/something.

Yeah. I don't think we are seriously considering putting any restrictions
in place on gitmaster

I could have sworn that was exactly what we were discussing, a pre-receive
hook on gitmaster.

On 2023-02-06 Mo 23:43, Noah Misch wrote:

Well, we did talk about adding a pre-commit hook to the repository, with
instructions for how to enable it. And I don't see a problem with adding the
pre-receive we're discussing here to src/tools/something.

Yeah. I don't think we are seriously considering putting any restrictions
in place on gitmaster

I could have sworn that was exactly what we were discussing, a pre-receive
hook on gitmaster.

That's one idea that's been put forward, but it seems clear that some
people are nervous about it.

Maybe a better course would be to continue improving the toolset and get
more people comfortable with using it locally and then talk about
integrating it upstream.

cheers

andrew

--
Andrew Dunstan
EDB:https://www.enterprisedb.com

On Tue, Feb 7, 2023 at 5:16 PM Andrew Dunstan <andrew@dunslane.net> wrote:

On 2023-02-06 Mo 23:43, Noah Misch wrote:

Well, we did talk about adding a pre-commit hook to the repository, with
instructions for how to enable it. And I don't see a problem with adding the
pre-receive we're discussing here to src/tools/something.

Yeah. I don't think we are seriously considering putting any restrictions
in place on gitmaster

I could have sworn that was exactly what we were discussing, a pre-receive
hook on gitmaster.

That's one idea that's been put forward, but it seems clear that some people are nervous about it.

Maybe a better course would be to continue improving the toolset and get more people comfortable with using it locally and then talk about integrating it upstream.

Yeah, that sounds more reasonable to me as well.

--
With Regards,
Amit Kapila.

On Tue, Feb 7, 2023 at 1:56 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Tue, Feb 7, 2023 at 5:16 PM Andrew Dunstan <andrew@dunslane.net> wrote:

On 2023-02-06 Mo 23:43, Noah Misch wrote:

Well, we did talk about adding a pre-commit hook to the repository, with
instructions for how to enable it. And I don't see a problem with adding

the

pre-receive we're discussing here to src/tools/something.

Yeah. I don't think we are seriously considering putting any

restrictions

in place on gitmaster

I could have sworn that was exactly what we were discussing, a

pre-receive

hook on gitmaster.

That's one idea that's been put forward, but it seems clear that some

people are nervous about it.

Maybe a better course would be to continue improving the toolset and get

more people comfortable with using it locally and then talk about
integrating it upstream.

Yeah, that sounds more reasonable to me as well.