Re: [PATCH 7/8] fetch: fetch unpopulated, changed submodules

From: Glen Choo <chooglen@google.com>
Date: Feb 14, 2022, 04:24 UTC
Message-ID: <kl6lh7926poc.fsf@chooglen-macbookpro.roam.corp.google.com>
In-Reply-To: <20220210225137.688066-1-jonathantanmy@google.com>

Jonathan Tan <jonathantanmy@google.com> writes:

Show 26 quoted lines

>> @@ -1495,6 +1499,15 @@ get_fetch_task(struct submodule_parallel_fetch *spf,
>>  		if (!task)
>>  			continue;
>>  
>> +		/*
>> +		 * We might have already considered this submodule
>> +		 * because we saw it when iterating the changed
>> +		 * submodule names.
>> +		 */
>> +		if (string_list_lookup(&spf->seen_submodule_names,
>> +				       task->sub->name))
>> +			continue;
>
> [snip]
>> +		/*
>> +		 * We might have already considered this submodule
>> +		 * because we saw it in the index.
>> +		 */
>> +		if (string_list_lookup(&spf->seen_submodule_names, item.string))
>> +			continue;
>
> Hmm...it's odd that the checks happen in both places, when theoretically
> we would do one after the other, so this check would only need to be in
> one place. Maybe this is because of how we had to implement it (looping
> over everything every time when we get the next fetch task) but if it's
> easy to avoid, that would be great.

Yes, in order for the code to be correct, we only need this check once, but I chose to check twice for defensiveness. That is, we avoid creating implicit dependencies between the functions like "function A does not consider whether a submodule has already been fetched, so it must always be called before function B".

Perhaps there is another concern that overrides this? e.g. performance.