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

On Tue, Aug 10, 2021 at 3:29 PM Jonathan Tan [off-list ref] wrote:

quoted

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.

quoted

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/ (local)
[2]: https://gitlab.com/-/snippets/1896951

Ah...another good catch. Thanks also for a link to your email - I'll
mention it when I add the comment you suggested.