Re: [PATCH 6/7] grep: add repository to OID grep sources

[Matheus Tavares Bernardino <matheus.bernardino@usp.br>]

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?

> 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/1896951

>         char *buf;
>         unsigned long size;
> @@ -198,7 +199,8 @@ struct grep_source {
>  void grep_source_init_file(struct grep_source *gs, const char *name,
>                            const char *path);
>  void grep_source_init_oid(struct grep_source *gs, const char *name,
> -                         const char *path, const struct object_id *oid);
> +                         const char *path, const struct object_id *oid,
> +                         struct repository *repo);
>  void grep_source_init_buf(struct grep_source *gs);
>  void grep_source_clear_data(struct grep_source *gs);
>  void grep_source_clear(struct grep_source *gs);
> --
> 2.33.0.rc1.237.g0d66db33f3-goog
>