Обсуждение: Add "format" target to make and ninja to run pgindent and pgperltidy

This tries to make running formatting a lot easier for committers, but
primarily for new contributors. You can not format the files by simply
running one of the folowing:

make format
ninja -C build format

My primary goal is to introduce Python formatting too (using ruff), and
integrate that in a similar manner. Right now we don't have many Python
files, but hopefully the pytest patch gets merged soonish and then we'll
get more and more Python files. So I'd like to get autoformatting out of
the gate.

But even without that, these rules should make it simpler to run the
current formatters:

Running pgindent is still not trivial for new users. You need to add our
pg_bsd_indent to PATH. And if you use meson to build you have to manually
specify src/ and contrib/ instead of ./ because otherwise pgindent will
try to format files in the build directory.

Running pgperltidy is even more complicated because you need to install
a specific version of perltidy. Recently we started erroring when that
version was not the one we expect.

On Wed, Dec 31, 2025 at 5:06 PM Jelte Fennema-Nio <postgres@jeltef.nl> wrote:
>
> This tries to make running formatting a lot easier for committers, but
> primarily for new contributors. You can not format the files by simply
> running one of the folowing:
>
> make format
> ninja -C build format
>

I generally like the idea. Since perltidy is not enforced regularly
(like pgindent), running it usually ends up modifying files which are
not part of the patch. So I avoid it if not necessary. Do you propose
to make it optional?

--
Best Wishes,
Ashutosh Bapat

Ashutosh Bapat <ashutosh.bapat.oss@gmail.com> writes:
> On Wed, Dec 31, 2025 at 5:06 PM Jelte Fennema-Nio <postgres@jeltef.nl> wrote:
>> This tries to make running formatting a lot easier for committers, but
>> primarily for new contributors.

> I generally like the idea. Since perltidy is not enforced regularly
> (like pgindent), running it usually ends up modifying files which are
> not part of the patch. So I avoid it if not necessary. Do you propose
> to make it optional?

The other obstacle is that not everybody will have the right version
of perltidy installed, but using some other version will create a
whole lot of extraneous noise.

On the whole I'd recommend not trying to automate the perltidy
step yet.  Cost/benefit is just not very good.

On the substance of the patch: I wonder whether we could make things
more reliable by using git metadata to figure out which .h and .c
files to point pgindent at.

            regards, tom lane

Ashutosh Bapat <ashutosh.bapat.oss@gmail.com> writes:

On Wed, Dec 31, 2025 at 5:06 PM Jelte Fennema-Nio <postgres@jeltef.nl> wrote:

This tries to make running formatting a lot easier for committers, but
primarily for new contributors.

I generally like the idea. Since perltidy is not enforced regularly
(like pgindent), running it usually ends up modifying files which are
not part of the patch. So I avoid it if not necessary. Do you propose
to make it optional?

The other obstacle is that not everybody will have the right version
of perltidy installed, but using some other version will create a
whole lot of extraneous noise.

pgperltidy actually checks the version now.

On the whole I'd recommend not trying to automate the perltidy
step yet.  Cost/benefit is just not very good.

I'd kinda like to unify these universes, though. We could import the pgperltidy logic into pgindent but disable it unless some flag were given and the correct version of perltidy were present.

On the substance of the patch: I wonder whether we could make things
more reliable by using git metadata to figure out which .h and .c
files to point pgindent at.
			

The git pre-commit hook I use operates with this set of files:

    files=$(git diff --cached --name-only --diff-filter=ACMR)

pgindent just currently looks at that set of files and ignores everything that's not a .c or .h file.

I guess what you're wanting is a test to see if the file is in git or a generated file? That doesn't really arise for me as I always do vpath builds, so generated files are always elsewhere.

cheers

andrew

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

Andrew Dunstan <andrew@dunslane.net> writes:
> On 2025-12-31 We 10:26 AM, Tom Lane wrote:
>> On the substance of the patch: I wonder whether we could make things
>> more reliable by using git metadata to figure out which .h and .c
>> files to point pgindent at.

> I guess what you're wanting is a test to see if the file is in git or a 
> generated file? That doesn't really arise for me as I always do vpath 
> builds, so generated files are always elsewhere.

Right.  But if we're trying to make this easy, we need to make
the automation work for all three use-cases (in-tree makefiles,
vpath makefiles, meson).  I was just wondering if relying on
git would simplify getting the same results in all three.

            regards, tom lane

Andrew Dunstan <andrew@dunslane.net> writes:

On 2025-12-31 We 10:26 AM, Tom Lane wrote:

On the substance of the patch: I wonder whether we could make things
more reliable by using git metadata to figure out which .h and .c
files to point pgindent at.

I guess what you're wanting is a test to see if the file is in git or a 
generated file? That doesn't really arise for me as I always do vpath 
builds, so generated files are always elsewhere.

Right.  But if we're trying to make this easy, we need to make
the automation work for all three use-cases (in-tree makefiles,
vpath makefiles, meson).  I was just wondering if relying on
git would simplify getting the same results in all three.
			

I think we could use

    git ls-files -t $file

or similar.

cheers

andrew

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

On Wed Dec 31, 2025 at 4:26 PM CET, Tom Lane wrote:
> Ashutosh Bapat <ashutosh.bapat.oss@gmail.com> writes:
>> On Wed, Dec 31, 2025 at 5:06 PM Jelte Fennema-Nio <postgres@jeltef.nl> wrote:
>>> This tries to make running formatting a lot easier for committers, but
>>> primarily for new contributors.
>
>> I generally like the idea. Since perltidy is not enforced regularly
>> (like pgindent), running it usually ends up modifying files which are
>> not part of the patch. So I avoid it if not necessary. Do you propose
>> to make it optional?
>
> The other obstacle is that not everybody will have the right version
> of perltidy installed, but using some other version will create a
> whole lot of extraneous noise.
>
> On the whole I'd recommend not trying to automate the perltidy
> step yet.  Cost/benefit is just not very good.

I would like to get to a point where it is enforced for every commit
pushed by committers, so the same as with pgindent. I agree that the
cost/benefit is not there currently, but that's what this patchset is
trying to address. The docs in the last patch try to explain clearly how
to get and configure the correct version of perltidy. And once you've
done that it's as easy as running a single make/ninja command to format
all the files.

Do you think that's not still not easy enough for committers? What would
be needed instead? Automatically fetching and installing perltidy to the
local build path when running make/ninja format?

> On the substance of the patch: I wonder whether we could make things
> more reliable by using git metadata to figure out which .h and .c
> files to point pgindent at.

I think that would definitely be helpful. The fact that pgindent runs on
files in the meson build directory when passing "." would be solved by that.

"Jelte Fennema-Nio" <postgres@jeltef.nl> writes:
> On Wed Dec 31, 2025 at 4:26 PM CET, Tom Lane wrote:
>> On the whole I'd recommend not trying to automate the perltidy
>> step yet.  Cost/benefit is just not very good.

> I would like to get to a point where it is enforced for every commit
> pushed by committers, so the same as with pgindent.

As an affected committer, I want to push back against having such
a requirement, because I don't think it is reasonable to require
everybody to have precisely version XYZ of perltidy installed.
If that's not the version provided by their platform-of-choice,
it's an annoying hurdle.

As a comparison point, we did not start requiring pgindent cleanliness
until we imported bsdindent into our tree, so as not to have an
external dependency for that.  (But I can't see vendoring perltidy,
even if there weren't license issues involved.)

I recognize the analogy to requiring a specific version of autoconf,
but the difference is that without autoconf you just plain can't work
on the configure code.  Here, the hurdle would be erected for no
reason stronger than neatnik-ism, and IMO that's not a good enough
reason to put yet another burden on committers.

I'm even less pleased by the notion that we'd soon add still another
such requirement for python.

            regards, tom lane

On Wed, 31 Dec 2025 at 19:37, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> As an affected committer, I want to push back against having such
> a requirement, because I don't think it is reasonable to require
> everybody to have precisely version XYZ of perltidy installed.
> If that's not the version provided by their platform-of-choice,
> it's an annoying hurdle.

That's why I tried to make that hurdle as small as possible. All
that's needed with the current patchset is:

wget https://cpan.metacpan.org/authors/id/S/SH/SHANCOCK/Perl-Tidy-20230309.tar.gz
cpanm -l $HOME/perltidy-20230309 Perl-Tidy-20230309.tar.gz
meson setup build -DPERLTIDY=$HOME/perltidy-20230309/bin/perltidy --reconfigure

> As a comparison point, we did not start requiring pgindent cleanliness
> until we imported bsdindent into our tree, so as not to have an
> external dependency for that.  (But I can't see vendoring perltidy,
> even if there weren't license issues involved.)

I agree vendoring perltidy seems like a bad idea. Given you think
those 3 commands above are still an annoying hurdle, I'm curious what
you think would be a small enough hurdle to not be annoying? A few
options I can see:

1. src/tools/indent/get_perltidy /your/path/of/choice (and then give
this path to configure/meson)
2. make/ninja get-perltidy (downloads + installs to a known directory
inside the build dir)
3. make/ninja format (which then downloads perltidy if it's not yet available)

> Here, the hurdle would be erected for no
> reason stronger than neatnik-ism, and IMO that's not a good enough
> reason to put yet another burden on committers.

To me, the goal of autoformatting is the exact opposite of
neatnik-ism. It means devs don't have to worry about styling their
code nicely because the tool will fix it. So no mental capacity is
spent considering arbitrary styling decisions like where to put
newlines etc.