Re: [RFC/PATCH] checkout: allow dwim for branch creation for "git checkout $branch --"

Duy Nguyen [off-list ref] writes:

has_dash_dash is calculated as (argc > 1) && !strcmp(argv[1], "--"),
so when argc == 1, the has_dash_dash must be zero, the "&&
!has_dash_dash" is redundant.

Yes, but I'd rather not have to read the detailed definition of
has_dash_dash to understand the code. With my version, the name of the
variable is actually sufficient to understand.

But it makes me wonder what we do with "git checkout abc def -- xyz".

Ouch. Both old and new say

$ git checkout foo bar --
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.

Fortunately, this is easy to fix, by adding this on top (or before, it
doesn't matter):

commit 060cf582f5e848c5ca57231d293943a5489c5433 (HEAD, master)
Author: Matthieu Moy [off-list ref]
Date:   Wed Sep 25 15:41:34 2013 +0200

    checkout: proper error message on 'git checkout foo bar --'
    
    The previous code was detecting the presence of "--" by looking only at
    argument 1. As a result, "git checkout foo bar --" was interpreted as an
    ambiguous file/revision list, and errored out with:
    
    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.

diff --git a/builtin/checkout.c b/builtin/checkout.c
index a5a12f6..d958d60 100644
--- 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);
 
        if (!strcmp(arg, "-"))
                arg = "@{-1}";

diff --git a/t/t2010-checkout-ambiguous.sh b/t/t2010-checkout-ambiguous.sh
index 7cc0a35..2836a3e 100755
--- 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 '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
+'
+
 test_done

I'll resend, together with tweaks to the first patch.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

On Wed, Sep 25, 2013 at 8:52 PM, Matthieu Moy
[off-list ref] wrote:

The translator in me says this string should be marked for translation
like others in git-checkout...

which makes C_LOCALE_OUTPUT a prerequisite for this test because it
needs the untranslated version of the string.

I'll resend, together with tweaks to the first patch.

--
Matthieu Moy
http://www-verimag.imag.fr/~moy/

-- 
Duy

Matthieu Moy [off-list ref] writes:

Duy Nguyen [off-list ref] writes:

quoted

has_dash_dash is calculated as (argc > 1) && !strcmp(argv[1], "--"),
so when argc == 1, the has_dash_dash must be zero, the "&&
!has_dash_dash" is redundant.

Yes, but I'd rather not have to read the detailed definition of
has_dash_dash to understand the code. With my version, the name of the
variable is actually sufficient to understand.

quoted

But it makes me wonder what we do with "git checkout abc def -- xyz".

Ouch. Both old and new say
...
I'll resend, together with tweaks to the first patch.

Did anything further happen to this discussion?  Is v3 the version
with agreement among the list members I just should pick up?