[PATCH for NEXT] grep: do not unnecessarily query repo for "--"

[PATCH for NEXT] grep: do not unnecessarily query repo for "--"

[Jonathan Tan]

When running a command of the form

  git grep --no-index pattern -- path

in the absence of a Git repository, an error message will be printed:

  fatal: BUG: setup_git_env called without repository

This is because "git grep" tries to interpret "--" as a rev. "git grep"
has always tried to first interpret "--" as a rev for at least a few
years, but this issue was upgraded from a pessimization to a bug in
commit 59332d1 ("Resurrect "git grep --no-index"", 2010-02-06), which
calls get_sha1 regardless of whether --no-index was specified. This bug
appeared to be benign until commit b1ef400 ("setup_git_env: avoid blind
fall-back to ".git"", 2016-10-20) when Git was taught to die in this
situation.  (This "git grep" bug appears to be one of the bugs that
commit b1ef400 is meant to flush out.)

Therefore, always interpret "--" as signaling the end of options,
instead of trying to interpret it as a rev first.

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---

There is probably a similar bug for commands of the form:

  git grep --no-index pattern foo

If there is a repo and "foo" is a rev, the "--no-index or --untracked
cannot be used with revs." error would occur. If there is a repo and
"foo" is not a rev, this command would proceed as usual. If there is no
repo, the "setup_git_env called without repository" error would occur.
(This is my understanding from reading the code - I haven't tested it
out.)

This patch does not fix this similar bug, but I decided to send it out
anyway because it still fixes a bug and unlocks the ability to
specify paths with "git grep --no-index".

 builtin/grep.c  |  9 +++++----
 t/t7810-grep.sh | 12 ++++++++++++
 2 files changed, 17 insertions(+), 4 deletions(-)

diff --git a/builtin/grep.c b/builtin/grep.c
index 2c727ef49..1b68d1638 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -1154,6 +1154,11 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
 		const char *arg = argv[i];
 		unsigned char sha1[20];
 		struct object_context oc;
+		if (!strcmp(arg, "--")) {
+			i++;
+			seen_dashdash = 1;
+			break;
+		}
 		/* Is it a rev? */
 		if (!get_sha1_with_context(arg, 0, sha1, &oc)) {
 			struct object *object = parse_object_or_die(sha1, arg);
@@ -1162,10 +1167,6 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
 			add_object_array_with_path(object, arg, &list, oc.mode, oc.path);
 			continue;
 		}
-		if (!strcmp(arg, "--")) {
-			i++;
-			seen_dashdash = 1;
-		}
 		break;
 	}
 
diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh
index 19f0108f8..29202f0e7 100755
--- a/t/t7810-grep.sh
+++ b/t/t7810-grep.sh
@@ -982,6 +982,18 @@ test_expect_success 'grep -e -- -- path' '
 	test_cmp expected actual
 '
 
+test_expect_success 'grep --no-index pattern -- path' '
+	rm -fr non &&
+	mkdir -p non/git &&
+	(
+		GIT_CEILING_DIRECTORIES="$(pwd)/non" &&
+		export GIT_CEILING_DIRECTORIES &&
+		cd non/git &&
+		echo hello >hello &&
+		git grep --no-index o -- .
+	)
+'
+
 cat >expected <<EOF
 hello.c:int main(int argc, const char **argv)
 hello.c:	printf("Hello world.\n");
-- 
2.11.0.483.g087da7b7c-goog

Re: [PATCH for NEXT] grep: do not unnecessarily query repo for "--"

[Jeff King]

On Mon, Feb 13, 2017 at 04:11:59PM -0800, Jonathan Tan wrote:

> When running a command of the form
> 
>   git grep --no-index pattern -- path
> 
> in the absence of a Git repository, an error message will be printed:
> 
>   fatal: BUG: setup_git_env called without repository
> 
> This is because "git grep" tries to interpret "--" as a rev. "git grep"
> has always tried to first interpret "--" as a rev for at least a few
> years, but this issue was upgraded from a pessimization to a bug in
> commit 59332d1 ("Resurrect "git grep --no-index"", 2010-02-06), which
> calls get_sha1 regardless of whether --no-index was specified. This bug
> appeared to be benign until commit b1ef400 ("setup_git_env: avoid blind
> fall-back to ".git"", 2016-10-20) when Git was taught to die in this
> situation.  (This "git grep" bug appears to be one of the bugs that
> commit b1ef400 is meant to flush out.)
> 
> Therefore, always interpret "--" as signaling the end of options,
> instead of trying to interpret it as a rev first.

Nicely explained.

> There is probably a similar bug for commands of the form:
> 
>   git grep --no-index pattern foo
> 
> If there is a repo and "foo" is a rev, the "--no-index or --untracked
> cannot be used with revs." error would occur. If there is a repo and
> "foo" is not a rev, this command would proceed as usual. If there is no
> repo, the "setup_git_env called without repository" error would occur.
> (This is my understanding from reading the code - I haven't tested it
> out.)

Yes, it's easy to see that "git grep --no-index foo bar" outside of a
repo generates the same BUG. I suspect that "--no-index" should just
disable looking up revs entirely, even if we are actually in a
repository directory.

> This patch does not fix this similar bug, but I decided to send it out
> anyway because it still fixes a bug and unlocks the ability to
> specify paths with "git grep --no-index".

Yes, I think even if we fix the other bug, fixing this "--" thing is an
improvement.

> diff --git a/builtin/grep.c b/builtin/grep.c
> index 2c727ef49..1b68d1638 100644
> --- a/builtin/grep.c
> +++ b/builtin/grep.c
> @@ -1154,6 +1154,11 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
>  		const char *arg = argv[i];
>  		unsigned char sha1[20];
>  		struct object_context oc;
> +		if (!strcmp(arg, "--")) {
> +			i++;
> +			seen_dashdash = 1;
> +			break;
> +		}
>  		/* Is it a rev? */
>  		if (!get_sha1_with_context(arg, 0, sha1, &oc)) {
>  			struct object *object = parse_object_or_die(sha1, arg);

So I think this is a definite improvement, but I see a few leftover
oddities:

  - the end logic for this loop is now:

      if (arg is a rev) {
         ... handle rev ...
         continue;
      }
      break;

    It would probably be more obvious as:

      if (arg is not a rev)
          break;
      ... handle rev ...

  - the rev-handling code does:

      if (!seen_dashdash)
          verify_non_filename(prefix, arg);

    But I do not see how seen_dashdash could ever be untrue. We set it
    inside this loop, and break immediately when we see it. And indeed,
    running:

      echo content >master
      git grep content master --

    does not work. The "--" should tell us that "master" is a rev, but
    we don't know yet that we have a dashdash.

    I think we need a separate loop to find the "--" first, and _then_
    walk through the arguments, treating them as revs or paths as
    appropriate. This is how setup_revisions() does it.

    So this isn't a problem introduced by your patch, but it's
    intimately related.

> diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh
> index 19f0108f8..29202f0e7 100755
> --- a/t/t7810-grep.sh
> +++ b/t/t7810-grep.sh
> @@ -982,6 +982,18 @@ test_expect_success 'grep -e -- -- path' '
>  	test_cmp expected actual
>  '
>  
> +test_expect_success 'grep --no-index pattern -- path' '
> +	rm -fr non &&
> +	mkdir -p non/git &&
> +	(
> +		GIT_CEILING_DIRECTORIES="$(pwd)/non" &&
> +		export GIT_CEILING_DIRECTORIES &&
> +		cd non/git &&
> +		echo hello >hello &&
> +		git grep --no-index o -- .
> +	)
> +'

Since de95302a4, you can do:

  nongit git grep --no-index -o -- .

Though if this is destined for maint, it might need to be done
separately this way and cleaned up later.

It might also be a good idea to confirm that the pathspec is actually
being respected in the --no-index case. Something like:

  echo hello >hello &&
  echo goodbye >goodbye &&
  echo hello:hello >expect &&
  git grep --no-index o -- hello >actual &&
  test_cmp expect actual

-Peff

[PATCH 0/7] grep rev/path parsing fixes

[Jeff King]

On Mon, Feb 13, 2017 at 08:20:37PM -0500, Jeff King wrote:

> > If there is a repo and "foo" is a rev, the "--no-index or --untracked
> > cannot be used with revs." error would occur. If there is a repo and
> > "foo" is not a rev, this command would proceed as usual. If there is no
> > repo, the "setup_git_env called without repository" error would occur.
> > (This is my understanding from reading the code - I haven't tested it
> > out.)
> 
> Yes, it's easy to see that "git grep --no-index foo bar" outside of a
> repo generates the same BUG. I suspect that "--no-index" should just
> disable looking up revs entirely, even if we are actually in a
> repository directory.

I've fixed that, along with a few other bugs and cleanups. The complete
series is below. Patch 2 is your (untouched) patch. My suggestions for
your test are in patch 3, which can either remain on its own or be
squashed in.

  [1/7]: grep: move thread initialization a little lower
  [2/7]: grep: do not unnecessarily query repo for "--"
  [3/7]: t7810: make "--no-index --" test more robust
  [4/7]: grep: re-order rev-parsing loop
  [5/7]: grep: fix "--" rev/pathspec disambiguation
  [6/7]: grep: avoid resolving revision names in --no-index case
  [7/7]: grep: do not diagnose misspelt revs with --no-index

 builtin/grep.c  | 78 +++++++++++++++++++++++++++++++++++++++------------------
 t/t7810-grep.sh | 66 ++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 119 insertions(+), 25 deletions(-)

-Peff

[PATCH 1/7] grep: move thread initialization a little lower

[Jeff King]

Originally, we set up the threads for grep before parsing
the non-option arguments. In 53b8d931b (grep: disable
threading in non-worktree case, 2011-12-12), the thread code
got bumped lower in the function because it now needed to
know whether we got any revision arguments.

That put a big block of code in between the parsing of revs
and the parsing of pathspecs, both of which share some loop
variables. That makes it harder to read the code than the
original, where the shared loops were right next to each
other.

Let's bump the thread initialization until after all of the
parsing is done.

Signed-off-by: Jeff King <peff@peff.net>
---
I double-checked to make sure no other code was relying on
the thread setup having happened. I think we could actually
bump it quite a bit lower (to right before we actually start
grepping), but I doubt it matters much in practice.

 builtin/grep.c | 28 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 14 deletions(-)

diff --git a/builtin/grep.c b/builtin/grep.c
index 2c727ef49..5a282c4d0 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -1169,6 +1169,20 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
 		break;
 	}
 
+	/* The rest are paths */
+	if (!seen_dashdash) {
+		int j;
+		for (j = i; j < argc; j++)
+			verify_filename(prefix, argv[j], j == i);
+	}
+
+	parse_pathspec(&pathspec, 0,
+		       PATHSPEC_PREFER_CWD |
+		       (opt.max_depth != -1 ? PATHSPEC_MAXDEPTH_VALID : 0),
+		       prefix, argv + i);
+	pathspec.max_depth = opt.max_depth;
+	pathspec.recursive = 1;
+
 #ifndef NO_PTHREADS
 	if (list.nr || cached || show_in_pager)
 		num_threads = 0;
@@ -1190,20 +1204,6 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
 	}
 #endif
 
-	/* The rest are paths */
-	if (!seen_dashdash) {
-		int j;
-		for (j = i; j < argc; j++)
-			verify_filename(prefix, argv[j], j == i);
-	}
-
-	parse_pathspec(&pathspec, 0,
-		       PATHSPEC_PREFER_CWD |
-		       (opt.max_depth != -1 ? PATHSPEC_MAXDEPTH_VALID : 0),
-		       prefix, argv + i);
-	pathspec.max_depth = opt.max_depth;
-	pathspec.recursive = 1;
-
 	if (recurse_submodules) {
 		gitmodules_config();
 		compile_submodule_options(&opt, &pathspec, cached, untracked,
-- 
2.12.0.rc1.471.ga79ec8999

[PATCH 2/7] grep: do not unnecessarily query repo for "--"

[Jeff King]

From: Jonathan Tan <jonathantanmy@google.com>

When running a command of the form

  git grep --no-index pattern -- path

in the absence of a Git repository, an error message will be printed:

  fatal: BUG: setup_git_env called without repository

This is because "git grep" tries to interpret "--" as a rev. "git grep"
has always tried to first interpret "--" as a rev for at least a few
years, but this issue was upgraded from a pessimization to a bug in
commit 59332d1 ("Resurrect "git grep --no-index"", 2010-02-06), which
calls get_sha1 regardless of whether --no-index was specified. This bug
appeared to be benign until commit b1ef400 ("setup_git_env: avoid blind
fall-back to ".git"", 2016-10-20) when Git was taught to die in this
situation.  (This "git grep" bug appears to be one of the bugs that
commit b1ef400 is meant to flush out.)

Therefore, always interpret "--" as signaling the end of options,
instead of trying to interpret it as a rev first.

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
Signed-off-by: Jeff King <peff@peff.net>
---
Unchanged from your original.

 builtin/grep.c  |  9 +++++----
 t/t7810-grep.sh | 12 ++++++++++++
 2 files changed, 17 insertions(+), 4 deletions(-)

diff --git a/builtin/grep.c b/builtin/grep.c
index 5a282c4d0..081e1b57a 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -1154,6 +1154,11 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
 		const char *arg = argv[i];
 		unsigned char sha1[20];
 		struct object_context oc;
+		if (!strcmp(arg, "--")) {
+			i++;
+			seen_dashdash = 1;
+			break;
+		}
 		/* Is it a rev? */
 		if (!get_sha1_with_context(arg, 0, sha1, &oc)) {
 			struct object *object = parse_object_or_die(sha1, arg);
@@ -1162,10 +1167,6 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
 			add_object_array_with_path(object, arg, &list, oc.mode, oc.path);
 			continue;
 		}
-		if (!strcmp(arg, "--")) {
-			i++;
-			seen_dashdash = 1;
-		}
 		break;
 	}
 
diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh
index 19f0108f8..29202f0e7 100755
--- a/t/t7810-grep.sh
+++ b/t/t7810-grep.sh
@@ -982,6 +982,18 @@ test_expect_success 'grep -e -- -- path' '
 	test_cmp expected actual
 '
 
+test_expect_success 'grep --no-index pattern -- path' '
+	rm -fr non &&
+	mkdir -p non/git &&
+	(
+		GIT_CEILING_DIRECTORIES="$(pwd)/non" &&
+		export GIT_CEILING_DIRECTORIES &&
+		cd non/git &&
+		echo hello >hello &&
+		git grep --no-index o -- .
+	)
+'
+
 cat >expected <<EOF
 hello.c:int main(int argc, const char **argv)
 hello.c:	printf("Hello world.\n");
-- 
2.12.0.rc1.471.ga79ec8999

[PATCH 3/7] t7810: make "--no-index --" test more robust

[Jeff King]

This makes sure that we actually use the pathspecs after
"--" correctly (as opposed to just seeing if grep errored
out).

Signed-off-by: Jeff King <peff@peff.net>
---
This could be squashed into the previous patch.

I didn't end up using the "nongit" helper, because we actually have to
do some other setup inside the subshell (this is the reason the earlier
tests in this script don't use the helper, either).

 t/t7810-grep.sh | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh
index 29202f0e7..2c1f7373e 100755
--- a/t/t7810-grep.sh
+++ b/t/t7810-grep.sh
@@ -990,7 +990,10 @@ test_expect_success 'grep --no-index pattern -- path' '
 		export GIT_CEILING_DIRECTORIES &&
 		cd non/git &&
 		echo hello >hello &&
-		git grep --no-index o -- .
+		echo goodbye >goodbye &&
+		echo hello:hello >expect &&
+		git grep --no-index o -- hello >actual &&
+		test_cmp expect actual
 	)
 '
 
-- 
2.12.0.rc1.471.ga79ec8999

[PATCH 4/7] grep: re-order rev-parsing loop

[Jeff King]

We loop over the arguments, but every branch of the loop
hits either a "continue" or a "break". Surely we can make
this simpler.

The final conditional is:

  if (arg is a rev) {
	  ... handle rev ...
	  continue;
  }
  break;

We can rewrite this as:

  if (arg is not a rev)
	  break;

  ... handle rev ...

That makes the flow a little bit simpler, and will make
things much easier to follow when we add more logic in
future patches.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/grep.c | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/builtin/grep.c b/builtin/grep.c
index 081e1b57a..461347adb 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -1154,20 +1154,22 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
 		const char *arg = argv[i];
 		unsigned char sha1[20];
 		struct object_context oc;
+		struct object *object;
+
 		if (!strcmp(arg, "--")) {
 			i++;
 			seen_dashdash = 1;
 			break;
 		}
-		/* Is it a rev? */
-		if (!get_sha1_with_context(arg, 0, sha1, &oc)) {
-			struct object *object = parse_object_or_die(sha1, arg);
-			if (!seen_dashdash)
-				verify_non_filename(prefix, arg);
-			add_object_array_with_path(object, arg, &list, oc.mode, oc.path);
-			continue;
-		}
-		break;
+
+		/* Stop at the first non-rev */
+		if (get_sha1_with_context(arg, 0, sha1, &oc))
+			break;
+
+		object = parse_object_or_die(sha1, arg);
+		if (!seen_dashdash)
+			verify_non_filename(prefix, arg);
+		add_object_array_with_path(object, arg, &list, oc.mode, oc.path);
 	}
 
 	/* The rest are paths */
-- 
2.12.0.rc1.471.ga79ec8999

[PATCH 5/7] grep: fix "--" rev/pathspec disambiguation

[Jeff King]

If we see "git grep pattern rev -- file" then we apply the
usual rev/pathspec disambiguation rules: any "rev" before
the "--" must be a revision, and we do not need to apply the
verify_non_filename() check.

But there are two bugs here:

  1. We keep a seen_dashdash flag to handle this case, but
     we set it in the same left-to-right pass over the
     arguments in which we parse "rev".

     So when we see "rev", we do not yet know that there is
     a "--", and we mistakenly complain if there is a
     matching file.

     We can fix this by making a preliminary pass over the
     arguments to find the "--", and only then checking the rev
     arguments.

  2. If we can't resolve "rev" but there isn't a dashdash,
     that's OK. We treat it like a path, and complain later
     if it doesn't exist.

     But if there _is_ a dashdash, then we know it must be a
     rev, and should treat it as such, complaining if it
     does not resolve. The current code instead ignores it
     and tries to treat it like a path.

This patch fixes both bugs, and tries to comment the parsing
flow a bit better.

It adds tests that cover the two bugs, but also some related
situations (which already worked, but this confirms that our
fixes did not break anything).

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/grep.c  | 29 ++++++++++++++++++++++++-----
 t/t7810-grep.sh | 33 +++++++++++++++++++++++++++++++++
 2 files changed, 57 insertions(+), 5 deletions(-)

diff --git a/builtin/grep.c b/builtin/grep.c
index 461347adb..e83b33bda 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -1149,7 +1149,22 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
 
 	compile_grep_patterns(&opt);
 
-	/* Check revs and then paths */
+	/*
+	 * We have to find "--" in a separate pass, because its presence
+	 * influences how we will parse arguments that come before it.
+	 */
+	for (i = 0; i < argc; i++) {
+		if (!strcmp(argv[i], "--")) {
+			seen_dashdash = 1;
+			break;
+		}
+	}
+
+	/*
+	 * Resolve any rev arguments. If we have a dashdash, then everything up
+	 * to it must resolve as a rev. If not, then we stop at the first
+	 * non-rev and assume everything else is a path.
+	 */
 	for (i = 0; i < argc; i++) {
 		const char *arg = argv[i];
 		unsigned char sha1[20];
@@ -1158,13 +1173,14 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
 
 		if (!strcmp(arg, "--")) {
 			i++;
-			seen_dashdash = 1;
 			break;
 		}
 
-		/* Stop at the first non-rev */
-		if (get_sha1_with_context(arg, 0, sha1, &oc))
+		if (get_sha1_with_context(arg, 0, sha1, &oc)) {
+			if (seen_dashdash)
+				die(_("unable to resolve revision: %s"), arg);
 			break;
+		}
 
 		object = parse_object_or_die(sha1, arg);
 		if (!seen_dashdash)
@@ -1172,7 +1188,10 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
 		add_object_array_with_path(object, arg, &list, oc.mode, oc.path);
 	}
 
-	/* The rest are paths */
+	/*
+	 * Anything left over is presumed to be a path. But in the non-dashdash
+	 * "do what I mean" case, we verify and complain when that isn't true.
+	 */
 	if (!seen_dashdash) {
 		int j;
 		for (j = i; j < argc; j++)
diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh
index 2c1f7373e..a6011f9b1 100755
--- a/t/t7810-grep.sh
+++ b/t/t7810-grep.sh
@@ -982,6 +982,39 @@ test_expect_success 'grep -e -- -- path' '
 	test_cmp expected actual
 '
 
+test_expect_success 'dashdash disambiguates rev as rev' '
+	test_when_finished "rm -f master" &&
+	echo content >master &&
+	echo master:hello.c >expect &&
+	git grep -l o master -- hello.c >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'dashdash disambiguates pathspec as pathspec' '
+	test_when_finished "git rm -f master" &&
+	echo content >master &&
+	git add master &&
+	echo master:content >expect &&
+	git grep o -- master >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'report bogus arg without dashdash' '
+	test_must_fail git grep o does-not-exist
+'
+
+test_expect_success 'report bogus rev with dashdash' '
+	test_must_fail git grep o hello.c --
+'
+
+test_expect_success 'allow non-existent path with dashdash' '
+	# We need a real match so grep exits with success.
+	tree=$(git ls-tree HEAD |
+	       sed s/hello.c/not-in-working-tree/ |
+	       git mktree) &&
+	git grep o "$tree" -- not-in-working-tree
+'
+
 test_expect_success 'grep --no-index pattern -- path' '
 	rm -fr non &&
 	mkdir -p non/git &&
-- 
2.12.0.rc1.471.ga79ec8999

[PATCH 6/7] grep: avoid resolving revision names in --no-index case

[Jeff King]

We disallow the use of revisions with --no-index, but we
don't actually check and complain until well after we've
parsed the revisions.

This is the cause of a few problems:

 1. We shouldn't be calling get_sha1() at all when we aren't
    in a repository, as it might access the ref or object
    databases. For now, this should generally just return
    failure, but eventually it will become a BUG().

 2. When there's a "--" disambiguator and you're outside a
    repository, we'll complain early with "unable to resolve
    revision". But we can give a much more specific error.

 3. When there isn't a "--" disambiguator, we still do the
    normal rev/path checks. This is silly, as we know we
    cannot have any revs with --no-index. Everything we see
    must be a path.

    Outside of a repository this doesn't matter (since we
    know it won't resolve), but inside one, we may complain
    unnecessarily if a filename happens to also match a
    refname.

This patch skips the get_sha1() call entirely in the
no-index case, and behaves as if it failed (with the
exception of giving a better error message).

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/grep.c  |  6 ++++++
 t/t7810-grep.sh | 13 +++++++++++++
 2 files changed, 19 insertions(+)

diff --git a/builtin/grep.c b/builtin/grep.c
index e83b33bda..c4c632594 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -1176,6 +1176,12 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
 			break;
 		}
 
+		if (!use_index) {
+			if (seen_dashdash)
+				die(_("--no-index cannot be used with revs"));
+			break;
+		}
+
 		if (get_sha1_with_context(arg, 0, sha1, &oc)) {
 			if (seen_dashdash)
 				die(_("unable to resolve revision: %s"), arg);
diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh
index a6011f9b1..c051c7ee8 100755
--- a/t/t7810-grep.sh
+++ b/t/t7810-grep.sh
@@ -1030,6 +1030,19 @@ test_expect_success 'grep --no-index pattern -- path' '
 	)
 '
 
+test_expect_success 'grep --no-index complains of revs' '
+	test_must_fail git grep --no-index o master -- 2>err &&
+	test_i18ngrep "no-index cannot be used with revs" err
+'
+
+test_expect_success 'grep --no-index prefers paths to revs' '
+	test_when_finished "rm -f master" &&
+	echo content >master &&
+	echo master:content >expect &&
+	git grep --no-index o master >actual &&
+	test_cmp expect actual
+'
+
 cat >expected <<EOF
 hello.c:int main(int argc, const char **argv)
 hello.c:	printf("Hello world.\n");
-- 
2.12.0.rc1.471.ga79ec8999

[PATCH 7/7] grep: do not diagnose misspelt revs with --no-index

[Jeff King]

If we are using --no-index, then our arguments cannot be
revs in the first place. Not only is it pointless to
diagnose them, but if we are not in a repository, we should
not be trying to resolve any names.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/grep.c  | 2 +-
 t/t7810-grep.sh | 5 +++++
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/builtin/grep.c b/builtin/grep.c
index c4c632594..1454bef49 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -1201,7 +1201,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
 	if (!seen_dashdash) {
 		int j;
 		for (j = i; j < argc; j++)
-			verify_filename(prefix, argv[j], j == i);
+			verify_filename(prefix, argv[j], j == i && use_index);
 	}
 
 	parse_pathspec(&pathspec, 0,
diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh
index c051c7ee8..0ff9f6cae 100755
--- a/t/t7810-grep.sh
+++ b/t/t7810-grep.sh
@@ -1043,6 +1043,11 @@ test_expect_success 'grep --no-index prefers paths to revs' '
 	test_cmp expect actual
 '
 
+test_expect_success 'grep --no-index does not "diagnose" revs' '
+	test_must_fail git grep --no-index o :1:hello.c 2>err &&
+	test_i18ngrep ! -i "did you mean" err
+'
+
 cat >expected <<EOF
 hello.c:int main(int argc, const char **argv)
 hello.c:	printf("Hello world.\n");
-- 
2.12.0.rc1.471.ga79ec8999

Re: [PATCH 0/7] grep rev/path parsing fixes

[Jeff King]

On Tue, Feb 14, 2017 at 01:00:21AM -0500, Jeff King wrote:

> On Mon, Feb 13, 2017 at 08:20:37PM -0500, Jeff King wrote:
> 
> > > If there is a repo and "foo" is a rev, the "--no-index or --untracked
> > > cannot be used with revs." error would occur. If there is a repo and
> > > "foo" is not a rev, this command would proceed as usual. If there is no
> > > repo, the "setup_git_env called without repository" error would occur.
> > > (This is my understanding from reading the code - I haven't tested it
> > > out.)
> > 
> > Yes, it's easy to see that "git grep --no-index foo bar" outside of a
> > repo generates the same BUG. I suspect that "--no-index" should just
> > disable looking up revs entirely, even if we are actually in a
> > repository directory.
> 
> I've fixed that, along with a few other bugs and cleanups. The complete
> series is below. Patch 2 is your (untouched) patch. My suggestions for
> your test are in patch 3, which can either remain on its own or be
> squashed in.
> 
>   [1/7]: grep: move thread initialization a little lower
>   [2/7]: grep: do not unnecessarily query repo for "--"
>   [3/7]: t7810: make "--no-index --" test more robust
>   [4/7]: grep: re-order rev-parsing loop
>   [5/7]: grep: fix "--" rev/pathspec disambiguation
>   [6/7]: grep: avoid resolving revision names in --no-index case
>   [7/7]: grep: do not diagnose misspelt revs with --no-index
> 
>  builtin/grep.c  | 78 +++++++++++++++++++++++++++++++++++++++------------------
>  t/t7810-grep.sh | 66 ++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 119 insertions(+), 25 deletions(-)

Just to clarify: these are all existing bugs, and I think these are
probably maint-worthy patches (even the --no-index ones; though we don't
BUG on the out-of-repo without the patch from 'next', the code is still
doing the wrong thing in subtle ways).

But AFAIK they are all much older bugs than the upcoming v2.12, so there
is no pressing need to fit them into the upcoming release.

-Peff

Re: [PATCH 6/7] grep: avoid resolving revision names in --no-index case

[Jonathan Tan]

On 02/13/2017 10:07 PM, Jeff King wrote:
> diff --git a/builtin/grep.c b/builtin/grep.c
> index e83b33bda..c4c632594 100644
> --- a/builtin/grep.c
> +++ b/builtin/grep.c
> @@ -1176,6 +1176,12 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
>  			break;
>  		}
>
> +		if (!use_index) {
> +			if (seen_dashdash)
> +				die(_("--no-index cannot be used with revs"));

There is a subsequent check that prints "--no-index or --untracked 
cannot be used with revs." - maybe we should just expand this part to 
incorporate that case. (That is, write `if (!use_index || untracked)` 
instead of `if (!use_index)`.) This also allows us to preserve the error 
message, which might be useful for someone using a translated version of 
Git.

> +			break;
> +		}
> +
>  		if (get_sha1_with_context(arg, 0, sha1, &oc)) {
>  			if (seen_dashdash)
>  				die(_("unable to resolve revision: %s"), arg);

Re: [PATCH 0/7] grep rev/path parsing fixes

[Jonathan Tan]

On 02/13/2017 10:00 PM, Jeff King wrote:
> I've fixed that, along with a few other bugs and cleanups. The complete
> series is below. Patch 2 is your (untouched) patch. My suggestions for
> your test are in patch 3, which can either remain on its own or be
> squashed in.
>
>   [1/7]: grep: move thread initialization a little lower
>   [2/7]: grep: do not unnecessarily query repo for "--"
>   [3/7]: t7810: make "--no-index --" test more robust
>   [4/7]: grep: re-order rev-parsing loop
>   [5/7]: grep: fix "--" rev/pathspec disambiguation
>   [6/7]: grep: avoid resolving revision names in --no-index case
>   [7/7]: grep: do not diagnose misspelt revs with --no-index

Thanks - these look good to me. I replied to 6/7 with a comment, but I 
also think that these are good as-is. Also, 3/7 can probably be squashed in.

Re: [PATCH 6/7] grep: avoid resolving revision names in --no-index case

[Jeff King]

On Tue, Feb 14, 2017 at 08:53:04AM -0800, Jonathan Tan wrote:

> On 02/13/2017 10:07 PM, Jeff King wrote:
> > diff --git a/builtin/grep.c b/builtin/grep.c
> > index e83b33bda..c4c632594 100644
> > --- a/builtin/grep.c
> > +++ b/builtin/grep.c
> > @@ -1176,6 +1176,12 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
> >  			break;
> >  		}
> > 
> > +		if (!use_index) {
> > +			if (seen_dashdash)
> > +				die(_("--no-index cannot be used with revs"));
> 
> There is a subsequent check that prints "--no-index or --untracked cannot be
> used with revs." - maybe we should just expand this part to incorporate that
> case. (That is, write `if (!use_index || untracked)` instead of `if
> (!use_index)`.) This also allows us to preserve the error message, which
> might be useful for someone using a translated version of Git.

I wasn't sure if we wanted to treat "untracked" in the same way.
Certainly we can catch the error here for the seen_dashdash case, but is
it also the case that:

  echo content >master
  git grep --untracked pattern master

should treat "master" as a path?

-Peff

Re: [PATCH 6/7] grep: avoid resolving revision names in --no-index case

[Jonathan Tan]

On 02/14/2017 10:04 AM, Jeff King wrote:
> On Tue, Feb 14, 2017 at 08:53:04AM -0800, Jonathan Tan wrote:
>
>> On 02/13/2017 10:07 PM, Jeff King wrote:
>>> diff --git a/builtin/grep.c b/builtin/grep.c
>>> index e83b33bda..c4c632594 100644
>>> --- a/builtin/grep.c
>>> +++ b/builtin/grep.c
>>> @@ -1176,6 +1176,12 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
>>>  			break;
>>>  		}
>>>
>>> +		if (!use_index) {
>>> +			if (seen_dashdash)
>>> +				die(_("--no-index cannot be used with revs"));
>>
>> There is a subsequent check that prints "--no-index or --untracked cannot be
>> used with revs." - maybe we should just expand this part to incorporate that
>> case. (That is, write `if (!use_index || untracked)` instead of `if
>> (!use_index)`.) This also allows us to preserve the error message, which
>> might be useful for someone using a translated version of Git.
>
> I wasn't sure if we wanted to treat "untracked" in the same way.
> Certainly we can catch the error here for the seen_dashdash case, but is
> it also the case that:
>
>   echo content >master
>   git grep --untracked pattern master
>
> should treat "master" as a path?
>
> -Peff

It is already the case that it cannot be treated as a rev:

   $ git grep --untracked pattern master
   fatal: --no-index or --untracked cannot be used with revs.

So I think it would be better if it was treated as a path, for 
consistency with --no-index. I'm OK either way, though.

Re: [PATCH 1/7] grep: move thread initialization a little lower

[Brandon Williams]

On 02/14, Jeff King wrote:
> Originally, we set up the threads for grep before parsing
> the non-option arguments. In 53b8d931b (grep: disable
> threading in non-worktree case, 2011-12-12), the thread code
> got bumped lower in the function because it now needed to
> know whether we got any revision arguments.
> 
> That put a big block of code in between the parsing of revs
> and the parsing of pathspecs, both of which share some loop
> variables. That makes it harder to read the code than the
> original, where the shared loops were right next to each
> other.
> 
> Let's bump the thread initialization until after all of the
> parsing is done.
> 
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> I double-checked to make sure no other code was relying on
> the thread setup having happened. I think we could actually
> bump it quite a bit lower (to right before we actually start
> grepping), but I doubt it matters much in practice.

Looks good.  And yes I don't believe anything needs the thread
initialization to happen earlier.

-- 
Brandon Williams

Re: [PATCH 4/7] grep: re-order rev-parsing loop

[Brandon Williams]

On 02/14, Jeff King wrote:
> -		/* Is it a rev? */
> -		if (!get_sha1_with_context(arg, 0, sha1, &oc)) {
> -			struct object *object = parse_object_or_die(sha1, arg);
> -			if (!seen_dashdash)
> -				verify_non_filename(prefix, arg);
> -			add_object_array_with_path(object, arg, &list, oc.mode, oc.path);
> -			continue;
> -		}
> -		break;
> +
> +		/* Stop at the first non-rev */
> +		if (get_sha1_with_context(arg, 0, sha1, &oc))
> +			break;
> +
> +		object = parse_object_or_die(sha1, arg);
> +		if (!seen_dashdash)
> +			verify_non_filename(prefix, arg);
> +		add_object_array_with_path(object, arg, &list, oc.mode, oc.path);

This is much more readable!

-- 
Brandon Williams

Re: [PATCH 5/7] grep: fix "--" rev/pathspec disambiguation

[Brandon Williams]

On 02/14, Jeff King wrote:
> -	/* Check revs and then paths */
> +	/*
> +	 * We have to find "--" in a separate pass, because its presence
> +	 * influences how we will parse arguments that come before it.
> +	 */
> +	for (i = 0; i < argc; i++) {
> +		if (!strcmp(argv[i], "--")) {
> +			seen_dashdash = 1;
> +			break;
> +		}
> +	}

So this simply checks if "--" is an argument that was provided.  This
then allows grep to know ahead of time how to handle revs/paths
preceding a "--" or in the absences of the "--".  Seems sensible to me.

> +
> +	/*
> +	 * Resolve any rev arguments. If we have a dashdash, then everything up
> +	 * to it must resolve as a rev. If not, then we stop at the first
> +	 * non-rev and assume everything else is a path.
> +	 */
>  	for (i = 0; i < argc; i++) {
>  		const char *arg = argv[i];
>  		unsigned char sha1[20];
> @@ -1158,13 +1173,14 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
>  
>  		if (!strcmp(arg, "--")) {
>  			i++;
> -			seen_dashdash = 1;
>  			break;
>  		}
>  
> -		/* Stop at the first non-rev */
> -		if (get_sha1_with_context(arg, 0, sha1, &oc))
> +		if (get_sha1_with_context(arg, 0, sha1, &oc)) {
> +			if (seen_dashdash)
> +				die(_("unable to resolve revision: %s"), arg);
>  			break;
> +		}
>  
>  		object = parse_object_or_die(sha1, arg);
>  		if (!seen_dashdash)
> @@ -1172,7 +1188,10 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
>  		add_object_array_with_path(object, arg, &list, oc.mode, oc.path);
>  	}
>  
> -	/* The rest are paths */
> +	/*
> +	 * Anything left over is presumed to be a path. But in the non-dashdash
> +	 * "do what I mean" case, we verify and complain when that isn't true.
> +	 */

-- 
Brandon Williams

Re: [PATCH 5/7] grep: fix "--" rev/pathspec disambiguation

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> If we see "git grep pattern rev -- file" then we apply the
> usual rev/pathspec disambiguation rules: any "rev" before
> the "--" must be a revision, and we do not need to apply the
> verify_non_filename() check.
>
> But there are two bugs here:
>
>   1. We keep a seen_dashdash flag to handle this case, but
>      we set it in the same left-to-right pass over the
>      arguments in which we parse "rev".
>
>      So when we see "rev", we do not yet know that there is
>      a "--", and we mistakenly complain if there is a
>      matching file.
>
>      We can fix this by making a preliminary pass over the
>      arguments to find the "--", and only then checking the rev
>      arguments.
>
>   2. If we can't resolve "rev" but there isn't a dashdash,
>      that's OK. We treat it like a path, and complain later
>      if it doesn't exist.
>
>      But if there _is_ a dashdash, then we know it must be a
>      rev, and should treat it as such, complaining if it
>      does not resolve. The current code instead ignores it
>      and tries to treat it like a path.
>
> This patch fixes both bugs, and tries to comment the parsing
> flow a bit better.

Good.  Thanks.

Re: [PATCH 5/7] grep: fix "--" rev/pathspec disambiguation

[Jeff King]

On Tue, Feb 14, 2017 at 10:56:21AM -0800, Brandon Williams wrote:

> On 02/14, Jeff King wrote:
> > -	/* Check revs and then paths */
> > +	/*
> > +	 * We have to find "--" in a separate pass, because its presence
> > +	 * influences how we will parse arguments that come before it.
> > +	 */
> > +	for (i = 0; i < argc; i++) {
> > +		if (!strcmp(argv[i], "--")) {
> > +			seen_dashdash = 1;
> > +			break;
> > +		}
> > +	}
> 
> So this simply checks if "--" is an argument that was provided.  This
> then allows grep to know ahead of time how to handle revs/paths
> preceding a "--" or in the absences of the "--".  Seems sensible to me.

By the way, we have to check again later for "--" when parsing the revs
themselves. In theory you could set seen_dashdash to the offset of the
dashdash in the array, and do the iteration more like:

  for (i = 0; i < dashdash_pos; i++)
          handle_rev(argv[i]);
  for (i = dashdash_pos + 1; i < argc; i++)
          handle_path(argv[i]);

But our loops also handle the case where there is no "--" at all, and I
think that approach ends up convoluting the logic. I didn't go very far
in that direction before giving it up, though.

-Peff

[PATCH 8/7] grep: treat revs the same for --untracked as for --no-index

[Jeff King]

On Tue, Feb 14, 2017 at 10:19:56AM -0800, Jonathan Tan wrote:

> > I wasn't sure if we wanted to treat "untracked" in the same way.
> > Certainly we can catch the error here for the seen_dashdash case, but is
> > it also the case that:
> > 
> >   echo content >master
> >   git grep --untracked pattern master
> > 
> > should treat "master" as a path?
> 
> It is already the case that it cannot be treated as a rev:
> 
>   $ git grep --untracked pattern master
>   fatal: --no-index or --untracked cannot be used with revs.
> 
> So I think it would be better if it was treated as a path, for consistency
> with --no-index. I'm OK either way, though.

Right, it's always been disallowed. But the early detection changes a
few user-visible behaviors like the exact error message, and how
disambiguation works (see below). I think the arguments for making that
change for --no-index are stronger than for --untracked. But it probably
makes sense for --untracked, too.

Here's a patch on top of my series that lumps the two together again. It
_could_ be squashed into the earlier patch, but I think I prefer keeping
it separate.

-- >8 --
Subject: [PATCH] grep: treat revs the same for --untracked as for --no-index

git-grep has always disallowed grepping in a tree (as
opposed to the working directory) with both --untracked
and --no-index. But we traditionally did so by first
collecting the revs, and then complaining when any were
provided.

The --no-index option recently learned to detect revs
much earlier. This has two user-visible effects:

  - we don't bother to resolve revision names at all. So
    when there's a rev/path ambiguity, we always choose to
    treat it as a path.

  - likewise, when you do specify a revision without "--",
    the error you get is "no such path" and not "--untracked
    cannot be used with revs".

The rationale for doing this with --no-index is that it is
meant to be used outside a repository, and so parsing revs
at all does not make sense.

This patch gives --untracked the same treatment. While it
_is_ meant to be used in a repository, it is explicitly
about grepping the non-repository contents. Telling the user
"we found a rev, but you are not allowed to use revs" is
not really helpful compared to "we treated your argument as
a path, and could not find it".

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/grep.c  | 10 +++++-----
 t/t7810-grep.sh |  2 +-
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/builtin/grep.c b/builtin/grep.c
index 1454bef49..9304c33e7 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -967,6 +967,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
 	int dummy;
 	int use_index = 1;
 	int pattern_type_arg = GREP_PATTERN_TYPE_UNSPECIFIED;
+	int allow_revs;
 
 	struct option options[] = {
 		OPT_BOOL(0, "cached", &cached,
@@ -1165,6 +1166,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
 	 * to it must resolve as a rev. If not, then we stop at the first
 	 * non-rev and assume everything else is a path.
 	 */
+	allow_revs = use_index && !untracked;
 	for (i = 0; i < argc; i++) {
 		const char *arg = argv[i];
 		unsigned char sha1[20];
@@ -1176,9 +1178,9 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
 			break;
 		}
 
-		if (!use_index) {
+		if (!allow_revs) {
 			if (seen_dashdash)
-				die(_("--no-index cannot be used with revs"));
+				die(_("--no-index or --untracked cannot be used with revs"));
 			break;
 		}
 
@@ -1201,7 +1203,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
 	if (!seen_dashdash) {
 		int j;
 		for (j = i; j < argc; j++)
-			verify_filename(prefix, argv[j], j == i && use_index);
+			verify_filename(prefix, argv[j], j == i && allow_revs);
 	}
 
 	parse_pathspec(&pathspec, 0,
@@ -1273,8 +1275,6 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
 
 	if (!use_index || untracked) {
 		int use_exclude = (opt_exclude < 0) ? use_index : !!opt_exclude;
-		if (list.nr)
-			die(_("--no-index or --untracked cannot be used with revs."));
 		hit = grep_directory(&opt, &pathspec, use_exclude, use_index);
 	} else if (0 <= opt_exclude) {
 		die(_("--[no-]exclude-standard cannot be used for tracked contents."));
diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh
index 0ff9f6cae..cee42097b 100755
--- a/t/t7810-grep.sh
+++ b/t/t7810-grep.sh
@@ -1032,7 +1032,7 @@ test_expect_success 'grep --no-index pattern -- path' '
 
 test_expect_success 'grep --no-index complains of revs' '
 	test_must_fail git grep --no-index o master -- 2>err &&
-	test_i18ngrep "no-index cannot be used with revs" err
+	test_i18ngrep "cannot be used with revs" err
 '
 
 test_expect_success 'grep --no-index prefers paths to revs' '
-- 
2.12.0.rc1.479.g59880b11e

Re: [PATCH 8/7] grep: treat revs the same for --untracked as for --no-index

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> ...
> The rationale for doing this with --no-index is that it is
> meant to be used outside a repository, and so parsing revs
> at all does not make sense.
>
> This patch gives --untracked the same treatment. While it
> _is_ meant to be used in a repository, it is explicitly
> about grepping the non-repository contents. Telling the user
> "we found a rev, but you are not allowed to use revs" is
> not really helpful compared to "we treated your argument as
> a path, and could not find it".

Yup, both sounds very sensible.  Thanks.