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

I’ve faced a problem that `git show <rev>:<filename>` returns an error when <filename> contains square brackets.

Interestingly, the problem is reproducible on "GNU bash, version 3.2.57(1)-release (x86_64-apple-darwin15)", but not on "zsh 5.0.7 (x86_64-pc-linux-gnu)”. The problem is also reproducible when called from a Java program by forking a process with given parameters.

Is it a bug or I just didn’t find the proper way to escape the brackets? 

Steps to reproduce:

    git init brackets
    cd brackets/
    echo ‘asd’ > bra[ckets].txt
    git add bra\[ckets\].txt
    git commit -m initial
    git show HEAD:bra[ckets].txt

Error:
fatal: ambiguous argument 'HEAD:bra[ckets].txt': both revision and filename
Use '--' to separate paths from revisions, like this:
'git <command> [<revision>...] -- [<file>...]’

Neither escaping, not quoting doesn’t help:
    git show HEAD:bra\[ckets\].txt
returns the same error

    git show "HEAD:bra\[ckets\].txt”
returns empty output

Thanks a lot!
-- Kirill

Hi Kirill,

On Sat, 6 Feb 2016, Kirill Likhodedov wrote:

Is it a bug or I just didn’t find the proper way to escape the brackets? 

Steps to reproduce:

    git init brackets
    cd brackets/
    echo ‘asd’ > bra[ckets].txt
    git add bra\[ckets\].txt
    git commit -m initial
    git show HEAD:bra[ckets].txt

This is expected behavior of the Bash you are using. The commands that I
think would reflect your intentions would be:

	git init brackets
	cd brackets
	echo 'asd' > 'bra[ckets].txt'
	git add 'bra[ckets].txt'
	git commit -m initial
	git show 'HEAD:bra[ckets].txt'

You could also escape the brackets with a backslash, as you did, but you
would have to do it *every* time you write the path, not just in the `git
add` incantation.

Ciao,
Johannes

Hi Johannes,

thanks for your answer, but unfortunately it doesn’t help.

On 06 Feb 2016, at 17:21 , Johannes Schindelin [off-list ref] wrote:

This is expected behavior of the Bash you are using. The commands that I
think would reflect your intentions would be:

	git init brackets
	cd brackets
	echo 'asd' > 'bra[ckets].txt'
	git add 'bra[ckets].txt'
	git commit -m initial
	git show 'HEAD:bra[ckets].txt’

Nope. This command sequence doesn’t work for me: the same error is returned:

    # git show 'HEAD:bra[ckets].txt'
    fatal: ambiguous argument 'HEAD:bra[ckets].txt': both revision and filename

You could also escape the brackets with a backslash, as you did, but you
would have to do it *every* time you write the path, not just in the `git
add` incantation.

As I mentioned at the end of my original message, escaping doesn't help either. `git add` works fine both with and without escape. It was auto-completed by bash completion, and I just forgot to remove the backslashes before pasting the code here. At any case, escaping doesn’t work with `git show`.

Hi Kirill,

On Sat, 6 Feb 2016, Kirill Likhodedov wrote:

quoted

On 06 Feb 2016, at 17:21 , Johannes Schindelin [off-list ref] wrote:

This is expected behavior of the Bash you are using. The commands that I
think would reflect your intentions would be:

	git init brackets
	cd brackets
	echo 'asd' > 'bra[ckets].txt'
	git add 'bra[ckets].txt'
	git commit -m initial
	git show 'HEAD:bra[ckets].txt’

Nope. This command sequence doesn’t work for me: the same error is returned:

    # git show 'HEAD:bra[ckets].txt'
    fatal: ambiguous argument 'HEAD:bra[ckets].txt': both revision and filename

Whoops. Sorry. I actually ran those commands now and it is true that it
still does not work, which is funny. Especially since

	git show 'HEAD:bra[ckets].txt' --

actually *does* work.

Ciao,
Johannes

On Sat, Feb 6, 2016 at 11:10 PM, Johannes Schindelin
[off-list ref] wrote:

Hi Kirill,

On Sat, 6 Feb 2016, Kirill Likhodedov wrote:

quoted

quoted

On 06 Feb 2016, at 17:21 , Johannes Schindelin [off-list ref] wrote:

This is expected behavior of the Bash you are using. The commands that I
think would reflect your intentions would be:

    git init brackets
    cd brackets
    echo 'asd' > 'bra[ckets].txt'
    git add 'bra[ckets].txt'
    git commit -m initial
    git show 'HEAD:bra[ckets].txt’

Nope. This command sequence doesn’t work for me: the same error is returned:

    # git show 'HEAD:bra[ckets].txt'
    fatal: ambiguous argument 'HEAD:bra[ckets].txt': both revision and filename

Whoops. Sorry. I actually ran those commands now and it is true that it
still does not work, which is funny. Especially since

        git show 'HEAD:bra[ckets].txt' --

actually *does* work.

It's from 28fcc0b (pathspec: avoid the need of "--" when wildcard is
used - 2015-05-02)
-- 
Duy

Hi Johannes,

On 06 Feb 2016, at 19:10 , Johannes Schindelin [off-list ref] wrote:

	git show 'HEAD:bra[ckets].txt' --

Nice catch! It works for me even without quotes. Although this “--“ is mentioned in the error message, I didn’t even try since its meaning is totally unrelated with the problem ;) Anyway, thanks a lot for finding the workaround.

Hi Duy,

It's from 28fcc0b (pathspec: avoid the need of "--" when wildcard is
used - 2015-05-02)

v2.5.0 is the first release which contains 28fcc0b.
I can confirm that older versions of Git work correctly without “--“:

# /opt/local/bin/git version
git version 1.7.1.1
# /opt/local/bin/git show HEAD:bra[ckets].txt 
asd

Looks like a regression?

Hi Kirill,

On Sun, 7 Feb 2016, Kirill Likhodedov wrote:

quoted

On 06 Feb 2016, at 19:10 , Johannes Schindelin [off-list ref] wrote:

	git show 'HEAD:bra[ckets].txt' --

Nice catch! It works for me even without quotes.

Only by chance. Once you have a HEAD:brat.txt file in the current working
directory, it will break.

Anyway, thanks a lot for finding the workaround.

I would not exactly call this a work-around, but a precise way to specify
that you are *not* talking about a file.

Ciao,
Johannes

On Sun, Feb 7, 2016 at 10:11 PM, Kirill Likhodedov
[off-list ref] wrote:

Hi Duy,

quoted

It's from 28fcc0b (pathspec: avoid the need of "--" when wildcard is
used - 2015-05-02)

v2.5.0 is the first release which contains 28fcc0b.
I can confirm that older versions of Git work correctly without “--“:

# /opt/local/bin/git version
git version 1.7.1.1
# /opt/local/bin/git show HEAD:bra[ckets].txt
asd

Looks like a regression?

No it's a deliberate trade-off. With that change, you can use
wildcards in pathspec without "--" (e.g. "git log 'a*'" instead of
"git log -- 'a*'"). And I still believe that happens a lot more often
than this case. Putting "--" is _the_ way to avoid ambiguation when
git fails to do it properly. Though in future we may make git smarter
at solving ambiguation (e.g. it could do glob() to test if a wildcard
pattern matches any path).
-- 
Duy

On Mon, Feb 08, 2016 at 12:06:44PM +0700, Duy Nguyen wrote:

On Sun, Feb 7, 2016 at 10:11 PM, Kirill Likhodedov
[off-list ref] wrote:

quoted

Hi Duy,

quoted

It's from 28fcc0b (pathspec: avoid the need of "--" when wildcard is
used - 2015-05-02)

v2.5.0 is the first release which contains 28fcc0b.
I can confirm that older versions of Git work correctly without “--“:

# /opt/local/bin/git version
git version 1.7.1.1
# /opt/local/bin/git show HEAD:bra[ckets].txt
asd

Looks like a regression?

No it's a deliberate trade-off. With that change, you can use
wildcards in pathspec without "--" (e.g. "git log 'a*'" instead of
"git log -- 'a*'"). And I still believe that happens a lot more often
than this case. Putting "--" is _the_ way to avoid ambiguation when
git fails to do it properly. Though in future we may make git smarter
at solving ambiguation (e.g. it could do glob() to test if a wildcard
pattern matches any path).

It's still sort-of a regression; we changed the rule and now things that
used to work don't. Using "--" is a good protection, but people who
didn't have to use "--" in some cases now do.

I wonder if we could fix this pretty simply, though, by skipping the
"does it have a wildcard" check when we see a colon in the path. That is
a good indication that we are using one of git's special rev syntaxes
(either "tree:path", or ":path", or ":/search string". That breaks
anybody who really wanted to look for "path:with:colons.*", but that
seems a lot less likely to me.

It doesn't cover:

  git log 'HEAD^{/Merge.*}'

which is similarly affected by 28fcc0b. Perhaps "^{" should be such a
magic string, as well. We can be liberal with such strings as they are
really just limiting the impact of 28fcc0b; we would fall back in those
cases to the usual "can it be resolved, or is it a path?" rule.

-Peff

On Mon, Feb 08, 2016 at 09:15:52AM -0500, Jeff King wrote:

I wonder if we could fix this pretty simply, though, by skipping the
"does it have a wildcard" check when we see a colon in the path. That is
a good indication that we are using one of git's special rev syntaxes
(either "tree:path", or ":path", or ":/search string". That breaks
anybody who really wanted to look for "path:with:colons.*", but that
seems a lot less likely to me.

Actually, I guess:

  :/foo

does have a meaning as a pathspec (though again, this is only about
limiting the wildcard case, so I think that's OK). More worrisome would
be:

  :(literal)[brackets]

which is almost certainly a pathspec.

So I guess I would revise my suggestion to: we could probably do a lot
better (but not perfectly, of course) by guessing at basic syntactic
components.

-Peff

On Mon, Feb 08, 2016 at 09:15:52AM -0500, Jeff King wrote:

I wonder if we could fix this pretty simply, though, by skipping the
"does it have a wildcard" check when we see a colon in the path. That is
a good indication that we are using one of git's special rev syntaxes
(either "tree:path", or ":path", or ":/search string". That breaks
anybody who really wanted to look for "path:with:colons.*", but that
seems a lot less likely to me.

It doesn't cover:

  git log 'HEAD^{/Merge.*}'

which is similarly affected by 28fcc0b. Perhaps "^{" should be such a
magic string, as well. We can be liberal with such strings as they are
really just limiting the impact of 28fcc0b; we would fall back in those
cases to the usual "can it be resolved, or is it a path?" rule.

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 (whereas if we actually tried to glob each pathspec, that would
keep the rule simple and work well in practice; I'm not sure how easy
that it is to do, though, if we are dealing with things like :(magic)
pathspecs, but maybe we should simply be dealing with them syntactically
much earlier).

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).

-- >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.

Signed-off-by: Jeff King <redacted>
---
 setup.c                      | 21 ++++++++++++++++++++-
 t/t6133-pathspec-rev-dwim.sh | 44 ++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 64 insertions(+), 1 deletion(-)
 create mode 100755 t/t6133-pathspec-rev-dwim.sh

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

@@ -130,6 +130,25 @@ int path_inside_repo(const char *prefix, const char *path)
 	return 0;
 }
 
+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;
+
+	return 1;
+}
+
 int check_filename(const char *prefix, const char *arg)
 {
 	const char *name;

@@ -139,7 +158,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/t6133-pathspec-rev-dwim.sh b/t/t6133-pathspec-rev-dwim.sh
new file mode 100755
index 0000000..8f68937
--- /dev/null
+++ b/t/t6133-pathspec-rev-dwim.sh

@@ -0,0 +1,44 @@
+#!/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 ':(magic) dwims to pathspec' '
+	git log -- ":(literal)br[ack]ets" >expect &&
+	git log    ":(literal)br[ack]ets" >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