threads / patch / 49555

patchsubmodule helper: convert relative URL to absolute URL if needed

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

## tl;dr

7 messages between Oct 12, 2018 and Oct 16, 2018. Diffs are folded; open one to read it.

replies: 6people: 3as markdown or json

Stefan Beller· Oct 12, 2018, 21:53 UTC · lore

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.

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.

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(-)
Show changes to 2 files +55 −17

builtin/submodule--helper.c, t/t7400-submodule-basic.sh

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)
+{
+	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, 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;
+			char *to_free = url;
+			url = compute_submodule_clone_url(url);
+			free(to_free);
 		}
 
 		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;
+	}
 
 	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' '
+	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:
+		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 &&
-- 
2.19.0
Jonathan Nieder· Oct 12, 2018, 22:27 UTC · re: Stefan Beller · lore

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

Hi,
Stefan Beller wrote:
Show 8 quoted lines
> 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/
Show 5 quoted lines
> 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?
Show 10 quoted lines
> 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;
Show 6 quoted lines
> +
> +	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
Show 31 quoted lines
> +		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);
Show 16 quoted lines
>  		}
>  
>  		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.
Show 12 quoted lines
>  
>  	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/.
Show 9 quoted lines
> +	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?
Show 17 quoted lines
> +		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
Stefan Beller· Oct 16, 2018, 00:19 UTC · re: Stefan Beller · lore

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.

When trying to obtain submodules that are set active this way, we'll fallback to the URL found in the .gitmodules, which may be relative to the superproject, but we do not resolve it, yet:

    git clone https://gerrit.googlesource.com/gerrit
    cd gerrit && grep url .gitmodules
	url = ../plugins/codemirror-editor
	...
    git config submodule.active .
    git submodule update
fatal: repository '../plugins/codemirror-editor' does not exist
fatal: clone of '../plugins/codemirror-editor' into submodule path '/tmp/gerrit/plugins/codemirror-editor' failed
Failed to clone 'plugins/codemirror-editor'. Retry scheduled
[...]
fatal: clone of '../plugins/codemirror-editor' into submodule path '/tmp/gerrit/plugins/codemirror-editor' failed
Failed to clone 'plugins/codemirror-editor' a second time, aborting
[...]

To resolve the issue, factor out the function that resolves the relative URLs in "git submodule init" (in the submodule helper in the init_submodule function) and call it at the appropriate place in the update_clone helper.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
Jonathan wrote:
> Ah, this is moved code. I should have used --color-moved. ;-)
Yes, any cleanup should go on top.
I extended the commit message and made sure not to leak memory.

When rerolling origin/xxx/sb-submodule-recursive-fetch-gets-the-tip-in-pu, there will be conflicts with this series, but I can work with that.

 builtin/submodule--helper.c | 48 ++++++++++++++++++++++++-------------
 t/t7400-submodule-basic.sh  | 24 +++++++++++++++++++
 2 files changed, 55 insertions(+), 17 deletions(-)
Show changes to 2 files +55 −17

builtin/submodule--helper.c, t/t7400-submodule-basic.sh

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index f6fb8991f3..03f5e0d03e 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;
 }
 
+
+static char *compute_submodule_clone_url(const char *rel_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, 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;
+			char *to_free = url;
+			url = compute_submodule_clone_url(url);
+			free(to_free);
 		}
 
 		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;
+	}
 
 	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..76a7cb0af7 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' '
+	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 the submodule is already active:
+		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 &&
-- 
2.19.0
Jonathan Nieder· Oct 16, 2018, 00:33 UTC · re: Stefan Beller · lore

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

Hi,
Stefan Beller wrote:
Show 23 quoted lines
> 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.
>
> When trying to obtain submodules that are set active this way, we'll
> fallback to the URL found in the .gitmodules, which may be relative to the
> superproject, but we do not resolve it, yet:
> 
>     git clone https://gerrit.googlesource.com/gerrit
>     cd gerrit && grep url .gitmodules
> 	url = ../plugins/codemirror-editor
> 	...
>     git config submodule.active .
>     git submodule update
> fatal: repository '../plugins/codemirror-editor' does not exist
> fatal: clone of '../plugins/codemirror-editor' into submodule path '/tmp/gerrit/plugins/codemirror-editor' failed
> Failed to clone 'plugins/codemirror-editor'. Retry scheduled
> [...]
> fatal: clone of '../plugins/codemirror-editor' into submodule path '/tmp/gerrit/plugins/codemirror-editor' failed
> Failed to clone 'plugins/codemirror-editor' a second time, aborting
> [...]
Thanks.

"git log" and other tools have the ability to rewrap lines and will get confused by this transcript. Can you indent it to unconfuse them?

> Signed-off-by: Stefan Beller <sbeller@google.com>
Please also credit the bug reporter:
Reported-by: Jaewoong Jung <jungjw@google.com>
[...]
Show 7 quoted lines
> --- 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;
>  }
>  
> +
nit: inconsistent vertical whitespace (extra blank line?)
Show 19 quoted lines
> +static char *compute_submodule_clone_url(const char *rel_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, rel_url, NULL);
> +	strbuf_release(&remotesb);
> +	free(remoteurl);
> +
> +	return relurl;
> +}

I think this would be easier to read with all the release commands together:

	...
	free(remote);
	free(remoteurl);
	strbuf_release(&remotesb);
	return relurl;
[...]
> @@ -634,21 +655,9 @@ static void init_submodule(const char *path, const char *prefix,
[...]
Show 8 quoted lines
> -			relurl = relative_url(remoteurl, url, NULL);
> -			strbuf_release(&remotesb);
> -			free(remoteurl);
> -			free(url);
> -			url = relurl;
> +			char *to_free = url;
> +			url = compute_submodule_clone_url(url);
> +			free(to_free);

I still think this would be easier to read with a style that makes the ownership passing clearer:

			char *oldurl = url;
			url = compute_submodule_clone_url(oldurl);
			free(oldurl);
With whatever subset of the above tweaks makes sense,
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
Thanks.
Junio C Hamano· Oct 16, 2018, 05:15 UTC · re: Jonathan Nieder · lore

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

Jonathan Nieder <jrnieder@gmail.com> writes:
Show 27 quoted lines
>>     git config submodule.active .
>>     git submodule update
>> fatal: repository '../plugins/codemirror-editor' does not exist
>> fatal: clone of '../plugins/codemirror-editor' into submodule path '/tmp/gerrit/plugins/codemirror-editor' failed
>> Failed to clone 'plugins/codemirror-editor'. Retry scheduled
>> [...]
>> fatal: clone of '../plugins/codemirror-editor' into submodule path '/tmp/gerrit/plugins/codemirror-editor' failed
>> Failed to clone 'plugins/codemirror-editor' a second time, aborting
>> [...]
>
> Thanks.
>
> "git log" and other tools have the ability to rewrap lines and will get
> confused by this transcript.  Can you indent it to unconfuse them?
>
>> Signed-off-by: Stefan Beller <sbeller@google.com>
>
> Please also credit the bug reporter:
>
> Reported-by: Jaewoong Jung <jungjw@google.com>
>
> ...
> nit: inconsistent vertical whitespace (extra blank line?)
> ...
> I think this would be easier to read with all the release commands
> together:
> ...
All good points.
Stefan Beller· Oct 16, 2018, 17:27 UTC · re: Junio C Hamano · lore

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.

When trying to obtain submodules that are set active this way, we'll fallback to the URL found in the .gitmodules, which may be relative to the superproject, but we do not resolve it, yet:

    git clone https://gerrit.googlesource.com/gerrit
    cd gerrit && grep url .gitmodules
	url = ../plugins/codemirror-editor
	...
    git config submodule.active .
    git submodule update
    fatal: repository '../plugins/codemirror-editor' does not exist
    fatal: clone of '../plugins/codemirror-editor' into submodule path '/tmp/gerrit/plugins/codemirror-editor' failed
    Failed to clone 'plugins/codemirror-editor'. Retry scheduled
    [...]
    fatal: clone of '../plugins/codemirror-editor' into submodule path '/tmp/gerrit/plugins/codemirror-editor' failed
    Failed to clone 'plugins/codemirror-editor' a second time, aborting
    [...]

To resolve the issue, factor out the function that resolves the relative URLs in "git submodule init" (in the submodule helper in the init_submodule function) and call it at the appropriate place in the update_clone helper.

Reported-by: Jaewoong Jung <jungjw@google.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
---
 builtin/submodule--helper.c | 51 ++++++++++++++++++++++++-------------
 t/t7400-submodule-basic.sh  | 24 +++++++++++++++++
 2 files changed, 58 insertions(+), 17 deletions(-)
Show changes to 2 files +58 −17

builtin/submodule--helper.c, t/t7400-submodule-basic.sh

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index f6fb8991f3..13c2e4b556 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -584,6 +584,26 @@ static int module_foreach(int argc, const char **argv, const char *prefix)
 	return 0;
 }
 
+static char *compute_submodule_clone_url(const char *rel_url)
+{
+	char *remoteurl, *relurl;
+	char *remote = get_default_remote();
+	struct strbuf remotesb = STRBUF_INIT;
+
+	strbuf_addf(&remotesb, "remote.%s.url", remote);
+	if (git_config_get_string(remotesb.buf, &remoteurl)) {
+		warning(_("could not look up configuration '%s'. Assuming this repository is its own authoritative upstream."), remotesb.buf);
+		remoteurl = xgetcwd();
+	}
+	relurl = relative_url(remoteurl, rel_url, NULL);
+
+	free(remote);
+	free(remoteurl);
+	strbuf_release(&remotesb);
+
+	return relurl;
+}
+
 struct init_cb {
 	const char *prefix;
 	unsigned int flags;
@@ -634,21 +654,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;
+			char *oldurl = url;
+			url = compute_submodule_clone_url(oldurl);
+			free(oldurl);
 		}
 
 		if (git_config_set_gently(sb.buf, url))
@@ -1514,6 +1522,7 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce,
 	struct strbuf sb = STRBUF_INIT;
 	const char *displaypath = NULL;
 	int needs_cloning = 0;
+	int need_free_url = 0;
 
 	if (ce_stage(ce)) {
 		if (suc->recursive_prefix)
@@ -1562,8 +1571,14 @@ 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);
+			need_free_url = 1;
+		} else
+			url = sub->url;
+	}
 
 	strbuf_reset(&sb);
 	strbuf_addf(&sb, "%s/.git", ce->name);
@@ -1608,6 +1623,8 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce,
 cleanup:
 	strbuf_reset(&displaypath_sb);
 	strbuf_reset(&sb);
+	if (need_free_url)
+		free((void*)url);
 
 	return needs_cloning;
 }
diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
index c0ffc1022a..76a7cb0af7 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' '
+	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 the submodule is already active:
+		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 &&
-- 
2.19.0
Jonathan Nieder· Oct 16, 2018, 21:05 UTC · re: Stefan Beller · lore

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

Stefan Beller wrote:
Show 6 quoted lines
> Reported-by: Jaewoong Jung <jungjw@google.com>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>  builtin/submodule--helper.c | 51 ++++++++++++++++++++++++-------------
>  t/t7400-submodule-basic.sh  | 24 +++++++++++++++++
>  2 files changed, 58 insertions(+), 17 deletions(-)
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
Thanks for your patient work.

← back to recent threads