Re: [PATCH v3 08/12] grep: allow submodule functions to run in parallel

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

On Wed, Jan 29, 2020 at 3:57 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> SZEDER Gábor <szeder.dev@gmail.com> writes:
> >
[...]
> > @@ -1071,6 +1072,14 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
> >                   && (opt.pre_context || opt.post_context ||
> >                       opt.file_break || opt.funcbody))
> >                       skip_first_line = 1;
> > +
> > +             /*
> > +              * Pre-read gitmodules (if not read already) to prevent racy
> > +              * lazy reading in worker threads.
> > +              */
> > +             if (recurse_submodules)
> > +                     repo_read_gitmodules(the_repository, 1);
> >
> > ... and eventually reach this condition, which then reads the
> > submodules even with '--no-index', which is just what a7f3240877 tried
> > to avoid, thus triggering the test failure.
> >
> > It might be that all we need is changing this condition to:
> >
> >  if (recurse_submodules && use_index)

Yes, I think that would work. I was only worried that, in case of
!use_index, the path taken could somehow lead to an unprotected call
to repo_read_gitmodules() (with threads spawned).Then, since the file
would not have been pre-loaded by the sequential code, we could
encounter a race condition. But by what I've inspected, when use_index
is false, grep_directory() will be called to traverse the files, and
it does not have repo_read_gitmodules() in its call graph[1]. So the
solution should be fine in the point of view of thread-safeness.

> Hmph, I wonder if "ignore --recurse-submodules if --no-index" should
> have been done as a single liner patch, something along the lines of
> "after parse_options() returns, drop recurse_submodules if no-index
> was given", i.e.
>
> @@ -958,6 +946,8 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
>                         /* die the same way as if we did it at the beginning */
>                         setup_git_directory();
>         }
> +       if (!use_index)
> +               recurse_submodules = 0; /* ignore */
>
>         /*
>          * skip a -- separator; we know it cannot be

Yeah, this seems more meaningful, IMHO, as we can easily see that the
recurse_submodules option was dropped in favor of using --no-index.

[1]: Well, in fact repo_read_gitmodules() *is* in grep_directory()'s
call graph, but the only path to it is through the
fill_textconv_grep() > fill_textconv() call, which is already guarded
by the obj_read_mutex. So there is no problem here.