Feedback-ID: i1ff147bf:Fastmail
From: =?utf-8?Q?Dagfinn_Ilmari_Manns=C3=A5ker?= <ilmari@ilmari.org>
To: Michael Paquier <michael@paquier.xyz>
Cc: Peter Smith <smithpb2250@gmail.com>,  Andrew Dunstan
 <andrew@dunslane.net>,  Tom Lane <tgl@sss.pgh.pa.us>,  PostgreSQL Hackers
 <pgsql-hackers@lists.postgresql.org>
Subject: Re: pg_createsubscriber TAP test wrapping makes command options
 hard to read.
References: <87pllx5cqn.fsf@wibble.ilmari.org>
	<87msh159c3.fsf@wibble.ilmari.org>
	<0f5256ec-d63e-4e0f-9b71-750a7a4e5c01@dunslane.net>
	<651128.1734017715@sss.pgh.pa.us> <87jzc46d8u.fsf@wibble.ilmari.org>
	<5f41d6c6-1cbb-494d-a5c4-3c449fc4e9ad@dunslane.net>
	<105f432a-6b0b-42d4-9061-65d263279be7@app.fastmail.com>
	<CAHut+PuD0aQfjAN0q_7FpdNdSEdrhONpTsb0tQi5ZEtZHj6bKg@mail.gmail.com>
	<87ldv542fk.fsf@wibble.ilmari.org>
	<CAHut+PvKrQXY7kM8ct=HC9xYPC6yhdJWnvCgijtgxmO-mus8uQ@mail.gmail.com>
	<Z49PYgXnbyU6IiEe@paquier.xyz>
Date: Tue, 21 Jan 2025 14:17:03 +0000
In-Reply-To: <Z49PYgXnbyU6IiEe@paquier.xyz> (Michael Paquier's message of
	"Tue, 21 Jan 2025 16:40:18 +0900")
Message-ID: <87frlc45gg.fsf@wibble.ilmari.org>
User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/27.1 (gnu/linux)
MIME-Version: 1.0
Content-Type: text/plain
Archived-At: <https://www.postgresql.org/message-id/87frlc45gg.fsf%40wibble.ilmari.org>
Precedence: bulk

Michael Paquier <michael@paquier.xyz> writes:

> On Tue, Jan 21, 2025 at 10:31:01AM +1100, Peter Smith wrote:
>> I applied the v5* patches and ran make check-world. All passed OK.
>> 
>> Your fat comma changes have greatly improved readability, particularly
>> in 040_createsubscriber (the file that caused me to start this
>> thread). Thanks!
>
> It is not an improvement in only 040_createsubscriber, all the places
> Dagfinn is touching here feel much better with this new syntax.

Thanks, I'm glad you agree.

> You have spent a lot of time on the change from short to long names
> and on the new convention grammar for the option/value duos, clearly.

I did it by grepping for the various functions that call programs, such
as command_((fails_)?like|ok) etc, and then reading the docs for each
command to find the corresponding long options.  It got almost
meditative after a while, especially once I got some Emacs keyboard
macros dialled in.

The ones in src/bin/pg_resetwal/t/001_basic.pl I deliberatly left short
because the error messages refer to the short spelling even if you used
the long one.

> It took me some time to read through the patch to catch
> inconsistencies.
>
> -   [ 'pg_verifybackup', '-s', $backup_path ],
> +   [ 'pg_verifybackup', '--skip-checksum', $backup_path ],
>     qr/backup successfully verified/,
>     '-s skips checksumming');
>
> This one in pg_verifybackup should perhaps switch to the long option
> for the test description, like the rest.

Yeah, good point.

> The option should be named --skip-checksums, causing a test failure in
> the CI.

All the tests passed for me on both Debian and macOS, I guess their
getopts accept unambiguous abbreviations of options.

> -my @pg_basebackup_cmd = (@pg_basebackup_defs, '-U', 'backupuser', '-Xfetch');
> +my @pg_basebackup_cmd = (
> +	@pg_basebackup_defs,
> +	'--user' => 'backupuser',
>
> There was a second series of failures in the test
> basebackup_to_shell/t/001_basic.pl because of this part.  The option
> name should be "--username" for pg_basebackup.

Yeah.

> With these two fixes, the CI is happy, which should hopefully be
> enough for the buildfarm.  But we'll see..
>
> I'd like to apply what you have done here (planning to do a second
> lookup, FWIW).  If anybody has objections, feel free to post that
> here.

Thanks again for reviewing this monster patch.

- ilmari