{"thread":{"id":"41356","subject":"git show doesn't work on file names with square brackets","startedAt":"2016-02-06T13:16:35Z","lastAt":"2016-02-10T21:52:51Z","messageCount":27,"participants":["Kirill Likhodedov","Johannes Schindelin","Duy Nguyen","Jeff King","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"277650","messageId":"6A7D4447-AC25-4591-9DA7-CD153198EC64@jetbrains.com","threadId":"41356","inReplyTo":null,"subject":"git show doesn't work on file names with square brackets","fromName":"Kirill Likhodedov","fromEmail":"kirill.likhodedov@jetbrains.com","sentAt":"2016-02-06T13:16:35Z","receivedAt":"2016-02-06T13:16:35Z","isPatch":false,"sender":{"key":"kirill.likhodedov@jetbrains.com","avatar":"https://gravatar.com/avatar/9fd55d2a110e8e96deb3ed7b503d7f3b9b5b1ed5ca9ef6683c15525b11b94e63?d=mp&s=160"},"body":"I’ve faced a problem that `git show :` returns an error when contains square brackets.\n\nInterestingly, 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.\n\nIs it a bug or I just didn’t find the proper way to escape the brackets? \n\nSteps to reproduce:\n\n git init brackets\n cd brackets/\n echo ‘asd’ > bra[ckets].txt\n git add bra\\[ckets\\].txt\n git commit -m initial\n git show HEAD:bra[ckets].txt\n\nError:\nfatal: ambiguous argument 'HEAD:bra[ckets].txt': both revision and filename\nUse '--' to separate paths from revisions, like this:\n'git [...] -- [...]’\n\nNeither escaping, not quoting doesn’t help:\n git show HEAD:bra\\[ckets\\].txt\nreturns the same error\n\n git show \"HEAD:bra\\[ckets\\].txt”\nreturns empty output\n\nThanks a lot!\n-- Kirill"},{"id":"277653","messageId":"alpine.DEB.2.20.1602061518220.2964@virtualbox","threadId":"41356","inReplyTo":"6A7D4447-AC25-4591-9DA7-CD153198EC64@jetbrains.com","subject":"Re: git show doesn't work on file names with square brackets","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-02-06T14:21:02Z","receivedAt":"2016-02-06T14:21:02Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Kirill,\n\nOn Sat, 6 Feb 2016, Kirill Likhodedov wrote:\n\n> Is it a bug or I just didn’t find the proper way to escape the brackets? \n> \n> Steps to reproduce:\n> \n> git init brackets\n> cd brackets/\n> echo ‘asd’ > bra[ckets].txt\n> git add bra\\[ckets\\].txt\n> git commit -m initial\n> git show HEAD:bra[ckets].txt\n\nThis is expected behavior of the Bash you are using. The commands that I\nthink would reflect your intentions would be:\n\n\tgit init brackets\n\tcd brackets\n\techo 'asd' > 'bra[ckets].txt'\n\tgit add 'bra[ckets].txt'\n\tgit commit -m initial\n\tgit show 'HEAD:bra[ckets].txt'\n\nYou could also escape the brackets with a backslash, as you did, but you\nwould have to do it *every* time you write the path, not just in the `git\nadd` incantation.\n\nCiao,\nJohannes"},{"id":"277654","messageId":"25D155FA-6F05-425C-AB2D-7F0B44E0D1C5@jetbrains.com","threadId":"41356","inReplyTo":"alpine.DEB.2.20.1602061518220.2964@virtualbox","subject":"Re: git show doesn't work on file names with square brackets","fromName":"Kirill Likhodedov","fromEmail":"kirill.likhodedov@jetbrains.com","sentAt":"2016-02-06T14:29:20Z","receivedAt":"2016-02-06T14:29:20Z","isPatch":false,"sender":{"key":"kirill.likhodedov@jetbrains.com","avatar":"https://gravatar.com/avatar/9fd55d2a110e8e96deb3ed7b503d7f3b9b5b1ed5ca9ef6683c15525b11b94e63?d=mp&s=160"},"body":"Hi Johannes,\n\nthanks for your answer, but unfortunately it doesn’t help.\n\n> On 06 Feb 2016, at 17:21 , Johannes Schindelin wrote:\n> \n> This is expected behavior of the Bash you are using. The commands that I\n> think would reflect your intentions would be:\n> \n> \tgit init brackets\n> \tcd brackets\n> \techo 'asd' > 'bra[ckets].txt'\n> \tgit add 'bra[ckets].txt'\n> \tgit commit -m initial\n> \tgit show 'HEAD:bra[ckets].txt’\n\n\nNope. This command sequence doesn’t work for me: the same error is returned:\n\n # git show 'HEAD:bra[ckets].txt'\n fatal: ambiguous argument 'HEAD:bra[ckets].txt': both revision and filename\n\n\n> You could also escape the brackets with a backslash, as you did, but you\n> would have to do it *every* time you write the path, not just in the `git\n> add` incantation.\n\n\nAs 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`."},{"id":"277662","messageId":"alpine.DEB.2.20.1602061708220.2964@virtualbox","threadId":"41356","inReplyTo":"25D155FA-6F05-425C-AB2D-7F0B44E0D1C5@jetbrains.com","subject":"Re: git show doesn't work on file names with square brackets","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-02-06T16:10:54Z","receivedAt":"2016-02-06T16:10:54Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Kirill,\n\nOn Sat, 6 Feb 2016, Kirill Likhodedov wrote:\n\n> > On 06 Feb 2016, at 17:21 , Johannes Schindelin wrote:\n> > \n> > This is expected behavior of the Bash you are using. The commands that I\n> > think would reflect your intentions would be:\n> > \n> > \tgit init brackets\n> > \tcd brackets\n> > \techo 'asd' > 'bra[ckets].txt'\n> > \tgit add 'bra[ckets].txt'\n> > \tgit commit -m initial\n> > \tgit show 'HEAD:bra[ckets].txt’\n> \n> \n> Nope. This command sequence doesn’t work for me: the same error is returned:\n> \n> # git show 'HEAD:bra[ckets].txt'\n> fatal: ambiguous argument 'HEAD:bra[ckets].txt': both revision and filename\n\nWhoops. Sorry. I actually ran those commands now and it is true that it\nstill does not work, which is funny. Especially since\n\n\tgit show 'HEAD:bra[ckets].txt' --\n\nactually *does* work.\n\nCiao,\nJohannes"},{"id":"277671","messageId":"CACsJy8ChZzYWXePSwF6D8vPZMuz3dQe1=jtw6rSG7M1oC+RiNw@mail.gmail.com","threadId":"41356","inReplyTo":"alpine.DEB.2.20.1602061708220.2964@virtualbox","subject":"Re: git show doesn't work on file names with square brackets","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2016-02-06T23:48:39Z","receivedAt":"2016-02-06T23:48:39Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sat, Feb 6, 2016 at 11:10 PM, Johannes Schindelin\n wrote:\n> Hi Kirill,\n>\n> On Sat, 6 Feb 2016, Kirill Likhodedov wrote:\n>\n>> > On 06 Feb 2016, at 17:21 , Johannes Schindelin wrote:\n>> >\n>> > This is expected behavior of the Bash you are using. The commands that I\n>> > think would reflect your intentions would be:\n>> >\n>> > git init brackets\n>> > cd brackets\n>> > echo 'asd' > 'bra[ckets].txt'\n>> > git add 'bra[ckets].txt'\n>> > git commit -m initial\n>> > git show 'HEAD:bra[ckets].txt’\n>>\n>>\n>> Nope. This command sequence doesn’t work for me: the same error is returned:\n>>\n>> # git show 'HEAD:bra[ckets].txt'\n>> fatal: ambiguous argument 'HEAD:bra[ckets].txt': both revision and filename\n>\n> Whoops. Sorry. I actually ran those commands now and it is true that it\n> still does not work, which is funny. Especially since\n>\n> git show 'HEAD:bra[ckets].txt' --\n>\n> actually *does* work.\n\nIt's from 28fcc0b (pathspec: avoid the need of \"--\" when wildcard is\nused - 2015-05-02)\n-- \nDuy\n"},{"id":"277694","messageId":"CABCFD40-C1B8-412C-90E3-24147AA6AFA5@jetbrains.com","threadId":"41356","inReplyTo":"alpine.DEB.2.20.1602061708220.2964@virtualbox","subject":"Re: git show doesn't work on file names with square brackets","fromName":"Kirill Likhodedov","fromEmail":"kirill.likhodedov@jetbrains.com","sentAt":"2016-02-07T15:09:35Z","receivedAt":"2016-02-07T15:09:35Z","isPatch":false,"sender":{"key":"kirill.likhodedov@jetbrains.com","avatar":"https://gravatar.com/avatar/9fd55d2a110e8e96deb3ed7b503d7f3b9b5b1ed5ca9ef6683c15525b11b94e63?d=mp&s=160"},"body":"Hi Johannes,\n\n> On 06 Feb 2016, at 19:10 , Johannes Schindelin wrote:\n> \n> \tgit show 'HEAD:bra[ckets].txt' --\n> \n\nNice 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."},{"id":"277695","messageId":"32B9BD70-F06C-49C4-B672-24173E69B99F@jetbrains.com","threadId":"41356","inReplyTo":"CACsJy8ChZzYWXePSwF6D8vPZMuz3dQe1=jtw6rSG7M1oC+RiNw@mail.gmail.com","subject":"Re: git show doesn't work on file names with square brackets","fromName":"Kirill Likhodedov","fromEmail":"kirill.likhodedov@jetbrains.com","sentAt":"2016-02-07T15:11:34Z","receivedAt":"2016-02-07T15:11:34Z","isPatch":false,"sender":{"key":"kirill.likhodedov@jetbrains.com","avatar":"https://gravatar.com/avatar/9fd55d2a110e8e96deb3ed7b503d7f3b9b5b1ed5ca9ef6683c15525b11b94e63?d=mp&s=160"},"body":"Hi Duy,\n\n> It's from 28fcc0b (pathspec: avoid the need of \"--\" when wildcard is\n> used - 2015-05-02)\n\nv2.5.0 is the first release which contains 28fcc0b.\nI can confirm that older versions of Git work correctly without “--“:\n\n# /opt/local/bin/git version\ngit version 1.7.1.1\n# /opt/local/bin/git show HEAD:bra[ckets].txt \nasd\n\nLooks like a regression?"},{"id":"277698","messageId":"alpine.DEB.2.20.1602071809440.2964@virtualbox","threadId":"41356","inReplyTo":"CABCFD40-C1B8-412C-90E3-24147AA6AFA5@jetbrains.com","subject":"Re: git show doesn't work on file names with square brackets","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-02-07T17:10:55Z","receivedAt":"2016-02-07T17:10:55Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Kirill,\n\nOn Sun, 7 Feb 2016, Kirill Likhodedov wrote:\n\n> > On 06 Feb 2016, at 19:10 , Johannes Schindelin wrote:\n> > \n> > \tgit show 'HEAD:bra[ckets].txt' --\n> > \n> \n> Nice catch! It works for me even without quotes.\n\nOnly by chance. Once you have a HEAD:brat.txt file in the current working\ndirectory, it will break.\n\n> Anyway, thanks a lot for finding the workaround.\n\nI would not exactly call this a work-around, but a precise way to specify\nthat you are *not* talking about a file.\n\nCiao,\nJohannes\n"},{"id":"277711","messageId":"CACsJy8AMEgk8UXF==VmvLXsL4R67u0+U4MiUGPtO6HX0Y30oXg@mail.gmail.com","threadId":"41356","inReplyTo":"32B9BD70-F06C-49C4-B672-24173E69B99F@jetbrains.com","subject":"Re: git show doesn't work on file names with square brackets","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2016-02-08T05:06:44Z","receivedAt":"2016-02-08T05:06:44Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sun, Feb 7, 2016 at 10:11 PM, Kirill Likhodedov\n wrote:\n> Hi Duy,\n>\n>> It's from 28fcc0b (pathspec: avoid the need of \"--\" when wildcard is\n>> used - 2015-05-02)\n>\n> v2.5.0 is the first release which contains 28fcc0b.\n> I can confirm that older versions of Git work correctly without “--“:\n>\n> # /opt/local/bin/git version\n> git version 1.7.1.1\n> # /opt/local/bin/git show HEAD:bra[ckets].txt\n> asd\n>\n> Looks like a regression?\n\nNo it's a deliberate trade-off. With that change, you can use\nwildcards in pathspec without \"--\" (e.g. \"git log 'a*'\" instead of\n\"git log -- 'a*'\"). And I still believe that happens a lot more often\nthan this case. Putting \"--\" is _the_ way to avoid ambiguation when\ngit fails to do it properly. Though in future we may make git smarter\nat solving ambiguation (e.g. it could do glob() to test if a wildcard\npattern matches any path).\n-- \nDuy\n"},{"id":"277731","messageId":"20160208141552.GC27054@sigill.intra.peff.net","threadId":"41356","inReplyTo":"CACsJy8AMEgk8UXF==VmvLXsL4R67u0+U4MiUGPtO6HX0Y30oXg@mail.gmail.com","subject":"Re: git show doesn't work on file names with square brackets","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-02-08T14:15:53Z","receivedAt":"2016-02-08T14:15:53Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 08, 2016 at 12:06:44PM +0700, Duy Nguyen wrote:\n\n> On Sun, Feb 7, 2016 at 10:11 PM, Kirill Likhodedov\n> wrote:\n> > Hi Duy,\n> >\n> >> It's from 28fcc0b (pathspec: avoid the need of \"--\" when wildcard is\n> >> used - 2015-05-02)\n> >\n> > v2.5.0 is the first release which contains 28fcc0b.\n> > I can confirm that older versions of Git work correctly without “--“:\n> >\n> > # /opt/local/bin/git version\n> > git version 1.7.1.1\n> > # /opt/local/bin/git show HEAD:bra[ckets].txt\n> > asd\n> >\n> > Looks like a regression?\n> \n> No it's a deliberate trade-off. With that change, you can use\n> wildcards in pathspec without \"--\" (e.g. \"git log 'a*'\" instead of\n> \"git log -- 'a*'\"). And I still believe that happens a lot more often\n> than this case. Putting \"--\" is _the_ way to avoid ambiguation when\n> git fails to do it properly. Though in future we may make git smarter\n> at solving ambiguation (e.g. it could do glob() to test if a wildcard\n> pattern matches any path).\n\nIt's still sort-of a regression; we changed the rule and now things that\nused to work don't. Using \"--\" is a good protection, but people who\ndidn't have to use \"--\" in some cases now do.\n\nI wonder if we could fix this pretty simply, though, by skipping the\n\"does it have a wildcard\" check when we see a colon in the path. That is\na good indication that we are using one of git's special rev syntaxes\n(either \"tree:path\", or \":path\", or \":/search string\". That breaks\nanybody who really wanted to look for \"path:with:colons.*\", but that\nseems a lot less likely to me.\n\nIt doesn't cover:\n\n git log 'HEAD^{/Merge.*}'\n\nwhich is similarly affected by 28fcc0b. Perhaps \"^{\" should be such a\nmagic string, as well. We can be liberal with such strings as they are\nreally just limiting the impact of 28fcc0b; we would fall back in those\ncases to the usual \"can it be resolved, or is it a path?\" rule.\n\n-Peff\n"},{"id":"277732","messageId":"20160208142439.GA8262@sigill.intra.peff.net","threadId":"41356","inReplyTo":"20160208141552.GC27054@sigill.intra.peff.net","subject":"Re: git show doesn't work on file names with square brackets","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-02-08T14:24:40Z","receivedAt":"2016-02-08T14:24:40Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 08, 2016 at 09:15:52AM -0500, Jeff King wrote:\n\n> I wonder if we could fix this pretty simply, though, by skipping the\n> \"does it have a wildcard\" check when we see a colon in the path. That is\n> a good indication that we are using one of git's special rev syntaxes\n> (either \"tree:path\", or \":path\", or \":/search string\". That breaks\n> anybody who really wanted to look for \"path:with:colons.*\", but that\n> seems a lot less likely to me.\n\nActually, I guess:\n\n :/foo\n\ndoes have a meaning as a pathspec (though again, this is only about\nlimiting the wildcard case, so I think that's OK). More worrisome would\nbe:\n\n :(literal)[brackets]\n\nwhich is almost certainly a pathspec.\n\nSo I guess I would revise my suggestion to: we could probably do a lot\nbetter (but not perfectly, of course) by guessing at basic syntactic\ncomponents.\n\n-Peff\n"},{"id":"277735","messageId":"20160208150709.GA13664@sigill.intra.peff.net","threadId":"41356","inReplyTo":"20160208141552.GC27054@sigill.intra.peff.net","subject":"Re: git show doesn't work on file names with square brackets","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-02-08T15:07:09Z","receivedAt":"2016-02-08T15:07:09Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 08, 2016 at 09:15:52AM -0500, Jeff King wrote:\n\n> I wonder if we could fix this pretty simply, though, by skipping the\n> \"does it have a wildcard\" check when we see a colon in the path. That is\n> a good indication that we are using one of git's special rev syntaxes\n> (either \"tree:path\", or \":path\", or \":/search string\". That breaks\n> anybody who really wanted to look for \"path:with:colons.*\", but that\n> seems a lot less likely to me.\n> \n> It doesn't cover:\n> \n> git log 'HEAD^{/Merge.*}'\n> \n> which is similarly affected by 28fcc0b. Perhaps \"^{\" should be such a\n> magic string, as well. We can be liberal with such strings as they are\n> really just limiting the impact of 28fcc0b; we would fall back in those\n> cases to the usual \"can it be resolved, or is it a path?\" rule.\n\nThe patch for that might look like this. I like it for its relative\nsimplicity, though it does make the rules even harder to explain to a\nuser (whereas if we actually tried to glob each pathspec, that would\nkeep the rule simple and work well in practice; I'm not sure how easy\nthat it is to do, though, if we are dealing with things like :(magic)\npathspecs, but maybe we should simply be dealing with them syntactically\nmuch earlier).\n\nThis breaks the second test in t2019 added by ae454f6, but I am not sure\nthat test is doing the right thing (I'm also not sure t2019 is the best\nplace for these tests; I added new ones here in a separate script).\n\n-- >8 --\nSubject: [PATCH] check_filename: tighten requirements for dwim-wildcards\n\nCommit 28fcc0b (pathspec: avoid the need of \"--\" when\nwildcard is used, 2015-05-02) introduced a convenience to\nour dwim-parsing: when \"--\" is not present, we guess that\nitems with wildcard characters are probably pathspecs.\n\nThis makes a lot of cases simpler (e.g., \"git log '*.c'\"),\nbut makes others harder. While revision expressions do not\ntypically have wildcard characters in them (because they are\nnot valid in refnames), there are a few constructs where we\ntake more arbitrary strings, such as:\n\n - pathnames in tree:path syntax (or :0:path) for the\n index)\n\n - :/foo and ^{/foo} for searching commit messages;\n likewise \"^{}\" is extensible and may learn new formats\n in the future\n\n - @{foo}, which can take arbitrary approxidate text (which\n is not itself that likely to have wildcards, but @{} is\n also a potential generic extension mechanism).\n\nWhen we see these constructs, they are almost certainly an\nattempt at a revision, and not a pathspec; we should not\ngive them the magic \"wildcard characters mean a pathspec\"\ntreatment.\n\nWe can afford to be fairly slack in our parsing here. We are\nnot making a real decision on \"this is or is not definitely\na revision\" here, but rather just deciding whether or not\nthe extra \"wildcards mean pathspecs\" magic kicks in.\n\nSigned-off-by: Jeff King \n---\n setup.c | 21 ++++++++++++++++++++-\n t/t6133-pathspec-rev-dwim.sh | 44 ++++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 64 insertions(+), 1 deletion(-)\n create mode 100755 t/t6133-pathspec-rev-dwim.sh\n\ndiff --git a/setup.c b/setup.c\nindex 2c4b22c..03ee4eb 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -130,6 +130,25 @@ int path_inside_repo(const char *prefix, const char *path)\n \treturn 0;\n }\n \n+static int dwim_as_wildcard(const char *arg)\n+{\n+\tconst char *p;\n+\n+\tif (no_wildcard(arg))\n+\t\treturn 0;\n+\tif (strstr(arg, \"^{\"))\n+\t\treturn 0; /* probably \"^{something}\" */\n+\tif (strstr(arg, \"@{\"))\n+\t\treturn 0; /* probably \"ref@{something}\" */\n+\n+\t/* catch \"tree:path\", but not \":(magic)\" */\n+\tp = strchr(arg, ':');\n+\tif (p && p[1] != '(')\n+\t\treturn 0;\n+\n+\treturn 1;\n+}\n+\n int check_filename(const char *prefix, const char *arg)\n {\n \tconst char *name;\n@@ -139,7 +158,7 @@ int check_filename(const char *prefix, const char *arg)\n \t\tif (arg[2] == '\\0') /* \":/\" is root dir, always exists */\n \t\t\treturn 1;\n \t\tname = arg + 2;\n-\t} else if (!no_wildcard(arg))\n+\t} else if (dwim_as_wildcard(arg))\n \t\treturn 1;\n \telse if (prefix)\n \t\tname = prefix_filename(prefix, strlen(prefix), arg);\ndiff --git a/t/t6133-pathspec-rev-dwim.sh b/t/t6133-pathspec-rev-dwim.sh\nnew file mode 100755\nindex 0000000..8f68937\n--- /dev/null\n+++ b/t/t6133-pathspec-rev-dwim.sh\n@@ -0,0 +1,44 @@\n+#!/bin/sh\n+\n+test_description='test dwim of revs versus pathspecs in revision parser'\n+. ./test-lib.sh\n+\n+test_expect_success 'setup' '\n+\ttest_commit base &&\n+\techo content >\"br[ack]ets\" &&\n+\tgit add . &&\n+\ttest_tick &&\n+\tgit commit -m brackets\n+'\n+\n+test_expect_success 'wildcard dwims to pathspec' '\n+\tgit log -- \"*.t\" >expect &&\n+\tgit log \"*.t\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success ':(magic) dwims to pathspec' '\n+\tgit log -- \":(literal)br[ack]ets\" >expect &&\n+\tgit log \":(literal)br[ack]ets\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'tree:path dwims to rev' '\n+\tgit show \"HEAD:br[ack]ets\" -- >expect &&\n+\tgit show \"HEAD:br[ack]ets\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success '^{foo} dwims to rev' '\n+\tgit log \"HEAD^{/b.*}\" -- >expect &&\n+\tgit log \"HEAD^{/b.*}\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success '@{foo} dwims to rev' '\n+\tgit log \"HEAD@{now [or thereabouts]}\" -- >expect &&\n+\tgit log \"HEAD@{now [or thereabouts]}\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_done\n-- \n2.7.1.526.gd04f550\n"},{"id":"277756","messageId":"xmqqpow7807l.fsf@gitster.mtv.corp.google.com","threadId":"41356","inReplyTo":"20160208150709.GA13664@sigill.intra.peff.net","subject":"Re: git show doesn't work on file names with square brackets","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-02-08T19:35:10Z","receivedAt":"2016-02-08T19:35:10Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> The patch for that might look like this. I like it for its relative\n> simplicity, though it does make the rules even harder to explain to a\n> user...\n\nTrue.\n\nTo be bluntly honest, I do not see the current \"string containing\nwildcard characters are taken as path, not rev, unless you use the\ndouble dash to disambiguate.\" all bad. Isn't it sort of crazy to\nhave square brackets in paths and if it requires clarification by\nthe user, I do not particulasrly see it as a problem.\n\nHaving said that, I do not think of a big reason to say this patch\nis a wrong thing to do, either.\n\n> This breaks the second test in t2019 added by ae454f6, but I am not sure\n> that test is doing the right thing (I'm also not sure t2019 is the best\n> place for these tests; I added new ones here in a separate script).\n\nI am inclined to agree that that particular test is casting an\nimplementation limitation in stone.\n\n> We can afford to be fairly slack in our parsing here. We are\n> not making a real decision on \"this is or is not definitely\n> a revision\" here, but rather just deciding whether or not\n> the extra \"wildcards mean pathspecs\" magic kicks in.\n>\n> Signed-off-by: Jeff King \n> ---\n> setup.c | 21 ++++++++++++++++++++-\n> t/t6133-pathspec-rev-dwim.sh | 44 ++++++++++++++++++++++++++++++++++++++++++++\n> 2 files changed, 64 insertions(+), 1 deletion(-)\n> create mode 100755 t/t6133-pathspec-rev-dwim.sh\n>\n> diff --git a/setup.c b/setup.c\n> index 2c4b22c..03ee4eb 100644\n> --- a/setup.c\n> +++ b/setup.c\n> @@ -130,6 +130,25 @@ int path_inside_repo(const char *prefix, const char *path)\n> \treturn 0;\n> }\n> \n> +static int dwim_as_wildcard(const char *arg)\n> +{\n> +\tconst char *p;\n> +\n> +\tif (no_wildcard(arg))\n> +\t\treturn 0;\n> +\tif (strstr(arg, \"^{\"))\n> +\t\treturn 0; /* probably \"^{something}\" */\n> +\tif (strstr(arg, \"@{\"))\n> +\t\treturn 0; /* probably \"ref@{something}\" */\n> +\n> +\t/* catch \"tree:path\", but not \":(magic)\" */\n> +\tp = strchr(arg, ':');\n> +\tif (p && p[1] != '(')\n> +\t\treturn 0;\n\nYou seem to reject \":(\" specifically, but I am not sure whom is it\ndesigned to help to special case \":(\". Those who write \":(top)\"\nwould not have to disambiguate with \"--\", but their preference is to\nspell things in longhand for more explicit control, so I do not\nthink they mind typing \"--\". On the other hand, those who write\n\":/\" and \":!\" (\":(top)\" and \":(exclude)\") would need to disambiguate\nwith \"--\" with the change.\n\nThat somehow feels backwards.\n\n\"A pathspec element with the magic prefix\" is hard to tell from\n\"Look for a path in the index\" but not from \"Look for a path in a\ntree-ish\", so if you get (p && p != arg), you know it is tree:path,\nI think.\n"},{"id":"277759","messageId":"20160208195230.GA30693@sigill.intra.peff.net","threadId":"41356","inReplyTo":"xmqqpow7807l.fsf@gitster.mtv.corp.google.com","subject":"Re: git show doesn't work on file names with square brackets","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-02-08T19:52:30Z","receivedAt":"2016-02-08T19:52:30Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 08, 2016 at 11:35:10AM -0800, Junio C Hamano wrote:\n\n> To be bluntly honest, I do not see the current \"string containing\n> wildcard characters are taken as path, not rev, unless you use the\n> double dash to disambiguate.\" all bad. Isn't it sort of crazy to\n> have square brackets in paths and if it requires clarification by\n> the user, I do not particulasrly see it as a problem.\n> \n> Having said that, I do not think of a big reason to say this patch\n> is a wrong thing to do, either.\n\nTo be honest, I think I am more concerned with \":/reg.*ex\" than\n\"tree:path\" using meta-characters. I agree that actual metacharacters in\nfile names are relatively rare.\n\n> > +static int dwim_as_wildcard(const char *arg)\n> > +{\n> > +\tconst char *p;\n> > +\n> > +\tif (no_wildcard(arg))\n> > +\t\treturn 0;\n> > +\tif (strstr(arg, \"^{\"))\n> > +\t\treturn 0; /* probably \"^{something}\" */\n> > +\tif (strstr(arg, \"@{\"))\n> > +\t\treturn 0; /* probably \"ref@{something}\" */\n> > +\n> > +\t/* catch \"tree:path\", but not \":(magic)\" */\n> > +\tp = strchr(arg, ':');\n> > +\tif (p && p[1] != '(')\n> > +\t\treturn 0;\n> \n> You seem to reject \":(\" specifically, but I am not sure whom is it\n> designed to help to special case \":(\". Those who write \":(top)\"\n> would not have to disambiguate with \"--\", but their preference is to\n> spell things in longhand for more explicit control, so I do not\n> think they mind typing \"--\". On the other hand, those who write\n> \":/\" and \":!\" (\":(top)\" and \":(exclude)\") would need to disambiguate\n> with \"--\" with the change.\n> \n> That somehow feels backwards.\n\nGood point. I forgot about the short-hands, and I agree that there is\nnot much point in doing a sloppy match of the long-hands if we do not\ncover the short-hands.\n\nIn retrospect, it is not worth trying to match magic pathspecs here at\nall, as it generally requires \"--\" anyway. It is only ones with wildcard\nwhich came along for the ride in 28fcc0b, and that was not the primary\nfocus of that patch. I.e., without my patch, we already have:\n\n $ git rev-list --count HEAD Makefile\n 2028\n\n $ git rev-list --count HEAD ':(top)Makefile'\n fatal: ambiguous argument ':(top)Makefile': unknown revision or path\n not in the working tree.\n Use '--' to separate paths from revisions, like this:\n 'git [...] -- [...]'\n\n $ git rev-list --count HEAD ':(top)M[a]kefile'\n 2028\n\nwhich is slightly ridiculous. With my patch, the final one behaves the\nsame as the second.\n\nHere is my patch again, with that part removed, and the tests fixed up.\nThough on reflection, I do think it would be better if we could simply\nexpand the wildcard globs to say \"does this match anything in the file\nsystem\". That makes a nice, simple rule that follows the spirit of the\noriginal. I'm not sure if it would be easy to apply magic like \":(top)\"\nthere, but even if we don't, we're not worse off than we are today\n(where that requires \"--\" unless it happens to have a wildcard, as\nabove).\n\n-- >8 --\nSubject: [PATCH] check_filename: tighten requirements for dwim-wildcards\n\nCommit 28fcc0b (pathspec: avoid the need of \"--\" when\nwildcard is used, 2015-05-02) introduced a convenience to\nour dwim-parsing: when \"--\" is not present, we guess that\nitems with wildcard characters are probably pathspecs.\n\nThis makes a lot of cases simpler (e.g., \"git log '*.c'\"),\nbut makes others harder. While revision expressions do not\ntypically have wildcard characters in them (because they are\nnot valid in refnames), there are a few constructs where we\ntake more arbitrary strings, such as:\n\n - pathnames in tree:path syntax (or :0:path) for the\n index)\n\n - :/foo and ^{/foo} for searching commit messages;\n likewise \"^{}\" is extensible and may learn new formats\n in the future\n\n - @{foo}, which can take arbitrary approxidate text (which\n is not itself that likely to have wildcards, but @{} is\n also a potential generic extension mechanism).\n\nWhen we see these constructs, they are almost certainly an\nattempt at a revision, and not a pathspec; we should not\ngive them the magic \"wildcard characters mean a pathspec\"\ntreatment.\n\nWe can afford to be fairly slack in our parsing here. We are\nnot making a real decision on \"this is or is not definitely\na revision\" here, but rather just deciding whether or not\nthe extra \"wildcards mean pathspecs\" magic kicks in.\n\nNote that we drop the tests in t2019 in favor of a more\ncomplete set in t6133. t2019 was not the right place for\nthem (it's about refname ambiguity, not dwim parsing\nambiguity), and the second test explicitly checked for the\nopposite result of the case we are fixing here (which didn't\nreally make any sense; as show by the test_must_fail in the\ntest, it would only serve to annoy people).\n\nSigned-off-by: Jeff King \n---\n setup.c | 15 ++++++++++++++-\n t/t2019-checkout-ambiguous-ref.sh | 26 --------------------------\n t/t6133-pathspec-rev-dwim.sh | 38 ++++++++++++++++++++++++++++++++++++++\n 3 files changed, 52 insertions(+), 27 deletions(-)\n create mode 100755 t/t6133-pathspec-rev-dwim.sh\n\ndiff --git a/setup.c b/setup.c\nindex 2c4b22c..eac1edc 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -130,6 +130,19 @@ int path_inside_repo(const char *prefix, const char *path)\n \treturn 0;\n }\n \n+static int dwim_as_wildcard(const char *arg)\n+{\n+\tif (no_wildcard(arg))\n+\t\treturn 0;\n+\tif (strstr(arg, \"^{\"))\n+\t\treturn 0; /* probably \"^{something}\" */\n+\tif (strstr(arg, \"@{\"))\n+\t\treturn 0; /* probably \"ref@{something}\" */\n+\tif (strchr(arg, ':'))\n+\t\treturn 0;\n+\treturn 1;\n+}\n+\n int check_filename(const char *prefix, const char *arg)\n {\n \tconst char *name;\n@@ -139,7 +152,7 @@ int check_filename(const char *prefix, const char *arg)\n \t\tif (arg[2] == '\\0') /* \":/\" is root dir, always exists */\n \t\t\treturn 1;\n \t\tname = arg + 2;\n-\t} else if (!no_wildcard(arg))\n+\t} else if (dwim_as_wildcard(arg))\n \t\treturn 1;\n \telse if (prefix)\n \t\tname = prefix_filename(prefix, strlen(prefix), arg);\ndiff --git a/t/t2019-checkout-ambiguous-ref.sh b/t/t2019-checkout-ambiguous-ref.sh\nindex 199b22d..b99d519 100755\n--- a/t/t2019-checkout-ambiguous-ref.sh\n+++ b/t/t2019-checkout-ambiguous-ref.sh\n@@ -56,30 +56,4 @@ test_expect_success VAGUENESS_SUCCESS 'checkout reports switch to branch' '\n \ttest_i18ngrep ! \"^HEAD is now at\" stderr\n '\n \n-test_expect_success 'wildcard ambiguation, paths win' '\n-\tgit init ambi &&\n-\t(\n-\t\tcd ambi &&\n-\t\techo a >a.c &&\n-\t\tgit add a.c &&\n-\t\techo b >a.c &&\n-\t\tgit checkout \"*.c\" &&\n-\t\techo a >expect &&\n-\t\ttest_cmp expect a.c\n-\t)\n-'\n-\n-test_expect_success !MINGW 'wildcard ambiguation, refs lose' '\n-\tgit init ambi2 &&\n-\t(\n-\t\tcd ambi2 &&\n-\t\techo a >\"*.c\" &&\n-\t\tgit add . &&\n-\t\ttest_must_fail git show :\"*.c\" &&\n-\t\tgit show :\"*.c\" -- >actual &&\n-\t\techo a >expect &&\n-\t\ttest_cmp expect actual\n-\t)\n-'\n-\n test_done\ndiff --git a/t/t6133-pathspec-rev-dwim.sh b/t/t6133-pathspec-rev-dwim.sh\nnew file mode 100755\nindex 0000000..2ffebee\n--- /dev/null\n+++ b/t/t6133-pathspec-rev-dwim.sh\n@@ -0,0 +1,38 @@\n+#!/bin/sh\n+\n+test_description='test dwim of revs versus pathspecs in revision parser'\n+. ./test-lib.sh\n+\n+test_expect_success 'setup' '\n+\ttest_commit base &&\n+\techo content >\"br[ack]ets\" &&\n+\tgit add . &&\n+\ttest_tick &&\n+\tgit commit -m brackets\n+'\n+\n+test_expect_success 'wildcard dwims to pathspec' '\n+\tgit log -- \"*.t\" >expect &&\n+\tgit log \"*.t\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'tree:path dwims to rev' '\n+\tgit show \"HEAD:br[ack]ets\" -- >expect &&\n+\tgit show \"HEAD:br[ack]ets\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success '^{foo} dwims to rev' '\n+\tgit log \"HEAD^{/b.*}\" -- >expect &&\n+\tgit log \"HEAD^{/b.*}\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success '@{foo} dwims to rev' '\n+\tgit log \"HEAD@{now [or thereabouts]}\" -- >expect &&\n+\tgit log \"HEAD@{now [or thereabouts]}\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_done\n-- \n2.7.1.526.gd04f550\n"},{"id":"277764","messageId":"20160208202043.GA6002@sigill.intra.peff.net","threadId":"41356","inReplyTo":"20160208195230.GA30693@sigill.intra.peff.net","subject":"Re: git show doesn't work on file names with square brackets","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-02-08T20:20:43Z","receivedAt":"2016-02-08T20:20:43Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 08, 2016 at 02:52:30PM -0500, Jeff King wrote:\n\n> Here is my patch again, with that part removed, and the tests fixed up.\n> Though on reflection, I do think it would be better if we could simply\n> expand the wildcard globs to say \"does this match anything in the file\n> system\". That makes a nice, simple rule that follows the spirit of the\n> original. I'm not sure if it would be easy to apply magic like \":(top)\"\n> there, but even if we don't, we're not worse off than we are today\n> (where that requires \"--\" unless it happens to have a wildcard, as\n> above).\n\nSo here is a hacky attempt at that. It uses glob(), which is not quite\nright for the reasons below, though I suspect works OK in practice.\n\nI think doing it correctly would require actually calling our\nread_directory() function. That feels kind of heavy-weight for this\ncase, but I guess in theory the pathspec limits it (and it's not like\nglob() does not have to walk the filesystem, too). So maybe it's not so\nbad.\n\n---\ndiff --git a/setup.c b/setup.c\nindex 2c4b22c..d8a7b9d 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -1,6 +1,7 @@\n #include \"cache.h\"\n #include \"dir.h\"\n #include \"string-list.h\"\n+#include \n \n static int inside_git_dir = -1;\n static int inside_work_tree = -1;\n@@ -130,6 +131,26 @@ int path_inside_repo(const char *prefix, const char *path)\n \treturn 0;\n }\n \n+/*\n+ * Return true if a file exists that matches the pattern\n+ * glob. Note that this _should_ use our regular wildmatch\n+ * pattern matches, but there is no ready-made glob()\n+ * function there. This is a cheap hack that makes\n+ * simple things like \"*.c\" work without having to\n+ * use a \"--\" disambiguator.\n+ *\n+ * A custom glob() could also do this more efficiently; we don't\n+ * care about collecting the results, and can quit as soon as\n+ * we see one.\n+ */\n+static int glob_exists(const char *pattern)\n+{\n+\tglob_t data;\n+\tint r = glob(pattern, GLOB_NOSORT, NULL, &data);\n+\tglobfree(&data);\n+\treturn !r;\n+}\n+\n int check_filename(const char *prefix, const char *arg)\n {\n \tconst char *name;\n@@ -139,12 +160,14 @@ int check_filename(const char *prefix, const char *arg)\n \t\tif (arg[2] == '\\0') /* \":/\" is root dir, always exists */\n \t\t\treturn 1;\n \t\tname = arg + 2;\n-\t} else if (!no_wildcard(arg))\n-\t\treturn 1;\n-\telse if (prefix)\n+\t} else if (prefix)\n \t\tname = prefix_filename(prefix, strlen(prefix), arg);\n \telse\n \t\tname = arg;\n+\n+\tif (!no_wildcard(arg))\n+\t\treturn glob_exists(name);\n+\n \tif (!lstat(name, &st))\n \t\treturn 1; /* file exists */\n \tif (errno == ENOENT || errno == ENOTDIR)\n"},{"id":"277768","messageId":"20160208205637.GA13732@sigill.intra.peff.net","threadId":"41356","inReplyTo":"20160208202043.GA6002@sigill.intra.peff.net","subject":"Re: git show doesn't work on file names with square brackets","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-02-08T20:56:38Z","receivedAt":"2016-02-08T20:56:38Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 08, 2016 at 03:20:43PM -0500, Jeff King wrote:\n\n> On Mon, Feb 08, 2016 at 02:52:30PM -0500, Jeff King wrote:\n> \n> > Here is my patch again, with that part removed, and the tests fixed up.\n> > Though on reflection, I do think it would be better if we could simply\n> > expand the wildcard globs to say \"does this match anything in the file\n> > system\". That makes a nice, simple rule that follows the spirit of the\n> > original. I'm not sure if it would be easy to apply magic like \":(top)\"\n> > there, but even if we don't, we're not worse off than we are today\n> > (where that requires \"--\" unless it happens to have a wildcard, as\n> > above).\n> \n> So here is a hacky attempt at that. It uses glob(), which is not quite\n> right for the reasons below, though I suspect works OK in practice.\n> \n> I think doing it correctly would require actually calling our\n> read_directory() function. That feels kind of heavy-weight for this\n> case, but I guess in theory the pathspec limits it (and it's not like\n> glob() does not have to walk the filesystem, too). So maybe it's not so\n> bad.\n\nAnd here that is. It does end up traversing quite a bit for something as\nsimple as \"*.foo\", because that doesn't let fill_directory() limit us at\nall.\n\nBut having looked at this, I can't help but wonder if the rule should\nnot be \"does the file exist\" in the first place, but \"is the file in the\nindex\". This dwimmery is about commands like \"log\" that are reading\nexisting commits. I cannot think of a case where we would want to\ninclude something that exists in the filesystem but not in the index.\n\n---\ndiff --git a/setup.c b/setup.c\nindex 2c4b22c..1a40516 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -1,5 +1,6 @@\n #include \"cache.h\"\n #include \"dir.h\"\n+#include \"pathspec.h\"\n #include \"string-list.h\"\n \n static int inside_git_dir = -1;\n@@ -130,6 +131,33 @@ int path_inside_repo(const char *prefix, const char *path)\n \treturn 0;\n }\n \n+/*\n+ * Return true if a file exists that matches the pattern\n+ * glob.\n+ */\n+static int pathspec_exists(const char *one_pathspec)\n+{\n+\tstruct dir_struct dir;\n+\tconst char *pathspec_v[] = { one_pathspec, NULL };\n+\tstruct pathspec pathspec;\n+\tint ret = 0;\n+\tint i;\n+\n+\tmemset(&dir, 0, sizeof(dir));\n+\tparse_pathspec(&pathspec, 0, 0, \"\", pathspec_v);\n+\n+\tfill_directory(&dir, &pathspec);\n+\tfor (i = 0; i < dir.nr; i++) {\n+\t\tif (dir_path_match(dir.entries[i], &pathspec, 0, NULL)) {\n+\t\t\tret = 1;\n+\t\t\tbreak;\n+\t\t}\n+\t}\n+\n+\tfree(dir.entries);\n+\treturn ret;\n+}\n+\n int check_filename(const char *prefix, const char *arg)\n {\n \tconst char *name;\n@@ -139,12 +167,14 @@ int check_filename(const char *prefix, const char *arg)\n \t\tif (arg[2] == '\\0') /* \":/\" is root dir, always exists */\n \t\t\treturn 1;\n \t\tname = arg + 2;\n-\t} else if (!no_wildcard(arg))\n-\t\treturn 1;\n-\telse if (prefix)\n+\t} else if (prefix)\n \t\tname = prefix_filename(prefix, strlen(prefix), arg);\n \telse\n \t\tname = arg;\n+\n+\tif (!no_wildcard(arg))\n+\t\treturn pathspec_exists(name);\n+\n \tif (!lstat(name, &st))\n \t\treturn 1; /* file exists */\n \tif (errno == ENOENT || errno == ENOTDIR)\n"},{"id":"277781","messageId":"xmqqlh6u6d8v.fsf@gitster.mtv.corp.google.com","threadId":"41356","inReplyTo":"20160208205637.GA13732@sigill.intra.peff.net","subject":"Re: git show doesn't work on file names with square brackets","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-02-08T22:36:32Z","receivedAt":"2016-02-08T22:36:32Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> But having looked at this, I can't help but wonder if the rule should\n> not be \"does the file exist\" in the first place, but \"is the file in the\n> index\". This dwimmery is about commands like \"log\" that are reading\n> existing commits. I cannot think of a case where we would want to\n> include something that exists in the filesystem but not in the index.\n\nYeah, checking in the index, once it is loaded, is reasonably quick\ncheck. A path that is not in the index or the current HEAD may or\nmay not exist on the filesystem, so at some point you would need an\nexplicit disambiguation anyway, and the reason why we check the\nfilesystem is not because that is conceptually better than checking\nin the index but merely because \"does lstat(2) tell us the path is\nthere?\" check was fairly a cheap way on the platform the system was\nprimarily developed on initially. Looking it up from HEAD would be\na lot more heavyweight and would not buy us anything, but looking it\nup in the index may turn out to be comparable to a single lstat(2).\n\nI dunno. I have a suspicion that anything conceptually more\nexpensive than a single lstat(2) is probably not worth doing, as\nthis \"sometimes you do not have to give --\" is merely a usability\nhack, and we have to always do \"git log -- removed-sometime-ago\"\nto find where in the history a certain path was lost.\n"},{"id":"277819","messageId":"xmqqziv939ir.fsf@gitster.mtv.corp.google.com","threadId":"41356","inReplyTo":"20160208195230.GA30693@sigill.intra.peff.net","subject":"Re: git show doesn't work on file names with square brackets","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-02-09T20:37:32Z","receivedAt":"2016-02-09T20:37:32Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Subject: [PATCH] check_filename: tighten requirements for dwim-wildcards\n>\n> Commit 28fcc0b (pathspec: avoid the need of \"--\" when\n> wildcard is used, 2015-05-02) introduced a convenience to\n> our dwim-parsing: when \"--\" is not present, we guess that\n> items with wildcard characters are probably pathspecs.\n>\n> This makes a lot of cases simpler (e.g., \"git log '*.c'\"),\n> but makes others harder. While revision expressions do not\n> typically have wildcard characters in them (because they are\n> not valid in refnames), there are a few constructs where we\n> take more arbitrary strings, such as:\n>\n> - pathnames in tree:path syntax (or :0:path) for the\n> index)\n>\n> - :/foo and ^{/foo} for searching commit messages;\n> likewise \"^{}\" is extensible and may learn new formats\n> in the future\n>\n> - @{foo}, which can take arbitrary approxidate text (which\n> is not itself that likely to have wildcards, but @{} is\n> also a potential generic extension mechanism).\n>\n> When we see these constructs, they are almost certainly an\n> attempt at a revision, and not a pathspec; we should not\n> give them the magic \"wildcard characters mean a pathspec\"\n> treatment.\n>\n> We can afford to be fairly slack in our parsing here. We are\n> not making a real decision on \"this is or is not definitely\n> a revision\" here, but rather just deciding whether or not\n> the extra \"wildcards mean pathspecs\" magic kicks in.\n>\n> Note that we drop the tests in t2019 in favor of a more\n> complete set in t6133. t2019 was not the right place for\n> them (it's about refname ambiguity, not dwim parsing\n> ambiguity), and the second test explicitly checked for the\n> opposite result of the case we are fixing here (which didn't\n> really make any sense; as show by the test_must_fail in the\n> test, it would only serve to annoy people).\n>\n> Signed-off-by: Jeff King \n\nI was leaning towards merging this version, but I became unsure\nwhile writing an entry for \"What's cooking\" (which will be used as a\nmerge summary message and then will appear in the Release Notes).\n\nWe would surely want\n\n $ git log ':/tighten.*'\n\nto find this commit, not take it as a pathspec. But running\n\n $ git log ':/*.c'\n\nin a subdirectory to find commits that touched any .c file, taking\nit as a pathspec, would equally be a sensible thing to want.\n\nI would feel that we should require \"--\" for both cases; with or\nwithout this patch, we already treat these as revs without \"--\",\nmaking the latter fail without \"--\". Also:\n\n $ git log \"HEAD^{/tighten.*}\"\n\nis already dwimmed as a rev.\n\nAnd a path with glob(3) metacharacters is an insane thing, be it\ninside a treeish or in the working tree, and I think it is OK to\nrequire users to explicitly say what they mean with \"--\".\n\nAnd the patch does not leave much if we ignore that \":\" bit.\nWith the patch, \"HEAD@{now [or thereabouts]}\" will be taken as a\nrev without \"--\", which is an improvement, but to me that seems\nto be the only improvement this change brings us.\n\nAnd I do not think we want either glob(3) or fill_directory() to\nslow things down, as this is merely a heuristic.\n\nWe may want to rethink the interface into check_filename(). The\ncallers of this function that try to help users who did not use \"--\"\nwant the function to say \"It is likely that this was meant as a\npathname\" and when this function says \"No, the user did not mean it\nas a filename.\" they will in turn ask the revision parser \"Is this a\nrev?\". At that point, if it is not a revision, these callers can\nsay \"Not a file, not a rev\" and die.\n\nIn order to allow \"':/tighten.*' is a rev, ':/*.c' is a pathspec,\nthey are equally likely and you must disambiguate\", the current\ninterface is inadequate.\n\n * check_filename() cannot say \"No, it is not a filename\"--a later\n call to get_sha1() will barf on \":/*.c\" saying that it is not a\n rev, but the fault is in Git that initially guessed it would be a\n rev when the user meant it as a pathspec.\n\n * The function cannot say \"Yes, it is a filename\"--then get_sha1()\n will not be called for \":/tighten.*\" and we would silently use it\n as pathspec, possibly producing an empty result.\n\nThere needs to be a way for it to say \"I refuse to disambiguate\".\n\nI actually think that no_wildcard() check added in check_filename()\nwas the original mistake. If we revert the check_filename() to a\nsimple \"Is this a filename?\" and move the \"does this thing have a\nwildcard\" aka \"can this be a pathspec even when check_filename()\nsays there is no file with that exact name?\" to the code that tries\nto allow users omit \"--\", i.e. the caller of check_filename(), would\nthat make the code structure and the semantics much cleaner, I\nwonder...\n"},{"id":"277867","messageId":"20160210154510.GB19867@sigill.intra.peff.net","threadId":"41356","inReplyTo":"xmqqlh6u6d8v.fsf@gitster.mtv.corp.google.com","subject":"Re: git show doesn't work on file names with square brackets","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-02-10T15:45:10Z","receivedAt":"2016-02-10T15:45:10Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 08, 2016 at 02:36:32PM -0800, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > But having looked at this, I can't help but wonder if the rule should\n> > not be \"does the file exist\" in the first place, but \"is the file in the\n> > index\". This dwimmery is about commands like \"log\" that are reading\n> > existing commits. I cannot think of a case where we would want to\n> > include something that exists in the filesystem but not in the index.\n> \n> Yeah, checking in the index, once it is loaded, is reasonably quick\n> check. A path that is not in the index or the current HEAD may or\n> may not exist on the filesystem, so at some point you would need an\n> explicit disambiguation anyway, and the reason why we check the\n> filesystem is not because that is conceptually better than checking\n> in the index but merely because \"does lstat(2) tell us the path is\n> there?\" check was fairly a cheap way on the platform the system was\n> primarily developed on initially. Looking it up from HEAD would be\n> a lot more heavyweight and would not buy us anything, but looking it\n> up in the index may turn out to be comparable to a single lstat(2).\n\nYeah, I had a notion that looking in the index would not be all that\nexpensive, since we often load it anyway. But this _is_ \"git log\" we are\ntalking about, which does not otherwise need to read the index at all. I\nsuspect lstat(2) is way faster if you have a huge repo, as it is should\nbe constant-ish, as opposed to O(size-of-index).\n\n> I dunno. I have a suspicion that anything conceptually more\n> expensive than a single lstat(2) is probably not worth doing, as\n> this \"sometimes you do not have to give --\" is merely a usability\n> hack, and we have to always do \"git log -- removed-sometime-ago\"\n> to find where in the history a certain path was lost.\n\nYeah, it just seemed a shame to me that things which clearly _aren't_\nambiguous to any sane viewer would be reported as such by git. I think\nthe \"--\" DWIM has worked so well precisely because people do not have\nsilly-named files in their repositories, so it Just Works most of the\ntime. The wildcard rule switches it from \"you put a file named HEAD in\nyour repository, now you pay the price for being silly\" to \"whoops,\neverything with a metacharacter is now ambiguous\".\n\n-Peff\n"},{"id":"277870","messageId":"20160210161548.GC19867@sigill.intra.peff.net","threadId":"41356","inReplyTo":"xmqqziv939ir.fsf@gitster.mtv.corp.google.com","subject":"Re: git show doesn't work on file names with square brackets","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-02-10T16:15:49Z","receivedAt":"2016-02-10T16:15:49Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Feb 09, 2016 at 12:37:32PM -0800, Junio C Hamano wrote:\n\n> I was leaning towards merging this version, but I became unsure\n> while writing an entry for \"What's cooking\" (which will be used as a\n> merge summary message and then will appear in the Release Notes).\n> \n> We would surely want\n> \n> $ git log ':/tighten.*'\n> \n> to find this commit, not take it as a pathspec. But running\n> \n> $ git log ':/*.c'\n> \n> in a subdirectory to find commits that touched any .c file, taking\n> it as a pathspec, would equally be a sensible thing to want.\n> \n> I would feel that we should require \"--\" for both cases; with or\n> without this patch, we already treat these as revs without \"--\",\n> making the latter fail without \"--\".\n\nYes, because \":/\" is treated specially in check_filename(), and avoids\nkicking in the wildcard behavior. That is certainly preferring revs to\npathspecs, but I think preferring one over the other is preferable to\nbarfing. If the user wants carefulness, they should use \"--\"\nunconditionally. If they want to DWIM, we should make it as painless as\npossible, even if we sometimes guess wrong.\n\n> Also:\n> \n> $ git log \"HEAD^{/tighten.*}\"\n> \n> is already dwimmed as a rev.\n\nIs it? That is the exact case I think regressed in v2.5.0, because it\nclearly _is_ a rev, didn't previously require a \"--\", and now does:\n\n $ git.v2.4.8 log --oneline -1 'HEAD^{/tighten.*}'\n 913c2c7 Merge branch 'jk/sanity' into maint\n\n $ git.v2.5.0 log --oneline -1 'HEAD^{/tighten.*}'\n fatal: ambiguous argument 'HEAD^{/tighten.*}': both revision and filename\n Use '--' to separate paths from revisions, like this:\n 'git [...] -- [...]'\n\nAnd that's what my patch is trying to do: pull back the over-broad match\nin 28fcc0b for cases that are pretty clearly revs.\n\n> And a path with glob(3) metacharacters is an insane thing, be it\n> inside a treeish or in the working tree, and I think it is OK to\n> require users to explicitly say what they mean with \"--\".\n\nYeah, I can buy that line of reasoning.\n\n> We may want to rethink the interface into check_filename(). The\n> callers of this function that try to help users who did not use \"--\"\n> want the function to say \"It is likely that this was meant as a\n> pathname\" and when this function says \"No, the user did not mean it\n> as a filename.\" they will in turn ask the revision parser \"Is this a\n> rev?\". At that point, if it is not a revision, these callers can\n> say \"Not a file, not a rev\" and die.\n> \n> In order to allow \"':/tighten.*' is a rev, ':/*.c' is a pathspec,\n> they are equally likely and you must disambiguate\", the current\n> interface is inadequate.\n\nHmm. I think at least for the revision-parser it is the other way\naround. We actually check first \"is it a rev\". If it is, and there is no\n\"--\", then we ask \"could it also be a filename\" and complain if so. If\nthe revision parser says \"no, it cannot be\", then we say \"well, could it\nplausibly be a filename\"?\n\nAnd both of those use the same check_filename() test, but I think they\nare asking two different things. The first one probably wants to say \"is\nit definitely a filename, because if so, that's ambiguous\". And it would\nbe OK to look at a wildcard and say \"sure, it _could_ be a path, but\ntaking it as a rev is reasonable\". IOW, to err on the side of \"not a\nfilename\", and allow something possibly ambiguous. And then the second\ntest is the opposite; we know it's not a rev, so if it could plausibly\nbe a file, then take it as one.\n\nBut I have a feeling from what you've written that you do not agree with\nthe \"err and allow something possibly ambiguous\" philosophy.\n\nI'll note also two things:\n\n 1. I've _just_ looked at revision.c here; there are other callers of\n verify_filename and verify_non_filename (and even a bare\n check_filename() call!) that may not all have the same needs.\n\n 2. The \":/*.c\" case is much more complicated. Before we get to\n check_filename() at all, we use get_sha1(), which says \"yes, this\n is a pattern\". And then barfs with a fatal error when it sees that\n \"*.c\" is not a valid regex. So no matter what check_filename()\n does, we would have to propagate that error from get_sha1() to make\n anything interesting work there.\n\n> I actually think that no_wildcard() check added in check_filename()\n> was the original mistake. If we revert the check_filename() to a\n> simple \"Is this a filename?\" and move the \"does this thing have a\n> wildcard\" aka \"can this be a pathspec even when check_filename()\n> says there is no file with that exact name?\" to the code that tries\n> to allow users omit \"--\", i.e. the caller of check_filename(), would\n> that make the code structure and the semantics much cleaner, I\n> wonder...\n\nYes. After writing the above, I was envisioning pushing the \"err on this\nside\" logic into check_filename() with a flag. The main callers are\nverify_filename() and verify_non_filename(), and they would use opposite\nflags from each other. But pulling that logic out to the caller would\nbe fine, too.\n\nIOW, something like this implements the \"permissive\" thing I wrote above\n(i.e., be inclusive when seeing if something could plausibly be a\nfilename, but exclusive when complaining that it _could_ be one):\n\ndiff --git a/setup.c b/setup.c\nindex 2c4b22c..995e924 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -139,9 +139,7 @@ int check_filename(const char *prefix, const char *arg)\n \t\tif (arg[2] == '\\0') /* \":/\" is root dir, always exists */\n \t\t\treturn 1;\n \t\tname = arg + 2;\n-\t} else if (!no_wildcard(arg))\n-\t\treturn 1;\n-\telse if (prefix)\n+\t} else if (prefix)\n \t\tname = prefix_filename(prefix, strlen(prefix), arg);\n \telse\n \t\tname = arg;\n@@ -202,7 +200,7 @@ void verify_filename(const char *prefix,\n {\n \tif (*arg == '-')\n \t\tdie(\"bad flag '%s' used after filename\", arg);\n-\tif (check_filename(prefix, arg))\n+\tif (check_filename(prefix, arg) || !no_wildcard(arg))\n \t\treturn;\n \tdie_verify_filename(prefix, arg, diagnose_misspelt_rev);\n }\n"},{"id":"277878","messageId":"xmqqpow4zcwd.fsf@gitster.mtv.corp.google.com","threadId":"41356","inReplyTo":"20160210161548.GC19867@sigill.intra.peff.net","subject":"Re: git show doesn't work on file names with square brackets","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-02-10T17:35:46Z","receivedAt":"2016-02-10T17:35:46Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Yes, because \":/\" is treated specially in check_filename(), and avoids\n> kicking in the wildcard behavior. That is certainly preferring revs to\n> pathspecs, but I think preferring one over the other is preferable to\n> barfing. If the user wants carefulness, they should use \"--\"\n> unconditionally. If they want to DWIM, we should make it as painless as\n> possible, even if we sometimes guess wrong.\n\nOK, I think that is sensible.\n\n> But I have a feeling from what you've written that you do not agree with\n> the \"err and allow something possibly ambiguous\" philosophy.\n\nNot anymore ;-)\n\n>> I actually think that no_wildcard() check added in check_filename()\n>> was the original mistake. If we revert the check_filename() to a\n>> simple \"Is this a filename?\" and move the \"does this thing have a\n>> wildcard\" aka \"can this be a pathspec even when check_filename()\n>> says there is no file with that exact name?\" to the code that tries\n>> to allow users omit \"--\", i.e. the caller of check_filename(), would\n>> that make the code structure and the semantics much cleaner, I\n>> wonder...\n>\n> Yes. After writing the above, I was envisioning pushing the \"err on this\n> side\" logic into check_filename() with a flag. The main callers are\n> verify_filename() and verify_non_filename(), and they would use opposite\n> flags from each other. But pulling that logic out to the caller would\n> be fine, too.\n>\n> IOW, something like this implements the \"permissive\" thing I wrote above\n> (i.e., be inclusive when seeing if something could plausibly be a\n> filename, but exclusive when complaining that it _could_ be one):\n\nYup, I think that is probably a better first step.\n\n> diff --git a/setup.c b/setup.c\n> index 2c4b22c..995e924 100644\n> --- a/setup.c\n> +++ b/setup.c\n> @@ -139,9 +139,7 @@ int check_filename(const char *prefix, const char *arg)\n> \t\tif (arg[2] == '\\0') /* \":/\" is root dir, always exists */\n> \t\t\treturn 1;\n> \t\tname = arg + 2;\n> -\t} else if (!no_wildcard(arg))\n> -\t\treturn 1;\n> -\telse if (prefix)\n> +\t} else if (prefix)\n> \t\tname = prefix_filename(prefix, strlen(prefix), arg);\n> \telse\n> \t\tname = arg;\n> @@ -202,7 +200,7 @@ void verify_filename(const char *prefix,\n> {\n> \tif (*arg == '-')\n> \t\tdie(\"bad flag '%s' used after filename\", arg);\n> -\tif (check_filename(prefix, arg))\n> +\tif (check_filename(prefix, arg) || !no_wildcard(arg))\n> \t\treturn;\n> \tdie_verify_filename(prefix, arg, diagnose_misspelt_rev);\n> }\n"},{"id":"277890","messageId":"20160210211206.GA5755@sigill.intra.peff.net","threadId":"41356","inReplyTo":"xmqqpow4zcwd.fsf@gitster.mtv.corp.google.com","subject":"Re: git show doesn't work on file names with square brackets","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-02-10T21:12:06Z","receivedAt":"2016-02-10T21:12:06Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Feb 10, 2016 at 09:35:46AM -0800, Junio C Hamano wrote:\n\n> > IOW, something like this implements the \"permissive\" thing I wrote above\n> > (i.e., be inclusive when seeing if something could plausibly be a\n> > filename, but exclusive when complaining that it _could_ be one):\n> \n> Yup, I think that is probably a better first step.\n\nThanks. And thank you for the discussion. I read your response last\nnight and almost just said \"OK, let's just scrap my patches, this isn't\nworth the trouble\". But after reading it again this morning, I think it\nforced me to look at the problem in a new way. And while I did scrap my\noriginal patches here, I think the result is accomplishing the same\nthing in a much saner way.\n\nHere's what I came up with.\n\n [1/3]: checkout: reorder check_filename conditional\n [2/3]: check_filename: tighten dwim-wildcard ambiguity\n [3/3]: get_sha1: don't die() on bogus search strings\n\nThe first is a minor preparatory cleanup, the second is the meat we've\nbeen discussing, and the third is a bonus, though it has some tradeoffs.\n\n-Peff\n"},{"id":"277891","messageId":"20160210211234.GA5799@sigill.intra.peff.net","threadId":"41356","inReplyTo":"20160210211206.GA5755@sigill.intra.peff.net","subject":"[PATCH 1/3] checkout: reorder check_filename conditional","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-02-10T21:12:34Z","receivedAt":"2016-02-10T21:12:34Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"If we have a \"--\" flag, we should not be doing DWIM magic\nbased on whether arguments can be filenames. Reorder the\nconditional to avoid the check_filename() call entirely in\nthis case. The outcome is the same, but the short-circuit\nmakes the dependency more clear.\n\nSigned-off-by: Jeff King \n---\n builtin/checkout.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex 5af84a3..f6a2809 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -982,7 +982,7 @@ static int parse_branchname_arg(int argc, const char **argv,\n \t\t */\n \t\tint recover_with_dwim = dwim_new_local_branch_ok;\n \n-\t\tif (check_filename(NULL, arg) && !has_dash_dash)\n+\t\tif (!has_dash_dash && check_filename(NULL, arg))\n \t\t\trecover_with_dwim = 0;\n \t\t/*\n \t\t * Accept \"git checkout foo\" and \"git checkout foo --\"\n-- \n2.7.1.545.gfd1d4e5\n"},{"id":"277892","messageId":"20160210211446.GB5799@sigill.intra.peff.net","threadId":"41356","inReplyTo":"20160210211206.GA5755@sigill.intra.peff.net","subject":"[PATCH 2/3] check_filename: tighten dwim-wildcard ambiguity","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-02-10T21:14:46Z","receivedAt":"2016-02-10T21:14:46Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When specifying both revisions and pathnames, we allow\n\" -- \" to be spelled without the \"--\" as long\nas it is not ambiguous. The original logic was something\nlike:\n\n 1. Resolve each item with get_sha1(). If successful,\n we know it can be a . Verify that it _isn't_ a\n filename, using verify_non_filename(), and complain of\n ambiguity otherwise.\n\n 2. If get_sha1() didn't succeed, make sure that it _is_\n a file, using verify_filename(). If not, complain\n that it is neither a nor a .\n\nBoth verify_filename() and verify_non_filename() rely on\ncheck_filename(), which definitely said \"yes, this is a\nfile\" or \"no, it is not\" using lstat().\n\nCommit 28fcc0b (pathspec: avoid the need of \"--\" when\nwildcard is used, 2015-05-02) introduced a convenience\nfeature: check_filename() will consider anything with\nwildcard meta-characters as a possible filename, without\neven checking the filesystem.\n\nThis works well for case 2. For such a wildcard, we would\npreviously have died and said \"it is neither\". Post-28fcc0b,\nwe assume it's a pathspec and proceed.\n\nBut it makes some instances of case 1 worse. We may have an\nextended sha1 expression that contains meta-characters\n(e.g., \"HEAD^{/foo.*bar}\"), and we now complain that it's\nalso a filename, due to the wildcard characters (even though\nthat wildcard would not match anything in the filesystem).\n\nOne solution would be to actually expand the pathname and\nsee if it matches anything on the filesystem. But that's\npotentially expensive, and we do not have to be so rigorous\nfor this DWIM magic (if you want rigor, use \"--\").\n\nInstead, we can just use different rules for cases 1 and 2.\nWhen we know something is a rev, we will complain only if it\nmeets a much higher standard for \"this is also a file\";\nnamely that it actually exists in the filesystem. Case 2\nremains the same: we use the looser \"it could be a filename\"\nstandard introduced by 28fcc0b.\n\nWe can accomplish this by pulling the wildcard logic out of\ncheck_filename() and putting it into verify_filename(). Its\npartner verify_non_filename() does not need a change, since\ncheck_filename() goes back to implementing the \"higher\nstandard\".\n\nBesides these two callers of check_filename(), there is one\nother: git-checkout does a similar DWIM itself. It hits this\ncode path only after get_sha1() has returned failure, making\nit case 2, which gets the special wildcard treatment.\n\nNote that we drop the tests in t2019 in favor of a more\ncomplete set in t6133. t2019 was not the right place for\nthem (it's about refname ambiguity, not dwim parsing\nambiguity), and the second test explicitly checked for the\nopposite result of the case we are fixing here (which didn't\nreally make any sense; as shown by the test_must_fail in the\ntest, it would only serve to annoy people).\n\nSigned-off-by: Jeff King \n---\n builtin/checkout.c | 3 ++-\n setup.c | 6 ++----\n t/t2019-checkout-ambiguous-ref.sh | 26 --------------------------\n t/t6133-pathspec-rev-dwim.sh | 38 ++++++++++++++++++++++++++++++++++++++\n 4 files changed, 42 insertions(+), 31 deletions(-)\n create mode 100755 t/t6133-pathspec-rev-dwim.sh\n\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex f6a2809..cfa66e2 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -982,7 +982,8 @@ static int parse_branchname_arg(int argc, const char **argv,\n \t\t */\n \t\tint recover_with_dwim = dwim_new_local_branch_ok;\n \n-\t\tif (!has_dash_dash && check_filename(NULL, arg))\n+\t\tif (!has_dash_dash &&\n+\t\t (check_filename(NULL, arg) || !no_wildcard(arg)))\n \t\t\trecover_with_dwim = 0;\n \t\t/*\n \t\t * Accept \"git checkout foo\" and \"git checkout foo --\"\ndiff --git a/setup.c b/setup.c\nindex 2c4b22c..995e924 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -139,9 +139,7 @@ int check_filename(const char *prefix, const char *arg)\n \t\tif (arg[2] == '\\0') /* \":/\" is root dir, always exists */\n \t\t\treturn 1;\n \t\tname = arg + 2;\n-\t} else if (!no_wildcard(arg))\n-\t\treturn 1;\n-\telse if (prefix)\n+\t} else if (prefix)\n \t\tname = prefix_filename(prefix, strlen(prefix), arg);\n \telse\n \t\tname = arg;\n@@ -202,7 +200,7 @@ void verify_filename(const char *prefix,\n {\n \tif (*arg == '-')\n \t\tdie(\"bad flag '%s' used after filename\", arg);\n-\tif (check_filename(prefix, arg))\n+\tif (check_filename(prefix, arg) || !no_wildcard(arg))\n \t\treturn;\n \tdie_verify_filename(prefix, arg, diagnose_misspelt_rev);\n }\ndiff --git a/t/t2019-checkout-ambiguous-ref.sh b/t/t2019-checkout-ambiguous-ref.sh\nindex 199b22d..b99d519 100755\n--- a/t/t2019-checkout-ambiguous-ref.sh\n+++ b/t/t2019-checkout-ambiguous-ref.sh\n@@ -56,30 +56,4 @@ test_expect_success VAGUENESS_SUCCESS 'checkout reports switch to branch' '\n \ttest_i18ngrep ! \"^HEAD is now at\" stderr\n '\n \n-test_expect_success 'wildcard ambiguation, paths win' '\n-\tgit init ambi &&\n-\t(\n-\t\tcd ambi &&\n-\t\techo a >a.c &&\n-\t\tgit add a.c &&\n-\t\techo b >a.c &&\n-\t\tgit checkout \"*.c\" &&\n-\t\techo a >expect &&\n-\t\ttest_cmp expect a.c\n-\t)\n-'\n-\n-test_expect_success !MINGW 'wildcard ambiguation, refs lose' '\n-\tgit init ambi2 &&\n-\t(\n-\t\tcd ambi2 &&\n-\t\techo a >\"*.c\" &&\n-\t\tgit add . &&\n-\t\ttest_must_fail git show :\"*.c\" &&\n-\t\tgit show :\"*.c\" -- >actual &&\n-\t\techo a >expect &&\n-\t\ttest_cmp expect actual\n-\t)\n-'\n-\n test_done\ndiff --git a/t/t6133-pathspec-rev-dwim.sh b/t/t6133-pathspec-rev-dwim.sh\nnew file mode 100755\nindex 0000000..8e5b338\n--- /dev/null\n+++ b/t/t6133-pathspec-rev-dwim.sh\n@@ -0,0 +1,38 @@\n+#!/bin/sh\n+\n+test_description='test dwim of revs versus pathspecs in revision parser'\n+. ./test-lib.sh\n+\n+test_expect_success 'setup' '\n+\ttest_commit base &&\n+\techo content >\"br[ack]ets\" &&\n+\tgit add . &&\n+\ttest_tick &&\n+\tgit commit -m brackets\n+'\n+\n+test_expect_success 'non-rev wildcard dwims to pathspec' '\n+\tgit log -- \"*.t\" >expect &&\n+\tgit log \"*.t\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'tree:path with metacharacters dwims to rev' '\n+\tgit show \"HEAD:br[ack]ets\" -- >expect &&\n+\tgit show \"HEAD:br[ack]ets\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success '^{foo} with metacharacters dwims to rev' '\n+\tgit log \"HEAD^{/b.*}\" -- >expect &&\n+\tgit log \"HEAD^{/b.*}\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success '@{foo} with metacharacters dwims to rev' '\n+\tgit log \"HEAD@{now [or thereabouts]}\" -- >expect &&\n+\tgit log \"HEAD@{now [or thereabouts]}\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_done\n-- \n2.7.1.545.gfd1d4e5\n"},{"id":"277893","messageId":"20160210211925.GC5799@sigill.intra.peff.net","threadId":"41356","inReplyTo":"20160210211206.GA5755@sigill.intra.peff.net","subject":"[PATCH 3/3] get_sha1: don't die() on bogus search strings","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-02-10T21:19:25Z","receivedAt":"2016-02-10T21:19:25Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The get_sha1() function generally returns an error code\nrather than dying, and we sometimes speculatively call it\nwith something that may be a revision or a pathspec, in\norder to see which one it might be.\n\nIf it sees a bogus \":/\" search string, though, it complains,\nwithout giving the caller the opportunity to recover. We can\ndemonstrate this in t6133 by looking for \":/*.t\", which\nshould mean \"*.t at the root of the tree\", but instead dies\nbecause of the invalid regex (the \"*\" has nothing to operate\non).\n\nWe can fix this by returning an error rather than calling\ndie(). Unfortunately, the tradeoff is that the error message\nis slightly worse in cases where we _do_ know we have a rev.\nE.g., running \"git log ':/*.t' --\" before yielded:\n\n fatal: Invalid search pattern: *.t\n\nand now we get only:\n\n fatal: bad revision ':/*.t'\n\nThere's not a simple way to fix this short of passing a\n\"quiet\" flag all the way through the get_sha1() stack.\n\nSigned-off-by: Jeff King \n---\nTo be honest, I'm not sure this is worth it. Part of me wants to say\nthat get_sha1() is simply wrong for dying. And it is, but given how\ninfrequently this would come up, it's perhaps a practical tradeoff to\nget the more accurate error message.\n\nAnd while it does confuse \":/*.t\", which is obviously a pathspec, that's\njust one specific case, that works because of the bogus regex. Something\nlike \":/foo.*\" could mean \"find foo.* at the root\" or it could mean\n\"find a commit message with foo followed by anything\", and we literally\ndo not know which.\n\nWe're likely to treat that one as a rev (assuming you use \"foo\" in your\ncommit messages, but who doesn't?). So you'd need to use \"--\" in the\ngeneral case anyway.\n\n sha1_name.c | 4 ++--\n t/t6133-pathspec-rev-dwim.sh | 10 ++++++++++\n 2 files changed, 12 insertions(+), 2 deletions(-)\n\ndiff --git a/sha1_name.c b/sha1_name.c\nindex 892db21..d61b3b9 100644\n--- a/sha1_name.c\n+++ b/sha1_name.c\n@@ -882,12 +882,12 @@ static int get_sha1_oneline(const char *prefix, unsigned char *sha1,\n \n \tif (prefix[0] == '!') {\n \t\tif (prefix[1] != '!')\n-\t\t\tdie (\"Invalid search pattern: %s\", prefix);\n+\t\t\treturn -1;\n \t\tprefix++;\n \t}\n \n \tif (regcomp(®ex, prefix, REG_EXTENDED))\n-\t\tdie(\"Invalid search pattern: %s\", prefix);\n+\t\treturn -1;\n \n \tfor (l = list; l; l = l->next) {\n \t\tl->item->object.flags |= ONELINE_SEEN;\ndiff --git a/t/t6133-pathspec-rev-dwim.sh b/t/t6133-pathspec-rev-dwim.sh\nindex 8e5b338..a290ffc 100755\n--- a/t/t6133-pathspec-rev-dwim.sh\n+++ b/t/t6133-pathspec-rev-dwim.sh\n@@ -35,4 +35,14 @@ test_expect_success '@{foo} with metacharacters dwims to rev' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success ':/*.t from a subdir dwims to a pathspec' '\n+\tmkdir subdir &&\n+\t(\n+\t\tcd subdir &&\n+\t\tgit log -- \":/*.t\" >expect &&\n+\t\tgit log \":/*.t\" >actual &&\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n test_done\n-- \n2.7.1.545.gfd1d4e5\n"},{"id":"277895","messageId":"xmqq8u2sz1yu.fsf@gitster.mtv.corp.google.com","threadId":"41356","inReplyTo":"20160210211234.GA5799@sigill.intra.peff.net","subject":"Re: [PATCH 1/3] checkout: reorder check_filename conditional","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-02-10T21:31:53Z","receivedAt":"2016-02-10T21:31:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> If we have a \"--\" flag, we should not be doing DWIM magic\n> based on whether arguments can be filenames. Reorder the\n> conditional to avoid the check_filename() call entirely in\n> this case. The outcome is the same, but the short-circuit\n> makes the dependency more clear.\n\nIt also allows check_filename() to die(), and lets the user to\nprevent it with \"--\"---\"Don't check when we do not have to\" is the\nright thing to do.\n\nThanks.\n\n> Signed-off-by: Jeff King \n> ---\n> builtin/checkout.c | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/builtin/checkout.c b/builtin/checkout.c\n> index 5af84a3..f6a2809 100644\n> --- a/builtin/checkout.c\n> +++ b/builtin/checkout.c\n> @@ -982,7 +982,7 @@ static int parse_branchname_arg(int argc, const char **argv,\n> \t\t */\n> \t\tint recover_with_dwim = dwim_new_local_branch_ok;\n> \n> -\t\tif (check_filename(NULL, arg) && !has_dash_dash)\n> +\t\tif (!has_dash_dash && check_filename(NULL, arg))\n> \t\t\trecover_with_dwim = 0;\n> \t\t/*\n> \t\t * Accept \"git checkout foo\" and \"git checkout foo --\"\n"},{"id":"277897","messageId":"xmqq1t8kz0zw.fsf@gitster.mtv.corp.google.com","threadId":"41356","inReplyTo":"20160210211925.GC5799@sigill.intra.peff.net","subject":"Re: [PATCH 3/3] get_sha1: don't die() on bogus search strings","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-02-10T21:52:51Z","receivedAt":"2016-02-10T21:52:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> The get_sha1() function generally returns an error code\n> rather than dying, and we sometimes speculatively call it\n> with something that may be a revision or a pathspec, in\n> order to see which one it might be.\n>\n> If it sees a bogus \":/\" search string, though, it complains,\n> without giving the caller the opportunity to recover. We can\n> demonstrate this in t6133 by looking for \":/*.t\", which\n> should mean \"*.t at the root of the tree\", but instead dies\n\nSlight nit. It means \"'*.t at anywhere in the tree\" aka \"pretend as\nif you gave this pathspec while at the root level of the working\ntree\".\n\n> because of the invalid regex (the \"*\" has nothing to operate\n> on).\n>\n> We can fix this by returning an error rather than calling\n> die(). Unfortunately, the tradeoff is that the error message\n> is slightly worse in cases where we _do_ know we have a rev.\n> E.g., running \"git log ':/*.t' --\" before yielded:\n>\n> fatal: Invalid search pattern: *.t\n>\n> and now we get only:\n>\n> fatal: bad revision ':/*.t'\n\nI do not think the latter is necessarily worse, though. It is being\nconsistent with these:\n\n $ git log mext --\t\t\t;# no such branch\n fatal: bad revision 'mext'\n $ git log ':/xxxt' --\t\t;# no commit matches that pattern\n fatal: bad revision ':/xxxt'\n\nso I would not even mind if somebody argued that the current\n\"invalid search pattern\" is a bug, and gave us this patch as a fix\nfor the inconsistency.\n\n> To be honest, I'm not sure this is worth it. Part of me wants to say\n> that get_sha1() is simply wrong for dying. And it is, but given how\n> infrequently this would come up, it's perhaps a practical tradeoff to\n> get the more accurate error message.\n\nI am on the fence, too, and part of me wants to say the same thing.\nI however happen to view the \"practical tradeoff\" a bit differently,\nso I am slightly inclined to take this.\n\n> And while it does confuse \":/*.t\", which is obviously a pathspec, that's\n> just one specific case, that works because of the bogus regex. Something\n> like \":/foo.*\" could mean \"find foo.* at the root\" or it could mean\n> \"find a commit message with foo followed by anything\", and we literally\n> do not know which.\n>\n> We're likely to treat that one as a rev (assuming you use \"foo\" in your\n> commit messages, but who doesn't?). So you'd need to use \"--\" in the\n> general case anyway.\n\nYeah, I agree it probably would not make much practical difference.\n\n> sha1_name.c | 4 ++--\n> t/t6133-pathspec-rev-dwim.sh | 10 ++++++++++\n> 2 files changed, 12 insertions(+), 2 deletions(-)\n>\n> diff --git a/sha1_name.c b/sha1_name.c\n> index 892db21..d61b3b9 100644\n> --- a/sha1_name.c\n> +++ b/sha1_name.c\n> @@ -882,12 +882,12 @@ static int get_sha1_oneline(const char *prefix, unsigned char *sha1,\n> \n> \tif (prefix[0] == '!') {\n> \t\tif (prefix[1] != '!')\n> -\t\t\tdie (\"Invalid search pattern: %s\", prefix);\n> +\t\t\treturn -1;\n> \t\tprefix++;\n> \t}\n> \n> \tif (regcomp(®ex, prefix, REG_EXTENDED))\n> -\t\tdie(\"Invalid search pattern: %s\", prefix);\n> +\t\treturn -1;\n> \n> \tfor (l = list; l; l = l->next) {\n> \t\tl->item->object.flags |= ONELINE_SEEN;\n> diff --git a/t/t6133-pathspec-rev-dwim.sh b/t/t6133-pathspec-rev-dwim.sh\n> index 8e5b338..a290ffc 100755\n> --- a/t/t6133-pathspec-rev-dwim.sh\n> +++ b/t/t6133-pathspec-rev-dwim.sh\n> @@ -35,4 +35,14 @@ test_expect_success '@{foo} with metacharacters dwims to rev' '\n> \ttest_cmp expect actual\n> '\n> \n> +test_expect_success ':/*.t from a subdir dwims to a pathspec' '\n> +\tmkdir subdir &&\n> +\t(\n> +\t\tcd subdir &&\n> +\t\tgit log -- \":/*.t\" >expect &&\n> +\t\tgit log \":/*.t\" >actual &&\n> +\t\ttest_cmp expect actual\n> +\t)\n> +'\n> +\n> test_done\n"}]}