On 2023-01-21 Sa 11:10, Tom Lane wrote:
> Andrew Dunstan <and...@dunslane.net> writes:
>>>> 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.
> That sounds like a good idea from here. I do not think we want a
> mandatory commit filter, but if individual committers care to work
> this into their process in some optional way, great! I can think
> of ways I'd use it during patch development, too.
Yes, it's intended for use at committers' discretion. We have no way of
forcing use of a git hook on committers, although we could reject pushes
that offend against certain rules. For the reasons you give below that's
not a good idea. A pre-commit hook can be avoided by using `git commit
-n` and there's are similar option/hook for `git merge`.
>
> (One reason not to want a mandatory filter is that you might wish
> to apply pgindent as a separate commit, so that you can then
> put that commit into .git-blame-ignore-revs. This could be handy
> for example when a patch needs to change the nesting level of a lot
> of pre-existing code, without making changes in it otherwise.)
Agreed.
> Looks reasonable, but you should also update
> src/tools/pgindent/pgindent.man, which AFAICT is our only
> documentation for pgindent switches. (Is it time for a
> --help option in pgindent?)
>
>
Yes, see revised patch.
> ... btw, can we get away with making the diff run be "diff -upd"
> not just "diff -u"? I find diff output for C files noticeably
> more useful with those options, but I'm unsure about their
> portability.
I think they are available on Linux, MacOS and FBSD, and on Windows (if
anyone's actually using it for this) it's likely to be Gnu diff. So I
think that's probably enough coverage.
cheers
andrew
--
Andrew Dunstan
EDB: https://www.enterprisedb.com
diff --git a/src/tools/pgindent/pgindent b/src/tools/pgindent/pgindent
index 741b0ccb58..bad2a17582 100755
--- a/src/tools/pgindent/pgindent
+++ b/src/tools/pgindent/pgindent
@@ -21,16 +21,27 @@ 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, $help);
+
+$help = 0;
my %options = (
+ "help" => \$help,
"typedefs=s" => \$typedefs_file,
"list-of-typedefs=s" => \$typedef_str,
"code-base=s" => \$code_base,
"excludes=s" => \$excludes,
"indent=s" => \$indent,
- "build" => \$build,);
-GetOptions(%options) || die "bad command line argument\n";
+ "build" => \$build,
+ "show-diff" => \$show_diff,
+ "silent_diff" => \$silent_diff,);
+GetOptions(%options) || usage ("bad command line argument");
+
+usage() if $help;
+
+usage ("Cannot have both --silent-diff and --show-diff")
+ if $silent_diff && $show_diff;
run_build($code_base) if ($build);
@@ -230,7 +241,6 @@ sub pre_indent
sub post_indent
{
my $source = shift;
- my $source_filename = shift;
# Restore CATALOG lines
$source =~ s!^/\*(CATALOG\(.*)\*/$!$1!gm;
@@ -280,33 +290,21 @@ sub run_indent
close($src_out);
return $source;
-
}
-
-# 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 $post_fh = new File::Temp(TEMPLATE => "pgdiffXXXXX");
- my $pre_fh = new File::Temp(TEMPLATE => "pgdiffbXXXXX");
- my $post_fh = new File::Temp(TEMPLATE => "pgdiffaXXXXX");
+ print $post_fh $indented;
- print $pre_fh $pre;
- print $post_fh $post;
-
- $pre_fh->close();
$post_fh->close();
- system( "diff $flags "
- . $pre_fh->filename . " "
- . $post_fh->filename
- . " >&2");
- return;
+ my $diff = `diff -upd $source_filename $post_fh->filename 2>&1`;
+ return $diff;
}
@@ -377,6 +375,33 @@ sub build_clean
return;
}
+sub usage
+{
+ my $message = shift;
+ my $helptext = <<'EOF';
+pgindent [OPTION]... [FILE]...
+Options:
+ --help show this message and quit
+ --typedefs=FILE file containing a list of typedefs
+ --list-of-typedefs=STR string containing typedefs, space separated
+ --code-base=DIR path to the base of PostgreSQL source code
+ --excludes=PATH file containing list of filename patterns to ignore
+ --indent=PATH path to pg_bsd_indent program
+ --build build the pg_bsd_indent program
+ --show-diff show the changes that would be made
+ --silent_diff exit with status 2 if any changes would be made
+EOF
+ if ($help)
+ {
+ print $helptext;
+ exit 0;
+ }
+ else
+ {
+ print STDERR "$message\n", $helptext;
+ exit 1;
+ }
+}
# main
@@ -404,6 +429,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,
@@ -427,9 +454,26 @@ foreach my $source_filename (@files)
next;
}
- $source = post_indent($source, $source_filename);
+ $source = post_indent($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);
+ }
+ }
- write_source($source, $source_filename) if $source ne $orig_source;
}
build_clean($code_base) if $build;
+
+exit 0;
diff --git a/src/tools/pgindent/pgindent.man b/src/tools/pgindent/pgindent.man
index 1c5aedda35..06bd748be1 100644
--- a/src/tools/pgindent/pgindent.man
+++ b/src/tools/pgindent/pgindent.man
@@ -33,6 +33,13 @@ file can be specified using the --excludes command line option. If indenting
a PostgreSQL source tree, this option isn't necessary, as it will find the file
src/tools/pgindent/exclude_file_patterns.
+There are also two non-destructive modes of pgindent. If given the --show-diffs
+option pgindent will show the changes it would make, but doesn't actually make
+them. If given instead the --silent-diff option, pgindent will exit with a
+status of 2 if it finds any indent changes are required, but will not
+make the changes or give any other information. This mode is intended for
+possible use in a git pre-commit hook.
+
Any non-option arguments are taken as the names of files to be indented. In this
case only these files will be changed, and nothing else will be touched. If the
first non-option argument is not a .c or .h file, it is treated as the name
