Re: [PATCH] grep: die gracefully when outside repository

From: Jeff King <peff@peff.net>
Date: Oct 15, 2023, 03:26 UTC
Message-ID: <20231015032636.GC554702@coredump.intra.peff.net>
In-Reply-To: <087c92e3904dd774f672373727c300bf7f5f6369.1697317276.git.code@khaugsbakk.name>

On Sat, Oct 14, 2023 at 11:02:38PM +0200, Kristoffer Haugsbakk wrote:

Show 16 quoted lines

> Die gracefully when `git grep --no-index` is run outside of a Git
> repository and the path is outside the directory tree.
> 
> If you are not in a Git repository and say:
> 
>     git grep --no-index search ..
> 
> You trigger a `BUG`:
> 
>     BUG: environment.c:213: git environment hasn't been setup
>     Aborted (core dumped)
> 
> Because `..` is a valid path which is treated as a pathspec. Then
> `pathspec` figures out that it is not in the current directory tree. The
> `BUG` is triggered when `pathspec` tries to advice the user about the path
> to the (non-existing) repository.

Is it even reasonable for "grep --no-index" to care about leaving the tree in the first place? That is, is there a reason we should not allow:

  git grep --no-index foo ../bar

? And if we do want to care, there is a weirdness here that even with your patch, we check to see if the file exists:

  $ git grep --no-index foo ../does-exist
  fatal: '../does-exist' is outside the directory tree

  $ git grep --no-index foo ../does-not-exist
  fatal: ../does-not-exist: no such path in the working tree.

If we want to avoid leaving the current directory, then I think we need to be checking much sooner (but again, I would argue that it is not worth caring about in no-index mode).

I do think your patch does not make anything worse (and indeed makes the error output much better). So I do not mind it in the meantime. But I have a feeling that we'd end up reverting it as part of the fix for the larger issue.

-Peff