patchgrep: do not unnecessarily query repo for "--"

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

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

Nicely explained.

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.

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

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.

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

On Mon, Feb 13, 2017 at 08:20:37PM -0500, 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

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

-Peff

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

On 02/14, Jeff King wrote:

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

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

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

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

On 02/14, Jeff King wrote:

This is much more readable!

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

On 02/14, Jeff King wrote:

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.

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

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

Jeff King <peff@peff.net> writes:

Good.  Thanks.

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(+)

On 02/13/2017 10:07 PM, Jeff King wrote:

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.

On Tue, Feb 14, 2017 at 08:53:04AM -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?

-Peff

On 02/14/2017 10:04 AM, Jeff King wrote:

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.

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

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

Jeff King <peff@peff.net> writes:

Yup, both sounds very sensible.  Thanks.

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

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

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

On 02/13/2017 10:00 PM, Jeff King wrote:

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.