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

From: Glen Choo <chooglen@google.com>
Date: May 16, 2022, 22:56 UTC
Message-ID: <kl6lr14thysl.fsf@chooglen-macbookpro.roam.corp.google.com>
In-Reply-To: <xmqqczgd16wx.fsf_-_@gitster.g>

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

Show 23 quoted lines

> diff --git a/builtin/fetch.c b/builtin/fetch.c
> index e3791f09ed..359321e731 100644
> --- a/builtin/fetch.c
> +++ b/builtin/fetch.c
> @@ -2187,6 +2187,10 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
>  		else if (argc > 1)
>  			die(_("fetch --all does not make sense with refspecs"));
>  		(void) for_each_remote(get_one_remote_for_fetch, &list);
> +
> +		/* no point doing fetch_multiple() of one */
> +		if (list.nr == 1)
> +			remote = remote_get(list.items[0].string);
>  	} else if (argc == 0) {
>  		/* No arguments -- use default remote */
>  		remote = remote_get(NULL);
> @@ -2261,7 +2265,7 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
>  		result = fetch_multiple(&list, max_children);
>  	}
>  
> -	if (!result && (recurse_submodules != RECURSE_SUBMODULES_OFF)) {
> +	if (!result && remote && (recurse_submodules != RECURSE_SUBMODULES_OFF)) {
>  		struct strvec options = STRVEC_INIT;
>  		int max_children = max_jobs;

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)) {

but I suppose that the commit message explains this well enough.

Show 21 quoted lines

> diff --git a/t/t5617-clone-submodules-remote.sh b/t/t5617-clone-submodules-remote.sh
> index ca8f80083a..8b6b482a39 100755
> --- a/t/t5617-clone-submodules-remote.sh
> +++ b/t/t5617-clone-submodules-remote.sh
> @@ -72,6 +72,19 @@ test_expect_success 'clone with --single-branch' '
>  	)
>  '
>  
> +test_expect_success 'fetch --all with --recurse-submodules' '
> +	test_when_finished "rm -fr super_clone" &&
> +	git clone --recurse-submodules srv.bare super_clone &&
> +	(
> +		cd super_clone &&
> +		git config submodule.recurse true &&
> +		git config fetch.parallel 0 &&
> +		git fetch --all 2>../fetch-log
> +	) &&
> +	grep "Fetching sub" fetch-log >fetch-subs &&
> +	test_line_count = 1 fetch-subs
> +'
> +

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

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

Whereas this test fails if we remove "&& remote".

  test_expect_success 'fetch --all with --recurse-submodules and more remotes' '
    test_when_finished "rm -fr super_clone" &&
    git clone --recurse-submodules srv.bare super_clone &&
    cp -r srv.bare srv.bare2 &&
    (
      cd super_clone &&
      git config submodule.recurse true &&
      git config fetch.parallel 0 &&
      git remote add other ../srv.bare2 &&
      git fetch --all 2>../fetch-log
    ) &&
    grep "Fetching sub" fetch-log >fetch-subs &&
    test_line_count = 2 fetch-subs
  '