Re: [PATCH v3 2/3] builtin/fetch: skip unnecessary tasks when using --negotiate-only

From: Glen Choo <chooglen@google.com>
Date: Dec 22, 2021, 17:28 UTC
Message-ID: <kl6l8rwczgzy.fsf@chooglen-macbookpro.roam.corp.google.com>
In-Reply-To: <xmqqa6gtkumz.fsf@gitster.g>

Junio C Hamano <gitster@pobox.com> writes:

Show 28 quoted lines

> Glen Choo <chooglen@google.com> writes:
>
>> @@ -2113,6 +2122,17 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
>>  		result = fetch_multiple(&list, max_children);
>>  	}
>>  
>> +	/*
>> +	 * Skip irrelevant tasks because we know objects were not
>> +	 * fetched.
>> +	 *
>> +	 * NEEDSWORK: as a future optimization, we can return early
>> +	 * whenever objects were not fetched e.g. if we already have all
>> +	 * of them.
>> +	 */
>> +	if (negotiate_only)
>> +		goto cleanup;
>
> Sorry if I did not mention this in the review of the earlier round,
> but I think the location this patch places the jump is wrong,
> especially with the NEEDSWORK comment.
>
> When we are not under negotiate_only, if our earlier call to
> transport_fetch_refs() learns to tell us that that we did not add
> any new objects, we would be able to jump to the clean-up label,
> making the above code to:
>
> 	if (negotiate_only || !num_fetched_objects)
> 		goto cleanup;

Thanks for the clarification. Yes, we agree that the location of the jump in this patch should be the same as the location of the jump after the future optimization.

Show 14 quoted lines

> But such a future enhancement is made harder by having this jump
> here---the optimization the NEEDSWORK comment alludes to has no
> reason to be incompatible with "--recurse-submodules".
>
> If the above block is placed _after_ the "if the main fetch was
> successful, and we are not told not to recurse into submodules, then
> do this" block we see below, then
>
>  (1) this patch still achieves its goal, as we have manually
>      and unconditionally turned recursion off;
>
>  (2) such a future enhancement will not be forbidden from working
>      with recurse-submodules feature.
>

I would have come to same conclusion if I agreed that we should recurse into submodules even if no objects are fetched. When I first wrote this patch, I was convinced that "no new objects" implies "no need to update submodules" (see my response at [1]), but I'm not sure any more and I'd like to check my understanding.

The way "fetch --recurse-submodules" works is that the changed submodules are calcuated from the newly updated tips fetched from the remote. If no objects were fetched, we already have all of the superproject commits.

In ~99% of the time, no objects were fetched because the remote doesn't have any info we do not know about - there are no new commits and no refs were updated. In this scenario, 'git fetch' can avoid recursing into submodules because there's no need to. But if we choose to recurse, the worst thing that happens is that we do some file I/O and realize that there are no changed submodules - essentially a no-op given that fetch is slow.

(This is where my understanding of objects vs refs needs to be checked) In the other ~1%, we might already have all commits, but a remote ref might still have moved, albeit to a known commit. In this case, the submodule would need to be updated because it might have changed.

If my understanding is correct, then my patch produces the wrong behavior in that ~1%. But even if my understanding is wrong, and we don't need to worry about that edge case, I see that there's unnecessary risk in trying to be too be clever in my reasoning and skipping what is essentially a no-op.

Is my understanding accurate? At any rate, I'm somewhat convinced to move the jump to just after the "if main fetch was successful, and we are not told not to recurse into submodules" block, i.e. before the "if we should write the commit graph" block.

[1] https://lore.kernel.org/git/kl6ltuf3ysnw.fsf@chooglen-macbookpro.roam.corp.google.com

Show 25 quoted lines

>>  	if (!result && (recurse_submodules != RECURSE_SUBMODULES_OFF)) {
>>  		struct strvec options = STRVEC_INIT;
>>  		int max_children = max_jobs;
>> diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
>> index 8212ca56dc..732031085e 100755
>> --- a/t/t5516-fetch-push.sh
>> +++ b/t/t5516-fetch-push.sh
>> @@ -229,6 +229,18 @@ test_expect_success 'push with negotiation proceeds anyway even if negotiation f
>>  	test_i18ngrep "push negotiation failed" err
>>  '
>>  
>> +test_expect_success 'push with negotiation does not attempt to fetch submodules' '
>> +	mk_empty submodule_upstream &&
>> +	test_commit -C submodule_upstream submodule_commit &&
>> +	git submodule add ./submodule_upstream submodule &&
>> +	mk_empty testrepo &&
>> +	git push testrepo $the_first_commit:refs/remotes/origin/first_commit &&
>> +	test_commit -C testrepo unrelated_commit &&
>> +	git -C testrepo config receive.hideRefs refs/remotes/origin/first_commit &&
>> +	git -c submodule.recurse=true -c protocol.version=2 -c push.negotiate=1 push testrepo refs/heads/main:refs/remotes/origin/main 2>err &&
>> +	! grep "Fetching submodule" err
>> +'
>> +
>>  test_expect_success 'push without wildcard' '
>>  	mk_empty testrepo &&