| Message ID | 20181012215314.44266-1-sbeller@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | submodule helper: convert relative URL to absolute URL if needed | expand |
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
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 &&
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(-)