Re: [RFC PATCH 3/3] grep: add option to ignore sparsity patterns

From: Matheus Tavares Bernardino <matheus.bernardino@usp.br>
Date: Mar 25, 2020, 23:15 UTC
Message-ID: <CAHd-oW6ANf_iNmYkgXt4jo1seB-AEx1TSH384v+oh1bicG8RDA@mail.gmail.com>
In-Reply-To: <CABPp-BEbNCYk0pCuEDQ_ViB2=varJPBsVODxNvJs0EVRyBqjBg@mail.gmail.com>

On Tue, Mar 24, 2020 at 4:55 AM Elijah Newren <newren@gmail.com> wrote:

Show 30 quoted lines

>
> On Mon, Mar 23, 2020 at 11:13 PM Matheus Tavares
> <matheus.bernardino@usp.br> wrote:
>
> > Signed-off-by: Matheus Tavares <matheus.bernardino@usp.br>
> > ---
> >
> > Note: I still have to make --ignore-sparsity be able to work together
> > with --untracked. Unfortunatelly, this won't be as simple because the
> > codeflow taken by --untracked goes to grep_directory() which just
> > iterates the working tree, without looking the index entries. So I will
> > have to either: make --untracked use grep_cache(), and grep the
> > untracked files later; or try matching the working tree paths against
> > the sparsity patterns, without looking for the skip_worktree bit in
> > the index (as I mentioned in the previous patch's comments). Any
> > preferences regarding these two approaches? (or other suggestions?)
>
> Hmm.  So, 'tracked' in git is the idea that we are keeping information
> about specific files.  'sparse-checkout' is the idea that we have a
> subset of those that we can work with without materializing all the
> other tracked files; it's clearly a subset of the realm of 'tracked'.
> 'untracked' is about getting everything outside the set of 'tracked'
> files, which to me means it is clearly outside the set of sparsity
> paths too (and thus you could take --untracked as implying
> --ignore-sparsity, though whether you do might not matter in practice
> because of the items I'll discuss next). Of course, I am also
> assuming `--untracked` is incompatible with --cached or specifying
> revisions or trees (based on it's definiton of "In addition to
> searching in the tracked files in the *working tree*, search also in
> untracked files." -- emphasis added.)

Hm, I see the point now, but I'm still a little confused: The "in the working tree" section of the definition would exclude non checked out files, right? However, git-grep's description says "Look for specified patterns in the tracked files *in the work tree*", and it still searches non checked out files (loading them from the cache, even when --cache is not given). I know that's exactly what we are trying to change with this patchset, but we will still give the --ignore-sparsity option to allow the old behavior when needed (unless we prohibit using --ignore-sparsity without --cached or $REV). I guess my doubt is whether the problem is in the implementation of the working tree grep, which considers non checked out files, or in the docs, which say "tracked files *in the work tree*".

I tend to go with the latter, since using `git grep --ignore-sparsity` in a sparse checked out working tree, to grep not present files as well, kind of makes sense to me. And if the problem is indeed in the docs, then I think we should also allow --ignore-sparsity when grepping with --untracked, since it's an analogous case.

Show 15 quoted lines

> If the incompatibility of
> --untracked and --cached/REVSIONS/TREES is not enforced, we may want
> to look into erroring out if they are given together.  Once we do, we
> don't have to worry about grep_cache() at all in the case of
> --untracked and shouldn't.  Files with the skip_worktree bit won't
> exist in the working directory, and thus won't be searched (this is
> what makes --untracked imply --ignore-sparsity not really matter).
>
> In short: With --untracked you are grepping ALL (non-ignored) files in
> the working directory -- either because they are both tracked and in
> the sparsity paths (anything tracked that isn't in the sparsity paths
> has the skip_worktree bit and thus isn't present), or because it is an
> untracked file.  [And this may be what grep_directory() already does.]
>
> Does that make sense?

It does, and thanks for a very detailed explanation. But as I mentioned before, I'm a little uncertain about --untracked implying --ignore-spasity. The commit that added --untracked (0a93fb8) says:

"grep --untracked" would find the specified patterns from files in untracked files in addition to its usual behaviour of finding them in the tracked files

So, in my mind, it feels like --untracked wasn't meant to limit the search to "all non-ignored files in the working directory", but to add untracked files to the search (which could also contain tracked but non checked out files). Wouldn't the "all non-ignored files in the working directory" case be the use of --no-index?

> > diff --git a/builtin/grep.c b/builtin/grep.c
> > index 52ec72a036..17eae3edd6 100644
> > --- a/builtin/grep.c
> > +++ b/builtin/grep.c

...

Show 9 quoted lines

> >
> > @@ -487,7 +492,7 @@ static int grep_cache(struct grep_opt *opt,
> >         for (nr = 0; nr < repo->index->cache_nr; nr++) {
> >                 const struct cache_entry *ce = repo->index->cache[nr];
> >
> > -               if (ce_skip_worktree(ce))
> > +               if (!ignore_sparsity && ce_skip_worktree(ce))
>
> Oh boy on the double negatives...maybe we want to rename this flag somehow?

Yeah, I also thought about that, but couldn't come up with a better name myself... My alternatives were all too verbose.

...

> I'm super excited to see work in this area.  I hope I'm not
> discouraging you by attempting to provide what I think is the bigger
> picture I'd like us to work towards.

Not at all! :) Thanks a lot for the bigger picture and other explanations. They help me understand the long-term goals and make better decisions now.