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 <sbel...@google.com> Please also credit the bug reporter: Reported-by: Jaewoong Jung <jun...@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 <jrnie...@gmail.com> Thanks.
