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

[Junio C Hamano <gitster@pobox.com>]

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/

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