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

From: Tom Lane <[email-masked]>
Date: Fri, 20 Jan 2023 12:09:05 -0500

Jelte Fennema <[email-masked]> writes:
> To me a master branch that pgindent never complains about sounds
> amazing! And I personally think rejection of unindented pushes and
> cfbot complaining about unindented patches would be a very good thing,
> because that seems to be the only solution that could achieve that.

The core problem here is that requiring that would translate to
requiring every code contributor to have a working copy of pg_bsd_indent.
Maybe that's not a huge lift, but it'd be YA obstacle to new contributors,
and we don't need any more of those.

Yeah, if we switched to some other tool maybe we could reduce the size
of that problem.  But as Bruce replied, we've not found another one that
(a) can be coaxed to make output comparable to what we're accustomed to
and (b) seems decently well maintained.

			regards, tom lane

---

From: Andres Freund <[email-masked]>
Date: Fri, 20 Jan 2023 09:58:06 -0800

Hi,

> requiring every code contributor to have a working copy of pg_bsd_indent.

Wouldn't just every committer suffice?

> and (b) seems decently well maintained.

One question around this is how much change we'd accept. clang-format for
example is well maintained and can get somewhat close to our style - but
there are things that can't easily be approximated.

Greetings,

Andres Freund

---

From: Tom Lane <[email-masked]>
Date: Fri, 20 Jan 2023 13:19:47 -0500

> Wouldn't just every committer suffice?

Not if we have cfbot complaining about it.

(Another problem here is that there's a sizable subset of committers
who clearly just don't care, and I'm not sure we can convince them to.)

> there are things that can't easily be approximated.

If somebody wants to invest the effort in seeing how close we can get
and what the remaining delta would look like, we could have a discussion
about whether it's an acceptable change.  I don't think anyone has
tried with clang-format.

			regards, tom lane

---

From: Andrew Dunstan <[email-masked]>
Date: Sat, 21 Jan 2023 08:26:05 -0500

> who clearly just don't care, and I'm not sure we can convince them to.)

I think we could do better with some automation tooling for committers
here. One low-risk and simple change would be to provide a
non-destructive mode for pgindent that would show you the changes if any
it would make. That could be worked into a git pre-commit hook that
committers could deploy. I can testify to the usefulness of such hooks -
I have one that while not perfect has saved me on at least two occasions
from forgetting to bump the catalog version.

I'll take a look at fleshing this out, for my own if no-one else's use.

cheers

andrew

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

---

From: Andrew Dunstan <[email-masked]>
Date: Sat, 21 Jan 2023 10:00:31 -0500

> I'll take a look at fleshing this out, for my own if no-one else's use.

Here's a quick patch for this. I have it in mind to use like this in a
pre-commit hook:

    # only do this on master
    test `git rev-parse --abbrev-ref HEAD` = "master" || exit 0

    src/tools/pgindent/pg_indent --silent `git diff --cached --name-only` || \

      { echo "Need a pgindent run" >&2 ; exit 1; }

The committer could then run

    src/tools/pgindent/pg_indent --show-diff `git diff --cached --name-only`

to see what changes it thinks are needed.

cheers

andrew

---

From: Andrew Dunstan <[email-masked]>
Date: Sat, 21 Jan 2023 10:02:15 -0500

> to see what changes it thinks are needed.
This time with patch.

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com
diff --git a/src/tools/pgindent/pgindent b/src/tools/pgindent/pgindent
index 741b0ccb58..2834f5c9fc 100755
--- a/src/tools/pgindent/pgindent
+++ b/src/tools/pgindent/pgindent
@@ -21,7 +21,8 @@ my $indent_opts =
 
 my $devnull = File::Spec->devnull;
 
-my ($typedefs_file, $typedef_str, $code_base, $excludes, $indent, $build);
+my ($typedefs_file, $typedef_str, $code_base, $excludes, $indent, $build,
+	$show_diff, $silent_diff);
 
 my %options = (
 	"typedefs=s"         => \$typedefs_file,
@@ -29,9 +30,14 @@ my %options = (
 	"code-base=s"        => \$code_base,
 	"excludes=s"         => \$excludes,
 	"indent=s"           => \$indent,
-	"build"              => \$build,);
+	"build"              => \$build,
+    "show-diff"          => \$show_diff,
+    "silent_diff"        => \$silent_diff,);
 GetOptions(%options) || die "bad command line argument\n";
 
+die "Cannot have both --silent-diff and --show-diff"
+  if $silent_diff && $show_diff;
+
 run_build($code_base) if ($build);
 
 # command line option wins, then first non-option arg,
@@ -283,30 +289,19 @@ sub run_indent
 
 }
 
-
-# for development diagnostics
-sub diff
+sub show_diff
 {
-	my $pre   = shift;
-	my $post  = shift;
-	my $flags = shift || "";
+	my $indented   = shift;
+	my $source_filename  = shift;
 
-	print STDERR "running diff\n";
-
-	my $pre_fh  = new File::Temp(TEMPLATE => "pgdiffbXXXXX");
 	my $post_fh = new File::Temp(TEMPLATE => "pgdiffaXXXXX");
 
-	print $pre_fh $pre;
-	print $post_fh $post;
+	print $post_fh $indented;
 
-	$pre_fh->close();
 	$post_fh->close();
 
-	system( "diff $flags "
-		  . $pre_fh->filename . " "
-		  . $post_fh->filename
-		  . " >&2");
-	return;
+	my $diff = `diff -u $source_filename $post_fh->filename  2>&1`);
+	return $diff;
 }

@@ -404,6 +399,8 @@ push(@files, @ARGV);
 
 foreach my $source_filename (@files)
 {
+	# ignore anything that's not a .c or .h file
+	next unless $source_filename =~/\.[ch]$/;
 
 	# Automatically ignore .c and .h files that correspond to a .y or .l
 	# file.  indent tends to get badly confused by Bison/flex output,
@@ -429,7 +426,24 @@ foreach my $source_filename (@files)
 
 	$source = post_indent($source, $source_filename);
 
-	write_source($source, $source_filename) if $source ne $orig_source;
+	if ($source ne $orig_source)
+	{
+		if ($silent_diff)
+		{
+			exit 2;
+		}
+		elsif ($show_diff)
+		{
+			print show_diff($source, $source_filename);
+		}
+		else
+		{
+			write_source($source, $source_filename);
+		}
+	}
+
 }
 
 build_clean($code_base) if $build;
+
+exit 0;