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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html