Re: [RFC PATCH v2] submodule: correct remote name with fetch

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 8, 2024, 19:23 UTC
Message-ID: <xmqqzfnez9dv.fsf@gitster.g>
In-Reply-To: <20241008015835.41678-2-daniel@mariadb.org>

Daniel Black <daniel@mariadb.org> writes:

Show 11 quoted lines

> The code fetches the submodules remote based on the superproject remote name
> instead of the submodule remote name[1].
>
> Instead of grabbing the default remote of the superproject repository, ask
> the default remote of the submodule we are going to run 'git fetch' in.
>
> 1. https://lore.kernel.org/git/ZJR5SPDj4Wt_gmRO@pweza/
>
> Signed-off-by: Daniel Black <daniel@mariadb.org>
> ---
>  builtin/submodule--helper.c |  9 ++++-

The proposed log message is very well written.

>  t/t5568-fetch-submodule.sh  | 65 +++++++++++++++++++++++++++++++++++++
>  2 files changed, 73 insertions(+), 1 deletion(-)
>  create mode 100755 t/t5568-fetch-submodule.sh

Hmph,

	$ git grep "submodule update" t/

gives quite a many hits in existing tests. Didn't any of them have sufficient preparation steps that testing of this bugfix can reuse?

A test on "submodule update" behaviour tends to need quite a lot of preparation. Preparing the superproject, addition of a submodule to it, cloning of these two projects, and then half-preparing a clone of these super-sub arrangement. All of that needs to happen before we can say "submodule update" and observe the outcome to see if the bug still exists. If we can piggy-back on a test script that already has such preparation, it would be far more preferrable than having to do another set of preparation.

Another thing. If this is not about a bug that only manifests when the HTTP transport is in use, it is strongly preferred to avoid turning it into an httpd test. Some developers and/or environments skip them.

Show 17 quoted lines

> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index a1ada86952..567d21d330 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -2322,7 +2322,14 @@ static int fetch_in_submodule(const char *module_path, int depth, int quiet,
>  		strvec_pushf(&cp.args, "--depth=%d", depth);
>  	if (oid) {
>  		char *hex = oid_to_hex(oid);
> -		char *remote = get_default_remote();
> +		char *remote;
> +		int code;
> +
> +		code = get_default_remote_submodule(module_path, &remote);
> +		if (code) {
> +			child_process_clear(&cp);
> +			return code;
> +		}

The get_default_remote_submodule() helper eventually calls into repo_get_default_remote() that returns an allocated string in remote, but it only does so when it succeeds, so this early return does not have to worry about leaking "remote" here. Good.

The code change looks quite straight-forward and looking good.

>  		strvec_pushl(&cp.args, remote, hex, NULL);
>  		free(remote);

Thanks.