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

From: Junio C Hamano <[email-masked]>
Date: Wed, 29 Jan 2020 10:49:51 -0800

SZEDER Gábor <[email-masked]> writes:

> this patch below and a7f3240877 (grep: ignore --recurse-submodules if
> --no-index is given, 2020-01-26) on topic
> 'pb/do-not-recurse-grep-no-index' don't work well together, and cause
> failure of the test 'grep --recurse-submodules --no-index ignores
> --recurse-submodules' in 't7814-grep-recurse-submodules.sh', i.e. in
> the new test added in a7f3240877.

Yup, I was looking at it and trying to see what was going on.

>> +	if (recurse_submodules && (!use_index || untracked))
>> +		die(_("option not supported with --recurse-submodules"));
>
> So this patch moves this condition here, expecting git to die with 
> '--recurse-submodules --no-index'.  However, a7f3240877 removes the
> '!use_index' part of the condition, so we won't die here ...
>
>>  	if (list.nr || cached || show_in_pager) {
>>  		if (num_threads > 1)
>>  			warning(_("invalid option combination, ignoring --threads"));
>> @@ -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)
>
> Or maybe not, but this change on top of 'pu' makes t7814 succeed
> again.

Sounds like a sensible idea.