Subject: Re: run pgindent on a regular basis / scripted manner

From: Andres Freund <[email-masked]>
Date: Sat, 4 Feb 2023 03:34:53 -0800

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 <[email-masked]> 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

---

From: Andrew Dunstan <[email-masked]>
Date: Sat, 4 Feb 2023 09:20:26 -0500

> 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

---

From: Tom Lane <[email-masked]>
Date: Sat, 04 Feb 2023 11:07:59 -0500

> 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

---

From: Justin Pryzby <[email-masked]>
Date: Sat, 4 Feb 2023 11:11:01 -0600

> 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

---

From: Tom Lane <[email-masked]>
Date: Sat, 04 Feb 2023 12:37:11 -0500

> 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

---

From: Andrew Dunstan <[email-masked]>
Date: Sun, 5 Feb 2023 09:29:08 -0500

On 2023-02-04 Sa 09:20, Andrew Dunstan wrote:
> 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

---

From: Robert Haas <[email-masked]>
Date: Mon, 6 Feb 2023 09:40:02 -0500

> 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

---

From: Tom Lane <[email-masked]>
Date: Mon, 06 Feb 2023 10:16:05 -0500

> 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

---

From: Andrew Dunstan <[email-masked]>
Date: Mon, 6 Feb 2023 10:21:07 -0500

On 2023-02-06 Mo 09:40, Robert Haas wrote:
> On Sat, Feb 4, 2023 at 12:37 PM Tom Lane<[email-masked]>  wrote:

Good suggestions. 1 and 3 seem fairly straightforward. I'll start on 
those, and look into 2.

cheers

andrew

---

From: Robert Haas <[email-masked]>
Date: Mon, 6 Feb 2023 10:35:57 -0500

> is going to be a ton easier than doing it inside pg_bsd_indent.

+1.

---

From: Robert Haas <[email-masked]>
Date: Mon, 6 Feb 2023 10:36:18 -0500

On Mon, Feb 6, 2023 at 10:21 AM Andrew Dunstan <[email-masked]> wrote:
> Good suggestions. 1 and 3 seem fairly straightforward. I'll start on those, and look into 2.

Thanks!

---

From: Andrew Dunstan <[email-masked]>
Date: Mon, 6 Feb 2023 12:03:47 -0500

On 2023-02-06 Mo 10:36, Robert Haas wrote:
> On Mon, Feb 6, 2023 at 10:21 AM Andrew Dunstan<[email-masked]>  wrote:

Here's a quick patch for 1 and 3. Would also need to adjust the docco.

cheers

andrew

---

From: Andrew Dunstan <[email-masked]>
Date: Mon, 6 Feb 2023 12:53:44 -0500

> 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
diff --git a/src/tools/pgindent/pgindent b/src/tools/pgindent/pgindent
index 56640e576a..ca329747a4 100755
--- a/src/tools/pgindent/pgindent
+++ b/src/tools/pgindent/pgindent
@@ -23,12 +23,14 @@ my $devnull = File::Spec->devnull;
 
 my ($typedefs_file, $typedef_str, $code_base,
 	@excludes,      $indent,      $build,
-	$show_diff,     $silent_diff, $help);
+	$show_diff,     $silent_diff, $help,
+	@commits,);
 
 $help = 0;
 
 my %options = (
 	"help"               => \$help,
+	"commit=s"           => \@commits,
 	"typedefs=s"         => \$typedefs_file,
 	"list-of-typedefs=s" => \$typedef_str,
 	"code-base=s"        => \$code_base,
@@ -44,6 +46,9 @@ usage() if $help;
 usage("Cannot have both --silent-diff and --show-diff")
   if $silent_diff && $show_diff;
 
+usage("Cannot use --commit with --code-base or command line file list")
+  if (@commits && ($code_base || @ARGV));
+
 run_build($code_base) if ($build);
 
 # command line option wins, then environment (which is how --build sets it) ,
@@ -388,6 +393,7 @@ Usage:
 pgindent [OPTION]... [FILE]...
 Options:
 	--help                  show this message and quit
+    --commit=gitref         use files modified by the named commit
 	--typedefs=FILE         file containing a list of typedefs
 	--list-of-typedefs=STR  string containing typedefs, space separated
 	--code-base=DIR         path to the base of PostgreSQL source code
@@ -396,7 +402,7 @@ Options:
 	--build                 build the pg_bsd_indent program
 	--show-diff             show the changes that would be made
 	--silent-diff           exit with status 2 if any changes would be made
-The --excludes option can be given more than once.
+The --excludes and --commit options can be given more than once.
 EOF
 	if ($help)
 	{
@@ -412,27 +418,38 @@ EOF
 
 # main
 
-# get the list of files under code base, if it's set
-File::Find::find(
-	{
-		wanted => sub {
-			my ($dev, $ino, $mode, $nlink, $uid, $gid);
-			(($dev, $ino, $mode, $nlink, $uid, $gid) = lstat($_))
-			  && -f _
-			  && /^.*\.[ch]\z/s
-			  && push(@files, $File::Find::name);
-		}
-	},
-	$code_base) if $code_base;
-
 $filtered_typedefs_fh = load_typedefs();
 
 check_indent();
 
-# any non-option arguments are files to be processed
-push(@files, @ARGV);
+build_clean($code_base) if $build;
 
-# the exclude list applies to command line arguments as well as found files
+my $wanted = sub
+{
+	my ($dev, $ino, $mode, $nlink, $uid, $gid);
+	(($dev, $ino, $mode, $nlink, $uid, $gid) = lstat($_))
+	  && -f _
+	  && /^.*\.[ch]\z/s
+	  && push(@files, $File::Find::name);
+};
+
+# get the list of files under code base, if it's set
+File::Find::find({wanted => $wanted }, $code_base) if $code_base;
+
+# any non-option arguments are files or directories to be processed
+File::Find::find({wanted => $wanted}, @ARGV) if @ARGV;
+
+# process named commits by comparing each with their immediate ancestor
+foreach my $commit (@commits)
+{
+	my $prev="$commit~";
+	my @affected=`git diff-tree --no-commit-id --name-only -r $commit $prev`;
+	die "git error" if $?;
+	chomp(@affected);
+	push(@files,@affected);
+}
+
+# remove excluded files from the file list
 process_exclude();
 
 foreach my $source_filename (@files)
@@ -481,6 +498,4 @@ foreach my $source_filename (@files)
 	}
 }
 
-build_clean($code_base) if $build;
-
 exit 0;

---

From: Andrew Dunstan <[email-masked]>
Date: Tue, 7 Feb 2023 08:17:49 -0500

> up for commit (e.g. pgindent --cached).

My git-fu is probably not all that it should be. I think we could 
possibly get at this list of files by running

   git status --porcelain --untracked-files=no --ignored=no -- .

And then your --dirty list would be lines beginning with ' M' while your 
--cached list would be lines beginning with 'A[ M]'

Does that seem plausible?

cheers

andrew

---

From: Justin Pryzby <[email-masked]>
Date: Tue, 7 Feb 2023 09:25:42 -0600

> that).  I could see inventing a --postgresql shortcut switch perhaps.

Or you could add ./.indent.pro, or ./src/tools/indent.profile for it to
read.

> But it's not clear to me why you're allergic to the perl wrapper?

My allergy is to the totality of the process, not to the perl component.
It's a bit weird to enforce a coding style that no upstream indent tool
supports.  But what's weirder is that, *having forked the indent tool*,
it still doesn't implement the desired style, and the perl wrapper tries
to work around that.

It would be more reasonable if the forked C program knew how to handle
the stuff for which the perl script currently has kludges to munge the
source code before indenting and then un-munging afterwards.

Or if the indentation were handled by the (or a) perl script itself.

Or if the perl script handled everything that an unpatched "ident"
didn't handle, rather than some things but not others, demanding use of
a patched indent as well as a perl wrapper (not just for looping around
files and fancy high-level shortcuts like indenting staged files).

On the one hand, "indent" ought to handle all the source-munging stuff.
On the other hand, it'd be better to use an unpatched indent tool.  The
current way is the worst of both worlds.

Currently, the perl wrapper supports the "/* -"-style comments that
postgres wants to use (why?) by munging the source code.  That could be
supported in pg-bsd-indent with a one line change.  I think an even
better option would be to change postgres' C files to use "/*-" without
a space, which requires neither perl munging nor patching indent.

On a less critical note, I wonder if it's a good idea to import
pgbsdindent as a git "submodule".

-- 
Justin

---

From: Tom Lane <[email-masked]>
Date: Tue, 07 Feb 2023 10:46:01 -0500

> source code before indenting and then un-munging afterwards.

[ shrug... ]  If you want to put cycles into that, nobody is stopping
you.  For me, it sounds like make-work.

			regards, tom lane

---

From: Andrew Dunstan <[email-masked]>
Date: Tue, 7 Feb 2023 10:46:51 -0500

On 2023-02-07 Tu 10:25, Justin Pryzby wrote:
>> Justin Pryzby<[email-masked]>  writes:

Historically we used to do a heck of a lot more in pgindent that is 
currently done in the pre_indent and post_indent functions. If you want 
to spend time implementing that logic in pg_bsd_indent so we can remove 
the remaining bits of that processing then go for it.

> pgbsdindent as a git "submodule".

Meh, git submodules can be a pain in the neck in my limited experience. 
I'd rather steer clear.

cheers

andrew

---

From: Robert Haas <[email-masked]>
Date: Tue, 7 Feb 2023 11:10:56 -0500

On Tue, Feb 7, 2023 at 8:17 AM Andrew Dunstan <[email-masked]> wrote:
> My git-fu is probably not all that it should be. I think we could possibly get at this list of files by running
>
> And then your --dirty list would be lines beginning with ' M' while your --cached list would be lines beginning with 'A[ M]'
>

I don't know if that works or not, but it does seem plausible, at
least. My idea would have been to use the --name-status option, which
works for both git diff and git show. You just look and see which
lines in the output start with M or A and then take the file names
from those lines.

So to indent files that are dirty, you would look at:

git diff --name-status

For what's cached:

git diff --name-status --cached

For the combination of the two:

git diff --name-status HEAD

For a prior commit:

git show --name-status $COMMITID

---

From: Jelte Fennema <[email-masked]>
Date: Tue, 7 Feb 2023 17:32:30 +0100

> lines in the output start with M or A and then take the file names

If you add `--diff-filter=ACMR`, then git diff/show will only show
Added, Copied, Modified, and Renamed files.

The pre-commit hook that Andrew added to the wiki uses that in
combination with --name-only to get the list of files that you want to
check on commit:
https://wiki.postgresql.org/wiki/Working_with_Git#Using_git_hooks

---

From: Robert Haas <[email-masked]>
Date: Tue, 7 Feb 2023 11:57:31 -0500

> https://wiki.postgresql.org/wiki/Working_with_Git#Using_git_hooks

Thanks, that sounds nicer than what I suggested.

---

From: Jelte Fennema <[email-masked]>
Date: Tue, 7 Feb 2023 18:21:19 +0100

> Here's a quick patch for 1 and 3. Would also need to adjust the docco.

When supplying the --commit flag it still formats all files for me. I
was able to fix that by replacing:
# no non-option arguments given. so do everything in the current directory
$code_base ||= '.' unless @ARGV;

with:
# no files, dirs or commits given. so do everything in the current directory
$code_base ||= '.' unless @ARGV || @commits;

Does the code-base flag still make sense if you can simply pass a
directory as regular args now?

---

From: Andrew Dunstan <[email-masked]>
Date: Wed, 8 Feb 2023 07:41:59 -0500

> $code_base ||= '.' unless @ARGV || @commits;

Yeah, thanks for testing. Here's a new patch with that change and the 
comment adjusted.

> directory as regular args now?

Probably not. I'll look into removing it.

cheers

andrew

index 56640e576a..34fb7d604d 100755

 $help = 0;
 
 my %options = (
 	"help"               => \$help,

 # command line option wins, then environment (which is how --build sets it) ,
@@ -53,8 +58,9 @@ $typedefs_file ||= $ENV{PGTYPEDEFS};
 # build mode sets PGINDENT
 $indent ||= $ENV{PGINDENT} || $ENV{INDENT} || "pg_bsd_indent";
 
-# no non-option arguments given. so do everything in the current directory
-$code_base ||= '.' unless @ARGV;
+# if no non-option arguments or commits are given, default to looking in the
+# current directory
+$code_base ||= '.' unless (@ARGV || @commits);
 
 my $sourcedir = locate_sourcedir();
 
@@ -388,6 +394,7 @@ Usage:
 pgindent [OPTION]... [FILE]...
 Options:
 	--help                  show this message and quit
+    --commit=gitref         use files modified by the named commit
 	--typedefs=FILE         file containing a list of typedefs
 	--list-of-typedefs=STR  string containing typedefs, space separated
 	--code-base=DIR         path to the base of PostgreSQL source code
@@ -396,7 +403,7 @@ Options:
 	--build                 build the pg_bsd_indent program
 	--show-diff             show the changes that would be made
 	--silent-diff           exit with status 2 if any changes would be made
-The --excludes option can be given more than once.
+The --excludes and --commit options can be given more than once.
 EOF
 	if ($help)
 	{
@@ -412,27 +419,38 @@ EOF
 
 # main

 check_indent();

 
 foreach my $source_filename (@files)
@@ -481,6 +499,4 @@ foreach my $source_filename (@files)
 	}
 }

---

From: Andrew Dunstan <[email-masked]>
Date: Wed, 8 Feb 2023 08:27:07 -0500

> Probably not. I'll look into removing it.

What we should probably do is remove all the build stuff along with 
$code_base. It dates back to the time when I developed this as an out of 
tree replacement for the old pgindent, and is just basically wasted 
space now. After I get done with the current round of enhancements I'll 
reorganize the script and get rid of things we don't need any more.

cheers

andrew

---

From: Jelte Fennema <[email-masked]>
Date: Wed, 8 Feb 2023 18:06:22 +0100

With the new patch --commit works as expected for me now. And sounds
good to up the script a bit afterwards.

On Wed, 8 Feb 2023 at 14:27, Andrew Dunstan <[email-masked]> wrote:
> What we should probably do is remove all the build stuff along with $code_base. It dates back to the time when I developed this as an out of tree replacement for the old pgindent, and is just basically wasted space now. After I get done with the current round of enhancements I'll reorganize the script and get rid of things we don't need any more.
>
>
> cheers
>
>
> andrew
>
> --
> Andrew Dunstan
> EDB: https://www.enterprisedb.com

---

From: Andrew Dunstan <[email-masked]>
Date: Wed, 8 Feb 2023 17:09:45 -0500

> good to up the script a bit afterwards.

Thanks, I have committed this. Still looking at Robert's other request.

cheers

andrew

---

From: "Shinoda, Noriyoshi (PN Japan FSIP)" <[email-masked]>
Date: Thu, 9 Feb 2023 02:29:41 +0000

Hi,
I tried the committed pgindent.
The attached small patch changes spaces in the usage message to tabs.
Options other than --commit start with a tab.

Regards,
Noriyoshi Shinoda

From: Andrew Dunstan <[email-masked]>
Sent: Thursday, February 9, 2023 7:10 AM
To: Jelte Fennema <[email-masked]>
Cc: Robert Haas <[email-masked]>; Tom Lane <[email-masked]>; Justin Pryzby <[email-masked]>; Andres Freund <[email-masked]>; Noah Misch <[email-masked]>; Peter Geoghegan <[email-masked]>; Bruce Momjian <[email-masked]>; Magnus Hagander <[email-masked]>; Alvaro Herrera <[email-masked]>; Stephen Frost <[email-masked]>; Jesse Zhang <[email-masked]>; [email-masked]
Subject: Re: run pgindent on a regular basis / scripted manner

cheers

andrew

--

Andrew Dunstan

EDB: https://www.enterprisedb.com<https://www.enterprisedb.com>

---

From: Andrew Dunstan <[email-masked]>
Date: Thu, 9 Feb 2023 13:34:34 -0500

> Options other than --commit start with a tab.

Thanks, pushed.

cheers

andrew

---

From: "[email-masked]" <[email-masked]>
Date: Fri, 10 Feb 2023 02:37:40 +0000

Hi,

I tried the new option --commit and found that it seems to try to indent files
which are deleted in the specified commit and reports an error.
 
cannot open file "src/backend/access/brin/brin.c": No such file or directory

It looks we should filter such files.

Regards,
Shi Yu

---

From: Jelte Fennema <[email-masked]>
Date: Fri, 10 Feb 2023 10:25:44 +0100

Ah yes, I had seen that when I read the initial --commit patch but
then forgot about it when the flag didn't work at all when I tried it.

Attached is a patch that fixes the issue. And also implements the
--dirty and --staged flags in pgindent that Robert Haas requested.

---

From: Andrew Dunstan <[email-masked]>
Date: Fri, 10 Feb 2023 09:26:50 -0500

On 2023-02-07 Tu 11:32, Jelte Fennema wrote:
> On Tue, 7 Feb 2023 at 17:11, Robert Haas<[email-masked]>  wrote:

OK, here's a patch based on Robert's and Jelte's ideas.

cheers

andrew

--
Andrew Dunstan
EDB:https://www.enterprisedb.com
diff --git a/src/tools/pgindent/pgindent b/src/tools/pgindent/pgindent
index 07970f758c..c9f42302c9 100755
--- a/src/tools/pgindent/pgindent
+++ b/src/tools/pgindent/pgindent
@@ -24,13 +24,15 @@ my $devnull = File::Spec->devnull;
 my ($typedefs_file, $typedef_str, $code_base,
 	@excludes,      $indent,      $build,
 	$show_diff,     $silent_diff, $help,
-	@commits,);
+	@commits, $dirty, $cached);
 
 $help = 0;
 
 my %options = (
 	"help"               => \$help,
 	"commit=s"           => \@commits,
+	"dirty"              => \$dirty,
+	"cached"             => \$cached,
@@ -46,8 +48,11 @@ usage() if $help;
 
-usage("Cannot use --commit with --code-base or command line file list")
-  if (@commits && ($code_base || @ARGV));
+usage("Cannot use --commit, --dirty or --cached with --code-base or command line file list")
+  if (($dirty || $cached || @commits) && ($code_base || @ARGV));
+
+usage( "Cannot use --dirty or --cached with --commit")
+  if (($dirty || $cached) && @commits);

@@ -395,6 +400,8 @@ pgindent [OPTION]... [FILE]...
 Options:
 	--help                  show this message and quit
 	--commit=gitref         use files modified by the named commit
+	--dirty                 use modified files under current directory
+	--cached                use staged files under current directory
 	--typedefs=FILE         file containing a list of typedefs
 	--list-of-typedefs=STR  string containing typedefs, space separated
 	--code-base=DIR         path to the base of PostgreSQL source code
@@ -450,6 +457,19 @@ foreach my $commit (@commits)
 	push(@files,@affected);
 }
 
+# look for dirty / cached files under the CWD
+if ($dirty || $cached)
+{
+	my $gitcmd = "git diff --diff-filter=ACMR --name-only";
+	$gitcmd .= " --cached" if ($cached && !$dirty);
+	$gitcmd .= " HEAD" if ($dirty && $cached);
+	$gitcmd .= " -- .";
+	my @affected=`$gitcmd`;
+	die "git error" if $?;
+	chomp(@affected);
+	push(@files,@affected);
+}
+
 # remove excluded files from the file list
 process_exclude();

---

From: Andrew Dunstan <[email-masked]>
Date: Fri, 10 Feb 2023 10:21:49 -0500

> --dirty and --staged flags in pgindent that Robert Haas requested.

[please don't top-post]

I don't think just adding a diff filter is really a sufficient fix. The 
file might have been deleted since the commit(s) in question. Here's a 
more general fix for missing files.

cheers

andrew

--
Andrew Dunstan
EDB:https://www.enterprisedb.com
diff --git a/src/tools/pgindent/pgindent b/src/tools/pgindent/pgindent
index 07970f758c..64284e7ea2 100755
--- a/src/tools/pgindent/pgindent
+++ b/src/tools/pgindent/pgindent
@@ -444,7 +444,7 @@ File::Find::find({wanted => $wanted}, @ARGV) if @ARGV;
 foreach my $commit (@commits)
 {
 	my $prev="$commit~";
-	my @affected=`git diff-tree --no-commit-id --name-only -r $commit $prev`;
+	my @affected=`git diff --diff-filter=ACMR --name-only $prev $commit`;
 	die "git error" if $?;
 	chomp(@affected);
 	push(@files,@affected);
@@ -458,6 +458,12 @@ foreach my $source_filename (@files)
 	# ignore anything that's not a .c or .h file
 	next unless $source_filename =~ /\.[ch]$/;
 
+	# don't try to indent a file that doesn't exist
+	unless (-f $source_filename)
+	{
+		warn "Could not find $source_filename";
+		next;
+	}
 	# Automatically ignore .c and .h files that correspond to a .y or .l
 	# file.  indent tends to get badly confused by Bison/flex output,
 	# and there's no value in indenting derived files anyway.

---

From: Andrew Dunstan <[email-masked]>
Date: Sun, 12 Feb 2023 09:16:25 -0500

On 2023-02-10 Fr 10:21, Andrew Dunstan wrote:
> I don't think just adding a diff filter is really a sufficient fix. 
> The file might have been deleted since the commit(s) in question. 
> Here's a more general fix for missing files.
>

OK, I've pushed this along with a check to make sure we only process 
each file once.

I'm not sure how much more I really want to do here. Given the way 
pgindent now processes command line arguments, maybe the best thing is 
for people to use that. Use of git aliases can help. Something like 
these for example

[alias]

     dirty = diff --name-only --diff-filter=ACMU -- .
     staged = diff --name-only --cached --diff-filter=ACMU -- .
     dstaged = diff --name-only --diff-filter=ACMU HEAD -- .

and then you could do

     pgindent `git dirty`

The only danger would be if there were no dirty files. Maybe we need a 
switch to inhibit using the current directory if there are no command 
line files.

Thoughts?

cheers

andrew

---

From: Tom Lane <[email-masked]>
Date: Sun, 12 Feb 2023 11:24:14 -0500

Andrew Dunstan <[email-masked]> writes:
> ... then you could do

It seems like "indent the whole tree" is about to become a minority
use-case.  Maybe instead of continuing to privilege that case, we
should say that it's invoked by some new switch like --all-files,
and without that only the stuff identified by command-line arguments
gets processed.

			regards, tom lane

---

From: Andrew Dunstan <[email-masked]>
Date: Sun, 12 Feb 2023 15:41:56 -0500

On 2023-02-12 Su 11:24, Tom Lane wrote:
> Andrew Dunstan<[email-masked]>  writes:
>> ... then you could do
>>       pgindent `git dirty`

I don't think we need --all-files. The attached gets rid of the build 
and code-base cruft, which is now in any case obsolete given we've put 
pg_bsd_indent in our code base. So the way to spell this instead of 
"pgindent --all-files" would be "pgindent ."

I added a warning if there are no files at all specified.

cheers

andrew

--
Andrew Dunstan
EDB:https://www.enterprisedb.com
diff --git a/src/tools/pgindent/pgindent b/src/tools/pgindent/pgindent
index a793971e07..b098cef02a 100755
--- a/src/tools/pgindent/pgindent
+++ b/src/tools/pgindent/pgindent
@@ -21,7 +21,7 @@ my $indent_opts =
 
 my $devnull = File::Spec->devnull;
 
-my ($typedefs_file, $typedef_str, $code_base,
+my ($typedefs_file, $typedef_str,
 	@excludes,      $indent,      $build,
 	$show_diff,     $silent_diff, $help,
 	@commits,);
@@ -33,10 +33,8 @@ my %options = (
 	"commit=s"           => \@commits,
 	"typedefs=s"         => \$typedefs_file,
 	"list-of-typedefs=s" => \$typedef_str,
-	"code-base=s"        => \$code_base,
 	"excludes=s"         => \@excludes,
 	"indent=s"           => \$indent,
-	"build"              => \$build,
 	"show-diff"          => \$show_diff,
 	"silent-diff"        => \$silent_diff,);
 GetOptions(%options) || usage("bad command line argument");
@@ -46,22 +44,16 @@ usage() if $help;
 usage("Cannot have both --silent-diff and --show-diff")
   if $silent_diff && $show_diff;
 
-usage("Cannot use --commit with --code-base or command line file list")
-  if (@commits && ($code_base || @ARGV));
+usage("Cannot use --commit with command line file list")
+  if (@commits && @ARGV);
 
-run_build($code_base) if ($build);
-
-# command line option wins, then environment (which is how --build sets it) ,
-# then locations. based on current dir, then default location
+# command line option wins, then environment, then locations based on current
+# dir, then default location
 $typedefs_file ||= $ENV{PGTYPEDEFS};
 
-# build mode sets PGINDENT
+# get indent location for environment or default
 $indent ||= $ENV{PGINDENT} || $ENV{INDENT} || "pg_bsd_indent";
 
-# if no non-option arguments or commits are given, default to looking in the
-# current directory
-$code_base ||= '.' unless (@ARGV || @commits);
-
 my $sourcedir = locate_sourcedir();
 
 # if it's the base of a postgres tree, we will exclude the files
@@ -121,8 +113,7 @@ sub check_indent
 sub locate_sourcedir
 {
 	# try fairly hard to locate the sourcedir
-	my $where = $code_base || '.';
-	my $sub = "$where/src/tools/pgindent";
+	my $sub = "./src/tools/pgindent";
 	return $sub if -d $sub;
 	# try to find it from an ancestor directory
 	$sub = "../src/tools/pgindent";
@@ -320,72 +311,6 @@ sub show_diff
 	return $diff;
 }
 
-sub run_build
-{
-	eval "use LWP::Simple;";    ## no critic (ProhibitStringyEval);
-
-	my $code_base = shift || '.';
-	my $save_dir = getcwd();
-
-	# look for the code root
-	foreach (1 .. 5)
-	{
-		last if -d "$code_base/src/tools/pgindent";
-		$code_base = "$code_base/..";
-	}
-
-	die "cannot locate src/tools/pgindent directory in \"$code_base\"\n"
-	  unless -d "$code_base/src/tools/pgindent";
-
-	chdir "$code_base/src/tools/pgindent";
-
-	my $typedefs_list_url =
-	  "https://buildfarm.postgresql.org/cgi-bin/typedefs.pl";
-
-	my $rv = getstore($typedefs_list_url, "tmp_typedefs.list");
-
-	die "cannot fetch typedefs list from $typedefs_list_url\n"
-	  unless is_success($rv);
-
-	$ENV{PGTYPEDEFS} = abs_path('tmp_typedefs.list');
-
-	my $indentrepo = "https://git.postgresql.org/git/pg_bsd_indent.git";
-	system("git clone $indentrepo >$devnull 2>&1");
-	die "could not fetch pg_bsd_indent sources from $indentrepo\n"
-	  unless $? == 0;
-
-	chdir "pg_bsd_indent" || die;
-	system("make all check >$devnull");
-	die "could not build pg_bsd_indent from source\n"
-	  unless $? == 0;
-
-	$ENV{PGINDENT} = abs_path('pg_bsd_indent');
-
-	chdir $save_dir;
-	return;
-}
-
-sub build_clean
-{
-	my $code_base = shift || '.';
-
-	# look for the code root
-	foreach (1 .. 5)
-	{
-		last if -d "$code_base/src/tools/pgindent";
-		$code_base = "$code_base/..";
-	}
-
-	die "cannot locate src/tools/pgindent directory in \"$code_base\"\n"
-	  unless -d "$code_base/src/tools/pgindent";
-
-	chdir "$code_base";
-
-	system("rm -rf src/tools/pgindent/pg_bsd_indent");
-	system("rm -f src/tools/pgindent/tmp_typedefs.list");
-	return;
-}
-
 sub usage
 {
 	my $message  = shift;
@@ -397,10 +322,8 @@ Options:
 	--commit=gitref         use files modified by the named commit
 	--typedefs=FILE         file containing a list of typedefs
 	--list-of-typedefs=STR  string containing typedefs, space separated
-	--code-base=DIR         path to the base of PostgreSQL source code
 	--excludes=PATH         file containing list of filename patterns to ignore
 	--indent=PATH           path to pg_bsd_indent program
-	--build                 build the pg_bsd_indent program
 	--show-diff             show the changes that would be made
 	--silent-diff           exit with status 2 if any changes would be made
 The --excludes and --commit options can be given more than once.
@@ -423,8 +346,6 @@ $filtered_typedefs_fh = load_typedefs();
 
 check_indent();
 
-build_clean($code_base) if $build;
-
 my $wanted = sub
 {
 	my ($dev, $ino, $mode, $nlink, $uid, $gid);
@@ -434,9 +355,6 @@ my $wanted = sub
 	  && push(@files, $File::Find::name);
 };
 
-# get the list of files under code base, if it's set
-File::Find::find({wanted => $wanted }, $code_base) if $code_base;
-
 # any non-option arguments are files or directories to be processed
 File::Find::find({wanted => $wanted}, @ARGV) if @ARGV;
 
@@ -450,6 +368,8 @@ foreach my $commit (@commits)
 	push(@files,@affected);
 }
 
+warn "No files to process" unless @files;
+
 # remove excluded files from the file list
 process_exclude();
 
diff --git a/src/tools/pgindent/pgindent.man b/src/tools/pgindent/pgindent.man
index 7406794ba3..fe411ee699 100644
--- a/src/tools/pgindent/pgindent.man
+++ b/src/tools/pgindent/pgindent.man
@@ -8,14 +8,9 @@ You can see all the options by running:
 	pgindent --help
 
 In its simplest form, if all the required objects are installed, simply run
-it without any parameters at the top of the source tree you want to process.
+it at the top of the source tree you want to process like this:
 
-	pgindent
-
-If you don't have all the requirements installed, pgindent will fetch and build
-them for you, if you're in a PostgreSQL source tree:
-
-	pgindent --build
+	pgindent .
 
 If your pg_bsd_indent program is not installed in your path, you can specify
 it by setting the environment variable INDENT, or PGINDENT, or by giving the
@@ -28,9 +23,6 @@ specified using the PGTYPEDEFS environment variable, or via the command line
 --typedefs option. If neither is used, it will look for it within the
 current source tree, or in /usr/local/etc/typedefs.list.
 
-If you want to indent a source tree other than the current working directory,
-you can specify it via the --code-base command line option.
-
 We don't want to indent certain files in the PostgreSQL source. pgindent
 will honor a file containing a list of patterns of files to avoid. This
 file can be specified using the --excludes command line option. If indenting

---

From: Justin Pryzby <[email-masked]>
Date: Sun, 12 Feb 2023 14:59:36 -0600

> and without that only the stuff identified by command-line arguments

It seems like if pgindent knows about git, it ought to process only
tracked files.  Then, it wouldn't need to manually exclude generated
files, and it wouldn't process vpath builds and who-knows-what else it
finds in CWD.

At least --commit doesn't seem to work when run outside of the root
source dir.

-- 
Justin

---

From: Tom Lane <[email-masked]>
Date: Sun, 12 Feb 2023 16:13:35 -0500

> "pgindent --all-files" would be "pgindent ."

Ah, of course.

> I added a warning if there are no files at all specified.

LGTM.

			regards, tom lane

---

From: Andrew Dunstan <[email-masked]>
Date: Mon, 13 Feb 2023 07:57:25 -0500

> files, and it wouldn't process vpath builds and who-knows-what else it

for vpath builds use an exclude file that excludes the vpath you use.

I don't really want restrict this to tracked files because it would mean 
you can't pgindent files before you `git add` them. And we would still 
need to do manual exclusion for some files that are tracked, e.g. the 
snowball files.

> At least --commit doesn't seem to work when run outside of the root

Yeah, I'll fix that, thanks for mentioning.

cheers

andrew

---

From: Andrew Dunstan <[email-masked]>
Date: Mon, 13 Feb 2023 08:27:37 -0500

Thanks, pushed.

cheers

andrew

---

From: Jelte Fennema <[email-masked]>
Date: Mon, 13 Feb 2023 15:02:39 +0100

On Sun, 12 Feb 2023 at 15:16, Andrew Dunstan <[email-masked]> wrote:
> I'm not sure how much more I really want to do here. Given the way pgindent now processes command line arguments, maybe the best thing is for people to use that. Use of git aliases can help. Something like these for example
>
>
> [alias]
>
> The only danger would be if there were no dirty files. Maybe we need a switch to inhibit using the current directory if there are no command line files.
>
>
> Thoughts?

I think indenting staged or dirty files is probably the most common
operation that people want to do with pgindent. So I think that having
dedicated flags makes sense. I agree that it's not strictly necessary
and git aliases help a lot. But the git aliases require you to set
them up. To me making the most common operation as easy as possible to
do, seems worth the few extra lines to pgindent.

Sidenote: You mentioned untracked files in another email. I think that
the --dirty flag should probably also include untracked files. A
command to do so is: git ls-files --others --exclude-standard

---

From: Andrew Dunstan <[email-masked]>
Date: Mon, 13 Feb 2023 11:46:59 -0500

On 2023-02-13 Mo 09:02, Jelte Fennema wrote:
> On Sun, 12 Feb 2023 at 15:16, Andrew Dunstan<[email-masked]>  wrote:

OK, but I'd like to hear from more people about what they want. 
Experience tells me that making assumptions about how people work is not 
a good idea. I doubt anyone's work pattern is like mine. I don't want to 
implement an option that three people are going to use.

> command to do so is: git ls-files --others --exclude-standard

Thanks for the info.

cheers

andrew

---

From: Jelte Fennema <[email-masked]>
Date: Mon, 13 Feb 2023 19:29:19 +0100

On Mon, 13 Feb 2023 at 17:47, Andrew Dunstan <[email-masked]> wrote:
> OK, but I'd like to hear from more people about what they want. Experience tells me that making assumptions about how people work is not a good idea. I doubt anyone's work pattern is like mine. I don't want to implement an option that three people are going to use.

In the general case I agree with you. But in this specific case I
don't. To me the whole point of this email thread is to nudge people
towards indenting the changes that they are committing. Thus indenting
those changes (either before or after adding) is the workflow that we
want to make as easy as possible. Because even if it's not people
their current workflow, by adding the flag it hopefully becomes their
workflow, because it's so easy to use. So my point is we want to
remove as few hurdles as possible for people to indent their changes
(and setting up git aliases or pre-commit hooks are all hurdles).

---

From: Justin Pryzby <[email-masked]>
Date: Wed, 15 Feb 2023 12:45:52 -0600

On Mon, Feb 13, 2023 at 07:57:25AM -0500, Andrew Dunstan wrote:
> I don't really want restrict this to tracked files because it would mean you
> can't pgindent files before you `git add` them.

I think you'd allow indenting files which were either tracked *or*
specified on the command line.

Also, it makes a more sense to "add" the file before indenting it, to
allow checking the output and remove unrelated changes.  So that doesn't
seem to me like a restriction of any significance.

But I would never want to indent an untracked file unless I specified
it.

-- 
Justin

---

From: Jelte Fennema <[email-masked]>
Date: Wed, 15 Feb 2023 22:00:41 +0100

> seem to me like a restriction of any significance.

For my workflow it would be the same, but afaik there's two ways that
people commonly use git (mine is 1):
1. Adding changes/files to the staging area using and then committing
those changes:
    git add (-p)/emacs magit/some other editor integration
2. Just add everything that's changed and commit all of it:
    git add -A + git commit/git commit -a

For workflow 1, a --staged/--cached flag would be enough IMHO. But
that's not at all helpful for workflow 2. That's why I proposed
--uncommitted too, to make indenting easier for workflow 2.

> But I would never want to indent an untracked file unless I specified

Would the --uncommitted flag I proposed be enough of an explicit way
of specifying that you want to indent untracked files?

---

From: "[email-masked]" <[email-masked]>
Date: Thu, 16 Feb 2023 08:26:07 +0000

Hi,

Commit #068a243b7 supported directories to be non-option arguments of pgindent.
But the help text doesn't mention that. Should we update it? Attach a small
patch which did that.

Regards,
Shi Yu

---

From: Andrew Dunstan <[email-masked]>
Date: Thu, 16 Feb 2023 11:44:01 -0500

On 2023-02-16 Th 03:26, [email-masked] wrote:
> On Thu, Feb 9, 2023 6:10 AM Andrew Dunstan<[email-masked]>  wrote:

Thanks, pushed.

cheers

andrew

---

From: Jelte Fennema <[email-masked]>
Date: Fri, 21 Apr 2023 09:58:17 +0200

Now that the PG16 feature freeze happened I think it's time to bump
this thread again. As far as I remember everyone that responded (even
previously silent people) were themselves proponents of being more
strict around pgindent.

I think there's two things needed to actually start doing this:
1. We need to reindent the tree to create an indented baseline
2. We need some automation to complain about unindented code being committed

For 2 the upstream thread listed two approaches:
a. Install a pre-receive git hook on the git server that rejects
pushes to master that are not indented
b. Add a test suite that checks if the code is correctly indented, so
the build farm would complain about it. (Suggested by Peter E)

I think both a and b would work to achieve 2. But as Peter E said, b
indeed sounds like less of a divergence of the status quo. So my vote
would be for b. If that doesn't achieve 2 for some reason, or turns
out to have problems we can always change to a afterwards.

---

From: Michael Paquier <[email-masked]>
Date: Sat, 22 Apr 2023 17:50:22 +0900

On Fri, Apr 21, 2023 at 09:58:17AM +0200, Jelte Fennema wrote:
> would be for b.

FWIW, I think that there is value for both of them.  Anyway, isn't 'a'
exactly the same as 'b' in design?  Both require a build of
pg_bsd_indent, meaning that 'a' would also need to run an equivalent
of the regression test suite, but it would be actually costly
especially if pg_bsd_indent itself is patched.  I think that getting
more noisy on this matter with 'b' would be enough, but as an extra
PG_TEST_EXTRA for committers to set.

Such a test suite would need a dependency to the 'git' command itself,
which is not something that could be safely run in a release tarball,
in any case.
--
Michael

---

From: Andrew Dunstan <[email-masked]>
Date: Sat, 22 Apr 2023 07:42:36 -0400

> which is not something that could be safely run in a release tarball,

Perhaps we should start with a buildfarm module, which would run 
pg_indent --show-diff. That would only need to run on one animal, so a 
failure wouldn't send the whole buildfarm red. This would be pretty easy 
to do.

cheers

andrew

---

From: Michael Paquier <[email-masked]>
Date: Sat, 22 Apr 2023 21:10:18 +0900

On Sat, Apr 22, 2023 at 07:42:36AM -0400, Andrew Dunstan wrote:
> Perhaps we should start with a buildfarm module, which would run pg_indent
> --show-diff.

Nice, I didn't know this one and it has been mentioned a bit on this
thread.  Indeed, it is possible to just rely on that.
--
Michael

---

From: Magnus Hagander <[email-masked]>
Date: Sat, 22 Apr 2023 14:47:12 +0200

On Sat, Apr 22, 2023 at 1:42 PM Andrew Dunstan <[email-masked]> wrote:
> Perhaps we should start with a buildfarm module, which would run pg_indent --show-diff. That would only need to run on one animal, so a failure wouldn't send the whole buildfarm red. This would be pretty easy to do.

Just to be clear, you guys are aware we  already have a git repo
that's supposed to track "head + pg_indent" at
https://git.postgresql.org/gitweb/?p=postgresql-pgindent.git;a=shortlog;h=refs/heads/master-pgindent
right?

I see it is currently not working and this has not been noticed by
anyone, so I guess it kind of indicates nobody is using it today. The
reason appears to be that it uses pg_bsd_indent that's in our apt
repos and that's 2.1.1 and not 2.1.2 at this point. But if this is a
service that would actually be useful, this could certainly be ficked
pretty easy.

But bottom line is that if pgindent is as predictable as it should be,
it might be easier to use that one central place that already does it
rather than have to build a buildfarm module?

//Magnus

---

From: Andrew Dunstan <[email-masked]>
Date: Sat, 22 Apr 2023 10:12:06 -0400

On 2023-04-22 Sa 08:47, Magnus Hagander wrote:
> On Sat, Apr 22, 2023 at 1:42 PM Andrew Dunstan<[email-masked]>  wrote:
>>

Now that pg_bsd_indent is in the core code why not just use that?

Happy if you can make something work without further effort on my part :-)

cheers

andew

---

From: Tom Lane <[email-masked]>
Date: Sat, 22 Apr 2023 10:24:13 -0400

>> indeed sounds like less of a divergence of the status quo. So my vote

I am absolutely against a pre-receive hook on gitmaster.  A buildfarm
check seems far more appropriate.

			regards, tom lane

---

From: Tom Lane <[email-masked]>
Date: Sat, 22 Apr 2023 10:39:48 -0400

> 1. We need to reindent the tree to create an indented baseline

As far as (1) goes, I've been holding off on that because there
are some large patches that still seem in danger of getting
reverted, notably 2489d76c4 and follow-ups.  A pgindent run
would change any such reversions from being mechanical into
possibly a fair bit of work.  We still have a couple of weeks
before it's necessary to make such decisions, so I don't want
to do the pgindent run before that.

Another obstacle in the way of (1) is that there was some discussion
of changing perltidy version and/or options.  But I don't believe
we have a final proposal on that, much less committed code.

			regards, tom lane

---

From: Andrew Dunstan <[email-masked]>
Date: Sat, 22 Apr 2023 11:10:12 -0400

> we have a final proposal on that, much less committed code.

Well, I posted a fairly concrete suggestion with an example patch 
upthread at

<https://www.postgresql.org/message-id/47011581-ddec-1a87-6828-6edfabe6b7b6%40dunslane.net>

I still think that's worth doing.

cheers

andrew

---

From: Tom Lane <[email-masked]>
Date: Sat, 22 Apr 2023 11:37:08 -0400

> I still think that's worth doing.

OK, so plan is (a) update perltidyrc to add --valign-exclusion-list,
(b) adjust pgindent/README to recommend perltidy version 20221112.

Questions:

* I see that there's now a 20230309 release, should we consider that
instead?

* emacs.samples provides pgsql-perl-style that claims to match
perltidy's rules.  Does that need any adjustment?  I don't see
anything in it that looks relevant, but I'm not terribly familiar
with emacs' Perl formatting options.

			regards, tom lane

---

From: Andrew Dunstan <[email-masked]>
Date: Sat, 22 Apr 2023 15:52:24 -0400

> * I see that there's now a 20230309 release, should we consider that

A test I just ran gave identical results to those from 20221112

> with emacs' Perl formatting options.

At least w.r.t. the vertical alignment issue, AFAICT the emacs style 
does not attempt to align anything vertically except the first non-space 
thing on the line. So if anything, by abandoning a lot of vertical 
alignment it would actually be closer to what the sample emacs style does.

The great advantage of not doing this alignment is that there is far 
less danger of perltidy trying to realign lines that have not in fact 
changed, because some nearby line has changed. So we'd have a good deal 
less pointless churn.

cheers

andrew

---

From: Tom Lane <[email-masked]>
Date: Sat, 22 Apr 2023 15:58:13 -0400

> A test I just ran gave identical results to those from 20221112

Cool, let's use perltidy 20230309 then.

> changed, because some nearby line has changed. So we'd have a good deal 

Yes, exactly.

			regards, tom lane

---

From: Andrew Dunstan <[email-masked]>
Date: Sun, 23 Apr 2023 11:04:33 -0400

> Cool, let's use perltidy 20230309 then.

OK, so when would we do this? The change to 20230309 + valign changes is 
fairly large:

188 files changed, 3657 insertions(+), 3395 deletions(-)

Maybe right before we fork the tree?

cheers

andrew

---

From: Tom Lane <[email-masked]>
Date: Sun, 23 Apr 2023 11:16:10 -0400

> OK, so when would we do this? The change to 20230309 + valign changes is 

I think we could go ahead and commit the perltidyrc and README changes
now.  But the ensuing reformatting should happen as part of the mass
pgindent run, probably next month sometime.

			regards, tom lane

---

From: Jelte Fennema <[email-masked]>
Date: Sun, 23 Apr 2023 17:29:15 +0200

> pgindent run, probably next month sometime.

I think it's better to make the changes close together, not with a
month in between. Otherwise no-one will be able to run perltidy on
their patches, because the config and the files are even more out of
sync than they are now. So I'd propose to commit the perltidyrc
changes right before the pgindent run.

---

From: Magnus Hagander <[email-masked]>
Date: Sun, 23 Apr 2023 23:52:24 +0200

> Now that pg_bsd_indent is in the core code why not just use that?

yeah, it just required building. And the lazy approach was to use the DEB :)

For a quick fix I've built the current HEAD and have it just using
that one -- right now it'll fail again when a change is made to it,
but I'll get that cleaned up.

It's back up and running, and results are at
https://git.postgresql.org/gitweb/?p=postgresql-pgindent.git;a=shortlog;h=refs/heads/master-pgindent

-- 
 Magnus Hagander
 Me: https://www.hagander.net/
 Work: https://www.redpill-linpro.com/

---

From: Peter Eisentraut <[email-masked]>
Date: Mon, 24 Apr 2023 16:09:22 +0200

> changes right before the pgindent run.

Does anyone find perltidy useful?  To me, it functions more like a 
JavaScript compiler in that once you process the source code, it is no 
longer useful for manual editing.  If we are going to have the buildfarm 
check indentation and that is going to be extended to Perl code, I have 
some concerns about that.

---

From: Tom Lane <[email-masked]>
Date: Mon, 24 Apr 2023 10:14:31 -0400

> some concerns about that.

I certainly don't like its current behavior where adding/changing one
line can have side-effects on nearby lines.  But we have a proposal
to clean that up, and I'm cautiously optimistic that it'll be better
in future.  Did you have other specific concerns?

			regards, tom lane

---

From: Peter Eisentraut <[email-masked]>
Date: Wed, 26 Apr 2023 09:38:40 +0200

> in future.  Did you have other specific concerns?

I think the worst is how it handles multi-line data structures like

         $newnode->command_ok(
             [
                 'psql', '-X',
                 '-v',   'ON_ERROR_STOP=1',
                 '-c',   $upcmds,
                 '-d',   $oldnode->connstr($updb),
             ],
             "ran version adaptation commands for database $updb");

or

         $node->command_fails_like(
             [
                 'pg_basebackup',   '-D',
                 "$tempdir/backup", '--compress',
                 $cft->[0]
             ],
             qr/$cfail/,
             'client ' . $cft->[2]);

Perhaps that is included in the upcoming changes you are referring to?

---

From: Tom Lane <[email-masked]>
Date: Wed, 26 Apr 2023 09:27:05 -0400

>              "ran version adaptation commands for database $updb");

Yeah, I agree, there is no case where that doesn't suck.  I don't
mind it imposing specific placements of brackets and so on ---
that's very analogous to what pgindent will do.  But it likes to
re-flow comma-separated lists, and generally manages to make a
complete logical hash of them when it does, as in your other

>                  "$tempdir/backup", '--compress',

Can we fix it to preserve the programmer's choices of line breaks
in comma-separated lists?

			regards, tom lane

---

From: Andrew Dunstan <[email-masked]>
Date: Wed, 26 Apr 2023 15:44:47 -0400

On 2023-04-26 We 09:27, Tom Lane wrote:
> Peter Eisentraut<[email-masked]>  writes:

I doubt there's something like that. You can freeze arbitrary blocks of 
code like this (from the manual)

#<<<  format skipping: do not let perltidy change my nice formatting
         my @list = (1,
                     1, 1,
                     1, 2, 1,
                     1, 3, 3, 1,
                     1, 4, 6, 4, 1,);
#>>>

But that gets old and ugly pretty quickly.

There is a --freeze-newlines option, but it's global. I don't think we 
want that.

cheers

andrew

---

From: Tom Lane <[email-masked]>
Date: Wed, 26 Apr 2023 16:05:07 -0400

> I doubt there's something like that.

I had a read-through of the latest version's man page, and found
this promising-looking entry:

-boc, --break-at-old-comma-breakpoints

    The -boc flag is another way to prevent comma-separated lists from
    being reformatted. Using -boc on the above example, plus additional
    flags to retain the original style, yields

    # perltidy -boc -lp -pt=2 -vt=1 -vtc=1
    my @list = (1,
                1, 1,
                1, 2, 1,
                1, 3, 3, 1,
                1, 4, 6, 4, 1,);

    A disadvantage of this flag compared to the methods discussed above is
    that all tables in the file must already be nicely formatted.

I've not tested this, but it looks like it would do what we need,
modulo needing to fix all the existing damage by hand ...

			regards, tom lane

---

From: Alvaro Herrera <[email-masked]>
Date: Fri, 28 Apr 2023 11:25:45 +0200

On 2023-Feb-05, Andrew Dunstan wrote:

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

I ran this experimentally with perltidy 20230309, and compared that with
the --novalign behavior (not to propose the latter -- just to be aware
of what else is vertical alignment doing.)

Based on the differences between both, I think we'll definitely want to
include =~ and |= in this list, and I think we should discuss whether to
also include "or" (for "do_stuff or die()" type of constructs) and "qw"
(mainly used in 'use Foo qw(one two)' import lists).  All these have
effects (albeit smaller than the list you gave) on our existing code.

If you change from an exclusion list to --novalign then you lose
alignment of trailing # comments, which personally I find a loss, even
though they're still a multi-line effect.  Another change would be that
it ditches alignment of "{" but that only changes msvc/Install.pm, so I
think we shouldn't worry; and then there's this one:

-use PostgreSQL::Test::Utils          ();
+use PostgreSQL::Test::Utils ();
 use PostgreSQL::Test::BackgroundPsql ();

which I think we could just change to qw() if we cared enough (but I bet
we don't).

All in all, I think sticking to
--valign-exclusion-list=", = => =~ |= || && if or qw unless"
is a good deal.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"Ellos andaban todos desnudos como su madre los parió, y también las mujeres,
aunque no vi más que una, harto moza, y todos los que yo vi eran todos
mancebos, que ninguno vi de edad de más de XXX años" (Cristóbal Colón)

---

From: Bruce Momjian <[email-masked]>
Date: Fri, 28 Apr 2023 14:08:18 -0400

On Wed, Apr 26, 2023 at 03:44:47PM -0400, Andrew Dunstan wrote:
> I doubt there's something like that. You can freeze arbitrary blocks of code
> like this (from the manual)
> 

Can those comments be added by a preprocessor before calling perltidy,
and then removed on completion?

-- 
  Bruce Momjian  <[email-masked]>        https://momjian.us
  EDB                                      https://enterprisedb.com

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

---

From: Andrew Dunstan <[email-masked]>
Date: Sun, 30 Apr 2023 09:02:04 -0400

> and then removed on completion?

I imagine so, but we'd need a way of determining algorithmically which 
lines to protect. That might not be at all simple. And then we'd have 
the maintenance burden of the preprocessor.

cheers

andrew

---

From: Tom Lane <[email-masked]>
Date: Sun, 30 Apr 2023 10:32:16 -0400

> the maintenance burden of the preprocessor.

Yeah, it's hard to see how you'd do that without writing a full Perl
parser.

			regards, tom lane

---

From: Tom Lane <[email-masked]>
Date: Wed, 17 May 2023 17:10:48 -0400

> 	-boc, --break-at-old-comma-breakpoints

Sadly, this seems completely not ready for prime time.  I experimented
with it under perltidy 20230309, and found that it caused hundreds
of kilobytes of gratuitous changes that don't seem to have a direct
connection to the claimed purpose.  Most of these seemed to be from
forcing a line break after a function call's open paren, like

@@ -50,10 +50,12 @@ detects_heap_corruption(
 #
 fresh_test_table('test');
 $node->safe_psql('postgres', q(VACUUM (FREEZE, DISABLE_PAGE_SKIPPING) test));
-detects_no_corruption("verify_heapam('test')",
+detects_no_corruption(
+	"verify_heapam('test')",
 	"all-frozen not corrupted table");
 corrupt_first_page('test');
-detects_heap_corruption("verify_heapam('test')",
+detects_heap_corruption(
+	"verify_heapam('test')",
 	"all-frozen corrupted table");
 detects_no_corruption(
 	"verify_heapam('test', skip := 'all-frozen')",

although in some places it just wanted to insert a space, like this:

@@ -77,9 +81,9 @@ print "standby 2: $result\n";
 is($result, qq(33|0|t), 'check streamed sequence content on standby 2');
 
 # Check that only READ-only queries can run on standbys
-is($node_standby_1->psql('postgres', 'INSERT INTO tab_int VALUES (1)'),
+is( $node_standby_1->psql('postgres', 'INSERT INTO tab_int VALUES (1)'),
 	3, 'read-only queries on standby 1');
-is($node_standby_2->psql('postgres', 'INSERT INTO tab_int VALUES (1)'),
+is( $node_standby_2->psql('postgres', 'INSERT INTO tab_int VALUES (1)'),
 	3, 'read-only queries on standby 2');
 
 # Tests for connection parameter target_session_attrs

So I don't think we want that.  Maybe in some future version it'll
be more under control.

Barring objections, I'll use the attached on Friday.

			regards, tom lane

commit 7874d0f178f2bcdc889ce410d3e126e6750d96b4
Author: Tom Lane <[email-masked]>
Date:   Wed May 17 16:43:38 2023 -0400

    Make agreed updates in perltidy options.
    
    Discussion: https://postgr.es/m/[email-masked]

diff --git a/src/tools/pgindent/README b/src/tools/pgindent/README
index 43c736b0a1..08874d12eb 100644
--- a/src/tools/pgindent/README
+++ b/src/tools/pgindent/README
@@ -14,16 +14,16 @@ PREREQUISITES:
    sibling directory src/tools/pg_bsd_indent; see the directions
    in that directory's README file.
 
-2) Install perltidy.  Please be sure it is version 20170521 (older and newer
+2) Install perltidy.  Please be sure it is version 20230309 (older and newer
    versions make different formatting choices, and we want consistency).
    You can get the correct version from
    https://cpan.metacpan.org/authors/id/S/SH/SHANCOCK/
    To install, follow the usual install process for a Perl module
    ("man perlmodinstall" explains it).  Or, if you have cpan installed,
    this should work:
-   cpan SHANCOCK/Perl-Tidy-20170521.tar.gz
+   cpan SHANCOCK/Perl-Tidy-20230309.tar.gz
    Or if you have cpanm installed, you can just use:
-   cpanm https://cpan.metacpan.org/authors/id/S/SH/SHANCOCK/Perl-Tidy-20170521.tar.gz
+   cpanm https://cpan.metacpan.org/authors/id/S/SH/SHANCOCK/Perl-Tidy-20230309.tar.gz
 
 DOING THE INDENT RUN:
 
diff --git a/src/tools/pgindent/perltidyrc b/src/tools/pgindent/perltidyrc
index 9f09f0a64e..589d6e1f06 100644
--- a/src/tools/pgindent/perltidyrc
+++ b/src/tools/pgindent/perltidyrc
@@ -14,3 +14,4 @@
 --paren-vertical-tightness=2
 --paren-vertical-tightness-closing=2
 --noblanks-before-comments
+--valign-exclusion-list=", = => =~ |= || && if or qw unless"

---

From: Andrew Dunstan <[email-masked]>
Date: Thu, 18 May 2023 08:59:28 -0400

> Barring objections, I'll use the attached on Friday.

LGTM

cheers

andrew

---

From: Jelte Fennema <[email-masked]>
Date: Thu, 15 Jun 2023 17:26:58 +0200

> Perhaps we should start with a buildfarm module, which would run pg_indent --show-diff. That would only need to run on one animal, so a failure wouldn't send the whole buildfarm red. This would be…

Just to be clear on where we are. Is there anything blocking us from
doing this, except for the PG16 branch cut? (that I guess is planned
somewhere in July?)

Just doing this for pgindent and not for perltidy would already be a
huge improvement over the current situation IMHO.

---

From: Andrew Dunstan <[email-masked]>
Date: Thu, 15 Jun 2023 12:12:27 -0400

On 2023-06-15 Th 11:26, Jelte Fennema wrote:
> On Sat, 22 Apr 2023 at 13:42, Andrew Dunstan<[email-masked]>  wrote:

The short answer is that some high priority demands from $dayjob got in 
the way. However, I hope to have it done soon.

cheers

andrew

---

From: Andrew Dunstan <[email-masked]>
Date: Sat, 17 Jun 2023 10:08:32 -0400

On 2023-06-15 Th 12:12, Andrew Dunstan wrote:
> The short answer is that some high priority demands from $dayjob got 
> in the way. However, I hope to have it done soon.
>

See 
<https://github.com/PGBuildFarm/client-code/commit/f9c1c15048b412d34ccda8020d989b3a7b566c05>

I have set up a new buildfarm animal called koel which will run the module.

cheers

andrew

---

From: Tom Lane <[email-masked]>
Date: Mon, 19 Jun 2023 17:07:03 -0400

> I have set up a new buildfarm animal called koel which will run the module.

Is koel tracking the right repo?  It just spit up with a bunch of
diffs that seem to have little to do with the commit it's claiming
caused them:

https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=koel&dt=2023-06-19%2019%3A49%3A03

			regards, tom lane

---

From: Michael Paquier <[email-masked]>
Date: Tue, 20 Jun 2023 11:09:08 +0900

On Sat, Jun 17, 2023 at 10:08:32AM -0400, Andrew Dunstan wrote:
> See <https://github.com/PGBuildFarm/client-code/commit/f9c1c15048b412d34ccda8020d989b3a7b566c05>

That's really cool!  Thanks for taking the time to do that!
--
Michael

---

From: Andrew Dunstan <[email-masked]>
Date: Tue, 20 Jun 2023 08:04:39 -0400

> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=koel&dt=2023-06-19%2019%3A49%3A03

Yeah, I changed it so that instead of just checking new commits it would 
check the whole tree. The problem with the incremental approach is that 
the next run it might turn green again but the issue would not have been 
fixed.

I think this is a one-off issue. Once we clean up the tree the problem 
would disappear and the commits it shows would be correct. I imaging 
that's going to happen any day now?

cheers

andrew

---

From: Tom Lane <[email-masked]>
Date: Tue, 20 Jun 2023 09:08:36 -0400

> the next run it might turn green again but the issue would not have been 

Ah.

> that's going to happen any day now?

I can go fix the problems now that we know there are some (already).
However, if what you're saying is that koel only checks recently-changed
files, that's going to be pretty misleading in future too.  If people
don't react to such reports right away, they'll disappear, no?

			regards, tom lane

---

From: Andrew Dunstan <[email-masked]>
Date: Tue, 20 Jun 2023 09:21:14 -0400

> don't react to such reports right away, they'll disappear, no?

That's what would have happened if I hadn't changed the way it worked 
(and that's why I changed it). Now it doesn't just check recent commits, 
it checks the whole tree, and will stay red until the tree is fixed.

cheers

andrew

---

From: Peter Geoghegan <[email-masked]>
Date: Fri, 11 Aug 2023 13:59:40 -0700

> I have set up a new buildfarm animal called koel which will run the module.

I'm starting to have doubts about this policy. There have now been
quite a few follow-up "fixes" to indentation issues that koel
complained about. None of these fixups have been included in
.git-blame-ignore-revs. If things continue like this then "git blame"
is bound to become much less usable over time.

I don't think that it makes sense to invent yet another rule for
.git-blame-ignore-revs, though. Will we need another buildfarm member
to enforce that rule, too?

-- 
Peter Geoghegan

---

From: Andres Freund <[email-masked]>
Date: Fri, 11 Aug 2023 14:25:12 -0700

Hi,

> is bound to become much less usable over time.

I'm not sure I buy that that's going to be a huge problem - most of the time
such fixups are pretty small compared to larger reindents.

> to enforce that rule, too?

We could a test that fails when there's some mis-indented code. That seems
like it might catch things earlier?

Greetings,

Andres Freund

---

From: Peter Geoghegan <[email-masked]>
Date: Fri, 11 Aug 2023 14:48:09 -0700

> like it might catch things earlier?

It definitely would. That would go a long way towards addressing my
concerns. But I suspect that that would run into problems that stem
from the fact that the buildfarm is testing something that isn't all
that simple. Don't typedefs need to be downloaded from some other
blessed buildfarm animal?

-- 
Peter Geoghegan

---

From: Tom Lane <[email-masked]>
Date: Fri, 11 Aug 2023 18:30:02 -0400

>> like it might catch things earlier?

+1 for including this in CI tests

> blessed buildfarm animal?

No.  I presume koel is using src/tools/pgindent/typedefs.list,
which has always been the "canonical" list but up to now we've
been lazy about maintaining it.  Part of the new regime is that
typedefs.list should now be updated on-the-fly by patches that
add new typedefs.

We should still compare against the buildfarm's list periodically;
but I imagine that the primary result of that will be to remove
no-longer-used typedefs from typedefs.list.

			regards, tom lane

---

From: Peter Geoghegan <[email-masked]>
Date: Fri, 11 Aug 2023 15:46:40 -0700

> typedefs.list should now be updated on-the-fly by patches that

My workflow up until now has avoiding making updates to typedefs.list
in patches. I only update typedefs locally, for long enough to indent
my code. The final patch doesn't retain any typedefs.list changes.

> no-longer-used typedefs from typedefs.list.

I believe that I came up with my current workflow due to the
difficulty of maintaining the typedef file itself. Random
platform/binutils implementation details created a lot of noise,
presumably because my setup wasn't exactly the same as Bruce's setup,
in whatever way. For example, the order of certain lines would change,
in a way that had nothing whatsoever to do with structs that my patch
added.

I guess that I can't do that anymore. Hopefully maintaining the
typedefs.list file isn't as inconvenient as it once seemed to me to
be.

-- 
Peter Geoghegan

---

From: Tom Lane <[email-masked]>
Date: Fri, 11 Aug 2023 19:02:06 -0400

> my code. The final patch doesn't retain any typedefs.list changes.

Yeah, I've done the same and will have to stop.

> typedefs.list file isn't as inconvenient as it once seemed to me to

I don't think it'll be a problem.  If your rule is "add new typedef
names added by your patch to typedefs.list, keeping them in
alphabetical order" then it doesn't seem very complicated, and
hopefully conflicts between concurrently-developed patches won't
be common.

			regards, tom lane

---

From: Tom Lane <[email-masked]>
Date: Fri, 11 Aug 2023 19:17:03 -0400

> is bound to become much less usable over time.

FWIW, I'm much more optimistic than that.  I think what we're seeing
is just the predictable result of not all committers having yet
incorporated "pgindent it before committing" into their workflow.
The need for followup fixes should diminish as people start doing
that.  If you want to hurry things along, peer pressure on committers
who clearly aren't bothering is the solution.

			regards, tom lane

---

From: Jelte Fennema <[email-masked]>
Date: Sat, 12 Aug 2023 01:18:04 +0200

On Fri, 11 Aug 2023 at 23:00, Peter Geoghegan <[email-masked]> wrote:
> complained about.

I think one thing that would help a lot in reducing the is for
committers to set up the local git commit hook that's on the wiki:
https://wiki.postgresql.org/wiki/Working_with_Git

That one fails the commit if there's wrongly indented files in the
commit. And if you still want to opt out for whatever reason you can
use git commit --no-verify

---

From: Andres Freund <[email-masked]>
Date: Fri, 11 Aug 2023 16:20:24 -0700

Hi,

> +1 for including this in CI tests

I didn't even mean CI - I meant 'make check-world' / 'meson test'. Which of
course would include CI automatically.

> typedefs.list should now be updated on-the-fly by patches that

Yea. Otherwise nobody else can indent reliably, without repeating the work of
adding typedefs.list entries of all the patches since the last time it was
updated in the repository.

Greetings,

Andres Freund

---

From: Tom Lane <[email-masked]>
Date: Fri, 11 Aug 2023 20:11:34 -0400

> course would include CI automatically.

Hmm.  I'm allergic to anything that significantly increases the cost
of check-world, and this seems like it'd do that.

Maybe we could automate it, but not as part of check-world per se?

			regards, tom lane

---

From: Andres Freund <[email-masked]>
Date: Fri, 11 Aug 2023 17:46:42 -0700

Hi,

> of check-world, and this seems like it'd do that.

Hm, compared to the cost of check-world it's not that large, but still,
annoying to make it larger.

We can make it lot cheaper, but perhaps not in a general enough fashion that
it's suitable for a test.

pgindent already can query git (for --commit). We could teach pgindent to
ask git what remote branch is being tracked, and constructed a list of files
of the difference between the remote branch and the local branch?

That option could do something like:
git diff --name-only $(git rev-parse --abbrev-ref --symbolic-full-name @{upstream})

That's pretty quick, even for a relatively large delta.

> Maybe we could automate it, but not as part of check-world per se?

We should definitely do that.  Another related thing that'd be useful to
script is updating typedefs.list with the additional typedefs found
locally. Right now the script for that still lives in the

Greetings,

Andres Freund

---

From: Andrew Dunstan <[email-masked]>
Date: Sat, 12 Aug 2023 11:57:25 -0400

On 2023-08-11 Fr 19:17, Tom Lane wrote:
> Peter Geoghegan<[email-masked]>  writes:

Yeah, part of the point of creating koel was to give committers a bit of 
a nudge in that direction.

With a git pre-commit hook it's pretty painless.

cheers

andrew

---

From: Andrew Dunstan <[email-masked]>
Date: Sat, 12 Aug 2023 17:03:37 -0400

> hopefully conflicts between concurrently-developed patches won't

My recollection is that missing typedefs cause indentation that kinda 
sticks out like a sore thumb.

The reason we moved to a buildfarm based typedefs list was that some 
typedefs are platform dependent, so any list really needs to be the 
union of the found typedefs on various platforms, and the buildfarm was 
a convenient vehicle for doing that. But that doesn't mean you shouldn't 
manually add a typedef you have added in your code.

cheers

andrew

---

From: Andres Freund <[email-masked]>
Date: Sat, 12 Aug 2023 14:14:04 -0700

Hi,

On 2023-08-12 17:03:37 -0400, Andrew Dunstan wrote:
> My recollection is that missing typedefs cause indentation that kinda sticks
> out like a sore thumb.
> 
> typedefs are platform dependent, so any list really needs to be the union of
> the found typedefs on various platforms, and the buildfarm was a convenient
> vehicle for doing that. But that doesn't mean you shouldn't manually add a
> typedef you have added in your code.

It's a somewhat annoying task though, find all the typedefs, add them to the
right place in the file (we have an out of order entry right now). I think a
script that *adds* (but doesn't remove) local typedefs would make this less
painful.

Greetings,

Andres Freund

---

From: Tom Lane <[email-masked]>
Date: Sat, 12 Aug 2023 18:46:57 -0400

>> My recollection is that missing typedefs cause indentation that kinda sticks

Yeah, it's usually pretty obvious: "typedef *var" gets changed to
"typedef * var", and similar oddities.

> script that *adds* (but doesn't remove) local typedefs would make this less

My practice has always been "add typedefs until pgindent doesn't do
anything I don't want".  If you have a new typedef that doesn't happen
to be used in a way that pgindent mangles, it's not that critical
to get it into the file right away.

			regards, tom lane

---

From: Peter Geoghegan <[email-masked]>
Date: Sat, 12 Aug 2023 17:13:27 -0700

> to get it into the file right away.

We seem to be seriously contemplating making every patch author do
this every time they need to get the tests to pass (after adding or
renaming a struct). Is that really an improvement over the old status
quo?

In principle I'm in favor of strictly enforcing indentation rules like
this. But it seems likely that our current tooling just isn't up to
the task.

-- 
Peter Geoghegan

---

From: Tom Lane <[email-masked]>
Date: Sat, 12 Aug 2023 20:20:17 -0400

> renaming a struct). Is that really an improvement over the old status

Hm.  I was envisioning that we should expect committers to deal
with this, not original patch submitters.  So that's an argument
against including it in the CI tests.  But I'm in favor of anything
we can do to make it more painless for committers to fix up patch
indentation.

			regards, tom lane

---

From: Peter Geoghegan <[email-masked]>
Date: Sat, 12 Aug 2023 17:53:29 -0700

> we can do to make it more painless for committers to fix up patch

Making it a special responsibility for committers comes with the same
set of problems that we see with catversion bumps. People are much
more likely to forget to do something that must happen last.

Maybe I'm wrong -- maybe the new policy is practicable. It might even
turn out to be worth the bother. Time will tell.

-- 
Peter Geoghegan

---

From: Andrew Dunstan <[email-masked]>
Date: Sun, 13 Aug 2023 10:33:21 -0400

On 2023-08-12 Sa 20:53, Peter Geoghegan wrote:
> On Sat, Aug 12, 2023 at 5:20 PM Tom Lane<[email-masked]>  wrote:

I agree with this.

> more likely to forget to do something that must happen last.

After I'd been caught by this once or twice I implemented a git hook 
test for that too - in fact it was the first hook I did. It's not 
perfect but it's saved me a couple of times:

check_catalog_version () {

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

     case "$files" in
         *src/include/catalog/catversion.h*)
             return 0;
             ;;
         *src/include/catalog/*)
             ;;
         *)
             return 0;
             ;;
     esac

     # changes include catalog but not catversion.h, so warn about it
     {
         echo 'Commit on master alters catalog but catversion not bumped'
         echo 'It can be forced with git commit --no-verify'
     } >&2

     exit 1
}

cheers

andrew

---

From: Michael Paquier <[email-masked]>
Date: Mon, 14 Aug 2023 08:33:40 +0900

On Sun, Aug 13, 2023 at 10:33:21AM -0400, Andrew Dunstan wrote:
> After I'd been caught by this once or twice I implemented a git hook test
> for that too - in fact it was the first hook I did. It's not perfect but
> it's saved me a couple of times:
> 

I find that pretty cool.  Thanks for sharing.
--
Michael

---

From: Peter Eisentraut <[email-masked]>
Date: Mon, 14 Aug 2023 16:04:04 +0200

On 12.08.23 23:14, Andres Freund wrote:
> script that*adds*  (but doesn't remove) local typedefs would make this less
> painful.

I was puzzled once that there does not appear to be such a script 
available.  Whatever the buildfarm does (before it merges it all 
together) should be available locally.  Then the workflow could be

type type type
compile
update typedefs
pgindent
commit

---

From: Peter Eisentraut <[email-masked]>
Date: Mon, 14 Aug 2023 16:08:30 +0200

> Maybe we could automate it, but not as part of check-world per se?

Also, during development, the code in progress is not always perfectly 
formatted, but I do want to keep running the test suites.

---

From: Andrew Dunstan <[email-masked]>
Date: Mon, 14 Aug 2023 15:58:34 -0400

> together) should be available locally.  Then the workflow could be

It's a bit more complicated :-)

You can see what the buildfarm does at 
<https://github.com/PGBuildFarm/client-code/blob/ec4cf43613a74cb88f228efcde09931cf9fd57e7/run_build.pl#L2562> 
It's been somewhat fragile over the years, which most people other than 
Tom and I have probably not noticed.

On most platforms it needs postgres to have been installed before 
looking for the typedefs.

cheers

andrew

---

From: Nathan Bossart <[email-masked]>
Date: Tue, 15 Aug 2023 13:31:09 -0700

> is bound to become much less usable over time.

Should we add those?  Patch attached.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
>From f4f158e153f9027330ce841ca79b5650dfd24a0e Mon Sep 17 00:00:00 2001
From: Nathan Bossart <[email-masked]>
Date: Tue, 15 Aug 2023 13:24:18 -0700
Subject: [PATCH v1 1/1] Add a few recent commits to .git-blame-ignore-revs.

---
 .git-blame-ignore-revs | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/.git-blame-ignore-revs b/.git-blame-ignore-revs
index 00b548c7b0..2ff86c8d0d 100644
--- a/.git-blame-ignore-revs
+++ b/.git-blame-ignore-revs
@@ -14,6 +14,27 @@
 #
 # $ git log --pretty=format:"%H # %cd%n# %s" $PGINDENTGITHASH -1 --date=iso
 
+89be0b89ae60c63856fd26d82a104781540e2312 # 2023-08-15 17:45:00 +0900
+# Fix code indentation vioaltion introduced in commit 9e9931d2b.
+
+5dc456b7dda4f7d0d7735b066607c190c74d174a # 2023-08-11 20:43:34 +0900
+# Fix code indentation violations introduced by recent commit
+
+62e9af4c63fbd36fb9af8450fb44bece76d7766f # 2023-07-25 12:35:58 +0530
+# Fix code indentation vioaltion introduced in commit d38ad8e31d.
+
+4e465aac36ce9a9533c68dbdc83e67579880e628 # 2023-07-18 14:04:31 +0900
+# Fix indentation in twophase.c
+
+328f492d2565cfbe383f13a69425d751fd79415f # 2023-07-13 22:26:10 +0900
+# Fix code indentation violation in commit b6e1157e7d
+
+69a674a170058e63e8178aec8a36a673efce8801 # 2023-07-06 11:49:18 +0530
+# Fix code indentation vioaltion introduced in commit cc32ec24fd.
+
+a4cfeeca5a97f2b5969c31aa69ba775af95ee5a3 # 2023-07-03 12:47:49 +0200
+# Fix code indentation violations
+
 b334612b8aee9f9a34378982d8938b201dfad323 # 2023-06-20 09:50:43 -0400
 # Pre-beta2 mechanical code beautification.
 
-- 
2.25.1

---

From: Peter Geoghegan <[email-masked]>
Date: Wed, 16 Aug 2023 13:15:55 -0700

> Should we add those?  Patch attached.

I think that that makes sense. I just don't want to normalize updating
.git-blame-ignore-revs very frequently. (Actually, it's more like I
don't want to normalize any scheme that makes updating the ignore list
very frequently start to seem reasonable.)

-- 
Peter Geoghegan

---

From: Nathan Bossart <[email-masked]>
Date: Thu, 17 Aug 2023 07:40:41 -0700

On Wed, Aug 16, 2023 at 01:15:55PM -0700, Peter Geoghegan wrote:
> I think that that makes sense.

Committed.

> I just don't want to normalize updating

Agreed.  I've found myself habitually running pgindent since becoming a
committer, but I'm sure I'll forget it one of these days.  From a quick
skim of this thread, it sounds like a pre-commit hook [0] might be the best
option at the moment.

[0] https://wiki.postgresql.org/wiki/Working_with_Git#Using_git_hooks

---

From: Peter Geoghegan <[email-masked]>
Date: Sun, 15 Oct 2023 17:52:17 -0700

> turn out to be worth the bother. Time will tell.

(Two months pass.)

There were two independent fixup commits to address complaints from
koel just today (from two different committers). Plus there was a
third issue (involving a third committer) this past Wednesday.

This policy isn't working.

-- 
Peter Geoghegan

---

From: Tom Lane <[email-masked]>
Date: Mon, 16 Oct 2023 20:45:00 -0400

> This policy isn't working.

Two thoughts about that:

1. We should not commit indent fixups on behalf of somebody else's
misindented commit.  Peer pressure on committers who aren't being
careful about this is the only way to improve matters; so complain
to the person at fault until they fix it.

2. We could raise awareness of this issue by adding indent verification
to CI testing.  I hesitate to suggest that, though, for a couple of
reasons:
   2a. It seems fairly expensive, though I might be misjudging.
   2b. It's often pretty handy to submit patches that aren't fully
   indent-clean; I have such a patch in flight right now at [1].

2b could be ameliorated by making the indent check be a separate
test process that doesn't obscure the results of other testing.

			regards, tom lane

[1] https://www.postgresql.org/message-id/2617358.1697501956%40sss.pgh.pa.us

---

From: Peter Geoghegan <[email-masked]>
Date: Mon, 16 Oct 2023 18:22:51 -0700

> to the person at fault until they fix it.

Thomas Munro's recent commit 01529c704008 was added to
.git-blame-ignore-revs by Michael Paquier, despite the fact that
Munro's commit technically isn't just a pure indentation fix (it also
fixed some typos). It's hard to judge Michael too harshly for this,
since in general it's harder to commit things when koel is already
complaining about existing misindetation -- I get why he'd prefer to
take care of that first.

>    indent-clean; I have such a patch in flight right now at [1].

It's also often handy to make a minor change to a comment or something
at the last minute, without necessarily having the comment indented
perfectly.

> test process that doesn't obscure the results of other testing.

I was hoping that "go back to the old status quo" would also appear as
an option.

My main objection to the new policy is that it's not quite clear what
process I should go through in order to be 100% confident that koel
won't start whining (short of waiting around for koel to whine). I
know how to run pgindent, of course, and haven't had any problems so
far...but it still seems quite haphazard. If we're going to make this
a hard rule, enforced on every commit, it should be dead easy to
comply with the rule.

-- 
Peter Geoghegan

---

From: Michael Paquier <[email-masked]>
Date: Tue, 17 Oct 2023 11:57:01 +0900

> test process that doesn't obscure the results of other testing.

I see an extra reason with not doing that: this increases the
difficulty when it comes to send and maintain patches to the lists and 
newcomers would need to learn more tooling.  I don't think that we
should make that more complicated for code-formatting reasons.
--
Michael

---

From: Jelte Fennema <[email-masked]>
Date: Tue, 17 Oct 2023 12:49:13 +0200

> should make that more complicated for code-formatting reasons.

Honestly, I don't think it's a huge hurdle for newcomers. Most open
source projects have a CI job that runs automatic code formatting, so
it's a pretty common thing for contributors to deal with. And as long
as we keep it a separate CI job from the normal tests, committers can
even choose to commit the patch if the formatting job fails, after
running pgindent themselves.

And personally as a contributor it's a much nicer experience to see
quickly in CI that I messed up the code style, then to hear it a
week/month later in an email when someone took the time to review and
mentions the styling is way off all over the place.

---

From: Robert Haas <[email-masked]>
Date: Tue, 17 Oct 2023 08:45:44 -0400

> This policy isn't working.

+1. I think this is more annoying than the status quo ante.

---

From: Robert Haas <[email-masked]>
Date: Tue, 17 Oct 2023 10:03:54 -0400

> +1. I think this is more annoying than the status quo ante.

Although ... I do think it's spared me some rebasing pain, and that
does have some real value. I wonder if we could think of other
alternatives. For example, maybe we could have a bot. If you push a
commit that's not indented properly, the bot reindents the tree,
updates git-blame-ignore-revs, and sends you an email admonishing you
for your error. Or we could have a server-side hook that will refuse
the misindented commit, with some kind of override for emergency
situations. What I really dislike about the current situation is that
it's doubling down on the idea that committers have to be perfect and
get everything right every time. Turns out, that's hard to do. If not,
why do people keep screwing things up? Somebody could theorize - and
this seems to be Tom and Jelte's theory, though perhaps I'm
misinterpreting their comments - that the people who have made
mistakes here are just lazy, and what they need to do is up their
game.

But I don't buy that. First, I think that most of our committers are
pretty intelligent and hard-working people who are trying to do the
right thing. We can't all be Tom Lane, no matter how hard we may try.
Second, even if it were true that the offending committers are "just
lazy," all of our contributors and many senior non-committer
contributors are people who have put thousands, if not tens of
thousands, of hours into the project. Making them feel bad serves us
poorly. At the end of the day, it doesn't matter whether it's too much
of a pain for the perfect committers we'd like to have. It matters
whether it's too much of a pain for the human committers that we do
have.

---

From: Tom Lane <[email-masked]>
Date: Tue, 17 Oct 2023 10:23:22 -0400

> alternatives.

An alternative I was thinking about after reading your earlier email
was going back to the status quo ante, but doing the manual tree-wide
reindents significantly more often than once a year.  Adding one at
the conclusion of each commitfest would be a natural thing to do,
for instance.  It's hard to say what frequency would lead to the
least rebasing pain, but we know once-a-year isn't ideal.

> For example, maybe we could have a bot. If you push a
> for your error.

I'm absolutely not in favor of completely-automated reindents.
pgindent is a pretty stupid tool and it will sometimes do stupid
things, which you have to correct for by tweaking the input
formatting.  The combination of the tool and human supervision
generally produces pretty good results, but the tool alone
not so much.

> Or we could have a server-side hook that will refuse
> situations.

Even though I'm in the camp that would like the tree correctly
indented at all times, I remain very much against a commit hook.
I think that'd be significantly more annoying than the current
situation, which you're already unhappy about the annoying-ness of.

The bottom line here, I think, is that there's a subset of committers
that would like perfectly mechanically-indented code at all times,
and there's another subset that just doesn't care that much.
We don't (and shouldn't IMO) have a mechanism to let one set force
their views on the other set.  The current approach is clearly
insufficient for that, and I don't think trying to institute stronger
enforcement is going to make anybody happy.

			regards, tom lane

---

From: Peter Geoghegan <[email-masked]>
Date: Tue, 17 Oct 2023 07:55:36 -0700

> least rebasing pain, but we know once-a-year isn't ideal.

That seems like the best alternative we have. The old status quo did
occasionally allow code with indentation that *clearly* wasn't up to
project standards to slip in. It could stay that way for quite a few
months at a time. That wasn't great either.

-- 
Peter Geoghegan

---

From: Jelte Fennema <[email-masked]>
Date: Tue, 17 Oct 2023 17:00:35 +0200

> situation, which you're already unhappy about the annoying-ness of.

Why do you think that would be significantly more annoying than the
current situation? Instead of getting delayed feedback you get instant
feedback when you push.

---

From: Robert Haas <[email-masked]>
Date: Tue, 17 Oct 2023 11:01:44 -0400

> least rebasing pain, but we know once-a-year isn't ideal.

Yes. I suspect once a commitfest still wouldn't be often enough. Maybe
once a month or something would be. But I'm not sure. You might rebase
once over the misindented commit and then have to rebase again over
the indent that fixed it. There's not really anything that quite
substitutes for doing it right on every commit.

> enforcement is going to make anybody happy.

I mean, I think we DO have such a mechanism. Everyone agrees that the
buildfarm has to stay green, and we have a buildfarm member that
checks pgindent, so that means everyone has to pgindent. We could
decide to kill that buildfarm member, in which case we go back to
people not having to pgindent, but right now they do.

And if it's going to remain the policy, it's better to enforce that
policy earlier rather than later. I mean, what is the point of having
a system where we let people do the wrong thing and then publicly
embarrass them afterwards? How is that better than preventing them
from doing the wrong thing in the first place? Even if they don't
subjectively feel embarrassed, nobody likes having to log back on in
the evening or the weekend and clean up after something they thought
they were done with.

In fact, that particular experience is one of the worst things about
being a committer. It actively discourages me, at least, from trying
to get other people's patches committed. This particular problem is
minor, but the overall experience of trying to get things committed is
that you have to check 300 things for every patch and if you get every
one of them right then nothing happens and if you get one of them
wrong then you get a bunch of irritated emails criticizing your
laziness, sloppiness, or whatever, and you have to drop everything to
go fix it immediately. What a deal! I'm sure this isn't the only
reason why we have such a huge backlog of patches needing committer
attention, but it sure doesn't help. And there is absolutely zero need
for this to be yet another thing that you can find out you did wrong
in the 1-24 hour period AFTER you type 'git push'.

---

From: Peter Geoghegan <[email-masked]>
Date: Tue, 17 Oct 2023 08:15:51 -0700

On Tue, Oct 17, 2023 at 8:01 AM Robert Haas <[email-masked]> wrote:
> go fix it immediately. What a deal!

Yep. Enforcing perfect indentation on koel necessitates rechecking
indentation after each and every last-minute fixup affecting C code --
the interactions makes it quite a bit harder to get everything right
on the first push. For example, if I spot an issue with a comment
during final pre-commit review, and fixup that commit, I have to run
pgindent again. On a long enough timeline, I'm going to forget to do
that.

-- 
Peter Geoghegan

---

From: Robert Haas <[email-masked]>
Date: Tue, 17 Oct 2023 11:23:52 -0400

> pgindent again. On a long enough timeline, I'm going to forget to do

I also just discovered that my pre-commit hook doesn't work if I pull
commits into master by cherry-picking. I had thought that I could have
my hook just check my commits to master and not all of my local dev
branches where I really don't want to mess with this when I'm just
banging out a rough draft of something. But now I see that I'm going
to need to work harder on this if I actually want it to catch all the
ways I might screw this up.

---

From: Peter Geoghegan <[email-masked]>
Date: Tue, 17 Oct 2023 08:47:01 -0700

> ways I might screw this up.

Once you figure all that out, you're still obligated to hand-polish
typedefs.list to be consistent with whatever Bruce's machine's copy of
objdump does (or is it Tom's?). You need to sort the entries so they
kinda look like they originated from the same source as existing
entries, since my Debian machine seems to produce somewhat different
results to RHEL, for whatever reason. It's hard to imagine a worse use
of committer time.

I think that something like this new policy could work if the
underlying tooling was very easy to use and gave perfectly consistent
results on everybody's development machine. Obviously, that just isn't
the case.

-- 
Peter Geoghegan

---

From: David Rowley <[email-masked]>
Date: Wed, 18 Oct 2023 17:40:40 +1300

> +1. I think this is more annoying than the status quo ante.

Maybe there are two camps of committers here; ones who care about
committing correctly indented code and ones who do not.

I don't mean that in a bad way, but if a committer just does not care
about correctly pgindented code then he/she likely didn't suffer
enough pain from how things used to be... having to unindent all the
unrelated indent fixes that were committed since the last pgindent run
I personally found slow/annoying/error-prone.

What I do now seems significantly easier. Assuming it's just 1 commit, just:

perl src/tools/pgindent/pgindent --commit HEAD
git diff # manual check to see if everything looks sane.
git commit -a --fixup HEAD
git rebase -i HEAD~2

If we were to go back to how it was before, then why should I go to
the trouble of unindenting all the unrelated indents from code changed
by other committers since the last pgindent run when those committers
are not bothering to and making my job harder each time they commit
incorrectly indented code.

How many of the committers who have broken koel are repeat offenders?
What is their opinion on this?
Did they just forget once or do they hate the process and want to go back?

I'm personally very grateful for all the work that was done to improve
pgindent and set out the new process. I'd really rather not go back to
how things were.

I agree that it's not nice to add yet another way of breaking the
buildfarm and even more so when the committer did make check-world
before committing. We have --enable-tap-tests, we could have
--enable-indent-checks and have pgindent check the code is correctly
indented during make check-world. Then just not have
--enable-indent-checks in CI.

I think many of us have scripts we use instead of typing out all the
configure options we want.  It's likely pretty easy to add
--enable-indent-checks to those.

David

---

From: Peter Eisentraut <[email-masked]>
Date: Wed, 18 Oct 2023 09:20:55 +0200

> --enable-indent-checks in CI.

This approach seems like a good improvement, even independent of 
everything else we might do about this.  Making it easier to use and 
less likely to be forgotten.  Also, this way, non-committer contributors 
can opt-in, if they want to earn bonus points.

---

From: Jelte Fennema <[email-masked]>
Date: Wed, 18 Oct 2023 11:07:00 +0200

> How many of the committers who have broken koel are repeat offenders?

I just checked the commits and there don't seem to be real repeat
offenders. The maximum number of times someone broke koel since its
inception is two. That was the case for only two people. The other 8
people only caused one breakage.

> Did they just forget once or do they hate the process and want to go back?

The commiters that broke koel since its inception are:
- Alexander Korotkov
- Amit Kapila
- Amit Langote
- Andres Freund
- Etsuro Fujita
- Jeff Davis
- Michael Paquier
- Peter Eisentraut
- Tatsuo Ishii
- Tomas Vondra

I included all of them in the To field of this message, in the hope
that they share their viewpoint. Because otherwise it stays guessing
what they think.

But based on the contents of the fixup commits a commonality seems to
be that the fixup only fixes a few lines, quite often touching only
comments. So it seems like the main reason for breaking koel is
forgetting to re-run pgindent after some final cleanup/wording
changes/typo fixes. And that seems like an expected flaw of being
human instead of a robot, which can only be worked around with better
automation.

> --enable-indent-checks in CI.

I think --enable-indent-checks sounds like a good improvement to the
status quo. But I'm not confident that it will help remove the cases
where only a comment needs to be re-indented. Do commiters really
always run check-world again when only changing a typo in a comment? I
know I probably wouldn't (or at least not always).

---

From: David Rowley <[email-masked]>
Date: Wed, 18 Oct 2023 22:34:45 +1300

> human instead of a robot, which can only be worked around with better

I wonder if you might just be assuming these were caused by
last-minute comment adjustments. I may have missed something on the
thread, but it could be that, or it could be due to the fact that
pgindent just simply does more adjustments to comments than it does
with code lines.

> know I probably wouldn't (or at least not always).

I can't speak for others, but I always make edits in a dev branch and
do "make check-world" before doing "git format-patch" before I "git
am" that patch into a clean repo.  Before I push, I'll always run
"make check-world" again as sometimes master might have moved on a few
commits from where the dev branch was taken (perhaps I need to update
the expected output of some newly added EXPLAIN tests if say doing a
planner adjustment).  I personally never adjust any code or comments
after the git am. I only sometimes adjust the commit message.

So, in theory at least, if --enable-indent-checks existed and I used
it, I shouldn't break koel...  let's see if I just jinxed myself.

It would be good to learn how many of the committers out of the ones
you listed that --enable-indent-checks would have saved from breaking
koel.

David

---

From: Vik Fearing <[email-masked]>
Date: Wed, 18 Oct 2023 15:04:13 +0200

On 10/17/23 16:23, Tom Lane wrote:
> An alternative I was thinking about after reading your earlier email was 
> going back to the status quo ante, but doing the manual tree-wide 
> reindents significantly more often than once a year. Adding one at the 
> conclusion of each commitfest would be a natural thing to do, for 
> instance. It's hard to say what frequency would lead to the least 
> rebasing pain, but we know once-a-year isn't ideal.

This is basically how the SQL Committee functions.  The Change Proposals 
(patches) submitted every meeting (commitfest) are always against the 
documents as they exist after the application of papers (commits) from 
the previous meeting.

One major difference is that Change Proposals are against the text, and 
patches are against the code.  It is not dissimilar to people saying 
what our documentation should say, and then someone implementing that 
change.

So I am in favor of a pgindent run *at least* at the end of each 
commitfest, giving a full month for patch authors to rebase before the 
next fest.
-- 
Vik Fearing

---

From: Robert Haas <[email-masked]>
Date: Wed, 18 Oct 2023 10:07:46 -0400

> can opt-in, if they want to earn bonus points.

Yeah. I'm not going to push anything that doesn't pass make
check-world, so this is appealing in that something that I'm already
doing would (or could be configured to) catch this problem also.

---

From: Bruce Momjian <[email-masked]>
Date: Wed, 18 Oct 2023 12:45:15 -0400

> in the 1-24 hour period AFTER you type 'git push'.

This comment resonated with me.  I do all my git operations with shell
scripts so I can check for all the mistakes I have made in the past and
generate errors. Even with all of that, committing is an
anxiety-producing activity because any small mistake is quickly revealed
to the world.  There aren't many things I do in a day where mistakes are
so impactful.

  Only you can decide what is important to you.

---

From: Andres Freund <[email-masked]>
Date: Wed, 18 Oct 2023 12:15:51 -0700

Hi,

>    2a. It seems fairly expensive, though I might be misjudging.

Compared to other things it's not that expensive. On my workstation, which is
slower on a per-core basis than CI, a whole tree pgindent --silent-diff takes
6.8s.  For That's doing things serially, it shouldn't be that hard to parallelize
the per-file processing.

For comparison, the current compiler warnings task takes 6-15min, depending on
the state of the ccache "database". Even when ccache is primed, running
cpluspluscheck or headerscheck is ~30s each. Adding a few more seconds for an
indentation check wouldn't be a problem.

> test process that doesn't obscure the results of other testing.

The compiler warnings task already executes a number of tests even if prior
tests have failed (to be able to find compiler warnings in different compilers
at once). Adding pgindent cleanliness to that would be fairly simple.

I still think that one of the more important things we ought to do is to make
it trivial to check if code is correctly indented and reindent it for the
user.  I've posted a preliminary patch to add a 'indent-tree' target a few
months back, at
https://postgr.es/m/20230527184201.2zdorrijg2inqt6v%40alap3.anarazel.de

I've updated that patch, now it has
- indent-tree, reindents the entire tree
- indent-head, which pgindent --commit=HEAD
- indent-check, fails if the tree isn't correctly indented
- indent-diff, like indent-check, but also shows the diff

If we tought pgindent to emit the list of files it processes to a dependency
file, we could make it cheap to call indent-check repeatedly, by teaching
meson/ninja to not reinvoke it if the input files haven't changed.  Personally
that'd make it more bearable to script indentation checks to happen
frequently.

I'll look into writing a command to update typedefs.list with all the local
changes.

Greetings,

Andres Freund
>From 288c109286fda4dbd40fab345fbaf71d91d2db39 Mon Sep 17 00:00:00 2001
From: Andres Freund <[email-masked]>
Date: Sat, 27 May 2023 11:38:27 -0700
Subject: [PATCH v2] Add meson targets to run pgindent

Author:
Reviewed-By:
Discussion: https://postgr.es/m/
Backpatch:
---
 meson.build                 | 36 ++++++++++++++++++++++++++++++++++++
 src/tools/pgindent/pgindent | 12 ++++++++++--
 2 files changed, 46 insertions(+), 2 deletions(-)

diff --git a/meson.build b/meson.build
index 862c955453f..7d4d02e7624 100644
--- a/meson.build
+++ b/meson.build
@@ -3013,6 +3013,42 @@ run_target('install-test-files',

+###############################################################
+# Indentation and similar targets
+###############################################################
+
+indent_base = [perl, files('src/tools/pgindent/pgindent'),
+    '--indent', pg_bsd_indent.full_path(),
+    '--sourcetree=@SOURCE_ROOT@']
+
+# reindent the entire tree
+run_target('indent-tree',
+  command: indent_base + ['.'],
+  depends: pg_bsd_indent
+)
+
+# reindent the last commit
+run_target('indent-head',
+  command: indent_base + ['--commit=HEAD'],
+  depends: pg_bsd_indent
+)
+
+# silently check if any file is not corectly indented (fails the build if
+# there are differences)
+run_target('indent-check',
+  command: indent_base + ['--silent-diff', '.'],
+  depends: pg_bsd_indent
+)
+
+# show a diff of all the unindented files (doesn't fail the build if there are
+# differences)
+run_target('indent-diff',
+  command: indent_base + ['--show-diff', '.'],
+  depends: pg_bsd_indent
+)
+
+
+
 ###############################################################
 # Test prep
 ###############################################################
diff --git a/src/tools/pgindent/pgindent b/src/tools/pgindent/pgindent
index bce63d95daf..08b0c95814f 100755
--- a/src/tools/pgindent/pgindent
+++ b/src/tools/pgindent/pgindent
@@ -23,7 +23,7 @@ my $devnull = File::Spec->devnull;
 
 my ($typedefs_file, $typedef_str, @excludes,
 	$indent, $build, $show_diff,
-	$silent_diff, $help, @commits,);
+	$silent_diff, $sourcetree, $help, @commits,);
 
 $help = 0;
 
@@ -35,7 +35,8 @@ my %options = (
 	"excludes=s" => \@excludes,
 	"indent=s" => \$indent,
 	"show-diff" => \$show_diff,
-	"silent-diff" => \$silent_diff,);
+	"silent-diff" => \$silent_diff,
+	"sourcetree=s" => \$sourcetree);
 GetOptions(%options) || usage("bad command line argument");
 
 usage() if $help;
@@ -53,6 +54,13 @@ $typedefs_file ||= $ENV{PGTYPEDEFS};
 # get indent location for environment or default
 $indent ||= $ENV{PGINDENT} || $ENV{INDENT} || "pg_bsd_indent";
 
+# When invoked from the build directory, change into source tree, otherwise
+# the heuristics in locate_sourcedir() don't work.
+if (defined $sourcetree)
+{
+	chdir $sourcetree;
+}
+
 my $sourcedir = locate_sourcedir();
 
2.38.0

---

From: Andres Freund <[email-masked]>
Date: Wed, 18 Oct 2023 17:56:35 -0700

Hi,

> I'll look into writing a command to update typedefs.list with all the local

It turns out that updating the in-tree typedefs.list would be very noisy. On
my local linux system I get
 1 file changed, 422 insertions(+), 1 deletion(-)

On a mac mini I get
 1 file changed, 351 insertions(+), 1 deletion(-)

We could possibly address that by updating the in-tree typedefs.list a bit
more aggressively. Sure looks like the source systems are on the older side.

But in the attached patch I've implemented this slightly differently. If the
tooling to do so is available, the indent-* targets explained above,
use/depend on src/tools/pgindent/typedefs.list.merged (in the build dir),
which is the combination of a src/tools/pgindent/typedefs.list.local generated
for the local binaries/libraries and the source tree
src/tools/pgindent/typedefs.list.

src/tools/pgindent/typedefs.list.local is generated in fragments, with one
fragment for each build target. That way the whole file doesn't have to be
regenerated all the time, which can save a good bit of time (althoug obviously
less when hacking on the backend).

This makes it quite quick to locally indent, without needing to manually
fiddle around with manually modifying typedefs.list or using a separate
typedefs.list.

In a third commit I added a 'nitpick' configure time option, defaulting to
off, which runs an indentation check. The failure mode of that currently isn't
very helpful though, as it just uses --silent-diff.

All of this currently is meson only, largely because I don't feel like
spending the time messing with the configure build, particularly before there
is any agreement on this being the thing to do.

Greetings,

Andres Freund
>From f7520fd87764cddbde9d6c5d25fdfe5314ec5cbc Mon Sep 17 00:00:00 2001
From: Andres Freund <[email-masked]>
Date: Sat, 27 May 2023 11:38:27 -0700
Subject: [PATCH v3 1/3] Add meson targets to run pgindent

Author:
Reviewed-By:
Discussion: https://postgr.es/m/
Backpatch:
---
 meson.build                 | 38 +++++++++++++++++++++++++++++++++++++
 src/tools/pgindent/pgindent | 12 ++++++++++--
 2 files changed, 48 insertions(+), 2 deletions(-)

diff --git a/meson.build b/meson.build
index 862c955453f..2cfeb60eb07 100644
--- a/meson.build
+++ b/meson.build
@@ -3013,6 +3013,44 @@ run_target('install-test-files',

+###############################################################
+# Indentation and similar targets
+###############################################################
+
+indent_base_cmd = [perl, files('src/tools/pgindent/pgindent'),
+    '--indent', pg_bsd_indent.full_path(),
+    '--sourcetree=@SOURCE_ROOT@']
+indent_depend = [pg_bsd_indent]
+
+# reindent the entire tree
+# Reindent the entire tree
+run_target('indent-tree',
+  command: indent_base_cmd + ['.'],
+  depends: indent_depend
+)
+
+# Reindent the last commit
+run_target('indent-head',
+  command: indent_base_cmd + ['--commit=HEAD'],
+  depends: indent_depend
+)
+
+# Silently check if any file is not corectly indented (fails the build if
+# there are differences)
+run_target('indent-check',
+  command: indent_base_cmd + ['--silent-diff', '.'],
+  depends: indent_depend
+)
+
+# Show a diff of all the unindented files (doesn't fail the build if there are
+# differences)
+run_target('indent-diff',
+  command: indent_base_cmd + ['--show-diff', '.'],
+  depends: indent_depend
+)
+
+
+
 ###############################################################
 # Test prep
 ###############################################################
diff --git a/src/tools/pgindent/pgindent b/src/tools/pgindent/pgindent
index bce63d95daf..08b0c95814f 100755
--- a/src/tools/pgindent/pgindent
+++ b/src/tools/pgindent/pgindent
@@ -23,7 +23,7 @@ my $devnull = File::Spec->devnull;

 $help = 0;

 usage() if $help;

>From b8d31613acfc9d554640b4e292f366d79084bb8e Mon Sep 17 00:00:00 2001
From: Andres Freund <[email-masked]>
Date: Wed, 18 Oct 2023 16:18:58 -0700
Subject: [PATCH v3 2/3] heavily wip: update typedefs

---
 meson.build                           | 14 +++++-
 src/backend/jit/llvm/meson.build      |  2 +-
 src/backend/snowball/meson.build      |  2 +-
 src/timezone/meson.build              |  2 +-
 src/tools/pgindent/merge_typedefs     | 24 +++++++++
 src/tools/pgindent/meson.build        | 70 +++++++++++++++++++++++++++
 src/tools/pgindent/typedefs_from_objs | 42 ++++++++++++++++
 7 files changed, 152 insertions(+), 4 deletions(-)
 create mode 100755 src/tools/pgindent/merge_typedefs
 create mode 100644 src/tools/pgindent/meson.build
 create mode 100755 src/tools/pgindent/typedefs_from_objs

diff --git a/meson.build b/meson.build
index 2cfeb60eb07..70f6b680c2c 100644
--- a/meson.build
+++ b/meson.build
@@ -2579,6 +2579,7 @@ add_project_link_arguments(ldflags, language: ['c', 'cpp'])
 
 # list of targets for various alias targets
 backend_targets = []
+data_targets = []
 bin_targets = []
 pl_targets = []
 contrib_targets = []
@@ -2894,6 +2895,8 @@ subdir('src/interfaces/ecpg/test')
 
 subdir('doc/src/sgml')
 
+subdir('src/tools/pgindent')
+
 generated_sources_ac += {'': ['GNUmakefile']}
 
 # After processing src/test, add test_install_libs to the testprep_targets
@@ -2982,6 +2985,7 @@ endif
 all_built = [
   backend_targets,
   bin_targets,
+  data_targets,
   libpq_st,
   pl_targets,
   contrib_targets,
@@ -3017,12 +3021,20 @@ run_target('install-test-files',
 # Indentation and similar targets
 ###############################################################
 
+# If the dependencies for generating a local typedefs.list are fulfilled, we
+# use a combination of a locally built and the source tree's typededefs.list
+# file (see src/tools/pgindent/meson.build) for reindenting. That ensures
+# newly added typedefs are indented correctly.
 indent_base_cmd = [perl, files('src/tools/pgindent/pgindent'),
     '--indent', pg_bsd_indent.full_path(),
     '--sourcetree=@SOURCE_ROOT@']
 indent_depend = [pg_bsd_indent]
 
-# reindent the entire tree
+if typedefs_supported
+  indent_base_cmd += ['--typedefs', typedefs_merged.full_path()]
+  indent_depend += typedefs_merged
+endif
+
 # Reindent the entire tree
 run_target('indent-tree',
   command: indent_base_cmd + ['.'],
diff --git a/src/backend/jit/llvm/meson.build b/src/backend/jit/llvm/meson.build
index 8ffaf414609..7e2f6d5bd5c 100644
--- a/src/backend/jit/llvm/meson.build
+++ b/src/backend/jit/llvm/meson.build
@@ -82,4 +82,4 @@ llvmjit_types = custom_target('llvmjit_types.bc',
   install_dir: dir_lib_pkg,
   depfile: '@BASENAME@.c.bc.d',
 )
-backend_targets += llvmjit_types
+data_targets += llvmjit_types
diff --git a/src/backend/snowball/meson.build b/src/backend/snowball/meson.build
index 0f669c0bf3c..69e777399fd 100644
--- a/src/backend/snowball/meson.build
+++ b/src/backend/snowball/meson.build
@@ -94,4 +94,4 @@ install_subdir('stopwords',
 )
 
 backend_targets += dict_snowball
-backend_targets += snowball_create
+data_targets += snowball_create
diff --git a/src/timezone/meson.build b/src/timezone/meson.build
index 7b85a01c6bd..779a51d85a4 100644
--- a/src/timezone/meson.build
+++ b/src/timezone/meson.build
@@ -50,7 +50,7 @@ if get_option('system_tzdata') == ''
     install_dir: dir_data,
   )
 
-  bin_targets += tzdata
+  data_targets += tzdata
 endif
 
 subdir('tznames')
diff --git a/src/tools/pgindent/merge_typedefs b/src/tools/pgindent/merge_typedefs
new file mode 100755
index 00000000000..c2e1f446c15
--- /dev/null
+++ b/src/tools/pgindent/merge_typedefs
@@ -0,0 +1,24 @@
+#!/usr/bin/env python3
+
+# Tool to merge multiple typedefs files into one
+
+import argparse
+import os
+import subprocess
+import sys
+
+parser = argparse.ArgumentParser(
+    description='merge multiple typedef files')
+
+parser.add_argument('--output', type=argparse.FileType('w'), required=True)
+parser.add_argument('input', type=argparse.FileType('r'), nargs='*')
+
+args = parser.parse_args()
+
+typedefs = set()
+
+for input in args.input:
+    for typedef in input.readlines():
+        typedefs.add(typedef.strip())
+
+print('\n'.join(sorted(typedefs)), file=args.output)
diff --git a/src/tools/pgindent/meson.build b/src/tools/pgindent/meson.build
new file mode 100644
index 00000000000..c0801a6c86c
--- /dev/null
+++ b/src/tools/pgindent/meson.build
@@ -0,0 +1,70 @@
+# Currently the code requires meson 0.63 or upwards. This could likely be
+# lifted (in fact, some older versions actually work, but only some), but for
+# now it's not crucial.
+typedefs_supported = meson.version().version_compare('>=0.63')
+if not typedefs_supported
+  subdir_done()
+endif
+
+typedefs_from_objs = files('typedefs_from_objs')
+typedefs_from_objs_cmd = [typedefs_from_objs, '--host', host_system, '--output', '@OUTPUT@', '@INPUT@']
+merge_typedefs = files('merge_typedefs')
+merge_typedefs_cmd = [merge_typedefs, '--output', '@OUTPUT@', '@INPUT@']
+
+# XXX: This list of targets should likely not be maintained here
+typedef_src_tgts = [
+  backend_targets,
+  bin_targets,
+  [libpq_st],
+  pl_targets,
+  contrib_targets,
+  ecpg_targets,
+]
+
+# We generate partial typedefs files for each binary/library. That makes using
+# this during incremental development much faster.
+#
+# The reason we process the object files instead of executables is that that
+# doesn't work on macos. There doesn't seem to be a reason to target
+# executables directly on other platforms.
+typedef_tgts = []
+foreach tgts : typedef_src_tgts
+  foreach tgt : tgts
+    # can't use tgt.name(), as we have have targets that differ just in suffix
+    name = fs.name(tgt.full_path())
+    tdname = 'typedefs.list.local-@0@'.format(name)
+    objs = tgt.extract_all_objects(recursive: true)
+    typedef_tgts += custom_target(tdname,
+                                  output: tdname,
+                                  command: typedefs_from_objs_cmd,
+                                  input: objs,
+                                  build_by_default: false,
+                                  )
+
+  endforeach
+endforeach
+
+# A typedefs.list file for the local build tree. This may not contain typedefs
+# referenced e.g. by platform specific code for other platforms.
+typedefs_local = custom_target('typedefs.list.local', output: 'typedefs.list.local',
+  command: merge_typedefs_cmd,
+  input: typedef_tgts,
+  build_by_default: false,
+)
+
+# The locally generated typedef.list, merge with the source tree version. This
+# is useful for reindenting code, particularly when the local tree has new
+# types added.
+typedefs_merged = custom_target('typedefs.list.merged', output: 'typedefs.list.merged',
+  command: merge_typedefs_cmd,
+  input: [typedefs_local, files('typedefs.list')],
+  build_by_default: false,
+)
+
+# FIXME: As-is this is rarely useful, as the local typedefs file will likely
+# contain too many changes.
+if cp.found()
+  run_target('update-typedefs',
+    command: [cp, typedefs_merged, '@SOURCE_ROOT@/src/tools/pgindent/typedefs.list'],
+  )
+endif
diff --git a/src/tools/pgindent/typedefs_from_objs b/src/tools/pgindent/typedefs_from_objs
new file mode 100755
index 00000000000..821c84cb3d8
--- /dev/null
+++ b/src/tools/pgindent/typedefs_from_objs
@@ -0,0 +1,42 @@
+#!/usr/bin/env python3
+
+# Tool to extract typedefs from object files.
+
+import argparse
+import re
+import shutil
+import subprocess
+import sys
+
+parser = argparse.ArgumentParser(
+    description='generate typedefs file for a set of object files')
+
+parser.add_argument('--output', type=argparse.FileType('w'), required=True)
+parser.add_argument('--host', type=str, required=True)
+parser.add_argument('input', nargs='*')
+
+args = parser.parse_args()
+
+if args.host == 'linux':
+    find_td_re = re.compile(r'^[^\n]+TAG_typedef\)\n[^\n]+DW_AT_name\s*:\s*\([^\)]+\): ([^\n]+)$', re.MULTILINE)
+    # FIXME: should probably be set by the caller? Except that the same binary
+    # name behaves very differently on different platforms :/
+    cmd = [shutil.which('objdump'), '-Wi']
+elif args.host == 'darwin':
+    find_td_re = re.compile(r'^[^\n]+TAG_typedef\n\s*DW_AT_type[^\n]+\n\s+DW_AT_name\s*\(\"([^\n]+)\"\)$', re.MULTILINE)
+    cmd = [shutil.which('dwarfdump')]
+else:
+    raise f'unsupported platform: {args.host}'
+
+lcmd = cmd + args.input
+sp = subprocess.run(lcmd, stdout=subprocess.PIPE, universal_newlines=True)
+if sp.returncode != 0:
+    print(f'{lcmd} failed with return code {sp.returncode}', file=sys.stderr)
+    sys.exit(sp.returncode)
+
+fa = find_td_re.findall(sp.stdout)
+
+for typedef in fa:
+    print(typedef, file=args.output)
+
+sys.exit(0)
-- 
2.38.0

>From 6688a89700af997b5adaa55f349c8db53a86290b Mon Sep 17 00:00:00 2001
From: Andres Freund <[email-masked]>
Date: Wed, 18 Oct 2023 17:51:21 -0700
Subject: [PATCH v3 3/3] WIP: meson: nitpick mode + test checking indentation

Author:
Reviewed-by:
Discussion: https://postgr.es/m/
Backpatch:
---
 meson.build       | 17 +++++++++++++++--
 meson_options.txt |  3 +++
 .cirrus.tasks.yml |  1 +
 3 files changed, 19 insertions(+), 2 deletions(-)

diff --git a/meson.build b/meson.build
index 70f6b680c2c..86a5d0995a1 100644
--- a/meson.build
+++ b/meson.build
@@ -44,6 +44,7 @@ cc = meson.get_compiler('c')
 not_found_dep = dependency('', required: false)
 thread_dep = dependency('threads')
 auto_features = get_option('auto_features')
+nitpick_mode = get_option('nitpick')

@@ -3025,9 +3026,10 @@ run_target('install-test-files',
 # use a combination of a locally built and the source tree's typededefs.list
 # file (see src/tools/pgindent/meson.build) for reindenting. That ensures
 # newly added typedefs are indented correctly.
-indent_base_cmd = [perl, files('src/tools/pgindent/pgindent'),
+indent_base_args = [files('src/tools/pgindent/pgindent'),
     '--indent', pg_bsd_indent.full_path(),
-    '--sourcetree=@SOURCE_ROOT@']
+    '--sourcetree=@0@'.format(meson.current_source_dir())]
+indent_base_cmd = [perl, indent_base_args]
 indent_depend = [pg_bsd_indent]
 
 if typedefs_supported
@@ -3364,6 +3366,17 @@ if meson.version().version_compare('>=0.57')
 endif

+# If we were asked to be nitpicky, add a test that fails if sources aren't
+# properly indented
+if nitpick_mode
+  test('indent-check',
+    perl, args: [indent_base_args, '--silent-diff', '.'],
+    depends: indent_depend,
+    suite: 'source',
+    priority: 10)
+endif
+
+
 
 ###############################################################
 # Pseudo targets
diff --git a/meson_options.txt b/meson_options.txt
index d2f95cfec36..90cb2177a44 100644
--- a/meson_options.txt
+++ b/meson_options.txt
@@ -52,6 +52,9 @@ option('atomics', type: 'boolean', value: true,
 option('spinlocks', type: 'boolean', value: true,
   description: 'Use spinlocks')
 
+option('nitpick', type: 'boolean', value: false,
+  description: 'annoy the hell out of you, committers ought to enable this')
+
 
 # Compilation options
 
diff --git a/.cirrus.tasks.yml b/.cirrus.tasks.yml
index e137769850d..5dd29bed4cb 100644
--- a/.cirrus.tasks.yml
+++ b/.cirrus.tasks.yml
@@ -358,6 +358,7 @@ task:
           meson setup \
             --buildtype=debug \
             -Dcassert=true \
+            -Dnitpick=true \
             ${LINUX_MESON_FEATURES} \
             -DPG_TEST_EXTRA="$PG_TEST_EXTRA" \
             build
-- 
2.38.0

---

From: Tom Lane <[email-masked]>
Date: Wed, 18 Oct 2023 21:29:37 -0400

>  1 file changed, 351 insertions(+), 1 deletion(-)

That seems like it needs a considerably closer look.  What exactly
is getting added/deleted?

> more aggressively. Sure looks like the source systems are on the older side.

Really?  Per [1] we've currently got contributions from calliphoridae
which is Debian sid, crake which is Fedora 38, indri/sifaka which are
macOS Sonoma.  Were you really expecting something newer, and if so what?

> src/tools/pgindent/typedefs.list.

Hmm ... that allows indenting your C files, but how do you get from that
to a non-noisy patch to commit to typedefs.list?

			regards, tom lane

[1] https://buildfarm.postgresql.org/cgi-bin/typedefs.pl?show_list

---

From: Andres Freund <[email-masked]>
Date: Wed, 18 Oct 2023 19:18:13 -0700

Hi,

> is getting added/deleted?

Types from bison, openssl, libxml, libxslt, icu and libc, at least. If I
enable LLVM, there are even more.

(I think I figured out what's happening further down)

> macOS Sonoma.  Were you really expecting something newer, and if so what?

It's quite odd, I see plenty more types than those. I can't really explain why
they're not being picked up on those animals.

E.g. for me bison generated files contain typedefs like

typedef int_least8_t yytype_int8;
typedef signed char yytype_int8;
typedef yytype_int8 yy_state_t;

yet they don't show up in the buildfarm typedefs output.   It's not a thing of
the binary, I checked that the symbols are present.

I have a hard time parsing the buildfarm code for generating the typedefs
file, tbh.

<stare>

Ah, I see. If I interpret that correctly, the code filters out symbols it
doesn't find in in some .[chly] file in the *source* directory. This code is,
uh, barely readable and massively underdocumented.

I guess I need to reimplement that :/.  Don't immediately see how this could
be implemented for in-tree autoconf builds...

> to a non-noisy patch to commit to typedefs.list?

It doesn't provide that on its own. Being able to painlessly indent the files
seems pretty worthwhile already. But clearly it'd much better if we can
automatically update typedefs.list.

Greetings,

Andres Freund

---

From: Andres Freund <[email-masked]>
Date: Wed, 18 Oct 2023 21:49:07 -0700

Hi,

> automatically update typedefs.list.

With code for that added, things seem to work quite nicely.  I added similar
logic to the buildfarm code that builds a list of all tokens in the source
code.

With that, the in-tree typedefs.list can be updated with new tokens found
locally *and* typdefs that aren't used anymore can be removed from the in-tree
typedefs.list (detected by no matching tokens found in the source code).

The only case this approach can't handle is newly referenced typedefs in code
that isn't built locally - which I think isn't particularly common and seems
somewhat fundamental. In those cases typedefs.list still can be updated
manually (and the sorting will still be "fixed" if necessary).

The code is still in a somewhat rough shape and I'll not finish polishing it
today. I've attached the code anyway, don't be too rough :).

The changes from running "ninja update-typedefs indent-tree" on debian and
macos are attached as 0004 - the set of changes looks quite correct to me.

The buildfarm code filtered out a few typedefs manually:
  push(@badsyms, 'date', 'interval', 'timestamp', 'ANY');
but I don't really see why? Possibly that was needed with an older
pg_bsd_indent to prevent odd stuff?

Right now building a new unified typedefs.list and copying it to the source
tree are still separate targets, but that probably makes less sense now? Or
perhaps it should be copied to the source tree when reindenting?

I've only handled linux and macos in the typedefs gathering code. But the
remaining OSs should be "just a bit of work" [TM].

Greetings,

Andres Freund
From: Andres Freund <[email-masked]>
Date: Sat, 27 May 2023 11:38:27 -0700
Subject: [PATCH v4 1/4] Add meson targets to run pgindent

Author:
Reviewed-By:

 

 $help = 0;

 usage() if $help;

>From dfe5ad9229e812d0f805d6a39cb296b9284e30f2 Mon Sep 17 00:00:00 2001
From: Andres Freund <[email-masked]>
Date: Wed, 18 Oct 2023 16:18:58 -0700
Subject: [PATCH v4 2/4] heavily wip: update typedefs

---
 meson.build                           | 14 ++++-
 src/backend/jit/llvm/meson.build      |  2 +-
 src/backend/snowball/meson.build      |  2 +-
 src/timezone/meson.build              |  2 +-
 src/tools/pgindent/merge_typedefs     | 86 +++++++++++++++++++++++++++
 src/tools/pgindent/meson.build        | 69 +++++++++++++++++++++
 src/tools/pgindent/typedefs_from_objs | 42 +++++++++++++
 7 files changed, 213 insertions(+), 4 deletions(-)
 create mode 100755 src/tools/pgindent/merge_typedefs
 create mode 100644 src/tools/pgindent/meson.build
 create mode 100755 src/tools/pgindent/typedefs_from_objs

diff --git a/meson.build b/meson.build
index 2cfeb60eb07..492ce9a48b1 100644
--- a/meson.build
+++ b/meson.build
@@ -2579,6 +2579,7 @@ add_project_link_arguments(ldflags, language: ['c', 'cpp'])

 subdir('doc/src/sgml')

 
 
+  indent_depend += typedefs

-  bin_targets += tzdata
+  data_targets += tzdata
 endif
 
 subdir('tznames')
diff --git a/src/tools/pgindent/merge_typedefs b/src/tools/pgindent/merge_typedefs
new file mode 100755
index 00000000000..9826fd1a675
--- /dev/null
+++ b/src/tools/pgindent/merge_typedefs
@@ -0,0 +1,86 @@
+#!/usr/bin/env python3
+
+# Tool to build a new typedefs.list file from
+# a) the current typedefs.list file in the source tree
+# b) newly generated typedefs.list fragments for build products
+# c) the source code
+#
+# To avoid littering the typedefs file with typedefs that are not in our source
+# code, the list of typedefs found is filtered with tokens found in source
+# files.
+#
+# If the local build references a typedef that's not present in the source
+# typedefs.list, we can discover such typedefs.
+#
+# If the source typedefs.list references a typedef that is not used anymore, it
+# will not appear in the source tree anymore and thus can be filtered out.
+#
+# However, typedefs in newly written code for a different platform (or a
+# disabled build option), will not be discovered.
+
+import argparse
+import os
+import subprocess
+import sys
+import re
+
+parser = argparse.ArgumentParser(
+    description='build new typedefs files from multiple inputs')
+
+parser.add_argument('--output-local', type=argparse.FileType('w'), required=True)
+parser.add_argument('--output-merged', type=argparse.FileType('w'), required=True)
+parser.add_argument('--current-typedefs', type=argparse.FileType('r'), required=True)
+parser.add_argument('--filter-source', type=str, required=False)
+parser.add_argument('input', type=argparse.FileType('r'), nargs='*')
+
+
+def merge_typedefs(files):
+    typedefs = set()
+    for input in files:
+        for typedef in input.readlines():
+            typedefs.add(typedef.strip())
+    return typedefs
+
+def source_list(srcdir):
+    filelist = []
+    os.chdir(srcdir)
+    accepted_files = re.compile(r'\.(c|h|l|y|.cpp)$')
+    for root, dirs, files in os.walk('.', topdown=True):
+        # don't go into hidden dirs or docs
+        for d in dirs:
+            if d.startswith('.') or d == 'doc':
+                dirs.remove(d)
+        for f in files:
+            if accepted_files.search(f):
+                filelist.append((os.path.join(root, f)))
+    return filelist
+
+def tokenize_sources(files):
+    comment_sub_re = re.compile(r'/\*.*?\*/', flags=re.MULTILINE|re.DOTALL)
+    token_find_re = re.compile(r'\b\w+\b')
+
+    tokens = set()
+    for file in files:
+        with open(file, 'r') as fh:
+            content = fh.read()
+            content = comment_sub_re.sub('', content)
+            tokens.update(token_find_re.findall(content))
+    return tokens
+
+def main(args):
+    local_typedefs = merge_typedefs(args.input)
+    current_typedefs = merge_typedefs([args.current_typedefs])
+
+    if args.filter_source:
+        filelist = source_list(args.filter_source)
+        tokens = tokenize_sources(filelist)
+        local_typedefs.intersection_update(tokens)
+        current_typedefs.intersection_update(tokens)
+
+    current_typedefs.update(local_typedefs)
+    print('\n'.join(sorted(local_typedefs)), file=args.output_local)
+    print('\n'.join(sorted(current_typedefs)), file=args.output_merged)
+
+if __name__ == '__main__':
+    main(parser.parse_args())
+    sys.exit(0)
diff --git a/src/tools/pgindent/meson.build b/src/tools/pgindent/meson.build
new file mode 100644
index 00000000000..de02c676a46
--- /dev/null
+++ b/src/tools/pgindent/meson.build
@@ -0,0 +1,69 @@
+# Currently the code requires meson 0.63 or upwards. This could likely be
+# lifted (in fact, some older versions actually work, but only some), but for
+# now it's not crucial.
+typedefs_supported = meson.version().version_compare('>=0.63')
+if not typedefs_supported
+  subdir_done()
+endif
+
+typedefs_from_objs = files('typedefs_from_objs')
+typedefs_from_objs_cmd = [typedefs_from_objs, '--host', host_system, '--output', '@OUTPUT@', '@INPUT@']
+merge_typedefs = files('merge_typedefs')
+
+# XXX: This list of targets should likely not be maintained here
+typedef_src_tgts = [
+  backend_targets,
+  bin_targets,
+  [libpq_st],
+  pl_targets,
+  contrib_targets,
+  ecpg_targets,
+]
+
+# We generate partial typedefs files for each binary/library. That makes using
+# this during incremental development much faster.
+#
+# The reason we process the object files instead of executables is that that
+# doesn't work on macos. There doesn't seem to be a reason to target
+# executables directly on other platforms.
+typedef_tgts = []
+foreach tgts : typedef_src_tgts
+  foreach tgt : tgts
+    # can't use tgt.name(), as we have have targets that differ just in suffix
+    name = fs.name(tgt.full_path())
+    tdname = 'typedefs.list.local-@0@'.format(name)
+    objs = tgt.extract_all_objects(recursive: true)
+    typedef_tgts += custom_target(tdname,
+                                  output: tdname,
+                                  command: typedefs_from_objs_cmd,
+                                  input: objs,
+                                  build_by_default: false,
+                                  )
+
+  endforeach
+endforeach
+
+
+# Build new typedefs.list files. typedefs.list.local will contain only
+# typedefs in the local build, whereas typedefs.list.merged is the combination
+# of typedefs.list.local and the source tree's typedefs.list. However,
+# typedefs that are not used in the code anymore are removed even from
+# typedefs.list.merged.
+typedefs = custom_target('typedefs.list', output: ['typedefs.list.local', 'typedefs.list.merged'],
+  command: [merge_typedefs,
+            '--output-local', '@OUTPUT0@',
+            '--output-merged', '@OUTPUT1@',
+            '--current-typedefs', files('typedefs.list'),
+            '--filter-source', '@SOURCE_ROOT@',
+            '@INPUT@'],
+  input: typedef_tgts,
+  build_by_default: false,
+)
+typedefs_local = typedefs[0]
+typedefs_merged = typedefs[1]
+
+if cp.found()
+  run_target('update-typedefs',
+    command: [cp, typedefs_merged, '@SOURCE_ROOT@/src/tools/pgindent/typedefs.list'],
+  )
+endif
diff --git a/src/tools/pgindent/typedefs_from_objs b/src/tools/pgindent/typedefs_from_objs
new file mode 100755
index 00000000000..821c84cb3d8
--- /dev/null
+++ b/src/tools/pgindent/typedefs_from_objs
@@ -0,0 +1,42 @@
+#!/usr/bin/env python3
+
+# Tool to extract typedefs from object files.
+
+import argparse
+import re
+import shutil
+import subprocess
+import sys
+
+parser = argparse.ArgumentParser(
+    description='generate typedefs file for a set of object files')
+
+parser.add_argument('--output', type=argparse.FileType('w'), required=True)
+parser.add_argument('--host', type=str, required=True)
+parser.add_argument('input', nargs='*')
+
+args = parser.parse_args()
+
+if args.host == 'linux':
+    find_td_re = re.compile(r'^[^\n]+TAG_typedef\)\n[^\n]+DW_AT_name\s*:\s*\([^\)]+\): ([^\n]+)$', re.MULTILINE)
+    # FIXME: should probably be set by the caller? Except that the same binary
+    # name behaves very differently on different platforms :/
+    cmd = [shutil.which('objdump'), '-Wi']
+elif args.host == 'darwin':
+    find_td_re = re.compile(r'^[^\n]+TAG_typedef\n\s*DW_AT_type[^\n]+\n\s+DW_AT_name\s*\(\"([^\n]+)\"\)$', re.MULTILINE)
+    cmd = [shutil.which('dwarfdump')]
+else:
+    raise f'unsupported platform: {args.host}'
+
+lcmd = cmd + args.input
+sp = subprocess.run(lcmd, stdout=subprocess.PIPE, universal_newlines=True)
+if sp.returncode != 0:
+    print(f'{lcmd} failed with return code {sp.returncode}', file=sys.stderr)
+    sys.exit(sp.returncode)
+
+fa = find_td_re.findall(sp.stdout)
+
+for typedef in fa:
+    print(typedef, file=args.output)
+
+sys.exit(0)
-- 
2.38.0

>From 017183e75c7a633dedabdbf57aa1ac80ee955f93 Mon Sep 17 00:00:00 2001
From: Andres Freund <[email-masked]>
Date: Wed, 18 Oct 2023 21:40:36 -0700
Subject: [PATCH v4 3/4] WIP: meson: nitpick mode + test checking indentation

Author:
Reviewed-by:
Discussion: https://postgr.es/m/
Backpatch:
---
 meson.build       | 20 +++++++++++++++++---
 meson_options.txt |  3 +++
 .cirrus.tasks.yml |  1 +
 3 files changed, 21 insertions(+), 3 deletions(-)

index 492ce9a48b1..854b7ecf56c 100644

@@ -3025,16 +3026,18 @@ run_target('install-test-files',
 
 if typedefs_supported
-  indent_base_cmd += ['--typedefs', typedefs_merged.full_path()]
+  indent_base_args += ['--typedefs', typedefs_merged.full_path()]
   indent_depend += typedefs
 endif
 
@@ -3364,6 +3367,17 @@ if meson.version().version_compare('>=0.57')
 endif

 
 
 # Compilation options

>From 7e06ca28697b365561371b2843d88c4caaccd347 Mon Sep 17 00:00:00 2001
From: Andres Freund <[email-masked]>
Date: Wed, 18 Oct 2023 21:31:22 -0700
Subject: [PATCH v4 4/4] Update typedefs, reindent using the new build targets

>From debian sid and macos ventura.
---
 contrib/sepgsql/hooks.c          |  2 +-
 contrib/sepgsql/label.c          |  2 +-
 contrib/sepgsql/uavc.c           |  2 +-
 src/tools/pgindent/typedefs.list | 14 +++++++++++---
 4 files changed, 14 insertions(+), 6 deletions(-)

diff --git a/contrib/sepgsql/hooks.c b/contrib/sepgsql/hooks.c
index fa734763602..443e30be994 100644
--- a/contrib/sepgsql/hooks.c
+++ b/contrib/sepgsql/hooks.c
@@ -50,7 +50,7 @@ typedef struct
 	 * command. Elsewhere (including the case of default) NULL.
 	 */
 	const char *createdb_dtemplate;
-}			sepgsql_context_info_t;
+} sepgsql_context_info_t;
 
 static sepgsql_context_info_t sepgsql_context_info;
 
diff --git a/contrib/sepgsql/label.c b/contrib/sepgsql/label.c
index 38ff4068ff2..e13640dc4d9 100644
--- a/contrib/sepgsql/label.c
+++ b/contrib/sepgsql/label.c
@@ -67,7 +67,7 @@ typedef struct
 {
 	SubTransactionId subid;
 	char	   *label;
-}			pending_label;
+} pending_label;
 
 /*
  * sepgsql_get_client_label
diff --git a/contrib/sepgsql/uavc.c b/contrib/sepgsql/uavc.c
index 6e3a892da24..6f63a19aa96 100644
--- a/contrib/sepgsql/uavc.c
+++ b/contrib/sepgsql/uavc.c
@@ -44,7 +44,7 @@ typedef struct
 	/* true, if tcontext is valid */
 	char	   *ncontext;		/* temporary scontext on execution of trusted
 								 * procedure, or NULL elsewhere */
-}			avc_cache;
+} avc_cache;
 
 /*
  * Declaration of static variables
diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list
index e69bb671bf8..d6178386252 100644
--- a/src/tools/pgindent/typedefs.list
+++ b/src/tools/pgindent/typedefs.list
@@ -2,6 +2,7 @@ ACCESS_ALLOWED_ACE
 ACL
 ACL_SIZE_INFORMATION
 AFFIX
+ANY
 ASN1_INTEGER
 ASN1_OBJECT
 ASN1_OCTET_STRING
@@ -1275,9 +1276,9 @@ JsonManifestWALRangeField
 JsonObjectAgg
 JsonObjectConstructor
 JsonOutput
-JsonParseExpr
 JsonParseContext
 JsonParseErrorType
+JsonParseExpr
 JsonPath
 JsonPathBool
 JsonPathExecContext
@@ -1913,7 +1914,6 @@ ParallelHashJoinBatch
 ParallelHashJoinBatchAccessor
 ParallelHashJoinState
 ParallelIndexScanDesc
-ParallelReadyList
 ParallelSlot
 ParallelSlotArray
 ParallelSlotResultHandler
@@ -3135,6 +3135,7 @@ _ino_t
 _locale_t
 _resultmap
 _stringlist
+access_vector_t
 acquireLocksOnSubLinks_context
 add_nulling_relids_context
 adjust_appendrel_attrs_context
@@ -3167,6 +3168,7 @@ assign_collations_context
 auth_password_hook_typ
 autovac_table
 av_relation
+avc_cache
 avl_dbase
 avl_node
 avl_tree
@@ -3247,6 +3249,7 @@ crosstab_HashEnt
 crosstab_cat_desc
 datapagemap_iterator_t
 datapagemap_t
+date
 dateKEY
 datetkn
 dce_uuid_t
@@ -3402,6 +3405,7 @@ indexed_tlist
 inet
 inetKEY
 inet_struct
+initRowMethod
 init_function
 inline_cte_walker_context
 inline_error_callback_arg
@@ -3418,6 +3422,7 @@ int64
 int64KEY
 int8
 internalPQconninfoOption
+interval
 intptr_t
 intset_internal_node
 intset_leaf_node
@@ -3519,6 +3524,7 @@ parse_error_callback_arg
 parser_context
 partition_method_t
 pendingPosition
+pending_label
 pgParameterStatus
 pg_atomic_flag
 pg_atomic_uint32
@@ -3710,7 +3716,9 @@ saophash_hash
 save_buffer
 scram_state
 scram_state_enum
+security_class_t
 sem_t
+sepgsql_context_info_t
 sequence_magic
 set_join_pathlist_hook_type
 set_rel_pathlist_hook_type
@@ -3792,6 +3800,7 @@ time_t
 timeout_handler_proc
 timeout_params
 timerCA
+timestamp
 tlist_vinfo
 toast_compress_header
 tokenize_error_callback_arg
@@ -3869,7 +3878,6 @@ wchar2mb_with_len_converter
 wchar_t
 win32_deadchild_waitinfo
 wint_t
-worker_spi_state
 worker_state
 worktable
 wrap
-- 
2.38.0

---

From: Jeff Davis <[email-masked]>
Date: Mon, 23 Oct 2023 17:50:52 -0700

> you listed that --enable-indent-checks would have saved from breaking

I'd find that a useful option.

Regards,
	Jeff Davis

---

From: Amit Langote <[email-masked]>
Date: Tue, 24 Oct 2023 10:16:55 +0900

> I'd find that a useful option.

+1.  While I’ve made it part of routine to keep my local work pgindented
since breaking Joel once, an option like this would still be useful.

---

From: Michael Paquier <[email-masked]>
Date: Tue, 24 Oct 2023 13:31:32 +0900

> since breaking Joel once, an option like this would still be useful.

I'd be OK with an option like that.  It is one of these things to type
once in a script, then forget about it.
--
Michael

---

From: Andrew Dunstan <[email-masked]>
Date: Tue, 24 Oct 2023 09:53:00 -0400

> know I probably wouldn't (or at least not always).

Yeah. In fact I'm betting that a lot of the offending commits we've seen 
come into this category. You build, you check, then you do some final 
polish. That's where a pre-commit hook can save you.

cheers

andrew

---

From: Amit Kapila <[email-masked]>
Date: Wed, 25 Oct 2023 15:12:33 +0530

+1.

-- 
With Regards,
Amit Kapila.

---

From: Andrew Dunstan <[email-masked]>
Date: Sat, 28 Oct 2023 11:47:44 -0400

On 2023-08-12 Sa 11:57, Andrew Dunstan wrote:
> Yeah, part of the point of creating koel was to give committers a bit 
> of a nudge in that direction.
>

Based on recent experience, where a lot koel's recent complaints seem to 
be about comments, I'd like to suggest a modest adjustment.

First, we should provide a mode of pgindent that doesn't reflow 
comments. pg_bsd_indent has a flag for this (-nfcb), so this should be 
relatively simple.  Second, koel could use that mode, so that it 
wouldn't complain about comments it thinks need to be reflowed. Of 
course, we'd fix these up with our regular pgindent runs.

cheers

andrew

---

From: Tom Lane <[email-masked]>
Date: Sat, 28 Oct 2023 12:09:15 -0400

> course, we'd fix these up with our regular pgindent runs.

Seems like a bit of a kluge.  Maybe it's the right thing to do, but
I don't think we have enough data points yet to be confident that
it'd meaningfully reduce the number of breakages.

On a more abstract level: the point of trying to maintain indent
cleanliness is so that if you modify a file and then want to run
pgindent on your own changes, you don't get incidental changes
elsewhere in the file.  This solution would break that, so I'm
not sure it isn't throwing the baby out with the bathwater.

			regards, tom lane

---

From: Andrew Dunstan <[email-masked]>
Date: Sun, 29 Oct 2023 10:22:27 -0400

> not sure it isn't throwing the baby out with the bathwater.

Yeah, could be.

cheers

andrew.