public inbox for [email protected]help / color / mirror / Atom feed
Add "format" target to make and ninja to run pgindent and pgperltidy 15+ messages / 6 participants [nested] [flat]
* Add "format" target to make and ninja to run pgindent and pgperltidy @ 2025-12-31 11:35 Jelte Fennema-Nio <[email protected]> 0 siblings, 1 reply; 15+ messages in thread From: Jelte Fennema-Nio @ 2025-12-31 11:35 UTC (permalink / raw) To: PostgreSQL Hackers <[email protected]>; +Cc: Daniel Gustafsson <[email protected]> This tries to make running formatting a lot easier for committers, but primarily for new contributors. You can not format the files by simply running one of the folowing: make format ninja -C build format My primary goal is to introduce Python formatting too (using ruff), and integrate that in a similar manner. Right now we don't have many Python files, but hopefully the pytest patch gets merged soonish and then we'll get more and more Python files. So I'd like to get autoformatting out of the gate. But even without that, these rules should make it simpler to run the current formatters: Running pgindent is still not trivial for new users. You need to add our pg_bsd_indent to PATH. And if you use meson to build you have to manually specify src/ and contrib/ instead of ./ because otherwise pgindent will try to format files in the build directory. Running pgperltidy is even more complicated because you need to install a specific version of perltidy. Recently we started erroring when that version was not the one we expect. Attachments: [text/x-patch] v1-0001-Add-format-target-for-make-and-ninja.patch (4.1K, 2-v1-0001-Add-format-target-for-make-and-ninja.patch) download | inline diff: From aaca6b3d2a0454183599d8f5f0d342d339b8a1a0 Mon Sep 17 00:00:00 2001 From: Jelte Fennema-Nio <[email protected]> Date: Wed, 31 Dec 2025 10:36:30 +0100 Subject: [PATCH v1 1/3] Add "format" target for make and ninja Running pgindent manually requires putting pg_bsd_indent in your path. This allows formatting the sources without having to do that. In follow on commits this will introduce more tools to these targets, like perltidy. --- GNUmakefile.in | 6 +++++- doc/src/sgml/targets-meson.txt | 1 + meson.build | 10 ++++++++++ src/tools/pgindent/README | 18 ++++++++++++++---- 4 files changed, 30 insertions(+), 5 deletions(-) diff --git a/GNUmakefile.in b/GNUmakefile.in index cf6e759486e..65c2dffba4e 100644 --- a/GNUmakefile.in +++ b/GNUmakefile.in @@ -134,4 +134,8 @@ headerscheck: submake-generated-headers cpluspluscheck: submake-generated-headers $(top_srcdir)/src/tools/pginclude/headerscheck --cplusplus $(top_srcdir) $(abs_top_builddir) -.PHONY: dist distcheck docs install-docs world check-world install-world installcheck-world headerscheck cpluspluscheck +format: submake-libpgport + @$(MAKE) -C $(top_builddir)/src/tools/pg_bsd_indent + $(PERL) $(top_srcdir)/src/tools/pgindent/pgindent --indent=$(top_builddir)/src/tools/pg_bsd_indent/pg_bsd_indent --typedefs=$(top_srcdir)/src/tools/pgindent/typedefs.list --excludes=$(top_srcdir)/src/tools/pgindent/exclude_file_patterns $(top_srcdir)/src $(top_srcdir)/contrib + +.PHONY: dist distcheck docs install-docs world check-world install-world installcheck-world headerscheck cpluspluscheck format diff --git a/doc/src/sgml/targets-meson.txt b/doc/src/sgml/targets-meson.txt index d0021a5eb10..b9486b9fba0 100644 --- a/doc/src/sgml/targets-meson.txt +++ b/doc/src/sgml/targets-meson.txt @@ -14,6 +14,7 @@ Code Targets: pl Build procedural languages Developer Targets: + format Run pgindent on C source files reformat-dat-files Rewrite catalog data files into standard format expand-dat-files Expand all data files to include defaults update-unicode Update unicode data to new version diff --git a/meson.build b/meson.build index ec08cd49056..585925ecebc 100644 --- a/meson.build +++ b/meson.build @@ -3837,6 +3837,16 @@ run_target('help', ] ) +run_target('format', + command: [perl, files('src/tools/pgindent/pgindent'), + '--indent', pg_bsd_indent, + '--typedefs', files('src/tools/pgindent/typedefs.list'), + '--excludes', files('src/tools/pgindent/exclude_file_patterns'), + meson.project_source_root() / 'src', + meson.project_source_root() / 'contrib'], + depends: [pg_bsd_indent], +) + ############################################################### diff --git a/src/tools/pgindent/README b/src/tools/pgindent/README index b6cd4c6f6b7..17b65c29c7d 100644 --- a/src/tools/pgindent/README +++ b/src/tools/pgindent/README @@ -11,9 +11,11 @@ http://adpgtech.blogspot.com/2015/05/running-pgindent-on-non-core-code-or.html PREREQUISITES: -1) Install pg_bsd_indent in your PATH. Its source code is in the - sibling directory src/tools/pg_bsd_indent; see the directions - in that directory's README file. +1) Install pg_bsd_indent in your PATH. If you use the "ninja format" + or "make format" build targets, they will build pg_bsd_indent for you, + so this step can be skipped. Otherwise, its source code is in the + 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 20230309 (older and newer versions make different formatting choices, and we want consistency). @@ -31,7 +33,15 @@ DOING THE INDENT RUN BEFORE A NORMAL COMMIT: 1) Change directory to the top of the source tree. -2) Run pgindent on the C files: +2) Run pgindent on the C files. With Meson: + + ninja -C build format + + Or with make: + + make format + + Or directly: src/tools/pgindent/pgindent . base-commit: 915711c8a4e60f606a8417ad033cea5385364c07 -- 2.52.0 [text/x-patch] v1-0002-Allow-running-pgperltidy-from-any-directory.patch (1.2K, 3-v1-0002-Allow-running-pgperltidy-from-any-directory.patch) download | inline diff: From 9e67f9b81db96b140f250d84c2168ed311ab38b7 Mon Sep 17 00:00:00 2001 From: Jelte Fennema-Nio <[email protected]> Date: Wed, 31 Dec 2025 11:07:58 +0100 Subject: [PATCH v1 2/3] Allow running pgperltidy from any directory pgperltidy was assuming it was run from the root directory of the source tree. This makes it agnostic to that. --- src/tools/pgindent/pgperltidy | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/tools/pgindent/pgperltidy b/src/tools/pgindent/pgperltidy index 87838d6bde3..75c99a84709 100755 --- a/src/tools/pgindent/pgperltidy +++ b/src/tools/pgindent/pgperltidy @@ -4,6 +4,10 @@ set -e +# Determine the directory where this script is located +SCRIPT_DIR=$(cd "$(dirname "$0")" && pwd) +TOOLS_DIR=$(dirname "$SCRIPT_DIR") + # set this to override default perltidy program: PERLTIDY=${PERLTIDY:-perltidy} @@ -13,6 +17,6 @@ if ! $PERLTIDY -v | grep -q $PERLTIDY_VERSION; then exit 1 fi -. src/tools/perlcheck/find_perl_files +. "$TOOLS_DIR/perlcheck/find_perl_files" -find_perl_files "$@" | xargs $PERLTIDY --profile=src/tools/pgindent/perltidyrc +find_perl_files "$@" | xargs $PERLTIDY --profile="$SCRIPT_DIR/perltidyrc" -- 2.52.0 [text/x-patch] v1-0003-Add-pgpertidy-to-format-target.patch (18.7K, 4-v1-0003-Add-pgpertidy-to-format-target.patch) download | inline diff: From e46791c4daeb4e149c29ca68d3dd4f05a4160c13 Mon Sep 17 00:00:00 2001 From: Jelte Fennema-Nio <[email protected]> Date: Wed, 31 Dec 2025 11:07:26 +0100 Subject: [PATCH v1 3/3] Add pgpertidy to "format" target During "./configure" and "meson setup" perltidy will now be detected, and the version will be compared to the exact version that we require. If it's the correct version the format target will now also start indenting perl files using pgindent. This also introduces format-c and format-perl targets for a bit more tighter controls on what is getting formatted. Finally, pgperltidy will now automatically configure PERL5LIB to make it possible to use local-lib installations of pgperltidy. This makes it easier to have multiple pgperltidy versions installed next to eachother for different Postgres versions. --- GNUmakefile.in | 12 ++++- configure | 91 +++++++++++++++++++++++++++++++++ configure.ac | 36 +++++++++++++ doc/src/sgml/targets-meson.txt | 4 +- meson.build | 50 +++++++++++++++++- meson_options.txt | 3 ++ src/Makefile.global.in | 2 + src/makefiles/meson.build | 1 + src/tools/pgindent/README | 63 +++++++++++++++-------- src/tools/pgindent/meson_format | 34 ++++++++++++ src/tools/pgindent/pgperltidy | 14 ++++- 11 files changed, 284 insertions(+), 26 deletions(-) create mode 100755 src/tools/pgindent/meson_format diff --git a/GNUmakefile.in b/GNUmakefile.in index 65c2dffba4e..2442fa454d7 100644 --- a/GNUmakefile.in +++ b/GNUmakefile.in @@ -134,8 +134,16 @@ headerscheck: submake-generated-headers cpluspluscheck: submake-generated-headers $(top_srcdir)/src/tools/pginclude/headerscheck --cplusplus $(top_srcdir) $(abs_top_builddir) -format: submake-libpgport +format-c: submake-libpgport @$(MAKE) -C $(top_builddir)/src/tools/pg_bsd_indent $(PERL) $(top_srcdir)/src/tools/pgindent/pgindent --indent=$(top_builddir)/src/tools/pg_bsd_indent/pg_bsd_indent --typedefs=$(top_srcdir)/src/tools/pgindent/typedefs.list --excludes=$(top_srcdir)/src/tools/pgindent/exclude_file_patterns $(top_srcdir)/src $(top_srcdir)/contrib -.PHONY: dist distcheck docs install-docs world check-world install-world installcheck-world headerscheck cpluspluscheck format +format-perl: + PERLTIDY="$(PERLTIDY)" $(top_srcdir)/src/tools/pgindent/pgperltidy $(top_srcdir) + +format: format-c +ifneq ($(PERLTIDY),) +format: format-perl +endif + +.PHONY: dist distcheck docs install-docs world check-world install-world installcheck-world headerscheck cpluspluscheck format format-c format-perl diff --git a/configure b/configure index f203f9d528b..c0ec13aa3a9 100755 --- a/configure +++ b/configure @@ -630,6 +630,7 @@ vpath_build PG_SYSROOT PG_VERSION_NUM LDFLAGS_EX_BE +PERLTIDY PROVE DBTOEPUB FOP @@ -19197,6 +19198,96 @@ $as_echo "$modulestderr" >&6; } fi fi +# Look for perltidy (optional, for the format target) +# Must be version 20230309 for consistent formatting +PERLTIDY_VERSION_REQ=20230309 +if test -n "$PERLTIDY"; then + # User explicitly specified PERLTIDY + # For local-lib installations, set PERL5LIB to find modules + pgac_perltidy_perl5lib="" + case "$PERLTIDY" in + /*) + pgac_perltidy_base=`dirname "\`dirname \"$PERLTIDY\"\`"` + if test -d "$pgac_perltidy_base/lib/perl5"; then + pgac_perltidy_perl5lib="$pgac_perltidy_base/lib/perl5" + fi + ;; + esac + if test -n "$pgac_perltidy_perl5lib"; then + pgac_perltidy_version=`PERL5LIB="$pgac_perltidy_perl5lib" $PERLTIDY -v 2>/dev/null | sed -n 's/.*v\([0-9]*\).*/\1/p'` + else + pgac_perltidy_version=`$PERLTIDY -v 2>/dev/null | sed -n 's/.*v\([0-9]*\).*/\1/p'` + fi + if test "$pgac_perltidy_version" != "$PERLTIDY_VERSION_REQ"; then + as_fn_error $? "perltidy version $PERLTIDY_VERSION_REQ is required, but $PERLTIDY is version $pgac_perltidy_version" "$LINENO" 5 + fi +else + # Look for perltidy in PATH + if test -z "$PERLTIDY"; then + for ac_prog in perltidy +do + # Extract the first word of "$ac_prog", so it can be a program name with args. +set dummy $ac_prog; ac_word=$2 +{ $as_echo "$as_me:${as_lineno-$LINENO}: checking for $ac_word" >&5 +$as_echo_n "checking for $ac_word... " >&6; } +if ${ac_cv_path_PERLTIDY+:} false; then : + $as_echo_n "(cached) " >&6 +else + case $PERLTIDY in + [\\/]* | ?:[\\/]*) + ac_cv_path_PERLTIDY="$PERLTIDY" # Let the user override the test with a path. + ;; + *) + as_save_IFS=$IFS; IFS=$PATH_SEPARATOR +for as_dir in $PATH +do + IFS=$as_save_IFS + test -z "$as_dir" && as_dir=. + for ac_exec_ext in '' $ac_executable_extensions; do + if as_fn_executable_p "$as_dir/$ac_word$ac_exec_ext"; then + ac_cv_path_PERLTIDY="$as_dir/$ac_word$ac_exec_ext" + $as_echo "$as_me:${as_lineno-$LINENO}: found $as_dir/$ac_word$ac_exec_ext" >&5 + break 2 + fi +done + done +IFS=$as_save_IFS + + ;; +esac +fi +PERLTIDY=$ac_cv_path_PERLTIDY +if test -n "$PERLTIDY"; then + { $as_echo "$as_me:${as_lineno-$LINENO}: result: $PERLTIDY" >&5 +$as_echo "$PERLTIDY" >&6; } +else + { $as_echo "$as_me:${as_lineno-$LINENO}: result: no" >&5 +$as_echo "no" >&6; } +fi + + + test -n "$PERLTIDY" && break +done + +else + # Report the value of PERLTIDY in configure's output in all cases. + { $as_echo "$as_me:${as_lineno-$LINENO}: checking for PERLTIDY" >&5 +$as_echo_n "checking for PERLTIDY... " >&6; } + { $as_echo "$as_me:${as_lineno-$LINENO}: result: $PERLTIDY" >&5 +$as_echo "$PERLTIDY" >&6; } +fi + + if test -n "$PERLTIDY"; then + pgac_perltidy_version=`$PERLTIDY -v 2>/dev/null | sed -n 's/.*v\([0-9]*\).*/\1/p'` + if test "$pgac_perltidy_version" != "$PERLTIDY_VERSION_REQ"; then + { $as_echo "$as_me:${as_lineno-$LINENO}: WARNING: perltidy version $PERLTIDY_VERSION_REQ is required for format-perl target, but found version $pgac_perltidy_version; disabling" >&5 +$as_echo "$as_me: WARNING: perltidy version $PERLTIDY_VERSION_REQ is required for format-perl target, but found version $pgac_perltidy_version; disabling" >&2;} + PERLTIDY="" + fi + fi +fi + + # If compiler will take -Wl,--as-needed (or various platform-specific # spellings thereof) then add that to LDFLAGS. This is much easier than # trying to filter LIBS to the minimum for each executable. diff --git a/configure.ac b/configure.ac index ec38ddefa36..da5261a4485 100644 --- a/configure.ac +++ b/configure.ac @@ -2408,6 +2408,42 @@ if test "$enable_tap_tests" = yes; then fi fi +# Look for perltidy (optional, for the format target) +# Must be version 20230309 for consistent formatting +PERLTIDY_VERSION_REQ=20230309 +if test -n "$PERLTIDY"; then + # User explicitly specified PERLTIDY + # For local-lib installations, set PERL5LIB to find modules + pgac_perltidy_perl5lib="" + case "$PERLTIDY" in + /*) + pgac_perltidy_base=`dirname "\`dirname \"$PERLTIDY\"\`"` + if test -d "$pgac_perltidy_base/lib/perl5"; then + pgac_perltidy_perl5lib="$pgac_perltidy_base/lib/perl5" + fi + ;; + esac + if test -n "$pgac_perltidy_perl5lib"; then + pgac_perltidy_version=`PERL5LIB="$pgac_perltidy_perl5lib" $PERLTIDY -v 2>/dev/null | sed -n 's/.*v\([[0-9]]*\).*/\1/p'` + else + pgac_perltidy_version=`$PERLTIDY -v 2>/dev/null | sed -n 's/.*v\([[0-9]]*\).*/\1/p'` + fi + if test "$pgac_perltidy_version" != "$PERLTIDY_VERSION_REQ"; then + AC_MSG_ERROR([perltidy version $PERLTIDY_VERSION_REQ is required, but $PERLTIDY is version $pgac_perltidy_version]) + fi +else + # Look for perltidy in PATH + PGAC_PATH_PROGS(PERLTIDY, perltidy) + if test -n "$PERLTIDY"; then + pgac_perltidy_version=`$PERLTIDY -v 2>/dev/null | sed -n 's/.*v\([[0-9]]*\).*/\1/p'` + if test "$pgac_perltidy_version" != "$PERLTIDY_VERSION_REQ"; then + AC_MSG_WARN([perltidy version $PERLTIDY_VERSION_REQ is required for format-perl target, but found version $pgac_perltidy_version; disabling]) + PERLTIDY="" + fi + fi +fi +AC_SUBST(PERLTIDY) + # If compiler will take -Wl,--as-needed (or various platform-specific # spellings thereof) then add that to LDFLAGS. This is much easier than # trying to filter LIBS to the minimum for each executable. diff --git a/doc/src/sgml/targets-meson.txt b/doc/src/sgml/targets-meson.txt index b9486b9fba0..391c759fde7 100644 --- a/doc/src/sgml/targets-meson.txt +++ b/doc/src/sgml/targets-meson.txt @@ -14,7 +14,9 @@ Code Targets: pl Build procedural languages Developer Targets: - format Run pgindent on C source files + format Run pgindent and perltidy on source files + format-c Run pgindent on C source files + format-perl Run perltidy on Perl source files reformat-dat-files Rewrite catalog data files into standard format expand-dat-files Expand all data files to include defaults update-unicode Update unicode data to new version diff --git a/meson.build b/meson.build index 585925ecebc..ee7d899f0d6 100644 --- a/meson.build +++ b/meson.build @@ -374,6 +374,39 @@ if flex.found() flex_version = flex_version_c.stdout().split(' ')[1].split('\n')[0] endif flex_wrapper = files('src/tools/pgflex') + +# perltidy is optional, but must be the correct version if specified +perltidy_version_req = '20230309' +perltidy_opt = get_option('PERLTIDY') +perltidy_prog = find_program(perltidy_opt, native: true, required: false) +perltidy_env = {} +if perltidy_prog.found() + # For local-lib installations, we need to set PERL5LIB to find modules + perltidy_path = perltidy_prog.full_path() + if perltidy_path.startswith('/') + perltidy_base = fs.parent(fs.parent(perltidy_path)) + perltidy_lib = perltidy_base / 'lib' / 'perl5' + if fs.is_dir(perltidy_lib) + perltidy_env = {'PERL5LIB': perltidy_lib} + endif + endif + + perltidy_version_c = run_command(perltidy_prog, '-v', check: false, env: perltidy_env) + if perltidy_version_c.returncode() == 0 and perltidy_version_c.stdout().contains(perltidy_version_req) + perltidy = perltidy_prog + perltidy_env += {'PERLTIDY': perltidy.full_path()} + elif perltidy_opt != 'perltidy' + # User explicitly specified a perltidy, but it's the wrong version + error('perltidy version @0@ is required, but @1@ is a different version'.format( + perltidy_version_req, perltidy_prog.full_path())) + else + # Found perltidy but wrong version - just disable it + perltidy = disabler() + perltidy_env = {} + endif +else + perltidy = disabler() +endif flex_cmd = [python, flex_wrapper, '--builddir', '@BUILD_ROOT@', '--srcdir', '@SOURCE_ROOT@', @@ -3837,7 +3870,7 @@ run_target('help', ] ) -run_target('format', +run_target('format-c', command: [perl, files('src/tools/pgindent/pgindent'), '--indent', pg_bsd_indent, '--typedefs', files('src/tools/pgindent/typedefs.list'), @@ -3847,6 +3880,21 @@ run_target('format', depends: [pg_bsd_indent], ) +run_target('format-perl', + command: [files('src/tools/pgindent/pgperltidy'), + meson.project_source_root()], + env: perltidy_env, +) + +format_env = {'PGINDENT': pg_bsd_indent.full_path()} +format_env += perltidy_env + +run_target('format', + command: [files('src/tools/pgindent/meson_format'), meson.project_source_root()], + depends: [pg_bsd_indent], + env: format_env, +) + ############################################################### diff --git a/meson_options.txt b/meson_options.txt index 06bf5627d3c..22cd2305cba 100644 --- a/meson_options.txt +++ b/meson_options.txt @@ -192,6 +192,9 @@ option('OPENSSL', type: 'string', value: 'openssl', option('PERL', type: 'string', value: 'perl', description: 'Path to perl binary') +option('PERLTIDY', type: 'string', value: 'perltidy', + description: 'Path to perltidy binary') + option('PROVE', type: 'string', value: 'prove', description: 'Path to prove binary') diff --git a/src/Makefile.global.in b/src/Makefile.global.in index 371cd7eba2c..e1c13477bf1 100644 --- a/src/Makefile.global.in +++ b/src/Makefile.global.in @@ -452,6 +452,8 @@ checkprep: $(if $(EXTRA_INSTALL),for extra in $(EXTRA_INSTALL); do $(MAKE) -C '$(top_builddir)'/$$extra DESTDIR='$(abs_top_builddir)'/tmp_install install || exit; done) PROVE = @PROVE@ +PERLTIDY = @PERLTIDY@ + # There are common routines in src/test/perl, and some test suites have # extra perl modules in their own directory. PG_PROVE_FLAGS = -I $(top_srcdir)/src/test/perl/ -I $(srcdir) diff --git a/src/makefiles/meson.build b/src/makefiles/meson.build index c6edf14ec44..3380cdcd6cc 100644 --- a/src/makefiles/meson.build +++ b/src/makefiles/meson.build @@ -144,6 +144,7 @@ pgxs_bins = { 'LZ4': program_lz4, 'OPENSSL': openssl, 'PERL': perl, + 'PERLTIDY': perltidy, 'PROVE': prove, 'PYTHON': python, 'TAR': tar, diff --git a/src/tools/pgindent/README b/src/tools/pgindent/README index 17b65c29c7d..c531f284351 100644 --- a/src/tools/pgindent/README +++ b/src/tools/pgindent/README @@ -17,23 +17,40 @@ 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 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-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-20230309.tar.gz +2) Install perltidy version 20230309 (older and newer versions make + different formatting choices, and we want consistency). You can get + this specific version from CPAN: + + wget https://cpan.metacpan.org/authors/id/S/SH/SHANCOCK/Perl-Tidy-20230309.tar.gz + + To install system-wide, follow the usual process for a Perl module + ("man perlmodinstall" explains it), or use cpan/cpanm: + + cpanm Perl-Tidy-20230309.tar.gz + + Alternatively, you can install perltidy to a local directory using + cpanm's local-lib feature: + + cpanm -l $HOME/perltidy-20230309 Perl-Tidy-20230309.tar.gz + + Then configure with (for meson): + + meson setup build -DPERLTIDY=$HOME/perltidy-20230309/bin/perltidy + + Or for autoconf: + + ./configure PERLTIDY=$HOME/perltidy-20230309/bin/perltidy + + If perltidy is found in PATH but is the wrong version, it will be + ignored. If you explicitly specify a PERLTIDY path with the wrong + version, configuration will fail with an error. DOING THE INDENT RUN BEFORE A NORMAL COMMIT: 1) Change directory to the top of the source tree. -2) Run pgindent on the C files. With Meson: +2) Run the formatting tools. With Meson: ninja -C build format @@ -41,9 +58,17 @@ DOING THE INDENT RUN BEFORE A NORMAL COMMIT: make format - Or directly: + This formats both C files (with pgindent) and Perl files (with perltidy). + Perl formatting is only performed if perltidy version 20230309 was found + during "meson setup" or "./configure". To format only C or only Perl: + + ninja -C build format-c # or: make format-c + ninja -C build format-perl # or: make format-perl + + Or run the tools directly: src/tools/pgindent/pgindent . + src/tools/pgindent/pgperltidy . If any files generate errors, restore their original versions with "git checkout", and see below for cleanup ideas. @@ -84,26 +109,22 @@ AT LEAST ONCE PER RELEASE CYCLE: (See https://buildfarm.postgresql.org/cgi-bin/typedefs.pl?show_list for a full list of typedef files, if you want to indent some back branch.) -2) Run pgindent as above. - -3) Indent the Perl code using perltidy: +2) Run pgindent and perltidy as above (step 2 under "DOING THE INDENT RUN"). + If perltidy is not configured, run it directly: src/tools/pgindent/pgperltidy . - If you want to use some perltidy version that's not in your PATH, - first set the PERLTIDY environment variable to point to it. - -4) Reformat the bootstrap catalog data files: +3) Reformat the bootstrap catalog data files: ./configure # "make" will not work in an unconfigured tree cd src/include/catalog make reformat-dat-files cd ../../.. -5) When you're done, "git commit" everything including the typedefs.list file +4) When you're done, "git commit" everything including the typedefs.list file you used. -6) Add the newly created commit(s) to the .git-blame-ignore-revs file so +5) Add the newly created commit(s) to the .git-blame-ignore-revs file so that "git blame" ignores the commits (for anybody that has opted-in to using the ignore file). Follow the instructions that appear at the top of the .git-blame-ignore-revs file. diff --git a/src/tools/pgindent/meson_format b/src/tools/pgindent/meson_format new file mode 100755 index 00000000000..69e70bb4221 --- /dev/null +++ b/src/tools/pgindent/meson_format @@ -0,0 +1,34 @@ +#!/bin/sh + +# src/tools/pgindent/meson_format +# Runs both pgindent (C) and pgperltidy (Perl) formatting +# +# Environment variables: +# PGINDENT - path to pg_bsd_indent (required) +# PERLTIDY - path to perltidy (optional, skips Perl formatting if not set) +# +# Arguments: source directory to format + +set -e + +if [ $# -ne 1 ]; then + echo "Usage: $0 <source_directory>" >&2 + exit 1 +fi + +SRCDIR="$1" + +# Determine the directory where this script is located +SCRIPT_DIR=$(cd "$(dirname "$0")" && pwd) + +# Run pgindent for C files +"$SCRIPT_DIR/pgindent" \ + --indent="$PGINDENT" \ + --typedefs="$SCRIPT_DIR/typedefs.list" \ + --excludes="$SCRIPT_DIR/exclude_file_patterns" \ + "$SRCDIR/src" "$SRCDIR/contrib" + +# Run pgperltidy for Perl files only if PERLTIDY is set +if [ -n "$PERLTIDY" ]; then + "$SCRIPT_DIR/pgperltidy" "$SRCDIR" +fi diff --git a/src/tools/pgindent/pgperltidy b/src/tools/pgindent/pgperltidy index 75c99a84709..3931a79bda2 100755 --- a/src/tools/pgindent/pgperltidy +++ b/src/tools/pgindent/pgperltidy @@ -11,9 +11,21 @@ TOOLS_DIR=$(dirname "$SCRIPT_DIR") # set this to override default perltidy program: PERLTIDY=${PERLTIDY:-perltidy} +# If PERLTIDY is an absolute path, set PERL5LIB to find local-lib modules +case "$PERLTIDY" in + /*) + perltidy_dir=$(dirname "$PERLTIDY") + perltidy_base=$(dirname "$perltidy_dir") + if [ -d "$perltidy_base/lib/perl5" ]; then + PERL5LIB="$perltidy_base/lib/perl5${PERL5LIB:+:$PERL5LIB}" + export PERL5LIB + fi + ;; +esac + PERLTIDY_VERSION=20230309 if ! $PERLTIDY -v | grep -q $PERLTIDY_VERSION; then - echo "You do not appear to have $PERLTIDY version $PERLTIDY_VERSION installed on your system." >&2 + echo "You do not appear to have $PERLTIDY version $PERLTIDY_VERSION installed on your system. See src/tools/pgindent/README on how to install it." >&2 exit 1 fi -- 2.52.0 ^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: Add "format" target to make and ninja to run pgindent and pgperltidy @ 2025-12-31 13:54 Ashutosh Bapat <[email protected]> parent: Jelte Fennema-Nio <[email protected]> 0 siblings, 1 reply; 15+ messages in thread From: Ashutosh Bapat @ 2025-12-31 13:54 UTC (permalink / raw) To: Jelte Fennema-Nio <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]>; Daniel Gustafsson <[email protected]> On Wed, Dec 31, 2025 at 5:06 PM Jelte Fennema-Nio <[email protected]> wrote: > > This tries to make running formatting a lot easier for committers, but > primarily for new contributors. You can not format the files by simply > running one of the folowing: > > make format > ninja -C build format > I generally like the idea. Since perltidy is not enforced regularly (like pgindent), running it usually ends up modifying files which are not part of the patch. So I avoid it if not necessary. Do you propose to make it optional? -- Best Wishes, Ashutosh Bapat ^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: Add "format" target to make and ninja to run pgindent and pgperltidy @ 2025-12-31 15:26 Tom Lane <[email protected]> parent: Ashutosh Bapat <[email protected]> 0 siblings, 2 replies; 15+ messages in thread From: Tom Lane @ 2025-12-31 15:26 UTC (permalink / raw) To: Ashutosh Bapat <[email protected]>; +Cc: Jelte Fennema-Nio <[email protected]>; PostgreSQL Hackers <[email protected]>; Daniel Gustafsson <[email protected]> Ashutosh Bapat <[email protected]> writes: > On Wed, Dec 31, 2025 at 5:06 PM Jelte Fennema-Nio <[email protected]> wrote: >> This tries to make running formatting a lot easier for committers, but >> primarily for new contributors. > I generally like the idea. Since perltidy is not enforced regularly > (like pgindent), running it usually ends up modifying files which are > not part of the patch. So I avoid it if not necessary. Do you propose > to make it optional? The other obstacle is that not everybody will have the right version of perltidy installed, but using some other version will create a whole lot of extraneous noise. On the whole I'd recommend not trying to automate the perltidy step yet. Cost/benefit is just not very good. On the substance of the patch: I wonder whether we could make things more reliable by using git metadata to figure out which .h and .c files to point pgindent at. regards, tom lane ^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: Add "format" target to make and ninja to run pgindent and pgperltidy @ 2025-12-31 15:48 Andrew Dunstan <[email protected]> parent: Tom Lane <[email protected]> 1 sibling, 2 replies; 15+ messages in thread From: Andrew Dunstan @ 2025-12-31 15:48 UTC (permalink / raw) To: Tom Lane <[email protected]>; Ashutosh Bapat <[email protected]>; +Cc: Jelte Fennema-Nio <[email protected]>; PostgreSQL Hackers <[email protected]>; Daniel Gustafsson <[email protected]> On 2025-12-31 We 10:26 AM, Tom Lane wrote: > Ashutosh Bapat<[email protected]> writes: >> On Wed, Dec 31, 2025 at 5:06 PM Jelte Fennema-Nio<[email protected]> wrote: >>> This tries to make running formatting a lot easier for committers, but >>> primarily for new contributors. >> I generally like the idea. Since perltidy is not enforced regularly >> (like pgindent), running it usually ends up modifying files which are >> not part of the patch. So I avoid it if not necessary. Do you propose >> to make it optional? > The other obstacle is that not everybody will have the right version > of perltidy installed, but using some other version will create a > whole lot of extraneous noise. pgperltidy actually checks the version now. > > On the whole I'd recommend not trying to automate the perltidy > step yet. Cost/benefit is just not very good. I'd kinda like to unify these universes, though. We could import the pgperltidy logic into pgindent but disable it unless some flag were given and the correct version of perltidy were present. > > On the substance of the patch: I wonder whether we could make things > more reliable by using git metadata to figure out which .h and .c > files to point pgindent at. > > The git pre-commit hook I use operates with this set of files: files=$(git diff --cached --name-only --diff-filter=ACMR) pgindent just currently looks at that set of files and ignores everything that's not a .c or .h file. I guess what you're wanting is a test to see if the file is in git or a generated file? That doesn't really arise for me as I always do vpath builds, so generated files are always elsewhere. cheers andrew -- Andrew Dunstan EDB:https://www.enterprisedb.com ^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: Add "format" target to make and ninja to run pgindent and pgperltidy @ 2025-12-31 15:54 Tom Lane <[email protected]> parent: Andrew Dunstan <[email protected]> 1 sibling, 1 reply; 15+ messages in thread From: Tom Lane @ 2025-12-31 15:54 UTC (permalink / raw) To: Andrew Dunstan <[email protected]>; +Cc: Ashutosh Bapat <[email protected]>; Jelte Fennema-Nio <[email protected]>; PostgreSQL Hackers <[email protected]>; Daniel Gustafsson <[email protected]> Andrew Dunstan <[email protected]> writes: > On 2025-12-31 We 10:26 AM, Tom Lane wrote: >> On the substance of the patch: I wonder whether we could make things >> more reliable by using git metadata to figure out which .h and .c >> files to point pgindent at. > I guess what you're wanting is a test to see if the file is in git or a > generated file? That doesn't really arise for me as I always do vpath > builds, so generated files are always elsewhere. Right. But if we're trying to make this easy, we need to make the automation work for all three use-cases (in-tree makefiles, vpath makefiles, meson). I was just wondering if relying on git would simplify getting the same results in all three. regards, tom lane ^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: Add "format" target to make and ninja to run pgindent and pgperltidy @ 2025-12-31 16:13 Andrew Dunstan <[email protected]> parent: Tom Lane <[email protected]> 0 siblings, 0 replies; 15+ messages in thread From: Andrew Dunstan @ 2025-12-31 16:13 UTC (permalink / raw) To: Tom Lane <[email protected]>; +Cc: Ashutosh Bapat <[email protected]>; Jelte Fennema-Nio <[email protected]>; PostgreSQL Hackers <[email protected]>; Daniel Gustafsson <[email protected]> On 2025-12-31 We 10:54 AM, Tom Lane wrote: > Andrew Dunstan<[email protected]> writes: >> On 2025-12-31 We 10:26 AM, Tom Lane wrote: >>> On the substance of the patch: I wonder whether we could make things >>> more reliable by using git metadata to figure out which .h and .c >>> files to point pgindent at. >> I guess what you're wanting is a test to see if the file is in git or a >> generated file? That doesn't really arise for me as I always do vpath >> builds, so generated files are always elsewhere. > Right. But if we're trying to make this easy, we need to make > the automation work for all three use-cases (in-tree makefiles, > vpath makefiles, meson). I was just wondering if relying on > git would simplify getting the same results in all three. > > I think we could use git ls-files -t $file or similar. cheers andrew -- Andrew Dunstan EDB:https://www.enterprisedb.com ^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: Add "format" target to make and ninja to run pgindent and pgperltidy @ 2025-12-31 18:17 Jelte Fennema-Nio <[email protected]> parent: Tom Lane <[email protected]> 1 sibling, 1 reply; 15+ messages in thread From: Jelte Fennema-Nio @ 2025-12-31 18:17 UTC (permalink / raw) To: Tom Lane <[email protected]>; Ashutosh Bapat <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]>; Daniel Gustafsson <[email protected]> On Wed Dec 31, 2025 at 4:26 PM CET, Tom Lane wrote: > Ashutosh Bapat <[email protected]> writes: >> On Wed, Dec 31, 2025 at 5:06 PM Jelte Fennema-Nio <[email protected]> wrote: >>> This tries to make running formatting a lot easier for committers, but >>> primarily for new contributors. > >> I generally like the idea. Since perltidy is not enforced regularly >> (like pgindent), running it usually ends up modifying files which are >> not part of the patch. So I avoid it if not necessary. Do you propose >> to make it optional? > > The other obstacle is that not everybody will have the right version > of perltidy installed, but using some other version will create a > whole lot of extraneous noise. > > On the whole I'd recommend not trying to automate the perltidy > step yet. Cost/benefit is just not very good. I would like to get to a point where it is enforced for every commit pushed by committers, so the same as with pgindent. I agree that the cost/benefit is not there currently, but that's what this patchset is trying to address. The docs in the last patch try to explain clearly how to get and configure the correct version of perltidy. And once you've done that it's as easy as running a single make/ninja command to format all the files. Do you think that's not still not easy enough for committers? What would be needed instead? Automatically fetching and installing perltidy to the local build path when running make/ninja format? > On the substance of the patch: I wonder whether we could make things > more reliable by using git metadata to figure out which .h and .c > files to point pgindent at. I think that would definitely be helpful. The fact that pgindent runs on files in the meson build directory when passing "." would be solved by that. ^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: Add "format" target to make and ninja to run pgindent and pgperltidy @ 2025-12-31 18:37 Tom Lane <[email protected]> parent: Jelte Fennema-Nio <[email protected]> 0 siblings, 0 replies; 15+ messages in thread From: Tom Lane @ 2025-12-31 18:37 UTC (permalink / raw) To: Jelte Fennema-Nio <[email protected]>; +Cc: Ashutosh Bapat <[email protected]>; PostgreSQL Hackers <[email protected]>; Daniel Gustafsson <[email protected]> "Jelte Fennema-Nio" <[email protected]> writes: > On Wed Dec 31, 2025 at 4:26 PM CET, Tom Lane wrote: >> On the whole I'd recommend not trying to automate the perltidy >> step yet. Cost/benefit is just not very good. > I would like to get to a point where it is enforced for every commit > pushed by committers, so the same as with pgindent. As an affected committer, I want to push back against having such a requirement, because I don't think it is reasonable to require everybody to have precisely version XYZ of perltidy installed. If that's not the version provided by their platform-of-choice, it's an annoying hurdle. As a comparison point, we did not start requiring pgindent cleanliness until we imported bsdindent into our tree, so as not to have an external dependency for that. (But I can't see vendoring perltidy, even if there weren't license issues involved.) I recognize the analogy to requiring a specific version of autoconf, but the difference is that without autoconf you just plain can't work on the configure code. Here, the hurdle would be erected for no reason stronger than neatnik-ism, and IMO that's not a good enough reason to put yet another burden on committers. I'm even less pleased by the notion that we'd soon add still another such requirement for python. regards, tom lane ^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: Add "format" target to make and ninja to run pgindent and pgperltidy @ 2026-03-04 09:18 Jelte Fennema-Nio <[email protected]> parent: Andrew Dunstan <[email protected]> 1 sibling, 1 reply; 15+ messages in thread From: Jelte Fennema-Nio @ 2026-03-04 09:18 UTC (permalink / raw) To: Andrew Dunstan <[email protected]>; Tom Lane <[email protected]>; Ashutosh Bapat <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]>; Daniel Gustafsson <[email protected]> On Wed Dec 31, 2025 at 4:48 PM CET, Andrew Dunstan wrote: > I'd kinda like to unify these universes, though. We could import the > pgperltidy logic into pgindent but disable it unless some flag were > given and the correct version of perltidy were present. Attached is an initial patchset that does that, as well as improving pgindent in various other ways. This has removed the make/meson integration and instead made pgindent smarter at finding pg_bsd_indent and perltidy. I believe that with all of these QoL improvements we could enable perltidy in pgindent by default (after doing a full indent run). I want to call pretty much all of this code was written by Claude Code (i.e. an AI). I plan to do another review pass over this code soonish. Especially the later commits, which I haven't checked in detail yet. But for now I at least wanted to share the direction of what I've been working on, because I saw that Peter marked himself as reviewer on the commitfest app and I don't think it makes sense for him to look at the previous patchset. Attachments: [text/x-patch] v2-0001-pgindent-Clean-up-temp-files-on-SIGINT.patch (958B, 2-v2-0001-pgindent-Clean-up-temp-files-on-SIGINT.patch) download | inline diff: From efa77687594e7f52b194aeb81292b3ecb7a165dc Mon Sep 17 00:00:00 2001 From: Jelte Fennema-Nio <[email protected]> Date: Wed, 4 Mar 2026 10:02:39 +0100 Subject: [PATCH v2 1/7] pgindent: Clean up temp files on SIGINT When pressing Ctrl+C while running pgindent, it would often leave around files like pgtypedefAXUEEA. This slightly changes SIGINT handling so those files are cleaned up. --- src/tools/pgindent/pgindent | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/tools/pgindent/pgindent b/src/tools/pgindent/pgindent index 7481696a584..ccc5db4b205 100755 --- a/src/tools/pgindent/pgindent +++ b/src/tools/pgindent/pgindent @@ -19,6 +19,9 @@ use File::Temp; use IO::Handle; use Getopt::Long; +# Ensure SIGINT triggers a clean exit so File::Temp can remove temp files. +$SIG{INT} = sub { exit 130; }; + # Update for pg_bsd_indent version my $INDENT_VERSION = "2.1.2"; base-commit: 2a525cc97e19868940c533787165bc7e7de3a80a -- 2.53.0 [text/x-patch] v2-0002-pgindent-Try-to-find-pg_bsd_indent-binary-in-comm.patch (2.1K, 3-v2-0002-pgindent-Try-to-find-pg_bsd_indent-binary-in-comm.patch) download | inline diff: From 87e05a46fba366e03df6c8b1fe7bd2b0a1ef560a Mon Sep 17 00:00:00 2001 From: Jelte Fennema-Nio <[email protected]> Date: Tue, 3 Mar 2026 23:46:52 +0100 Subject: [PATCH v2 2/7] pgindent: Try to find pg_bsd_indent binary in common locations To run pgindent you need to to have the right version of pg_bsd_indent in your PATH, or specify it manually. This is a bit of a hassle, especially for newcomers or when working on backbranches. So this chnages pgindent to search for a pg_bsd_indent that's built from the current sources, both in-tree (for in-tree autoconf builds) and in a build directory (for meson or autoconf vpath builds). --- src/tools/pgindent/pgindent | 29 ++++++++++++++++++++++++++--- 1 file changed, 26 insertions(+), 3 deletions(-) diff --git a/src/tools/pgindent/pgindent b/src/tools/pgindent/pgindent index ccc5db4b205..6570726381c 100755 --- a/src/tools/pgindent/pgindent +++ b/src/tools/pgindent/pgindent @@ -56,11 +56,34 @@ usage("Cannot use --commit with command line file list") # dir, then default location $typedefs_file ||= $ENV{PGTYPEDEFS}; -# get indent location for environment or default -$indent ||= $ENV{PGINDENT} || $ENV{INDENT} || "pg_bsd_indent"; - my $sourcedir = locate_sourcedir(); +# get indent location: command line wins, then environment, then try to find +# a compiled pg_bsd_indent in the source tree, then fall back to PATH. +$indent ||= $ENV{PGINDENT} || $ENV{INDENT}; +if (!$indent && $sourcedir) +{ + my $srcroot = "$sourcedir/../../.."; + my $bsd_indent_subdir = "src/tools/pg_bsd_indent/pg_bsd_indent"; + + # Look for pg_bsd_indent: first in-tree (autoconf in-tree build), + # then in a "build" directory (meson or autoconf vpath), + # then any "build*" directory. + foreach my $candidate ( + "$srcroot/$bsd_indent_subdir", + glob("$srcroot/build/$bsd_indent_subdir"), + glob("$srcroot/build*/$bsd_indent_subdir")) + { + if (-x $candidate) + { + $indent = $candidate; + last; + } + } +} +$indent ||= "pg_bsd_indent"; + + # if it's the base of a postgres tree, we will exclude the files # postgres wants excluded if ($sourcedir) -- 2.53.0 [text/x-patch] v2-0003-pgindent-Integrate-pgperltidy-functionality-into-.patch (11.5K, 4-v2-0003-pgindent-Integrate-pgperltidy-functionality-into-.patch) download | inline diff: From 0c7e7a2215cb017ba59dc20258f9edfb19a14c1a Mon Sep 17 00:00:00 2001 From: Jelte Fennema-Nio <[email protected]> Date: Wed, 31 Dec 2025 11:07:58 +0100 Subject: [PATCH v2 3/7] pgindent: Integrate pgperltidy functionality into pgindent Over time our pgindent script has gotten a lot of quality of life features, like the --commit, --diff and --check flags. This integrates the functionality of pgperltidy into pgindent, so it can benefit from these same quality of life improvements, as well as future ones. This commit adds a --perltidy flag to pgindent, which when given will cause pgindent to also format Perl files in addition to the C files it would normally format. It also adds a --perl-only flag, to (as the name suggests) only format Perl files. --- src/tools/pgindent/README | 14 +-- src/tools/pgindent/pgindent | 185 ++++++++++++++++++++++++++++------ src/tools/pgindent/pgperltidy | 18 ---- 3 files changed, 161 insertions(+), 56 deletions(-) delete mode 100755 src/tools/pgindent/pgperltidy diff --git a/src/tools/pgindent/README b/src/tools/pgindent/README index b6cd4c6f6b7..2e31471d7e6 100644 --- a/src/tools/pgindent/README +++ b/src/tools/pgindent/README @@ -35,6 +35,10 @@ DOING THE INDENT RUN BEFORE A NORMAL COMMIT: src/tools/pgindent/pgindent . + To also format Perl files at the same time, add --perltidy: + + src/tools/pgindent/pgindent --perltidy=perltidy . + If any files generate errors, restore their original versions with "git checkout", and see below for cleanup ideas. @@ -76,12 +80,12 @@ AT LEAST ONCE PER RELEASE CYCLE: 2) Run pgindent as above. -3) Indent the Perl code using perltidy: +3) Indent the Perl code using perltidy (if not already done in step 2): - src/tools/pgindent/pgperltidy . + src/tools/pgindent/pgindent --perl-only . If you want to use some perltidy version that's not in your PATH, - first set the PERLTIDY environment variable to point to it. + use --perltidy=PATH or set the PERLTIDY environment variable. 4) Reformat the bootstrap catalog data files: @@ -166,6 +170,4 @@ Note that we do not exclude ecpg's header files from the run. Some of them get copied verbatim into ecpg's output, meaning that ecpg's expected files may need to be updated to match. -The perltidy run processes all *.pl and *.pm files, plus a few -executable Perl scripts that are not named that way. See the "find" -rules in pgperltidy for details. +When --perltidy is given, pgindent also processes *.pl and *.pm files. diff --git a/src/tools/pgindent/pgindent b/src/tools/pgindent/pgindent index 6570726381c..f8ac9c8268b 100755 --- a/src/tools/pgindent/pgindent +++ b/src/tools/pgindent/pgindent @@ -2,12 +2,12 @@ # Copyright (c) 2021-2026, PostgreSQL Global Development Group -# Program to maintain uniform layout style in our C code. +# Program to maintain uniform layout style in our C and Perl code. # Exit codes: # 0 -- all OK # 1 -- error invoking pgindent, nothing done # 2 -- --check mode and at least one file requires changes -# 3 -- pg_bsd_indent failed on at least one file +# 3 -- pg_bsd_indent or perltidy failed on at least one file use strict; use warnings FATAL => 'all'; @@ -32,10 +32,15 @@ my $indent_opts = my $devnull = File::Spec->devnull; my ($typedefs_file, $typedef_str, @excludes, $indent, $diff, - $check, $help, @commits,); + $check, $help, @commits, $perltidy_arg, $perl_only,); $help = 0; +# Save @ARGV before parsing so we can distinguish --perltidy=PATH (where +# the value should be used as the perltidy path) from --perltidy PATH +# (where PATH is a file to format that Getopt::Long greedily consumed). +my @orig_argv = @ARGV; + my %options = ( "help" => \$help, "commit=s" => \@commits, @@ -43,10 +48,21 @@ my %options = ( "list-of-typedefs=s" => \$typedef_str, "excludes=s" => \@excludes, "indent=s" => \$indent, + "perltidy:s" => \$perltidy_arg, + "perl-only" => \$perl_only, "diff" => \$diff, "check" => \$check,); GetOptions(%options) || usage("bad command line argument"); +if (defined($perltidy_arg) && $perltidy_arg ne '') +{ + unless (grep { $_ eq "--perltidy=$perltidy_arg" } @orig_argv) + { + unshift(@ARGV, $perltidy_arg); + $perltidy_arg = ''; + } +} + usage() if $help; usage("Cannot use --commit with command line file list") @@ -56,6 +72,12 @@ usage("Cannot use --commit with command line file list") # dir, then default location $typedefs_file ||= $ENV{PGTYPEDEFS}; +# get perltidy location: command line wins, then environment, then default. +# --perltidy (with or without a path) and --perl-only all imply perltidy is +# wanted, falling back to the PERLTIDY env var, then "perltidy" in PATH. +my $perltidy = $perltidy_arg || $ENV{PERLTIDY} + || (defined($perltidy_arg) || $perl_only ? "perltidy" : undef); + my $sourcedir = locate_sourcedir(); # get indent location: command line wins, then environment, then try to find @@ -142,6 +164,30 @@ sub check_indent return; } +my $PERLTIDY_VERSION = "20230309"; + +sub check_perltidy +{ + my $ver = `$perltidy -v 2>&1`; + if ($? != 0) + { + print STDERR + "You do not appear to have $perltidy installed on your system.\n" + . "See src/tools/pgindent/README for installation instructions.\n"; + exit 1; + } + + if ($ver !~ m/$PERLTIDY_VERSION/) + { + print STDERR + "You do not appear to have $perltidy version $PERLTIDY_VERSION installed on your system.\n" + . "See src/tools/pgindent/README for installation instructions.\n"; + exit 1; + } + + return; +} + sub locate_sourcedir { # try fairly hard to locate the sourcedir @@ -327,6 +373,43 @@ sub run_indent return $source; } +sub format_c +{ + my $source = shift; + my $source_filename = shift; + my $error_message = ''; + + my $formatted = pre_indent($source); + $formatted = run_indent($formatted, \$error_message); + if ($formatted eq "") + { + print STDERR "Failure in $source_filename: " . $error_message . "\n"; + return ("", 1); + } + return post_indent($formatted); +} + +sub format_perl +{ + my $source = shift; + my $source_filename = shift; + + my $tmp_fh = new File::Temp(TEMPLATE => "pgperltidyXXXXX"); + my $tmp_filename = $tmp_fh->filename; + print $tmp_fh $source; + $tmp_fh->close(); + + my $perltidy_profile = "$sourcedir/perltidyrc"; + my $err = + `$perltidy --profile=$perltidy_profile -b -bext='/' $tmp_filename 2>&1`; + if ($? != 0) + { + print STDERR "Failure in $source_filename: " . $err . "\n"; + return ("", 1); + } + return read_source($tmp_filename); +} + sub diff { my $indented = shift; @@ -356,6 +439,8 @@ Options: --list-of-typedefs=STR string containing typedefs, space separated --excludes=PATH file containing list of filename patterns to ignore --indent=PATH path to pg_bsd_indent program + --perltidy[=PATH] enable Perl formatting (optionally set perltidy path) + --perl-only format only Perl files, skip C formatting --diff show the changes that would be made --check exit with status 2 if any changes would be made The --excludes and --commit options can be given more than once. @@ -374,20 +459,64 @@ EOF # main -$filtered_typedefs_fh = load_typedefs(); +my $do_c = !$perl_only; +my $do_perl = defined($perltidy); + +if ($do_c) +{ + $filtered_typedefs_fh = load_typedefs(); + check_indent(); +} + +if ($do_perl) +{ + check_perltidy(); +} -check_indent(); +sub is_c_file +{ + my $filename = shift; + # It needs to have .c or .h extension + return 0 unless $filename =~ /\.[ch]$/; + # Automatically ignore .c and .h files that correspond to a .y or .l file. + # pg_bsd_indent tends to get badly confused by Bison/flex output, and + # there's no value in indenting derived files anyway. + my $otherfile = $filename; + $otherfile =~ s/\.[ch]$/.y/; + return 0 if $otherfile ne $filename && -f $otherfile; + $otherfile =~ s/\.y$/.l/; + return 0 if $otherfile ne $filename && -f $otherfile; + return 1; +} + +sub is_perl_file +{ + my $filename = shift; + # take all .pl and .pm files + return 1 if $filename =~ /\.p[lm]$/; + # take executable files that file(1) thinks are perl files + return 0 unless -x $filename; + my $file_out = `file "$filename"`; + return $file_out =~ /:.*perl[0-9]*\b/i; +} my $wanted = sub { my ($dev, $ino, $mode, $nlink, $uid, $gid); (($dev, $ino, $mode, $nlink, $uid, $gid) = lstat($_)) && -f _ - && /^.*\.[ch]\z/s - && push(@files, $File::Find::name); + || return; + if ($do_c && is_c_file($File::Find::name)) + { + push(@files, $File::Find::name); + } + elsif ($do_perl && is_perl_file($File::Find::name)) + { + push(@files, $File::Find::name); + } }; # any non-option arguments are files or directories to be processed -File::Find::find({ wanted => $wanted }, @ARGV) if @ARGV; +File::Find::find({ wanted => $wanted, no_chdir => 1 }, @ARGV) if @ARGV; # commit file locations are relative to the source root chdir "$sourcedir/../../.." if @commits && $sourcedir; @@ -399,7 +528,8 @@ foreach my $commit (@commits) my @affected = `git diff --diff-filter=ACMR --name-only $prev $commit`; die "git error" if $?; chomp(@affected); - push(@files, @affected); + push(@files, grep { is_c_file($_) } @affected) if $do_c; + push(@files, grep { is_perl_file($_) } @affected) if $do_perl; } warn "No files to process" unless @files; @@ -416,51 +546,42 @@ foreach my $source_filename (@files) next if $processed{$source_filename}; $processed{$source_filename} = 1; - # ignore anything that's not a .c or .h file - next unless $source_filename =~ /\.[ch]$/; - - # don't try to indent a file that doesn't exist + # don't try to format a file that doesn't exist unless (-f $source_filename) { warn "Could not find $source_filename"; next; } - # Automatically ignore .c and .h files that correspond to a .y or .l - # file. indent tends to get badly confused by Bison/flex output, - # and there's no value in indenting derived files anyway. - my $otherfile = $source_filename; - $otherfile =~ s/\.[ch]$/.y/; - next if $otherfile ne $source_filename && -f $otherfile; - $otherfile =~ s/\.y$/.l/; - next if $otherfile ne $source_filename && -f $otherfile; my $source = read_source($source_filename); - my $orig_source = $source; - my $error_message = ''; + my ($formatted, $failure); - $source = pre_indent($source); + if ($source_filename =~ /\.[ch]$/) + { + ($formatted, $failure) = format_c($source, $source_filename); + } + else + { + ($formatted, $failure) = format_perl($source, $source_filename); + } - $source = run_indent($source, \$error_message); - if ($source eq "") + if ($failure) { - print STDERR "Failure in $source_filename: " . $error_message . "\n"; $status = 3; next; } - $source = post_indent($source); - - if ($source ne $orig_source) + if ($formatted ne $source) { if (!$diff && !$check) { - write_source($source, $source_filename); + write_source($formatted, $source_filename); } else { if ($diff) { - print diff($source, $source_filename); + print diff($formatted, $source_filename); } if ($check) diff --git a/src/tools/pgindent/pgperltidy b/src/tools/pgindent/pgperltidy deleted file mode 100755 index 87838d6bde3..00000000000 --- a/src/tools/pgindent/pgperltidy +++ /dev/null @@ -1,18 +0,0 @@ -#!/bin/sh - -# src/tools/pgindent/pgperltidy - -set -e - -# set this to override default perltidy program: -PERLTIDY=${PERLTIDY:-perltidy} - -PERLTIDY_VERSION=20230309 -if ! $PERLTIDY -v | grep -q $PERLTIDY_VERSION; then - echo "You do not appear to have $PERLTIDY version $PERLTIDY_VERSION installed on your system." >&2 - exit 1 -fi - -. src/tools/perlcheck/find_perl_files - -find_perl_files "$@" | xargs $PERLTIDY --profile=src/tools/pgindent/perltidyrc -- 2.53.0 [text/x-patch] v2-0004-pgindent-Use-git-ls-files-to-discover-files.patch (2.1K, 5-v2-0004-pgindent-Use-git-ls-files-to-discover-files.patch) download | inline diff: From ba28acaa64c33d8a8afe12c91a76f5dd5b57483c Mon Sep 17 00:00:00 2001 From: Jelte Fennema-Nio <[email protected]> Date: Tue, 3 Mar 2026 09:43:33 +0100 Subject: [PATCH v2 4/7] pgindent: Use git ls-files to discover files --- src/tools/pgindent/pgindent | 56 +++++++++++++++++++++++++++++-------- 1 file changed, 44 insertions(+), 12 deletions(-) diff --git a/src/tools/pgindent/pgindent b/src/tools/pgindent/pgindent index f8ac9c8268b..983fe0b1d4f 100755 --- a/src/tools/pgindent/pgindent +++ b/src/tools/pgindent/pgindent @@ -13,7 +13,6 @@ use strict; use warnings FATAL => 'all'; use Cwd qw(abs_path getcwd); -use File::Find; use File::Spec; use File::Temp; use IO::Handle; @@ -500,23 +499,56 @@ sub is_perl_file return $file_out =~ /:.*perl[0-9]*\b/i; } -my $wanted = sub { - my ($dev, $ino, $mode, $nlink, $uid, $gid); - (($dev, $ino, $mode, $nlink, $uid, $gid) = lstat($_)) - && -f _ - || return; - if ($do_c && is_c_file($File::Find::name)) +sub discover_files +{ + my @paths = @_; + my @discovered; + + # Separate individual files from directories + my @dirs; + foreach my $path (@paths) { - push(@files, $File::Find::name); + if (-f $path) + { + push(@discovered, $path); + } + elsif (-d $path) + { + push(@dirs, $path); + } + else + { + warn "Could not find $path"; + } } - elsif ($do_perl && is_perl_file($File::Find::name)) + + # Use git ls-files for directories to avoid searching build trees etc. + if (@dirs) { - push(@files, $File::Find::name); + my @git_files = `git ls-files -- @dirs`; + die "git ls-files error" if $?; + chomp(@git_files); + push(@discovered, @git_files); } -}; + + return @discovered; +} # any non-option arguments are files or directories to be processed -File::Find::find({ wanted => $wanted, no_chdir => 1 }, @ARGV) if @ARGV; +if (@ARGV) +{ + foreach my $f (discover_files(@ARGV)) + { + if ($do_c && is_c_file($f)) + { + push(@files, $f); + } + elsif ($do_perl && is_perl_file($f)) + { + push(@files, $f); + } + } +} # commit file locations are relative to the source root chdir "$sourcedir/../../.." if @commits && $sourcedir; -- 2.53.0 [text/x-patch] v2-0005-pgindent-Default-to-indenting-the-current-directo.patch (1.1K, 6-v2-0005-pgindent-Default-to-indenting-the-current-directo.patch) download | inline diff: From d9e41c2931340a7632b35e0369b8aa61a7db8a7b Mon Sep 17 00:00:00 2001 From: Jelte Fennema-Nio <[email protected]> Date: Tue, 3 Mar 2026 20:56:15 +0100 Subject: [PATCH v2 5/7] pgindent: Default to indenting the current directory if no files are given Previously running pgindent without giving it any files would result in this output: src/tools/pgindent/pgindent No files to process at src/tools/pgindent/pgindent line 526. This instead indents the current directory, which is probably what the user intended. --- src/tools/pgindent/pgindent | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/tools/pgindent/pgindent b/src/tools/pgindent/pgindent index 983fe0b1d4f..f0a87c68d97 100755 --- a/src/tools/pgindent/pgindent +++ b/src/tools/pgindent/pgindent @@ -67,6 +67,9 @@ usage() if $help; usage("Cannot use --commit with command line file list") if (@commits && @ARGV); +# default to current directory if no files/dirs given +@ARGV = ('.') unless @ARGV || @commits; + # command line option wins, then environment, then locations based on current # dir, then default location $typedefs_file ||= $ENV{PGTYPEDEFS}; -- 2.53.0 [text/x-patch] v2-0006-pgindent-Add-easy-way-of-getting-perltidy.patch (5.9K, 7-v2-0006-pgindent-Add-easy-way-of-getting-perltidy.patch) download | inline diff: From deaad631d0b99970a20743571403eafd0fd8d15d Mon Sep 17 00:00:00 2001 From: Jelte Fennema-Nio <[email protected]> Date: Wed, 4 Mar 2026 09:04:15 +0100 Subject: [PATCH v2 6/7] pgindent: Add easy way of getting perltidy We need a very specific perltidy version, but getting that installed is quite a hassle. Especially for people who don't use perl on a daily basis. This adds a small script to download the exact perltidy version that we need. --- src/tools/pgindent/.gitignore | 1 + src/tools/pgindent/README | 16 ++++----- src/tools/pgindent/get_perltidy | 62 +++++++++++++++++++++++++++++++++ src/tools/pgindent/pgindent | 30 +++++++++++++--- 4 files changed, 96 insertions(+), 13 deletions(-) create mode 100644 src/tools/pgindent/.gitignore create mode 100755 src/tools/pgindent/get_perltidy diff --git a/src/tools/pgindent/.gitignore b/src/tools/pgindent/.gitignore new file mode 100644 index 00000000000..1497217113b --- /dev/null +++ b/src/tools/pgindent/.gitignore @@ -0,0 +1 @@ +/perltidy/ diff --git a/src/tools/pgindent/README b/src/tools/pgindent/README index 2e31471d7e6..264038e7e09 100644 --- a/src/tools/pgindent/README +++ b/src/tools/pgindent/README @@ -17,14 +17,14 @@ PREREQUISITES: 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-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-20230309.tar.gz + + The easiest way is to use the get_perltidy script, which downloads, + verifies, and installs the correct version into the source tree: + + src/tools/pgindent/get_perltidy + + This installs perltidy into src/tools/pgindent/perltidy/, which + pgindent will find automatically. DOING THE INDENT RUN BEFORE A NORMAL COMMIT: diff --git a/src/tools/pgindent/get_perltidy b/src/tools/pgindent/get_perltidy new file mode 100755 index 00000000000..09ce4a195cb --- /dev/null +++ b/src/tools/pgindent/get_perltidy @@ -0,0 +1,62 @@ +#!/bin/sh + +# src/tools/pgindent/get_perltidy +# +# Downloads and installs perltidy 20230309 into src/tools/pgindent/perltidy/. + +set -e + +PERLTIDY_VERSION=20230309 +PERLTIDY_TARBALL="Perl-Tidy-${PERLTIDY_VERSION}.tar.gz" +PERLTIDY_URL="https://cpan.metacpan.org/authors/id/S/SH/SHANCOCK/${PERLTIDY_TARBALL}" +PERLTIDY_SHA256="e22949a208c618d671a18c5829b451abbe9da0da2cddd78fdbfcb036c7361c18" + +# Determine the directory this script lives in, i.e. src/tools/pgindent +SCRIPT_DIR=$(cd "$(dirname "$0")" && pwd) + +PERLTIDY_DIR="$SCRIPT_DIR/perltidy" +PERLTIDY_BIN="$PERLTIDY_DIR/bin/perltidy" + +# Check if already installed with the correct version +if [ -x "$PERLTIDY_BIN" ]; then + perltidy_lib="$PERLTIDY_DIR/lib/perl5" + installed_version=$(PERL5LIB="$perltidy_lib${PERL5LIB:+:$PERL5LIB}" "$PERLTIDY_BIN" -v 2>/dev/null || true) + if echo "$installed_version" | grep -q "$PERLTIDY_VERSION"; then + echo "perltidy $PERLTIDY_VERSION is already installed in $PERLTIDY_DIR" + exit 0 + fi +fi + +WORK_DIR=$(mktemp -d) +trap 'rm -rf "$WORK_DIR"' EXIT + +# Download +TARBALL="$WORK_DIR/$PERLTIDY_TARBALL" +if command -v curl >/dev/null 2>&1; then + curl -sSL -o "$TARBALL" "$PERLTIDY_URL" +elif command -v wget >/dev/null 2>&1; then + wget -q -O "$TARBALL" "$PERLTIDY_URL" +else + echo "error: neither curl nor wget found" >&2 + exit 1 +fi + +# Verify SHA256 +if command -v sha256sum >/dev/null 2>&1; then + echo "$PERLTIDY_SHA256 $TARBALL" | sha256sum -c - >/dev/null +elif command -v shasum >/dev/null 2>&1; then + echo "$PERLTIDY_SHA256 $TARBALL" | shasum -a 256 -c - >/dev/null +else + echo "error: neither sha256sum nor shasum found" >&2 + exit 1 +fi + +# Extract, build, install +cd "$WORK_DIR" +tar xzf "$TARBALL" +cd "Perl-Tidy-${PERLTIDY_VERSION}" +perl Makefile.PL INSTALL_BASE="$PERLTIDY_DIR" >/dev/null +make >/dev/null +make install >/dev/null + +echo "perltidy $PERLTIDY_VERSION installed in $PERLTIDY_DIR" diff --git a/src/tools/pgindent/pgindent b/src/tools/pgindent/pgindent index f0a87c68d97..d906c00059f 100755 --- a/src/tools/pgindent/pgindent +++ b/src/tools/pgindent/pgindent @@ -74,14 +74,34 @@ usage("Cannot use --commit with command line file list") # dir, then default location $typedefs_file ||= $ENV{PGTYPEDEFS}; -# get perltidy location: command line wins, then environment, then default. -# --perltidy (with or without a path) and --perl-only all imply perltidy is -# wanted, falling back to the PERLTIDY env var, then "perltidy" in PATH. -my $perltidy = $perltidy_arg || $ENV{PERLTIDY} - || (defined($perltidy_arg) || $perl_only ? "perltidy" : undef); +# get perltidy location: command line wins, then environment, then try to +# find a get_perltidy-installed copy in the source tree, then fall back to +# PATH. --perltidy (with or without a path) and --perl-only all imply +# perltidy is wanted. +my $perltidy = $perltidy_arg || $ENV{PERLTIDY}; my $sourcedir = locate_sourcedir(); +if (!$perltidy && (defined($perltidy_arg) || $perl_only) && $sourcedir) +{ + # Look for a get_perltidy-installed perltidy in the source tree. + my $candidate = "$sourcedir/perltidy/bin/perltidy"; + if (-x $candidate) + { + $perltidy = $candidate; + + # Local installs need PERL5LIB set so perltidy can find its + # modules. + my $libdir = "$sourcedir/perltidy/lib/perl5"; + if (-d $libdir) + { + $ENV{PERL5LIB} = + $ENV{PERL5LIB} ? "$libdir:$ENV{PERL5LIB}" : $libdir; + } + } +} +$perltidy ||= (defined($perltidy_arg) || $perl_only ? "perltidy" : undef); + # get indent location: command line wins, then environment, then try to find # a compiled pg_bsd_indent in the source tree, then fall back to PATH. $indent ||= $ENV{PGINDENT} || $ENV{INDENT}; -- 2.53.0 [text/x-patch] v2-0007-pgindent-Allow-parallel-pgindent-runs.patch (5.1K, 8-v2-0007-pgindent-Allow-parallel-pgindent-runs.patch) download | inline diff: From 73d5a2647e19b451ad25f8eaa3da5c04b30b821d Mon Sep 17 00:00:00 2001 From: Jelte Fennema-Nio <[email protected]> Date: Wed, 4 Mar 2026 09:24:24 +0100 Subject: [PATCH v2 7/7] pgindent: Allow parallel pgindent runs Running pgindent on the whole source tree can take a while, especially when also having it run perltidy on perl files. This adds support for pgindent to indent files in parallel. This speeds up a full pgindent run from more than a minute on my machine to ~7 seconds. --- src/tools/pgindent/pgindent | 112 ++++++++++++++++++++++++++++++------ 1 file changed, 93 insertions(+), 19 deletions(-) diff --git a/src/tools/pgindent/pgindent b/src/tools/pgindent/pgindent index d906c00059f..d286bba3750 100755 --- a/src/tools/pgindent/pgindent +++ b/src/tools/pgindent/pgindent @@ -17,6 +17,8 @@ use File::Spec; use File::Temp; use IO::Handle; use Getopt::Long; +use Fcntl qw(:flock); +use POSIX qw(:sys_wait_h); # Ensure SIGINT triggers a clean exit so File::Temp can remove temp files. $SIG{INT} = sub { exit 130; }; @@ -30,10 +32,12 @@ my $indent_opts = my $devnull = File::Spec->devnull; -my ($typedefs_file, $typedef_str, @excludes, $indent, $diff, - $check, $help, @commits, $perltidy_arg, $perl_only,); +my ($typedefs_file, $typedef_str, @excludes, $indent, + $diff, $check, $help, @commits, + $perltidy_arg, $perl_only, $jobs,); $help = 0; +$jobs = 0; # Save @ARGV before parsing so we can distinguish --perltidy=PATH (where # the value should be used as the perltidy path) from --perltidy PATH @@ -50,7 +54,8 @@ my %options = ( "perltidy:s" => \$perltidy_arg, "perl-only" => \$perl_only, "diff" => \$diff, - "check" => \$check,); + "check" => \$check, + "jobs|j=i" => \$jobs,); GetOptions(%options) || usage("bad command line argument"); if (defined($perltidy_arg) && $perltidy_arg ne '') @@ -62,6 +67,27 @@ if (defined($perltidy_arg) && $perltidy_arg ne '') } } +sub get_num_cpus +{ + # Try nproc (Linux, some BSDs), then sysctl (macOS, FreeBSD). + for my $cmd ('nproc', 'sysctl -n hw.ncpu') + { + my $n = `$cmd 2>$devnull`; + chomp $n; + return $n + 0 if ($? == 0 && $n =~ /^\d+$/ && $n > 0); + } + return 1; +} + +if ($jobs == 0) +{ + $jobs = get_num_cpus(); +} +elsif ($jobs < 0) +{ + usage("--jobs must be a non-negative number"); +} + usage() if $help; usage("Cannot use --commit with command line file list") @@ -465,6 +491,7 @@ Options: --perl-only format only Perl files, skip C formatting --diff show the changes that would be made --check exit with status 2 if any changes would be made + --jobs=N, -j N number of parallel workers (0 = num CPUs, default 0) The --excludes and --commit options can be given more than once. EOF if ($help) @@ -592,20 +619,18 @@ warn "No files to process" unless @files; # remove excluded files from the file list process_exclude(); -my %processed; -my $status = 0; +# Used by forked children to serialize diff output to STDOUT via flock(). +my $stdout_lock_fh = new File::Temp(TEMPLATE => "pglockXXXXX"); -foreach my $source_filename (@files) +sub process_file { - # skip duplicates - next if $processed{$source_filename}; - $processed{$source_filename} = 1; + my $source_filename = shift; # don't try to format a file that doesn't exist unless (-f $source_filename) { warn "Could not find $source_filename"; - next; + return 0; } my $source = read_source($source_filename); @@ -620,11 +645,7 @@ foreach my $source_filename (@files) ($formatted, $failure) = format_perl($source, $source_filename); } - if ($failure) - { - $status = 3; - next; - } + return 3 if $failure; if ($formatted ne $source) { @@ -636,14 +657,67 @@ foreach my $source_filename (@files) { if ($diff) { - print diff($formatted, $source_filename); + my $output = diff($formatted, $source_filename); + flock($stdout_lock_fh, LOCK_EX); + print $output; + STDOUT->flush(); + flock($stdout_lock_fh, LOCK_UN); } - if ($check) + return 2 if $check; + } + } + + return 0; +} + +# deduplicate file list +my %seen; +@files = grep { !$seen{$_}++ } @files; + +my $status = 0; + +if ($jobs <= 1) +{ + foreach my $source_filename (@files) + { + my $file_status = process_file($source_filename); + $status = $file_status if $file_status > $status; + last if $check && $status >= 2 && !$diff; + } +} +else +{ + my %children; # pid => 1 + + my $file_idx = 0; + while ($file_idx < scalar(@files) || %children) + { + # Fork new children up to $jobs limit + while ($file_idx < scalar(@files) && scalar(keys %children) < $jobs) + { + my $source_filename = $files[ $file_idx++ ]; + + my $pid = fork(); + die "fork failed: $!\n" unless defined $pid; + + if ($pid == 0) { - $status ||= 2; - last unless $diff; + # child + my $child_status = process_file($source_filename); + exit $child_status; } + + $children{$pid} = 1; + } + + # Wait for at least one child to finish + my $pid = waitpid(-1, 0); + if ($pid > 0 && exists $children{$pid}) + { + delete $children{$pid}; + my $child_status = $? >> 8; + $status = $child_status if $child_status > $status; } } } -- 2.53.0 ^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: Add "format" target to make and ninja to run pgindent and pgperltidy @ 2026-03-12 09:10 Peter Eisentraut <[email protected]> parent: Jelte Fennema-Nio <[email protected]> 0 siblings, 1 reply; 15+ messages in thread From: Peter Eisentraut @ 2026-03-12 09:10 UTC (permalink / raw) To: Jelte Fennema-Nio <[email protected]>; Andrew Dunstan <[email protected]>; Tom Lane <[email protected]>; Ashutosh Bapat <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]>; Daniel Gustafsson <[email protected]> On 04.03.26 10:18, Jelte Fennema-Nio wrote: > On Wed Dec 31, 2025 at 4:48 PM CET, Andrew Dunstan wrote: >> I'd kinda like to unify these universes, though. We could import the >> pgperltidy logic into pgindent but disable it unless some flag were >> given and the correct version of perltidy were present. > > Attached is an initial patchset that does that, as well as improving > pgindent in various other ways. This has removed the make/meson > integration and instead made pgindent smarter at finding pg_bsd_indent > and perltidy. I believe that with all of these QoL improvements we could > enable perltidy in pgindent by default (after doing a full indent run). > > I want to call pretty much all of this code was written by Claude Code > (i.e. an AI). I plan to do another review pass over this code soonish. > Especially the later commits, which I haven't checked in detail yet. > But for now I at least wanted to share the direction of what I've been > working on, because I saw that Peter marked himself as reviewer on the > commitfest app and I don't think it makes sense for him to look at the > previous patchset. Apparently, this is still work in progress, but here are some comments from me. I'm very interested in the patch "pgindent: Clean up temp files on SIGINT", because this is an annoying problem. But I didn't find any documentation about why this is the correct solution. I didn't find anything on the File::Temp man page, for example. Do you have more details? The patch "pgindent: Use git ls-files to discover files" is also interested and pretty straightforward. I guess we don't really care about being able to run this in non-git trees? The other stuff I don't think I'm really on board with. In particular, I don't like integrating pgperltidy into pgindent. These things are in practice run at different times and the underlying tools update differently and require different management, so integrating them all might lead to various annoyances. I kind of liked the original idea of a "make format", and then you could have subtargets like "make format-c" and "make format-perl" if you don't have all the tools. The parallel pgindent is pretty nice, but I wonder how "use POSIX" works on Windows? (But in practice I mostly use pgindent --commit=@, which is still much faster.) ^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: Add "format" target to make and ninja to run pgindent and pgperltidy @ 2026-03-13 08:29 Jelte Fennema-Nio <[email protected]> parent: Peter Eisentraut <[email protected]> 0 siblings, 3 replies; 15+ messages in thread From: Jelte Fennema-Nio @ 2026-03-13 08:29 UTC (permalink / raw) To: Peter Eisentraut <[email protected]>; Andrew Dunstan <[email protected]>; Tom Lane <[email protected]>; Ashutosh Bapat <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]>; Daniel Gustafsson <[email protected]> On Thu Mar 12, 2026 at 10:10 AM CET, Peter Eisentraut wrote: > Apparently, this is still work in progress, but here are some comments > from me. Thanks for the review. I went over the code in detail now myself, and haven't found anything hugehely weird (although I did a bit of cleanup and additional comments). As said before I'm definetly not a Perl expert though, so I might have missed some wrong details. But all the logic at least seems sound. I also reorderd the commits to have the perltidy integration as the last ones, and the general pgindent QoL imorovements first. > I'm very interested in the patch "pgindent: Clean up temp files on > SIGINT", because this is an annoying problem. But I didn't find any > documentation about why this is the correct solution. I didn't find > anything on the File::Temp man page, for example. Do you have more details? I explained this black magic better now, and also did the same for SIGTERM. I also added a second commit which cleans up the .BAK files that pg_bsd_indent creates. Feel free to combine those commits if you think that's better. > The patch "pgindent: Use git ls-files to discover files" is also > interested and pretty straightforward. I guess we don't really care > about being able to run this in non-git trees? I don't think we care. This is a tool for Postgres developers, and I think we can assume all of those are using git to work on Postgres. > The other stuff I don't think I'm really on board with. In particular, > I don't like integrating pgperltidy into pgindent. These things are in > practice run at different times and the underlying tools update > differently and require different management, so integrating them all > might lead to various annoyances. So, to be clear. My goal is to get to a point where they ARE run at the same time, because their goal is the same, just for different filetypes. Afaict the primary reason that pg_bsd_indent is run on every commit, but perltidy is not, is that the later is currently much more annoying to run. Integrating it into pgindent is my attempt at making that barier much lower, so that we can make it part of the standard workflow. > I kind of liked the original idea of a "make format", and then you could > have subtargets like "make format-c" and "make format-perl" if you don't > have all the tools. I liked it too initially, but I then realized that means you lose all the nice flags that pgindent provides, e.g. --commit=@ or --check. Ofcourse you could add a FORMATARGS variable, but at that point why not just have people run pgindent directly? Also, perltidy is very slow to run. Running it on the whole codebase takes about a minute for me. So the new parallel and git ls-files support in pgindent are really helpful to have it finish in a reasonable time. > The parallel pgindent is pretty nice, but I wonder how "use POSIX" works > on Windows? Removed the POSIX thing. That wasn't needed. I'm wondering how many people use pginden ton Windows though. > (But in practice I mostly use pgindent --commit=@, which is still much > faster.) (--commit=@ should also get faster with the parallelization, because now it can run of the files from the commit in parallel) Attachments: [text/x-patch] v3-0001-pgindent-Clean-up-temp-files-created-by-File-Temp.patch (1.3K, 2-v3-0001-pgindent-Clean-up-temp-files-created-by-File-Temp.patch) download | inline diff: From 8862ec4be4139f6d7e84932b3435a4ea5940037d Mon Sep 17 00:00:00 2001 From: Jelte Fennema-Nio <[email protected]> Date: Wed, 4 Mar 2026 10:02:39 +0100 Subject: [PATCH v3 1/8] pgindent: Clean up temp files created by File::Temp on SIGINT When pressing Ctrl+C while running pgindent, it would often leave around files like pgtypedefAXUEEA. This slightly changes SIGINT handling so those files are cleaned up. --- src/tools/pgindent/pgindent | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/tools/pgindent/pgindent b/src/tools/pgindent/pgindent index 7481696a584..b96891b2252 100755 --- a/src/tools/pgindent/pgindent +++ b/src/tools/pgindent/pgindent @@ -19,6 +19,14 @@ use File::Temp; use IO::Handle; use Getopt::Long; +# By default Perl's SIGINT/SIGTERM kill the process without running END blocks, +# so File::Temp never gets to clean up. Converting the signal into an exit() +# makes END-block cleanup run. See: http://www.perlmonks.org/?node_id=714225 +# Exit codes use the 128+signum convention so callers can tell the process was +# killed by a signal. +$SIG{INT} = sub { exit 130; }; # 128 + 2 (SIGINT) +$SIG{TERM} = sub { exit 143; }; # 128 + 15 (SIGTERM) + # Update for pg_bsd_indent version my $INDENT_VERSION = "2.1.2"; base-commit: a0b6ef29a51818a4073a5f390ed10ef6453d5c11 -- 2.53.0 [text/x-patch] v3-0002-pgindent-Always-clean-up-.BAK-files-from-pg_bsd_i.patch (1.5K, 3-v3-0002-pgindent-Always-clean-up-.BAK-files-from-pg_bsd_i.patch) download | inline diff: From f63ca9cb6869452e85aec7b09d4ae471ab1f3030 Mon Sep 17 00:00:00 2001 From: Jelte Fennema-Nio <[email protected]> Date: Fri, 13 Mar 2026 00:21:04 +0100 Subject: [PATCH v3 2/8] pgindent: Always clean up .BAK files from pg_bsd_indent The previous commit let pgindent clean up File::Temp files on SIGINT. This extends that to also cleaning up the .BAK files, created by pg_bsd_indent. --- src/tools/pgindent/pgindent | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/src/tools/pgindent/pgindent b/src/tools/pgindent/pgindent index b96891b2252..6ce695895eb 100755 --- a/src/tools/pgindent/pgindent +++ b/src/tools/pgindent/pgindent @@ -27,6 +27,12 @@ use Getopt::Long; $SIG{INT} = sub { exit 130; }; # 128 + 2 (SIGINT) $SIG{TERM} = sub { exit 143; }; # 128 + 15 (SIGTERM) +# pg_bsd_indent creates a .BAK file that File::Temp doesn't know about. This +# END block makes sure that that file is cleaned up in case someone presses +# Ctrl+C during pgindent. +my $bak_to_cleanup; +END { unlink $bak_to_cleanup if defined $bak_to_cleanup; } + # Update for pg_bsd_indent version my $INDENT_VERSION = "2.1.2"; @@ -295,11 +301,14 @@ sub run_indent print $tmp_fh $source; $tmp_fh->close(); + $bak_to_cleanup = "$filename.BAK"; + $$error_message = `$cmd $filename 2>&1`; return "" if ($? || length($$error_message) > 0); - unlink "$filename.BAK"; + unlink $bak_to_cleanup; + $bak_to_cleanup = undef; open(my $src_out, '<', $filename) || die $!; local ($/) = undef; -- 2.53.0 [text/x-patch] v3-0003-pgindent-Use-git-ls-files-to-discover-files.patch (2.3K, 4-v3-0003-pgindent-Use-git-ls-files-to-discover-files.patch) download | inline diff: From ae515e0d4a848da800ecf7e7743902a80a2344ea Mon Sep 17 00:00:00 2001 From: Jelte Fennema-Nio <[email protected]> Date: Tue, 3 Mar 2026 09:43:33 +0100 Subject: [PATCH v3 3/8] pgindent: Use git ls-files to discover files When running pgindent on the whole source tree, it would often also indent build artifacts. This changes the pgindent directory search logic to become git-aware, and only indent files that are actually tracked by git. Any files that are explicitly part of the pgindent its arguments, are always indented though, even if they are not tracked by git. This can be useful to force indentation of an untracked file and/or for editor integration. --- src/tools/pgindent/pgindent | 44 +++++++++++++++++++++++++++++-------- 1 file changed, 35 insertions(+), 9 deletions(-) diff --git a/src/tools/pgindent/pgindent b/src/tools/pgindent/pgindent index 6ce695895eb..b6fbda9a298 100755 --- a/src/tools/pgindent/pgindent +++ b/src/tools/pgindent/pgindent @@ -13,7 +13,6 @@ use strict; use warnings FATAL => 'all'; use Cwd qw(abs_path getcwd); -use File::Find; use File::Spec; use File::Temp; use IO::Handle; @@ -369,16 +368,43 @@ $filtered_typedefs_fh = load_typedefs(); check_indent(); -my $wanted = sub { - my ($dev, $ino, $mode, $nlink, $uid, $gid); - (($dev, $ino, $mode, $nlink, $uid, $gid) = lstat($_)) - && -f _ - && /^.*\.[ch]\z/s - && push(@files, $File::Find::name); -}; +sub discover_files +{ + my @paths = @_; + my @discovered; + + # Separate individual files from directories + my @dirs; + foreach my $path (@paths) + { + if (-f $path) + { + push(@discovered, $path); + } + elsif (-d $path) + { + push(@dirs, $path); + } + else + { + warn "Could not find $path"; + } + } + + # Use git ls-files for directories to avoid searching build trees etc. + if (@dirs) + { + my @git_files = `git ls-files -- @dirs`; + die "git ls-files error" if $?; + chomp(@git_files); + push(@discovered, grep { /\.[ch]$/ } @git_files); + } + + return @discovered; +} # any non-option arguments are files or directories to be processed -File::Find::find({ wanted => $wanted }, @ARGV) if @ARGV; +push(@files, discover_files(@ARGV)) if @ARGV; # commit file locations are relative to the source root chdir "$sourcedir/../../.." if @commits && $sourcedir; -- 2.53.0 [text/x-patch] v3-0004-pgindent-Default-to-indenting-the-current-directo.patch (1.1K, 5-v3-0004-pgindent-Default-to-indenting-the-current-directo.patch) download | inline diff: From 1433ef85b04bb772f3e470a1d41a17c7c96392cd Mon Sep 17 00:00:00 2001 From: Jelte Fennema-Nio <[email protected]> Date: Tue, 3 Mar 2026 20:56:15 +0100 Subject: [PATCH v3 4/8] pgindent: Default to indenting the current directory if no files are given Previously running pgindent without giving it any files would result in this output: src/tools/pgindent/pgindent No files to process at src/tools/pgindent/pgindent line 526. This instead indents the current directory, which is probably what the user intended. --- src/tools/pgindent/pgindent | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/tools/pgindent/pgindent b/src/tools/pgindent/pgindent index b6fbda9a298..e6bba48893c 100755 --- a/src/tools/pgindent/pgindent +++ b/src/tools/pgindent/pgindent @@ -62,6 +62,9 @@ usage() if $help; usage("Cannot use --commit with command line file list") if (@commits && @ARGV); +# default to current directory if no files/dirs given +@ARGV = ('.') unless @ARGV || @commits; + # command line option wins, then environment, then locations based on current # dir, then default location $typedefs_file ||= $ENV{PGTYPEDEFS}; -- 2.53.0 [text/x-patch] v3-0005-pgindent-Allow-parallel-pgindent-runs.patch (5.8K, 6-v3-0005-pgindent-Allow-parallel-pgindent-runs.patch) download | inline diff: From 5a17a9619c1a6a611fbfdeb313c32f35e1149502 Mon Sep 17 00:00:00 2001 From: Jelte Fennema-Nio <[email protected]> Date: Wed, 4 Mar 2026 09:24:24 +0100 Subject: [PATCH v3 5/8] pgindent: Allow parallel pgindent runs Running pgindent on the whole source tree can take a while. This adds support for pgindent to indent files in parallel. This speeds up a full pgindent run from ~7 seconds to 1 second on my machine. Especially with future commits that integrate perltidy into pgindent the wins are huge, because perltidy is much slower at formatting than pg_bsd_indent. With those later commits the time it takes to do a full pgindent run (including perltidy) takes more than a minute on my machine without the parallelization, but only take ~7 seconds when run in parallel. --- src/tools/pgindent/pgindent | 114 ++++++++++++++++++++++++++++++------ 1 file changed, 95 insertions(+), 19 deletions(-) diff --git a/src/tools/pgindent/pgindent b/src/tools/pgindent/pgindent index e6bba48893c..85271c9986e 100755 --- a/src/tools/pgindent/pgindent +++ b/src/tools/pgindent/pgindent @@ -17,6 +17,7 @@ use File::Spec; use File::Temp; use IO::Handle; use Getopt::Long; +use Fcntl qw(:flock); # By default Perl's SIGINT/SIGTERM kill the process without running END blocks, # so File::Temp never gets to clean up. Converting the signal into an exit() @@ -41,10 +42,11 @@ my $indent_opts = my $devnull = File::Spec->devnull; -my ($typedefs_file, $typedef_str, @excludes, $indent, $diff, - $check, $help, @commits,); +my ($typedefs_file, $typedef_str, @excludes, $indent, + $diff, $check, $help, @commits, $jobs,); $help = 0; +$jobs = 0; my %options = ( "help" => \$help, @@ -54,9 +56,32 @@ my %options = ( "excludes=s" => \@excludes, "indent=s" => \$indent, "diff" => \$diff, - "check" => \$check,); + "check" => \$check, + "jobs|j=i" => \$jobs,); GetOptions(%options) || usage("bad command line argument"); +sub get_num_cpus +{ + # Try nproc (Linux, some BSDs), then sysctl (macOS, FreeBSD). + for my $cmd ('nproc', 'sysctl -n hw.ncpu') + { + my $n = `$cmd 2>$devnull`; + chomp $n; + return $n + 0 if ($? == 0 && $n =~ /^\d+$/ && $n > 0); + } + return 1; +} + +if ($jobs == 0) +{ + $jobs = get_num_cpus(); +} +elsif ($jobs < 0) +{ + usage("--jobs must be a non-negative number"); +} + + usage() if $help; usage("Cannot use --commit with command line file list") @@ -351,6 +376,7 @@ Options: --indent=PATH path to pg_bsd_indent program --diff show the changes that would be made --check exit with status 2 if any changes would be made + --jobs=N, -j N number of parallel workers (0 = num CPUs, default 0) The --excludes and --commit options can be given more than once. EOF if ($help) @@ -427,32 +453,30 @@ warn "No files to process" unless @files; # remove excluded files from the file list process_exclude(); -my %processed; -my $status = 0; +# Used by forked children to serialize diff output to STDOUT via flock(). +my $stdout_lock_fh = new File::Temp(TEMPLATE => "pglockXXXXX"); -foreach my $source_filename (@files) +sub process_file { - # skip duplicates - next if $processed{$source_filename}; - $processed{$source_filename} = 1; + my $source_filename = shift; # ignore anything that's not a .c or .h file - next unless $source_filename =~ /\.[ch]$/; + return 0 unless $source_filename =~ /\.[ch]$/; # don't try to indent a file that doesn't exist unless (-f $source_filename) { warn "Could not find $source_filename"; - next; + return 0; } # Automatically ignore .c and .h files that correspond to a .y or .l # file. indent tends to get badly confused by Bison/flex output, # and there's no value in indenting derived files anyway. my $otherfile = $source_filename; $otherfile =~ s/\.[ch]$/.y/; - next if $otherfile ne $source_filename && -f $otherfile; + return 0 if $otherfile ne $source_filename && -f $otherfile; $otherfile =~ s/\.y$/.l/; - next if $otherfile ne $source_filename && -f $otherfile; + return 0 if $otherfile ne $source_filename && -f $otherfile; my $source = read_source($source_filename); my $orig_source = $source; @@ -464,8 +488,7 @@ foreach my $source_filename (@files) if ($source eq "") { print STDERR "Failure in $source_filename: " . $error_message . "\n"; - $status = 3; - next; + return 3; } $source = post_indent($source); @@ -480,14 +503,67 @@ foreach my $source_filename (@files) { if ($diff) { - print diff($source, $source_filename); + my $output = diff($source, $source_filename); + flock($stdout_lock_fh, LOCK_EX); + print $output; + STDOUT->flush(); + flock($stdout_lock_fh, LOCK_UN); } - if ($check) + return 2 if $check; + } + } + + return 0; +} + +# deduplicate file list +my %seen; +@files = grep { !$seen{$_}++ } @files; + +my $status = 0; + +if ($jobs <= 1) +{ + foreach my $source_filename (@files) + { + my $file_status = process_file($source_filename); + $status = $file_status if $file_status > $status; + last if $check && $status >= 2 && !$diff; + } +} +else +{ + my %children; # pid => 1 + + my $file_idx = 0; + while ($file_idx < scalar(@files) || %children) + { + # Fork new children up to $jobs limit + while ($file_idx < scalar(@files) && scalar(keys %children) < $jobs) + { + my $source_filename = $files[ $file_idx++ ]; + + my $pid = fork(); + die "fork failed: $!\n" unless defined $pid; + + if ($pid == 0) { - $status ||= 2; - last unless $diff; + # child + my $child_status = process_file($source_filename); + exit $child_status; } + + $children{$pid} = 1; + } + + # Wait for at least one child to finish + my $pid = waitpid(-1, 0); + if ($pid > 0 && exists $children{$pid}) + { + delete $children{$pid}; + my $child_status = $? >> 8; + $status = $child_status if $child_status > $status; } } } -- 2.53.0 [text/x-patch] v3-0006-pgindent-Try-to-find-pg_bsd_indent-binary-in-comm.patch (2.1K, 7-v3-0006-pgindent-Try-to-find-pg_bsd_indent-binary-in-comm.patch) download | inline diff: From b6c4a895ff08b46d65114728f413ace63a143729 Mon Sep 17 00:00:00 2001 From: Jelte Fennema-Nio <[email protected]> Date: Tue, 3 Mar 2026 23:46:52 +0100 Subject: [PATCH v3 6/8] pgindent: Try to find pg_bsd_indent binary in common locations To run pgindent you need to to have the right version of pg_bsd_indent in your PATH, or specify it manually. This is a bit of a hassle, especially for newcomers or when working on backbranches. So this chnages pgindent to search for a pg_bsd_indent that's built from the current sources, both in-tree (for in-tree autoconf builds) and in a build directory (for meson or autoconf vpath builds). --- src/tools/pgindent/pgindent | 29 ++++++++++++++++++++++++++--- 1 file changed, 26 insertions(+), 3 deletions(-) diff --git a/src/tools/pgindent/pgindent b/src/tools/pgindent/pgindent index 85271c9986e..97bebc6282e 100755 --- a/src/tools/pgindent/pgindent +++ b/src/tools/pgindent/pgindent @@ -94,11 +94,34 @@ usage("Cannot use --commit with command line file list") # dir, then default location $typedefs_file ||= $ENV{PGTYPEDEFS}; -# get indent location for environment or default -$indent ||= $ENV{PGINDENT} || $ENV{INDENT} || "pg_bsd_indent"; - my $sourcedir = locate_sourcedir(); +# get indent location: command line wins, then environment, then try to find +# a compiled pg_bsd_indent in the source tree, then fall back to PATH. +$indent ||= $ENV{PGINDENT} || $ENV{INDENT}; +if (!$indent && $sourcedir) +{ + my $srcroot = "$sourcedir/../../.."; + my $bsd_indent_subdir = "src/tools/pg_bsd_indent/pg_bsd_indent"; + + # Look for pg_bsd_indent: first in-tree (autoconf in-tree build), + # then in a "build" directory (meson or autoconf vpath), + # then any "build*" directory. + foreach my $candidate ( + "$srcroot/$bsd_indent_subdir", + "$srcroot/build/$bsd_indent_subdir", + glob("$srcroot/build*/$bsd_indent_subdir")) + { + if (-x $candidate) + { + $indent = $candidate; + last; + } + } +} +$indent ||= "pg_bsd_indent"; + + # if it's the base of a postgres tree, we will exclude the files # postgres wants excluded if ($sourcedir) -- 2.53.0 [text/x-patch] v3-0007-pgindent-Integrate-pgperltidy-functionality-into-.patch (11.7K, 8-v3-0007-pgindent-Integrate-pgperltidy-functionality-into-.patch) download | inline diff: From 861edbfb36b063d13d52da73956f6b2faf8f72ef Mon Sep 17 00:00:00 2001 From: Jelte Fennema-Nio <[email protected]> Date: Wed, 31 Dec 2025 11:07:58 +0100 Subject: [PATCH v3 7/8] pgindent: Integrate pgperltidy functionality into pgindent Over time our pgindent script has gotten a lot of quality of life features, like the --commit, --diff and --check flags. This integrates the functionality of pgperltidy into pgindent, so it can benefit from these same quality of life improvements, as well as future ones. This commit adds a --perltidy flag to pgindent, which when given will cause pgindent to also format Perl files in addition to the C files it would normally format. It also adds a --perl-only flag, to (as the name suggests) only format Perl files. --- src/tools/pgindent/README | 14 +-- src/tools/pgindent/pgindent | 191 ++++++++++++++++++++++++++++------ src/tools/pgindent/pgperltidy | 18 ---- 3 files changed, 168 insertions(+), 55 deletions(-) delete mode 100755 src/tools/pgindent/pgperltidy diff --git a/src/tools/pgindent/README b/src/tools/pgindent/README index b6cd4c6f6b7..2e31471d7e6 100644 --- a/src/tools/pgindent/README +++ b/src/tools/pgindent/README @@ -35,6 +35,10 @@ DOING THE INDENT RUN BEFORE A NORMAL COMMIT: src/tools/pgindent/pgindent . + To also format Perl files at the same time, add --perltidy: + + src/tools/pgindent/pgindent --perltidy=perltidy . + If any files generate errors, restore their original versions with "git checkout", and see below for cleanup ideas. @@ -76,12 +80,12 @@ AT LEAST ONCE PER RELEASE CYCLE: 2) Run pgindent as above. -3) Indent the Perl code using perltidy: +3) Indent the Perl code using perltidy (if not already done in step 2): - src/tools/pgindent/pgperltidy . + src/tools/pgindent/pgindent --perl-only . If you want to use some perltidy version that's not in your PATH, - first set the PERLTIDY environment variable to point to it. + use --perltidy=PATH or set the PERLTIDY environment variable. 4) Reformat the bootstrap catalog data files: @@ -166,6 +170,4 @@ Note that we do not exclude ecpg's header files from the run. Some of them get copied verbatim into ecpg's output, meaning that ecpg's expected files may need to be updated to match. -The perltidy run processes all *.pl and *.pm files, plus a few -executable Perl scripts that are not named that way. See the "find" -rules in pgperltidy for details. +When --perltidy is given, pgindent also processes *.pl and *.pm files. diff --git a/src/tools/pgindent/pgindent b/src/tools/pgindent/pgindent index 97bebc6282e..9d635b7a22b 100755 --- a/src/tools/pgindent/pgindent +++ b/src/tools/pgindent/pgindent @@ -2,12 +2,12 @@ # Copyright (c) 2021-2026, PostgreSQL Global Development Group -# Program to maintain uniform layout style in our C code. +# Program to maintain uniform layout style in our C and Perl code. # Exit codes: # 0 -- all OK # 1 -- error invoking pgindent, nothing done # 2 -- --check mode and at least one file requires changes -# 3 -- pg_bsd_indent failed on at least one file +# 3 -- pg_bsd_indent or perltidy failed on at least one file use strict; use warnings FATAL => 'all'; @@ -43,11 +43,17 @@ my $indent_opts = my $devnull = File::Spec->devnull; my ($typedefs_file, $typedef_str, @excludes, $indent, - $diff, $check, $help, @commits, $jobs,); + $diff, $check, $help, @commits, + $perltidy_arg, $perl_only, $jobs,); $help = 0; $jobs = 0; +# Save @ARGV before parsing so we can distinguish --perltidy=PATH (where +# the value should be used as the perltidy path) from --perltidy PATH +# (where PATH is a file to format that Getopt::Long greedily consumed). +my @orig_argv = @ARGV; + my %options = ( "help" => \$help, "commit=s" => \@commits, @@ -55,6 +61,8 @@ my %options = ( "list-of-typedefs=s" => \$typedef_str, "excludes=s" => \@excludes, "indent=s" => \$indent, + "perltidy:s" => \$perltidy_arg, + "perl-only" => \$perl_only, "diff" => \$diff, "check" => \$check, "jobs|j=i" => \$jobs,); @@ -81,6 +89,16 @@ elsif ($jobs < 0) usage("--jobs must be a non-negative number"); } +if (defined($perltidy_arg) && $perltidy_arg ne '') +{ + # Check wheter we have --perltidy=PATH or --perltidy PATH, and if the + # latter, consider the argument to be a file to format, not a perltidy path. + unless (grep { $_ eq "--perltidy=$perltidy_arg" } @orig_argv) + { + unshift(@ARGV, $perltidy_arg); + $perltidy_arg = ''; + } +} usage() if $help; @@ -94,6 +112,12 @@ usage("Cannot use --commit with command line file list") # dir, then default location $typedefs_file ||= $ENV{PGTYPEDEFS}; +# get perltidy location: command line wins, then environment, then default. +# --perltidy (with or without a path) and --perl-only all imply perltidy is +# wanted, falling back to the PERLTIDY env var, then "perltidy" in PATH. +my $perltidy = $perltidy_arg || $ENV{PERLTIDY} + || (defined($perltidy_arg) || $perl_only ? "perltidy" : undef); + my $sourcedir = locate_sourcedir(); # get indent location: command line wins, then environment, then try to find @@ -180,6 +204,30 @@ sub check_indent return; } +my $PERLTIDY_VERSION = "20230309"; + +sub check_perltidy +{ + my $ver = `$perltidy -v 2>&1`; + if ($? != 0) + { + print STDERR + "You do not appear to have $perltidy installed on your system.\n" + . "See src/tools/pgindent/README for installation instructions.\n"; + exit 1; + } + + if ($ver !~ m/$PERLTIDY_VERSION/) + { + print STDERR + "You do not appear to have $perltidy version $PERLTIDY_VERSION installed on your system.\n" + . "See src/tools/pgindent/README for installation instructions.\n"; + exit 1; + } + + return; +} + sub locate_sourcedir { # try fairly hard to locate the sourcedir @@ -368,6 +416,43 @@ sub run_indent return $source; } +sub format_c +{ + my $source = shift; + my $source_filename = shift; + my $error_message = ''; + + my $formatted = pre_indent($source); + $formatted = run_indent($formatted, \$error_message); + if ($formatted eq "") + { + print STDERR "Failure in $source_filename: " . $error_message . "\n"; + return ("", 1); + } + return post_indent($formatted); +} + +sub format_perl +{ + my $source = shift; + my $source_filename = shift; + + my $tmp_fh = new File::Temp(TEMPLATE => "pgperltidyXXXXX"); + my $tmp_filename = $tmp_fh->filename; + print $tmp_fh $source; + $tmp_fh->close(); + + my $perltidy_profile = "$sourcedir/perltidyrc"; + my $err = + `$perltidy --profile=$perltidy_profile -b -bext='/' $tmp_filename 2>&1`; + if ($? != 0) + { + print STDERR "Failure in $source_filename: " . $err . "\n"; + return ("", 1); + } + return read_source($tmp_filename); +} + sub diff { my $indented = shift; @@ -397,6 +482,8 @@ Options: --list-of-typedefs=STR string containing typedefs, space separated --excludes=PATH file containing list of filename patterns to ignore --indent=PATH path to pg_bsd_indent program + --perltidy[=PATH] enable Perl formatting (optionally set perltidy path) + --perl-only format only Perl files, skip C formatting --diff show the changes that would be made --check exit with status 2 if any changes would be made --jobs=N, -j N number of parallel workers (0 = num CPUs, default 0) @@ -416,9 +503,46 @@ EOF # main -$filtered_typedefs_fh = load_typedefs(); +my $do_c = !$perl_only; +my $do_perl = defined($perltidy); + +if ($do_c) +{ + $filtered_typedefs_fh = load_typedefs(); + check_indent(); +} -check_indent(); +if ($do_perl) +{ + check_perltidy(); +} + +sub is_c_file +{ + my $filename = shift; + # It needs to have .c or .h extension + return 0 unless $filename =~ /\.[ch]$/; + # Automatically ignore .c and .h files that correspond to a .y or .l file. + # pg_bsd_indent tends to get badly confused by Bison/flex output, and + # there's no value in indenting derived files anyway. + my $otherfile = $filename; + $otherfile =~ s/\.[ch]$/.y/; + return 0 if $otherfile ne $filename && -f $otherfile; + $otherfile =~ s/\.y$/.l/; + return 0 if $otherfile ne $filename && -f $otherfile; + return 1; +} + +sub is_perl_file +{ + my $filename = shift; + # take all .pl and .pm files + return 1 if $filename =~ /\.p[lm]$/; + # take executable files that file(1) thinks are perl files + return 0 unless -x $filename; + my $file_out = `file "$filename"`; + return $file_out =~ /:.*perl[0-9]*\b/i; +} sub discover_files { @@ -449,14 +573,27 @@ sub discover_files my @git_files = `git ls-files -- @dirs`; die "git ls-files error" if $?; chomp(@git_files); - push(@discovered, grep { /\.[ch]$/ } @git_files); + push(@discovered, @git_files); } return @discovered; } # any non-option arguments are files or directories to be processed -push(@files, discover_files(@ARGV)) if @ARGV; +if (@ARGV) +{ + foreach my $f (discover_files(@ARGV)) + { + if ($do_c && is_c_file($f)) + { + push(@files, $f); + } + elsif ($do_perl && is_perl_file($f)) + { + push(@files, $f); + } + } +} # commit file locations are relative to the source root chdir "$sourcedir/../../.." if @commits && $sourcedir; @@ -468,7 +605,8 @@ foreach my $commit (@commits) my @affected = `git diff --diff-filter=ACMR --name-only $prev $commit`; die "git error" if $?; chomp(@affected); - push(@files, @affected); + push(@files, grep { is_c_file($_) } @affected) if $do_c; + push(@files, grep { is_perl_file($_) } @affected) if $do_perl; } warn "No files to process" unless @files; @@ -483,50 +621,41 @@ sub process_file { my $source_filename = shift; - # ignore anything that's not a .c or .h file - return 0 unless $source_filename =~ /\.[ch]$/; - - # don't try to indent a file that doesn't exist + # don't try to format a file that doesn't exist unless (-f $source_filename) { warn "Could not find $source_filename"; return 0; } - # Automatically ignore .c and .h files that correspond to a .y or .l - # file. indent tends to get badly confused by Bison/flex output, - # and there's no value in indenting derived files anyway. - my $otherfile = $source_filename; - $otherfile =~ s/\.[ch]$/.y/; - return 0 if $otherfile ne $source_filename && -f $otherfile; - $otherfile =~ s/\.y$/.l/; - return 0 if $otherfile ne $source_filename && -f $otherfile; my $source = read_source($source_filename); - my $orig_source = $source; - my $error_message = ''; + my ($formatted, $failure); - $source = pre_indent($source); + if ($source_filename =~ /\.[ch]$/) + { + ($formatted, $failure) = format_c($source, $source_filename); + } + else + { + ($formatted, $failure) = format_perl($source, $source_filename); + } - $source = run_indent($source, \$error_message); - if ($source eq "") + if ($failure) { - print STDERR "Failure in $source_filename: " . $error_message . "\n"; return 3; } - $source = post_indent($source); - - if ($source ne $orig_source) + if ($formatted ne $source) { if (!$diff && !$check) { - write_source($source, $source_filename); + write_source($formatted, $source_filename); } else { if ($diff) { - my $output = diff($source, $source_filename); + my $output = diff($formatted, $source_filename); flock($stdout_lock_fh, LOCK_EX); print $output; STDOUT->flush(); diff --git a/src/tools/pgindent/pgperltidy b/src/tools/pgindent/pgperltidy deleted file mode 100755 index 87838d6bde3..00000000000 --- a/src/tools/pgindent/pgperltidy +++ /dev/null @@ -1,18 +0,0 @@ -#!/bin/sh - -# src/tools/pgindent/pgperltidy - -set -e - -# set this to override default perltidy program: -PERLTIDY=${PERLTIDY:-perltidy} - -PERLTIDY_VERSION=20230309 -if ! $PERLTIDY -v | grep -q $PERLTIDY_VERSION; then - echo "You do not appear to have $PERLTIDY version $PERLTIDY_VERSION installed on your system." >&2 - exit 1 -fi - -. src/tools/perlcheck/find_perl_files - -find_perl_files "$@" | xargs $PERLTIDY --profile=src/tools/pgindent/perltidyrc -- 2.53.0 [text/x-patch] v3-0008-pgindent-Add-easy-way-of-getting-perltidy.patch (5.9K, 9-v3-0008-pgindent-Add-easy-way-of-getting-perltidy.patch) download | inline diff: From 3f5ea701cf8b9abf4fb97804daaee15889b15987 Mon Sep 17 00:00:00 2001 From: Jelte Fennema-Nio <[email protected]> Date: Wed, 4 Mar 2026 09:04:15 +0100 Subject: [PATCH v3 8/8] pgindent: Add easy way of getting perltidy We need a very specific perltidy version, but getting that installed is quite a hassle. Especially for people who don't use perl on a daily basis. This adds a small script to download the exact perltidy version that we need. --- src/tools/pgindent/.gitignore | 1 + src/tools/pgindent/README | 16 ++++----- src/tools/pgindent/get_perltidy | 62 +++++++++++++++++++++++++++++++++ src/tools/pgindent/pgindent | 30 +++++++++++++--- 4 files changed, 96 insertions(+), 13 deletions(-) create mode 100644 src/tools/pgindent/.gitignore create mode 100755 src/tools/pgindent/get_perltidy diff --git a/src/tools/pgindent/.gitignore b/src/tools/pgindent/.gitignore new file mode 100644 index 00000000000..1497217113b --- /dev/null +++ b/src/tools/pgindent/.gitignore @@ -0,0 +1 @@ +/perltidy/ diff --git a/src/tools/pgindent/README b/src/tools/pgindent/README index 2e31471d7e6..264038e7e09 100644 --- a/src/tools/pgindent/README +++ b/src/tools/pgindent/README @@ -17,14 +17,14 @@ PREREQUISITES: 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-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-20230309.tar.gz + + The easiest way is to use the get_perltidy script, which downloads, + verifies, and installs the correct version into the source tree: + + src/tools/pgindent/get_perltidy + + This installs perltidy into src/tools/pgindent/perltidy/, which + pgindent will find automatically. DOING THE INDENT RUN BEFORE A NORMAL COMMIT: diff --git a/src/tools/pgindent/get_perltidy b/src/tools/pgindent/get_perltidy new file mode 100755 index 00000000000..09ce4a195cb --- /dev/null +++ b/src/tools/pgindent/get_perltidy @@ -0,0 +1,62 @@ +#!/bin/sh + +# src/tools/pgindent/get_perltidy +# +# Downloads and installs perltidy 20230309 into src/tools/pgindent/perltidy/. + +set -e + +PERLTIDY_VERSION=20230309 +PERLTIDY_TARBALL="Perl-Tidy-${PERLTIDY_VERSION}.tar.gz" +PERLTIDY_URL="https://cpan.metacpan.org/authors/id/S/SH/SHANCOCK/${PERLTIDY_TARBALL}" +PERLTIDY_SHA256="e22949a208c618d671a18c5829b451abbe9da0da2cddd78fdbfcb036c7361c18" + +# Determine the directory this script lives in, i.e. src/tools/pgindent +SCRIPT_DIR=$(cd "$(dirname "$0")" && pwd) + +PERLTIDY_DIR="$SCRIPT_DIR/perltidy" +PERLTIDY_BIN="$PERLTIDY_DIR/bin/perltidy" + +# Check if already installed with the correct version +if [ -x "$PERLTIDY_BIN" ]; then + perltidy_lib="$PERLTIDY_DIR/lib/perl5" + installed_version=$(PERL5LIB="$perltidy_lib${PERL5LIB:+:$PERL5LIB}" "$PERLTIDY_BIN" -v 2>/dev/null || true) + if echo "$installed_version" | grep -q "$PERLTIDY_VERSION"; then + echo "perltidy $PERLTIDY_VERSION is already installed in $PERLTIDY_DIR" + exit 0 + fi +fi + +WORK_DIR=$(mktemp -d) +trap 'rm -rf "$WORK_DIR"' EXIT + +# Download +TARBALL="$WORK_DIR/$PERLTIDY_TARBALL" +if command -v curl >/dev/null 2>&1; then + curl -sSL -o "$TARBALL" "$PERLTIDY_URL" +elif command -v wget >/dev/null 2>&1; then + wget -q -O "$TARBALL" "$PERLTIDY_URL" +else + echo "error: neither curl nor wget found" >&2 + exit 1 +fi + +# Verify SHA256 +if command -v sha256sum >/dev/null 2>&1; then + echo "$PERLTIDY_SHA256 $TARBALL" | sha256sum -c - >/dev/null +elif command -v shasum >/dev/null 2>&1; then + echo "$PERLTIDY_SHA256 $TARBALL" | shasum -a 256 -c - >/dev/null +else + echo "error: neither sha256sum nor shasum found" >&2 + exit 1 +fi + +# Extract, build, install +cd "$WORK_DIR" +tar xzf "$TARBALL" +cd "Perl-Tidy-${PERLTIDY_VERSION}" +perl Makefile.PL INSTALL_BASE="$PERLTIDY_DIR" >/dev/null +make >/dev/null +make install >/dev/null + +echo "perltidy $PERLTIDY_VERSION installed in $PERLTIDY_DIR" diff --git a/src/tools/pgindent/pgindent b/src/tools/pgindent/pgindent index 9d635b7a22b..8003dadc4e1 100755 --- a/src/tools/pgindent/pgindent +++ b/src/tools/pgindent/pgindent @@ -112,14 +112,34 @@ usage("Cannot use --commit with command line file list") # dir, then default location $typedefs_file ||= $ENV{PGTYPEDEFS}; -# get perltidy location: command line wins, then environment, then default. -# --perltidy (with or without a path) and --perl-only all imply perltidy is -# wanted, falling back to the PERLTIDY env var, then "perltidy" in PATH. -my $perltidy = $perltidy_arg || $ENV{PERLTIDY} - || (defined($perltidy_arg) || $perl_only ? "perltidy" : undef); +# get perltidy location: command line wins, then environment, then try to +# find a get_perltidy-installed copy in the source tree, then fall back to +# PATH. --perltidy (with or without a path) and --perl-only all imply +# perltidy is wanted. +my $perltidy = $perltidy_arg || $ENV{PERLTIDY}; my $sourcedir = locate_sourcedir(); +if (!$perltidy && (defined($perltidy_arg) || $perl_only) && $sourcedir) +{ + # Look for a get_perltidy-installed perltidy in the source tree. + my $candidate = "$sourcedir/perltidy/bin/perltidy"; + if (-x $candidate) + { + $perltidy = $candidate; + + # Local installs need PERL5LIB set so perltidy can find its + # modules. + my $libdir = "$sourcedir/perltidy/lib/perl5"; + if (-d $libdir) + { + $ENV{PERL5LIB} = + $ENV{PERL5LIB} ? "$libdir:$ENV{PERL5LIB}" : $libdir; + } + } +} +$perltidy ||= (defined($perltidy_arg) || $perl_only ? "perltidy" : undef); + # get indent location: command line wins, then environment, then try to find # a compiled pg_bsd_indent in the source tree, then fall back to PATH. $indent ||= $ENV{PGINDENT} || $ENV{INDENT}; -- 2.53.0 ^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: Add "format" target to make and ninja to run pgindent and pgperltidy @ 2026-03-13 15:11 Jelte Fennema-Nio <[email protected]> parent: Jelte Fennema-Nio <[email protected]> 2 siblings, 1 reply; 15+ messages in thread From: Jelte Fennema-Nio @ 2026-03-13 15:11 UTC (permalink / raw) To: Daniel Gustafsson <[email protected]>; +Cc: Peter Eisentraut <[email protected]>; Andrew Dunstan <[email protected]>; Tom Lane <[email protected]>; Ashutosh Bapat <[email protected]>; PostgreSQL Hackers <[email protected]> On Fri, 13 Mar 2026 at 11:03, Daniel Gustafsson <[email protected]> wrote: > The primary reason thus far has been that pgperltidy requires a specific > version of perltidy, no other version is accepted. Imposing the burden of > installing that on a greater subset of people than a subset of committers > didn't seem palatable. I think that makes sense if the burden is big, but with patch 0008 it becomes as simple as running a single command: src/tools/pgindent/get_perltidy ^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: Add "format" target to make and ninja to run pgindent and pgperltidy @ 2026-03-13 22:26 Daniel Gustafsson <[email protected]> parent: Jelte Fennema-Nio <[email protected]> 0 siblings, 0 replies; 15+ messages in thread From: Daniel Gustafsson @ 2026-03-13 22:26 UTC (permalink / raw) To: Jelte Fennema-Nio <[email protected]>; +Cc: Peter Eisentraut <[email protected]>; Andrew Dunstan <[email protected]>; Tom Lane <[email protected]>; Ashutosh Bapat <[email protected]>; PostgreSQL Hackers <[email protected]> > On 13 Mar 2026, at 16:11, Jelte Fennema-Nio <[email protected]> wrote: > > On Fri, 13 Mar 2026 at 11:03, Daniel Gustafsson <[email protected]> wrote: >> The primary reason thus far has been that pgperltidy requires a specific >> version of perltidy, no other version is accepted. Imposing the burden of >> installing that on a greater subset of people than a subset of committers >> didn't seem palatable. > > I think that makes sense if the burden is big, but with patch 0008 it > becomes as simple as running a single command: > src/tools/pgindent/get_perltidy In todays world of supply-chain attacks, don't think it's all that palatable to take responsibility for installing code in peoples environments. Regardless, I don't think the patch should remove the installation instructions. -- Daniel Gustafsson ^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: Add "format" target to make and ninja to run pgindent and pgperltidy @ 2026-03-27 15:28 Jelte Fennema-Nio <[email protected]> parent: Jelte Fennema-Nio <[email protected]> 2 siblings, 0 replies; 15+ messages in thread From: Jelte Fennema-Nio @ 2026-03-27 15:28 UTC (permalink / raw) To: Tom Lane <[email protected]>; +Cc: Peter Eisentraut <[email protected]>; Andrew Dunstan <[email protected]>; Ashutosh Bapat <[email protected]>; PostgreSQL Hackers <[email protected]>; Daniel Gustafsson <[email protected]> On Fri, 27 Mar 2026 at 15:55, Tom Lane <[email protected]> wrote: > We did not start expecting commits to be pgindent-clean until pgindent > was integrated into our tree Merging these commits does not mean we force committers to run perltidy on every commit. That a completely separate discussion that is not worth having until after we make perltidy less of a pain to run. Even without forcing committers to run perltidy I think 0007 and 0008 are still beneficial. > The 0008 patch doesn't fix that, and in fact I think it would be > dangerous to even provide that script in our tree. It's a supply- > chain attack waiting to happen. I strongly disagree. Instead I think, our current pgindent README[1] is a supply-chain attack waiting to happen. Our pgindent README tells people to get a tar file from the CPAN website, but WITHOUT the signature checks that the script in 0008 includes. These added signature checks prevent it from being a supply chain risk. > Even if it were guaranteed 100% > secure, too many developers are subject to (perfectly reasonable) > corporate security policies that would look with disfavor on > unauthorized installation of Perl modules. I'd be curious to know which committer is not allowed to download and run a specific signature verified perl module, but is allowed to get the latest postgres source code from main. [1]: https://github.com/postgres/postgres/blob/9a9998163bda0d8c17d84ea22ced6a60f8018634/src/tools/pginden... P.S. Reading your response, I cannot help but interpret it as an attempt to sidestep any future discussion about always running perltidy, by pre-emptively rejecting any and all improvements that would make perltidy easier to run. ^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: Add "format" target to make and ninja to run pgindent and pgperltidy @ 2026-04-03 07:52 Jelte Fennema-Nio <[email protected]> parent: Jelte Fennema-Nio <[email protected]> 2 siblings, 0 replies; 15+ messages in thread From: Jelte Fennema-Nio @ 2026-04-03 07:52 UTC (permalink / raw) To: Peter Eisentraut <[email protected]>; Andrew Dunstan <[email protected]>; Tom Lane <[email protected]>; Ashutosh Bapat <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]>; Daniel Gustafsson <[email protected]> On Fri Mar 27, 2026 at 2:32 PM CET, Peter Eisentraut wrote: > I have committed these two. Thanks! > v3-0003-pgindent-Use-git-ls-files-to-discover-files.patch > > This looks structurally correct, but I wonder whether this: > > my @git_files = `git ls-files -- @dirs`; > > could be written in a way that is more robust against funny file names. > (pgindent is also used against trees that are not stock PostgreSQL.) I have changed it to use open so the args are not string interpolated and -z so the resulting files are null-separated instead of newline-separated. Is this the kind of thing you had in mind? > v3-0004-pgindent-Default-to-indenting-the-current-directo.patch > > Note that other tools also share this behavior with pgindent: > > src/tools/pgindent/pgperltidy > src/tools/perlcheck/pgperlcritic > src/tools/perlcheck/pgperlsyncheck > > If we change one, we should change all. I think that's fair, but because the other tools don't use git ls-files I don't wanna do that in this commit. Instead I updated the later commits to not just integrate pgperltidy into pgindent, but create a new pgcheck tool that combines all of these tools (the old scripts are kept as tiny wrappers). So after committing all these tools behave the same way, not just in wheter no arguments means "current directory" but also in all the flags they accept. Attachments: [text/x-patch] v4-0001-pgindent-Use-git-ls-files-to-discover-files.patch (2.8K, 2-v4-0001-pgindent-Use-git-ls-files-to-discover-files.patch) download | inline diff: From 7bc24625457bfaf54320523db49b44c2da4ead1e Mon Sep 17 00:00:00 2001 From: Jelte Fennema-Nio <[email protected]> Date: Tue, 3 Mar 2026 09:43:33 +0100 Subject: [PATCH v4 1/7] pgindent: Use git ls-files to discover files When running pgindent on the whole source tree, it would often also indent build artifacts. This changes the pgindent directory search logic to become git-aware, and only indent files that are actually tracked by git. Any files that are explicitly part of the pgindent its arguments, are always indented though, even if they are not tracked by git. This can be useful to force indentation of an untracked file and/or for editor integration. --- src/tools/pgindent/pgindent | 54 ++++++++++++++++++++++++++++++------- 1 file changed, 44 insertions(+), 10 deletions(-) diff --git a/src/tools/pgindent/pgindent b/src/tools/pgindent/pgindent index b2ec5e2914b..f5bc4b3642a 100755 --- a/src/tools/pgindent/pgindent +++ b/src/tools/pgindent/pgindent @@ -13,7 +13,6 @@ use strict; use warnings FATAL => 'all'; use Cwd qw(abs_path getcwd); -use File::Find; use File::Spec; use File::Temp; use IO::Handle; @@ -338,6 +337,49 @@ sub diff return $diff; } +sub discover_files +{ + my @paths = @_; + my @discovered; + + # Separate individual files from directories + my @dirs; + foreach my $path (@paths) + { + if (-f $path) + { + push(@discovered, $path); + } + elsif (-d $path) + { + push(@dirs, $path); + } + else + { + warn "Could not find $path"; + } + } + + # Use git ls-files for directories to avoid searching build trees etc. + if (@dirs) + { + # Use -z to handle files with newlines in their names, and split on \0 + # afterward. + open(my $git_ls, '-|', 'git', 'ls-files', '-z', '--', @dirs) + || die "could not open git ls-files: $!"; + binmode($git_ls); + # Slurp all output into a single string by temporarily unsetting + # the line separator. + my $git_output = do { local $/; <$git_ls> }; + close($git_ls); + die "git ls-files error" if $?; + my @git_files = split(/\0/, $git_output); + push(@discovered, grep { /\.[ch]$/ } @git_files); + } + + return @discovered; +} + sub usage { my $message = shift; @@ -373,16 +415,8 @@ $filtered_typedefs_fh = load_typedefs(); check_indent(); -my $wanted = sub { - my ($dev, $ino, $mode, $nlink, $uid, $gid); - (($dev, $ino, $mode, $nlink, $uid, $gid) = lstat($_)) - && -f _ - && /^.*\.[ch]\z/s - && push(@files, $File::Find::name); -}; - # any non-option arguments are files or directories to be processed -File::Find::find({ wanted => $wanted }, @ARGV) if @ARGV; +push(@files, discover_files(@ARGV)) if @ARGV; # commit file locations are relative to the source root chdir "$sourcedir/../../.." if @commits && $sourcedir; base-commit: effaa464afd355e8927bf430cfe6a0ddd2ee5695 -- 2.53.0 [text/x-patch] v4-0002-pgindent-Default-to-indenting-the-current-directo.patch (1.1K, 3-v4-0002-pgindent-Default-to-indenting-the-current-directo.patch) download | inline diff: From d3d4a52c66773529b10bc826568df78a2cba91b4 Mon Sep 17 00:00:00 2001 From: Jelte Fennema-Nio <[email protected]> Date: Tue, 3 Mar 2026 20:56:15 +0100 Subject: [PATCH v4 2/7] pgindent: Default to indenting the current directory if no files are given Previously running pgindent without giving it any files would result in this output: src/tools/pgindent/pgindent No files to process at src/tools/pgindent/pgindent line 526. This instead indents the current directory, which is probably what the user intended. --- src/tools/pgindent/pgindent | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/tools/pgindent/pgindent b/src/tools/pgindent/pgindent index f5bc4b3642a..e2e6f19678a 100755 --- a/src/tools/pgindent/pgindent +++ b/src/tools/pgindent/pgindent @@ -63,6 +63,9 @@ usage() if $help; usage("Cannot use --commit with command line file list") if (@commits && @ARGV); +# default to current directory if no files/dirs given +@ARGV = ('.') unless @ARGV || @commits; + # command line option wins, then environment, then locations based on current # dir, then default location $typedefs_file ||= $ENV{PGTYPEDEFS}; -- 2.53.0 [text/x-patch] v4-0003-pgindent-Allow-parallel-pgindent-runs.patch (7.1K, 4-v4-0003-pgindent-Allow-parallel-pgindent-runs.patch) download | inline diff: From 1924e5dbd9a88ce0238814ec4e5a67e6f2eea02d Mon Sep 17 00:00:00 2001 From: Jelte Fennema-Nio <[email protected]> Date: Wed, 4 Mar 2026 09:24:24 +0100 Subject: [PATCH v4 3/7] pgindent: Allow parallel pgindent runs Running pgindent on the whole source tree can take a while. This adds support for pgindent to indent files in parallel. This speeds up a full pgindent run from ~7 seconds to 1 second on my machine. Especially with future commits that integrate perltidy into pgindent the wins are huge, because perltidy is much slower at formatting than pg_bsd_indent. With those later commits the time it takes to do a full pgindent run (including perltidy) takes more than a minute on my machine without the parallelization, but only take ~7 seconds when run in parallel. --- src/tools/pgindent/pgindent | 181 +++++++++++++++++++++++++----------- 1 file changed, 129 insertions(+), 52 deletions(-) diff --git a/src/tools/pgindent/pgindent b/src/tools/pgindent/pgindent index e2e6f19678a..ca56758ffac 100755 --- a/src/tools/pgindent/pgindent +++ b/src/tools/pgindent/pgindent @@ -17,6 +17,7 @@ use File::Spec; use File::Temp; use IO::Handle; use Getopt::Long; +use Fcntl qw(:flock); # By default Perl's SIGINT/SIGTERM kill the process without running # END blocks, so File::Temp never gets to clean up. Converting the @@ -42,10 +43,11 @@ my $indent_opts = my $devnull = File::Spec->devnull; -my ($typedefs_file, $typedef_str, @excludes, $indent, $diff, - $check, $help, @commits,); +my ($typedefs_file, $typedef_str, @excludes, $indent, + $diff, $check, $help, @commits, $jobs,); $help = 0; +$jobs = 0; my %options = ( "help" => \$help, @@ -55,9 +57,20 @@ my %options = ( "excludes=s" => \@excludes, "indent=s" => \$indent, "diff" => \$diff, - "check" => \$check,); + "check" => \$check, + "jobs|j=i" => \$jobs,); GetOptions(%options) || usage("bad command line argument"); +if ($jobs == 0) +{ + $jobs = get_num_cpus(); +} +elsif ($jobs < 0) +{ + usage("--jobs must be a non-negative number"); +} + + usage() if $help; usage("Cannot use --commit with command line file list") @@ -103,6 +116,19 @@ my %excluded = map { +"$_\n" => 1 } qw( # globals my @files; my $filtered_typedefs_fh; +my $stdout_lock_fh; + +sub get_num_cpus +{ + # Try nproc (Linux, some BSDs), then sysctl (macOS, FreeBSD). + for my $cmd ('nproc', 'sysctl -n hw.ncpu') + { + my $n = `$cmd 2>$devnull`; + chomp $n; + return $n + 0 if ($? == 0 && $n =~ /^\d+$/ && $n > 0); + } + return 1; +} sub check_indent { @@ -383,6 +409,67 @@ sub discover_files return @discovered; } +sub process_file +{ + my $source_filename = shift; + + # ignore anything that's not a .c or .h file + return 0 unless $source_filename =~ /\.[ch]$/; + + # don't try to indent a file that doesn't exist + unless (-f $source_filename) + { + warn "Could not find $source_filename"; + return 0; + } + # Automatically ignore .c and .h files that correspond to a .y or .l + # file. indent tends to get badly confused by Bison/flex output, + # and there's no value in indenting derived files anyway. + my $otherfile = $source_filename; + $otherfile =~ s/\.[ch]$/.y/; + return 0 if $otherfile ne $source_filename && -f $otherfile; + $otherfile =~ s/\.y$/.l/; + return 0 if $otherfile ne $source_filename && -f $otherfile; + + my $source = read_source($source_filename); + my $orig_source = $source; + my $error_message = ''; + + $source = pre_indent($source); + + $source = run_indent($source, \$error_message); + if ($source eq "") + { + print STDERR "Failure in $source_filename: " . $error_message . "\n"; + return 3; + } + + $source = post_indent($source); + + if ($source ne $orig_source) + { + if (!$diff && !$check) + { + write_source($source, $source_filename); + } + else + { + if ($diff) + { + my $output = diff($source, $source_filename); + flock($stdout_lock_fh, LOCK_EX) or die "flock: $!"; + print $output; + STDOUT->flush(); + flock($stdout_lock_fh, LOCK_UN) or die "flock: $!"; + } + + return 2 if $check; + } + } + + return 0; +} + sub usage { my $message = shift; @@ -398,6 +485,7 @@ Options: --indent=PATH path to pg_bsd_indent program --diff show the changes that would be made --check exit with status 2 if any changes would be made + --jobs=N, -j N number of parallel workers (0 = num CPUs, default 0) The --excludes and --commit options can be given more than once. EOF if ($help) @@ -439,67 +527,56 @@ warn "No files to process" unless @files; # remove excluded files from the file list process_exclude(); -my %processed; -my $status = 0; +# Used by forked children to serialize diff output to STDOUT via flock(). +$stdout_lock_fh = new File::Temp(TEMPLATE => "pglockXXXXX"); -foreach my $source_filename (@files) -{ - # skip duplicates - next if $processed{$source_filename}; - $processed{$source_filename} = 1; +# deduplicate file list +my %seen; +@files = grep { !$seen{$_}++ } @files; - # ignore anything that's not a .c or .h file - next unless $source_filename =~ /\.[ch]$/; +my $status = 0; - # don't try to indent a file that doesn't exist - unless (-f $source_filename) +if ($jobs <= 1) +{ + foreach my $source_filename (@files) { - warn "Could not find $source_filename"; - next; + my $file_status = process_file($source_filename); + $status = $file_status if $file_status > $status; + last if $check && $status >= 2 && !$diff; } - # Automatically ignore .c and .h files that correspond to a .y or .l - # file. indent tends to get badly confused by Bison/flex output, - # and there's no value in indenting derived files anyway. - my $otherfile = $source_filename; - $otherfile =~ s/\.[ch]$/.y/; - next if $otherfile ne $source_filename && -f $otherfile; - $otherfile =~ s/\.y$/.l/; - next if $otherfile ne $source_filename && -f $otherfile; - - my $source = read_source($source_filename); - my $orig_source = $source; - my $error_message = ''; - - $source = pre_indent($source); +} +else +{ + my %children; # pid => 1 - $source = run_indent($source, \$error_message); - if ($source eq "") + my $file_idx = 0; + while ($file_idx < scalar(@files) || %children) { - print STDERR "Failure in $source_filename: " . $error_message . "\n"; - $status = 3; - next; - } + # Fork new children up to $jobs limit + while ($file_idx < scalar(@files) && scalar(keys %children) < $jobs) + { + my $source_filename = $files[ $file_idx++ ]; - $source = post_indent($source); + my $pid = fork(); + die "fork failed: $!\n" unless defined $pid; - if ($source ne $orig_source) - { - if (!$diff && !$check) - { - write_source($source, $source_filename); - } - else - { - if ($diff) + if ($pid == 0) { - print diff($source, $source_filename); + # child + my $child_status = process_file($source_filename); + exit $child_status; } - if ($check) - { - $status ||= 2; - last unless $diff; - } + $children{$pid} = 1; + } + + # Wait for at least one child to finish + my $pid = waitpid(-1, 0); + if ($pid > 0 && exists $children{$pid}) + { + delete $children{$pid}; + my $child_status = $? >> 8; + $status = $child_status if $child_status > $status; } } } -- 2.53.0 [text/x-patch] v4-0004-pgindent-Try-to-find-pg_bsd_indent-binary-in-comm.patch (2.1K, 5-v4-0004-pgindent-Try-to-find-pg_bsd_indent-binary-in-comm.patch) download | inline diff: From b5155b27a1ee35289183d9021941db3be741d664 Mon Sep 17 00:00:00 2001 From: Jelte Fennema-Nio <[email protected]> Date: Tue, 3 Mar 2026 23:46:52 +0100 Subject: [PATCH v4 4/7] pgindent: Try to find pg_bsd_indent binary in common locations To run pgindent you need to to have the right version of pg_bsd_indent in your PATH, or specify it manually. This is a bit of a hassle, especially for newcomers or when working on backbranches. So this chnages pgindent to search for a pg_bsd_indent that's built from the current sources, both in-tree (for in-tree autoconf builds) and in a build directory (for meson or autoconf vpath builds). --- src/tools/pgindent/pgindent | 28 +++++++++++++++++++++++++--- 1 file changed, 25 insertions(+), 3 deletions(-) diff --git a/src/tools/pgindent/pgindent b/src/tools/pgindent/pgindent index ca56758ffac..27ebdbdbf25 100755 --- a/src/tools/pgindent/pgindent +++ b/src/tools/pgindent/pgindent @@ -83,11 +83,33 @@ usage("Cannot use --commit with command line file list") # dir, then default location $typedefs_file ||= $ENV{PGTYPEDEFS}; -# get indent location for environment or default -$indent ||= $ENV{PGINDENT} || $ENV{INDENT} || "pg_bsd_indent"; - my $sourcedir = locate_sourcedir(); +# get indent location: command line wins, then environment, then try to find +# a compiled pg_bsd_indent in the source tree, then fall back to PATH. +$indent ||= $ENV{PGINDENT} || $ENV{INDENT}; +if (!$indent && $sourcedir) +{ + my $srcroot = "$sourcedir/../../.."; + my $bsd_indent_subdir = "src/tools/pg_bsd_indent/pg_bsd_indent"; + + # Look for pg_bsd_indent: first in-tree (autoconf in-tree build), + # then in a "build" directory (meson or autoconf vpath), + # then any "build*" directory. + foreach my $candidate ( + "$srcroot/$bsd_indent_subdir", + "$srcroot/build/$bsd_indent_subdir", + glob("$srcroot/build*/$bsd_indent_subdir")) + { + if (-x $candidate) + { + $indent = $candidate; + last; + } + } +} +$indent ||= "pg_bsd_indent"; + # if it's the base of a postgres tree, we will exclude the files # postgres wants excluded if ($sourcedir) -- 2.53.0 [text/x-patch] v4-0005-Move-pgindent-to-separate-pgcheck-script.patch (1.3K, 6-v4-0005-Move-pgindent-to-separate-pgcheck-script.patch) download | inline diff: From 9fa01defbb6c922612c3a6eb1142206bbf255256 Mon Sep 17 00:00:00 2001 From: Jelte Fennema-Nio <[email protected]> Date: Mon, 30 Mar 2026 23:16:32 +0200 Subject: [PATCH v4 5/7] Move pgindent to separate pgcheck script Over time our pgindent script has gotten a lot of quality of life features, like the --commit, --diff, --check, and --jobs flags. Our pgperltidy, pgperlcritic and pgperlsyncheck are missing these features. In the next commit these other tools will gain these quality of life features too, by introducing a new tool called "pgcheck" that combines the functionality of all tools. This commit only does: git mv src/tools/pgindent/pgindent src/tools/pgcheck/pgcheck The resulting pgcheck script is not even functional, since some of the hardcoded relative paths in the script are now incorrect. The only reason this is separate from the next commit, is to to make git understand that we moved the pgindent file, so the git history of the pgindent file is transferred over correctly to the pgcheck file. --- src/tools/{pgindent/pgindent => pgcheck/pgcheck} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename src/tools/{pgindent/pgindent => pgcheck/pgcheck} (100%) diff --git a/src/tools/pgindent/pgindent b/src/tools/pgcheck/pgcheck similarity index 100% rename from src/tools/pgindent/pgindent rename to src/tools/pgcheck/pgcheck -- 2.53.0 [text/x-patch] v4-0006-Combine-all-formatting-and-linting-tools-into-pgc.patch (20.0K, 7-v4-0006-Combine-all-formatting-and-linting-tools-into-pgc.patch) download | inline diff: From 35c2318a9205a4a5c5e380af625c939186521647 Mon Sep 17 00:00:00 2001 From: Jelte Fennema-Nio <[email protected]> Date: Mon, 30 Mar 2026 23:31:03 +0200 Subject: [PATCH v4 6/7] Combine all formatting and linting tools into pgcheck Over time our pgindent script has gotten a lot of quality of life features, like the --commit, --diff, --check, and --jobs flags. Our pgperltidy, pgperlcritic and pgperlsyncheck scripts were missing all these features. This commit introduces a new tool called "pgcheck" that combines the functionality of all these tools, so all of them can benefit from the improvements that have been made to pgindent. The old scripts are still available as simple wrappers around pgcheck. --- .editorconfig | 6 + .gitattributes | 3 + src/tools/perlcheck/pgperlcritic | 20 +- src/tools/perlcheck/pgperlsyncheck | 16 +- src/tools/pgcheck/README | 21 ++ src/tools/pgcheck/pgcheck | 387 ++++++++++++++++++++++++++--- src/tools/pgindent/pgindent | 8 + src/tools/pgindent/pgperltidy | 18 +- 8 files changed, 397 insertions(+), 82 deletions(-) create mode 100644 src/tools/pgcheck/README create mode 100755 src/tools/pgindent/pgindent diff --git a/.editorconfig b/.editorconfig index 0ee9bd28ac4..4fa6fd1aadb 100644 --- a/.editorconfig +++ b/.editorconfig @@ -67,6 +67,12 @@ indent_style = space tab_width = unset indent_size = 1 +[src/tools/pgcheck/pgcheck] +trim_trailing_whitespace = true +insert_final_newline = true +indent_style = tab +tab_width = 4 + [*.data] indent_style = unset indent_size = unset diff --git a/.gitattributes b/.gitattributes index 00092168393..8e141e960fd 100644 --- a/.gitattributes +++ b/.gitattributes @@ -15,6 +15,9 @@ README conflict-marker-size=48 README.* conflict-marker-size=48 +# Some files are perl even without perly extensions +src/tools/pgcheck/pgcheck whitespace=space-before-tab,trailing-space,indent-with-non-tab,tabwidth=4 + # Certain data files that contain special whitespace, and other special cases *.data -whitespace contrib/pgcrypto/sql/pgp-armor.sql whitespace=-blank-at-eol diff --git a/src/tools/perlcheck/pgperlcritic b/src/tools/perlcheck/pgperlcritic index 2ec6f20de31..0220052acf0 100755 --- a/src/tools/perlcheck/pgperlcritic +++ b/src/tools/perlcheck/pgperlcritic @@ -1,20 +1,8 @@ #!/bin/sh -# src/tools/perlcheck/pgperlcritic +# Copyright (c) 2021-2026, PostgreSQL Global Development Group -test -f src/tools/perlcheck/perlcriticrc || { - echo could not find src/tools/perlcheck/perlcriticrc - exit 1 - } +# Wrapper that runs pgcheck in perlcritic-only mode. +# See src/tools/pgcheck/pgcheck for the full implementation. -set -e - -# set this to override default perlcritic program: -PERLCRITIC=${PERLCRITIC:-perlcritic} - -. src/tools/perlcheck/find_perl_files - -find_perl_files "$@" | xargs $PERLCRITIC \ - --quiet \ - --program-extensions .pl \ - --profile=src/tools/perlcheck/perlcriticrc +exec "$(dirname "$0")/../pgcheck/pgcheck" --perlcritic "$@" diff --git a/src/tools/perlcheck/pgperlsyncheck b/src/tools/perlcheck/pgperlsyncheck index 657d2afcc02..3928311e13a 100755 --- a/src/tools/perlcheck/pgperlsyncheck +++ b/src/tools/perlcheck/pgperlsyncheck @@ -1,16 +1,8 @@ #!/bin/sh -# script to detect compile time errors and warnings in all perl files +# Copyright (c) 2021-2026, PostgreSQL Global Development Group -INCLUDES="-I src/backend/catalog" -INCLUDES="-I src/test/perl -I src/backend/utils/mb/Unicode $INCLUDES" -INCLUDES="-I src/bin/pg_rewind -I src/test/ssl/t $INCLUDES" +# Wrapper that runs pgcheck in perl-syncheck-only mode. +# See src/tools/pgcheck/pgcheck for the full implementation. -set -e - -. src/tools/perlcheck/find_perl_files - -# for zsh -setopt shwordsplit 2>/dev/null || true - -find_perl_files "$@" | xargs -L 1 perl $INCLUDES -cw 2>&1 | grep -v OK +exec "$(dirname "$0")/../pgcheck/pgcheck" --perl-syncheck "$@" diff --git a/src/tools/pgcheck/README b/src/tools/pgcheck/README new file mode 100644 index 00000000000..47c628f5b44 --- /dev/null +++ b/src/tools/pgcheck/README @@ -0,0 +1,21 @@ +pgcheck +======= + +pgcheck is a unified tool for formatting and checking PostgreSQL's C +and Perl code. It consolidates the functionality of pgindent, +pgperltidy, pgperlcritic, and pgperlsyncheck into a single script. +The individual tool scripts still exist as thin wrappers around pgcheck. + + src/tools/pgcheck/pgcheck + +By default pgcheck runs all tools on the current directory. You can select +specific ones or use a variety of options: + + src/tools/pgcheck/pgcheck --pgindent src contrib + src/tools/pgcheck/pgcheck --perltidy --commit HEAD --check --diff + src/tools/pgcheck/pgcheck --perlcritic --perl-syncheck + +Run pgcheck --help for the full list of options. + +See src/tools/pgindent/README for prerequisites and detailed usage +instructions. diff --git a/src/tools/pgcheck/pgcheck b/src/tools/pgcheck/pgcheck index 27ebdbdbf25..d0586e9ae6c 100755 --- a/src/tools/pgcheck/pgcheck +++ b/src/tools/pgcheck/pgcheck @@ -2,12 +2,21 @@ # Copyright (c) 2021-2026, PostgreSQL Global Development Group -# Program to maintain uniform layout style in our C code. +# Unified tool for formatting and checking PostgreSQL C and Perl code. +# +# Supported tools +# --pgindent format C code with pg_bsd_indent +# --perltidy format Perl code with perltidy +# --perlcritic lint Perl code with perlcritic +# --perl-syncheck syntax-check Perl code with perl -cw +# +# When no tool flags are given, all tools are run. +# # Exit codes: # 0 -- all OK -# 1 -- error invoking pgindent, nothing done +# 1 -- error invoking pgcheck, nothing done # 2 -- --check mode and at least one file requires changes -# 3 -- pg_bsd_indent failed on at least one file +# 3 -- a formatting or checking tool failed on at least one file use strict; use warnings FATAL => 'all'; @@ -44,11 +53,21 @@ my $indent_opts = my $devnull = File::Spec->devnull; my ($typedefs_file, $typedef_str, @excludes, $indent, - $diff, $check, $help, @commits, $jobs,); + $diff, $check, $help, @commits, + $perltidy_arg, $perlcritic_arg, $jobs,); + +# tool selectors +my ($do_pgindent, $do_perltidy, $do_perlcritic, $do_perl_syncheck); $help = 0; $jobs = 0; +# Save @ARGV before parsing so we can distinguish --perltidy=PATH (where +# the value should be used as the perltidy path) from --perltidy PATH +# (where PATH is a file to format that Getopt::Long greedily consumed). +# Same for --perlcritic. +my @orig_argv = @ARGV; + my %options = ( "help" => \$help, "commit=s" => \@commits, @@ -56,6 +75,10 @@ my %options = ( "list-of-typedefs=s" => \$typedef_str, "excludes=s" => \@excludes, "indent=s" => \$indent, + "pgindent" => \$do_pgindent, + "perltidy:s" => \$perltidy_arg, + "perlcritic:s" => \$perlcritic_arg, + "perl-syncheck" => \$do_perl_syncheck, "diff" => \$diff, "check" => \$check, "jobs|j=i" => \$jobs,); @@ -70,6 +93,32 @@ elsif ($jobs < 0) usage("--jobs must be a non-negative number"); } +# For --perltidy and --perlcritic, distinguish =PATH from bare PATH. The PATH +# in =PATH is interpreted as a path to the perltidy/perlcritic binary, but a +# separated PATH is interpreted as a file to be processed. +foreach my $pair ([ \$perltidy_arg, "perltidy" ], + [ \$perlcritic_arg, "perlcritic" ]) +{ + my ($ref, $name) = @$pair; + if (defined($$ref) && $$ref ne '') + { + unless (grep { $_ eq "--$name=$$ref" } @orig_argv) + { + # If it's not =PATH add the argument back to @ARGV for processing + # as a file argument + unshift(@ARGV, $$ref); + # Set the $perltidy_arg/perlcritic_arg to the empty string, to + # indicate for the later code that the tool was selected but no + # path was given. + $$ref = ''; + } + } +} + +# --perltidy and --perlcritic as tool selectors: if given (even +# without a value), they select their respective tool. +$do_perltidy = 1 if defined($perltidy_arg); +$do_perlcritic = 1 if defined($perlcritic_arg); usage() if $help; @@ -79,6 +128,22 @@ usage("Cannot use --commit with command line file list") # default to current directory if no files/dirs given @ARGV = ('.') unless @ARGV || @commits; +# If no tool flags were given, run everything. +my $any_op = + $do_pgindent + || $do_perltidy + || $do_perlcritic + || $do_perl_syncheck; +if (!$any_op) +{ + $do_pgindent = 1; + $do_perltidy = 1; + $do_perlcritic = 1; + $do_perl_syncheck = 1; +} + +my $do_perl_any = $do_perltidy || $do_perlcritic || $do_perl_syncheck; + # command line option wins, then environment, then locations based on current # dir, then default location $typedefs_file ||= $ENV{PGTYPEDEFS}; @@ -110,6 +175,12 @@ if (!$indent && $sourcedir) } $indent ||= "pg_bsd_indent"; +# get perltidy location: command line wins, then environment, then PATH. +my $perltidy = $perltidy_arg || $ENV{PERLTIDY} || "perltidy"; + +# get perlcritic location: command line wins, then environment, then PATH. +my $perlcritic = $perlcritic_arg || $ENV{PERLCRITIC} || "perlcritic"; + # if it's the base of a postgres tree, we will exclude the files # postgres wants excluded if ($sourcedir) @@ -181,6 +252,57 @@ sub check_indent return; } +my $PERLTIDY_VERSION = "20230309"; + +sub check_perltidy +{ + if (!$sourcedir) + { + print STDERR + "Cannot find perltidyrc: not running inside a PostgreSQL source tree.\n"; + exit 1; + } + + my $ver = `$perltidy -v 2>&1`; + if ($? != 0) + { + print STDERR + "You do not appear to have $perltidy installed on your system.\n" + . "See src/tools/pgindent/README for installation instructions.\n"; + exit 1; + } + + if ($ver !~ m/$PERLTIDY_VERSION/) + { + print STDERR + "You do not appear to have $perltidy version $PERLTIDY_VERSION installed on your system.\n" + . "See src/tools/pgindent/README for installation instructions.\n"; + exit 1; + } + + return; +} + +sub check_perlcritic +{ + if (!$sourcedir) + { + print STDERR + "Cannot find perlcriticrc: not running inside a PostgreSQL source tree.\n"; + exit 1; + } + + my $ver = `$perlcritic --version 2>&1`; + if ($? != 0) + { + print STDERR + "You do not appear to have $perlcritic installed on your system.\n"; + exit 1; + } + + return; +} + sub locate_sourcedir { # try fairly hard to locate the sourcedir @@ -372,6 +494,78 @@ sub run_indent return $source; } +sub format_c +{ + my $source = shift; + my $source_filename = shift; + my $error_message = ''; + + my $formatted = pre_indent($source); + $formatted = run_indent($formatted, \$error_message); + if ($formatted eq "") + { + print "Failure in $source_filename: " . $error_message . "\n"; + return ("", 1); + } + return post_indent($formatted); +} + +sub format_perl +{ + my $source = shift; + my $source_filename = shift; + + my $tmp_fh = new File::Temp(TEMPLATE => "pgperltidyXXXXX"); + my $tmp_filename = $tmp_fh->filename; + print $tmp_fh $source; + $tmp_fh->close(); + + my $perltidy_profile = "$sourcedir/perltidyrc"; + my $err = + `$perltidy --profile=$perltidy_profile -b -bext='/' $tmp_filename 2>&1`; + if ($? != 0) + { + print "Failure in $source_filename: " . $err . "\n"; + return ("", 1); + } + return read_source($tmp_filename); +} + +sub run_perlcritic +{ + my $source_filename = shift; + + my $perlcheck_dir = "$sourcedir/../perlcheck"; + my $criticrc = "$perlcheck_dir/perlcriticrc"; + + my $err = + `$perlcritic --quiet --program-extensions .pl --profile=$criticrc "$source_filename" 2>&1`; + return ("", 0) if $? == 0; + return ($err, 1); +} + +sub run_perl_syncheck +{ + my $source_filename = shift; + + my $includes = join( + ' ', + map { "-I $_" } ( + "src/backend/catalog", "src/test/perl", + "src/backend/utils/mb/Unicode", "src/bin/pg_rewind", + "src/test/ssl/t",)); + + my $output = `perl $includes -cw "$source_filename" 2>&1`; + my $failed = $? != 0; + + # Filter out "OK" lines — perl -cw prints "<file> syntax OK" on success + my @lines = grep { !/\bsyntax OK$/ } split(/\n/, $output); + my $filtered = join("\n", @lines); + $filtered .= "\n" if $filtered; + + return ($filtered, $failed || scalar(@lines) > 0); +} + sub diff { my $indented = shift; @@ -388,6 +582,33 @@ sub diff return $diff; } +sub is_c_file +{ + my $filename = shift; + # It needs to have .c or .h extension + return 0 unless $filename =~ /\.[ch]$/; + # Automatically ignore .c and .h files that correspond to a .y or .l file. + # pg_bsd_indent tends to get badly confused by Bison/flex output, and + # there's no value in indenting derived files anyway. + my $otherfile = $filename; + $otherfile =~ s/\.[ch]$/.y/; + return 0 if $otherfile ne $filename && -f $otherfile; + $otherfile =~ s/\.y$/.l/; + return 0 if $otherfile ne $filename && -f $otherfile; + return 1; +} + +sub is_perl_file +{ + my $filename = shift; + # take all .pl and .pm files + return 1 if $filename =~ /\.p[lm]$/; + # take executable files that file(1) thinks are perl files + return 0 unless -x $filename; + my $file_out = `file "$filename"`; + return $file_out =~ /:.*perl[0-9]*\b/i; +} + sub discover_files { my @paths = @_; @@ -425,7 +646,7 @@ sub discover_files close($git_ls); die "git ls-files error" if $?; my @git_files = split(/\0/, $git_output); - push(@discovered, grep { /\.[ch]$/ } @git_files); + push(@discovered, @git_files); } return @discovered; @@ -435,57 +656,111 @@ sub process_file { my $source_filename = shift; - # ignore anything that's not a .c or .h file - return 0 unless $source_filename =~ /\.[ch]$/; - - # don't try to indent a file that doesn't exist + # don't try to format a file that doesn't exist unless (-f $source_filename) { warn "Could not find $source_filename"; return 0; } - # Automatically ignore .c and .h files that correspond to a .y or .l - # file. indent tends to get badly confused by Bison/flex output, - # and there's no value in indenting derived files anyway. - my $otherfile = $source_filename; - $otherfile =~ s/\.[ch]$/.y/; - return 0 if $otherfile ne $source_filename && -f $otherfile; - $otherfile =~ s/\.y$/.l/; - return 0 if $otherfile ne $source_filename && -f $otherfile; - - my $source = read_source($source_filename); - my $orig_source = $source; - my $error_message = ''; - $source = pre_indent($source); + # The more complex filtering is already done by discover_files, at this + # point we can simply check the file extension to determine if it's a C + # file. + my $is_c = $source_filename =~ /\.[ch]$/; + # All other files are Perl files + my $is_perl = !$is_c; - $source = run_indent($source, \$error_message); - if ($source eq "") + # Formatting step: pgindent or perltidy + if ($is_c && $do_pgindent) { - print STDERR "Failure in $source_filename: " . $error_message . "\n"; - return 3; - } + my $source = read_source($source_filename); + my ($formatted, $failure) = format_c($source, $source_filename); - $source = post_indent($source); + if ($failure) + { + return 3; + } - if ($source ne $orig_source) + if ($formatted ne $source) + { + if (!$diff && !$check) + { + write_source($formatted, $source_filename); + } + else + { + if ($diff) + { + my $output = diff($formatted, $source_filename); + flock($stdout_lock_fh, LOCK_EX) or die "flock: $!"; + print $output; + STDOUT->flush(); + flock($stdout_lock_fh, LOCK_UN) or die "flock: $!"; + } + + return 2 if $check; + } + } + } + elsif ($is_perl && $do_perltidy) { - if (!$diff && !$check) + my $source = read_source($source_filename); + my ($formatted, $failure) = format_perl($source, $source_filename); + + if ($failure) { - write_source($source, $source_filename); + return 3; } - else + + if ($formatted ne $source) + { + if (!$diff && !$check) + { + write_source($formatted, $source_filename); + } + else + { + if ($diff) + { + my $output = diff($formatted, $source_filename); + flock($stdout_lock_fh, LOCK_EX) or die "flock: $!"; + print $output; + STDOUT->flush(); + flock($stdout_lock_fh, LOCK_UN) or die "flock: $!"; + } + + return 2 if $check; + } + } + } + + # Validation steps: perlcritic and syncheck (only for perl files) + if ($is_perl) + { + if ($do_perlcritic) { - if ($diff) + my ($output, $failed) = run_perlcritic($source_filename); + if ($failed) { - my $output = diff($source, $source_filename); flock($stdout_lock_fh, LOCK_EX) or die "flock: $!"; print $output; STDOUT->flush(); flock($stdout_lock_fh, LOCK_UN) or die "flock: $!"; + return 3; } + } - return 2 if $check; + if ($do_perl_syncheck) + { + my ($output, $failed) = run_perl_syncheck($source_filename); + if ($failed) + { + flock($stdout_lock_fh, LOCK_EX) or die "flock: $!"; + print $output; + STDOUT->flush(); + flock($stdout_lock_fh, LOCK_UN) or die "flock: $!"; + return 3; + } } } @@ -497,9 +772,13 @@ sub usage my $message = shift; my $helptext = <<'EOF'; Usage: -pgindent [OPTION]... [FILE|DIR]... +pgcheck [OPTION]... [FILE|DIR]... Options: --help show this message and quit + --pgindent format C code with pg_bsd_indent + --perltidy[=PATH] format Perl code with perltidy + --perlcritic[=PATH] lint Perl code with perlcritic + --perl-syncheck syntax-check Perl code with perl -cw --commit=gitref use files modified by the named commit --typedefs=FILE file containing a list of typedefs --list-of-typedefs=STR string containing typedefs, space separated @@ -508,6 +787,8 @@ Options: --diff show the changes that would be made --check exit with status 2 if any changes would be made --jobs=N, -j N number of parallel workers (0 = num CPUs, default 0) +When no --pgindent, --perltidy, --perlcritic, or --perl-syncheck flags are +provided, all of these tools are run. The --excludes and --commit options can be given more than once. EOF if ($help) @@ -524,12 +805,37 @@ EOF # main -$filtered_typedefs_fh = load_typedefs(); +if ($do_pgindent) +{ + $filtered_typedefs_fh = load_typedefs(); + check_indent(); +} -check_indent(); +if ($do_perltidy) +{ + check_perltidy(); +} + +if ($do_perlcritic) +{ + check_perlcritic(); +} # any non-option arguments are files or directories to be processed -push(@files, discover_files(@ARGV)) if @ARGV; +if (@ARGV) +{ + foreach my $f (discover_files(@ARGV)) + { + if ($do_pgindent && is_c_file($f)) + { + push(@files, $f); + } + elsif ($do_perl_any && is_perl_file($f)) + { + push(@files, $f); + } + } +} # commit file locations are relative to the source root chdir "$sourcedir/../../.." if @commits && $sourcedir; @@ -541,7 +847,8 @@ foreach my $commit (@commits) my @affected = `git diff --diff-filter=ACMR --name-only $prev $commit`; die "git error" if $?; chomp(@affected); - push(@files, @affected); + push(@files, grep { is_c_file($_) } @affected) if $do_pgindent; + push(@files, grep { is_perl_file($_) } @affected) if $do_perl_any; } warn "No files to process" unless @files; diff --git a/src/tools/pgindent/pgindent b/src/tools/pgindent/pgindent new file mode 100755 index 00000000000..f1553075030 --- /dev/null +++ b/src/tools/pgindent/pgindent @@ -0,0 +1,8 @@ +#!/bin/sh + +# Copyright (c) 2021-2026, PostgreSQL Global Development Group + +# Wrapper that runs pgcheck in pgindent-only mode. +# See src/tools/pgcheck/pgcheck for the full implementation. + +exec "$(dirname "$0")/../pgcheck/pgcheck" --pgindent "$@" diff --git a/src/tools/pgindent/pgperltidy b/src/tools/pgindent/pgperltidy index 87838d6bde3..72273f50c9e 100755 --- a/src/tools/pgindent/pgperltidy +++ b/src/tools/pgindent/pgperltidy @@ -1,18 +1,8 @@ #!/bin/sh -# src/tools/pgindent/pgperltidy +# Copyright (c) 2021-2026, PostgreSQL Global Development Group -set -e +# Wrapper that runs pgcheck in perltidy-only mode. +# See src/tools/pgcheck/pgcheck for the full implementation. -# set this to override default perltidy program: -PERLTIDY=${PERLTIDY:-perltidy} - -PERLTIDY_VERSION=20230309 -if ! $PERLTIDY -v | grep -q $PERLTIDY_VERSION; then - echo "You do not appear to have $PERLTIDY version $PERLTIDY_VERSION installed on your system." >&2 - exit 1 -fi - -. src/tools/perlcheck/find_perl_files - -find_perl_files "$@" | xargs $PERLTIDY --profile=src/tools/pgindent/perltidyrc +exec "$(dirname "$0")/../pgcheck/pgcheck" --perltidy "$@" -- 2.53.0 [text/x-patch] v4-0007-pgcheck-Add-easy-way-of-getting-perltidy.patch (5.5K, 8-v4-0007-pgcheck-Add-easy-way-of-getting-perltidy.patch) download | inline diff: From c65e6330ec14ea1a05b027b29269dcd5331e336c Mon Sep 17 00:00:00 2001 From: Jelte Fennema-Nio <[email protected]> Date: Wed, 4 Mar 2026 09:04:15 +0100 Subject: [PATCH v4 7/7] pgcheck: Add easy way of getting perltidy pgcheck needs a very specific perltidy version, but getting that installed is quite a hassle. Especially for people who don't use perl on a daily basis. This commit adds a small script to download the exact perltidy version that we need and install it locally in the repo. It also teaches pgcheck to use that locally installed perltidy if it is available. --- src/tools/pgcheck/pgcheck | 25 +++++++++++-- src/tools/pgindent/.gitignore | 1 + src/tools/pgindent/README | 16 ++++----- src/tools/pgindent/get_perltidy | 62 +++++++++++++++++++++++++++++++++ 4 files changed, 94 insertions(+), 10 deletions(-) create mode 100644 src/tools/pgindent/.gitignore create mode 100755 src/tools/pgindent/get_perltidy diff --git a/src/tools/pgcheck/pgcheck b/src/tools/pgcheck/pgcheck index d0586e9ae6c..f9fef61195a 100755 --- a/src/tools/pgcheck/pgcheck +++ b/src/tools/pgcheck/pgcheck @@ -175,8 +175,29 @@ if (!$indent && $sourcedir) } $indent ||= "pg_bsd_indent"; -# get perltidy location: command line wins, then environment, then PATH. -my $perltidy = $perltidy_arg || $ENV{PERLTIDY} || "perltidy"; +# get perltidy location: command line wins, then environment, then try to +# find a get_perltidy-installed copy in the source tree, then fall back to +# PATH. +my $perltidy = $perltidy_arg || $ENV{PERLTIDY}; +if (!$perltidy && $sourcedir) +{ + # Look for a get_perltidy-installed perltidy in the source tree. + my $candidate = "$sourcedir/perltidy/bin/perltidy"; + if (-x $candidate) + { + $perltidy = $candidate; + + # Local installs need PERL5LIB set so perltidy can find its + # modules. + my $libdir = "$sourcedir/perltidy/lib/perl5"; + if (-d $libdir) + { + $ENV{PERL5LIB} = + $ENV{PERL5LIB} ? "$libdir:$ENV{PERL5LIB}" : $libdir; + } + } +} +$perltidy ||= "perltidy"; # get perlcritic location: command line wins, then environment, then PATH. my $perlcritic = $perlcritic_arg || $ENV{PERLCRITIC} || "perlcritic"; diff --git a/src/tools/pgindent/.gitignore b/src/tools/pgindent/.gitignore new file mode 100644 index 00000000000..1497217113b --- /dev/null +++ b/src/tools/pgindent/.gitignore @@ -0,0 +1 @@ +/perltidy/ diff --git a/src/tools/pgindent/README b/src/tools/pgindent/README index b6cd4c6f6b7..ebc18d83e9c 100644 --- a/src/tools/pgindent/README +++ b/src/tools/pgindent/README @@ -17,14 +17,14 @@ PREREQUISITES: 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-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-20230309.tar.gz + + The easiest way is to use the get_perltidy script, which downloads, + verifies, and installs the correct version into the source tree: + + src/tools/pgindent/get_perltidy + + This installs perltidy into src/tools/pgindent/perltidy/, which + pgindent will find automatically. DOING THE INDENT RUN BEFORE A NORMAL COMMIT: diff --git a/src/tools/pgindent/get_perltidy b/src/tools/pgindent/get_perltidy new file mode 100755 index 00000000000..09ce4a195cb --- /dev/null +++ b/src/tools/pgindent/get_perltidy @@ -0,0 +1,62 @@ +#!/bin/sh + +# src/tools/pgindent/get_perltidy +# +# Downloads and installs perltidy 20230309 into src/tools/pgindent/perltidy/. + +set -e + +PERLTIDY_VERSION=20230309 +PERLTIDY_TARBALL="Perl-Tidy-${PERLTIDY_VERSION}.tar.gz" +PERLTIDY_URL="https://cpan.metacpan.org/authors/id/S/SH/SHANCOCK/${PERLTIDY_TARBALL}" +PERLTIDY_SHA256="e22949a208c618d671a18c5829b451abbe9da0da2cddd78fdbfcb036c7361c18" + +# Determine the directory this script lives in, i.e. src/tools/pgindent +SCRIPT_DIR=$(cd "$(dirname "$0")" && pwd) + +PERLTIDY_DIR="$SCRIPT_DIR/perltidy" +PERLTIDY_BIN="$PERLTIDY_DIR/bin/perltidy" + +# Check if already installed with the correct version +if [ -x "$PERLTIDY_BIN" ]; then + perltidy_lib="$PERLTIDY_DIR/lib/perl5" + installed_version=$(PERL5LIB="$perltidy_lib${PERL5LIB:+:$PERL5LIB}" "$PERLTIDY_BIN" -v 2>/dev/null || true) + if echo "$installed_version" | grep -q "$PERLTIDY_VERSION"; then + echo "perltidy $PERLTIDY_VERSION is already installed in $PERLTIDY_DIR" + exit 0 + fi +fi + +WORK_DIR=$(mktemp -d) +trap 'rm -rf "$WORK_DIR"' EXIT + +# Download +TARBALL="$WORK_DIR/$PERLTIDY_TARBALL" +if command -v curl >/dev/null 2>&1; then + curl -sSL -o "$TARBALL" "$PERLTIDY_URL" +elif command -v wget >/dev/null 2>&1; then + wget -q -O "$TARBALL" "$PERLTIDY_URL" +else + echo "error: neither curl nor wget found" >&2 + exit 1 +fi + +# Verify SHA256 +if command -v sha256sum >/dev/null 2>&1; then + echo "$PERLTIDY_SHA256 $TARBALL" | sha256sum -c - >/dev/null +elif command -v shasum >/dev/null 2>&1; then + echo "$PERLTIDY_SHA256 $TARBALL" | shasum -a 256 -c - >/dev/null +else + echo "error: neither sha256sum nor shasum found" >&2 + exit 1 +fi + +# Extract, build, install +cd "$WORK_DIR" +tar xzf "$TARBALL" +cd "Perl-Tidy-${PERLTIDY_VERSION}" +perl Makefile.PL INSTALL_BASE="$PERLTIDY_DIR" >/dev/null +make >/dev/null +make install >/dev/null + +echo "perltidy $PERLTIDY_VERSION installed in $PERLTIDY_DIR" -- 2.53.0 ^ permalink raw reply [nested|flat] 15+ messages in thread
end of thread, other threads:[~2026-04-03 07:52 UTC | newest] Thread overview: 15+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2025-12-31 11:35 Add "format" target to make and ninja to run pgindent and pgperltidy Jelte Fennema-Nio <[email protected]> 2025-12-31 13:54 ` Ashutosh Bapat <[email protected]> 2025-12-31 15:26 ` Tom Lane <[email protected]> 2025-12-31 15:48 ` Andrew Dunstan <[email protected]> 2025-12-31 15:54 ` Tom Lane <[email protected]> 2025-12-31 16:13 ` Andrew Dunstan <[email protected]> 2026-03-04 09:18 ` Jelte Fennema-Nio <[email protected]> 2026-03-12 09:10 ` Peter Eisentraut <[email protected]> 2026-03-13 08:29 ` Jelte Fennema-Nio <[email protected]> 2026-03-13 15:11 ` Jelte Fennema-Nio <[email protected]> 2026-03-13 22:26 ` Daniel Gustafsson <[email protected]> 2026-03-27 15:28 ` Jelte Fennema-Nio <[email protected]> 2026-04-03 07:52 ` Jelte Fennema-Nio <[email protected]> 2025-12-31 18:17 ` Jelte Fennema-Nio <[email protected]> 2025-12-31 18:37 ` Tom Lane <[email protected]>
This inbox is served by agora; see mirroring instructions for how to clone and mirror all data and code used for this inbox