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@xxxxxxxxxx>
> ---
> 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
