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

Jeff King [off-list ref] writes:

The patch for that might look like this. I like it for its relative
simplicity, though it does make the rules even harder to explain to a
user...

True.

To be bluntly honest, I do not see the current "string containing
wildcard characters are taken as path, not rev, unless you use the
double dash to disambiguate." all bad.  Isn't it sort of crazy to
have square brackets in paths and if it requires clarification by
the user, I do not particulasrly see it as a problem.

Having said that, I do not think of a big reason to say this patch
is a wrong thing to do, either.

This breaks the second test in t2019 added by ae454f6, but I am not sure
that test is doing the right thing (I'm also not sure t2019 is the best
place for these tests; I added new ones here in a separate script).

I am inclined to agree that that particular test is casting an
implementation limitation in stone.

You seem to reject ":(" specifically, but I am not sure whom is it
designed to help to special case ":(".  Those who write ":(top)"
would not have to disambiguate with "--", but their preference is to
spell things in longhand for more explicit control, so I do not
think they mind typing "--".  On the other hand, those who write
":/" and ":!" (":(top)" and ":(exclude)") would need to disambiguate
with "--" with the change.

That somehow feels backwards.

"A pathspec element with the magic prefix" is hard to tell from
"Look for a path in the index" but not from "Look for a path in a
tree-ish", so if you get (p && p != arg), you know it is tree:path,
I think.

On Mon, Feb 08, 2016 at 11:35:10AM -0800, Junio C Hamano wrote:

To be bluntly honest, I do not see the current "string containing
wildcard characters are taken as path, not rev, unless you use the
double dash to disambiguate." all bad.  Isn't it sort of crazy to
have square brackets in paths and if it requires clarification by
the user, I do not particulasrly see it as a problem.

Having said that, I do not think of a big reason to say this patch
is a wrong thing to do, either.

To be honest, I think I am more concerned with ":/reg.*ex" than
"tree:path" using meta-characters. I agree that actual metacharacters in
file names are relatively rare.

quoted

+static int dwim_as_wildcard(const char *arg)
+{
+	const char *p;
+
+	if (no_wildcard(arg))
+		return 0;
+	if (strstr(arg, "^{"))
+		return 0; /* probably "^{something}" */
+	if (strstr(arg, "@{"))
+		return 0; /* probably "ref@{something}" */
+
+	/* catch "tree:path", but not ":(magic)" */
+	p = strchr(arg, ':');
+	if (p && p[1] != '(')
+		return 0;

You seem to reject ":(" specifically, but I am not sure whom is it
designed to help to special case ":(".  Those who write ":(top)"
would not have to disambiguate with "--", but their preference is to
spell things in longhand for more explicit control, so I do not
think they mind typing "--".  On the other hand, those who write
":/" and ":!" (":(top)" and ":(exclude)") would need to disambiguate
with "--" with the change.

That somehow feels backwards.

Good point. I forgot about the short-hands, and I agree that there is
not much point in doing a sloppy match of the long-hands if we do not
cover the short-hands.

In retrospect, it is not worth trying to match magic pathspecs here at
all, as it generally requires "--" anyway. It is only ones with wildcard
which came along for the ride in 28fcc0b, and that was not the primary
focus of that patch. I.e., without my patch, we already have:

  $ git rev-list --count HEAD Makefile
  2028

  $ git rev-list --count HEAD ':(top)Makefile'
  fatal: ambiguous argument ':(top)Makefile': unknown revision or path
  not in the working tree.
  Use '--' to separate paths from revisions, like this:
  'git <command> [<revision>...] -- [<file>...]'

  $ git rev-list --count HEAD ':(top)M[a]kefile'
  2028

which is slightly ridiculous. With my patch, the final one behaves the
same as the second.

Here is my patch again, with that part removed, and the tests fixed up.
Though on reflection, I do think it would be better if we could simply
expand the wildcard globs to say "does this match anything in the file
system". That makes a nice, simple rule that follows the spirit of the
original. I'm not sure if it would be easy to apply magic like ":(top)"
there, but even if we don't, we're not worse off than we are today
(where that requires "--" unless it happens to have a wildcard, as
above).

-- >8 --
Subject: [PATCH] check_filename: tighten requirements for dwim-wildcards

Commit 28fcc0b (pathspec: avoid the need of "--" when
wildcard is used, 2015-05-02) introduced a convenience to
our dwim-parsing: when "--" is not present, we guess that
items with wildcard characters are probably pathspecs.

This makes a lot of cases simpler (e.g., "git log '*.c'"),
but makes others harder. While revision expressions do not
typically have wildcard characters in them (because they are
not valid in refnames), there are a few constructs where we
take more arbitrary strings, such as:

  - pathnames in tree:path syntax (or :0:path) for the
    index)

  - :/foo and ^{/foo} for searching commit messages;
    likewise "^{}" is extensible and may learn new formats
    in the future

  - @{foo}, which can take arbitrary approxidate text (which
    is not itself that likely to have wildcards, but @{} is
    also a potential generic extension mechanism).

When we see these constructs, they are almost certainly an
attempt at a revision, and not a pathspec; we should not
give them the magic "wildcard characters mean a pathspec"
treatment.

We can afford to be fairly slack in our parsing here. We are
not making a real decision on "this is or is not definitely
a revision" here, but rather just deciding whether or not
the extra "wildcards mean pathspecs" magic kicks in.

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 show by the test_must_fail in the
test, it would only serve to annoy people).

Signed-off-by: Jeff King <redacted>
---
 setup.c                           | 15 ++++++++++++++-
 t/t2019-checkout-ambiguous-ref.sh | 26 --------------------------
 t/t6133-pathspec-rev-dwim.sh      | 38 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 52 insertions(+), 27 deletions(-)
 create mode 100755 t/t6133-pathspec-rev-dwim.sh

diff --git a/setup.c b/setup.c
index 2c4b22c..eac1edc 100644
--- a/setup.c
+++ b/setup.c

@@ -130,6 +130,19 @@ int path_inside_repo(const char *prefix, const char *path)
 	return 0;
 }
 
+static int dwim_as_wildcard(const char *arg)
+{
+	if (no_wildcard(arg))
+		return 0;
+	if (strstr(arg, "^{"))
+		return 0; /* probably "^{something}" */
+	if (strstr(arg, "@{"))
+		return 0; /* probably "ref@{something}" */
+	if (strchr(arg, ':'))
+		return 0;
+	return 1;
+}
+
 int check_filename(const char *prefix, const char *arg)
 {
 	const char *name;

@@ -139,7 +152,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))
+	} else if (dwim_as_wildcard(arg))
 		return 1;
 	else if (prefix)
 		name = prefix_filename(prefix, strlen(prefix), arg);

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..2ffebee
--- /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 'wildcard dwims to pathspec' '
+	git log -- "*.t" >expect &&
+	git log    "*.t" >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'tree:path 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} dwims to rev' '
+	git log "HEAD^{/b.*}" -- >expect &&
+	git log "HEAD^{/b.*}"    >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success '@{foo} 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.526.gd04f550

On Mon, Feb 08, 2016 at 02:52:30PM -0500, Jeff King wrote:

Here is my patch again, with that part removed, and the tests fixed up.
Though on reflection, I do think it would be better if we could simply
expand the wildcard globs to say "does this match anything in the file
system". That makes a nice, simple rule that follows the spirit of the
original. I'm not sure if it would be easy to apply magic like ":(top)"
there, but even if we don't, we're not worse off than we are today
(where that requires "--" unless it happens to have a wildcard, as
above).

So here is a hacky attempt at that. It uses glob(), which is not quite
right for the reasons below, though I suspect works OK in practice.

I think doing it correctly would require actually calling our
read_directory() function. That feels kind of heavy-weight for this
case, but I guess in theory the pathspec limits it (and it's not like
glob() does not have to walk the filesystem, too). So maybe it's not so
bad.

---

diff --git a/setup.c b/setup.c
index 2c4b22c..d8a7b9d 100644
--- a/setup.c
+++ b/setup.c

@@ -1,6 +1,7 @@
 #include "cache.h"
 #include "dir.h"
 #include "string-list.h"
+#include <glob.h>
 
 static int inside_git_dir = -1;
 static int inside_work_tree = -1;

@@ -130,6 +131,26 @@ int path_inside_repo(const char *prefix, const char *path)
 	return 0;
 }
 
+/*
+ * Return true if a file exists that matches the pattern
+ * glob. Note that this _should_ use our regular wildmatch
+ * pattern matches, but there is no ready-made glob()
+ * function there. This is a cheap hack that makes
+ * simple things like "*.c" work without having to
+ * use a "--" disambiguator.
+ *
+ * A custom glob() could also do this more efficiently; we don't
+ * care about collecting the results, and can quit as soon as
+ * we see one.
+ */
+static int glob_exists(const char *pattern)
+{
+	glob_t data;
+	int r = glob(pattern, GLOB_NOSORT, NULL, &data);
+	globfree(&data);
+	return !r;
+}
+
 int check_filename(const char *prefix, const char *arg)
 {
 	const char *name;

@@ -139,12 +160,14 @@ 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;
+
+	if (!no_wildcard(arg))
+		return glob_exists(name);
+
 	if (!lstat(name, &st))
 		return 1; /* file exists */
 	if (errno == ENOENT || errno == ENOTDIR)

On Mon, Feb 08, 2016 at 03:20:43PM -0500, Jeff King wrote:

On Mon, Feb 08, 2016 at 02:52:30PM -0500, Jeff King wrote:

quoted

Here is my patch again, with that part removed, and the tests fixed up.
Though on reflection, I do think it would be better if we could simply
expand the wildcard globs to say "does this match anything in the file
system". That makes a nice, simple rule that follows the spirit of the
original. I'm not sure if it would be easy to apply magic like ":(top)"
there, but even if we don't, we're not worse off than we are today
(where that requires "--" unless it happens to have a wildcard, as
above).

So here is a hacky attempt at that. It uses glob(), which is not quite
right for the reasons below, though I suspect works OK in practice.

I think doing it correctly would require actually calling our
read_directory() function. That feels kind of heavy-weight for this
case, but I guess in theory the pathspec limits it (and it's not like
glob() does not have to walk the filesystem, too). So maybe it's not so
bad.

And here that is. It does end up traversing quite a bit for something as
simple as "*.foo", because that doesn't let fill_directory() limit us at
all.

But having looked at this, I can't help but wonder if the rule should
not be "does the file exist" in the first place, but "is the file in the
index". This dwimmery is about commands like "log" that are reading
existing commits. I cannot think of a case where we would want to
include something that exists in the filesystem but not in the index.

---

diff --git a/setup.c b/setup.c
index 2c4b22c..1a40516 100644
--- a/setup.c
+++ b/setup.c

@@ -1,5 +1,6 @@
 #include "cache.h"
 #include "dir.h"
+#include "pathspec.h"
 #include "string-list.h"
 
 static int inside_git_dir = -1;

@@ -130,6 +131,33 @@ int path_inside_repo(const char *prefix, const char *path)
 	return 0;
 }
 
+/*
+ * Return true if a file exists that matches the pattern
+ * glob.
+ */
+static int pathspec_exists(const char *one_pathspec)
+{
+	struct dir_struct dir;
+	const char *pathspec_v[] = { one_pathspec, NULL };
+	struct pathspec pathspec;
+	int ret = 0;
+	int i;
+
+	memset(&dir, 0, sizeof(dir));
+	parse_pathspec(&pathspec, 0, 0, "", pathspec_v);
+
+	fill_directory(&dir, &pathspec);
+	for (i = 0; i < dir.nr; i++) {
+		if (dir_path_match(dir.entries[i], &pathspec, 0, NULL)) {
+			ret = 1;
+			break;
+		}
+	}
+
+	free(dir.entries);
+	return ret;
+}
+
 int check_filename(const char *prefix, const char *arg)
 {
 	const char *name;

@@ -139,12 +167,14 @@ 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;
+
+	if (!no_wildcard(arg))
+		return pathspec_exists(name);
+
 	if (!lstat(name, &st))
 		return 1; /* file exists */
 	if (errno == ENOENT || errno == ENOTDIR)