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

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 1, 2024, 17:27 UTC
Message-ID: <xmqqed4zzqbg.fsf@gitster.g>
In-Reply-To: <20241001072423.1165932-1-daniel@mariadb.org>

Daniel Black <daniel@mariadb.org> writes:

> The fetching of submodules erroniously used
> the main repository remote name instead of the
> submodule remote name[1].

Please write the problem description in the present tense, i.e. "the code does this, which is incorrect in this way".

> Correct this by using the correct function
> to reteive the remote name.

That's gives the same as "fix it" ;-).

Can we phrase it in end-user observable terms, for example, "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". It is a mere implementation detail of doing so, to call get_default_remote_submodule() instead of get_default_remote().

> 1. https://www.spinics.net/lists/git/msg462320.html

Please use the URL that shows the Message-Id when referring to a message in the list archive. E.g.,

    https://lore.kernel.org/git/ZJR5SPDj4Wt_gmRO@pweza/
    https://public-inbox.org/git/ZJR5SPDj4Wt_gmRO@pweza/

Show 20 quoted lines

> Signed-off-by: Daniel Black <daniel@mariadb.org>
> ---
>  builtin/submodule--helper.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index a1ada86952..210ae2570a 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -2322,7 +2322,12 @@ 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)
> +			return code;

We never failed in this function to return without calling run_command(). Now we do.

Shouldn't we clean up the child_process structure we allocated and prepared before returning like this? It would just be the matter of calling child_process_clear(&cp), perhaps?

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

Thanks.