{"thread":{"id":"45135","subject":"[PATCH for NEXT] grep: do not unnecessarily query repo for \"--\"","startedAt":"2017-02-14T00:12:15Z","lastAt":"2017-02-14T21:59:06Z","messageCount":22,"participants":["Jonathan Tan","Jeff King","Brandon Williams","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"311478","messageId":"20170214001159.19079-1-jonathantanmy@google.com","threadId":"45135","inReplyTo":null,"subject":"[PATCH for NEXT] grep: do not unnecessarily query repo for \"--\"","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2017-02-14T00:11:59Z","receivedAt":"2017-02-14T00:12:15Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"When running a command of the form\n\n git grep --no-index pattern -- path\n\nin the absence of a Git repository, an error message will be printed:\n\n fatal: BUG: setup_git_env called without repository\n\nThis is because \"git grep\" tries to interpret \"--\" as a rev. \"git grep\"\nhas always tried to first interpret \"--\" as a rev for at least a few\nyears, but this issue was upgraded from a pessimization to a bug in\ncommit 59332d1 (\"Resurrect \"git grep --no-index\"\", 2010-02-06), which\ncalls get_sha1 regardless of whether --no-index was specified. This bug\nappeared to be benign until commit b1ef400 (\"setup_git_env: avoid blind\nfall-back to \".git\"\", 2016-10-20) when Git was taught to die in this\nsituation. (This \"git grep\" bug appears to be one of the bugs that\ncommit b1ef400 is meant to flush out.)\n\nTherefore, always interpret \"--\" as signaling the end of options,\ninstead of trying to interpret it as a rev first.\n\nSigned-off-by: Jonathan Tan \n---\n\nThere is probably a similar bug for commands of the form:\n\n git grep --no-index pattern foo\n\nIf there is a repo and \"foo\" is a rev, the \"--no-index or --untracked\ncannot be used with revs.\" error would occur. If there is a repo and\n\"foo\" is not a rev, this command would proceed as usual. If there is no\nrepo, the \"setup_git_env called without repository\" error would occur.\n(This is my understanding from reading the code - I haven't tested it\nout.)\n\nThis patch does not fix this similar bug, but I decided to send it out\nanyway because it still fixes a bug and unlocks the ability to\nspecify paths with \"git grep --no-index\".\n\n builtin/grep.c | 9 +++++----\n t/t7810-grep.sh | 12 ++++++++++++\n 2 files changed, 17 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex 2c727ef49..1b68d1638 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -1154,6 +1154,11 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n \t\tconst char *arg = argv[i];\n \t\tunsigned char sha1[20];\n \t\tstruct object_context oc;\n+\t\tif (!strcmp(arg, \"--\")) {\n+\t\t\ti++;\n+\t\t\tseen_dashdash = 1;\n+\t\t\tbreak;\n+\t\t}\n \t\t/* Is it a rev? */\n \t\tif (!get_sha1_with_context(arg, 0, sha1, &oc)) {\n \t\t\tstruct object *object = parse_object_or_die(sha1, arg);\n@@ -1162,10 +1167,6 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n \t\t\tadd_object_array_with_path(object, arg, &list, oc.mode, oc.path);\n \t\t\tcontinue;\n \t\t}\n-\t\tif (!strcmp(arg, \"--\")) {\n-\t\t\ti++;\n-\t\t\tseen_dashdash = 1;\n-\t\t}\n \t\tbreak;\n \t}\n \ndiff --git a/t/t7810-grep.sh b/t/t7810-grep.sh\nindex 19f0108f8..29202f0e7 100755\n--- a/t/t7810-grep.sh\n+++ b/t/t7810-grep.sh\n@@ -982,6 +982,18 @@ test_expect_success 'grep -e -- -- path' '\n \ttest_cmp expected actual\n '\n \n+test_expect_success 'grep --no-index pattern -- path' '\n+\trm -fr non &&\n+\tmkdir -p non/git &&\n+\t(\n+\t\tGIT_CEILING_DIRECTORIES=\"$(pwd)/non\" &&\n+\t\texport GIT_CEILING_DIRECTORIES &&\n+\t\tcd non/git &&\n+\t\techo hello >hello &&\n+\t\tgit grep --no-index o -- .\n+\t)\n+'\n+\n cat >expected < When running a command of the form\n> \n> git grep --no-index pattern -- path\n> \n> in the absence of a Git repository, an error message will be printed:\n> \n> fatal: BUG: setup_git_env called without repository\n> \n> This is because \"git grep\" tries to interpret \"--\" as a rev. \"git grep\"\n> has always tried to first interpret \"--\" as a rev for at least a few\n> years, but this issue was upgraded from a pessimization to a bug in\n> commit 59332d1 (\"Resurrect \"git grep --no-index\"\", 2010-02-06), which\n> calls get_sha1 regardless of whether --no-index was specified. This bug\n> appeared to be benign until commit b1ef400 (\"setup_git_env: avoid blind\n> fall-back to \".git\"\", 2016-10-20) when Git was taught to die in this\n> situation. (This \"git grep\" bug appears to be one of the bugs that\n> commit b1ef400 is meant to flush out.)\n> \n> Therefore, always interpret \"--\" as signaling the end of options,\n> instead of trying to interpret it as a rev first.\n\nNicely explained.\n\n> There is probably a similar bug for commands of the form:\n> \n> git grep --no-index pattern foo\n> \n> If there is a repo and \"foo\" is a rev, the \"--no-index or --untracked\n> cannot be used with revs.\" error would occur. If there is a repo and\n> \"foo\" is not a rev, this command would proceed as usual. If there is no\n> repo, the \"setup_git_env called without repository\" error would occur.\n> (This is my understanding from reading the code - I haven't tested it\n> out.)\n\nYes, it's easy to see that \"git grep --no-index foo bar\" outside of a\nrepo generates the same BUG. I suspect that \"--no-index\" should just\ndisable looking up revs entirely, even if we are actually in a\nrepository directory.\n\n> This patch does not fix this similar bug, but I decided to send it out\n> anyway because it still fixes a bug and unlocks the ability to\n> specify paths with \"git grep --no-index\".\n\nYes, I think even if we fix the other bug, fixing this \"--\" thing is an\nimprovement.\n\n> diff --git a/builtin/grep.c b/builtin/grep.c\n> index 2c727ef49..1b68d1638 100644\n> --- a/builtin/grep.c\n> +++ b/builtin/grep.c\n> @@ -1154,6 +1154,11 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n> \t\tconst char *arg = argv[i];\n> \t\tunsigned char sha1[20];\n> \t\tstruct object_context oc;\n> +\t\tif (!strcmp(arg, \"--\")) {\n> +\t\t\ti++;\n> +\t\t\tseen_dashdash = 1;\n> +\t\t\tbreak;\n> +\t\t}\n> \t\t/* Is it a rev? */\n> \t\tif (!get_sha1_with_context(arg, 0, sha1, &oc)) {\n> \t\t\tstruct object *object = parse_object_or_die(sha1, arg);\n\nSo I think this is a definite improvement, but I see a few leftover\noddities:\n\n - the end logic for this loop is now:\n\n if (arg is a rev) {\n ... handle rev ...\n continue;\n }\n break;\n\n It would probably be more obvious as:\n\n if (arg is not a rev)\n break;\n ... handle rev ...\n\n - the rev-handling code does:\n\n if (!seen_dashdash)\n verify_non_filename(prefix, arg);\n\n But I do not see how seen_dashdash could ever be untrue. We set it\n inside this loop, and break immediately when we see it. And indeed,\n running:\n\n echo content >master\n git grep content master --\n\n does not work. The \"--\" should tell us that \"master\" is a rev, but\n we don't know yet that we have a dashdash.\n\n I think we need a separate loop to find the \"--\" first, and _then_\n walk through the arguments, treating them as revs or paths as\n appropriate. This is how setup_revisions() does it.\n\n So this isn't a problem introduced by your patch, but it's\n intimately related.\n\n> diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh\n> index 19f0108f8..29202f0e7 100755\n> --- a/t/t7810-grep.sh\n> +++ b/t/t7810-grep.sh\n> @@ -982,6 +982,18 @@ test_expect_success 'grep -e -- -- path' '\n> \ttest_cmp expected actual\n> '\n> \n> +test_expect_success 'grep --no-index pattern -- path' '\n> +\trm -fr non &&\n> +\tmkdir -p non/git &&\n> +\t(\n> +\t\tGIT_CEILING_DIRECTORIES=\"$(pwd)/non\" &&\n> +\t\texport GIT_CEILING_DIRECTORIES &&\n> +\t\tcd non/git &&\n> +\t\techo hello >hello &&\n> +\t\tgit grep --no-index o -- .\n> +\t)\n> +'\n\nSince de95302a4, you can do:\n\n nongit git grep --no-index -o -- .\n\nThough if this is destined for maint, it might need to be done\nseparately this way and cleaned up later.\n\nIt might also be a good idea to confirm that the pathspec is actually\nbeing respected in the --no-index case. Something like:\n\n echo hello >hello &&\n echo goodbye >goodbye &&\n echo hello:hello >expect &&\n git grep --no-index o -- hello >actual &&\n test_cmp expect actual\n\n-Peff\n"},{"id":"311511","messageId":"20170214060021.einv7372exbxa23z@sigill.intra.peff.net","threadId":"45135","inReplyTo":"20170214012037.u2eg2n7mvteullcx@sigill.intra.peff.net","subject":"[PATCH 0/7] grep rev/path parsing fixes","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-02-14T06:00:21Z","receivedAt":"2017-02-14T06:00:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 13, 2017 at 08:20:37PM -0500, Jeff King wrote:\n\n> > If there is a repo and \"foo\" is a rev, the \"--no-index or --untracked\n> > cannot be used with revs.\" error would occur. If there is a repo and\n> > \"foo\" is not a rev, this command would proceed as usual. If there is no\n> > repo, the \"setup_git_env called without repository\" error would occur.\n> > (This is my understanding from reading the code - I haven't tested it\n> > out.)\n> \n> Yes, it's easy to see that \"git grep --no-index foo bar\" outside of a\n> repo generates the same BUG. I suspect that \"--no-index\" should just\n> disable looking up revs entirely, even if we are actually in a\n> repository directory.\n\nI've fixed that, along with a few other bugs and cleanups. The complete\nseries is below. Patch 2 is your (untouched) patch. My suggestions for\nyour test are in patch 3, which can either remain on its own or be\nsquashed in.\n\n [1/7]: grep: move thread initialization a little lower\n [2/7]: grep: do not unnecessarily query repo for \"--\"\n [3/7]: t7810: make \"--no-index --\" test more robust\n [4/7]: grep: re-order rev-parsing loop\n [5/7]: grep: fix \"--\" rev/pathspec disambiguation\n [6/7]: grep: avoid resolving revision names in --no-index case\n [7/7]: grep: do not diagnose misspelt revs with --no-index\n\n builtin/grep.c | 78 +++++++++++++++++++++++++++++++++++++++------------------\n t/t7810-grep.sh | 66 ++++++++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 119 insertions(+), 25 deletions(-)\n\n-Peff\n"},{"id":"311512","messageId":"20170214060237.gvzzbmjzerlqnzjv@sigill.intra.peff.net","threadId":"45135","inReplyTo":"20170214060021.einv7372exbxa23z@sigill.intra.peff.net","subject":"[PATCH 1/7] grep: move thread initialization a little lower","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-02-14T06:02:38Z","receivedAt":"2017-02-14T06:02:44Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Originally, we set up the threads for grep before parsing\nthe non-option arguments. In 53b8d931b (grep: disable\nthreading in non-worktree case, 2011-12-12), the thread code\ngot bumped lower in the function because it now needed to\nknow whether we got any revision arguments.\n\nThat put a big block of code in between the parsing of revs\nand the parsing of pathspecs, both of which share some loop\nvariables. That makes it harder to read the code than the\noriginal, where the shared loops were right next to each\nother.\n\nLet's bump the thread initialization until after all of the\nparsing is done.\n\nSigned-off-by: Jeff King \n---\nI double-checked to make sure no other code was relying on\nthe thread setup having happened. I think we could actually\nbump it quite a bit lower (to right before we actually start\ngrepping), but I doubt it matters much in practice.\n\n builtin/grep.c | 28 ++++++++++++++--------------\n 1 file changed, 14 insertions(+), 14 deletions(-)\n\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex 2c727ef49..5a282c4d0 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -1169,6 +1169,20 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n \t\tbreak;\n \t}\n \n+\t/* The rest are paths */\n+\tif (!seen_dashdash) {\n+\t\tint j;\n+\t\tfor (j = i; j < argc; j++)\n+\t\t\tverify_filename(prefix, argv[j], j == i);\n+\t}\n+\n+\tparse_pathspec(&pathspec, 0,\n+\t\t PATHSPEC_PREFER_CWD |\n+\t\t (opt.max_depth != -1 ? PATHSPEC_MAXDEPTH_VALID : 0),\n+\t\t prefix, argv + i);\n+\tpathspec.max_depth = opt.max_depth;\n+\tpathspec.recursive = 1;\n+\n #ifndef NO_PTHREADS\n \tif (list.nr || cached || show_in_pager)\n \t\tnum_threads = 0;\n@@ -1190,20 +1204,6 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n \t}\n #endif\n \n-\t/* The rest are paths */\n-\tif (!seen_dashdash) {\n-\t\tint j;\n-\t\tfor (j = i; j < argc; j++)\n-\t\t\tverify_filename(prefix, argv[j], j == i);\n-\t}\n-\n-\tparse_pathspec(&pathspec, 0,\n-\t\t PATHSPEC_PREFER_CWD |\n-\t\t (opt.max_depth != -1 ? PATHSPEC_MAXDEPTH_VALID : 0),\n-\t\t prefix, argv + i);\n-\tpathspec.max_depth = opt.max_depth;\n-\tpathspec.recursive = 1;\n-\n \tif (recurse_submodules) {\n \t\tgitmodules_config();\n \t\tcompile_submodule_options(&opt, &pathspec, cached, untracked,\n-- \n2.12.0.rc1.471.ga79ec8999\n\n"},{"id":"311513","messageId":"20170214060303.7y7osfhwnxadcr32@sigill.intra.peff.net","threadId":"45135","inReplyTo":"20170214060021.einv7372exbxa23z@sigill.intra.peff.net","subject":"[PATCH 2/7] grep: do not unnecessarily query repo for \"--\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-02-14T06:03:03Z","receivedAt":"2017-02-14T06:03:09Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"From: Jonathan Tan \n\nWhen running a command of the form\n\n git grep --no-index pattern -- path\n\nin the absence of a Git repository, an error message will be printed:\n\n fatal: BUG: setup_git_env called without repository\n\nThis is because \"git grep\" tries to interpret \"--\" as a rev. \"git grep\"\nhas always tried to first interpret \"--\" as a rev for at least a few\nyears, but this issue was upgraded from a pessimization to a bug in\ncommit 59332d1 (\"Resurrect \"git grep --no-index\"\", 2010-02-06), which\ncalls get_sha1 regardless of whether --no-index was specified. This bug\nappeared to be benign until commit b1ef400 (\"setup_git_env: avoid blind\nfall-back to \".git\"\", 2016-10-20) when Git was taught to die in this\nsituation. (This \"git grep\" bug appears to be one of the bugs that\ncommit b1ef400 is meant to flush out.)\n\nTherefore, always interpret \"--\" as signaling the end of options,\ninstead of trying to interpret it as a rev first.\n\nSigned-off-by: Jonathan Tan \nSigned-off-by: Jeff King \n---\nUnchanged from your original.\n\n builtin/grep.c | 9 +++++----\n t/t7810-grep.sh | 12 ++++++++++++\n 2 files changed, 17 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex 5a282c4d0..081e1b57a 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -1154,6 +1154,11 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n \t\tconst char *arg = argv[i];\n \t\tunsigned char sha1[20];\n \t\tstruct object_context oc;\n+\t\tif (!strcmp(arg, \"--\")) {\n+\t\t\ti++;\n+\t\t\tseen_dashdash = 1;\n+\t\t\tbreak;\n+\t\t}\n \t\t/* Is it a rev? */\n \t\tif (!get_sha1_with_context(arg, 0, sha1, &oc)) {\n \t\t\tstruct object *object = parse_object_or_die(sha1, arg);\n@@ -1162,10 +1167,6 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n \t\t\tadd_object_array_with_path(object, arg, &list, oc.mode, oc.path);\n \t\t\tcontinue;\n \t\t}\n-\t\tif (!strcmp(arg, \"--\")) {\n-\t\t\ti++;\n-\t\t\tseen_dashdash = 1;\n-\t\t}\n \t\tbreak;\n \t}\n \ndiff --git a/t/t7810-grep.sh b/t/t7810-grep.sh\nindex 19f0108f8..29202f0e7 100755\n--- a/t/t7810-grep.sh\n+++ b/t/t7810-grep.sh\n@@ -982,6 +982,18 @@ test_expect_success 'grep -e -- -- path' '\n \ttest_cmp expected actual\n '\n \n+test_expect_success 'grep --no-index pattern -- path' '\n+\trm -fr non &&\n+\tmkdir -p non/git &&\n+\t(\n+\t\tGIT_CEILING_DIRECTORIES=\"$(pwd)/non\" &&\n+\t\texport GIT_CEILING_DIRECTORIES &&\n+\t\tcd non/git &&\n+\t\techo hello >hello &&\n+\t\tgit grep --no-index o -- .\n+\t)\n+'\n+\n cat >expected <\n---\nThis could be squashed into the previous patch.\n\nI didn't end up using the \"nongit\" helper, because we actually have to\ndo some other setup inside the subshell (this is the reason the earlier\ntests in this script don't use the helper, either).\n\n t/t7810-grep.sh | 5 ++++-\n 1 file changed, 4 insertions(+), 1 deletion(-)\n\ndiff --git a/t/t7810-grep.sh b/t/t7810-grep.sh\nindex 29202f0e7..2c1f7373e 100755\n--- a/t/t7810-grep.sh\n+++ b/t/t7810-grep.sh\n@@ -990,7 +990,10 @@ test_expect_success 'grep --no-index pattern -- path' '\n \t\texport GIT_CEILING_DIRECTORIES &&\n \t\tcd non/git &&\n \t\techo hello >hello &&\n-\t\tgit grep --no-index o -- .\n+\t\techo goodbye >goodbye &&\n+\t\techo hello:hello >expect &&\n+\t\tgit grep --no-index o -- hello >actual &&\n+\t\ttest_cmp expect actual\n \t)\n '\n \n-- \n2.12.0.rc1.471.ga79ec8999\n\n"},{"id":"311515","messageId":"20170214060417.yq27zdcgmrckjmua@sigill.intra.peff.net","threadId":"45135","inReplyTo":"20170214060021.einv7372exbxa23z@sigill.intra.peff.net","subject":"[PATCH 4/7] grep: re-order rev-parsing loop","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-02-14T06:04:17Z","receivedAt":"2017-02-14T06:04:24Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We loop over the arguments, but every branch of the loop\nhits either a \"continue\" or a \"break\". Surely we can make\nthis simpler.\n\nThe final conditional is:\n\n if (arg is a rev) {\n\t ... handle rev ...\n\t continue;\n }\n break;\n\nWe can rewrite this as:\n\n if (arg is not a rev)\n\t break;\n\n ... handle rev ...\n\nThat makes the flow a little bit simpler, and will make\nthings much easier to follow when we add more logic in\nfuture patches.\n\nSigned-off-by: Jeff King \n---\n builtin/grep.c | 20 +++++++++++---------\n 1 file changed, 11 insertions(+), 9 deletions(-)\n\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex 081e1b57a..461347adb 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -1154,20 +1154,22 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n \t\tconst char *arg = argv[i];\n \t\tunsigned char sha1[20];\n \t\tstruct object_context oc;\n+\t\tstruct object *object;\n+\n \t\tif (!strcmp(arg, \"--\")) {\n \t\t\ti++;\n \t\t\tseen_dashdash = 1;\n \t\t\tbreak;\n \t\t}\n-\t\t/* Is it a rev? */\n-\t\tif (!get_sha1_with_context(arg, 0, sha1, &oc)) {\n-\t\t\tstruct object *object = parse_object_or_die(sha1, arg);\n-\t\t\tif (!seen_dashdash)\n-\t\t\t\tverify_non_filename(prefix, arg);\n-\t\t\tadd_object_array_with_path(object, arg, &list, oc.mode, oc.path);\n-\t\t\tcontinue;\n-\t\t}\n-\t\tbreak;\n+\n+\t\t/* Stop at the first non-rev */\n+\t\tif (get_sha1_with_context(arg, 0, sha1, &oc))\n+\t\t\tbreak;\n+\n+\t\tobject = parse_object_or_die(sha1, arg);\n+\t\tif (!seen_dashdash)\n+\t\t\tverify_non_filename(prefix, arg);\n+\t\tadd_object_array_with_path(object, arg, &list, oc.mode, oc.path);\n \t}\n \n \t/* The rest are paths */\n-- \n2.12.0.rc1.471.ga79ec8999\n\n"},{"id":"311516","messageId":"20170214060555.yzh6hhi2t7pkeqvi@sigill.intra.peff.net","threadId":"45135","inReplyTo":"20170214060021.einv7372exbxa23z@sigill.intra.peff.net","subject":"[PATCH 5/7] grep: fix \"--\" rev/pathspec disambiguation","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-02-14T06:05:55Z","receivedAt":"2017-02-14T06:06:01Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"If we see \"git grep pattern rev -- file\" then we apply the\nusual rev/pathspec disambiguation rules: any \"rev\" before\nthe \"--\" must be a revision, and we do not need to apply the\nverify_non_filename() check.\n\nBut there are two bugs here:\n\n 1. We keep a seen_dashdash flag to handle this case, but\n we set it in the same left-to-right pass over the\n arguments in which we parse \"rev\".\n\n So when we see \"rev\", we do not yet know that there is\n a \"--\", and we mistakenly complain if there is a\n matching file.\n\n We can fix this by making a preliminary pass over the\n arguments to find the \"--\", and only then checking the rev\n arguments.\n\n 2. If we can't resolve \"rev\" but there isn't a dashdash,\n that's OK. We treat it like a path, and complain later\n if it doesn't exist.\n\n But if there _is_ a dashdash, then we know it must be a\n rev, and should treat it as such, complaining if it\n does not resolve. The current code instead ignores it\n and tries to treat it like a path.\n\nThis patch fixes both bugs, and tries to comment the parsing\nflow a bit better.\n\nIt adds tests that cover the two bugs, but also some related\nsituations (which already worked, but this confirms that our\nfixes did not break anything).\n\nSigned-off-by: Jeff King \n---\n builtin/grep.c | 29 ++++++++++++++++++++++++-----\n t/t7810-grep.sh | 33 +++++++++++++++++++++++++++++++++\n 2 files changed, 57 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex 461347adb..e83b33bda 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -1149,7 +1149,22 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n \n \tcompile_grep_patterns(&opt);\n \n-\t/* Check revs and then paths */\n+\t/*\n+\t * We have to find \"--\" in a separate pass, because its presence\n+\t * influences how we will parse arguments that come before it.\n+\t */\n+\tfor (i = 0; i < argc; i++) {\n+\t\tif (!strcmp(argv[i], \"--\")) {\n+\t\t\tseen_dashdash = 1;\n+\t\t\tbreak;\n+\t\t}\n+\t}\n+\n+\t/*\n+\t * Resolve any rev arguments. If we have a dashdash, then everything up\n+\t * to it must resolve as a rev. If not, then we stop at the first\n+\t * non-rev and assume everything else is a path.\n+\t */\n \tfor (i = 0; i < argc; i++) {\n \t\tconst char *arg = argv[i];\n \t\tunsigned char sha1[20];\n@@ -1158,13 +1173,14 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n \n \t\tif (!strcmp(arg, \"--\")) {\n \t\t\ti++;\n-\t\t\tseen_dashdash = 1;\n \t\t\tbreak;\n \t\t}\n \n-\t\t/* Stop at the first non-rev */\n-\t\tif (get_sha1_with_context(arg, 0, sha1, &oc))\n+\t\tif (get_sha1_with_context(arg, 0, sha1, &oc)) {\n+\t\t\tif (seen_dashdash)\n+\t\t\t\tdie(_(\"unable to resolve revision: %s\"), arg);\n \t\t\tbreak;\n+\t\t}\n \n \t\tobject = parse_object_or_die(sha1, arg);\n \t\tif (!seen_dashdash)\n@@ -1172,7 +1188,10 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n \t\tadd_object_array_with_path(object, arg, &list, oc.mode, oc.path);\n \t}\n \n-\t/* The rest are paths */\n+\t/*\n+\t * Anything left over is presumed to be a path. But in the non-dashdash\n+\t * \"do what I mean\" case, we verify and complain when that isn't true.\n+\t */\n \tif (!seen_dashdash) {\n \t\tint j;\n \t\tfor (j = i; j < argc; j++)\ndiff --git a/t/t7810-grep.sh b/t/t7810-grep.sh\nindex 2c1f7373e..a6011f9b1 100755\n--- a/t/t7810-grep.sh\n+++ b/t/t7810-grep.sh\n@@ -982,6 +982,39 @@ test_expect_success 'grep -e -- -- path' '\n \ttest_cmp expected actual\n '\n \n+test_expect_success 'dashdash disambiguates rev as rev' '\n+\ttest_when_finished \"rm -f master\" &&\n+\techo content >master &&\n+\techo master:hello.c >expect &&\n+\tgit grep -l o master -- hello.c >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'dashdash disambiguates pathspec as pathspec' '\n+\ttest_when_finished \"git rm -f master\" &&\n+\techo content >master &&\n+\tgit add master &&\n+\techo master:content >expect &&\n+\tgit grep o -- master >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'report bogus arg without dashdash' '\n+\ttest_must_fail git grep o does-not-exist\n+'\n+\n+test_expect_success 'report bogus rev with dashdash' '\n+\ttest_must_fail git grep o hello.c --\n+'\n+\n+test_expect_success 'allow non-existent path with dashdash' '\n+\t# We need a real match so grep exits with success.\n+\ttree=$(git ls-tree HEAD |\n+\t sed s/hello.c/not-in-working-tree/ |\n+\t git mktree) &&\n+\tgit grep o \"$tree\" -- not-in-working-tree\n+'\n+\n test_expect_success 'grep --no-index pattern -- path' '\n \trm -fr non &&\n \tmkdir -p non/git &&\n-- \n2.12.0.rc1.471.ga79ec8999\n\n"},{"id":"311517","messageId":"20170214060729.v4r24y5tuaov3jrh@sigill.intra.peff.net","threadId":"45135","inReplyTo":"20170214060021.einv7372exbxa23z@sigill.intra.peff.net","subject":"[PATCH 6/7] grep: avoid resolving revision names in --no-index case","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-02-14T06:07:29Z","receivedAt":"2017-02-14T06:07:35Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We disallow the use of revisions with --no-index, but we\ndon't actually check and complain until well after we've\nparsed the revisions.\n\nThis is the cause of a few problems:\n\n 1. We shouldn't be calling get_sha1() at all when we aren't\n in a repository, as it might access the ref or object\n databases. For now, this should generally just return\n failure, but eventually it will become a BUG().\n\n 2. When there's a \"--\" disambiguator and you're outside a\n repository, we'll complain early with \"unable to resolve\n revision\". But we can give a much more specific error.\n\n 3. When there isn't a \"--\" disambiguator, we still do the\n normal rev/path checks. This is silly, as we know we\n cannot have any revs with --no-index. Everything we see\n must be a path.\n\n Outside of a repository this doesn't matter (since we\n know it won't resolve), but inside one, we may complain\n unnecessarily if a filename happens to also match a\n refname.\n\nThis patch skips the get_sha1() call entirely in the\nno-index case, and behaves as if it failed (with the\nexception of giving a better error message).\n\nSigned-off-by: Jeff King \n---\n builtin/grep.c | 6 ++++++\n t/t7810-grep.sh | 13 +++++++++++++\n 2 files changed, 19 insertions(+)\n\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex e83b33bda..c4c632594 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -1176,6 +1176,12 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n \t\t\tbreak;\n \t\t}\n \n+\t\tif (!use_index) {\n+\t\t\tif (seen_dashdash)\n+\t\t\t\tdie(_(\"--no-index cannot be used with revs\"));\n+\t\t\tbreak;\n+\t\t}\n+\n \t\tif (get_sha1_with_context(arg, 0, sha1, &oc)) {\n \t\t\tif (seen_dashdash)\n \t\t\t\tdie(_(\"unable to resolve revision: %s\"), arg);\ndiff --git a/t/t7810-grep.sh b/t/t7810-grep.sh\nindex a6011f9b1..c051c7ee8 100755\n--- a/t/t7810-grep.sh\n+++ b/t/t7810-grep.sh\n@@ -1030,6 +1030,19 @@ test_expect_success 'grep --no-index pattern -- path' '\n \t)\n '\n \n+test_expect_success 'grep --no-index complains of revs' '\n+\ttest_must_fail git grep --no-index o master -- 2>err &&\n+\ttest_i18ngrep \"no-index cannot be used with revs\" err\n+'\n+\n+test_expect_success 'grep --no-index prefers paths to revs' '\n+\ttest_when_finished \"rm -f master\" &&\n+\techo content >master &&\n+\techo master:content >expect &&\n+\tgit grep --no-index o master >actual &&\n+\ttest_cmp expect actual\n+'\n+\n cat >expected <\n---\n builtin/grep.c | 2 +-\n t/t7810-grep.sh | 5 +++++\n 2 files changed, 6 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex c4c632594..1454bef49 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -1201,7 +1201,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n \tif (!seen_dashdash) {\n \t\tint j;\n \t\tfor (j = i; j < argc; j++)\n-\t\t\tverify_filename(prefix, argv[j], j == i);\n+\t\t\tverify_filename(prefix, argv[j], j == i && use_index);\n \t}\n \n \tparse_pathspec(&pathspec, 0,\ndiff --git a/t/t7810-grep.sh b/t/t7810-grep.sh\nindex c051c7ee8..0ff9f6cae 100755\n--- a/t/t7810-grep.sh\n+++ b/t/t7810-grep.sh\n@@ -1043,6 +1043,11 @@ test_expect_success 'grep --no-index prefers paths to revs' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'grep --no-index does not \"diagnose\" revs' '\n+\ttest_must_fail git grep --no-index o :1:hello.c 2>err &&\n+\ttest_i18ngrep ! -i \"did you mean\" err\n+'\n+\n cat >expected < On Mon, Feb 13, 2017 at 08:20:37PM -0500, Jeff King wrote:\n> \n> > > If there is a repo and \"foo\" is a rev, the \"--no-index or --untracked\n> > > cannot be used with revs.\" error would occur. If there is a repo and\n> > > \"foo\" is not a rev, this command would proceed as usual. If there is no\n> > > repo, the \"setup_git_env called without repository\" error would occur.\n> > > (This is my understanding from reading the code - I haven't tested it\n> > > out.)\n> > \n> > Yes, it's easy to see that \"git grep --no-index foo bar\" outside of a\n> > repo generates the same BUG. I suspect that \"--no-index\" should just\n> > disable looking up revs entirely, even if we are actually in a\n> > repository directory.\n> \n> I've fixed that, along with a few other bugs and cleanups. The complete\n> series is below. Patch 2 is your (untouched) patch. My suggestions for\n> your test are in patch 3, which can either remain on its own or be\n> squashed in.\n> \n> [1/7]: grep: move thread initialization a little lower\n> [2/7]: grep: do not unnecessarily query repo for \"--\"\n> [3/7]: t7810: make \"--no-index --\" test more robust\n> [4/7]: grep: re-order rev-parsing loop\n> [5/7]: grep: fix \"--\" rev/pathspec disambiguation\n> [6/7]: grep: avoid resolving revision names in --no-index case\n> [7/7]: grep: do not diagnose misspelt revs with --no-index\n> \n> builtin/grep.c | 78 +++++++++++++++++++++++++++++++++++++++------------------\n> t/t7810-grep.sh | 66 ++++++++++++++++++++++++++++++++++++++++++++++++\n> 2 files changed, 119 insertions(+), 25 deletions(-)\n\nJust to clarify: these are all existing bugs, and I think these are\nprobably maint-worthy patches (even the --no-index ones; though we don't\nBUG on the out-of-repo without the patch from 'next', the code is still\ndoing the wrong thing in subtle ways).\n\nBut AFAIK they are all much older bugs than the upcoming v2.12, so there\nis no pressing need to fit them into the upcoming release.\n\n-Peff\n"},{"id":"311544","messageId":"eef97cc4-d616-b298-bc99-b2772b757190@google.com","threadId":"45135","inReplyTo":"20170214060729.v4r24y5tuaov3jrh@sigill.intra.peff.net","subject":"Re: [PATCH 6/7] grep: avoid resolving revision names in --no-index case","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2017-02-14T16:53:04Z","receivedAt":"2017-02-14T16:53:11Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"On 02/13/2017 10:07 PM, Jeff King wrote:\n> diff --git a/builtin/grep.c b/builtin/grep.c\n> index e83b33bda..c4c632594 100644\n> --- a/builtin/grep.c\n> +++ b/builtin/grep.c\n> @@ -1176,6 +1176,12 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n> \t\t\tbreak;\n> \t\t}\n>\n> +\t\tif (!use_index) {\n> +\t\t\tif (seen_dashdash)\n> +\t\t\t\tdie(_(\"--no-index cannot be used with revs\"));\n\nThere is a subsequent check that prints \"--no-index or --untracked \ncannot be used with revs.\" - maybe we should just expand this part to \nincorporate that case. (That is, write `if (!use_index || untracked)` \ninstead of `if (!use_index)`.) This also allows us to preserve the error \nmessage, which might be useful for someone using a translated version of \nGit.\n\n> +\t\t\tbreak;\n> +\t\t}\n> +\n> \t\tif (get_sha1_with_context(arg, 0, sha1, &oc)) {\n> \t\t\tif (seen_dashdash)\n> \t\t\t\tdie(_(\"unable to resolve revision: %s\"), arg);\n"},{"id":"311545","messageId":"77693036-756d-3be3-c885-ca7891da533c@google.com","threadId":"45135","inReplyTo":"20170214060021.einv7372exbxa23z@sigill.intra.peff.net","subject":"Re: [PATCH 0/7] grep rev/path parsing fixes","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2017-02-14T16:58:49Z","receivedAt":"2017-02-14T16:58:57Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"On 02/13/2017 10:00 PM, Jeff King wrote:\n> I've fixed that, along with a few other bugs and cleanups. The complete\n> series is below. Patch 2 is your (untouched) patch. My suggestions for\n> your test are in patch 3, which can either remain on its own or be\n> squashed in.\n>\n> [1/7]: grep: move thread initialization a little lower\n> [2/7]: grep: do not unnecessarily query repo for \"--\"\n> [3/7]: t7810: make \"--no-index --\" test more robust\n> [4/7]: grep: re-order rev-parsing loop\n> [5/7]: grep: fix \"--\" rev/pathspec disambiguation\n> [6/7]: grep: avoid resolving revision names in --no-index case\n> [7/7]: grep: do not diagnose misspelt revs with --no-index\n\nThanks - these look good to me. I replied to 6/7 with a comment, but I \nalso think that these are good as-is. Also, 3/7 can probably be squashed in.\n"},{"id":"311551","messageId":"20170214180453.tpze2hdv3eytxfju@sigill.intra.peff.net","threadId":"45135","inReplyTo":"eef97cc4-d616-b298-bc99-b2772b757190@google.com","subject":"Re: [PATCH 6/7] grep: avoid resolving revision names in --no-index case","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-02-14T18:04:53Z","receivedAt":"2017-02-14T18:05:05Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Feb 14, 2017 at 08:53:04AM -0800, Jonathan Tan wrote:\n\n> On 02/13/2017 10:07 PM, Jeff King wrote:\n> > diff --git a/builtin/grep.c b/builtin/grep.c\n> > index e83b33bda..c4c632594 100644\n> > --- a/builtin/grep.c\n> > +++ b/builtin/grep.c\n> > @@ -1176,6 +1176,12 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n> > \t\t\tbreak;\n> > \t\t}\n> > \n> > +\t\tif (!use_index) {\n> > +\t\t\tif (seen_dashdash)\n> > +\t\t\t\tdie(_(\"--no-index cannot be used with revs\"));\n> \n> There is a subsequent check that prints \"--no-index or --untracked cannot be\n> used with revs.\" - maybe we should just expand this part to incorporate that\n> case. (That is, write `if (!use_index || untracked)` instead of `if\n> (!use_index)`.) This also allows us to preserve the error message, which\n> might be useful for someone using a translated version of Git.\n\nI wasn't sure if we wanted to treat \"untracked\" in the same way.\nCertainly we can catch the error here for the seen_dashdash case, but is\nit also the case that:\n\n echo content >master\n git grep --untracked pattern master\n\nshould treat \"master\" as a path?\n\n-Peff\n"},{"id":"311554","messageId":"82212eaa-76d2-3357-8e06-5e4d56028c2e@google.com","threadId":"45135","inReplyTo":"20170214180453.tpze2hdv3eytxfju@sigill.intra.peff.net","subject":"Re: [PATCH 6/7] grep: avoid resolving revision names in --no-index case","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2017-02-14T18:19:56Z","receivedAt":"2017-02-14T18:20:03Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"On 02/14/2017 10:04 AM, Jeff King wrote:\n> On Tue, Feb 14, 2017 at 08:53:04AM -0800, Jonathan Tan wrote:\n>\n>> On 02/13/2017 10:07 PM, Jeff King wrote:\n>>> diff --git a/builtin/grep.c b/builtin/grep.c\n>>> index e83b33bda..c4c632594 100644\n>>> --- a/builtin/grep.c\n>>> +++ b/builtin/grep.c\n>>> @@ -1176,6 +1176,12 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n>>> \t\t\tbreak;\n>>> \t\t}\n>>>\n>>> +\t\tif (!use_index) {\n>>> +\t\t\tif (seen_dashdash)\n>>> +\t\t\t\tdie(_(\"--no-index cannot be used with revs\"));\n>>\n>> There is a subsequent check that prints \"--no-index or --untracked cannot be\n>> used with revs.\" - maybe we should just expand this part to incorporate that\n>> case. (That is, write `if (!use_index || untracked)` instead of `if\n>> (!use_index)`.) This also allows us to preserve the error message, which\n>> might be useful for someone using a translated version of Git.\n>\n> I wasn't sure if we wanted to treat \"untracked\" in the same way.\n> Certainly we can catch the error here for the seen_dashdash case, but is\n> it also the case that:\n>\n> echo content >master\n> git grep --untracked pattern master\n>\n> should treat \"master\" as a path?\n>\n> -Peff\n\nIt is already the case that it cannot be treated as a rev:\n\n $ git grep --untracked pattern master\n fatal: --no-index or --untracked cannot be used with revs.\n\nSo I think it would be better if it was treated as a path, for \nconsistency with --no-index. I'm OK either way, though.\n"},{"id":"311558","messageId":"20170214184606.GA44208@google.com","threadId":"45135","inReplyTo":"20170214060237.gvzzbmjzerlqnzjv@sigill.intra.peff.net","subject":"Re: [PATCH 1/7] grep: move thread initialization a little lower","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-02-14T18:46:06Z","receivedAt":"2017-02-14T18:46:13Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 02/14, Jeff King wrote:\n> Originally, we set up the threads for grep before parsing\n> the non-option arguments. In 53b8d931b (grep: disable\n> threading in non-worktree case, 2011-12-12), the thread code\n> got bumped lower in the function because it now needed to\n> know whether we got any revision arguments.\n> \n> That put a big block of code in between the parsing of revs\n> and the parsing of pathspecs, both of which share some loop\n> variables. That makes it harder to read the code than the\n> original, where the shared loops were right next to each\n> other.\n> \n> Let's bump the thread initialization until after all of the\n> parsing is done.\n> \n> Signed-off-by: Jeff King \n> ---\n> I double-checked to make sure no other code was relying on\n> the thread setup having happened. I think we could actually\n> bump it quite a bit lower (to right before we actually start\n> grepping), but I doubt it matters much in practice.\n\nLooks good. And yes I don't believe anything needs the thread\ninitialization to happen earlier.\n\n-- \nBrandon Williams\n"},{"id":"311559","messageId":"20170214184836.GB44208@google.com","threadId":"45135","inReplyTo":"20170214060417.yq27zdcgmrckjmua@sigill.intra.peff.net","subject":"Re: [PATCH 4/7] grep: re-order rev-parsing loop","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-02-14T18:48:36Z","receivedAt":"2017-02-14T18:48:43Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 02/14, Jeff King wrote:\n> -\t\t/* Is it a rev? */\n> -\t\tif (!get_sha1_with_context(arg, 0, sha1, &oc)) {\n> -\t\t\tstruct object *object = parse_object_or_die(sha1, arg);\n> -\t\t\tif (!seen_dashdash)\n> -\t\t\t\tverify_non_filename(prefix, arg);\n> -\t\t\tadd_object_array_with_path(object, arg, &list, oc.mode, oc.path);\n> -\t\t\tcontinue;\n> -\t\t}\n> -\t\tbreak;\n> +\n> +\t\t/* Stop at the first non-rev */\n> +\t\tif (get_sha1_with_context(arg, 0, sha1, &oc))\n> +\t\t\tbreak;\n> +\n> +\t\tobject = parse_object_or_die(sha1, arg);\n> +\t\tif (!seen_dashdash)\n> +\t\t\tverify_non_filename(prefix, arg);\n> +\t\tadd_object_array_with_path(object, arg, &list, oc.mode, oc.path);\n\nThis is much more readable!\n\n-- \nBrandon Williams\n"},{"id":"311562","messageId":"20170214185621.GC44208@google.com","threadId":"45135","inReplyTo":"20170214060555.yzh6hhi2t7pkeqvi@sigill.intra.peff.net","subject":"Re: [PATCH 5/7] grep: fix \"--\" rev/pathspec disambiguation","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-02-14T18:56:21Z","receivedAt":"2017-02-14T18:56:29Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 02/14, Jeff King wrote:\n> -\t/* Check revs and then paths */\n> +\t/*\n> +\t * We have to find \"--\" in a separate pass, because its presence\n> +\t * influences how we will parse arguments that come before it.\n> +\t */\n> +\tfor (i = 0; i < argc; i++) {\n> +\t\tif (!strcmp(argv[i], \"--\")) {\n> +\t\t\tseen_dashdash = 1;\n> +\t\t\tbreak;\n> +\t\t}\n> +\t}\n\nSo this simply checks if \"--\" is an argument that was provided. This\nthen allows grep to know ahead of time how to handle revs/paths\npreceding a \"--\" or in the absences of the \"--\". Seems sensible to me.\n\n> +\n> +\t/*\n> +\t * Resolve any rev arguments. If we have a dashdash, then everything up\n> +\t * to it must resolve as a rev. If not, then we stop at the first\n> +\t * non-rev and assume everything else is a path.\n> +\t */\n> \tfor (i = 0; i < argc; i++) {\n> \t\tconst char *arg = argv[i];\n> \t\tunsigned char sha1[20];\n> @@ -1158,13 +1173,14 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n> \n> \t\tif (!strcmp(arg, \"--\")) {\n> \t\t\ti++;\n> -\t\t\tseen_dashdash = 1;\n> \t\t\tbreak;\n> \t\t}\n> \n> -\t\t/* Stop at the first non-rev */\n> -\t\tif (get_sha1_with_context(arg, 0, sha1, &oc))\n> +\t\tif (get_sha1_with_context(arg, 0, sha1, &oc)) {\n> +\t\t\tif (seen_dashdash)\n> +\t\t\t\tdie(_(\"unable to resolve revision: %s\"), arg);\n> \t\t\tbreak;\n> +\t\t}\n> \n> \t\tobject = parse_object_or_die(sha1, arg);\n> \t\tif (!seen_dashdash)\n> @@ -1172,7 +1188,10 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n> \t\tadd_object_array_with_path(object, arg, &list, oc.mode, oc.path);\n> \t}\n> \n> -\t/* The rest are paths */\n> +\t/*\n> +\t * Anything left over is presumed to be a path. But in the non-dashdash\n> +\t * \"do what I mean\" case, we verify and complain when that isn't true.\n> +\t */\n\n-- \nBrandon Williams\n"},{"id":"311565","messageId":"xmqqpoik8ttf.fsf@gitster.mtv.corp.google.com","threadId":"45135","inReplyTo":"20170214060555.yzh6hhi2t7pkeqvi@sigill.intra.peff.net","subject":"Re: [PATCH 5/7] grep: fix \"--\" rev/pathspec disambiguation","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-02-14T19:18:36Z","receivedAt":"2017-02-14T19:18:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> If we see \"git grep pattern rev -- file\" then we apply the\n> usual rev/pathspec disambiguation rules: any \"rev\" before\n> the \"--\" must be a revision, and we do not need to apply the\n> verify_non_filename() check.\n>\n> But there are two bugs here:\n>\n> 1. We keep a seen_dashdash flag to handle this case, but\n> we set it in the same left-to-right pass over the\n> arguments in which we parse \"rev\".\n>\n> So when we see \"rev\", we do not yet know that there is\n> a \"--\", and we mistakenly complain if there is a\n> matching file.\n>\n> We can fix this by making a preliminary pass over the\n> arguments to find the \"--\", and only then checking the rev\n> arguments.\n>\n> 2. If we can't resolve \"rev\" but there isn't a dashdash,\n> that's OK. We treat it like a path, and complain later\n> if it doesn't exist.\n>\n> But if there _is_ a dashdash, then we know it must be a\n> rev, and should treat it as such, complaining if it\n> does not resolve. The current code instead ignores it\n> and tries to treat it like a path.\n>\n> This patch fixes both bugs, and tries to comment the parsing\n> flow a bit better.\n\nGood. Thanks.\n"},{"id":"311567","messageId":"20170214195127.q6gs6ayp6w7uxi72@sigill.intra.peff.net","threadId":"45135","inReplyTo":"20170214185621.GC44208@google.com","subject":"Re: [PATCH 5/7] grep: fix \"--\" rev/pathspec disambiguation","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-02-14T19:51:27Z","receivedAt":"2017-02-14T19:51:39Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Feb 14, 2017 at 10:56:21AM -0800, Brandon Williams wrote:\n\n> On 02/14, Jeff King wrote:\n> > -\t/* Check revs and then paths */\n> > +\t/*\n> > +\t * We have to find \"--\" in a separate pass, because its presence\n> > +\t * influences how we will parse arguments that come before it.\n> > +\t */\n> > +\tfor (i = 0; i < argc; i++) {\n> > +\t\tif (!strcmp(argv[i], \"--\")) {\n> > +\t\t\tseen_dashdash = 1;\n> > +\t\t\tbreak;\n> > +\t\t}\n> > +\t}\n> \n> So this simply checks if \"--\" is an argument that was provided. This\n> then allows grep to know ahead of time how to handle revs/paths\n> preceding a \"--\" or in the absences of the \"--\". Seems sensible to me.\n\nBy the way, we have to check again later for \"--\" when parsing the revs\nthemselves. In theory you could set seen_dashdash to the offset of the\ndashdash in the array, and do the iteration more like:\n\n for (i = 0; i < dashdash_pos; i++)\n handle_rev(argv[i]);\n for (i = dashdash_pos + 1; i < argc; i++)\n handle_path(argv[i]);\n\nBut our loops also handle the case where there is no \"--\" at all, and I\nthink that approach ends up convoluting the logic. I didn't go very far\nin that direction before giving it up, though.\n\n-Peff\n"},{"id":"311589","messageId":"20170214215436.kqca4c7gv2kwevw7@sigill.intra.peff.net","threadId":"45135","inReplyTo":"82212eaa-76d2-3357-8e06-5e4d56028c2e@google.com","subject":"[PATCH 8/7] grep: treat revs the same for --untracked as for --no-index","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-02-14T21:54:36Z","receivedAt":"2017-02-14T21:54:49Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Feb 14, 2017 at 10:19:56AM -0800, Jonathan Tan wrote:\n\n> > I wasn't sure if we wanted to treat \"untracked\" in the same way.\n> > Certainly we can catch the error here for the seen_dashdash case, but is\n> > it also the case that:\n> > \n> > echo content >master\n> > git grep --untracked pattern master\n> > \n> > should treat \"master\" as a path?\n> \n> It is already the case that it cannot be treated as a rev:\n> \n> $ git grep --untracked pattern master\n> fatal: --no-index or --untracked cannot be used with revs.\n> \n> So I think it would be better if it was treated as a path, for consistency\n> with --no-index. I'm OK either way, though.\n\nRight, it's always been disallowed. But the early detection changes a\nfew user-visible behaviors like the exact error message, and how\ndisambiguation works (see below). I think the arguments for making that\nchange for --no-index are stronger than for --untracked. But it probably\nmakes sense for --untracked, too.\n\nHere's a patch on top of my series that lumps the two together again. It\n_could_ be squashed into the earlier patch, but I think I prefer keeping\nit separate.\n\n-- >8 --\nSubject: [PATCH] grep: treat revs the same for --untracked as for --no-index\n\ngit-grep has always disallowed grepping in a tree (as\nopposed to the working directory) with both --untracked\nand --no-index. But we traditionally did so by first\ncollecting the revs, and then complaining when any were\nprovided.\n\nThe --no-index option recently learned to detect revs\nmuch earlier. This has two user-visible effects:\n\n - we don't bother to resolve revision names at all. So\n when there's a rev/path ambiguity, we always choose to\n treat it as a path.\n\n - likewise, when you do specify a revision without \"--\",\n the error you get is \"no such path\" and not \"--untracked\n cannot be used with revs\".\n\nThe rationale for doing this with --no-index is that it is\nmeant to be used outside a repository, and so parsing revs\nat all does not make sense.\n\nThis patch gives --untracked the same treatment. While it\n_is_ meant to be used in a repository, it is explicitly\nabout grepping the non-repository contents. Telling the user\n\"we found a rev, but you are not allowed to use revs\" is\nnot really helpful compared to \"we treated your argument as\na path, and could not find it\".\n\nSigned-off-by: Jeff King \n---\n builtin/grep.c | 10 +++++-----\n t/t7810-grep.sh | 2 +-\n 2 files changed, 6 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex 1454bef49..9304c33e7 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -967,6 +967,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n \tint dummy;\n \tint use_index = 1;\n \tint pattern_type_arg = GREP_PATTERN_TYPE_UNSPECIFIED;\n+\tint allow_revs;\n \n \tstruct option options[] = {\n \t\tOPT_BOOL(0, \"cached\", &cached,\n@@ -1165,6 +1166,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n \t * to it must resolve as a rev. If not, then we stop at the first\n \t * non-rev and assume everything else is a path.\n \t */\n+\tallow_revs = use_index && !untracked;\n \tfor (i = 0; i < argc; i++) {\n \t\tconst char *arg = argv[i];\n \t\tunsigned char sha1[20];\n@@ -1176,9 +1178,9 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n \t\t\tbreak;\n \t\t}\n \n-\t\tif (!use_index) {\n+\t\tif (!allow_revs) {\n \t\t\tif (seen_dashdash)\n-\t\t\t\tdie(_(\"--no-index cannot be used with revs\"));\n+\t\t\t\tdie(_(\"--no-index or --untracked cannot be used with revs\"));\n \t\t\tbreak;\n \t\t}\n \n@@ -1201,7 +1203,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n \tif (!seen_dashdash) {\n \t\tint j;\n \t\tfor (j = i; j < argc; j++)\n-\t\t\tverify_filename(prefix, argv[j], j == i && use_index);\n+\t\t\tverify_filename(prefix, argv[j], j == i && allow_revs);\n \t}\n \n \tparse_pathspec(&pathspec, 0,\n@@ -1273,8 +1275,6 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n \n \tif (!use_index || untracked) {\n \t\tint use_exclude = (opt_exclude < 0) ? use_index : !!opt_exclude;\n-\t\tif (list.nr)\n-\t\t\tdie(_(\"--no-index or --untracked cannot be used with revs.\"));\n \t\thit = grep_directory(&opt, &pathspec, use_exclude, use_index);\n \t} else if (0 <= opt_exclude) {\n \t\tdie(_(\"--[no-]exclude-standard cannot be used for tracked contents.\"));\ndiff --git a/t/t7810-grep.sh b/t/t7810-grep.sh\nindex 0ff9f6cae..cee42097b 100755\n--- a/t/t7810-grep.sh\n+++ b/t/t7810-grep.sh\n@@ -1032,7 +1032,7 @@ test_expect_success 'grep --no-index pattern -- path' '\n \n test_expect_success 'grep --no-index complains of revs' '\n \ttest_must_fail git grep --no-index o master -- 2>err &&\n-\ttest_i18ngrep \"no-index cannot be used with revs\" err\n+\ttest_i18ngrep \"cannot be used with revs\" err\n '\n \n test_expect_success 'grep --no-index prefers paths to revs' '\n-- \n2.12.0.rc1.479.g59880b11e\n\n"},{"id":"311591","messageId":"xmqqziho77to.fsf@gitster.mtv.corp.google.com","threadId":"45135","inReplyTo":"20170214215436.kqca4c7gv2kwevw7@sigill.intra.peff.net","subject":"Re: [PATCH 8/7] grep: treat revs the same for --untracked as for --no-index","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-02-14T21:58:59Z","receivedAt":"2017-02-14T21:59:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> ...\n> The rationale for doing this with --no-index is that it is\n> meant to be used outside a repository, and so parsing revs\n> at all does not make sense.\n>\n> This patch gives --untracked the same treatment. While it\n> _is_ meant to be used in a repository, it is explicitly\n> about grepping the non-repository contents. Telling the user\n> \"we found a rev, but you are not allowed to use revs\" is\n> not really helpful compared to \"we treated your argument as\n> a path, and could not find it\".\n\nYup, both sounds very sensible. Thanks.\n"}]}