Re: [PATCH] archive: error instead of triggering a segfault in `git archive --remote=""`

[Patrick Steinhardt <ps@pks.im>]

On Sun, Mar 09, 2025 at 10:12:35AM +0000, emilylime via GitGitGadget wrote:
> From: emilylime <emilyyyylime+git@gmail.com>

This is missing a bit of a description:

    - What is the observed bug?
    - When does the bug trigger?
    - Optional: since when does the bug exist?
    - How do we fix it?
    - Optional: are there alternative ways to fix this bug that you have
      considered but found to be less optimal.

> Signed-off-by: emilylime <emilyyyylime+git@gmail.com>

We usually prefer people to sign off with their full name.

> diff --git a/builtin/archive.c b/builtin/archive.c
> index 13ea7308c8b..b6fdbfc7dca 100644
> --- a/builtin/archive.c
> +++ b/builtin/archive.c
> @@ -97,6 +97,10 @@ int cmd_archive(int argc,
>  	argc = parse_options(argc, argv, prefix, local_opts, NULL,
>  			     PARSE_OPT_KEEP_ALL);
>  
> +	if (remote && !remote[0]) {

Okay, so this triggers in case the user passes "--remote ''"?
I see that we ultimately pass the string to `remote_get()`, so does that
function segfault? If so, can other callers of that function segfault in
a similar way? In that case, we should probably address the issue deeper
down in the call stack.

> +		usage(N_("Option 'remote' may not be left empty"));

Error and usage strings should start with a lower-case letter.

> +	}

The curly braces aren't required.

It would also be nice to add a testcase, e.g. in "t/t5000-tar-tree.sh".

Thanks!

Patrick