Re: [PATCH v2 2/2] checkout: proper error message on 'git checkout foo bar --'

[Jonathan Nieder <jrnieder@gmail.com>]

Hi,

Matthieu Moy wrote:

> error: pathspec 'foo' did not match any file(s) known to git.
> error: pathspec 'bar' did not match any file(s) known to git.
> error: pathspec '--' did not match any file(s) known to git.
>
> This patch fixes it by walking through the argument list to find the
> "--", and now complains about the number of references given.

Good catch.  Just some nits below.

[...]
> --- a/builtin/checkout.c
> +++ b/builtin/checkout.c
> @@ -882,6 +882,7 @@ static int parse_branchname_arg(int argc, const char **argv,
>  	unsigned char branch_rev[20];
>  	const char *arg;
>  	int has_dash_dash;
> +	int i;
>  
>  	/*
>  	 * case 1: git checkout <ref> -- [<paths>]
> @@ -925,7 +926,15 @@ static int parse_branchname_arg(int argc, const char **argv,
>  		return 1;
>  
>  	arg = argv[0];
> -	has_dash_dash = (argc > 1) && !strcmp(argv[1], "--");
> +	has_dash_dash = 0;
> +	for (i = 0; i < argc; i++) {
> +		if (!strcmp(argv[i], "--")) {
> +			has_dash_dash = i;
> +			break;
> +		}
> +	}
> +	if (has_dash_dash >= 2)
> +		die(_("only one reference expected, %d given."), has_dash_dash);

(The argv[0] == "--" case is handled a few lines above.)

At first I skipped the loop and read this as "if (there are two or more
'--' arguments)".  How about doing one of the following to make it
easier to read quickly?

 (a) rename has_dash_dash here to dash_dash_pos, or
 (b) put the check in the loop, like so:

	has_dash_dash = 0;
	for (i = 0; i < ...) {
		if (strcmp(argv[i], "--"))
			continue;
		if (i == 0)	/* case (2) */
			return 1;
		if (i > 1)
			die(_("only one reference expected ...);

		has_dash_dash = 1;
		break;
	}

[...]
> --- a/t/t2010-checkout-ambiguous.sh
> +++ b/t/t2010-checkout-ambiguous.sh
> @@ -47,4 +47,10 @@ test_expect_success 'disambiguate checking out from a tree-ish' '
>  	git diff --exit-code --quiet
>  '
>  
> +test_expect_success C_LOCALE_OUTPUT 'accurate error message with more than one ref' '
> +	test_must_fail git checkout HEAD master -- 2>actual &&
> +	echo "fatal: only one reference expected, 2 given." >expect &&
> +	test_cmp expect actual

Nits:

 - if we change this from 'fatal' to 'error' or reword the message as
   part of a libification some day, it would be a nuisance to have to
   update this test.  Maybe a 'grep' could make it more flexible.

 - most of the test (though not the interesting part) can run in the
   !C_LOCALE_OUTPUT case if you use test_i18ncmp or test_i18ngrep

 - even the check for "2" can run in the !C_LOCALE_OUTPUT case. :)
   e.g. something like

	test_must_fail ... 2>actual &&
	grep 2 actual &&
	test_i18ngrep "one reference expected, 2 given" actual

Thanks,
Jonathan