Re: git show doesn't work on file names with square brackets

Jeff King [off-list ref] writes:

Yes, because ":/" is treated specially in check_filename(), and avoids
kicking in the wildcard behavior. That is certainly preferring revs to
pathspecs, but I think preferring one over the other is preferable to
barfing. If the user wants carefulness, they should use "--"
unconditionally. If they want to DWIM, we should make it as painless as
possible, even if we sometimes guess wrong.

OK, I think that is sensible.

But I have a feeling from what you've written that you do not agree with
the "err and allow something possibly ambiguous" philosophy.

Not anymore ;-)

quoted

I actually think that no_wildcard() check added in check_filename()
was the original mistake.  If we revert the check_filename() to a
simple "Is this a filename?" and move the "does this thing have a
wildcard" aka "can this be a pathspec even when check_filename()
says there is no file with that exact name?" to the code that tries
to allow users omit "--", i.e. the caller of check_filename(), would
that make the code structure and the semantics much cleaner, I
wonder...

Yes. After writing the above, I was envisioning pushing the "err on this
side" logic into check_filename() with a flag. The main callers are
verify_filename() and verify_non_filename(), and they would use opposite
flags from each other.  But pulling that logic out to the caller would
be fine, too.

IOW, something like this implements the "permissive" thing I wrote above
(i.e., be inclusive when seeing if something could plausibly be a
filename, but exclusive when complaining that it _could_ be one):

Yup, I think that is probably a better first step.

On Wed, Feb 10, 2016 at 09:35:46AM -0800, Junio C Hamano wrote:

quoted

IOW, something like this implements the "permissive" thing I wrote above
(i.e., be inclusive when seeing if something could plausibly be a
filename, but exclusive when complaining that it _could_ be one):

Yup, I think that is probably a better first step.

Thanks. And thank you for the discussion. I read your response last
night and almost just said "OK, let's just scrap my patches, this isn't
worth the trouble". But after reading it again this morning, I think it
forced me to look at the problem in a new way. And while I did scrap my
original patches here, I think the result is accomplishing the same
thing in a much saner way.

Here's what I came up with.

  [1/3]: checkout: reorder check_filename conditional
  [2/3]: check_filename: tighten dwim-wildcard ambiguity
  [3/3]: get_sha1: don't die() on bogus search strings

The first is a minor preparatory cleanup, the second is the meat we've
been discussing, and the third is a bonus, though it has some tradeoffs.

-Peff

If we have a "--" flag, we should not be doing DWIM magic
based on whether arguments can be filenames. Reorder the
conditional to avoid the check_filename() call entirely in
this case. The outcome is the same, but the short-circuit
makes the dependency more clear.

Signed-off-by: Jeff King <redacted>
---
 builtin/checkout.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index 5af84a3..f6a2809 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c

@@ -982,7 +982,7 @@ static int parse_branchname_arg(int argc, const char **argv,
 		 */
 		int recover_with_dwim = dwim_new_local_branch_ok;
 
-		if (check_filename(NULL, arg) && !has_dash_dash)
+		if (!has_dash_dash && check_filename(NULL, arg))
 			recover_with_dwim = 0;
 		/*
 		 * Accept "git checkout foo" and "git checkout foo --"

-- 
2.7.1.545.gfd1d4e5

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 <redacted>
---
 builtin/checkout.c                |  3 ++-
 setup.c                           |  6 ++----
 t/t2019-checkout-ambiguous-ref.sh | 26 --------------------------
 t/t6133-pathspec-rev-dwim.sh      | 38 ++++++++++++++++++++++++++++++++++++++
 4 files changed, 42 insertions(+), 31 deletions(-)
 create mode 100755 t/t6133-pathspec-rev-dwim.sh

diff --git a/builtin/checkout.c b/builtin/checkout.c
index f6a2809..cfa66e2 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c

@@ -982,7 +982,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 2c4b22c..995e924 100644
--- a/setup.c
+++ b/setup.c

@@ -139,9 +139,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;

@@ -202,7 +200,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 199b22d..b99d519 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 0000000..8e5b338
--- /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

-- 
2.7.1.545.gfd1d4e5

The get_sha1() function generally returns an error code
rather than dying, and we sometimes speculatively call it
with something that may be a revision or a pathspec, in
order to see which one it might be.

If it sees a bogus ":/" search string, though, it complains,
without giving the caller the opportunity to recover. We can
demonstrate this in t6133 by looking for ":/*.t", which
should mean "*.t at the root of the tree", but instead dies
because of the invalid regex (the "*" has nothing to operate
on).

We can fix this by returning an error rather than calling
die(). Unfortunately, the tradeoff is that the error message
is slightly worse in cases where we _do_ know we have a rev.
E.g., running "git log ':/*.t' --" before yielded:

  fatal: Invalid search pattern: *.t

and now we get only:

  fatal: bad revision ':/*.t'

There's not a simple way to fix this short of passing a
"quiet" flag all the way through the get_sha1() stack.

Signed-off-by: Jeff King <redacted>
---
To be honest, I'm not sure this is worth it. Part of me wants to say
that get_sha1() is simply wrong for dying. And it is, but given how
infrequently this would come up, it's perhaps a practical tradeoff to
get the more accurate error message.

And while it does confuse ":/*.t", which is obviously a pathspec, that's
just one specific case, that works because of the bogus regex. Something
like ":/foo.*" could mean "find foo.* at the root" or it could mean
"find a commit message with foo followed by anything", and we literally
do not know which.

We're likely to treat that one as a rev (assuming you use "foo" in your
commit messages, but who doesn't?). So you'd need to use "--" in the
general case anyway.

 sha1_name.c                  |  4 ++--
 t/t6133-pathspec-rev-dwim.sh | 10 ++++++++++
 2 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/sha1_name.c b/sha1_name.c
index 892db21..d61b3b9 100644
--- a/sha1_name.c
+++ b/sha1_name.c

@@ -882,12 +882,12 @@ static int get_sha1_oneline(const char *prefix, unsigned char *sha1,
 
 	if (prefix[0] == '!') {
 		if (prefix[1] != '!')
-			die ("Invalid search pattern: %s", prefix);
+			return -1;
 		prefix++;
 	}
 
 	if (regcomp(&regex, prefix, REG_EXTENDED))
-		die("Invalid search pattern: %s", prefix);
+		return -1;
 
 	for (l = list; l; l = l->next) {
 		l->item->object.flags |= ONELINE_SEEN;

diff --git a/t/t6133-pathspec-rev-dwim.sh b/t/t6133-pathspec-rev-dwim.sh
index 8e5b338..a290ffc 100755
--- a/t/t6133-pathspec-rev-dwim.sh
+++ b/t/t6133-pathspec-rev-dwim.sh

@@ -35,4 +35,14 @@ test_expect_success '@{foo} with metacharacters dwims to rev' '
 	test_cmp expect actual
 '
 
+test_expect_success ':/*.t from a subdir dwims to a pathspec' '
+	mkdir subdir &&
+	(
+		cd subdir &&
+		git log -- ":/*.t" >expect &&
+		git log    ":/*.t" >actual &&
+		test_cmp expect actual
+	)
+'
+
 test_done

-- 
2.7.1.545.gfd1d4e5