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

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

Daniel Black <daniel@mariadb.org> writes:

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


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