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

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 22, 2021, 06:42 UTC
Message-ID: <xmqqa6gtkumz.fsf@gitster.g>
In-Reply-To: <20211222001134.28933-3-chooglen@google.com>

Glen Choo <chooglen@google.com> writes:

Show 14 quoted lines

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

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.

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