Message ID | 20181016001949.173333-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. > > 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> [...] > --- 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?) > +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, [...] > - 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.
Jonathan Nieder <jrnieder@gmail.com> writes: >> 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.
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 &&