check_filename: tighten dwim-wildcard ambiguity

When specifying both revisions and pathnames, we allow
"<rev> -- <pathspec>" to be spelled without the "--" as long
as it is not ambiguous. The original logic was something
like:

  1. Resolve each item with get_sha1(). If successful,
     we know it can be a <rev>. Verify that it _isn't_ a
     filename, using verify_non_filename(), and complain of
     ambiguity otherwise.

  2. If get_sha1() didn't succeed, make sure that it _is_
     a file, using verify_filename(). If not, complain
     that it is neither a <rev> nor a <pathspec>.

Both verify_filename() and verify_non_filename() rely on
check_filename(), which definitely said "yes, this is a
file" or "no, it is not" using lstat().

Commit 28fcc0b (pathspec: avoid the need of "--" when
wildcard is used, 2015-05-02) introduced a convenience
feature: check_filename() will consider anything with
wildcard meta-characters as a possible filename, without
even checking the filesystem.

This works well for case 2. For such a wildcard, we would
previously have died and said "it is neither". Post-28fcc0b,
we assume it's a pathspec and proceed.

But it makes some instances of case 1 worse. We may have an
extended sha1 expression that contains meta-characters
(e.g., "HEAD^{/foo.*bar}"), and we now complain that it's
also a filename, due to the wildcard characters (even though
that wildcard would not match anything in the filesystem).

One solution would be to actually expand the pathname and
see if it matches anything on the filesystem. But that's
potentially expensive, and we do not have to be so rigorous
for this DWIM magic (if you want rigor, use "--").

Instead, we can just use different rules for cases 1 and 2.
When we know something is a rev, we will complain only if it
meets a much higher standard for "this is also a file";
namely that it actually exists in the filesystem. Case 2
remains the same: we use the looser "it could be a filename"
standard introduced by 28fcc0b.

We can accomplish this by pulling the wildcard logic out of
check_filename() and putting it into verify_filename(). Its
partner verify_non_filename() does not need a change, since
check_filename() goes back to implementing the "higher
standard".

Besides these two callers of check_filename(), there is one
other: git-checkout does a similar DWIM itself. It hits this
code path only after get_sha1() has returned failure, making
it case 2, which gets the special wildcard treatment.

Note that we drop the tests in t2019 in favor of a more
complete set in t6133. t2019 was not the right place for
them (it's about refname ambiguity, not dwim parsing
ambiguity), and the second test explicitly checked for the
opposite result of the case we are fixing here (which didn't
really make any sense; as shown by the test_must_fail in the
test, it would only serve to annoy people).

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

4 files changed, 42 insertions, 31 deletions

diff --git a/builtin/checkout.c b/builtin/checkout.c
index d34f58eba6..e5c16af9ef 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -965,7 +965,8 @@ static int parse_branchname_arg(int argc, const char **argv,
 		 */
 		int recover_with_dwim = dwim_new_local_branch_ok;
 
-		if (!has_dash_dash && check_filename(NULL, arg))
+		if (!has_dash_dash &&
+		    (check_filename(NULL, arg) || !no_wildcard(arg)))
 			recover_with_dwim = 0;
 		/*
 		 * Accept "git checkout foo" and "git checkout foo --"
diff --git a/setup.c b/setup.c
index 1055b82708..69a8adaaf2 100644
--- a/setup.c
+++ b/setup.c
@@ -140,9 +140,7 @@ int check_filename(const char *prefix, const char *arg)
 		if (arg[2] == '\0') /* ":/" is root dir, always exists */
 			return 1;
 		name = arg + 2;
-	} else if (!no_wildcard(arg))
-		return 1;
-	else if (prefix)
+	} else if (prefix)
 		name = prefix_filename(prefix, strlen(prefix), arg);
 	else
 		name = arg;
@@ -203,7 +201,7 @@ void verify_filename(const char *prefix,
 {
 	if (*arg == '-')
 		die("bad flag '%s' used after filename", arg);
-	if (check_filename(prefix, arg))
+	if (check_filename(prefix, arg) || !no_wildcard(arg))
 		return;
 	die_verify_filename(prefix, arg, diagnose_misspelt_rev);
 }
diff --git a/t/t2019-checkout-ambiguous-ref.sh b/t/t2019-checkout-ambiguous-ref.sh
index 199b22d85e..b99d5192a9 100755
--- a/t/t2019-checkout-ambiguous-ref.sh
+++ b/t/t2019-checkout-ambiguous-ref.sh
@@ -56,30 +56,4 @@ test_expect_success VAGUENESS_SUCCESS 'checkout reports switch to branch' '
 	test_i18ngrep ! "^HEAD is now at" stderr
 '
 
-test_expect_success 'wildcard ambiguation, paths win' '
-	git init ambi &&
-	(
-		cd ambi &&
-		echo a >a.c &&
-		git add a.c &&
-		echo b >a.c &&
-		git checkout "*.c" &&
-		echo a >expect &&
-		test_cmp expect a.c
-	)
-'
-
-test_expect_success !MINGW 'wildcard ambiguation, refs lose' '
-	git init ambi2 &&
-	(
-		cd ambi2 &&
-		echo a >"*.c" &&
-		git add . &&
-		test_must_fail git show :"*.c" &&
-		git show :"*.c" -- >actual &&
-		echo a >expect &&
-		test_cmp expect actual
-	)
-'
-
 test_done
diff --git a/t/t6133-pathspec-rev-dwim.sh b/t/t6133-pathspec-rev-dwim.sh
new file mode 100755
index 0000000000..8e5b338633
--- /dev/null
+++ b/t/t6133-pathspec-rev-dwim.sh
@@ -0,0 +1,38 @@
+#!/bin/sh
+
+test_description='test dwim of revs versus pathspecs in revision parser'
+. ./test-lib.sh
+
+test_expect_success 'setup' '
+	test_commit base &&
+	echo content >"br[ack]ets" &&
+	git add . &&
+	test_tick &&
+	git commit -m brackets
+'
+
+test_expect_success 'non-rev wildcard dwims to pathspec' '
+	git log -- "*.t" >expect &&
+	git log    "*.t" >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'tree:path with metacharacters dwims to rev' '
+	git show "HEAD:br[ack]ets" -- >expect &&
+	git show "HEAD:br[ack]ets"    >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success '^{foo} with metacharacters dwims to rev' '
+	git log "HEAD^{/b.*}" -- >expect &&
+	git log "HEAD^{/b.*}"    >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success '@{foo} with metacharacters dwims to rev' '
+	git log "HEAD@{now [or thereabouts]}" -- >expect &&
+	git log "HEAD@{now [or thereabouts]}"    >actual &&
+	test_cmp expect actual
+'
+
+test_done