Hi, On 2023-10-16 20:45:00 -0400, Tom Lane wrote: > Peter Geoghegan <p...@bowt.ie> 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 <and...@anarazel.de> Date: Sat, 27 May 2023 11:38:27 -0700 Subject: [PATCH v2] Add meson targets to run pgindent Author: Reviewed-By: Discussion: https://postgr.es/m/ Backpatch: --- meson.build | 36 ++++++++++++++++++++++++++++++++++++ src/tools/pgindent/pgindent | 12 ++++++++++-- 2 files changed, 46 insertions(+), 2 deletions(-) diff --git a/meson.build b/meson.build index 862c955453f..7d4d02e7624 100644 --- a/meson.build +++ b/meson.build @@ -3013,6 +3013,42 @@ run_target('install-test-files', +############################################################### +# Indentation and similar targets +############################################################### + +indent_base = [perl, files('src/tools/pgindent/pgindent'), + '--indent', pg_bsd_indent.full_path(), + '--sourcetree=@SOURCE_ROOT@'] + +# reindent the entire tree +run_target('indent-tree', + command: indent_base + ['.'], + depends: pg_bsd_indent +) + +# reindent the last commit +run_target('indent-head', + command: indent_base + ['--commit=HEAD'], + depends: pg_bsd_indent +) + +# silently check if any file is not corectly indented (fails the build if +# there are differences) +run_target('indent-check', + command: indent_base + ['--silent-diff', '.'], + depends: pg_bsd_indent +) + +# show a diff of all the unindented files (doesn't fail the build if there are +# differences) +run_target('indent-diff', + command: indent_base + ['--show-diff', '.'], + depends: pg_bsd_indent +) + + + ############################################################### # Test prep ############################################################### diff --git a/src/tools/pgindent/pgindent b/src/tools/pgindent/pgindent index bce63d95daf..08b0c95814f 100755 --- a/src/tools/pgindent/pgindent +++ b/src/tools/pgindent/pgindent @@ -23,7 +23,7 @@ my $devnull = File::Spec->devnull; my ($typedefs_file, $typedef_str, @excludes, $indent, $build, $show_diff, - $silent_diff, $help, @commits,); + $silent_diff, $sourcetree, $help, @commits,); $help = 0; @@ -35,7 +35,8 @@ my %options = ( "excludes=s" => \@excludes, "indent=s" => \$indent, "show-diff" => \$show_diff, - "silent-diff" => \$silent_diff,); + "silent-diff" => \$silent_diff, + "sourcetree=s" => \$sourcetree); GetOptions(%options) || usage("bad command line argument"); usage() if $help; @@ -53,6 +54,13 @@ $typedefs_file ||= $ENV{PGTYPEDEFS}; # get indent location for environment or default $indent ||= $ENV{PGINDENT} || $ENV{INDENT} || "pg_bsd_indent"; +# When invoked from the build directory, change into source tree, otherwise +# the heuristics in locate_sourcedir() don't work. +if (defined $sourcetree) +{ + chdir $sourcetree; +} + my $sourcedir = locate_sourcedir(); # if it's the base of a postgres tree, we will exclude the files -- 2.38.0
