{"thread":{"id":"46041","subject":"[RFC/PATCH] recognize pathspec magic without \"--\" disambiguation","startedAt":"2017-05-25T15:27:54Z","lastAt":"2017-05-28T00:04:13Z","messageCount":13,"participants":["Jeff King","Junio C Hamano","Ævar Arnfjörð Bjarmason"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"320713","messageId":"20170525152739.t63dbsq2dojy2y2h@sigill.intra.peff.net","threadId":"46041","inReplyTo":null,"subject":"[RFC/PATCH] recognize pathspec magic without \"--\" disambiguation","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-05-25T15:27:40Z","receivedAt":"2017-05-25T15:27:54Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"For commands that take revisions and pathspecs, magic\npathspecs like \":^Documentation\" or \":/Documentation\" have\nto appear on the right-hand side of the disambiguating \"--\",\nlike:\n\n git log -- :^Documentation\n\nThis makes them more annoying to use than they need to be.\nWe loosened the rules for wildcards in 28fcc0b71 (pathspec:\navoid the need of \"--\" when wildcard is used, 2015-05-02).\nLet's do the same for arguments that look like pathspec\nmagic (colon followed by any punctuation).\n\nThe obvious and desired impact is that you can now do:\n\n git log :^Documentation\n\nBut let's consider related invocations and whether we're\nmaking them better or worse:\n\n - git log :/foo\n (when \"foo\" matches a commit message)\n\n This one should remain the same. Like the existing\n wildcard rule, we're touching only verify_filename(),\n not verify_non_filename(). So cases that _do_ resolve\n as a rev will continue to do so.\n\n - git log :^foo\n (when \"^foo\" exists in your index)\n\n The same logic applies; it would continue to work. And\n ditto for any other weird filenames in your index like\n \"(attr)foo\".\n\n - git log :/foo\n (when \"foo\" does _not_ match a commit message)\n\n We won't treat this as a revision, because it doesn't\n match anything. So prior to this patch, we'd either\n treat it as a path (if \"foo\" does exist at the root of\n the project) or complain \"this isn't a rev, and nor is\n it a path\". Whereas after this patch, we'd always\n treat it like a path, whether it exists or not (so in\n the second case instead of an error, you're likely to\n get an empty \"log\", unless \"foo\" really did exist\n somewhere in your history). So this is a potential\n downside; if the user intended a search of the commit\n messages, they may prefer the error message to the\n attempted pathspec match.\n\n This same downside actually exists currently when you\n have an asterisk in your regex. E.g.,\n\n git log :/foo.*bar\n\n will be treated as a pathspec (if it doesn't match a\n commit message) due to the wildcard matching in\n 28fcc0b71.\n\n - git log :^foo\n (when \"^foo\" isn't in your index)\n\n The same outcomes apply here as above, but this\n \"downside\" is very unlikely to occur, as \"^foo\" is\n such a funny name (and again, things like \"(attr)foo\"\n as a filename are sufficiently uncommon not to worry\n about).\n\n - git log :%foo\n (or any other pathspec magic char that doesn't exist)\n\n The new check doesn't actually parse the pathspec\n magic, but allows any punctuation (which includes the\n long-form \":(magic)\"). At first glance this seems\n overly permissive, but it actually yields a better\n error message here: instead of complaining that\n \":%foo\" is neither a rev nor a path, we treat it as a\n pathspec and complain that \"%\" is not a known magic\n (the same as we would if the \"--\" were given).\n\n Of course, the flip side here is when you really\n meant a file named \"%foo\" in your index, but it didn't\n exist. That seems reasonably unlikely (and the error\n message is clear enough to point the user in the right\n direction).\n\nSo the collateral damage doesn't seem too bad (it's really\njust the case where :/foo doesn't match anything). There are\ntwo possibilities for reducing that:\n\n 1. Instead of accepting all pathspec magic, just allow\n certain ones like \":^\" which are unlikely to be used to\n specify a revision (or alternatively, just disallow\n potentially confusing ones like \":/\").\n\n That works, but it makes the rules inconsistent and\n confusing for the user (i.e., \"--\" is sometimes needed\n for magic and sometimes not).\n\n 2. Instead of recognizing pathspec magic, teach\n check_filename() to parse off the filename bit and see\n if that exists (e.g., for \"^foo\" see if \"foo\" exists).\n We already do this for \":/\", but it's done in a very\n ad-hoc way. We parse it ourselves, rather than relying\n on the pathspec code, and handle only \"/\" magic, and\n not even its \":(top)\" long form.\n\n That could be fixed by asking the pathspec code to\n parse it for us (including all magic), and then trying\n to match the resulting name against the working tree.\n But not all pathspec magic actually results in a\n pathname. For instance, when should \":(attr)foo\" be\n valid? We can't just look for \"foo\" in the filesystem\n (it's an attribute, not a pathname).\n\n By comparison, recognizing things that look like\n pathspec magic is a simple and easy to understand rule.\n\nSigned-off-by: Jeff King \n---\nI wrote all the above to try to convince myself that this\ndoesn't have any serious regression cases. And I think I\ndid.\n\nBut I actually we could make the rules in alternative (2)\nabove work. check_filename() would ask the pathspec code to\nparse each argument and get one of three results:\n\n 1. it's not pathspec magic; treat it like a filename\n (e.g., just \"foo\", or even bogus stuff like \":%foo\")\n\n 2. it is pathspec magic, and here is the matching filename\n that ought to exist (e.g., \"foo\" from \":^foo\" or\n \":(exclude)foo\")\n\n 3. it is pathspec magic, but there's no matching filename.\n Assume it's a pathspec (e.g., \"(attr)foo\").\n\nI'm on the fence on whether it's worth the trouble versus\nthe simple rule implemented by this patch.\n\n setup.c | 19 ++++++++++++++++++-\n t/t4208-log-magic-pathspec.sh | 20 ++++++++++++++++++++\n 2 files changed, 38 insertions(+), 1 deletion(-)\n\ndiff --git a/setup.c b/setup.c\nindex 0309c2782..1ca286e82 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -181,6 +181,23 @@ static void NORETURN die_verify_filename(const char *prefix,\n \n }\n \n+/*\n+ * Check for arguments that don't resolve as actual files,\n+ * but which look sufficiently like pathspecs that we'll consider\n+ * them such for the purposes of rev/pathspec DWIM parsing.\n+ */\n+static int looks_like_pathspec(const char *arg)\n+{\n+\tif (!no_wildcard(arg))\n+\t\treturn 1;\n+\n+\t/* pathspec magic like \":^subdir\" */\n+\tif (arg[0] == ':' && ispunct(arg[1]))\n+\t\treturn 1;\n+\n+\treturn 0;\n+}\n+\n /*\n * Verify a filename that we got as an argument for a pathspec\n * entry. Note that a filename that begins with \"-\" never verifies\n@@ -207,7 +224,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) || !no_wildcard(arg))\n+\tif (check_filename(prefix, arg) || looks_like_pathspec(arg))\n \t\treturn;\n \tdie_verify_filename(prefix, arg, diagnose_misspelt_rev);\n }\ndiff --git a/t/t4208-log-magic-pathspec.sh b/t/t4208-log-magic-pathspec.sh\nindex 001343e2f..c87dda53c 100755\n--- a/t/t4208-log-magic-pathspec.sh\n+++ b/t/t4208-log-magic-pathspec.sh\n@@ -61,4 +61,24 @@ test_expect_success 'command line pathspec parsing for \"git log\"' '\n \tgit log --merge -- a\n '\n \n+# This differs from the \":/a\" check above in that :/in only _looks_\n+# like a pathspec, but doesn't match an actual file. So we prefer\n+# it as a rev.\n+test_expect_success '\"git log :/in\" should not be ambiguous' '\n+\tgit log :/in\n+'\n+\n+test_expect_success '\"git log :^sub\" should not be ambiguous' '\n+\tgit log :^sub\n+'\n+\n+test_expect_success '\"git log :(exclude)sub\" should not be ambiguous' '\n+\tgit log \":(exclude)sub\"\n+'\n+\n+test_expect_success '\"git log :%foo\" should complain of bad magic' '\n+\ttest_must_fail git log :%foo 2>error &&\n+\ttest_i18ngrep pathspec.magic error\n+'\n+\n test_done\n-- \n2.13.0.496.ge44ba89db\n"},{"id":"320782","messageId":"xmqq4lw8cql8.fsf@gitster.mtv.corp.google.com","threadId":"46041","inReplyTo":"20170525152739.t63dbsq2dojy2y2h@sigill.intra.peff.net","subject":"Re: [RFC/PATCH] recognize pathspec magic without \"--\" disambiguation","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-05-26T02:13:55Z","receivedAt":"2017-05-26T02:14:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> But let's consider related invocations and whether we're\n> making them better or worse:\n>\n> - git log :/foo\n> (when \"foo\" matches a commit message)\n>\n> This one should remain the same. Like the existing\n> wildcard rule, we're touching only verify_filename(),\n> not verify_non_filename(). So cases that _do_ resolve\n> as a rev will continue to do so.\n>\n> - git log :^foo\n> (when \"^foo\" exists in your index)\n>\n> The same logic applies; it would continue to work. And\n> ditto for any other weird filenames in your index like\n> \"(attr)foo\".\n\n\"git show :$path\" where $path happens to be \"^foo\" would grab the\ncontents of the $path out of the index and I think that is what you\nmeant, but use of \"git log\" in the example made me scratch my head\ngreatly.\n\n> - git log :/foo\n> (when \"foo\" does _not_ match a commit message)\n>\t...\n> This same downside actually exists currently when you\n> have an asterisk in your regex. E.g.,\n>\n> git log :/foo.*bar\n>\n> will be treated as a pathspec (if it doesn't match a\n> commit message) due to the wildcard matching in\n> 28fcc0b71.\n\nIn other words, we are not making things worse?\n\n> I wrote all the above to try to convince myself that this\n> doesn't have any serious regression cases. And I think I\n> did.\n>\n> But I actually we could make the rules in alternative (2)\n> above work. check_filename() would ask the pathspec code to\n> parse each argument and get one of three results:\n>\n> 1. it's not pathspec magic; treat it like a filename\n> (e.g., just \"foo\", or even bogus stuff like \":%foo\")\n>\n> 2. it is pathspec magic, and here is the matching filename\n> that ought to exist (e.g., \"foo\" from \":^foo\" or\n> \":(exclude)foo\")\n>\n> 3. it is pathspec magic, but there's no matching filename.\n> Assume it's a pathspec (e.g., \"(attr)foo\").\n>\n> I'm on the fence on whether it's worth the trouble versus\n> the simple rule implemented by this patch.\n\nUnlike \"git log builtin-checkout.c\" that errors out (only because\nthere is no such file in the checkout of the current version) and\nmakes its solution obvious to the users, this change has the risk of\nsilently accepting an ambiguous input and computing result that is\ndifferent from what the user intended to. So I am not sure. \n\nAs you pointedout, \":/\" especially does look like a likely point of\nfailure, in that both \"this is path at the top\" pathspec magic and\n\"the commit with this string\" are not something that we can say with\nconfidence that are rarely used because they are so esoteric.\n\nAs to \"is it OK to build a rule that we cannot explain easily?\", I\nthink it is OK to say \"if it is not a rev, and if it is not a\npathname in the current working tree, you must disambiguate, but Git\nhelps by guessing in some cases---if you want to have more control\n(e.g. you are a script), explicitly disambiguate and you'd be OK\",\nand leave the \"some cases\" vague, as long as we are only making\nreasonably conservative guesses.\n"},{"id":"320814","messageId":"20170526132432.zcoml5vphrzd557t@sigill.intra.peff.net","threadId":"46041","inReplyTo":"xmqq4lw8cql8.fsf@gitster.mtv.corp.google.com","subject":"Re: [RFC/PATCH] recognize pathspec magic without \"--\" disambiguation","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-05-26T13:24:32Z","receivedAt":"2017-05-26T13:24:41Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, May 26, 2017 at 11:13:55AM +0900, Junio C Hamano wrote:\n\n> > - git log :^foo\n> > (when \"^foo\" exists in your index)\n> >\n> > The same logic applies; it would continue to work. And\n> > ditto for any other weird filenames in your index like\n> > \"(attr)foo\".\n> \n> \"git show :$path\" where $path happens to be \"^foo\" would grab the\n> contents of the $path out of the index and I think that is what you\n> meant, but use of \"git log\" in the example made me scratch my head\n> greatly.\n\nYeah, sorry about that confusion. My original motivation for this was\nfor use with git-grep, but since the existing tests all used \"git log\",\nI tried to switch to that for consistency. But index paths obviously\nmake no sense with \"git log\" (we _do_ still parse it the same as we\nwould for git-show, etc, even though git-log doesn't do anything with\nit).\n\nAs an aside, I wonder if git-log should issue a warning when there are\npending objects that it doesn't handle.\n\n> > - git log :/foo\n> > (when \"foo\" does _not_ match a commit message)\n> >\t...\n> > This same downside actually exists currently when you\n> > have an asterisk in your regex. E.g.,\n> >\n> > git log :/foo.*bar\n> >\n> > will be treated as a pathspec (if it doesn't match a\n> > commit message) due to the wildcard matching in\n> > 28fcc0b71.\n> \n> In other words, we are not making things worse?\n\nWe're making them slightly worse. The \"problem\" used to trigger with\n\":/foo.*bar\", and now it would trigger with \":/foobar\", too. I don't\nknow if that's a meaningful distinction, though.\n\n> Unlike \"git log builtin-checkout.c\" that errors out (only because\n> there is no such file in the checkout of the current version) and\n> makes its solution obvious to the users, this change has the risk of\n> silently accepting an ambiguous input and computing result that is\n> different from what the user intended to. So I am not sure. \n\nRight, that's what I was trying to catalogue in the commit message.\n\n> As you pointedout, \":/\" especially does look like a likely point of\n> failure, in that both \"this is path at the top\" pathspec magic and\n> \"the commit with this string\" are not something that we can say with\n> confidence that are rarely used because they are so esoteric.\n> \n> As to \"is it OK to build a rule that we cannot explain easily?\", I\n> think it is OK to say \"if it is not a rev, and if it is not a\n> pathname in the current working tree, you must disambiguate, but Git\n> helps by guessing in some cases---if you want to have more control\n> (e.g. you are a script), explicitly disambiguate and you'd be OK\",\n> and leave the \"some cases\" vague, as long as we are only making\n> reasonably conservative guesses.\n\nI don't know. That is punting on the issue by not making any promises.\nBut that's a small consolation to somebody who does:\n\n $ git log :^foo\n [ok, works great...]\n $ git log :/foo\n fatal: ambiguous argument ':/foo': unknown revision or path not in the working tree.\n [stupid git! why don't you work?]\n\nAn explanation like \"it's complicated, and the rules are meant to be\nconservative and avoid confusion\" does not really help Git's reputation\nfor being arcane.\n\nI dunno. The one saving grace of \":/\" is that we actually do handle\nthe \":/foo\" case completely in check_filename(). So going back to my\n:/foobar example: if you have a file \"foobar\" in your root directory, it\n_is_ considered ambiguous.\n\nSo if that was the one exception, it would probably be OK in practice.\n\nWhich again makes me feel a bit like the right solution is doing the\nexistence checks on the non-magic portion of the pathspec for more magic\nbesides \":/\".\n\nUnfortunately, since writing my last message I did look into asking the\npathspec code to parse the arguments for us, but I think it would take\nquite a bit of refactoring. It's very ready to die() or emit warnings on\nbogus pathspecs, so it's not a good match for this kind of speculative\n\"is it a pathspec\" test.\n\nThe middle ground would be to replicate a simple subset of the rules in\nverify_filename(). If we assume that all arguments with \":(\" are\npathspecs (which seem rather unlikely to have false positives), then\nthat leaves only a few short-magic patterns: :/, :!, and :^.\n\nWe already specially handle :/ here. So it would really just be adding\nthe other two (which are just aliases of each other). It's not that much\ncode. The dirty thing is just that we're replicating some of the\npathspec logic, and any new magic would have to be updated here, too.\n\nI'll see if I can work up a patch.\n\n-Peff\n"},{"id":"320832","messageId":"20170526190607.jr44oygs6mzm3q6e@sigill.intra.peff.net","threadId":"46041","inReplyTo":"20170526132432.zcoml5vphrzd557t@sigill.intra.peff.net","subject":"[PATCH 0/6] make \"^:foo\" work without disambiguating \"--\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-05-26T19:06:07Z","receivedAt":"2017-05-26T19:06:31Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, May 26, 2017 at 09:24:32AM -0400, Jeff King wrote:\n\n> The middle ground would be to replicate a simple subset of the rules in\n> verify_filename(). If we assume that all arguments with \":(\" are\n> pathspecs (which seem rather unlikely to have false positives), then\n> that leaves only a few short-magic patterns: :/, :!, and :^.\n> \n> We already specially handle :/ here. So it would really just be adding\n> the other two (which are just aliases of each other). It's not that much\n> code. The dirty thing is just that we're replicating some of the\n> pathspec logic, and any new magic would have to be updated here, too.\n> \n> I'll see if I can work up a patch.\n\nSo here's what I came up with. TBH, I could live without patch 5. What I\ncare most about is making the \":^\" work. But I don't really see a\ndownside to it.\n\n [1/6]: t4208: add check for \":/\" without matching file\n [2/6]: check_filename(): refactor \":/\" handling\n [3/6]: check_filename(): use skip_prefix\n [4/6]: check_filename(): handle \":^\" path magic\n [5/6]: verify_filename(): treat \":(magic)\" as a pathspec\n [6/6]: verify_filename(): flip order of checks\n\n setup.c | 42 ++++++++++++++++++++++++++++++++----------\n t/t4208-log-magic-pathspec.sh | 32 ++++++++++++++++++++++++++++++++\n 2 files changed, 64 insertions(+), 10 deletions(-)\n\n-Peff\n"},{"id":"320833","messageId":"20170526190641.s4pwrb4ty7zfdmcd@sigill.intra.peff.net","threadId":"46041","inReplyTo":"20170526190607.jr44oygs6mzm3q6e@sigill.intra.peff.net","subject":"[PATCH 1/6] t4208: add check for \":/\" without matching file","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-05-26T19:06:41Z","receivedAt":"2017-05-26T19:07:08Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The DWIM magic in check_filename() doesn't just recognize\n\":/\". It actually makes sure that the file it points to\nexists. t4208 checks only the case where the path is\npresent, not the opposite. Since the next patches will be\ntouching this area, let's add a test to make sure it\ncontinues working.\n\nSigned-off-by: Jeff King \n---\n t/t4208-log-magic-pathspec.sh | 6 ++++++\n 1 file changed, 6 insertions(+)\n\ndiff --git a/t/t4208-log-magic-pathspec.sh b/t/t4208-log-magic-pathspec.sh\nindex 001343e2f..70bc64100 100755\n--- a/t/t4208-log-magic-pathspec.sh\n+++ b/t/t4208-log-magic-pathspec.sh\n@@ -29,6 +29,12 @@ test_expect_success '\"git log -- :/a\" should not be ambiguous' '\n \tgit log -- :/a\n '\n \n+# This differs from the \":/a\" check above in that :/in looks like a pathspec,\n+# but doesn't match an actual file.\n+test_expect_success '\"git log :/in\" should not be ambiguous' '\n+\tgit log :/in\n+'\n+\n test_expect_success '\"git log :\" should be ambiguous' '\n \ttest_must_fail git log : 2>error &&\n \ttest_i18ngrep ambiguous error\n-- \n2.13.0.496.ge44ba89db\n\n"},{"id":"320834","messageId":"20170526190742.d53haiwyr3uegbub@sigill.intra.peff.net","threadId":"46041","inReplyTo":"20170526190607.jr44oygs6mzm3q6e@sigill.intra.peff.net","subject":"[PATCH 3/6] check_filename(): use skip_prefix","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-05-26T19:07:42Z","receivedAt":"2017-05-26T19:08:14Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This avoids some magic numbers (and we'll be adding more\nsimilar calls in a minute).\n\nSigned-off-by: Jeff King \n---\n setup.c | 5 ++---\n 1 file changed, 2 insertions(+), 3 deletions(-)\n\ndiff --git a/setup.c b/setup.c\nindex 000ffa810..f2a8070bd 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -137,10 +137,9 @@ int check_filename(const char *prefix, const char *arg)\n \tchar *to_free = NULL;\n \tstruct stat st;\n \n-\tif (starts_with(arg, \":/\")) {\n-\t\tif (arg[2] == '\\0') /* \":/\" is root dir, always exists */\n+\tif (skip_prefix(arg, \":/\", &arg)) {\n+\t\tif (!*arg) /* \":/\" is root dir, always exists */\n \t\t\treturn 1;\n-\t\targ += 2;\n \t\tprefix = NULL;\n \t}\n \n-- \n2.13.0.496.ge44ba89db\n\n"},{"id":"320835","messageId":"20170526190731.dlllzhqqnpsyvqva@sigill.intra.peff.net","threadId":"46041","inReplyTo":"20170526190607.jr44oygs6mzm3q6e@sigill.intra.peff.net","subject":"[PATCH 2/6] check_filename(): refactor \":/\" handling","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-05-26T19:07:31Z","receivedAt":"2017-05-26T19:08:44Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We handle arguments with the \":/\" pathspec magic specially,\nmaking sure the name exists at the top-level. We'll want to\nhandle more pathspec magic in future patches, so let's do a\nlittle rearranging to make that easier.\n\nInstead of relying on an if/else cascade to avoid the\nprefix_filename() call, we'll just set prefix to NULL.\nLikewise, we'll get rid of the \"name\" variable entirely, and\njust push the \"arg\" pointer forward to skip past the magic.\nThat means by the time we get to the prefix-handling, we're\nset up appropriately whether we saw \":/\" or not.\n\nNote that this does impact the final error message we\nproduce when stat() fails, as it shows \"arg\" (which we'll\nhave modified to skip magic and include the prefix). This is\na good thing; the original message would say something like\n\"failed to stat ':/foo'\", which is confusing (we tried to\nstat \"foo\").\n\nSigned-off-by: Jeff King \n---\n setup.c | 15 ++++++++-------\n 1 file changed, 8 insertions(+), 7 deletions(-)\n\ndiff --git a/setup.c b/setup.c\nindex 0309c2782..000ffa810 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -134,19 +134,20 @@ int path_inside_repo(const char *prefix, const char *path)\n \n int check_filename(const char *prefix, const char *arg)\n {\n-\tconst char *name;\n \tchar *to_free = NULL;\n \tstruct stat st;\n \n \tif (starts_with(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 (prefix)\n-\t\tname = to_free = prefix_filename(prefix, arg);\n-\telse\n-\t\tname = arg;\n-\tif (!lstat(name, &st)) {\n+\t\targ += 2;\n+\t\tprefix = NULL;\n+\t}\n+\n+\tif (prefix)\n+\t\targ = to_free = prefix_filename(prefix, arg);\n+\n+\tif (!lstat(arg, &st)) {\n \t\tfree(to_free);\n \t\treturn 1; /* file exists */\n \t}\n-- \n2.13.0.496.ge44ba89db\n\n"},{"id":"320836","messageId":"20170526190839.773oeulwzxnl3x3n@sigill.intra.peff.net","threadId":"46041","inReplyTo":"20170526190607.jr44oygs6mzm3q6e@sigill.intra.peff.net","subject":"[PATCH 4/6] check_filename(): handle \":^\" path magic","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-05-26T19:08:39Z","receivedAt":"2017-05-26T19:09:17Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We special-case \"git log :/foo\" to work when \"foo\" exists in\nthe working tree. But :^ (and its alias :!) do not get the\nsame treatment, requiring the user to supply a\ndisambiguating \"--\". Let's make them work without requiring\nthe user to type the \"--\".\n\nSigned-off-by: Jeff King \n---\n setup.c | 4 ++++\n t/t4208-log-magic-pathspec.sh | 13 +++++++++++++\n 2 files changed, 17 insertions(+)\n\ndiff --git a/setup.c b/setup.c\nindex f2a8070bd..86bb7c9a9 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -141,6 +141,10 @@ int check_filename(const char *prefix, const char *arg)\n \t\tif (!*arg) /* \":/\" is root dir, always exists */\n \t\t\treturn 1;\n \t\tprefix = NULL;\n+\t} else if (skip_prefix(arg, \":!\", &arg) ||\n+\t\t skip_prefix(arg, \":^\", &arg)) {\n+\t\tif (!*arg) /* excluding everything is silly, but allowed */\n+\t\t\treturn 1;\n \t}\n \n \tif (prefix)\ndiff --git a/t/t4208-log-magic-pathspec.sh b/t/t4208-log-magic-pathspec.sh\nindex 70bc64100..67250ebdc 100755\n--- a/t/t4208-log-magic-pathspec.sh\n+++ b/t/t4208-log-magic-pathspec.sh\n@@ -52,6 +52,19 @@ test_expect_success 'git log HEAD -- :/' '\n \ttest_cmp expected actual\n '\n \n+test_expect_success '\"git log :^sub\" is not ambiguous' '\n+\tgit log :^sub\n+'\n+\n+test_expect_success '\"git log :^does-not-exist\" does not match anything' '\n+\ttest_must_fail git log :^does-not-exist\n+'\n+\n+test_expect_success '\"git log :!\" behaves the same as :^' '\n+\tgit log :!sub &&\n+\ttest_must_fail git log :!does-not-exist\n+'\n+\n test_expect_success 'command line pathspec parsing for \"git log\"' '\n \tgit reset --hard &&\n \t>a &&\n-- \n2.13.0.496.ge44ba89db\n\n"},{"id":"320839","messageId":"20170526191053.v2mmx7oour6u5chp@sigill.intra.peff.net","threadId":"46041","inReplyTo":"20170526190607.jr44oygs6mzm3q6e@sigill.intra.peff.net","subject":"[PATCH 6/6] verify_filename(): flip order of checks","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-05-26T19:10:53Z","receivedAt":"2017-05-26T19:11:10Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The looks_like_pathspec() check is much cheaper than\ncheck_filename(), which actually stats the file. Since\neither is sufficient for our return value, we should do the\ncheaper one first, potentially short-circuiting the other.\n\nSigned-off-by: Jeff King \n---\nProbably doesn't matter, but it bugged me and it was subtle enough that\nI pulled it out into its own commit.\n\n setup.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/setup.c b/setup.c\nindex 89fcc12ab..1de87ed84 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -229,7 +229,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) || looks_like_pathspec(arg))\n+\tif (looks_like_pathspec(arg) || check_filename(prefix, arg))\n \t\treturn;\n \tdie_verify_filename(prefix, arg, diagnose_misspelt_rev);\n }\n-- \n2.13.0.496.ge44ba89db\n"},{"id":"320841","messageId":"20170526191031.vqvkdkdp6kv6krgo@sigill.intra.peff.net","threadId":"46041","inReplyTo":"20170526190607.jr44oygs6mzm3q6e@sigill.intra.peff.net","subject":"[PATCH 5/6] verify_filename(): treat \":(magic)\" as a pathspec","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-05-26T19:10:31Z","receivedAt":"2017-05-26T19:11:24Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"For commands that take revisions and pathspecs, magic\npathspecs like \":(exclude)foo\" require the user to specify\na disambiguating \"--\", since they do not match a file in the\nfilesystem, like:\n\n git grep foo -- :(exclude)bar\n\nThis makes them more annoying to use than they need to be.\nWe loosened the rules for wildcards in 28fcc0b71 (pathspec:\navoid the need of \"--\" when wildcard is used, 2015-05-02).\nLet's do the same for pathspecs with long-form magic.\n\nWe already handle the short-forms \":/\" and \":^\" specially in\ncheck_filename(), so we don't need to handle them here. And\nin fact, we could do the same with long-form magic, parsing\nout the actual filename and making sure it exists. But there\nare a few reasons not to do it that way:\n\n - the parsing gets much more complicated, and we'd want to\n hand it off to the pathspec code. But that code isn't\n ready to do this kind of speculative parsing (it's happy\n to die() when it sees a syntactically invalid pathspec).\n\n - not all pathspec magic maps to a filesystem path. E.g.,\n :(attr) should be treated as a pathspec regardless of\n what is in the filesystem\n\n - we can be a bit looser with \":(\" than with the\n short-form \":/\", because it is much less likely to have\n a false positive. Whereas \":/\" also means \"search for a\n commit with this regex\".\n\nNote that because the change is in verify_filename() and not\nin its helper check_filename(), this doesn't affect the\nverify_non_filename() case. I.e., if an item that matches\nour new rule doesn't resolve as an object, we may fallback\nto treating it as a pathspec (rather than complaining it\ndoesn't exist). But if it does resolve (e.g., as a file in\nthe index that starts with an open-paren), we won't then\ncomplain that it's also a valid pathspec. This matches the\nwildcard-exception behavior.\n\nAnd of course in either case, one can always insert the \"--\"\nto get more precise results.\n\nSigned-off-by: Jeff King \n---\n setup.c | 20 +++++++++++++++++++-\n t/t4208-log-magic-pathspec.sh | 13 +++++++++++++\n 2 files changed, 32 insertions(+), 1 deletion(-)\n\ndiff --git a/setup.c b/setup.c\nindex 86bb7c9a9..89fcc12ab 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -185,6 +185,24 @@ static void NORETURN die_verify_filename(const char *prefix,\n \n }\n \n+/*\n+ * Check for arguments that don't resolve as actual files,\n+ * but which look sufficiently like pathspecs that we'll consider\n+ * them such for the purposes of rev/pathspec DWIM parsing.\n+ */\n+static int looks_like_pathspec(const char *arg)\n+{\n+\t/* anything with a wildcard character */\n+\tif (!no_wildcard(arg))\n+\t\treturn 1;\n+\n+\t/* long-form pathspec magic */\n+\tif (starts_with(arg, \":(\"))\n+\t\treturn 1;\n+\n+\treturn 0;\n+}\n+\n /*\n * Verify a filename that we got as an argument for a pathspec\n * entry. Note that a filename that begins with \"-\" never verifies\n@@ -211,7 +229,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) || !no_wildcard(arg))\n+\tif (check_filename(prefix, arg) || looks_like_pathspec(arg))\n \t\treturn;\n \tdie_verify_filename(prefix, arg, diagnose_misspelt_rev);\n }\ndiff --git a/t/t4208-log-magic-pathspec.sh b/t/t4208-log-magic-pathspec.sh\nindex 67250ebdc..25129dfa0 100755\n--- a/t/t4208-log-magic-pathspec.sh\n+++ b/t/t4208-log-magic-pathspec.sh\n@@ -65,6 +65,19 @@ test_expect_success '\"git log :!sub\" behaves the same as :^' '\n \ttest_must_fail git log :!does-not-exist\n '\n \n+test_expect_success '\"git log :(exclude)sub\" is not ambiguous' '\n+\tgit log \":(exclude)sub\"\n+'\n+\n+test_expect_success '\"git log :(exclude)sub --\" must resolve as an object' '\n+\ttest_must_fail git log \":(exclude)sub\" --\n+'\n+\n+test_expect_success '\"git log :(unknown-magic) complains of bogus magic' '\n+\ttest_must_fail git log \":(unknown-magic)\" 2>error &&\n+\ttest_i18ngrep pathspec.magic error\n+'\n+\n test_expect_success 'command line pathspec parsing for \"git log\"' '\n \tgit reset --hard &&\n \t>a &&\n-- \n2.13.0.496.ge44ba89db\n\n"},{"id":"320865","messageId":"CACBZZX6K7ppVB0qYah76_+pjTKjsco3rHT0xRyKtF2H1dS4k_w@mail.gmail.com","threadId":"46041","inReplyTo":"20170525152739.t63dbsq2dojy2y2h@sigill.intra.peff.net","subject":"Re: [RFC/PATCH] recognize pathspec magic without \"--\" disambiguation","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-05-27T09:54:07Z","receivedAt":"2017-05-27T09:54:34Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Thu, May 25, 2017 at 5:27 PM, Jeff King wrote:\n> git log :/foo.*bar\n\nAnother option would be to deprecate the :/rx syntax over some period\nin favor of ^{/rx}.\n\nI think it's too ugly to live, and really useless. It's equivalent to\n\"--grep= --all\". Does anyone use this and not really mean to use\n^{/rx}? E.g. \"git show :/fix\" might show a fix on some unrelated\nbranch you recently rebased.\n\n> will be treated as a pathspec (if it doesn't match a\n> commit message) due to the wildcard matching in\n> 28fcc0b71.\n\nSo it might DWYM after hanging there looking at your entire history\nfor a commit message matching foo.*bar? And if you make such a commit\nit'll start meaning something else entirely?\n"},{"id":"320872","messageId":"20170527203937.hto7lwxfbjvl5f7o@sigill.intra.peff.net","threadId":"46041","inReplyTo":"CACBZZX6K7ppVB0qYah76_+pjTKjsco3rHT0xRyKtF2H1dS4k_w@mail.gmail.com","subject":"Re: [RFC/PATCH] recognize pathspec magic without \"--\" disambiguation","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-05-27T20:39:38Z","receivedAt":"2017-05-27T20:39:46Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, May 27, 2017 at 11:54:07AM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n> On Thu, May 25, 2017 at 5:27 PM, Jeff King wrote:\n> > git log :/foo.*bar\n> \n> Another option would be to deprecate the :/rx syntax over some period\n> in favor of ^{/rx}.\n\nYeah, the latter is more flexible (can start at a tip of your choosing)\nand syntactically matches other object selectors much better.\n\n> I think it's too ugly to live, and really useless. It's equivalent to\n> \"--grep= --all\". Does anyone use this and not really mean to use\n> ^{/rx}? E.g. \"git show :/fix\" might show a fix on some unrelated\n> branch you recently rebased.\n\nIt's actually _worse_ than that --grep because it only picks one result.\nSo not only do we look at unrelated branches, but they may take\nprecedence (based on commit timestamp) over the current branch.\n\nIt is shorter than \"HEAD^{/re}\", though. So I suspect people do use it\nand would be slightly annoyed if it went away.\n\n> > will be treated as a pathspec (if it doesn't match a\n> > commit message) due to the wildcard matching in\n> > 28fcc0b71.\n> \n> So it might DWYM after hanging there looking at your entire history\n> for a commit message matching foo.*bar? And if you make such a commit\n> it'll start meaning something else entirely?\n\nYeah. That's a good reason not to use \":/\" without a disambiguating \"--\"\n(which _does_ work, even without my series, because check_filename()\ndoes specific path-matching). At best, you pay for a complete useless\nhistory traversal before the command actually starts running. But much\nmore likely is that Git just complains of ambiguity, because people tend\nto mention top-level paths in their commit messages. E.g.:\n\n $ cd t\n $ git grep foo :/Documentation\n fatal: ambiguous argument ':/Documentation': both revision and filename\n\nSo it really is a pretty horrible syntax.\n\n-Peff\n"},{"id":"320876","messageId":"xmqqd1atc0eh.fsf@gitster.mtv.corp.google.com","threadId":"46041","inReplyTo":"20170527203937.hto7lwxfbjvl5f7o@sigill.intra.peff.net","subject":"Re: [RFC/PATCH] recognize pathspec magic without \"--\" disambiguation","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-05-28T00:04:06Z","receivedAt":"2017-05-28T00:04:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Yeah. That's a good reason not to use \":/\" without a disambiguating \"--\"\n> (which _does_ work, even without my series, because check_filename()\n> does specific path-matching). At best, you pay for a complete useless\n> history traversal before the command actually starts running. But much\n> more likely is that Git just complains of ambiguity, because people tend\n> to mention top-level paths in their commit messages. E.g.:\n>\n> $ cd t\n> $ git grep foo :/Documentation\n> fatal: ambiguous argument ':/Documentation': both revision and filename\n>\n> So it really is a pretty horrible syntax.\n\nIt does not allow it to be further annotated like \"HEAD^{/^Merge\nbranch 'foo'}^2\". Yes, I agree that \":/string\" was a pretty poor\ndesign that was done without thinking things through.\n\nBut back then in 2007, it is a bit unfair to blame the initial\ndesign for not thinking things through. There weren't as many\nprecedents, good or bad, to learn from than we have today ;-)\n\n"}]}