git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Glen Choo <chooglen@google.com>
To: Junio C Hamano <gitster@pobox.com>
Cc: git@vger.kernel.org, Jonathan Tan <jonathantanmy@google.com>
Subject: Re: [PATCH v3 2/3] builtin/fetch: skip unnecessary tasks when using --negotiate-only
Date: Wed, 22 Dec 2021 09:28:17 -0800	[thread overview]
Message-ID: <kl6l8rwczgzy.fsf@chooglen-macbookpro.roam.corp.google.com> (raw)
In-Reply-To: <xmqqa6gtkumz.fsf@gitster.g>

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

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

> 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

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

  reply	other threads:[~2021-12-22 17:28 UTC|newest]

Thread overview: 50+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-12-07 19:29 [PATCH] builtin/fetch: skip unnecessary tasks when using --negotiate-only Glen Choo
2021-12-09 22:12 ` Jonathan Tan
2021-12-09 22:36   ` Glen Choo
2021-12-13 22:58     ` Jonathan Tan
2021-12-16 18:11       ` Glen Choo
2021-12-17  0:02 ` [PATCH v2] " Glen Choo
2021-12-17 23:35   ` Junio C Hamano
2021-12-20 19:37     ` Glen Choo
2021-12-20 19:56       ` Junio C Hamano
2021-12-20 20:54         ` Glen Choo
2021-12-20 22:12           ` Junio C Hamano
2021-12-21  0:18             ` Glen Choo
2021-12-21 23:07       ` Glen Choo
2021-12-22  0:11   ` [PATCH v3 0/3] " Glen Choo
2021-12-22  0:11     ` [PATCH v3 1/3] builtin/fetch: use goto cleanup in cmd_fetch() Glen Choo
2021-12-22  0:11     ` [PATCH v3 2/3] builtin/fetch: skip unnecessary tasks when using --negotiate-only Glen Choo
2021-12-22  6:42       ` Junio C Hamano
2021-12-22 17:28         ` Glen Choo [this message]
2021-12-22 19:29           ` Junio C Hamano
2021-12-22 20:27             ` Glen Choo
2021-12-22  0:11     ` [PATCH v3 3/3] builtin/fetch: die on --negotiate-only and --recurse-submodules Glen Choo
2021-12-22  6:46       ` Junio C Hamano
2021-12-23 19:08       ` Jonathan Tan
2022-01-13  0:44     ` [PATCH v4 0/3] fetch: skip unnecessary tasks when using --negotiate-only Glen Choo
2022-01-13  0:44       ` [PATCH v4 1/3] fetch: use goto cleanup in cmd_fetch() Glen Choo
2022-01-13  0:45       ` [PATCH v4 2/3] fetch: skip tasks related to fetching objects Glen Choo
2022-01-13  0:45       ` [PATCH v4 3/3] fetch --negotiate-only: do not update submodules Glen Choo
2022-01-13  1:16         ` Junio C Hamano
2022-01-18 18:54       ` [PATCH v5 0/3] fetch: skip unnecessary tasks when using --negotiate-only Glen Choo
2022-01-18 18:54         ` [PATCH v5 1/3] fetch: use goto cleanup in cmd_fetch() Glen Choo
2022-01-18 18:54         ` [PATCH v5 2/3] fetch: skip tasks related to fetching objects Glen Choo
2022-01-18 18:54         ` [PATCH v5 3/3] fetch --negotiate-only: do not update submodules Glen Choo
2022-01-18 22:05           ` Junio C Hamano
2022-01-18 23:41             ` Glen Choo
2022-01-19  0:26               ` Junio C Hamano
2022-01-19  0:00         ` [PATCH v6 0/3] fetch: skip unnecessary tasks when using --negotiate-only Glen Choo
2022-01-19  0:00           ` [PATCH v6 1/3] fetch: use goto cleanup in cmd_fetch() Glen Choo
2022-01-19  0:00           ` [PATCH v6 2/3] fetch: skip tasks related to fetching objects Glen Choo
2022-01-19  0:00           ` [PATCH v6 3/3] fetch --negotiate-only: do not update submodules Glen Choo
2022-01-20  2:38             ` Jiang Xin
2022-01-20 17:40               ` Glen Choo
2022-01-20 17:49           ` [PATCH v7 0/3] fetch: skip unnecessary tasks when using --negotiate-only Glen Choo
2022-01-20 17:49             ` [PATCH v7 1/3] fetch: use goto cleanup in cmd_fetch() Glen Choo
2022-01-20 17:49             ` [PATCH v7 2/3] fetch: skip tasks related to fetching objects Glen Choo
2022-01-20 17:49             ` [PATCH v7 3/3] fetch --negotiate-only: do not update submodules Glen Choo
2022-01-20 23:08               ` Junio C Hamano
2022-01-20 23:16                 ` Glen Choo
2022-01-20 21:58             ` Re* [PATCH v7 0/3] fetch: skip unnecessary tasks when using --negotiate-only Junio C Hamano
2022-01-20 23:15               ` Glen Choo
2022-01-21  2:17               ` Jiang Xin

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=kl6l8rwczgzy.fsf@chooglen-macbookpro.roam.corp.google.com \
    --to=chooglen@google.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=jonathantanmy@google.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).