Re: run pgindent on a regular basis / scripted manner

On 2023-02-06 Mo 12:17, Peter Eisentraut wrote:
> On 02.02.23 07:40, Tom Lane wrote:
>> Noah Misch <noah(at)leadboat(dot)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