Re: [PATCH] fetch: do not run a redundant fetch from submodule

From: Junio C Hamano <gitster@pobox.com>
Date: May 16, 2022, 23:33 UTC
Message-ID: <xmqqpmkdyrw9.fsf@gitster.g>
In-Reply-To: <kl6lr14thysl.fsf@chooglen-macbookpro.roam.corp.google.com>

Glen Choo <chooglen@google.com> writes:

> IMO "&& remote" is non-inuitive enough to warrant a comment, e.g.
>
>   /* do not update submodules if fetch_multiple() was called */
>   if (!result && remote && (recurse_submodules != RECURSE_SUBMODULES_OFF)) {

As I am not so familiar with this codepath when submodules are involved, I need to be explained why having called fetch_multiple() means there is no need to update submodules".

	/* 
	 * This is only needed after fetch_one(), which does not
         * fetch submodules by itself.
	 *
	 * fetch_multiple() has already updated submodules to grab
	 * commits necessary for the fetched history from each remote,
	 * so there is no need to fetch submodules from here.
	 */

perhaps?

> The test looks good, but I think it belongs in
> t/t5526-fetch-submodules.sh. I don't see anything else in
> t5617-clone-submodules-remote.sh that calls "git fetch".

Good eyes.

> In addition, I think we need one more test that adds another remote. In
> the above test, we only have one remote, so we hit your new optimization
> and already pass the test without the need for "&& remote".

True, too.