Merge branch 'ph/checkout'

* ph/checkout:
  git-checkout: improve error messages, detect ambiguities.
  git-checkout: fix command line parsing.

2 files changed, 117 insertions, 18 deletions

diff --git a/builtin-checkout.c b/builtin-checkout.c
index fbd5105a8..411cc513c 100644
--- 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(),
 	};
+	int has_dash_dash;
 
 	memset(&opts, 0, sizeof(opts));
 	memset(&new, 0, sizeof(new));
@@ -438,12 +439,57 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
 
 	opts.track = git_branch_track;
 
-	argc = parse_options(argc, argv, options, checkout_usage, 0);
+	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 (!strcmp(argv[0], "--")) {       /* case (2) */
+			argv++;
+			argc--;
+			goto no_reference;
+		}
+
 		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))
@@ -452,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;
-			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--;
 		}
 	}
 
-	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);
 
diff --git a/t/t2010-checkout-ambiguous.sh b/t/t2010-checkout-ambiguous.sh
new file mode 100755
index 000000000..7cc0a3582
--- /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