Re: [PATCH] submodule helper: convert relative URL to absolute URL if needed

From: Jonathan Nieder <jrnieder@gmail.com>
To: Stefan Beller <sbeller@google.com>
Cc: git@vger.kernel.org, Jaewoong Jung <jungjw@google.com>
Subject: Re: [PATCH] submodule helper: convert relative URL to absolute URL if needed
Date: Fri, 12 Oct 2018 15:27:12 -0700	[thread overview]
Message-ID: <20181012222712.GC52080@aiede.svl.corp.google.com> (raw)
In-Reply-To: <20181012215314.44266-1-sbeller@google.com>

Hi,

Stefan Beller wrote:

> The submodule helper update_clone called by "git submodule update",
> clones submodules if needed. As submodules used to have the URL indicating
> if they were active, the step to resolve relative URLs was done in the
> "submodule init" step. Nowadays submodules can be configured active without
> calling an explicit init, e.g. via configuring submodule.active.
>
> Then we'll fallback to the URL found in the .gitmodules, which may be
> relative to the superproject, but we do not resolve it, yet.

Oh!  Good catch.

> To do so, factor out the function that resolves the relative URLs in
> "git submodule init" (in the submodule helper in the init_submodule
> function) and cal it at the appropriate place in the update_clone helper.

s/cal/call/

> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>  builtin/submodule--helper.c | 48 ++++++++++++++++++++++++-------------
>  t/t7400-submodule-basic.sh  | 24 +++++++++++++++++++
>  2 files changed, 55 insertions(+), 17 deletions(-)

What is the symptom when this fails?

> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index f6fb8991f3..a9a3ac38be 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -584,6 +584,27 @@ static int module_foreach(int argc, const char **argv, const char *prefix)
>  	return 0;
>  }
>  
> +
> +char *compute_submodule_clone_url(const char *rel_url)

Should be static.

Is the caller responsible for freeing the returned buffer?

> +{
> +	char *remoteurl, *relurl;
> +	char *remote = get_default_remote();
> +	struct strbuf remotesb = STRBUF_INIT;

optional: could rename, something like

	struct strbuf key = STRBUF_INIT;

	remote = get_default_remote();
	strbuf_addf(&key, "remote.%s.url", remote);

 [...]
	strbuf_release(&key);
	free(remote);
	return result;

> +
> +	strbuf_addf(&remotesb, "remote.%s.url", remote);
> +	free(remote);
> +
> +	if (git_config_get_string(remotesb.buf, &remoteurl)) {
> +		warning(_("could not lookup configuration '%s'. Assuming this repository is its own authoritative upstream."), remotesb.buf);

nit: lookup is the noun, "look up" is the verb

> +		remoteurl = xgetcwd();
> +	}
> +	relurl = relative_url(remoteurl, rel_url, NULL);
> +	strbuf_release(&remotesb);
> +	free(remoteurl);
> +
> +	return relurl;
> +}
> +
>  struct init_cb {
>  	const char *prefix;
>  	unsigned int flags;
> @@ -634,21 +655,9 @@ static void init_submodule(const char *path, const char *prefix,
>  		/* Possibly a url relative to parent */
>  		if (starts_with_dot_dot_slash(url) ||
>  		    starts_with_dot_slash(url)) {
> -			char *remoteurl, *relurl;
> -			char *remote = get_default_remote();
> -			struct strbuf remotesb = STRBUF_INIT;
> -			strbuf_addf(&remotesb, "remote.%s.url", remote);
> -			free(remote);
> -
> -			if (git_config_get_string(remotesb.buf, &remoteurl)) {
> -				warning(_("could not lookup configuration '%s'. Assuming this repository is its own authoritative upstream."), remotesb.buf);
> -				remoteurl = xgetcwd();
> -			}
> -			relurl = relative_url(remoteurl, url, NULL);
> -			strbuf_release(&remotesb);
> -			free(remoteurl);
> -			free(url);
> -			url = relurl;

Ah, this is moved code. I should have used --color-moved. ;-)

> +			char *to_free = url;
> +			url = compute_submodule_clone_url(url);
> +			free(to_free);

Maybe:

			char *old_url = url;
			url = ...(old_url);
			free(old_url);

>  		}
>  
>  		if (git_config_set_gently(sb.buf, url))
> @@ -1562,8 +1571,13 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce,
>  
>  	strbuf_reset(&sb);
>  	strbuf_addf(&sb, "submodule.%s.url", sub->name);
> -	if (repo_config_get_string_const(the_repository, sb.buf, &url))
> -		url = sub->url;
> +	if (repo_config_get_string_const(the_repository, sb.buf, &url)) {
> +		if (starts_with_dot_slash(sub->url) ||
> +		    starts_with_dot_dot_slash(sub->url))
> +			url = compute_submodule_clone_url(sub->url);
> +		else
> +			url = sub->url;
> +	}

Nice.

>  
>  	strbuf_reset(&sb);
>  	strbuf_addf(&sb, "%s/.git", ce->name);
> diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
> index c0ffc1022a..3f5dd5e4ef 100755
> --- a/t/t7400-submodule-basic.sh
> +++ b/t/t7400-submodule-basic.sh
> @@ -1224,6 +1224,30 @@ test_expect_success 'submodule update and setting submodule.<name>.active' '
>  	test_cmp expect actual
>  '
>  
> +test_expect_success 'clone active submodule without submodule url set' '

Thanks for the test \o/.

> +	test_when_finished "rm -rf test/test" &&
> +	mkdir test &&
> +	# another dir breaks accidental relative paths still being correct
> +	git clone file://"$pwd"/multisuper test/test &&
> +	(
> +		cd test/test &&
> +		git config submodule.active "." &&
> +
> +		# do not pass --init flag, as it is already active:

What does "it" refer to here?

> +		git submodule update &&
> +		git submodule status >actual_raw &&
> +
> +		cut -c 1,43- actual_raw >actual &&
> +		cat >expect <<-\EOF &&
> +		 sub0 (test2)
> +		 sub1 (test2)
> +		 sub2 (test2)
> +		 sub3 (test2)
> +		EOF
> +		test_cmp expect actual
> +	)
> +'
> +
>  test_expect_success 'clone --recurse-submodules with a pathspec works' '
>  	test_when_finished "rm -rf multisuper_clone" &&
>  	cat >expected <<-\EOF &&

Thanks for the quick fix.

Jonathan