git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [RFC PATCH v2 3/4] grep: honor sparse checkout patterns

From
Matheus Tavares Bernardino <matheus.bernardino@usp.br>
Date
May 13, 2020, 00:05 UTC
Message-ID
<CAHd-oW4k-9ZHQJcqEUqzxw0YShPWCaG61=v4wyBvXte2sSTV5g@mail.gmail.com>
In-Reply-To
<xmqqh7wmqn7k.fsf@gitster.c.googlers.com>
On Mon, May 11, 2020 at 4:35 PM Junio C Hamano <gitster@pobox.com> wrote:
Show 28 quoted lines
>
> Matheus Tavares <matheus.bernardino@usp.br> writes:
>
> > One of the main uses for a sparse checkout is to allow users to focus on
> > the subset of files in a repository in which they are interested. But
> > git-grep currently ignores the sparsity patterns and report all matches
> > found outside this subset, which kind of goes in the opposite direction.
> > Let's fix that, making it honor the sparsity boundaries for every
> > grepping case:
> >
> > - git grep in worktree
> > - git grep --cached
> > - git grep $REVISION
>
> It makes sense for these to be limited within the "sparse" area.
>
> > - git grep --untracked and git grep --no-index (which already respect
> >   sparse checkout boundaries)
>
> I can understand the former; those untracked files are what _could_
> be brought into attention by "git add", so limiting to the same
> "sparse" area may make sense.
>
> I am not sure about the latter, though, as "--no-index" is an
> explicit request to pretend that we are dealing with a random
> collection of files, not managed in a git repository.  But perhaps
> there is a similar justification like how "--untracked" is
> unjustifiable.  I dunno.

Yeah, I think there was no need to mention those two cases here. My intention was to say that, in these cases, we should stick to the files that are present in the working tree (which should match the sparsity patterns + untracked {and ignored, in --no-index}), as opposed to how the worktree grep used to behave until now, falling back to the cache on files excluded by the sparse checkout.

Show 24 quoted lines
> > diff --git a/builtin/grep.c b/builtin/grep.c
> > index a5056f395a..91ee0b2734 100644
> > --- a/builtin/grep.c
> > +++ b/builtin/grep.c
> > @@ -410,7 +410,7 @@ static int grep_cache(struct grep_opt *opt,
> >                     const struct pathspec *pathspec, int cached);
> >  static int grep_tree(struct grep_opt *opt, const struct pathspec *pathspec,
> >                    struct tree_desc *tree, struct strbuf *base, int tn_len,
> > -                  int check_attr);
> > +                  int is_root_tree);
> >
> >  static int grep_submodule(struct grep_opt *opt,
> >                         const struct pathspec *pathspec,
> > @@ -508,6 +508,10 @@ 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) && !S_ISGITLINK(ce->ce_mode))
> > +                     continue;
>
> Hmph.  Why exclude gitlink from this rule?  If a submodule sits at a
> path that is excluded by the sparse pattern, should we still recurse
> into it?

The idea behind not skipping gitlinks here was to be compliant with what we have in the working tree. In 4fd683b ("sparse-checkout: document interactions with submodules"), we decided that, if the sparse-checkout patterns exclude a submodule, the submodule would still appear in the working tree. The purpose was to keep these features (submodules and sparse-checkout) independent. Along the same lines, I think we should always recurse into initialized submodules in grep, and then load their own sparsity patterns, to decide what should be grepped within.

[...]
Show 22 quoted lines
> > +static int grep_tree(struct grep_opt *opt, const struct pathspec *pathspec,
> > +                  struct tree_desc *tree, struct strbuf *base, int tn_len,
> > +                  int is_root_tree)
> > +{
> > +     struct pattern_list *patterns = NULL;
> > +     int ret;
> > +
> > +     if (is_root_tree)
> > +             patterns = get_sparsity_patterns(opt->repo);
> > +
> > +     ret = do_grep_tree(opt, pathspec, tree, base, tn_len, is_root_tree,
> > +                        patterns, 0);
> > +
> > +     if (patterns) {
> > +             clear_pattern_list(patterns);
> > +             free(patterns);
> > +     }
>
> OK, it is not like this codepath is driven by "git log" to grep from
> top-level tree objects of many commits, so it is OK to grab the
> sparsity patterns once before do_grep_tree() and discard it when we
> are done.

Yeah. A possible performance problem here would be when users pass many trees to git-grep (since we are reloading the pattern lists, from both the_repository and submodules, for each tree). But, as Elijah pointed out [1], the cases where this overhead might be somewhat noticeable should be very rare.

[1]: https://lore.kernel.org/git/CABPp-BGUf-4exGW23xka1twf2D=nFOz1CkD_f-rDX_AGdVEeDA@mail.gmail.com/
Previous: Junio C HamanoNext: Junio C Hamano
Message 42 of 62 in “grep: honor sparse checkout and add option to ignore it”
  1. 0/3 grep: honor sparse checkout and add option to ignore itMatheus Tavares, Mar 24, 2020
  2. 1/3 doc: grep: unify info on configuration variablesMatheus Tavares, Mar 24, 2020
  3. Elijah NewrenMar 24, 2020
  4. Junio C HamanoMar 24, 2020
  5. Matheus TavaresMar 24, 2020
  6. 2/3 grep: honor sparse checkout patternsMatheus Tavares, Mar 24, 2020
  7. Elijah NewrenMar 24, 2020
  8. Derrick StoleeMar 24, 2020
  9. Elijah NewrenMar 24, 2020
  10. Derrick StoleeMar 24, 2020
  11. Matheus Tavares BernardinoMar 24, 2020
  12. Matheus Tavares BernardinoMar 24, 2020
  13. Matheus Tavares BernardinoApr 21, 2020
  14. Elijah NewrenApr 21, 2020
  15. Derrick StoleeApr 22, 2020
  16. Matheus Tavares BernardinoApr 23, 2020
  17. 3/3 grep: add option to ignore sparsity patternsMatheus Tavares, Mar 24, 2020
  18. Elijah NewrenMar 24, 2020
  19. Junio C HamanoMar 24, 2020
  20. Elijah NewrenMar 24, 2020
  21. Junio C HamanoMar 25, 2020
  22. Matheus Tavares BernardinoMar 30, 2020
  23. Elijah NewrenMar 31, 2020
  24. Derrick StoleeMar 31, 2020
  25. Matheus Tavares BernardinoApr 27, 2020
  26. Elijah NewrenApr 29, 2020
  27. Elijah NewrenApr 29, 2020
  28. Matheus Tavares BernardinoMar 25, 2020
  29. Elijah NewrenMar 26, 2020
  30. Junio C HamanoMar 27, 2020
  31. Elijah NewrenMar 27, 2020
  32. Matheus Tavares BernardinoMar 30, 2020
  33. Elijah NewrenMar 31, 2020
  34. 0/4 grep: honor sparse checkout and add option to ignore itMatheus Tavares, May 10, 2020
  35. 1/4 doc: grep: unify info on configuration variablesMatheus Tavares, May 10, 2020
  36. 2/4 config: load the correct config.worktree fileMatheus Tavares, May 10, 2020
  37. Junio C HamanoMay 11, 2020
  38. Matheus Tavares BernardinoMay 12, 2020
  39. Junio C HamanoMay 12, 2020
  40. 3/4 grep: honor sparse checkout patternsMatheus Tavares, May 10, 2020
  41. Junio C HamanoMay 11, 2020
  42. Matheus Tavares BernardinoMay 13, 2020
  43. Junio C HamanoMay 13, 2020
  44. Elijah NewrenMay 21, 2020
  45. Matheus Tavares BernardinoMay 21, 2020
  46. Elijah NewrenMay 21, 2020
  47. Matheus Tavares BernardinoMay 22, 2020
  48. Elijah NewrenMay 22, 2020
  49. Elijah NewrenMay 22, 2020
  50. Matheus Tavares BernardinoMay 22, 2020
  51. Elijah NewrenMay 22, 2020
  52. Derrick StoleeJun 10, 2020
  53. Matheus Tavares BernardinoJun 10, 2020
  54. Derrick StoleeJun 10, 2020
  55. Matheus Tavares BernardinoJun 10, 2020
  56. Elijah NewrenJun 10, 2020
  57. Elijah NewrenJun 10, 2020
  58. Elijah NewrenMay 21, 2020
  59. 4/4 config: add setting to ignore sparsity patterns in some cmdsMatheus Tavares, May 10, 2020
  60. Matheus Tavares BernardinoMay 10, 2020
  61. Elijah NewrenMay 21, 2020
  62. Elijah NewrenMay 21, 2020

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.