Re: [PATCH v2] backfill: handle unexpected arguments

[Junio C Hamano <gitster@pobox.com>]

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

> 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)


> 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

> +		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