Re: run pgindent on a regular basis / scripted manner

[Andrew Dunstan]

On 2023-10-28 Sa 12:09, Tom Lane wrote:

Andrew Dunstan  writes:

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.

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.



Yeah, could be.


cheers


andrew.

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

Re: run pgindent on a regular basis / scripted manner

[Tom Lane]

Andrew Dunstan  writes:
> 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.

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

Re: run pgindent on a regular basis / scripted manner

[Andrew Dunstan]

On 2023-08-12 Sa 11:57, Andrew Dunstan wrote:



On 2023-08-11 Fr 19:17, Tom Lane wrote:

Peter Geoghegan  writes:

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.

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.



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.




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


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

Re: run pgindent on a regular basis / scripted manner

[Andrew Dunstan]

On 2023-10-27 Fr 03:14, Étienne BERSAC wrote:

Hello,



Yes, there's a lot to look out for, and you're a damn sight better at
it
than I am. But we should try to automate the things that can be
automated, even if that leaves many tasks that can't be. I have three
things in my pre-commit hook: a check for catalog updates, a check
for
new typedefs, and an indent check.

Could you share your configuration ? Could we provide more helper and
integration to help produce consistent code ?



Sure. pre-commit hook file attached. I'm sure this could be improved on.


cheers


andrew


--
Andrew Dunstan
EDB: https://www.enterprisedb.com
#!/bin/sh

set -u

: ${PGAUTOINDENT:=no}
: ${PGTYPEDEFCHECK:=no}

branch=$(git rev-parse --abbrev-ref HEAD)
files=$(git diff --cached --name-only --diff-filter=ACMR)

check_typedef() {

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

test $PGTYPEDEFCHECK = yes || return 0

tdmod=`git status --porcelain src/tools/pgindent/typedefs.list`

test -z "$tdmod" && return 0

tdfiles=`git diff --cached --name-only -Stypedef $files | grep 
'[.][ch]$'`

test -z "$tdfiles" && return 0

# changes include typedef but list not changed
{
echo 'Commit on master contains a typedef but typedefs.list not 
changed'
echo 'It can be forced with git commit --no-verify'
echo 'or PGTYPEDEFCHECK=no'
} >&2

exit 1

}

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
}

check_indent () {

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

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

# but make sure we have some

perl -e 'foreach (@ARGV) { exit 0 if /\.[ch]$/; }; exit 1;' $files || \
return 0;

echo Indenting $files

src/tools/pgindent/pgindent --silent-diff $files && return 0

exec 2>&1

if [ "$PGAUTOINDENT" = yes ] ; then
echo "Running pgindent on changed files"
src/tools/pgindent/pgindent $files
echo "You need to rerun git commit to pick up pgindent changes"
else
echo 'Need a pgindent run, e.g:'
echo -n 'src/tools/pgindent/pgindent '
echo '`git diff --name-only --diff-filter=ACMR`'
fi

exit 1
}

# nothing to do if there are no files
test -z "$files" && exit 0

check_catalog_version
check_indent
check_typedef

Re: run pgindent on a regular basis / scripted manner

[Étienne BERSAC]

Hello,


> Yes, there's a lot to look out for, and you're a damn sight better at
> it 
> than I am. But we should try to automate the things that can be 
> automated, even if that leaves many tasks that can't be. I have three
> things in my pre-commit hook: a check for catalog updates, a check
> for 
> new typedefs, and an indent check.

Could you share your configuration ? Could we provide more helper and
integration to help produce consistent code ?

For logfmt extension, I configured clang-formatd so that Emacs format
the buffer on save. Any editor running clangd will use this. This is
ease my mind about formatting. I need to investigate how to use
pgindent instead or at lease ensure clang-format produce same output as
pgindent.

https://gitlab.com/dalibo/logfmt/-/blob/0d808b368e649b23ac06ce2657354b67be398b21/.clang-format

Automate nitpicking in CI is good, but checking locally before sending
the patch will save way more round-trip.

Regards,
Étienne

Re: run pgindent on a regular basis / scripted manner

[Amit Kapila]

On Tue, Oct 24, 2023 at 6:21 AM Jeff Davis  wrote:
>
> On Wed, 2023-10-18 at 22:34 +1300, David Rowley wrote:
> > 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.
>
> I'd find that a useful option.
>

+1.

-- 
With Regards,
Amit Kapila.

Re: run pgindent on a regular basis / scripted manner

[Andrew Dunstan]

On 2023-10-18 We 05:07, Jelte Fennema wrote:

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).



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

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

Re: run pgindent on a regular basis / scripted manner

[Bruce Momjian]

On Tue, Oct 24, 2023 at 09:40:01AM -0400, Andrew Dunstan wrote:
> Slightly off topic, but apropos your message, maybe we should recommend a
> standard git commit template.

I use this and then automatically remove any sections that are empty.

---


|--- gitweb subject length limit |-email limit-|


Reported-by:

Diagnosed-by:

Bug:

Discussion:

Author:

Reviewed-by:

Tested-by:

Backpatch-through:

-- 
  Bruce Momjian  https://momjian.us
  EDB  https://enterprisedb.com

  Only you can decide what is important to you.

Re: run pgindent on a regular basis / scripted manner

[Andrew Dunstan]

On 2023-10-17 Tu 09:52, Robert Haas wrote:

On Tue, Oct 17, 2023 at 6:34 AM Jelte Fennema  wrote:

I think *it is* dead easy to comply. If you run the following commands
before committing/after rebasing, then koel should always be happy:

src/tools/pgindent/pgindent src # works always but a bit slow
src/tools/pgindent/pgindent $(git diff --name-only --diff-filter=ACMR)
# much faster, but only works if you DID NOT change typedefs.list

In isolation, that's true, but the list of mistakes that you can make
while committing which will inconvenience everyone working on the
project is very long. Another one that comes up frequently is
forgetting to bump CATALOG_VERSION_NO, but you also need a good commit
message, and good comments, and a good Discussion link in the commit
message, and the right list of authors and reviewers, and to update
the docs (with spaces, not tabs) and the Makefiles (with tabs, not
spaces) and the meson stuff and, as if that weren't enough already,
you actually need the code to work! And that includes not only working
regularly but also with CLOBBER_CACHE_ALWAYS and debug_parallel_query
and so on. It's very easy to miss something somewhere. I put a LOT of
work into polishing my commits before I push them, and it's still not
that uncommon that I screw something up.



Yes, there's a lot to look out for, and you're a damn sight better at it 
than I am. But we should try to automate the things that can be 
automated, even if that leaves many tasks that can't be. I have three 
things in my pre-commit hook: a check for catalog updates, a check for 
new typedefs, and an indent check. And every one of them has saved me 
from doing things I should not be doing. They aren't perfect but they 
are useful.


Slightly off topic, but apropos your message, maybe we should recommend 
a standard git commit template.



cheers


andrew

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

Re: run pgindent on a regular basis / scripted manner

[Michael Paquier]

On Tue, Oct 24, 2023 at 10:16:55AM +0900, Amit Langote wrote:
> +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.

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


signature.asc
Description: PGP signature

Re: run pgindent on a regular basis / scripted manner

[Amit Langote]

On Tue, Oct 24, 2023 at 9:51 Jeff Davis  wrote:

> On Wed, 2023-10-18 at 22:34 +1300, David Rowley wrote:
> > 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.
>
> 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.

>

Re: run pgindent on a regular basis / scripted manner

[Jeff Davis]

On Wed, 2023-10-18 at 22:34 +1300, David Rowley wrote:
> 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.

I'd find that a useful option.

Regards,
Jeff Davis

Re: run pgindent on a regular basis / scripted manner

[Andres Freund]

Hi,

On 2023-10-18 19:18:13 -0700, Andres Freund wrote:
> On 2023-10-18 21:29:37 -0400, Tom Lane wrote:
> 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...
>
> > > 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.
> >
> > Hmm ... that allows indenting your C files, but how do you get from that
> > 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.

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 f7520fd87764cddbde9d6c5d25fdfe5314ec5cbc Mon Sep 17 00:00:00 2001
From: Andres Freund 
Date: Sat, 27 May 2023 11:38:27 -0700
Subject: [PATCH v4 1/4] 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;
 
 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 @@ 

Re: run pgindent on a regular basis / scripted manner

[Andres Freund]

Hi,

On 2023-10-18 21:29:37 -0400, Tom Lane wrote:
> Andres Freund  writes:
> > 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(-)
>
> That seems like it needs a considerably closer look.  What exactly
> 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)



> > 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.
>
> 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?

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.



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...


> > 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.
>
> Hmm ... that allows indenting your C files, but how do you get from that
> 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

Re: run pgindent on a regular basis / scripted manner

[Tom Lane]

Andres Freund  writes:
> 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(-)

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

> 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.

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?

> 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.

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

Re: run pgindent on a regular basis / scripted manner

[Andres Freund]

Hi,

On 2023-10-18 12:15:51 -0700, Andres Freund wrote:
> 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.

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 
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;
 
 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" => 

Re: run pgindent on a regular basis / scripted manner

[Andres Freund]

Hi,

On 2023-10-16 20:45:00 -0400, Tom Lane wrote:
> Peter Geoghegan  writes:
> 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.

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.


>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.

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 
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
+# 

Re: run pgindent on a regular basis / scripted manner

[Bruce Momjian]

On Tue, Oct 17, 2023 at 11:01:44AM -0400, Robert Haas wrote:
> 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'.

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.

-- 
  Bruce Momjian  https://momjian.us
  EDB  https://enterprisedb.com

  Only you can decide what is important to you.

Re: run pgindent on a regular basis / scripted manner

[Robert Haas]

On Wed, Oct 18, 2023 at 3:21 AM Peter Eisentraut  wrote:
> On 18.10.23 06:40, David Rowley wrote:
> > 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.
>
> 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.

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.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

Re: run pgindent on a regular basis / scripted manner

[Vik Fearing]

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

Re: run pgindent on a regular basis / scripted manner

[David Rowley]

On Wed, 18 Oct 2023 at 22:07, Jelte Fennema  wrote:
> 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.

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.

On Wed, 18 Oct 2023 at 06:40, David Rowley  wrote:
> > 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 --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).

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

Re: run pgindent on a regular basis / scripted manner

[Jelte Fennema]

On Wed, 18 Oct 2023 at 06:40, David Rowley  wrote:
> 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.

> What is their opinion on this?
> 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.

> 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 --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).

Re: run pgindent on a regular basis / scripted manner

[Jelte Fennema]

On Tue, 17 Oct 2023 at 23:01, Magnus Hagander  wrote:
> And unless we're only enforcing it on master, we'd also need to make
> provisions for different versions of it on different branches, I
> think?

Only enforcing on master sounds fine to me, that's what koel is doing
too afaik. In practice this seems to be enough to solve my main issue
of having to manually remove unrelated indents when rebasing my
patches. Enforcing on different branches seems like it would add a lot
of complexity. So I'm not sure that's worth doing at this point, since
currently some committers are proposing to stop enforcing continuous
indentation because of problems with the current flow. I think we
should only enforce it on more branches once we have the flow
mastered.

> It does need a lot more
> sandboxing than what's in there now, but that's not too hard of a
> problem to solve, *if* this is what we want.

Yeah, I didn't bother with that. Since that seems very tightly coupled
with the environment that the git server is running, and I have no
clue what that environment is.

Re: run pgindent on a regular basis / scripted manner

[Magnus Hagander]

On Tue, Oct 17, 2023 at 11:07 PM Tom Lane  wrote:
>
> Magnus Hagander  writes:
> > If it doesn't know how to rebuild it, aren't we going to be stuck in a
> > catch-22 if we need to change it in certain ways? Since an old version
> > of pg_bsd_indent would reject the patch that might include updating
> > it. (And when it does, one should expect the push to take quite a long
> > time, but given the infrequency I agree that part is probably not an
> > issue)
>
> Everyone who has proposed this has included a caveat that there must
> be a way to override the check.  Given that minimal expectation, it
> shouldn't be too hard to deal with pg_bsd_indent-updating commits.

I haven't managed to fully keep up with the thread, so I missed that.
And i can't directly find it looking back either - but as long as
three's an actual idea for how to do that, the problem goes away :)

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

Re: run pgindent on a regular basis / scripted manner

[Peter Eisentraut]

On 18.10.23 06:40, David Rowley wrote:
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.


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.

Re: run pgindent on a regular basis / scripted manner

[David Rowley]

On Wed, 18 Oct 2023 at 01:47, Robert Haas  wrote:
> On Sat, Aug 12, 2023 at 5:53 PM Peter Geoghegan  wrote:
> > This policy isn't working.
>
> +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

Re: run pgindent on a regular basis / scripted manner

[Tom Lane]

Magnus Hagander  writes:
> If it doesn't know how to rebuild it, aren't we going to be stuck in a
> catch-22 if we need to change it in certain ways? Since an old version
> of pg_bsd_indent would reject the patch that might include updating
> it. (And when it does, one should expect the push to take quite a long
> time, but given the infrequency I agree that part is probably not an
> issue)

Everyone who has proposed this has included a caveat that there must
be a way to override the check.  Given that minimal expectation, it
shouldn't be too hard to deal with pg_bsd_indent-updating commits.

regards, tom lane

Re: run pgindent on a regular basis / scripted manner

[Magnus Hagander]

On Tue, Oct 17, 2023 at 10:43 PM Jelte Fennema  wrote:
>
> On Tue, 17 Oct 2023 at 18:53, Maciek Sakrejda  wrote:
> > Git push does have an --atomic flag to treat the entire push as a single 
> > operation.
>
> I decided to play around a bit with server hooks. Attached is a git
> "update" hook that rejects pushes to the master branch when the new
> HEAD of master does not pass pgindent. It tries to do the minimal
> amount of work necessary. Together with the --atomic flag of git push
> I think this would work quite well.
>
> Note: It does require that pg_bsd_indent is in PATH. While not perfect
> seems like it would be acceptable in practice to me. Its version is
> not updated very frequently. So manually updating it on the git server
> when we do does not seem like a huge issue to me.

If it doesn't know how to rebuild it, aren't we going to be stuck in a
catch-22 if we need to change it in certain ways? Since an old version
of pg_bsd_indent would reject the patch that might include updating
it. (And when it does, one should expect the push to take quite a long
time, but given the infrequency I agree that part is probably not an
issue)

And unless we're only enforcing it on master, we'd also need to make
provisions for different versions of it on different branches, I
think?

Other than that, I agree it's fairly simple. It does nede a lot more
sandboxing than what's in there now, but that's not too hard of a
problem to solve, *if* this is what we want.

(And of course needs to be integrated with the existing script since
AFAIK you can't chain git hooks unless you do it manually - but that's
mostliy mechanical)

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

Re: run pgindent on a regular basis / scripted manner

[Jelte Fennema]

On Tue, 17 Oct 2023 at 18:53, Maciek Sakrejda  wrote:
> Git push does have an --atomic flag to treat the entire push as a single 
> operation.

I decided to play around a bit with server hooks. Attached is a git
"update" hook that rejects pushes to the master branch when the new
HEAD of master does not pass pgindent. It tries to do the minimal
amount of work necessary. Together with the --atomic flag of git push
I think this would work quite well.

Note: It does require that pg_bsd_indent is in PATH. While not perfect
seems like it would be acceptable in practice to me. Its version is
not updated very frequently. So manually updating it on the git server
when we do does not seem like a huge issue to me.

The easiest way to try it out is by cloning the postgres repo in two
different local directories, let's call them A and B. And then
configure directory B to be the origin remote of A. By placing the
update script in B/.git/hooks/ it will execute whenever you push
master from A to B.


update
Description: Binary data

Re: run pgindent on a regular basis / scripted manner

[Maciek Sakrejda]

On Tue, Oct 17, 2023, 09:22 Robert Haas  wrote:

> On Tue, Oct 17, 2023 at 12:18 PM Tom Lane  wrote:
> > Robert Haas  writes:
> > Is that actually possible?  I had the idea that "git push" is an
> > atomic operation, ie 100% or nothing.  Is it only atomic per-branch?
>
> I believe so.


Git push does have an --atomic flag to treat the entire push as a single
operation.

Re: run pgindent on a regular basis / scripted manner

[Robert Haas]

On Tue, Oct 17, 2023 at 12:18 PM Tom Lane  wrote:
> Robert Haas  writes:
> > One potential problem with a server-side hook is that if you back-port
> > a commit to older branches and then push the commits all together
> > (which is my workflow) then you might get failure to push on some
> > branches but not others. I don't know if there's any way to avoid
> > that, but it seems not great. You could think of enforcing the policy
> > only on master to try to avoid this, but that still leaves a risk that
> > you manage to push to all the back-branches and not to master.
>
> Is that actually possible?  I had the idea that "git push" is an
> atomic operation, ie 100% or nothing.  Is it only atomic per-branch?

I believe so. For instance:

[rhaas pgsql]$ git push rhaas
Enumerating objects: 2980, done.
Counting objects: 100% (2980/2980), done.
Delta compression using up to 16 threads
Compressing objects: 100% (940/940), done.
Writing objects: 100% (2382/2382), 454.52 KiB | 7.70 MiB/s, done.
Total 2382 (delta 2024), reused 1652 (delta 1429), pack-reused 0
remote: Resolving deltas: 100% (2024/2024), completed with 579 local objects.
To ssh://git.postgresql.org/users/rhaas/postgres.git
   e434e21e11..2406c4e34c  master -> master
 ! [rejected]  walsummarizer2 -> walsummarizer2 (non-fast-forward)
error: failed to push some refs to
'ssh://git.postgresql.org/users/rhaas/postgres.git'
hint: Updates were rejected because a pushed branch tip is behind its remote
hint: counterpart. Check out this branch and integrate the remote changes
hint: (e.g. 'git pull ...') before pushing again.
hint: See the 'Note about fast-forwards' in 'git push --help' for details.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

Re: run pgindent on a regular basis / scripted manner

[Robert Haas]

On Tue, Oct 17, 2023 at 12:17 PM Tom Lane  wrote:
> Hmm, I've not found it that hard to manage the typedefs list.
> If I run pgindent and it adds weird spacing around uses of a new
> typedef name, I go "oh, I better add that to the list" and do so.
> End of problem.  There's not a requirement that you remove disused
> typedef names, nor that you alphabetize perfectly.  I'm content
> to update those sorts of details from the buildfarm's list once a
> year or so.

+1 to all of that. At least for me, managing typedefs.list isn't the
problem. The problem is remembering to actually do it, and keep it
updated as the patch set is adjusted and rebased.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

Re: run pgindent on a regular basis / scripted manner

[Tom Lane]

Robert Haas  writes:
> One potential problem with a server-side hook is that if you back-port
> a commit to older branches and then push the commits all together
> (which is my workflow) then you might get failure to push on some
> branches but not others. I don't know if there's any way to avoid
> that, but it seems not great. You could think of enforcing the policy
> only on master to try to avoid this, but that still leaves a risk that
> you manage to push to all the back-branches and not to master.

Is that actually possible?  I had the idea that "git push" is an
atomic operation, ie 100% or nothing.  Is it only atomic per-branch?

regards, tom lane

Re: run pgindent on a regular basis / scripted manner

[Tom Lane]

Peter Geoghegan  writes:
> That's beside the point. The point is that I'm obligated to keep
> typedef.list up to date in general, a task that is made significantly
> harder by random objdump implementation details. And I probably need
> to do this not just once per commit, but several times, since in
> practice I need to defensively run and rerun pgindent as the patch is
> tweaked.

Hmm, I've not found it that hard to manage the typedefs list.
If I run pgindent and it adds weird spacing around uses of a new
typedef name, I go "oh, I better add that to the list" and do so.
End of problem.  There's not a requirement that you remove disused
typedef names, nor that you alphabetize perfectly.  I'm content
to update those sorts of details from the buildfarm's list once a
year or so.

This does assume that you inspect pgindent's changes rather than
just accepting them blindly --- but as I commented upthread, the
tool really requires that anyway.

regards, tom lane

Re: run pgindent on a regular basis / scripted manner

[Peter Geoghegan]

On Tue, Oct 17, 2023 at 9:03 AM Jelte Fennema  wrote:
> To make koel pass you don't need to worry about hand-polishing
> typedefs.list. koel uses the typedefs.list that's committed into the
> repo, just like when you run pgindent yourself. If you forget to
> update the typedefs.list with new types, then worst case the pgindent
> output will look weird. But it will look weird both on your own
> machine and on koel.

That's beside the point. The point is that I'm obligated to keep
typedef.list up to date in general, a task that is made significantly
harder by random objdump implementation details. And I probably need
to do this not just once per commit, but several times, since in
practice I need to defensively run and rerun pgindent as the patch is
tweaked.

-- 
Peter Geoghegan

Re: run pgindent on a regular basis / scripted manner

[Jelte Fennema]

On Tue, 17 Oct 2023 at 17:47, Peter Geoghegan  wrote:
> 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.

To make koel pass you don't need to worry about hand-polishing
typedefs.list. koel uses the typedefs.list that's committed into the
repo, just like when you run pgindent yourself. If you forget to
update the typedefs.list with new types, then worst case the pgindent
output will look weird. But it will look weird both on your own
machine and on koel. So afaik the current tooling should give
perfectly consistent results on everybody's development machine. If
you have an example of where it doesn't then we should fix that
problem.

On Tue, 17 Oct 2023 at 17:47, Peter Geoghegan  wrote:
>
> On Tue, Oct 17, 2023 at 8:24 AM Robert Haas  wrote:
> > 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.
>
> 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

Re: run pgindent on a regular basis / scripted manner

[Peter Geoghegan]

On Tue, Oct 17, 2023 at 8:24 AM Robert Haas  wrote:
> 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.

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

Re: run pgindent on a regular basis / scripted manner

[Robert Haas]

On Tue, Oct 17, 2023 at 11:23 AM Jelte Fennema  wrote:
> To clarify, I did not intend to imply people that commit unindented
> code are lazy. It's expected that humans forget to run pgindent before
> committing from time to time (I do too). That's why I proposed a
> server side git hook to reject badly indented commits very early in
> this thread. But some others said that buildfarm animals were the way
> to go for Postgres development flow. And since I'm not a committer I
> left it at that. I was already happy enough that there was consensus
> on indenting continuously, so that the semi-regular rebases for the
> few open CF entries that I have are a lot less annoying.

Thanks for clarifying. I didn't really think you were trying to be
accusatory, but I didn't really understand what else to think either,
so this is helpful context.

> But based on the current feedback I think we should seriously consider
> a server-side "update" git hook again. People are obviously not
> perfect machines. And for whatever reason not everyone installs the
> pre-commit hook from the wiki. So the koel keeps complaining. A
> server-side hook would solve all of this IMHO.

One potential problem with a server-side hook is that if you back-port
a commit to older branches and then push the commits all together
(which is my workflow) then you might get failure to push on some
branches but not others. I don't know if there's any way to avoid
that, but it seems not great. You could think of enforcing the policy
only on master to try to avoid this, but that still leaves a risk that
you manage to push to all the back-branches and not to master.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

Re: run pgindent on a regular basis / scripted manner

[Robert Haas]

On Tue, Oct 17, 2023 at 11:16 AM Peter Geoghegan  wrote:
> 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.

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.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

Re: run pgindent on a regular basis / scripted manner

[Jelte Fennema]

On Tue, 17 Oct 2023 at 16:04, Robert Haas  wrote:
> 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.

To clarify, I did not intend to imply people that commit unindented
code are lazy. It's expected that humans forget to run pgindent before
committing from time to time (I do too). That's why I proposed a
server side git hook to reject badly indented commits very early in
this thread. But some others said that buildfarm animals were the way
to go for Postgres development flow. And since I'm not a committer I
left it at that. I was already happy enough that there was consensus
on indenting continuously, so that the semi-regular rebases for the
few open CF entries that I have are a lot less annoying.

But based on the current feedback I think we should seriously consider
a server-side "update" git hook again. People are obviously not
perfect machines. And for whatever reason not everyone installs the
pre-commit hook from the wiki. So the koel keeps complaining. A
server-side hook would solve all of this IMHO.

Re: run pgindent on a regular basis / scripted manner

[Peter Geoghegan]

On Tue, Oct 17, 2023 at 8:01 AM Robert Haas  wrote:
> 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!

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

Re: run pgindent on a regular basis / scripted manner

[Robert Haas]

On Tue, Oct 17, 2023 at 10:23 AM 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.

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.

> 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.

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'.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

Re: run pgindent on a regular basis / scripted manner

[Jelte Fennema]

On Tue, 17 Oct 2023 at 16:23, Tom Lane  wrote:
> > Or we could have a server-side hook that will refuse
> > the misindented commit, with some kind of override for emergency
> > 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.

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.

Re: run pgindent on a regular basis / scripted manner

[Peter Geoghegan]

On Tue, Oct 17, 2023 at 7:23 AM Tom Lane  wrote:
> Robert Haas  writes:
> > On Tue, Oct 17, 2023 at 8:45 AM Robert Haas  wrote:
> >> +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.
>
> 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.

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

Re: run pgindent on a regular basis / scripted manner

[Tom Lane]

Robert Haas  writes:
> On Tue, Oct 17, 2023 at 8:45 AM Robert Haas  wrote:
>> +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.

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
> 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.

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
> the misindented commit, with some kind of override for emergency
> 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

Re: run pgindent on a regular basis / scripted manner

[Robert Haas]

On Tue, Oct 17, 2023 at 8:45 AM Robert Haas  wrote:
> > This policy isn't working.
>
> +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.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

Re: run pgindent on a regular basis / scripted manner

[Robert Haas]

On Tue, Oct 17, 2023 at 6:34 AM Jelte Fennema  wrote:
> I think *it is* dead easy to comply. If you run the following commands
> before committing/after rebasing, then koel should always be happy:
>
> src/tools/pgindent/pgindent src # works always but a bit slow
> src/tools/pgindent/pgindent $(git diff --name-only --diff-filter=ACMR)
> # much faster, but only works if you DID NOT change typedefs.list

In isolation, that's true, but the list of mistakes that you can make
while committing which will inconvenience everyone working on the
project is very long. Another one that comes up frequently is
forgetting to bump CATALOG_VERSION_NO, but you also need a good commit
message, and good comments, and a good Discussion link in the commit
message, and the right list of authors and reviewers, and to update
the docs (with spaces, not tabs) and the Makefiles (with tabs, not
spaces) and the meson stuff and, as if that weren't enough already,
you actually need the code to work! And that includes not only working
regularly but also with CLOBBER_CACHE_ALWAYS and debug_parallel_query
and so on. It's very easy to miss something somewhere. I put a LOT of
work into polishing my commits before I push them, and it's still not
that uncommon that I screw something up.

--
Robert Haas
EDB: http://www.enterprisedb.com

Re: run pgindent on a regular basis / scripted manner

[Robert Haas]

On Sun, Oct 15, 2023 at 8:52 PM Peter Geoghegan  wrote:
> On Sat, Aug 12, 2023 at 5:53 PM Peter Geoghegan  wrote:
> > Maybe I'm wrong -- maybe the new policy is practicable. It might even
> > 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.

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

-- 
Robert Haas
EDB: http://www.enterprisedb.com

Re: run pgindent on a regular basis / scripted manner

[Jelte Fennema]

On Tue, 17 Oct 2023 at 04:57, Michael Paquier  wrote:
> 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.

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.

Re: run pgindent on a regular basis / scripted manner

[Jelte Fennema]

On Tue, 17 Oct 2023 at 03:23, Peter Geoghegan  wrote:
> 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.

I think *it is* dead easy to comply. If you run the following commands
before committing/after rebasing, then koel should always be happy:

src/tools/pgindent/pgindent src # works always but a bit slow
src/tools/pgindent/pgindent $(git diff --name-only --diff-filter=ACMR)
# much faster, but only works if you DID NOT change typedefs.list

If you have specific cases where it does not work. Then I think we
should talk about those/fix them. But looking at the last few commits
in .git-blame-ignore-revs I only see examples of people simply not
running pgindent before they commit.

I guess it's easy to forget, but that's why the wiki contains a
pre-commit hook[1] that you can use to remind yourself/run pgindent
automatically. The only annoying thing is that it does not trigger
when rebasing, but you can work around that by using rebase its -x
flag[2].

[1]: https://wiki.postgresql.org/wiki/Working_with_Git#Using_git_hooks
[2]: https://adamj.eu/tech/2022/11/07/pre-commit-run-hooks-rebase/

Re: run pgindent on a regular basis / scripted manner

[Michael Paquier]

On Mon, Oct 16, 2023 at 08:45:00PM -0400, Tom Lane wrote:
> 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.

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


signature.asc
Description: PGP signature

Re: run pgindent on a regular basis / scripted manner

[Peter Geoghegan]

On Mon, Oct 16, 2023 at 6:32 PM Tom Lane  wrote:
> But it's *not* a hard rule --- we explicitly rejected mechanisms
> that would make it so (such as a precommit hook).  I view "koel
> is unhappy" as something that you ought to clean up, but if you
> don't get to it for a day or three there's not much harm done.

It's hard to square that with what you said about needing greater peer
pressure on committers.

> Right now I think we just need to raise
> committers' awareness of this enough that they routinely run
> pgindent on the files they're touching.  In the problem cases
> so far, they very clearly didn't.  I don't see much point in
> worrying about second-order problems until that first-order
> problem is tamped down.

Realistically, if you're the committer that broke koel, you are at
least the subject of mild disapproval -- you have likely
inconvenienced others. I always try to avoid that -- it pretty much
rounds up to "hard rule" in my thinking. Babysitting koel really does
seem like it could cut into my dinner plans or what have you.

-- 
Peter Geoghegan

Re: run pgindent on a regular basis / scripted manner

[Tom Lane]

Peter Geoghegan  writes:
> 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.

But it's *not* a hard rule --- we explicitly rejected mechanisms
that would make it so (such as a precommit hook).  I view "koel
is unhappy" as something that you ought to clean up, but if you
don't get to it for a day or three there's not much harm done.

In theory koel might complain even if you'd locally gotten
clean results from pgindent (as a consequence of skew in the
typedef lists being used, for example).  We've not seen cases
of that so far though.  Right now I think we just need to raise
committers' awareness of this enough that they routinely run
pgindent on the files they're touching.  In the problem cases
so far, they very clearly didn't.  I don't see much point in
worrying about second-order problems until that first-order
problem is tamped down.

regards, tom lane

Re: run pgindent on a regular basis / scripted manner

[Peter Geoghegan]

On Mon, Oct 16, 2023 at 5:45 PM Tom Lane  wrote:
> 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.

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.

> 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].

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.

> 2b could be ameliorated by making the indent check be a separate
> 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

Re: run pgindent on a regular basis / scripted manner

[Tom Lane]

Peter Geoghegan  writes:
> 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.

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

Re: run pgindent on a regular basis / scripted manner

[Peter Geoghegan]

On Sat, Aug 12, 2023 at 5:53 PM Peter Geoghegan  wrote:
> Maybe I'm wrong -- maybe the new policy is practicable. It might even
> 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

Re: run pgindent on a regular basis / scripted manner

[Nathan Bossart]

On Wed, Aug 16, 2023 at 01:15:55PM -0700, Peter Geoghegan wrote:
> On Tue, Aug 15, 2023 at 1:31 PM Nathan Bossart  
> wrote:
>> Should we add those?  Patch attached.
> 
> I think that that makes sense.

Committed.

> 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.)

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

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

Re: run pgindent on a regular basis / scripted manner

[Peter Geoghegan]

On Tue, Aug 15, 2023 at 1:31 PM Nathan Bossart  wrote:
> 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

Re: run pgindent on a regular basis / scripted manner

[Nathan Bossart]

On Fri, Aug 11, 2023 at 01:59:40PM -0700, Peter Geoghegan wrote:
> 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.

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 
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

Re: run pgindent on a regular basis / scripted manner

[Andrew Dunstan]

On 2023-08-14 Mo 10:04, Peter Eisentraut wrote:

On 12.08.23 23:14, Andres Freund wrote:
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.


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




It's a bit more complicated :-)

You can see what the buildfarm does at 
 
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

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

Re: run pgindent on a regular basis / scripted manner

[Peter Eisentraut]

On 12.08.23 02:11, Tom Lane wrote:

Andres Freund  writes:

On 2023-08-11 18:30:02 -0400, Tom Lane wrote:

+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.


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?


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

Re: run pgindent on a regular basis / scripted manner

[Peter Eisentraut]

On 12.08.23 23:14, Andres Freund wrote:

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.


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

Re: run pgindent on a regular basis / scripted manner

[Michael Paquier]

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:
> 
> check_catalog_version () {

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


signature.asc
Description: PGP signature

Re: run pgindent on a regular basis / scripted manner

[Andrew Dunstan]

On 2023-08-12 Sa 20:53, Peter Geoghegan wrote:

On Sat, Aug 12, 2023 at 5:20 PM Tom Lane  wrote:

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.



I agree with this.



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.



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

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

Re: run pgindent on a regular basis / scripted manner

[Peter Geoghegan]

On Sat, Aug 12, 2023 at 5:20 PM Tom Lane  wrote:
> 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.

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

Re: run pgindent on a regular basis / scripted manner

[Tom Lane]

Peter Geoghegan  writes:
> 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?

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

Re: run pgindent on a regular basis / scripted manner

[Peter Geoghegan]

On Sat, Aug 12, 2023 at 3:47 PM Tom Lane  wrote:
> > 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.
>
> 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.

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

Re: run pgindent on a regular basis / scripted manner

[Tom Lane]

Andres Freund  writes:
> 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.

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

> 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.

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

Re: run pgindent on a regular basis / scripted manner

[Andres Freund]

Hi,

On 2023-08-12 17:03:37 -0400, Andrew Dunstan wrote:
> On 2023-08-11 Fr 19:02, Tom Lane wrote:
> > Peter Geoghegan  writes:
> > > 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.
> > Yeah, I've done the same and will have to stop.
> > 
> > > 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.
> > 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.
>
> 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.

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

Re: run pgindent on a regular basis / scripted manner

[Andrew Dunstan]

On 2023-08-11 Fr 19:02, Tom Lane wrote:

Peter Geoghegan  writes:

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.

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


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.

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.





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

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

Re: run pgindent on a regular basis / scripted manner

[Andrew Dunstan]

On 2023-08-11 Fr 19:17, Tom Lane wrote:

Peter Geoghegan  writes:

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.

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.



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

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

Re: run pgindent on a regular basis / scripted manner

[Andres Freund]

Hi,

On 2023-08-11 20:11:34 -0400, Tom Lane wrote:
> Andres Freund  writes:
> > On 2023-08-11 18:30:02 -0400, Tom Lane wrote:
> >> +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.
>
> Hmm.  I'm allergic to anything that significantly increases the cost
> 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

Re: run pgindent on a regular basis / scripted manner

[Michael Paquier]

On Fri, Aug 11, 2023 at 08:11:34PM -0400, Tom Lane wrote:
> 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?

It does not have to be part of check-world by default, as we could
make it optional with PG_TEST_EXTRA.  I bet that most committers set
this option for most of the test suites anyway, so the extra cost is
OK from here.  I don't find a single indent run to be that costly,
especially with parallelism:
$ time ./src/tools/pgindent/pgindent .
real 0m5.039s
user 0m3.403s
sys 0m1.540s
--
Michael


signature.asc
Description: PGP signature

Re: run pgindent on a regular basis / scripted manner

[Tom Lane]

Andres Freund  writes:
> On 2023-08-11 18:30:02 -0400, Tom Lane wrote:
>> +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.

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

Re: run pgindent on a regular basis / scripted manner

[Andres Freund]

Hi,

On 2023-08-11 18:30:02 -0400, Tom Lane wrote:
> Peter Geoghegan  writes:
> > On Fri, Aug 11, 2023 at 2:25 PM Andres Freund  wrote:
> >> We could a test that fails when there's some mis-indented code. That seems
> >> like it might catch things earlier?
> 
> +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.


> > 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?
> 
> 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.

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

Re: run pgindent on a regular basis / scripted manner

[Jelte Fennema]

On Fri, 11 Aug 2023 at 23:00, Peter Geoghegan  wrote:
> 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.

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

Re: run pgindent on a regular basis / scripted manner

[Tom Lane]

Peter Geoghegan  writes:
> 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.

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

Re: run pgindent on a regular basis / scripted manner

[Tom Lane]

Peter Geoghegan  writes:
> 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.

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

> 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.

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

Re: run pgindent on a regular basis / scripted manner

[Peter Geoghegan]

On Fri, Aug 11, 2023 at 3:30 PM Tom Lane  wrote:
> 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.

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.

> 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.

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

Re: run pgindent on a regular basis / scripted manner

[Tom Lane]

Peter Geoghegan  writes:
> On Fri, Aug 11, 2023 at 2:25 PM Andres Freund  wrote:
>> We could a test that fails when there's some mis-indented code. That seems
>> like it might catch things earlier?

+1 for including this in CI tests

> 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?

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

Re: run pgindent on a regular basis / scripted manner

[Peter Geoghegan]

On Fri, Aug 11, 2023 at 2:25 PM Andres Freund  wrote:
> > 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?
>
> We could a test that fails when there's some mis-indented code. That seems
> 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

Re: run pgindent on a regular basis / scripted manner

[Andres Freund]

Hi,

On 2023-08-11 13:59:40 -0700, Peter Geoghegan wrote:
> On Sat, Jun 17, 2023 at 7:08 AM Andrew Dunstan  wrote:
> > 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'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.


> 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?

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

Greetings,

Andres Freund

Re: run pgindent on a regular basis / scripted manner

[Peter Geoghegan]

On Sat, Jun 17, 2023 at 7:08 AM Andrew Dunstan  wrote:
> 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

Re: run pgindent on a regular basis / scripted manner

[Andrew Dunstan]

On 2023-06-20 Tu 09:08, Tom Lane wrote:

Andrew Dunstan  writes:

On 2023-06-19 Mo 17:07, Tom Lane wrote:

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:

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.

Ah.


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?

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?





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

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

Re: run pgindent on a regular basis / scripted manner

[Tom Lane]

Andrew Dunstan  writes:
> On 2023-06-19 Mo 17:07, Tom Lane wrote:
>> 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:

> 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.

Ah.

> 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?

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

Re: run pgindent on a regular basis / scripted manner

[Andrew Dunstan]

On 2023-06-19 Mo 17:07, Tom Lane wrote:

Andrew Dunstan  writes:

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=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

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

Re: run pgindent on a regular basis / scripted manner

[Michael Paquier]

On Sat, Jun 17, 2023 at 10:08:32AM -0400, Andrew Dunstan wrote:
> See 
> 
> I have set up a new buildfarm animal called koel which will run the module.

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


signature.asc
Description: PGP signature

Re: run pgindent on a regular basis / scripted manner

[Tom Lane]

Andrew Dunstan  writes:
> 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=2023-06-19%2019%3A49%3A03

regards, tom lane

Re: run pgindent on a regular basis / scripted manner

[Andrew Dunstan]

On 2023-06-15 Th 12:12, Andrew Dunstan wrote:



On 2023-06-15 Th 11:26, Jelte Fennema wrote:

On Sat, 22 Apr 2023 at 13:42, Andrew Dunstan  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 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.



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





See 




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


cheers


andrew


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

Re: run pgindent on a regular basis / scripted manner

[Andrew Dunstan]

On 2023-06-15 Th 11:26, Jelte Fennema wrote:

On Sat, 22 Apr 2023 at 13:42, Andrew Dunstan  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 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.



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

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

Re: run pgindent on a regular basis / scripted manner

[Jelte Fennema]

On Sat, 22 Apr 2023 at 13:42, Andrew Dunstan  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 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.

Re: run pgindent on a regular basis / scripted manner

[Andrew Dunstan]

On 2023-05-17 We 17:10, Tom Lane wrote:

I wrote:

Andrew Dunstan  writes:

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

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.





LGTM


cheers


andrew

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

Re: run pgindent on a regular basis / scripted manner

[Tom Lane]

I wrote:
> Andrew Dunstan  writes:
>> 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

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 
Date:   Wed May 17 16:43:38 2023 -0400

Make agreed updates in perltidy options.

Discussion: https://postgr.es/m/20230428092545.qfb3y5wcu4cm75ur@alvherre.pgsql

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"

Re: run pgindent on a regular basis / scripted manner

[Tom Lane]

Andrew Dunstan  writes:
> On 2023-04-28 Fr 14:08, Bruce Momjian wrote:
>> Can those comments be added by a preprocessor before calling perltidy,
>> 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.

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

regards, tom lane

Re: run pgindent on a regular basis / scripted manner

[Andrew Dunstan]

On 2023-04-28 Fr 14:08, Bruce Momjian wrote:

On Wed, Apr 26, 2023 at 03:44:47PM -0400, Andrew Dunstan wrote:

On 2023-04-26 We 09:27, Tom Lane wrote:
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.

Can those comments be added by a preprocessor before calling perltidy,
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

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

Re: run pgindent on a regular basis / scripted manner

[Andrew Dunstan]

On 2023-04-28 Fr 05:25, Alvaro Herrera wrote:

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.)



Thanks for looking.




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.



I'm good with all of these I think





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).



Yeah, me too.





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



wfm


cheers


andrew

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

Re: run pgindent on a regular basis / scripted manner

[Bruce Momjian]

On Wed, Apr 26, 2023 at 03:44:47PM -0400, Andrew Dunstan wrote:
> On 2023-04-26 We 09:27, Tom Lane wrote:
> 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.

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

-- 
  Bruce Momjian  https://momjian.us
  EDB  https://enterprisedb.com

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

Re: run pgindent on a regular basis / scripted manner

[Alvaro Herrera]

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)

Re: run pgindent on a regular basis / scripted manner

[Tom Lane]

Andrew Dunstan  writes:
> On 2023-04-26 We 09:27, Tom Lane wrote:
>> 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
>> example:

> 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

Re: run pgindent on a regular basis / scripted manner

[Andrew Dunstan]

On 2023-04-26 We 09:27, Tom Lane wrote:

Peter Eisentraut  writes:

On 24.04.23 16:14, Tom Lane wrote:

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?

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");

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
example:


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

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




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

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