On Wed, Jan 29, 2020 at 3:57 PM Junio C Hamano <gitster@xxxxxxxxx> wrote: > > SZEDER Gábor <szeder.dev@xxxxxxxxx> 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.
