pgindent to ignore build directories
Hackorum builds and tests every patch posted to the lists, not only commitfest submissions. This is Hackorum's own CI rather than the PostgreSQL project's, and it is still under testing - please report anything that looks wrong.
This thread has been committed, so CI has stopped here. Anything below is the last result it produced.
You can run a PostgreSQL built from this patch straight from Docker, with no checkout and no build:
docker run --rm -p 5432:5432 ghcr.io/hackorum-dev/postgres-patch:t253924psql -h localhost -U postgresBuilt from patchset v7 (message #7), October 05, 2026 at 07:21 AM.
Every patchset is also pushed to a branch of our PostgreSQL fork, so you can check out the same tree CI built. Without a PostgreSQL checkout:
git clone --branch t253924_7 https://github.com/hackorum-dev/postgres.gitIn a checkout you already have, add the fork once:
git remote add hackorum https://github.com/hackorum-dev/postgres.gitthen, for this patchset and every later one:
git fetch hackorum t253924_7 && git checkout t253924_7Patchset v7 (message #7) is on t253924_7
A problem I'm having is that if I'm having a build directory inside the
source tree
meson setup build
(as is documented) and then run pgindent
src/tools/pgindent/pgindent .
it descends into the build directory and then chokes on various files in
there. (exclude_file_patterns doesn't help because those patterns are
applied relative to the source directory.)
A subproblem is that if pgindent fails on many files, it doesn't clean
up after itself but leaves many pgsrc*.BAK files lying around. The
first patch fixes that.
Then, my first idea was to add an option to pgindent to exclude
directories from being processed. Like
src/tools/pgindent/pgindent . --exclude-dir=build
The second patch implements that.
But then I had the idea that we could recognize build directories
automatically by checking what files they contain. The third patch
implements that. That way, my initial pgindent invocation above works
out of the box and I don't have to remember to enumerate the build
directories on the command line.
(So the second patch is maybe obsolete, but the third one builds on it,
so I left it in here for discussion. People seem to have a variety of
workflows, so let's see what everyone wants.)
Attachments:
t253924_10001-pgindent-Don-t-leave-a-.BAK-file-behind-when-pg_bsd_.patchtext/plain; charset=UTF-8; name=0001-pgindent-Don-t-leave-a-.BAK-file-behind-when-pg_bsd_.patchDownload+5-3
0002-pgindent-Add-exclude-dir-option.patchtext/plain; charset=UTF-8; name=0002-pgindent-Add-exclude-dir-option.patchDownload+62-5
0003-pgindent-Skip-build-directories-automatically.patchtext/plain; charset=UTF-8; name=0003-pgindent-Skip-build-directories-automatically.patchDownload+31-5
Peter Eisentraut <peter@eisentraut.org> writes:
A problem I'm having is that if I'm having a build directory inside the
source tree
meson setup build
(as is documented) and then run pgindent
src/tools/pgindent/pgindent .
it descends into the build directory and then chokes on various files in
there.
Right, we should handle that.
A subproblem is that if pgindent fails on many files, it doesn't clean
up after itself but leaves many pgsrc*.BAK files lying around. The
first patch fixes that.
I object to this patch. src/tools/pgindent/README documents that
the presence of those files is useful for detecting where pgindent
failed. Without them there's not an easy signal.
Then, my first idea was to add an option to pgindent to exclude
directories from being processed. Like
src/tools/pgindent/pgindent . --exclude-dir=build
The second patch implements that.
That seems a bit tedious.
But then I had the idea that we could recognize build directories
automatically by checking what files they contain. The third patch
implements that. That way, my initial pgindent invocation above works
out of the box and I don't have to remember to enumerate the build
directories on the command line.
+1 for this one. I could do without 0002, for myself; what use would
it have once the automatic logic exists?
regards, tom lane
Hi,
On Thu, 24 Sept 2026 at 17:29, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Peter Eisentraut <peter@eisentraut.org> writes:
A subproblem is that if pgindent fails on many files, it doesn't clean
up after itself but leaves many pgsrc*.BAK files lying around. The
first patch fixes that.I object to this patch. src/tools/pgindent/README documents that
the presence of those files is useful for detecting where pgindent
failed. Without them there's not an easy signal.
I remember being frustrated with too many pgsrc*.BAK files but that
was when pgindent processed the build folder. I think we might not
need 0001 after 0003.
--
Regards,
Nazir Bilal Yavuz
Microsoft
On Thu, 24 Sept 2026 at 16:29, Tom Lane <tgl@sss.pgh.pa.us> wrote:
I object to this patch. src/tools/pgindent/README documents that
the presence of those files is useful for detecting where pgindent
failed. Without them there's not an easy signal.
Hard disagree. I don't think BAK files serve that purpose well, and
they should be removed always imo (or possibly not even created in the
first place). There are two much better signals for detecting whether
and how pgindent fails:
1. stderr of pgindent
2. exit code of pgindent
The BAK files have useless names (e.g. pgsrcq4373.BAK). So if you want
to use them to detect where pgindent failed you have to look at their
contents and figure out the original based on that. While stderr
actually shows you what file failed and what the problem was, e.g.:
Failure in ./build/src/backend/bootstrap/bootparse.h: Error@82:
Statement nesting error
Error@113: Stuff missing from end of file
But I do agree that the pgindent README should call out these better
methods when changing the BAK file behaviour.
Jelte Fennema-Nio <postgres@jeltef.nl> writes:
On Thu, 24 Sept 2026 at 16:29, Tom Lane <tgl@sss.pgh.pa.us> wrote:
I object to this patch. src/tools/pgindent/README documents that
the presence of those files is useful for detecting where pgindent
failed. Without them there's not an easy signal.
Hard disagree. I don't think BAK files serve that purpose well, and
they should be removed always imo (or possibly not even created in the
first place). There are two much better signals for detecting whether
and how pgindent fails:
1. stderr of pgindent
2. exit code of pgindent
You fail to get my point. When doing a full-tree run, it's not enough
to get a binary pass/fail signal: it's necessary to know which files
pgindent failed on, so you can go look at them and fix them. IMO
the .BAK files are actually quite well adapted for this, because you
can go fix the first failing file, remove its .BAK file, and then the
other .BAK files are still there to remind you what else to look at.
pgindent's exit code is utterly inadequate for that. And even if it
prints just what you need to know on stderr, that's transient data
that has probably scrolled off your terminal window by the time you
finished with the first problem.
Is that perfect? Hardly; I can definitely think of better UX
experiences. But it beats having zero bread-crumbs, which is where
Peter's patch would leave us. If you want to get rid of the .BAK
files, provide a superior substitute *first*.
regards, tom lane
On Mon, 28 Sept 2026 at 05:25, Tom Lane <tgl@sss.pgh.pa.us> wrote:
And even if it
prints just what you need to know on stderr, that's transient data
that has probably scrolled off your terminal window by the time you
finished with the first problem.
That seems easily solved by piping stderr to a file (e.g. using tee):
src/tools/pgindent/pgindent . 2>| tee pgindent-errors.log
Another option is to simply rerun pgindent again after fixing a file
to get the rest of the failing files.
I'm personally much more annoyed by a bunch of random garbage files
being created in my working directory than by having to do either of
those when I want to run pgindent on my full tree.
On 24.09.26 17:43, Nazir Bilal Yavuz wrote:
On Thu, 24 Sept 2026 at 17:29, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Peter Eisentraut <peter@eisentraut.org> writes:
A subproblem is that if pgindent fails on many files, it doesn't clean
up after itself but leaves many pgsrc*.BAK files lying around. The
first patch fixes that.I object to this patch. src/tools/pgindent/README documents that
the presence of those files is useful for detecting where pgindent
failed. Without them there's not an easy signal.I remember being frustrated with too many pgsrc*.BAK files but that
was when pgindent processed the build folder. I think we might not
need 0001 after 0003.
Here is an updated patch that just implements automatic skipping of
build directories. It's quite compact and seems universally useful.
Hi,
On Thu, 1 Oct 2026 at 10:04, Peter Eisentraut <peter@eisentraut.org> wrote:
On 24.09.26 17:43, Nazir Bilal Yavuz wrote:
On Thu, 24 Sept 2026 at 17:29, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Peter Eisentraut <peter@eisentraut.org> writes:
A subproblem is that if pgindent fails on many files, it doesn't clean
up after itself but leaves many pgsrc*.BAK files lying around. The
first patch fixes that.I object to this patch. src/tools/pgindent/README documents that
the presence of those files is useful for detecting where pgindent
failed. Without them there's not an easy signal.I remember being frustrated with too many pgsrc*.BAK files but that
was when pgindent processed the build folder. I think we might not
need 0001 after 0003.Here is an updated patch that just implements automatic skipping of
build directories. It's quite compact and seems universally useful.
I agree. I tested v2 with both meson and autoconf and it works
correctly, v2 LGTM.
--
Regards,
Nazir Bilal Yavuz
Microsoft
On 01.10.26 11:00, Nazir Bilal Yavuz wrote:
Hi,
On Thu, 1 Oct 2026 at 10:04, Peter Eisentraut <peter@eisentraut.org> wrote:
On 24.09.26 17:43, Nazir Bilal Yavuz wrote:
On Thu, 24 Sept 2026 at 17:29, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Peter Eisentraut <peter@eisentraut.org> writes:
A subproblem is that if pgindent fails on many files, it doesn't clean
up after itself but leaves many pgsrc*.BAK files lying around. The
first patch fixes that.I object to this patch. src/tools/pgindent/README documents that
the presence of those files is useful for detecting where pgindent
failed. Without them there's not an easy signal.I remember being frustrated with too many pgsrc*.BAK files but that
was when pgindent processed the build folder. I think we might not
need 0001 after 0003.Here is an updated patch that just implements automatic skipping of
build directories. It's quite compact and seems universally useful.I agree. I tested v2 with both meson and autoconf and it works
correctly, v2 LGTM.
Committed, thanks.