git-checkout: improve error messages, detect ambiguities.

The patch is twofold: it moves the option consistency checks just under
the parse_options call so that it doesn't get in the way of the tree
reference vs. pathspecs desambiguation.

The other part rewrites the way to understand arguments so that when
git-checkout fails it does with an understandable message. Compared to the
previous behavior we now have:

- a better error message when doing:

git checkout <blob reference> --

now complains about the reference not pointing to a tree, instead of
things like:

error: pathspec <blob reference> did not match any file(s) known to git.
error: pathspec '--' did not match any file(s) known to git.

- a better error message when doing:

git checkout <path> --

It now complains about <path> not being a reference instead of the
completely obscure:

error: pathspec '--' did not match any file(s) known to git.

- an error when -- wasn't used, and the first argument is ambiguous
(i.e. can be interpreted as both ref and as path).

Signed-off-by: Pierre Habouzit <madcoder@debian.org>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/builtin-checkout.c b/builtin-checkout.c
index 9cadf9c299eb9875c74ac3beb629123ce2d9915c..411cc513c65ba854221ad52dd6aeaaac7d213c9d 100644 (file)
--- a/builtin-checkout.c
+++ b/builtin-checkout.c
@@ -430,6 +430,7 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
                OPT_BOOLEAN('m', NULL, &opts.merge, "merge"),
                OPT_END(),
        };
                OPT_BOOLEAN('m', NULL, &opts.merge, "merge"),
                OPT_END(),
        };

+       int has_dash_dash;

 
        memset(&opts, 0, sizeof(opts));
        memset(&new, 0, sizeof(new));
 
        memset(&opts, 0, sizeof(opts));
        memset(&new, 0, sizeof(new));


@@ -440,11 +441,55 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
 
        argc = parse_options(argc, argv, options, checkout_usage,
                             PARSE_OPT_KEEP_DASHDASH);
 
        argc = parse_options(argc, argv, options, checkout_usage,
                             PARSE_OPT_KEEP_DASHDASH);

+
+       if (!opts.new_branch && (opts.track != git_branch_track))
+               die("git checkout: --track and --no-track require -b");
+
+       if (opts.force && opts.merge)
+               die("git checkout: -f and -m are incompatible");
+
+       /*
+        * case 1: git checkout <ref> -- [<paths>]
+        *
+        *   <ref> must be a valid tree, everything after the '--' must be
+        *   a path.
+        *
+        * case 2: git checkout -- [<paths>]
+        *
+        *   everything after the '--' must be paths.
+        *
+        * case 3: git checkout <something> [<paths>]
+        *
+        *   With no paths, if <something> is a commit, that is to
+        *   switch to the branch or detach HEAD at it.
+        *
+        *   Otherwise <something> shall not be ambiguous.
+        *   - If it's *only* a reference, treat it like case (1).
+        *   - If it's only a path, treat it like case (2).
+        *   - else: fail.
+        *
+        */

        if (argc) {
        if (argc) {

+               if (!strcmp(argv[0], "--")) {       /* case (2) */
+                       argv++;
+                       argc--;
+                       goto no_reference;
+               }
+

                arg = argv[0];
                arg = argv[0];

-               if (get_sha1(arg, rev))
-                       ;
-               else if ((new.commit = lookup_commit_reference_gently(rev, 1))) {
+               has_dash_dash = (argc > 1) && !strcmp(argv[1], "--");
+
+               if (get_sha1(arg, rev)) {
+                       if (has_dash_dash)          /* case (1) */
+                               die("invalid reference: %s", arg);
+                       goto no_reference;          /* case (3 -> 2) */
+               }
+
+               /* we can't end up being in (2) anymore, eat the argument */
+               argv++;
+               argc--;
+
+               if ((new.commit = lookup_commit_reference_gently(rev, 1))) {

                        new.name = arg;
                        setup_branch_path(&new);
                        if (resolve_ref(new.path, rev, 1, NULL))
                        new.name = arg;
                        setup_branch_path(&new);
                        if (resolve_ref(new.path, rev, 1, NULL))


@@ -453,25 +498,28 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
                                new.path = NULL;
                        parse_commit(new.commit);
                        source_tree = new.commit->tree;
                                new.path = NULL;
                        parse_commit(new.commit);
                        source_tree = new.commit->tree;

-                       argv++;
-                       argc--;
-               } else if ((source_tree = parse_tree_indirect(rev))) {
+               } else
+                       source_tree = parse_tree_indirect(rev);
+
+               if (!source_tree)                   /* case (1): want a tree */
+                       die("reference is not a tree: %s", arg);
+               if (!has_dash_dash) {/* case (3 -> 1) */
+                       /*
+                        * Do not complain the most common case
+                        *      git checkout branch
+                        * even if there happen to be a file called 'branch';
+                        * it would be extremely annoying.
+                        */
+                       if (argc)
+                               verify_non_filename(NULL, arg);
+               }
+               else {

                        argv++;
                        argc--;
                }
        }
 
                        argv++;
                        argc--;
                }
        }
 

-       if (argc && !strcmp(argv[0], "--")) {
-               argv++;
-               argc--;
-       }
-
-       if (!opts.new_branch && (opts.track != git_branch_track))
-               die("git checkout: --track and --no-track require -b");
-
-       if (opts.force && opts.merge)
-               die("git checkout: -f and -m are incompatible");
-
+no_reference:

        if (argc) {
                const char **pathspec = get_pathspec(prefix, argv);
 
        if (argc) {
                const char **pathspec = get_pathspec(prefix, argv);
 

diff --git a/t/t2010-checkout-ambiguous.sh b/t/t2010-checkout-ambiguous.sh
new file mode 100755 (executable)
index 0000000..7cc0a35
--- /dev/null
+++ b/t/t2010-checkout-ambiguous.sh
@@ -0,0 +1,50 @@
+#!/bin/sh
+
+test_description='checkout and pathspecs/refspecs ambiguities'
+
+. ./test-lib.sh
+
+test_expect_success 'setup' '
+       echo hello >world &&
+       echo hello >all &&
+       git add all world &&
+       git commit -m initial &&
+       git branch world
+'
+
+test_expect_success 'reference must be a tree' '
+       test_must_fail git checkout $(git hash-object ./all) --
+'
+
+test_expect_success 'branch switching' '
+       test "refs/heads/master" = "$(git symbolic-ref HEAD)" &&
+       git checkout world -- &&
+       test "refs/heads/world" = "$(git symbolic-ref HEAD)"
+'
+
+test_expect_success 'checkout world from the index' '
+       echo bye > world &&
+       git checkout -- world &&
+       git diff --exit-code --quiet
+'
+
+test_expect_success 'non ambiguous call' '
+       git checkout all
+'
+
+test_expect_success 'allow the most common case' '
+       git checkout world &&
+       test "refs/heads/world" = "$(git symbolic-ref HEAD)"
+'
+
+test_expect_success 'check ambiguity' '
+       test_must_fail git checkout world all
+'
+
+test_expect_success 'disambiguate checking out from a tree-ish' '
+       echo bye > world &&
+       git checkout world -- world &&
+       git diff --exit-code --quiet
+'
+
+test_done