Re: [PATCH 6/7] grep: add repository to OID grep sources
- From
- Jonathan Tan <jonathantanmy@google.com>
- Date
- Aug 13, 2021, 16:47 UTC
- Message-ID
- <20210813164734.3219965-1-jonathantanmy@google.com>
- In-Reply-To
- <CAHd-oW4ntWB7KOKxhZU4wM-Xg9MARVN2EmpxEg-rEfc4i_SUnw@mail.gmail.com>
Show 10 quoted lines
> On Tue, Aug 10, 2021 at 3:29 PM Jonathan Tan <jonathantanmy@google.com> wrote: > > > > Record the repository whenever an OID grep source is created, and teach > > the worker threads to explicitly provide the repository when accessing > > objects. > > This patch fixes the second NEEDSWORK comment at grep_submodule(), > right? Maybe this comment could be replaced with a mention that > add_submodule_odb_by_path() is called for testing purposes, and it > should no longer produce a real addition to the alternates list?
Good catch. I'll update the comment.
Show 33 quoted lines
> > diff --git a/grep.h b/grep.h
> > index f4a3090f1c..c5234f9b38 100644
> > --- a/grep.h
> > +++ b/grep.h
> > @@ -187,6 +187,7 @@ struct grep_source {
> > GREP_SOURCE_BUF,
> > } type;
> > void *identifier;
> > + struct repository *repo; /* if GREP_SOURCE_OID */
>
> Hmm, the grep threads now have two `struct repository` references, one
> in `struct grep_source` and one in `struct grep_opt` (see
> builtin/grep.c:run()). The one from `struct grep_opt` will always be
> `the_repository` (in the worker threads). I wonder if, in the long
> run, we could instruct the worker threads to use the new reference
> from `struct grep_source` throughout grep.c, and perhaps even remove
> the one from `struct grep_opt`.
>
> This would solve the issue I mentioned in [1], about git grep
> currently ignoring the textconv attributes from submodules, and
> instead applying the ones from the superproject in the submodules'
> files. (Here there is an example of this bug: [2] .)
>
> It would also allow grep to use the textconv cache from each
> submodule, instead of saving/reading everything from the
> superproject's textconv cache.
>
> While this doesn't happen, though, could we perhaps add a comment
> somewhere to avoid any confusion regarding the two different
> repository pointers that the worker threads hold?
>
> [1]: https://lore.kernel.org/git/CAHd-oW5iEQarYVxEXoTG-ua2zdoybTrSjCBKtO0YT292fm0NQQ@mail.gmail.com/
> [2]: https://gitlab.com/-/snippets/1896951Ah...another good catch. Thanks also for a link to your email - I'll mention it when I add the comment you suggested.