Re: [PATCH] submodule helper: convert relative URL to absolute URL if needed
- From
Jonathan Nieder <jrnieder@gmail.com>
- Date
- Oct 16, 2018, 00:33 UTC
- Message-ID
- <20181016003324.GA104911@aiede.svl.corp.google.com>
- In-Reply-To
- <20181016001949.173333-1-sbeller@google.com>
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.