Re: [PATCH v4 1/8] fetch: fix `--no-recurse-submodules` with multi-remote fetches

From: Junio C Hamano <gitster@pobox.com>
Date: May 9, 2023, 17:49 UTC
Message-ID: <xmqq1qjpz96s.fsf@gitster.g>
In-Reply-To: <d82b42ed345ac7b482bf5dd96456f131ecb8c746.1683636885.git.ps@pks.im>

Patrick Steinhardt <ps@pks.im> writes:

Show 11 quoted lines

> When running `git fetch --no-recurse-submodules`, the exectation is that
> we don't fetch any submodules. And while this works for fetches of a
> single remote, it doesn't when fetching multiple remotes at once. The
> result is that we do recurse into submodules even though the user has
> explicitly asked us not to.
>
> This is because while we pass on `--recurse-submodules={yes,on-demand}`
> if specified by the user, we don't pass on `--no-recurse-submodules` to
> the subprocess spawned to perform the submodule fetch.
>
> Fix this by also forwarding this flag as expected.

Makes sense.

Show 8 quoted lines

> +test_expect_success "fetch --all with --no-recurse-submodules only fetches superproject" '
> +	test_when_finished "rm -rf src_clone" &&
> +
> +	git clone --recurse-submodules src src_clone &&
> +	(
> +		cd src_clone &&
> +		git remote add secondary ../src &&
> +		git config submodule.recurse true &&

The above two is essential to this test; we are interested in making sure that --no-recurse-submodules is propagated down even when the "--all" option is used, and we want another remote for that. We set the default to recurse, so that passing "--no-recurse-submodules" would defeat it, but just refraining to pass "--recurse-submodules" would cause us to recurse.

> +		git config fetch.parallel 0 &&

Is this necessary for the purpose of the test, though? It should not hurt, but we do not require the end-users to set it in real life for the parallel fetching to work, either, right?

Just being curious.

Show 9 quoted lines

> +		git fetch --all --no-recurse-submodules 2>../actual
> +	) &&
> +
> +	cat >expect <<-EOF &&
> +	From ../src
> +	 * [new branch]      master     -> secondary/master
> +	EOF
> +	test_cmp expect actual
> +'

In the context of a series that attempts to introduce a new stable output format for machine consumption, which implies the traditional output can change to match human users' preference, this test feels a bit brittle, but let's wait until the end of the series to judge that.

Looking good.  Thanks.

>  test_done