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

From: Matheus Tavares Bernardino <matheus.bernardino@usp.br>
Date: Jan 29, 2020, 20:42 UTC
Message-ID: <CAHd-oW590ZnNnCdD5LLiBQB73LRUVEf41wv7FLJvGMwd2kLYww@mail.gmail.com>
In-Reply-To: <xmqqy2tqdr9t.fsf@gitster-ct.c.googlers.com>

On Wed, Jan 29, 2020 at 3:57 PM Junio C Hamano <gitster@pobox.com> wrote:

>
> SZEDER Gábor <szeder.dev@gmail.com> writes:
> >

[...]

Show 19 quoted lines

> > @@ -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.

Show 14 quoted lines

> 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.