Re: [RFC/PATCH] archive: "--list" does not take further options

From: René Scharfe <l.s.r@web.de>
Date: Dec 21, 2023, 07:30 UTC
Message-ID: <296e8d69-c1d7-4ad2-943a-dfc54940abc2@web.de>
In-Reply-To: <xmqqttocp98r.fsf@gitster.g>

Am 21.12.23 um 03:41 schrieb Junio C Hamano:

Show 29 quoted lines

> "git archive --list blah" should notice an extra command line
> parameter that goes unused.  Make it so.
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>  * This was done to convince myself that even though cmd_archive()
>    calls parse_options with PARSE_OPT_KEEP_UNKNOWN_OPT and then
>    uses the resulting argc/argv without apparent filtering of the
>    "--end-of-options" thing, it is correctly handling it, either
>    locally or remotely.
>
>    - locally, write_archive() uses parse_archive_args(), which calls
>      parse_options() without KEEP_UNKNOWN_OPT and "--end-of-options"
>      is handled there just fine.
>
>    - remotely, run_remote_archiver() relays the local parameters,
>      including "--end-of-options" via the "argument" packet.  Then
>      the arguments are assembled into a strvec and used by the
>      upload-archive running on the other side to spawn an
>      upload-archive--writer process with.
>      cmd_upload_archive_writer() then makes the same write_archive()
>      call; "--end-of-options" would still be in argv[] if the
>      original "git archive --remote" invocation had one, but it is
>      consumed the same way as the local case in write_archive() by
>      calling parse_archive_args().
>
>    I do not like the remote error behaviour this one adds at all.
>    Do we use a more proper mechanism to propagate a remote error
>    back for other subcommands we can reuse here?

Don't we have one? It would affect other unsupported options as well, and this seems to work just fine, e.g.:

   $ git archive --remote=. --format=foo HEAD
   remote: fatal: Unknown archive format 'foo'
   remote: git upload-archive: archiver died with error
   fatal: sent error to the client: git upload-archive: archiver died with error

Show 20 quoted lines

>
>  archive.c           |  7 +++++++
>  t/t5000-tar-tree.sh | 14 ++++++++++++++
>  2 files changed, 21 insertions(+)
>
> diff --git c/archive.c w/archive.c
> index 9aeaf2bd87..3244e9f9f2 100644
> --- c/archive.c
> +++ w/archive.c
> @@ -641,6 +641,13 @@ static int parse_archive_args(int argc, const char **argv,
>  		base = "";
>
>  	if (list) {
> +		if (argc) {
> +			if (!is_remote)
> +				die(_("extra command line parameter '%s'"), *argv);
> +			else
> +				printf("!ERROR! extra command line parameter '%s'\n",
> +				       *argv);
> +		}

So just call die() here?

Show 17 quoted lines

>  		for (i = 0; i < nr_archivers; i++)
>  			if (!is_remote || archivers[i]->flags & ARCHIVER_REMOTE)
>  				printf("%s\n", archivers[i]->name);
> diff --git c/t/t5000-tar-tree.sh w/t/t5000-tar-tree.sh
> index 918a2fc7c6..04592f45b0 100755
> --- c/t/t5000-tar-tree.sh
> +++ w/t/t5000-tar-tree.sh
> @@ -124,6 +124,20 @@ test_expect_success 'setup' '
>  	EOF
>  '
>
> +test_expect_success '--list notices extra parameters' '
> +	test_must_fail git archive --list blah &&
> +	# NEEDSWORK: remote error does not result in non-zero
> +	# exit, which we might want to change later.
> +	git archive --remote=. --list blah >remote-out &&
> +	grep "!ERROR! " remote-out

... and use test_must_fail in both cases?

Show 11 quoted lines

> +'
> +
> +test_expect_success 'end-of-options is correctly eaten' '
> +	git archive --list --end-of-options &&
> +	git archive --remote=. --list --end-of-options >remote-out &&
> +	! grep "!ERROR! " remote-out
> +'
> +
>  test_expect_success 'populate workdir' '
>  	mkdir a &&
>  	echo simple textfile >a/a &&