{"thread":{"id":"49555","subject":"[PATCH] submodule helper: convert relative URL to absolute URL if needed","startedAt":"2018-10-12T21:53:20Z","lastAt":"2018-10-16T21:05:43Z","messageCount":7,"participants":["Stefan Beller","Jonathan Nieder","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"360356","messageId":"20181012215314.44266-1-sbeller@google.com","threadId":"49555","inReplyTo":null,"subject":"[PATCH] submodule helper: convert relative URL to absolute URL if needed","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-10-12T21:53:14Z","receivedAt":"2018-10-12T21:53:20Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"The submodule helper update_clone called by \"git submodule update\",\nclones submodules if needed. As submodules used to have the URL indicating\nif they were active, the step to resolve relative URLs was done in the\n\"submodule init\" step. Nowadays submodules can be configured active without\ncalling an explicit init, e.g. via configuring submodule.active.\n\nThen we'll fallback to the URL found in the .gitmodules, which may be\nrelative to the superproject, but we do not resolve it, yet.\n\nTo do so, factor out the function that resolves the relative URLs in\n\"git submodule init\" (in the submodule helper in the init_submodule\nfunction) and cal it at the appropriate place in the update_clone helper.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n builtin/submodule--helper.c | 48 ++++++++++++++++++++++++-------------\n t/t7400-submodule-basic.sh  | 24 +++++++++++++++++++\n 2 files changed, 55 insertions(+), 17 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex f6fb8991f3..a9a3ac38be 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -584,6 +584,27 @@ static int module_foreach(int argc, const char **argv, const char *prefix)\n \treturn 0;\n }\n \n+\n+char *compute_submodule_clone_url(const char *rel_url)\n+{\n+\tchar *remoteurl, *relurl;\n+\tchar *remote = get_default_remote();\n+\tstruct strbuf remotesb = STRBUF_INIT;\n+\n+\tstrbuf_addf(&remotesb, \"remote.%s.url\", remote);\n+\tfree(remote);\n+\n+\tif (git_config_get_string(remotesb.buf, &remoteurl)) {\n+\t\twarning(_(\"could not lookup configuration '%s'. Assuming this repository is its own authoritative upstream.\"), remotesb.buf);\n+\t\tremoteurl = xgetcwd();\n+\t}\n+\trelurl = relative_url(remoteurl, rel_url, NULL);\n+\tstrbuf_release(&remotesb);\n+\tfree(remoteurl);\n+\n+\treturn relurl;\n+}\n+\n struct init_cb {\n \tconst char *prefix;\n \tunsigned int flags;\n@@ -634,21 +655,9 @@ static void init_submodule(const char *path, const char *prefix,\n \t\t/* Possibly a url relative to parent */\n \t\tif (starts_with_dot_dot_slash(url) ||\n \t\t    starts_with_dot_slash(url)) {\n-\t\t\tchar *remoteurl, *relurl;\n-\t\t\tchar *remote = get_default_remote();\n-\t\t\tstruct strbuf remotesb = STRBUF_INIT;\n-\t\t\tstrbuf_addf(&remotesb, \"remote.%s.url\", remote);\n-\t\t\tfree(remote);\n-\n-\t\t\tif (git_config_get_string(remotesb.buf, &remoteurl)) {\n-\t\t\t\twarning(_(\"could not lookup configuration '%s'. Assuming this repository is its own authoritative upstream.\"), remotesb.buf);\n-\t\t\t\tremoteurl = xgetcwd();\n-\t\t\t}\n-\t\t\trelurl = relative_url(remoteurl, url, NULL);\n-\t\t\tstrbuf_release(&remotesb);\n-\t\t\tfree(remoteurl);\n-\t\t\tfree(url);\n-\t\t\turl = relurl;\n+\t\t\tchar *to_free = url;\n+\t\t\turl = compute_submodule_clone_url(url);\n+\t\t\tfree(to_free);\n \t\t}\n \n \t\tif (git_config_set_gently(sb.buf, url))\n@@ -1562,8 +1571,13 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce,\n \n \tstrbuf_reset(&sb);\n \tstrbuf_addf(&sb, \"submodule.%s.url\", sub->name);\n-\tif (repo_config_get_string_const(the_repository, sb.buf, &url))\n-\t\turl = sub->url;\n+\tif (repo_config_get_string_const(the_repository, sb.buf, &url)) {\n+\t\tif (starts_with_dot_slash(sub->url) ||\n+\t\t    starts_with_dot_dot_slash(sub->url))\n+\t\t\turl = compute_submodule_clone_url(sub->url);\n+\t\telse\n+\t\t\turl = sub->url;\n+\t}\n \n \tstrbuf_reset(&sb);\n \tstrbuf_addf(&sb, \"%s/.git\", ce->name);\ndiff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh\nindex c0ffc1022a..3f5dd5e4ef 100755\n--- a/t/t7400-submodule-basic.sh\n+++ b/t/t7400-submodule-basic.sh\n@@ -1224,6 +1224,30 @@ test_expect_success 'submodule update and setting submodule.<name>.active' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'clone active submodule without submodule url set' '\n+\ttest_when_finished \"rm -rf test/test\" &&\n+\tmkdir test &&\n+\t# another dir breaks accidental relative paths still being correct\n+\tgit clone file://\"$pwd\"/multisuper test/test &&\n+\t(\n+\t\tcd test/test &&\n+\t\tgit config submodule.active \".\" &&\n+\n+\t\t# do not pass --init flag, as it is already active:\n+\t\tgit submodule update &&\n+\t\tgit submodule status >actual_raw &&\n+\n+\t\tcut -c 1,43- actual_raw >actual &&\n+\t\tcat >expect <<-\\EOF &&\n+\t\t sub0 (test2)\n+\t\t sub1 (test2)\n+\t\t sub2 (test2)\n+\t\t sub3 (test2)\n+\t\tEOF\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n test_expect_success 'clone --recurse-submodules with a pathspec works' '\n \ttest_when_finished \"rm -rf multisuper_clone\" &&\n \tcat >expected <<-\\EOF &&\n-- \n2.19.0\n\n"},{"id":"360358","messageId":"20181012222712.GC52080@aiede.svl.corp.google.com","threadId":"49555","inReplyTo":"20181012215314.44266-1-sbeller@google.com","subject":"Re: [PATCH] submodule helper: convert relative URL to absolute URL if needed","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2018-10-12T22:27:12Z","receivedAt":"2018-10-12T22:27:18Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nStefan Beller wrote:\n\n> The submodule helper update_clone called by \"git submodule update\",\n> clones submodules if needed. As submodules used to have the URL indicating\n> if they were active, the step to resolve relative URLs was done in the\n> \"submodule init\" step. Nowadays submodules can be configured active without\n> calling an explicit init, e.g. via configuring submodule.active.\n>\n> Then we'll fallback to the URL found in the .gitmodules, which may be\n> relative to the superproject, but we do not resolve it, yet.\n\nOh!  Good catch.\n\n> To do so, factor out the function that resolves the relative URLs in\n> \"git submodule init\" (in the submodule helper in the init_submodule\n> function) and cal it at the appropriate place in the update_clone helper.\n\ns/cal/call/\n\n> Signed-off-by: Stefan Beller <sbeller@google.com>\n> ---\n>  builtin/submodule--helper.c | 48 ++++++++++++++++++++++++-------------\n>  t/t7400-submodule-basic.sh  | 24 +++++++++++++++++++\n>  2 files changed, 55 insertions(+), 17 deletions(-)\n\nWhat is the symptom when this fails?\n\n> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\n> index f6fb8991f3..a9a3ac38be 100644\n> --- a/builtin/submodule--helper.c\n> +++ b/builtin/submodule--helper.c\n> @@ -584,6 +584,27 @@ static int module_foreach(int argc, const char **argv, const char *prefix)\n>  \treturn 0;\n>  }\n>  \n> +\n> +char *compute_submodule_clone_url(const char *rel_url)\n\nShould be static.\n\nIs the caller responsible for freeing the returned buffer?\n\n> +{\n> +\tchar *remoteurl, *relurl;\n> +\tchar *remote = get_default_remote();\n> +\tstruct strbuf remotesb = STRBUF_INIT;\n\noptional: could rename, something like\n\n\tstruct strbuf key = STRBUF_INIT;\n\n\tremote = get_default_remote();\n\tstrbuf_addf(&key, \"remote.%s.url\", remote);\n\n [...]\n\tstrbuf_release(&key);\n\tfree(remote);\n\treturn result;\n\n\n> +\n> +\tstrbuf_addf(&remotesb, \"remote.%s.url\", remote);\n> +\tfree(remote);\n> +\n> +\tif (git_config_get_string(remotesb.buf, &remoteurl)) {\n> +\t\twarning(_(\"could not lookup configuration '%s'. Assuming this repository is its own authoritative upstream.\"), remotesb.buf);\n\nnit: lookup is the noun, \"look up\" is the verb\n\n> +\t\tremoteurl = xgetcwd();\n> +\t}\n> +\trelurl = relative_url(remoteurl, rel_url, NULL);\n> +\tstrbuf_release(&remotesb);\n> +\tfree(remoteurl);\n> +\n> +\treturn relurl;\n> +}\n> +\n>  struct init_cb {\n>  \tconst char *prefix;\n>  \tunsigned int flags;\n> @@ -634,21 +655,9 @@ static void init_submodule(const char *path, const char *prefix,\n>  \t\t/* Possibly a url relative to parent */\n>  \t\tif (starts_with_dot_dot_slash(url) ||\n>  \t\t    starts_with_dot_slash(url)) {\n> -\t\t\tchar *remoteurl, *relurl;\n> -\t\t\tchar *remote = get_default_remote();\n> -\t\t\tstruct strbuf remotesb = STRBUF_INIT;\n> -\t\t\tstrbuf_addf(&remotesb, \"remote.%s.url\", remote);\n> -\t\t\tfree(remote);\n> -\n> -\t\t\tif (git_config_get_string(remotesb.buf, &remoteurl)) {\n> -\t\t\t\twarning(_(\"could not lookup configuration '%s'. Assuming this repository is its own authoritative upstream.\"), remotesb.buf);\n> -\t\t\t\tremoteurl = xgetcwd();\n> -\t\t\t}\n> -\t\t\trelurl = relative_url(remoteurl, url, NULL);\n> -\t\t\tstrbuf_release(&remotesb);\n> -\t\t\tfree(remoteurl);\n> -\t\t\tfree(url);\n> -\t\t\turl = relurl;\n\nAh, this is moved code. I should have used --color-moved. ;-)\n\n> +\t\t\tchar *to_free = url;\n> +\t\t\turl = compute_submodule_clone_url(url);\n> +\t\t\tfree(to_free);\n\nMaybe:\n\n\t\t\tchar *old_url = url;\n\t\t\turl = ...(old_url);\n\t\t\tfree(old_url);\n\n>  \t\t}\n>  \n>  \t\tif (git_config_set_gently(sb.buf, url))\n> @@ -1562,8 +1571,13 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce,\n>  \n>  \tstrbuf_reset(&sb);\n>  \tstrbuf_addf(&sb, \"submodule.%s.url\", sub->name);\n> -\tif (repo_config_get_string_const(the_repository, sb.buf, &url))\n> -\t\turl = sub->url;\n> +\tif (repo_config_get_string_const(the_repository, sb.buf, &url)) {\n> +\t\tif (starts_with_dot_slash(sub->url) ||\n> +\t\t    starts_with_dot_dot_slash(sub->url))\n> +\t\t\turl = compute_submodule_clone_url(sub->url);\n> +\t\telse\n> +\t\t\turl = sub->url;\n> +\t}\n\nNice.\n\n>  \n>  \tstrbuf_reset(&sb);\n>  \tstrbuf_addf(&sb, \"%s/.git\", ce->name);\n> diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh\n> index c0ffc1022a..3f5dd5e4ef 100755\n> --- a/t/t7400-submodule-basic.sh\n> +++ b/t/t7400-submodule-basic.sh\n> @@ -1224,6 +1224,30 @@ test_expect_success 'submodule update and setting submodule.<name>.active' '\n>  \ttest_cmp expect actual\n>  '\n>  \n> +test_expect_success 'clone active submodule without submodule url set' '\n\nThanks for the test \\o/.\n\n> +\ttest_when_finished \"rm -rf test/test\" &&\n> +\tmkdir test &&\n> +\t# another dir breaks accidental relative paths still being correct\n> +\tgit clone file://\"$pwd\"/multisuper test/test &&\n> +\t(\n> +\t\tcd test/test &&\n> +\t\tgit config submodule.active \".\" &&\n> +\n> +\t\t# do not pass --init flag, as it is already active:\n\nWhat does \"it\" refer to here?\n\n> +\t\tgit submodule update &&\n> +\t\tgit submodule status >actual_raw &&\n> +\n> +\t\tcut -c 1,43- actual_raw >actual &&\n> +\t\tcat >expect <<-\\EOF &&\n> +\t\t sub0 (test2)\n> +\t\t sub1 (test2)\n> +\t\t sub2 (test2)\n> +\t\t sub3 (test2)\n> +\t\tEOF\n> +\t\ttest_cmp expect actual\n> +\t)\n> +'\n> +\n>  test_expect_success 'clone --recurse-submodules with a pathspec works' '\n>  \ttest_when_finished \"rm -rf multisuper_clone\" &&\n>  \tcat >expected <<-\\EOF &&\n\nThanks for the quick fix.\n\nJonathan\n"},{"id":"360560","messageId":"20181016001949.173333-1-sbeller@google.com","threadId":"49555","inReplyTo":"20181012215314.44266-1-sbeller@google.com","subject":"[PATCH] submodule helper: convert relative URL to absolute URL if needed","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-10-16T00:19:49Z","receivedAt":"2018-10-16T00:19:55Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"The submodule helper update_clone called by \"git submodule update\",\nclones submodules if needed. As submodules used to have the URL indicating\nif they were active, the step to resolve relative URLs was done in the\n\"submodule init\" step. Nowadays submodules can be configured active without\ncalling an explicit init, e.g. via configuring submodule.active.\n\nWhen trying to obtain submodules that are set active this way, we'll\nfallback to the URL found in the .gitmodules, which may be relative to the\nsuperproject, but we do not resolve it, yet:\n\n    git clone https://gerrit.googlesource.com/gerrit\n    cd gerrit && grep url .gitmodules\n\turl = ../plugins/codemirror-editor\n\t...\n    git config submodule.active .\n    git submodule update\nfatal: repository '../plugins/codemirror-editor' does not exist\nfatal: clone of '../plugins/codemirror-editor' into submodule path '/tmp/gerrit/plugins/codemirror-editor' failed\nFailed to clone 'plugins/codemirror-editor'. Retry scheduled\n[...]\nfatal: clone of '../plugins/codemirror-editor' into submodule path '/tmp/gerrit/plugins/codemirror-editor' failed\nFailed to clone 'plugins/codemirror-editor' a second time, aborting\n[...]\n\nTo resolve the issue, factor out the function that resolves the relative\nURLs in \"git submodule init\" (in the submodule helper in the init_submodule\nfunction) and call it at the appropriate place in the update_clone helper.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n\nJonathan wrote:\n> Ah, this is moved code. I should have used --color-moved. ;-)\n\nYes, any cleanup should go on top.\n\nI extended the commit message and made sure not to leak memory.\n\nWhen rerolling origin/xxx/sb-submodule-recursive-fetch-gets-the-tip-in-pu,\nthere will be conflicts with this series, but I can work with that.\n\n builtin/submodule--helper.c | 48 ++++++++++++++++++++++++-------------\n t/t7400-submodule-basic.sh  | 24 +++++++++++++++++++\n 2 files changed, 55 insertions(+), 17 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex f6fb8991f3..03f5e0d03e 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -584,6 +584,27 @@ static int module_foreach(int argc, const char **argv, const char *prefix)\n \treturn 0;\n }\n \n+\n+static char *compute_submodule_clone_url(const char *rel_url)\n+{\n+\tchar *remoteurl, *relurl;\n+\tchar *remote = get_default_remote();\n+\tstruct strbuf remotesb = STRBUF_INIT;\n+\n+\tstrbuf_addf(&remotesb, \"remote.%s.url\", remote);\n+\tfree(remote);\n+\n+\tif (git_config_get_string(remotesb.buf, &remoteurl)) {\n+\t\twarning(_(\"could not lookup configuration '%s'. Assuming this repository is its own authoritative upstream.\"), remotesb.buf);\n+\t\tremoteurl = xgetcwd();\n+\t}\n+\trelurl = relative_url(remoteurl, rel_url, NULL);\n+\tstrbuf_release(&remotesb);\n+\tfree(remoteurl);\n+\n+\treturn relurl;\n+}\n+\n struct init_cb {\n \tconst char *prefix;\n \tunsigned int flags;\n@@ -634,21 +655,9 @@ static void init_submodule(const char *path, const char *prefix,\n \t\t/* Possibly a url relative to parent */\n \t\tif (starts_with_dot_dot_slash(url) ||\n \t\t    starts_with_dot_slash(url)) {\n-\t\t\tchar *remoteurl, *relurl;\n-\t\t\tchar *remote = get_default_remote();\n-\t\t\tstruct strbuf remotesb = STRBUF_INIT;\n-\t\t\tstrbuf_addf(&remotesb, \"remote.%s.url\", remote);\n-\t\t\tfree(remote);\n-\n-\t\t\tif (git_config_get_string(remotesb.buf, &remoteurl)) {\n-\t\t\t\twarning(_(\"could not lookup configuration '%s'. Assuming this repository is its own authoritative upstream.\"), remotesb.buf);\n-\t\t\t\tremoteurl = xgetcwd();\n-\t\t\t}\n-\t\t\trelurl = relative_url(remoteurl, url, NULL);\n-\t\t\tstrbuf_release(&remotesb);\n-\t\t\tfree(remoteurl);\n-\t\t\tfree(url);\n-\t\t\turl = relurl;\n+\t\t\tchar *to_free = url;\n+\t\t\turl = compute_submodule_clone_url(url);\n+\t\t\tfree(to_free);\n \t\t}\n \n \t\tif (git_config_set_gently(sb.buf, url))\n@@ -1562,8 +1571,13 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce,\n \n \tstrbuf_reset(&sb);\n \tstrbuf_addf(&sb, \"submodule.%s.url\", sub->name);\n-\tif (repo_config_get_string_const(the_repository, sb.buf, &url))\n-\t\turl = sub->url;\n+\tif (repo_config_get_string_const(the_repository, sb.buf, &url)) {\n+\t\tif (starts_with_dot_slash(sub->url) ||\n+\t\t    starts_with_dot_dot_slash(sub->url))\n+\t\t\turl = compute_submodule_clone_url(sub->url);\n+\t\telse\n+\t\t\turl = sub->url;\n+\t}\n \n \tstrbuf_reset(&sb);\n \tstrbuf_addf(&sb, \"%s/.git\", ce->name);\ndiff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh\nindex c0ffc1022a..76a7cb0af7 100755\n--- a/t/t7400-submodule-basic.sh\n+++ b/t/t7400-submodule-basic.sh\n@@ -1224,6 +1224,30 @@ test_expect_success 'submodule update and setting submodule.<name>.active' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'clone active submodule without submodule url set' '\n+\ttest_when_finished \"rm -rf test/test\" &&\n+\tmkdir test &&\n+\t# another dir breaks accidental relative paths still being correct\n+\tgit clone file://\"$pwd\"/multisuper test/test &&\n+\t(\n+\t\tcd test/test &&\n+\t\tgit config submodule.active \".\" &&\n+\n+\t\t# do not pass --init flag, as the submodule is already active:\n+\t\tgit submodule update &&\n+\t\tgit submodule status >actual_raw &&\n+\n+\t\tcut -c 1,43- actual_raw >actual &&\n+\t\tcat >expect <<-\\EOF &&\n+\t\t sub0 (test2)\n+\t\t sub1 (test2)\n+\t\t sub2 (test2)\n+\t\t sub3 (test2)\n+\t\tEOF\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n test_expect_success 'clone --recurse-submodules with a pathspec works' '\n \ttest_when_finished \"rm -rf multisuper_clone\" &&\n \tcat >expected <<-\\EOF &&\n-- \n2.19.0\n\n"},{"id":"360562","messageId":"20181016003324.GA104911@aiede.svl.corp.google.com","threadId":"49555","inReplyTo":"20181016001949.173333-1-sbeller@google.com","subject":"Re: [PATCH] submodule helper: convert relative URL to absolute URL if needed","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2018-10-16T00:33:24Z","receivedAt":"2018-10-16T00:33:30Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nStefan Beller wrote:\n\n> The submodule helper update_clone called by \"git submodule update\",\n> clones submodules if needed. As submodules used to have the URL indicating\n> if they were active, the step to resolve relative URLs was done in the\n> \"submodule init\" step. Nowadays submodules can be configured active without\n> calling an explicit init, e.g. via configuring submodule.active.\n>\n> When trying to obtain submodules that are set active this way, we'll\n> fallback to the URL found in the .gitmodules, which may be relative to the\n> superproject, but we do not resolve it, yet:\n> \n>     git clone https://gerrit.googlesource.com/gerrit\n>     cd gerrit && grep url .gitmodules\n> \turl = ../plugins/codemirror-editor\n> \t...\n>     git config submodule.active .\n>     git submodule update\n> fatal: repository '../plugins/codemirror-editor' does not exist\n> fatal: clone of '../plugins/codemirror-editor' into submodule path '/tmp/gerrit/plugins/codemirror-editor' failed\n> Failed to clone 'plugins/codemirror-editor'. Retry scheduled\n> [...]\n> fatal: clone of '../plugins/codemirror-editor' into submodule path '/tmp/gerrit/plugins/codemirror-editor' failed\n> Failed to clone 'plugins/codemirror-editor' a second time, aborting\n> [...]\n\nThanks.\n\n\"git log\" and other tools have the ability to rewrap lines and will get\nconfused by this transcript.  Can you indent it to unconfuse them?\n\n> Signed-off-by: Stefan Beller <sbeller@google.com>\n\nPlease also credit the bug reporter:\n\nReported-by: Jaewoong Jung <jungjw@google.com>\n\n[...]\n> --- a/builtin/submodule--helper.c\n> +++ b/builtin/submodule--helper.c\n> @@ -584,6 +584,27 @@ static int module_foreach(int argc, const char **argv, const char *prefix)\n>  \treturn 0;\n>  }\n>  \n> +\n\nnit: inconsistent vertical whitespace (extra blank line?)\n\n> +static char *compute_submodule_clone_url(const char *rel_url)\n> +{\n> +\tchar *remoteurl, *relurl;\n> +\tchar *remote = get_default_remote();\n> +\tstruct strbuf remotesb = STRBUF_INIT;\n> +\n> +\tstrbuf_addf(&remotesb, \"remote.%s.url\", remote);\n> +\tfree(remote);\n> +\n> +\tif (git_config_get_string(remotesb.buf, &remoteurl)) {\n> +\t\twarning(_(\"could not lookup configuration '%s'. Assuming this repository is its own authoritative upstream.\"), remotesb.buf);\n> +\t\tremoteurl = xgetcwd();\n> +\t}\n> +\trelurl = relative_url(remoteurl, rel_url, NULL);\n> +\tstrbuf_release(&remotesb);\n> +\tfree(remoteurl);\n> +\n> +\treturn relurl;\n> +}\n\nI think this would be easier to read with all the release commands\ntogether:\n\n\t...\n\n\tfree(remote);\n\tfree(remoteurl);\n\tstrbuf_release(&remotesb);\n\treturn relurl;\n\n[...]\n> @@ -634,21 +655,9 @@ static void init_submodule(const char *path, const char *prefix,\n[...]\n> -\t\t\trelurl = relative_url(remoteurl, url, NULL);\n> -\t\t\tstrbuf_release(&remotesb);\n> -\t\t\tfree(remoteurl);\n> -\t\t\tfree(url);\n> -\t\t\turl = relurl;\n> +\t\t\tchar *to_free = url;\n> +\t\t\turl = compute_submodule_clone_url(url);\n> +\t\t\tfree(to_free);\n\nI still think this would be easier to read with a style that makes\nthe ownership passing clearer:\n\n\t\t\tchar *oldurl = url;\n\t\t\turl = compute_submodule_clone_url(oldurl);\n\t\t\tfree(oldurl);\n\nWith whatever subset of the above tweaks makes sense,\nReviewed-by: Jonathan Nieder <jrnieder@gmail.com>\n\nThanks.\n"},{"id":"360581","messageId":"xmqq4ldm1nh6.fsf@gitster-ct.c.googlers.com","threadId":"49555","inReplyTo":"20181016003324.GA104911@aiede.svl.corp.google.com","subject":"Re: [PATCH] submodule helper: convert relative URL to absolute URL if needed","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-10-16T05:15:17Z","receivedAt":"2018-10-16T05:15:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder <jrnieder@gmail.com> writes:\n\n>>     git config submodule.active .\n>>     git submodule update\n>> fatal: repository '../plugins/codemirror-editor' does not exist\n>> fatal: clone of '../plugins/codemirror-editor' into submodule path '/tmp/gerrit/plugins/codemirror-editor' failed\n>> Failed to clone 'plugins/codemirror-editor'. Retry scheduled\n>> [...]\n>> fatal: clone of '../plugins/codemirror-editor' into submodule path '/tmp/gerrit/plugins/codemirror-editor' failed\n>> Failed to clone 'plugins/codemirror-editor' a second time, aborting\n>> [...]\n>\n> Thanks.\n>\n> \"git log\" and other tools have the ability to rewrap lines and will get\n> confused by this transcript.  Can you indent it to unconfuse them?\n>\n>> Signed-off-by: Stefan Beller <sbeller@google.com>\n>\n> Please also credit the bug reporter:\n>\n> Reported-by: Jaewoong Jung <jungjw@google.com>\n>\n> ...\n> nit: inconsistent vertical whitespace (extra blank line?)\n> ...\n> I think this would be easier to read with all the release commands\n> together:\n> ...\n\nAll good points.\n"},{"id":"360643","messageId":"20181016172703.134901-1-sbeller@google.com","threadId":"49555","inReplyTo":"xmqq4ldm1nh6.fsf@gitster-ct.c.googlers.com","subject":"[PATCH] submodule helper: convert relative URL to absolute URL if needed","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-10-16T17:27:03Z","receivedAt":"2018-10-16T17:27:10Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"The submodule helper update_clone called by \"git submodule update\",\nclones submodules if needed. As submodules used to have the URL indicating\nif they were active, the step to resolve relative URLs was done in the\n\"submodule init\" step. Nowadays submodules can be configured active without\ncalling an explicit init, e.g. via configuring submodule.active.\n\nWhen trying to obtain submodules that are set active this way, we'll\nfallback to the URL found in the .gitmodules, which may be relative to the\nsuperproject, but we do not resolve it, yet:\n\n    git clone https://gerrit.googlesource.com/gerrit\n    cd gerrit && grep url .gitmodules\n\turl = ../plugins/codemirror-editor\n\t...\n    git config submodule.active .\n    git submodule update\n    fatal: repository '../plugins/codemirror-editor' does not exist\n    fatal: clone of '../plugins/codemirror-editor' into submodule path '/tmp/gerrit/plugins/codemirror-editor' failed\n    Failed to clone 'plugins/codemirror-editor'. Retry scheduled\n    [...]\n    fatal: clone of '../plugins/codemirror-editor' into submodule path '/tmp/gerrit/plugins/codemirror-editor' failed\n    Failed to clone 'plugins/codemirror-editor' a second time, aborting\n    [...]\n\nTo resolve the issue, factor out the function that resolves the relative\nURLs in \"git submodule init\" (in the submodule helper in the init_submodule\nfunction) and call it at the appropriate place in the update_clone helper.\n\nReported-by: Jaewoong Jung <jungjw@google.com>\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n builtin/submodule--helper.c | 51 ++++++++++++++++++++++++-------------\n t/t7400-submodule-basic.sh  | 24 +++++++++++++++++\n 2 files changed, 58 insertions(+), 17 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex f6fb8991f3..13c2e4b556 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -584,6 +584,26 @@ static int module_foreach(int argc, const char **argv, const char *prefix)\n \treturn 0;\n }\n \n+static char *compute_submodule_clone_url(const char *rel_url)\n+{\n+\tchar *remoteurl, *relurl;\n+\tchar *remote = get_default_remote();\n+\tstruct strbuf remotesb = STRBUF_INIT;\n+\n+\tstrbuf_addf(&remotesb, \"remote.%s.url\", remote);\n+\tif (git_config_get_string(remotesb.buf, &remoteurl)) {\n+\t\twarning(_(\"could not look up configuration '%s'. Assuming this repository is its own authoritative upstream.\"), remotesb.buf);\n+\t\tremoteurl = xgetcwd();\n+\t}\n+\trelurl = relative_url(remoteurl, rel_url, NULL);\n+\n+\tfree(remote);\n+\tfree(remoteurl);\n+\tstrbuf_release(&remotesb);\n+\n+\treturn relurl;\n+}\n+\n struct init_cb {\n \tconst char *prefix;\n \tunsigned int flags;\n@@ -634,21 +654,9 @@ static void init_submodule(const char *path, const char *prefix,\n \t\t/* Possibly a url relative to parent */\n \t\tif (starts_with_dot_dot_slash(url) ||\n \t\t    starts_with_dot_slash(url)) {\n-\t\t\tchar *remoteurl, *relurl;\n-\t\t\tchar *remote = get_default_remote();\n-\t\t\tstruct strbuf remotesb = STRBUF_INIT;\n-\t\t\tstrbuf_addf(&remotesb, \"remote.%s.url\", remote);\n-\t\t\tfree(remote);\n-\n-\t\t\tif (git_config_get_string(remotesb.buf, &remoteurl)) {\n-\t\t\t\twarning(_(\"could not lookup configuration '%s'. Assuming this repository is its own authoritative upstream.\"), remotesb.buf);\n-\t\t\t\tremoteurl = xgetcwd();\n-\t\t\t}\n-\t\t\trelurl = relative_url(remoteurl, url, NULL);\n-\t\t\tstrbuf_release(&remotesb);\n-\t\t\tfree(remoteurl);\n-\t\t\tfree(url);\n-\t\t\turl = relurl;\n+\t\t\tchar *oldurl = url;\n+\t\t\turl = compute_submodule_clone_url(oldurl);\n+\t\t\tfree(oldurl);\n \t\t}\n \n \t\tif (git_config_set_gently(sb.buf, url))\n@@ -1514,6 +1522,7 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce,\n \tstruct strbuf sb = STRBUF_INIT;\n \tconst char *displaypath = NULL;\n \tint needs_cloning = 0;\n+\tint need_free_url = 0;\n \n \tif (ce_stage(ce)) {\n \t\tif (suc->recursive_prefix)\n@@ -1562,8 +1571,14 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce,\n \n \tstrbuf_reset(&sb);\n \tstrbuf_addf(&sb, \"submodule.%s.url\", sub->name);\n-\tif (repo_config_get_string_const(the_repository, sb.buf, &url))\n-\t\turl = sub->url;\n+\tif (repo_config_get_string_const(the_repository, sb.buf, &url)) {\n+\t\tif (starts_with_dot_slash(sub->url) ||\n+\t\t    starts_with_dot_dot_slash(sub->url)) {\n+\t\t\turl = compute_submodule_clone_url(sub->url);\n+\t\t\tneed_free_url = 1;\n+\t\t} else\n+\t\t\turl = sub->url;\n+\t}\n \n \tstrbuf_reset(&sb);\n \tstrbuf_addf(&sb, \"%s/.git\", ce->name);\n@@ -1608,6 +1623,8 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce,\n cleanup:\n \tstrbuf_reset(&displaypath_sb);\n \tstrbuf_reset(&sb);\n+\tif (need_free_url)\n+\t\tfree((void*)url);\n \n \treturn needs_cloning;\n }\ndiff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh\nindex c0ffc1022a..76a7cb0af7 100755\n--- a/t/t7400-submodule-basic.sh\n+++ b/t/t7400-submodule-basic.sh\n@@ -1224,6 +1224,30 @@ test_expect_success 'submodule update and setting submodule.<name>.active' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'clone active submodule without submodule url set' '\n+\ttest_when_finished \"rm -rf test/test\" &&\n+\tmkdir test &&\n+\t# another dir breaks accidental relative paths still being correct\n+\tgit clone file://\"$pwd\"/multisuper test/test &&\n+\t(\n+\t\tcd test/test &&\n+\t\tgit config submodule.active \".\" &&\n+\n+\t\t# do not pass --init flag, as the submodule is already active:\n+\t\tgit submodule update &&\n+\t\tgit submodule status >actual_raw &&\n+\n+\t\tcut -c 1,43- actual_raw >actual &&\n+\t\tcat >expect <<-\\EOF &&\n+\t\t sub0 (test2)\n+\t\t sub1 (test2)\n+\t\t sub2 (test2)\n+\t\t sub3 (test2)\n+\t\tEOF\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n test_expect_success 'clone --recurse-submodules with a pathspec works' '\n \ttest_when_finished \"rm -rf multisuper_clone\" &&\n \tcat >expected <<-\\EOF &&\n-- \n2.19.0\n\n"},{"id":"360670","messageId":"20181016210538.GA96853@aiede.svl.corp.google.com","threadId":"49555","inReplyTo":"20181016172703.134901-1-sbeller@google.com","subject":"Re: [PATCH] submodule helper: convert relative URL to absolute URL if needed","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2018-10-16T21:05:38Z","receivedAt":"2018-10-16T21:05:43Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Stefan Beller wrote:\n\n> Reported-by: Jaewoong Jung <jungjw@google.com>\n> Signed-off-by: Stefan Beller <sbeller@google.com>\n> ---\n>  builtin/submodule--helper.c | 51 ++++++++++++++++++++++++-------------\n>  t/t7400-submodule-basic.sh  | 24 +++++++++++++++++\n>  2 files changed, 58 insertions(+), 17 deletions(-)\n\nReviewed-by: Jonathan Nieder <jrnieder@gmail.com>\n\nThanks for your patient work.\n"}]}