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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to