Re: [PATCH v2] backfill: handle unexpected arguments

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 22, 2026, 01:13 UTC
Message-ID: <xmqqfr5sacps.fsf@gitster.g>
In-Reply-To: <20260321174730.34762-1-r.siddharth.shrimali@gmail.com>

Siddharth Shrimali <r.siddharth.shrimali@gmail.com> writes:

Show 10 quoted lines

> git backfill takes no non-option arguments. However, if extra
> arguments are passed with git backfill, parse_options() leaves
> them in argc and the command ignores them silently, giving the
> user no indication that something is wrong.
>
> Add a check after parse_options() to call usage_with_options()
> if any unexpected arguments remain. To ensure the user understands
> why the command failed, print an error message specifying the unknown
> argument before showing the usage string. This is consistent with how
> other Git commands such as git-bugreport handle this situation.

While it is a thing to aim for, I doubt the updated behaviour is consistent with how bugreport behaves.

	$ git bugreport extra-arg
        error: unknown argument `extra-arg'
        usage: git bugreport [(-o | --output-directory) <path>]
                      [(-s | --suffix) <format> | --no-suffix]
                      [--diagnose[=<mode>]]

I.e. the command does not show the list of options with descriptions to the user, who did not make any mistake in specifying any dashed option. On the other hand,

	$ git bugreport --mistyped-option

would give usage_with_options(), because unlike the case where the user gave only an extra argument, it is clear in this case that the user failed to choose the right option, which might be helped if we tell them what each option does. Also with

	$ git bugreport --suffix

the user will get a more targetted help that is specific to the option given incorrectly.

Compared to that, the way the updated code would react to

	$ git backfill extra-arg

cannot be called "consistent with how" bugreport handles this situation, I would have to say. I didn't check how the command behaves in the other two cases (i.e., option that does not exist, and an option that needs a value but the user forgot to give one)

Show 11 quoted lines

> diff --git a/builtin/backfill.c b/builtin/backfill.c
> index e9a33e81be..5a333afde0 100644
> --- a/builtin/backfill.c
> +++ b/builtin/backfill.c
> @@ -135,6 +135,11 @@ int cmd_backfill(int argc, const char **argv, const char *prefix, struct reposit
>  
>  	argc = parse_options(argc, argv, prefix, options, builtin_backfill_usage,
>  			     0);
> +	
> +	if (argc) {
> +		error(_("unknown argument `%s'"), argv[0]);

Yikes.  Please do not add more uses of cute quotes.

Perhaps we found a microproject target for the next intern season? Let's not spread cute quotes any more than what we already have, and clean existing up ones before it becomes too late.

    $ git grep -e "'%s'" \*.c | wc -l
    2012
    $ git grep -e "\`%s'" \*.c | wc -l
    16

Show 16 quoted lines

> +		usage_with_options(builtin_backfill_usage, options);
> +	}
>  
>  	repo_config(repo, git_default_config, NULL);
>  
> diff --git a/t/t5620-backfill.sh b/t/t5620-backfill.sh
> index 58c81556e7..d74e1be74b 100755
> --- a/t/t5620-backfill.sh
> +++ b/t/t5620-backfill.sh
> @@ -176,6 +176,11 @@ test_expect_success 'backfill --sparse without cone mode (negative)' '
>  	test_line_count = 12 missing
>  '
>  
> +test_expect_success 'backfill rejects unexpected arguments' '
> +	test_must_fail git -C backfill1 backfill unexpected-arg 2>err &&
> +	grep "unknown argument .*unexpected-arg" err

The test is a bit underspecified, isn't it?

This test will still pass if the code called usage_with_options() to show the list of option descriptions after the short usage, instead of showing just the short usage by calling usage().

As with any test, it is important to test that the output has what we expect to see, but at the same time we need to ensure that the output does not have what we do not want to see.

> +'
> +
>  . "$TEST_DIRECTORY"/lib-httpd.sh
>  start_httpd