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

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

[Kirill Likhodedov]

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

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

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 815 bytes --]

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

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

[Kirill Likhodedov]

Hi Johannes,

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

> On 06 Feb 2016, at 17:21 , Johannes Schindelin <Johannes.Schindelin@gmx.de> 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`.

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

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 894 bytes --]

Hi Kirill,

On Sat, 6 Feb 2016, Kirill Likhodedov wrote:

> > On 06 Feb 2016, at 17:21 , Johannes Schindelin <Johannes.Schindelin@gmx.de> 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

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

[Duy Nguyen]

On Sat, Feb 6, 2016 at 11:10 PM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> Hi Kirill,
>
> On Sat, 6 Feb 2016, Kirill Likhodedov wrote:
>
>> > On 06 Feb 2016, at 17:21 , Johannes Schindelin <Johannes.Schindelin@gmx.de> 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

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

[Kirill Likhodedov]

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?

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

[Duy Nguyen]

On Sun, Feb 7, 2016 at 10:11 PM, Kirill Likhodedov
<kirill.likhodedov@jetbrains.com> wrote:
> 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?

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

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

[Jeff King]

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

> On Sun, Feb 7, 2016 at 10:11 PM, Kirill Likhodedov
> <kirill.likhodedov@jetbrains.com> wrote:
> > 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?
> 
> 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

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

[Jeff King]

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

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

[Jeff King]

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 <peff@peff.net>
---
 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

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

[Junio C Hamano]

Jeff King <peff@peff.net> 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.

> 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 <peff@peff.net>
> ---
>  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;

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.

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

[Jeff King]

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.

> > +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 <peff@peff.net>
---
 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

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

[Jeff King]

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)

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

[Jeff King]

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

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

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

Yeah, checking in the index, once it is loaded, is reasonably quick
check.  A path that is not in the index or the current HEAD may or
may not exist on the filesystem, so at some point you would need an
explicit disambiguation anyway, and the reason why we check the
filesystem is not because that is conceptually better than checking
in the index but merely because "does lstat(2) tell us the path is
there?" check was fairly a cheap way on the platform the system was
primarily developed on initially.  Looking it up from HEAD would be
a lot more heavyweight and would not buy us anything, but looking it
up in the index may turn out to be comparable to a single lstat(2).

I dunno.  I have a suspicion that anything conceptually more
expensive than a single lstat(2) is probably not worth doing, as
this "sometimes you do not have to give --" is merely a usability
hack, and we have to always do "git log -- removed-sometime-ago"
to find where in the history a certain path was lost.

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

[Jeff King]

On Mon, Feb 08, 2016 at 02:36:32PM -0800, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > 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.
> 
> Yeah, checking in the index, once it is loaded, is reasonably quick
> check.  A path that is not in the index or the current HEAD may or
> may not exist on the filesystem, so at some point you would need an
> explicit disambiguation anyway, and the reason why we check the
> filesystem is not because that is conceptually better than checking
> in the index but merely because "does lstat(2) tell us the path is
> there?" check was fairly a cheap way on the platform the system was
> primarily developed on initially.  Looking it up from HEAD would be
> a lot more heavyweight and would not buy us anything, but looking it
> up in the index may turn out to be comparable to a single lstat(2).

Yeah, I had a notion that looking in the index would not be all that
expensive, since we often load it anyway. But this _is_ "git log" we are
talking about, which does not otherwise need to read the index at all. I
suspect lstat(2) is way faster if you have a huge repo, as it is should
be constant-ish, as opposed to O(size-of-index).

> I dunno.  I have a suspicion that anything conceptually more
> expensive than a single lstat(2) is probably not worth doing, as
> this "sometimes you do not have to give --" is merely a usability
> hack, and we have to always do "git log -- removed-sometime-ago"
> to find where in the history a certain path was lost.

Yeah, it just seemed a shame to me that things which clearly _aren't_
ambiguous to any sane viewer would be reported as such by git. I think
the "--" DWIM has worked so well precisely because people do not have
silly-named files in their repositories, so it Just Works most of the
time. The wildcard rule switches it from "you put a file named HEAD in
your repository, now you pay the price for being silly" to "whoops,
everything with a metacharacter is now ambiguous".

-Peff

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> 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 <peff@peff.net>

I was leaning towards merging this version, but I became unsure
while writing an entry for "What's cooking" (which will be used as a
merge summary message and then will appear in the Release Notes).

We would surely want

    $ git log ':/tighten.*'

to find this commit, not take it as a pathspec.  But running

    $ git log ':/*.c'

in a subdirectory to find commits that touched any .c file, taking
it as a pathspec, would equally be a sensible thing to want.

I would feel that we should require "--" for both cases; with or
without this patch, we already treat these as revs without "--",
making the latter fail without "--".  Also:

    $ git log "HEAD^{/tighten.*}"

is already dwimmed as a rev.

And a path with glob(3) metacharacters is an insane thing, be it
inside a treeish or in the working tree, and I think it is OK to
require users to explicitly say what they mean with "--".

And the patch does not leave much if we ignore that ":" bit.
With the patch, "HEAD@{now [or thereabouts]}" will be taken as a
rev without "--", which is an improvement, but to me that seems
to be the only improvement this change brings us.

And I do not think we want either glob(3) or fill_directory() to
slow things down, as this is merely a heuristic.

We may want to rethink the interface into check_filename().  The
callers of this function that try to help users who did not use "--"
want the function to say "It is likely that this was meant as a
pathname" and when this function says "No, the user did not mean it
as a filename." they will in turn ask the revision parser "Is this a
rev?".  At that point, if it is not a revision, these callers can
say "Not a file, not a rev" and die.

In order to allow "':/tighten.*' is a rev, ':/*.c' is a pathspec,
they are equally likely and you must disambiguate", the current
interface is inadequate.

 * check_filename() cannot say "No, it is not a filename"--a later
   call to get_sha1() will barf on ":/*.c" saying that it is not a
   rev, but the fault is in Git that initially guessed it would be a
   rev when the user meant it as a pathspec.

 * The function cannot say "Yes, it is a filename"--then get_sha1()
   will not be called for ":/tighten.*" and we would silently use it
   as pathspec, possibly producing an empty result.

There needs to be a way for it to say "I refuse to disambiguate".

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

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

[Jeff King]

On Tue, Feb 09, 2016 at 12:37:32PM -0800, Junio C Hamano wrote:

> I was leaning towards merging this version, but I became unsure
> while writing an entry for "What's cooking" (which will be used as a
> merge summary message and then will appear in the Release Notes).
> 
> We would surely want
> 
>     $ git log ':/tighten.*'
> 
> to find this commit, not take it as a pathspec.  But running
> 
>     $ git log ':/*.c'
> 
> in a subdirectory to find commits that touched any .c file, taking
> it as a pathspec, would equally be a sensible thing to want.
> 
> I would feel that we should require "--" for both cases; with or
> without this patch, we already treat these as revs without "--",
> making the latter fail without "--".

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.

> Also:
> 
>     $ git log "HEAD^{/tighten.*}"
> 
> is already dwimmed as a rev.

Is it? That is the exact case I think regressed in v2.5.0, because it
clearly _is_ a rev, didn't previously require a "--", and now does:

  $ git.v2.4.8 log --oneline -1 'HEAD^{/tighten.*}'
  913c2c7 Merge branch 'jk/sanity' into maint

  $ git.v2.5.0 log --oneline -1 'HEAD^{/tighten.*}'
  fatal: ambiguous argument 'HEAD^{/tighten.*}': both revision and filename
  Use '--' to separate paths from revisions, like this:
  'git <command> [<revision>...] -- [<file>...]'

And that's what my patch is trying to do: pull back the over-broad match
in 28fcc0b for cases that are pretty clearly revs.

> And a path with glob(3) metacharacters is an insane thing, be it
> inside a treeish or in the working tree, and I think it is OK to
> require users to explicitly say what they mean with "--".

Yeah, I can buy that line of reasoning.

> We may want to rethink the interface into check_filename().  The
> callers of this function that try to help users who did not use "--"
> want the function to say "It is likely that this was meant as a
> pathname" and when this function says "No, the user did not mean it
> as a filename." they will in turn ask the revision parser "Is this a
> rev?".  At that point, if it is not a revision, these callers can
> say "Not a file, not a rev" and die.
> 
> In order to allow "':/tighten.*' is a rev, ':/*.c' is a pathspec,
> they are equally likely and you must disambiguate", the current
> interface is inadequate.

Hmm. I think at least for the revision-parser it is the other way
around. We actually check first "is it a rev". If it is, and there is no
"--", then we ask "could it also be a filename" and complain if so. If
the revision parser says "no, it cannot be", then we say "well, could it
plausibly be a filename"?

And both of those use the same check_filename() test, but I think they
are asking two different things. The first one probably wants to say "is
it definitely a filename, because if so, that's ambiguous". And it would
be OK to look at a wildcard and say "sure, it _could_ be a path, but
taking it as a rev is reasonable". IOW, to err on the side of "not a
filename", and allow something possibly ambiguous. And then the second
test is the opposite; we know it's not a rev, so if it could plausibly
be a file, then take it as one.

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

I'll note also two things:

  1. I've _just_ looked at revision.c here; there are other callers of
     verify_filename and verify_non_filename (and even a bare
     check_filename() call!) that may not all have the same needs.

  2. The ":/*.c" case is much more complicated. Before we get to
     check_filename() at all, we use get_sha1(), which says "yes, this
     is a pattern". And then barfs with a fatal error when it sees that
     "*.c" is not a valid regex. So no matter what check_filename()
     does, we would have to propagate that error from get_sha1() to make
     anything interesting work there.

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

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);
 }

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

[Junio C Hamano]

Jeff King <peff@peff.net> 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 ;-)

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

> 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);
>  }

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

[Jeff King]

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

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

[PATCH 1/3] checkout: reorder check_filename conditional

[Jeff King]

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 <peff@peff.net>
---
 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

Re: [PATCH 1/3] checkout: reorder check_filename conditional

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

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

It also allows check_filename() to die(), and lets the user to
prevent it with "--"---"Don't check when we do not have to" is the
right thing to do.

Thanks.

> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  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 --"

[PATCH 2/3] check_filename: tighten dwim-wildcard ambiguity

[Jeff King]

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

[PATCH 3/3] get_sha1: don't die() on bogus search strings

[Jeff King]

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 <peff@peff.net>
---
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

Re: [PATCH 3/3] get_sha1: don't die() on bogus search strings

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> 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

Slight nit.  It means "'*.t at anywhere in the tree" aka "pretend as
if you gave this pathspec while at the root level of the working
tree".

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

I do not think the latter is necessarily worse, though.  It is being
consistent with these:

    $ git log mext --			;# no such branch
    fatal: bad revision 'mext'
    $ git log ':/xxxt' --		;# no commit matches that pattern
    fatal: bad revision ':/xxxt'

so I would not even mind if somebody argued that the current
"invalid search pattern" is a bug, and gave us this patch as a fix
for the inconsistency.

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

I am on the fence, too, and part of me wants to say the same thing.
I however happen to view the "practical tradeoff" a bit differently,
so I am slightly inclined to take this.

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

Yeah, I agree it probably would not make much practical difference.

>  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

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

[Kirill Likhodedov]

Hi Johannes,

> On 06 Feb 2016, at 19:10 , Johannes Schindelin <Johannes.Schindelin@gmx.de> 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.

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

[Johannes Schindelin]

Hi Kirill,

On Sun, 7 Feb 2016, Kirill Likhodedov wrote:

> > On 06 Feb 2016, at 19:10 , Johannes Schindelin <Johannes.Schindelin@gmx.de> 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