From: Jonathan Nieder <jrnieder@gmail.com>
To: Stefan Beller <sbeller@google.com>
Cc: git@vger.kernel.org, Jaewoong Jung <jungjw@google.com>
Subject: Re: [PATCH] submodule helper: convert relative URL to absolute URL if needed
Date: Fri, 12 Oct 2018 15:27:12 -0700 [thread overview]
Message-ID: <20181012222712.GC52080@aiede.svl.corp.google.com> (raw)
In-Reply-To: <20181012215314.44266-1-sbeller@google.com>
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
next prev parent reply other threads:[~2018-10-12 22:27 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-10-12 21:53 [PATCH] submodule helper: convert relative URL to absolute URL if needed Stefan Beller
2018-10-12 22:27 ` Jonathan Nieder [this message]
2018-10-16 0:19 ` Stefan Beller
2018-10-16 0:33 ` Jonathan Nieder
2018-10-16 5:15 ` Junio C Hamano
2018-10-16 17:27 ` Stefan Beller
2018-10-16 21:05 ` Jonathan Nieder
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20181012222712.GC52080@aiede.svl.corp.google.com \
--to=jrnieder@gmail.com \
--cc=git@vger.kernel.org \
--cc=jungjw@google.com \
--cc=sbeller@google.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).