Horton

Thread

Commits

  1. Fix inconsistent quoting for some options in TAP tests

  2. Apply more consistent style for command options in TAP tests

  1. More Perl cleanups

    Dagfinn Ilmari Mannsåker <ilmari@ilmari.org> — 2025-03-14T17:39:30Z

    Hi Hackers,

In some recent-ish commits (ce1b0f9d, fd4c4ede and cc2c9fa6) we
converted a lot of the TAP tests to use long command line options and
fat commas (=>) to separate command line options from their arguments,
so that perltidy does't break lines between them.

However, those patches were nowhere near complete, so here's a follow-up
to fix all the cases I could find.  While I was there I failed to resist
the urge to do some other minor tidy-ups that I think make things
neater, but perltidy has no opinion on. I also eliminated some variables
that were only initialised and then used once.

The second patch might be more controversial: it eliminates unnecessary
quoting from hash keys, both inside curly braces and before fat commas
(except where a hash has a mix of keys that need quoting and not).

- ilmari

  2. Re: More Perl cleanups

    Michael Paquier <michael@paquier.xyz> — 2025-03-15T06:26:10Z

    On Fri, Mar 14, 2025 at 05:39:30PM +0000, Dagfinn Ilmari Mannsåker wrote:
> In some recent-ish commits (ce1b0f9d, fd4c4ede and cc2c9fa6) we
> converted a lot of the TAP tests to use long command line options and
> fat commas (=>) to separate command line options from their arguments,
> so that perltidy does't break lines between them.
> 
> However, those patches were nowhere near complete, so here's a follow-up
> to fix all the cases I could find.  While I was there I failed to resist
> the urge to do some other minor tidy-ups that I think make things
> neater, but perltidy has no opinion on. I also eliminated some variables
> that were only initialised and then used once.

Thanks for digging into all that.  Agreed that it would be nice to
apply a more consistent style for this release.  I'll look more into
what you have here.

> The second patch might be more controversial: it eliminates unnecessary
> quoting from hash keys, both inside curly braces and before fat commas
> (except where a hash has a mix of keys that need quoting and not).

Hmm.  I can partially get behind this one, seeing things like that in
what you have sent:

@@ -24,8 +24,8 @@ my $node = PostgreSQL::Test::Cluster->new('primary');
 # Make sure pg_hba.conf is set up to allow connections from backupuser.
 # This is only needed on Windows machines that don't use UNIX sockets.
 $node->init(
-	'allows_streaming' => 1,
-	'auth_extra' => [ '--create-role' => 'backupuser' ]);
+	allows_streaming => 1,
+	auth_extra => [ '--create-role' => 'backupuser' ]);

This option handling style is inconsistent in the tree.  Most of the
time these keywords are not quoted when it comes to our internal test
modules..
--
Michael

  3. Re: More Perl cleanups

    Michael Paquier <michael@paquier.xyz> — 2025-03-17T05:11:30Z

    On Sat, Mar 15, 2025 at 03:26:10PM +0900, Michael Paquier wrote:
> Thanks for digging into all that.  Agreed that it would be nice to
> apply a more consistent style for this release.  I'll look more into
> what you have here.

The changes in pg_dump's 010_dump_connstr.pl read the same.

extension_schema in test_pg_dump adds silently a --no-sync.

> Hmm.  I can partially get behind this one, seeing things like that in
> what you have sent:
> 
> @@ -24,8 +24,8 @@ my $node = PostgreSQL::Test::Cluster->new('primary');
>  # Make sure pg_hba.conf is set up to allow connections from backupuser.
>  # This is only needed on Windows machines that don't use UNIX sockets.
>  $node->init(
> -	'allows_streaming' => 1,
> -	'auth_extra' => [ '--create-role' => 'backupuser' ]);
> +	allows_streaming => 1,
> +	auth_extra => [ '--create-role' => 'backupuser' ]);
> 
> This option handling style is inconsistent in the tree.  Most of the
> time these keywords are not quoted when it comes to our internal test
> modules..

So even for the option handling, what we have here is a mixed bad of
inconsistencies and rather consistent-ish behaviors.

Some notes:
src/bin/pg_basebackup/t/010_pg_basebackup.pl quotes tablespace_map.
src/bin/pg_basebackup/t/011_in_place_tablespace.pl quotes allows_streaming.
slot_type and restart_lsn are quoted everywhere now.
src/bin/pg_combinebackup/t/008_promote.pl quotes has_streaming.
ENV is a mixed bag of various things, single, double or no quotes.
pg_verifybackup/t/003_corruption.pl, 008, 009, 010 are a set of local
changes.
001_uri.pl was inconsistent, still local.
The parts about auth_extra are inconsistent (like in
002_connection_limits.pl).
Inconsistency in BackgroundPsql.pm, but it's always quoted now.
021_row_visibility.pl and 032_relfilenode_reuse.pl are local.  Hm, not
planning to bother here.
Okay for 003_sslinfo.pl and 002_scram.pl, that mix both styles,
unquoting looks fine.
Not sure about the build scripts as a whole.

One point that applies for sure is that some existing options use an
inconsistent style across the tree in the TAP tests, mainly for init()
and init_from_backup().  I've extracted these from 0002, and applied
the subset.  For the rest, if there are more opinions, feel free..
--
Michael