{"thread":{"id":"41177","subject":"[PATCHv2] submodule: Port resolve_relative_url from shell to C","startedAt":"2016-01-12T23:35:26Z","lastAt":"2016-01-13T16:58:13Z","messageCount":4,"participants":["Stefan Beller","Johannes Sixt","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"275842","messageId":"1452641726-25625-1-git-send-email-sbeller@google.com","threadId":"41177","inReplyTo":null,"subject":"[PATCHv2] submodule: Port resolve_relative_url from shell to C","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-01-12T23:35:26Z","receivedAt":"2016-01-12T23:35:26Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Later on we want to deprecate the `git submodule init` command and make\nit implicit in other submodule commands. As these other commands are\nwritten in C already, we'd need the init functionality in C, too.\nThe `resolve_relative_url` function is a major part of that init\nfunctionality, so start by porting this function to C.\n\nAs I was porting the functionality I noticed some odds with the inputs.\nTo fully understand the situation I added some logging to the function\ntemporarily to capture all calls to the function throughout the test\nsuite. Duplicates have been removed and all unique testing inputs have\nbeen recorded into t0060.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n\nI tried considering an alternative implementation for `relative_url`,\nwhich is not a direct translation of the shell code. There are some\nadvanced path/url functions, such as `normalize_path_copy`, however\nusing that function is not straightforward as it seems. The idea would\nhave been to use that on a concatenation of remoteurl and url, however\nthere are cases like (\"foo/.\" \"../.\") to result in \"foo/.\", so we really\nneed to count the slashes ourselves.\n\nThis applies on top of current master (2.7.0).\n\nThanks,\nStefan\n\n---\n builtin/submodule--helper.c | 191 ++++++++++++++++++++++++++++++++++++++++++++\n git-submodule.sh            |  81 +------------------\n t/t0060-path-utils.sh       |  41 ++++++++++\n 3 files changed, 236 insertions(+), 77 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex f4c3eff..db9d627 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -9,6 +9,195 @@\n #include \"submodule-config.h\"\n #include \"string-list.h\"\n #include \"run-command.h\"\n+#include \"remote.h\"\n+#include \"refs.h\"\n+#include \"connect.h\"\n+\n+static char *get_default_remote(void)\n+{\n+\tchar *dest = NULL, *ret;\n+\tunsigned char sha1[20];\n+\tint flag;\n+\tstruct strbuf sb = STRBUF_INIT;\n+\tconst char *refname = resolve_ref_unsafe(\"HEAD\", 0, sha1, &flag);\n+\n+\tif (!refname)\n+\t\tdie(\"No such ref: HEAD\");\n+\n+\trefname = shorten_unambiguous_ref(refname, 0);\n+\tstrbuf_addf(&sb, \"branch.%s.remote\", refname);\n+\tif (git_config_get_string(sb.buf, &dest))\n+\t\tret = xstrdup(\"origin\");\n+\telse\n+\t\tret = xstrdup(dest);\n+\n+\tstrbuf_release(&sb);\n+\treturn ret;\n+}\n+\n+static int starts_with_dot_slash(const char *str)\n+{\n+\treturn str[0] == '.' && is_dir_sep(str[1]);\n+}\n+\n+static int starts_with_dot_dot_slash(const char *str)\n+{\n+\treturn str[0] == '.' && str[1] == '.' && is_dir_sep(str[2]);\n+}\n+\n+static char *last_dir_separator(char *str)\n+{\n+\tchar* p = str + strlen(str);\n+\twhile (p-- != str)\n+\t\tif (is_dir_sep(*p))\n+\t\t\treturn p;\n+\treturn NULL;\n+}\n+\n+/*\n+ * The `url` argument is the URL that navigates to the submodule origin\n+ * repo. When relative, this URL is relative to the superproject origin\n+ * URL repo. The `up_path` argument, if specified, is the relative\n+ * path that navigates from the submodule working tree to the superproject\n+ * working tree. Returns the origin URL of the submodule.\n+ *\n+ * Return either an absolute URL or filesystem path (if the superproject\n+ * origin URL is an absolute URL or filesystem path, respectively) or a\n+ * relative file system path (if the superproject origin URL is a relative\n+ * file system path).\n+ *\n+ * When the output is a relative file system path, the path is either\n+ * relative to the submodule working tree, if up_path is specified, or to\n+ * the superproject working tree otherwise.\n+ */\n+static char *relative_url(const char *remote_url,\n+\t\t\t\tconst char *url,\n+\t\t\t\tconst char *up_path)\n+{\n+\tint is_relative = 0;\n+\tint colonsep = 0;\n+\tchar *out;\n+\tchar *remoteurl = xstrdup(remote_url);\n+\tstruct strbuf sb = STRBUF_INIT;\n+\tsize_t len;\n+\n+\tlen = strlen(remoteurl);\n+\tif (is_dir_sep(remoteurl[len]))\n+\t\tremoteurl[len] = '\\0';\n+\n+\tif (!url_is_local_not_ssh(remoteurl) || is_absolute_path(remoteurl))\n+\t\tis_relative = 0;\n+\telse {\n+\t\tis_relative = 1;\n+\n+\t\t/* prepend a './' to ensure all relative remoteurls start with\n+\t\t * './' or '../' */\n+\t\tif (!starts_with_dot_slash(remoteurl) &&\n+\t\t    !starts_with_dot_dot_slash(remoteurl)) {\n+\t\t\tstrbuf_reset(&sb);\n+\t\t\tstrbuf_addf(&sb, \"./%s\", remoteurl);\n+\t\t\tfree(remoteurl);\n+\t\t\tremoteurl = strbuf_detach(&sb, NULL);\n+\t\t}\n+\t}\n+\t/* When the url starts with '../', remove that and the last directory\n+\t * in remoteurl */\n+\twhile (url) {\n+\t\tif (starts_with_dot_dot_slash(url)) {\n+\t\t\tchar *rfind;\n+\t\t\turl += 3;\n+\n+\t\t\trfind = last_dir_separator(remoteurl);\n+\t\t\tif (rfind)\n+\t\t\t\t*rfind = '\\0';\n+\t\t\telse {\n+\t\t\t\trfind = strrchr(remoteurl, ':');\n+\t\t\t\tif (rfind) {\n+\t\t\t\t\t*rfind = '\\0';\n+\t\t\t\t\tcolonsep = 1;\n+\t\t\t\t} else {\n+\t\t\t\t\tif (is_relative || !strcmp(\".\", remoteurl))\n+\t\t\t\t\t\tdie(N_(\"cannot strip one component off url '%s'\"), remoteurl);\n+\t\t\t\t\telse\n+\t\t\t\t\t\tremoteurl = \".\";\n+\t\t\t\t}\n+\t\t\t}\n+\t\t} else if (starts_with_dot_slash(url)) {\n+\t\t\turl += 2;\n+\t\t} else\n+\t\t\tbreak;\n+\t}\n+\tstrbuf_reset(&sb);\n+\tstrbuf_addf(&sb, \"%s%s%s\", remoteurl, colonsep ? \":\" : \"/\", url);\n+\n+\tif (starts_with_dot_slash(sb.buf))\n+\t\tout = xstrdup(sb.buf + 2);\n+\telse\n+\t\tout = xstrdup(sb.buf);\n+\tstrbuf_reset(&sb);\n+\n+\tfree(remoteurl);\n+\tif (!up_path || !is_relative)\n+\t\treturn out;\n+\telse {\n+\t\tstrbuf_addf(&sb, \"%s%s\", up_path, out);\n+\n+\t\tfree(out);\n+\t\treturn strbuf_detach(&sb, NULL);\n+\t}\n+}\n+\n+static int resolve_relative_url(int argc, const char **argv, const char *prefix)\n+{\n+\tchar *remoteurl = NULL;\n+\tchar *remote = get_default_remote();\n+\tconst char *up_path = NULL;\n+\tchar *res;\n+\tconst char *url;\n+\tstruct strbuf sb = STRBUF_INIT;\n+\n+\tif (argc != 2 && argc != 3)\n+\t\tdie(\"BUG: resolve_relative_url only accepts one or two arguments\");\n+\n+\turl = argv[1];\n+\tstrbuf_addf(&sb, \"remote.%s.url\", remote);\n+\tfree(remote);\n+\n+\tif (git_config_get_string(sb.buf, &remoteurl))\n+\t\t/* the repository is its own authoritative upstream */\n+\t\tremoteurl = xgetcwd();\n+\n+\tif (argc == 3)\n+\t\tup_path = argv[2];\n+\n+\tres = relative_url(remoteurl, url, up_path);\n+\tprintf(\"%s\\n\", res);\n+\n+\tfree(res);\n+\treturn 0;\n+}\n+\n+static int resolve_relative_url_test(int argc, const char **argv, const char *prefix)\n+{\n+\tchar *remoteurl, *res;\n+\tconst char *up_path, *url;\n+\n+\tif (argc != 4)\n+\t\tdie(\"BUG: resolve_relative_url only accepts three arguments: <up_path> <remoteurl> <url>\");\n+\n+\tup_path = argv[1];\n+\tremoteurl = xstrdup(argv[2]);\n+\turl = argv[3];\n+\n+\tif (!strcmp(up_path, \"(null)\"))\n+\t\tup_path = NULL;\n+\n+\tres = relative_url(remoteurl, url, up_path);\n+\tprintf(\"%s\\n\", res);\n+\n+\tfree(res);\n+\treturn 0;\n+}\n \n struct module_list {\n \tconst struct cache_entry **entries;\n@@ -264,6 +453,8 @@ static struct cmd_struct commands[] = {\n \t{\"list\", module_list},\n \t{\"name\", module_name},\n \t{\"clone\", module_clone},\n+\t{\"resolve-relative-url\", resolve_relative_url},\n+\t{\"resolve-relative-url-test\", resolve_relative_url_test},\n };\n \n int cmd_submodule__helper(int argc, const char **argv, const char *prefix)\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex 9bc5c5f..3e409af 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -46,79 +46,6 @@ prefix=\n custom_name=\n depth=\n \n-# The function takes at most 2 arguments. The first argument is the\n-# URL that navigates to the submodule origin repo. When relative, this URL\n-# is relative to the superproject origin URL repo. The second up_path\n-# argument, if specified, is the relative path that navigates\n-# from the submodule working tree to the superproject working tree.\n-#\n-# The output of the function is the origin URL of the submodule.\n-#\n-# The output will either be an absolute URL or filesystem path (if the\n-# superproject origin URL is an absolute URL or filesystem path,\n-# respectively) or a relative file system path (if the superproject\n-# origin URL is a relative file system path).\n-#\n-# When the output is a relative file system path, the path is either\n-# relative to the submodule working tree, if up_path is specified, or to\n-# the superproject working tree otherwise.\n-resolve_relative_url ()\n-{\n-\tremote=$(get_default_remote)\n-\tremoteurl=$(git config \"remote.$remote.url\") ||\n-\t\tremoteurl=$(pwd) # the repository is its own authoritative upstream\n-\turl=\"$1\"\n-\tremoteurl=${remoteurl%/}\n-\tsep=/\n-\tup_path=\"$2\"\n-\n-\tcase \"$remoteurl\" in\n-\t*:*|/*)\n-\t\tis_relative=\n-\t\t;;\n-\t./*|../*)\n-\t\tis_relative=t\n-\t\t;;\n-\t*)\n-\t\tis_relative=t\n-\t\tremoteurl=\"./$remoteurl\"\n-\t\t;;\n-\tesac\n-\n-\twhile test -n \"$url\"\n-\tdo\n-\t\tcase \"$url\" in\n-\t\t../*)\n-\t\t\turl=\"${url#../}\"\n-\t\t\tcase \"$remoteurl\" in\n-\t\t\t*/*)\n-\t\t\t\tremoteurl=\"${remoteurl%/*}\"\n-\t\t\t\t;;\n-\t\t\t*:*)\n-\t\t\t\tremoteurl=\"${remoteurl%:*}\"\n-\t\t\t\tsep=:\n-\t\t\t\t;;\n-\t\t\t*)\n-\t\t\t\tif test -z \"$is_relative\" || test \".\" = \"$remoteurl\"\n-\t\t\t\tthen\n-\t\t\t\t\tdie \"$(eval_gettext \"cannot strip one component off url '\\$remoteurl'\")\"\n-\t\t\t\telse\n-\t\t\t\t\tremoteurl=.\n-\t\t\t\tfi\n-\t\t\t\t;;\n-\t\t\tesac\n-\t\t\t;;\n-\t\t./*)\n-\t\t\turl=\"${url#./}\"\n-\t\t\t;;\n-\t\t*)\n-\t\t\tbreak;;\n-\t\tesac\n-\tdone\n-\tremoteurl=\"$remoteurl$sep${url%/}\"\n-\techo \"${is_relative:+${up_path}}${remoteurl#./}\"\n-}\n-\n # Resolve a path to be relative to another path.  This is intended for\n # converting submodule paths when git-submodule is run in a subdirectory\n # and only handles paths where the directory separator is '/'.\n@@ -281,7 +208,7 @@ cmd_add()\n \t\tdie \"$(gettext \"Relative path can only be used from the toplevel of the working tree\")\"\n \n \t\t# dereference source url relative to parent's url\n-\t\trealrepo=$(resolve_relative_url \"$repo\") || exit\n+\t\trealrepo=$(git submodule--helper resolve-relative-url \"$repo\") || exit\n \t\t;;\n \t*:*|/*)\n \t\t# absolute url\n@@ -485,7 +412,7 @@ cmd_init()\n \t\t\t# Possibly a url relative to parent\n \t\t\tcase \"$url\" in\n \t\t\t./*|../*)\n-\t\t\t\turl=$(resolve_relative_url \"$url\") || exit\n+\t\t\t\turl=$(git submodule--helper resolve-relative-url \"$url\") || exit\n \t\t\t\t;;\n \t\t\tesac\n \t\t\tgit config submodule.\"$name\".url \"$url\" ||\n@@ -1190,9 +1117,9 @@ cmd_sync()\n \t\t\t# guarantee a trailing /\n \t\t\tup_path=${up_path%/}/ &&\n \t\t\t# path from submodule work tree to submodule origin repo\n-\t\t\tsub_origin_url=$(resolve_relative_url \"$url\" \"$up_path\") &&\n+\t\t\tsub_origin_url=$(git submodule--helper resolve-relative-url \"$url\" \"$up_path\") &&\n \t\t\t# path from superproject work tree to submodule origin repo\n-\t\t\tsuper_config_url=$(resolve_relative_url \"$url\") || exit\n+\t\t\tsuper_config_url=$(git submodule--helper resolve-relative-url \"$url\") || exit\n \t\t\t;;\n \t\t*)\n \t\t\tsub_origin_url=\"$url\"\ndiff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh\nindex 627ef85..2ae1bbd 100755\n--- a/t/t0060-path-utils.sh\n+++ b/t/t0060-path-utils.sh\n@@ -19,6 +19,12 @@ relative_path() {\n \t\"test \\\"\\$(test-path-utils relative_path '$1' '$2')\\\" = '$expected'\"\n }\n \n+test_submodule_relative_url() {\n+\texpected=\"$4\"\n+\ttest_expect_success \"test_submodule_relative_url: $1 $2 $3 => $4\" \\\n+\t\"test \\\"\\$(git submodule--helper resolve-relative-url-test '$1' '$2' '$3')\\\" = '$expected'\"\n+}\n+\n test_git_path() {\n \ttest_expect_success \"git-path $1 $2 => $3\" \"\n \t\t$1 git rev-parse --git-path $2 >actual &&\n@@ -286,4 +292,39 @@ test_git_path GIT_COMMON_DIR=bar config                   bar/config\n test_git_path GIT_COMMON_DIR=bar packed-refs              bar/packed-refs\n test_git_path GIT_COMMON_DIR=bar shallow                  bar/shallow\n \n+test_submodule_relative_url \"(null)\" \"../foo/bar\" \"../bar/a/b/c\" \"../foo/bar/a/b/c\"\n+test_submodule_relative_url \"../../../\" \"../foo/bar\" \"../bar/a/b/c\" \"../../../../foo/bar/a/b/c\"\n+test_submodule_relative_url \"(null)\" \"../foo/bar\" \"../submodule\" \"../foo/submodule\"\n+test_submodule_relative_url \"../\" \"../foo/bar\" \"../submodule\" \"../../foo/submodule\"\n+test_submodule_relative_url \"(null)\" \"../foo/submodule\" \"../submodule\" \"../foo/submodule\"\n+test_submodule_relative_url \"../\" \"../foo/submodule\" \"../submodule\" \"../../foo/submodule\"\n+test_submodule_relative_url \"(null)\" \"../foo\" \"../submodule\" \"../submodule\"\n+test_submodule_relative_url \"../\" \"../foo\" \"../submodule\" \"../../submodule\"\n+test_submodule_relative_url \"(null)\" \"./foo/bar\" \"../submodule\" \"foo/submodule\"\n+test_submodule_relative_url \"../\" \"./foo/bar\" \"../submodule\" \"../foo/submodule\"\n+test_submodule_relative_url \"(null)\" \"./foo\" \"../submodule\" \"submodule\"\n+test_submodule_relative_url \"../\" \"./foo\" \"../submodule\" \"../submodule\"\n+test_submodule_relative_url \"(null)\" \"//somewhere else/repo\" \"../subrepo\" \"//somewhere else/subrepo\"\n+test_submodule_relative_url \"(null)\" \"/u//trash directory.t7406-submodule-update/subsuper_update_r\" \"../subsubsuper_update_r\" \"/u//trash directory.t7406-submodule-update/subsubsuper_update_r\"\n+test_submodule_relative_url \"(null)\" \"/u//trash directory.t7406-submodule-update/super_update_r2\" \"../subsuper_update_r\" \"/u//trash directory.t7406-submodule-update/subsuper_update_r\"\n+test_submodule_relative_url \"(null)\" \"/u/trash directory.t3600-rm/.\" \"../.\" \"/u/trash directory.t3600-rm/.\"\n+test_submodule_relative_url \"(null)\" \"/u/trash directory.t3600-rm\" \"./.\" \"/u/trash directory.t3600-rm/.\"\n+test_submodule_relative_url \"(null)\" \"/u/trash directory.t7400-submodule-basic/addtest\" \"../repo\" \"/u/trash directory.t7400-submodule-basic/repo\"\n+test_submodule_relative_url \"../\" \"/u/trash directory.t7400-submodule-basic/addtest\" \"../repo\" \"/u/trash directory.t7400-submodule-basic/repo\"\n+test_submodule_relative_url \"(null)\" \"/u/trash directory.t7400-submodule-basic\" \"./å äö\" \"/u/trash directory.t7400-submodule-basic/å äö\"\n+test_submodule_relative_url \"(null)\" \"/u/trash directory.t7403-submodule-sync/.\" \"../submodule\" \"/u/trash directory.t7403-submodule-sync/submodule\"\n+test_submodule_relative_url \"(null)\" \"/u/trash directory.t7407-submodule-foreach/submodule\" \"../submodule\" \"/u/trash directory.t7407-submodule-foreach/submodule\"\n+test_submodule_relative_url \"(null)\" \"/u/trash directory.t7409-submodule-detached-worktree/home2/../remote\" \"../bundle1\" \"/u/trash directory.t7409-submodule-detached-worktree/home2/../bundle1\"\n+test_submodule_relative_url \"(null)\" \"/u/trash directory.t7613-merge-submodule/submodule_update_repo\" \"./.\" \"/u/trash directory.t7613-merge-submodule/submodule_update_repo/.\"\n+test_submodule_relative_url \"(null)\" \"file:///tmp/repo\" \"../subrepo\" \"file:///tmp/subrepo\"\n+test_submodule_relative_url \"(null)\" \"foo/bar\" \"../submodule\" \"foo/submodule\"\n+test_submodule_relative_url \"../\" \"foo/bar\" \"../submodule\" \"../foo/submodule\"\n+test_submodule_relative_url \"(null)\" \"foo\" \"../submodule\" \"submodule\"\n+test_submodule_relative_url \"../\" \"foo\" \"../submodule\" \"../submodule\"\n+test_submodule_relative_url \"(null)\" \"helper:://hostname/repo\" \"../subrepo\" \"helper:://hostname/subrepo\"\n+test_submodule_relative_url \"(null)\" \"ssh://hostname/repo\" \"../subrepo\" \"ssh://hostname/subrepo\"\n+test_submodule_relative_url \"(null)\" \"ssh://hostname:22/repo\" \"../subrepo\" \"ssh://hostname:22/subrepo\"\n+test_submodule_relative_url \"(null)\" \"user@host:path/to/repo\" \"../subrepo\" \"user@host:path/to/subrepo\"\n+test_submodule_relative_url \"(null)\" \"user@host:repo\" \"../subrepo\" \"user@host:subrepo\"\n+\n test_done\n-- \n2.7.0.9.g9d9d16f.dirty\n"},{"id":"275848","messageId":"1452643808-29384-1-git-send-email-sbeller@google.com","threadId":"41177","inReplyTo":"1452641726-25625-1-git-send-email-sbeller@google.com","subject":"[PATCHv3] submodule: Port resolve_relative_url from shell to C","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-01-13T00:10:08Z","receivedAt":"2016-01-13T00:10:08Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Later on we want to deprecate the `git submodule init` command and make\nit implicit in other submodule commands. As these other commands are\nwritten in C already, we'd need the init functionality in C, too.\nThe `resolve_relative_url` function is a major part of that init\nfunctionality, so start by porting this function to C.\n\nAs I was porting the functionality I noticed some odds with the inputs.\nTo fully understand the situation I added some logging to the function\ntemporarily to capture all calls to the function throughout the test\nsuite. Duplicates have been removed and all unique testing inputs have\nbeen recorded into t0060.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\nI tried considering an alternative implementation for `relative_url`,\nwhich is not a direct translation of the shell code. There are some\nadvanced path/url functions, such as `normalize_path_copy`, however\nusing that function is not straightforward as it seems. The idea would\nhave been to use that on a concatenation of remoteurl and url, however\nthere are cases like (\"foo/.\" \"../.\") to result in \"foo/.\", so we really\nneed to count the slashes ourselves.\n\nThis applies on top of current master (2.7.0).\n\nThanks,\nStefan\n\nInterdiff to v2 (sent half an hour ago):\n\n        diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\n        index db9d627..3dd8008 100644\n        --- a/builtin/submodule--helper.c\n        +++ b/builtin/submodule--helper.c\n        @@ -90,8 +90,8 @@ static char *relative_url(const char *remote_url,\n                else {\n                        is_relative = 1;\n         \n        -               /* prepend a './' to ensure all relative remoteurls start with\n        -                * './' or '../' */\n        +               /* Prepend a './' to ensure all relative remoteurls start\n        +                * with './' or '../'. */\n                        if (!starts_with_dot_slash(remoteurl) &&\n                            !starts_with_dot_dot_slash(remoteurl)) {\n                                strbuf_reset(&sb);\n        @@ -100,8 +100,8 @@ static char *relative_url(const char *remote_url,\n                                remoteurl = strbuf_detach(&sb, NULL);\n                        }\n                }\n        -       /* When the url starts with '../', remove that and the last directory\n        -        * in remoteurl */\n        +       /* When the url starts with '../', remove that and the\n        +        * last directory in remoteurl. */\n                while (url) {\n                        if (starts_with_dot_dot_slash(url)) {\n                                char *rfind;\n\n---\n builtin/submodule--helper.c | 191 ++++++++++++++++++++++++++++++++++++++++++++\n git-submodule.sh            |  81 +------------------\n t/t0060-path-utils.sh       |  41 ++++++++++\n 3 files changed, 236 insertions(+), 77 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex f4c3eff..3dd8008 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -9,6 +9,195 @@\n #include \"submodule-config.h\"\n #include \"string-list.h\"\n #include \"run-command.h\"\n+#include \"remote.h\"\n+#include \"refs.h\"\n+#include \"connect.h\"\n+\n+static char *get_default_remote(void)\n+{\n+\tchar *dest = NULL, *ret;\n+\tunsigned char sha1[20];\n+\tint flag;\n+\tstruct strbuf sb = STRBUF_INIT;\n+\tconst char *refname = resolve_ref_unsafe(\"HEAD\", 0, sha1, &flag);\n+\n+\tif (!refname)\n+\t\tdie(\"No such ref: HEAD\");\n+\n+\trefname = shorten_unambiguous_ref(refname, 0);\n+\tstrbuf_addf(&sb, \"branch.%s.remote\", refname);\n+\tif (git_config_get_string(sb.buf, &dest))\n+\t\tret = xstrdup(\"origin\");\n+\telse\n+\t\tret = xstrdup(dest);\n+\n+\tstrbuf_release(&sb);\n+\treturn ret;\n+}\n+\n+static int starts_with_dot_slash(const char *str)\n+{\n+\treturn str[0] == '.' && is_dir_sep(str[1]);\n+}\n+\n+static int starts_with_dot_dot_slash(const char *str)\n+{\n+\treturn str[0] == '.' && str[1] == '.' && is_dir_sep(str[2]);\n+}\n+\n+static char *last_dir_separator(char *str)\n+{\n+\tchar* p = str + strlen(str);\n+\twhile (p-- != str)\n+\t\tif (is_dir_sep(*p))\n+\t\t\treturn p;\n+\treturn NULL;\n+}\n+\n+/*\n+ * The `url` argument is the URL that navigates to the submodule origin\n+ * repo. When relative, this URL is relative to the superproject origin\n+ * URL repo. The `up_path` argument, if specified, is the relative\n+ * path that navigates from the submodule working tree to the superproject\n+ * working tree. Returns the origin URL of the submodule.\n+ *\n+ * Return either an absolute URL or filesystem path (if the superproject\n+ * origin URL is an absolute URL or filesystem path, respectively) or a\n+ * relative file system path (if the superproject origin URL is a relative\n+ * file system path).\n+ *\n+ * When the output is a relative file system path, the path is either\n+ * relative to the submodule working tree, if up_path is specified, or to\n+ * the superproject working tree otherwise.\n+ */\n+static char *relative_url(const char *remote_url,\n+\t\t\t\tconst char *url,\n+\t\t\t\tconst char *up_path)\n+{\n+\tint is_relative = 0;\n+\tint colonsep = 0;\n+\tchar *out;\n+\tchar *remoteurl = xstrdup(remote_url);\n+\tstruct strbuf sb = STRBUF_INIT;\n+\tsize_t len;\n+\n+\tlen = strlen(remoteurl);\n+\tif (is_dir_sep(remoteurl[len]))\n+\t\tremoteurl[len] = '\\0';\n+\n+\tif (!url_is_local_not_ssh(remoteurl) || is_absolute_path(remoteurl))\n+\t\tis_relative = 0;\n+\telse {\n+\t\tis_relative = 1;\n+\n+\t\t/* Prepend a './' to ensure all relative remoteurls start\n+\t\t * with './' or '../'. */\n+\t\tif (!starts_with_dot_slash(remoteurl) &&\n+\t\t    !starts_with_dot_dot_slash(remoteurl)) {\n+\t\t\tstrbuf_reset(&sb);\n+\t\t\tstrbuf_addf(&sb, \"./%s\", remoteurl);\n+\t\t\tfree(remoteurl);\n+\t\t\tremoteurl = strbuf_detach(&sb, NULL);\n+\t\t}\n+\t}\n+\t/* When the url starts with '../', remove that and the\n+\t * last directory in remoteurl. */\n+\twhile (url) {\n+\t\tif (starts_with_dot_dot_slash(url)) {\n+\t\t\tchar *rfind;\n+\t\t\turl += 3;\n+\n+\t\t\trfind = last_dir_separator(remoteurl);\n+\t\t\tif (rfind)\n+\t\t\t\t*rfind = '\\0';\n+\t\t\telse {\n+\t\t\t\trfind = strrchr(remoteurl, ':');\n+\t\t\t\tif (rfind) {\n+\t\t\t\t\t*rfind = '\\0';\n+\t\t\t\t\tcolonsep = 1;\n+\t\t\t\t} else {\n+\t\t\t\t\tif (is_relative || !strcmp(\".\", remoteurl))\n+\t\t\t\t\t\tdie(N_(\"cannot strip one component off url '%s'\"), remoteurl);\n+\t\t\t\t\telse\n+\t\t\t\t\t\tremoteurl = \".\";\n+\t\t\t\t}\n+\t\t\t}\n+\t\t} else if (starts_with_dot_slash(url)) {\n+\t\t\turl += 2;\n+\t\t} else\n+\t\t\tbreak;\n+\t}\n+\tstrbuf_reset(&sb);\n+\tstrbuf_addf(&sb, \"%s%s%s\", remoteurl, colonsep ? \":\" : \"/\", url);\n+\n+\tif (starts_with_dot_slash(sb.buf))\n+\t\tout = xstrdup(sb.buf + 2);\n+\telse\n+\t\tout = xstrdup(sb.buf);\n+\tstrbuf_reset(&sb);\n+\n+\tfree(remoteurl);\n+\tif (!up_path || !is_relative)\n+\t\treturn out;\n+\telse {\n+\t\tstrbuf_addf(&sb, \"%s%s\", up_path, out);\n+\n+\t\tfree(out);\n+\t\treturn strbuf_detach(&sb, NULL);\n+\t}\n+}\n+\n+static int resolve_relative_url(int argc, const char **argv, const char *prefix)\n+{\n+\tchar *remoteurl = NULL;\n+\tchar *remote = get_default_remote();\n+\tconst char *up_path = NULL;\n+\tchar *res;\n+\tconst char *url;\n+\tstruct strbuf sb = STRBUF_INIT;\n+\n+\tif (argc != 2 && argc != 3)\n+\t\tdie(\"BUG: resolve_relative_url only accepts one or two arguments\");\n+\n+\turl = argv[1];\n+\tstrbuf_addf(&sb, \"remote.%s.url\", remote);\n+\tfree(remote);\n+\n+\tif (git_config_get_string(sb.buf, &remoteurl))\n+\t\t/* the repository is its own authoritative upstream */\n+\t\tremoteurl = xgetcwd();\n+\n+\tif (argc == 3)\n+\t\tup_path = argv[2];\n+\n+\tres = relative_url(remoteurl, url, up_path);\n+\tprintf(\"%s\\n\", res);\n+\n+\tfree(res);\n+\treturn 0;\n+}\n+\n+static int resolve_relative_url_test(int argc, const char **argv, const char *prefix)\n+{\n+\tchar *remoteurl, *res;\n+\tconst char *up_path, *url;\n+\n+\tif (argc != 4)\n+\t\tdie(\"BUG: resolve_relative_url only accepts three arguments: <up_path> <remoteurl> <url>\");\n+\n+\tup_path = argv[1];\n+\tremoteurl = xstrdup(argv[2]);\n+\turl = argv[3];\n+\n+\tif (!strcmp(up_path, \"(null)\"))\n+\t\tup_path = NULL;\n+\n+\tres = relative_url(remoteurl, url, up_path);\n+\tprintf(\"%s\\n\", res);\n+\n+\tfree(res);\n+\treturn 0;\n+}\n \n struct module_list {\n \tconst struct cache_entry **entries;\n@@ -264,6 +453,8 @@ static struct cmd_struct commands[] = {\n \t{\"list\", module_list},\n \t{\"name\", module_name},\n \t{\"clone\", module_clone},\n+\t{\"resolve-relative-url\", resolve_relative_url},\n+\t{\"resolve-relative-url-test\", resolve_relative_url_test},\n };\n \n int cmd_submodule__helper(int argc, const char **argv, const char *prefix)\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex 9bc5c5f..3e409af 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -46,79 +46,6 @@ prefix=\n custom_name=\n depth=\n \n-# The function takes at most 2 arguments. The first argument is the\n-# URL that navigates to the submodule origin repo. When relative, this URL\n-# is relative to the superproject origin URL repo. The second up_path\n-# argument, if specified, is the relative path that navigates\n-# from the submodule working tree to the superproject working tree.\n-#\n-# The output of the function is the origin URL of the submodule.\n-#\n-# The output will either be an absolute URL or filesystem path (if the\n-# superproject origin URL is an absolute URL or filesystem path,\n-# respectively) or a relative file system path (if the superproject\n-# origin URL is a relative file system path).\n-#\n-# When the output is a relative file system path, the path is either\n-# relative to the submodule working tree, if up_path is specified, or to\n-# the superproject working tree otherwise.\n-resolve_relative_url ()\n-{\n-\tremote=$(get_default_remote)\n-\tremoteurl=$(git config \"remote.$remote.url\") ||\n-\t\tremoteurl=$(pwd) # the repository is its own authoritative upstream\n-\turl=\"$1\"\n-\tremoteurl=${remoteurl%/}\n-\tsep=/\n-\tup_path=\"$2\"\n-\n-\tcase \"$remoteurl\" in\n-\t*:*|/*)\n-\t\tis_relative=\n-\t\t;;\n-\t./*|../*)\n-\t\tis_relative=t\n-\t\t;;\n-\t*)\n-\t\tis_relative=t\n-\t\tremoteurl=\"./$remoteurl\"\n-\t\t;;\n-\tesac\n-\n-\twhile test -n \"$url\"\n-\tdo\n-\t\tcase \"$url\" in\n-\t\t../*)\n-\t\t\turl=\"${url#../}\"\n-\t\t\tcase \"$remoteurl\" in\n-\t\t\t*/*)\n-\t\t\t\tremoteurl=\"${remoteurl%/*}\"\n-\t\t\t\t;;\n-\t\t\t*:*)\n-\t\t\t\tremoteurl=\"${remoteurl%:*}\"\n-\t\t\t\tsep=:\n-\t\t\t\t;;\n-\t\t\t*)\n-\t\t\t\tif test -z \"$is_relative\" || test \".\" = \"$remoteurl\"\n-\t\t\t\tthen\n-\t\t\t\t\tdie \"$(eval_gettext \"cannot strip one component off url '\\$remoteurl'\")\"\n-\t\t\t\telse\n-\t\t\t\t\tremoteurl=.\n-\t\t\t\tfi\n-\t\t\t\t;;\n-\t\t\tesac\n-\t\t\t;;\n-\t\t./*)\n-\t\t\turl=\"${url#./}\"\n-\t\t\t;;\n-\t\t*)\n-\t\t\tbreak;;\n-\t\tesac\n-\tdone\n-\tremoteurl=\"$remoteurl$sep${url%/}\"\n-\techo \"${is_relative:+${up_path}}${remoteurl#./}\"\n-}\n-\n # Resolve a path to be relative to another path.  This is intended for\n # converting submodule paths when git-submodule is run in a subdirectory\n # and only handles paths where the directory separator is '/'.\n@@ -281,7 +208,7 @@ cmd_add()\n \t\tdie \"$(gettext \"Relative path can only be used from the toplevel of the working tree\")\"\n \n \t\t# dereference source url relative to parent's url\n-\t\trealrepo=$(resolve_relative_url \"$repo\") || exit\n+\t\trealrepo=$(git submodule--helper resolve-relative-url \"$repo\") || exit\n \t\t;;\n \t*:*|/*)\n \t\t# absolute url\n@@ -485,7 +412,7 @@ cmd_init()\n \t\t\t# Possibly a url relative to parent\n \t\t\tcase \"$url\" in\n \t\t\t./*|../*)\n-\t\t\t\turl=$(resolve_relative_url \"$url\") || exit\n+\t\t\t\turl=$(git submodule--helper resolve-relative-url \"$url\") || exit\n \t\t\t\t;;\n \t\t\tesac\n \t\t\tgit config submodule.\"$name\".url \"$url\" ||\n@@ -1190,9 +1117,9 @@ cmd_sync()\n \t\t\t# guarantee a trailing /\n \t\t\tup_path=${up_path%/}/ &&\n \t\t\t# path from submodule work tree to submodule origin repo\n-\t\t\tsub_origin_url=$(resolve_relative_url \"$url\" \"$up_path\") &&\n+\t\t\tsub_origin_url=$(git submodule--helper resolve-relative-url \"$url\" \"$up_path\") &&\n \t\t\t# path from superproject work tree to submodule origin repo\n-\t\t\tsuper_config_url=$(resolve_relative_url \"$url\") || exit\n+\t\t\tsuper_config_url=$(git submodule--helper resolve-relative-url \"$url\") || exit\n \t\t\t;;\n \t\t*)\n \t\t\tsub_origin_url=\"$url\"\ndiff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh\nindex 627ef85..2ae1bbd 100755\n--- a/t/t0060-path-utils.sh\n+++ b/t/t0060-path-utils.sh\n@@ -19,6 +19,12 @@ relative_path() {\n \t\"test \\\"\\$(test-path-utils relative_path '$1' '$2')\\\" = '$expected'\"\n }\n \n+test_submodule_relative_url() {\n+\texpected=\"$4\"\n+\ttest_expect_success \"test_submodule_relative_url: $1 $2 $3 => $4\" \\\n+\t\"test \\\"\\$(git submodule--helper resolve-relative-url-test '$1' '$2' '$3')\\\" = '$expected'\"\n+}\n+\n test_git_path() {\n \ttest_expect_success \"git-path $1 $2 => $3\" \"\n \t\t$1 git rev-parse --git-path $2 >actual &&\n@@ -286,4 +292,39 @@ test_git_path GIT_COMMON_DIR=bar config                   bar/config\n test_git_path GIT_COMMON_DIR=bar packed-refs              bar/packed-refs\n test_git_path GIT_COMMON_DIR=bar shallow                  bar/shallow\n \n+test_submodule_relative_url \"(null)\" \"../foo/bar\" \"../bar/a/b/c\" \"../foo/bar/a/b/c\"\n+test_submodule_relative_url \"../../../\" \"../foo/bar\" \"../bar/a/b/c\" \"../../../../foo/bar/a/b/c\"\n+test_submodule_relative_url \"(null)\" \"../foo/bar\" \"../submodule\" \"../foo/submodule\"\n+test_submodule_relative_url \"../\" \"../foo/bar\" \"../submodule\" \"../../foo/submodule\"\n+test_submodule_relative_url \"(null)\" \"../foo/submodule\" \"../submodule\" \"../foo/submodule\"\n+test_submodule_relative_url \"../\" \"../foo/submodule\" \"../submodule\" \"../../foo/submodule\"\n+test_submodule_relative_url \"(null)\" \"../foo\" \"../submodule\" \"../submodule\"\n+test_submodule_relative_url \"../\" \"../foo\" \"../submodule\" \"../../submodule\"\n+test_submodule_relative_url \"(null)\" \"./foo/bar\" \"../submodule\" \"foo/submodule\"\n+test_submodule_relative_url \"../\" \"./foo/bar\" \"../submodule\" \"../foo/submodule\"\n+test_submodule_relative_url \"(null)\" \"./foo\" \"../submodule\" \"submodule\"\n+test_submodule_relative_url \"../\" \"./foo\" \"../submodule\" \"../submodule\"\n+test_submodule_relative_url \"(null)\" \"//somewhere else/repo\" \"../subrepo\" \"//somewhere else/subrepo\"\n+test_submodule_relative_url \"(null)\" \"/u//trash directory.t7406-submodule-update/subsuper_update_r\" \"../subsubsuper_update_r\" \"/u//trash directory.t7406-submodule-update/subsubsuper_update_r\"\n+test_submodule_relative_url \"(null)\" \"/u//trash directory.t7406-submodule-update/super_update_r2\" \"../subsuper_update_r\" \"/u//trash directory.t7406-submodule-update/subsuper_update_r\"\n+test_submodule_relative_url \"(null)\" \"/u/trash directory.t3600-rm/.\" \"../.\" \"/u/trash directory.t3600-rm/.\"\n+test_submodule_relative_url \"(null)\" \"/u/trash directory.t3600-rm\" \"./.\" \"/u/trash directory.t3600-rm/.\"\n+test_submodule_relative_url \"(null)\" \"/u/trash directory.t7400-submodule-basic/addtest\" \"../repo\" \"/u/trash directory.t7400-submodule-basic/repo\"\n+test_submodule_relative_url \"../\" \"/u/trash directory.t7400-submodule-basic/addtest\" \"../repo\" \"/u/trash directory.t7400-submodule-basic/repo\"\n+test_submodule_relative_url \"(null)\" \"/u/trash directory.t7400-submodule-basic\" \"./å äö\" \"/u/trash directory.t7400-submodule-basic/å äö\"\n+test_submodule_relative_url \"(null)\" \"/u/trash directory.t7403-submodule-sync/.\" \"../submodule\" \"/u/trash directory.t7403-submodule-sync/submodule\"\n+test_submodule_relative_url \"(null)\" \"/u/trash directory.t7407-submodule-foreach/submodule\" \"../submodule\" \"/u/trash directory.t7407-submodule-foreach/submodule\"\n+test_submodule_relative_url \"(null)\" \"/u/trash directory.t7409-submodule-detached-worktree/home2/../remote\" \"../bundle1\" \"/u/trash directory.t7409-submodule-detached-worktree/home2/../bundle1\"\n+test_submodule_relative_url \"(null)\" \"/u/trash directory.t7613-merge-submodule/submodule_update_repo\" \"./.\" \"/u/trash directory.t7613-merge-submodule/submodule_update_repo/.\"\n+test_submodule_relative_url \"(null)\" \"file:///tmp/repo\" \"../subrepo\" \"file:///tmp/subrepo\"\n+test_submodule_relative_url \"(null)\" \"foo/bar\" \"../submodule\" \"foo/submodule\"\n+test_submodule_relative_url \"../\" \"foo/bar\" \"../submodule\" \"../foo/submodule\"\n+test_submodule_relative_url \"(null)\" \"foo\" \"../submodule\" \"submodule\"\n+test_submodule_relative_url \"../\" \"foo\" \"../submodule\" \"../submodule\"\n+test_submodule_relative_url \"(null)\" \"helper:://hostname/repo\" \"../subrepo\" \"helper:://hostname/subrepo\"\n+test_submodule_relative_url \"(null)\" \"ssh://hostname/repo\" \"../subrepo\" \"ssh://hostname/subrepo\"\n+test_submodule_relative_url \"(null)\" \"ssh://hostname:22/repo\" \"../subrepo\" \"ssh://hostname:22/subrepo\"\n+test_submodule_relative_url \"(null)\" \"user@host:path/to/repo\" \"../subrepo\" \"user@host:path/to/subrepo\"\n+test_submodule_relative_url \"(null)\" \"user@host:repo\" \"../subrepo\" \"user@host:subrepo\"\n+\n test_done\n-- \n2.7.0.9.g9d9d16f.dirty\n"},{"id":"275884","messageId":"5695FFDA.9050409@kdbg.org","threadId":"41177","inReplyTo":"1452643808-29384-1-git-send-email-sbeller@google.com","subject":"Re: [PATCHv3] submodule: Port resolve_relative_url from shell to C","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2016-01-13T07:42:18Z","receivedAt":"2016-01-13T07:42:18Z","isPatch":false,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 13.01.2016 um 01:10 schrieb Stefan Beller:\n> Later on we want to deprecate the `git submodule init` command and make\n> it implicit in other submodule commands. As these other commands are\n> written in C already, we'd need the init functionality in C, too.\n> The `resolve_relative_url` function is a major part of that init\n> functionality, so start by porting this function to C.\n\nMaybe tone down the word \"major\" to \"a large and non-trivial function\"?\nOtherwise, the lack of proof for the claim is irritating. (As we saw,\nthe savings with the port to C are not breath-taking. But we have to\nstart somewhere.)\n\n> \n> As I was porting the functionality I noticed some odds with the inputs.\n> To fully understand the situation I added some logging to the function\n> temporarily to capture all calls to the function throughout the test\n> suite. Duplicates have been removed and all unique testing inputs have\n> been recorded into t0060.\n> \n> Signed-off-by: Stefan Beller <sbeller@google.com>\n> ---\n...\n> +/*\n> + * The `url` argument is the URL that navigates to the submodule origin\n> + * repo. When relative, this URL is relative to the superproject origin\n> + * URL repo. The `up_path` argument, if specified, is the relative\n> + * path that navigates from the submodule working tree to the superproject\n> + * working tree. Returns the origin URL of the submodule.\n> + *\n> + * Return either an absolute URL or filesystem path (if the superproject\n> + * origin URL is an absolute URL or filesystem path, respectively) or a\n> + * relative file system path (if the superproject origin URL is a relative\n> + * file system path).\n> + *\n> + * When the output is a relative file system path, the path is either\n> + * relative to the submodule working tree, if up_path is specified, or to\n> + * the superproject working tree otherwise.\n> + */\n> +static char *relative_url(const char *remote_url,\n> +\t\t\t\tconst char *url,\n> +\t\t\t\tconst char *up_path)\n> +{\n> +\tint is_relative = 0;\n> +\tint colonsep = 0;\n> +\tchar *out;\n> +\tchar *remoteurl = xstrdup(remote_url);\n> +\tstruct strbuf sb = STRBUF_INIT;\n> +\tsize_t len;\n> +\n> +\tlen = strlen(remoteurl);\n> +\tif (is_dir_sep(remoteurl[len]))\n> +\t\tremoteurl[len] = '\\0';\n> +\n> +\tif (!url_is_local_not_ssh(remoteurl) || is_absolute_path(remoteurl))\n> +\t\tis_relative = 0;\n> +\telse {\n> +\t\tis_relative = 1;\n> +\n> +\t\t/* Prepend a './' to ensure all relative remoteurls start\n> +\t\t * with './' or '../'. */\n> +\t\tif (!starts_with_dot_slash(remoteurl) &&\n> +\t\t    !starts_with_dot_dot_slash(remoteurl)) {\n> +\t\t\tstrbuf_reset(&sb);\n> +\t\t\tstrbuf_addf(&sb, \"./%s\", remoteurl);\n> +\t\t\tfree(remoteurl);\n> +\t\t\tremoteurl = strbuf_detach(&sb, NULL);\n> +\t\t}\n> +\t}\n> +\t/* When the url starts with '../', remove that and the\n> +\t * last directory in remoteurl. */\n> +\twhile (url) {\n> +\t\tif (starts_with_dot_dot_slash(url)) {\n> +\t\t\tchar *rfind;\n> +\t\t\turl += 3;\n> +\n> +\t\t\trfind = last_dir_separator(remoteurl);\n> +\t\t\tif (rfind)\n> +\t\t\t\t*rfind = '\\0';\n> +\t\t\telse {\n> +\t\t\t\trfind = strrchr(remoteurl, ':');\n> +\t\t\t\tif (rfind) {\n> +\t\t\t\t\t*rfind = '\\0';\n> +\t\t\t\t\tcolonsep = 1;\n> +\t\t\t\t} else {\n> +\t\t\t\t\tif (is_relative || !strcmp(\".\", remoteurl))\n> +\t\t\t\t\t\tdie(N_(\"cannot strip one component off url '%s'\"), remoteurl);\n\nThis must be _(\"...\"), not N_(\"...\"), otherwise no translation\nhappens at run-time.\n\n> +\t\t\t\t\telse\n> +\t\t\t\t\t\tremoteurl = \".\";\n\nThis must be xstrdup(\".\") because remoteurl is free()d below.\n\n> +\t\t\t\t}\n> +\t\t\t}\n> +\t\t} else if (starts_with_dot_slash(url)) {\n> +\t\t\turl += 2;\n> +\t\t} else\n> +\t\t\tbreak;\n> +\t}\n> +\tstrbuf_reset(&sb);\n> +\tstrbuf_addf(&sb, \"%s%s%s\", remoteurl, colonsep ? \":\" : \"/\", url);\n> +\n> +\tif (starts_with_dot_slash(sb.buf))\n> +\t\tout = xstrdup(sb.buf + 2);\n> +\telse\n> +\t\tout = xstrdup(sb.buf);\n> +\tstrbuf_reset(&sb);\n> +\n> +\tfree(remoteurl);\n> +\tif (!up_path || !is_relative)\n> +\t\treturn out;\n\nNit: This conditional clearly feels like (and is) an early function\nexit...\n\n> +\telse {\n> +\t\tstrbuf_addf(&sb, \"%s%s\", up_path, out);\n> +\n> +\t\tfree(out);\n> +\t\treturn strbuf_detach(&sb, NULL);\n> +\t}\n\n... hence, you don't need to place the rest of the function into the\nelse branch.\n\n> +}\n\nup_path seems to be ignored when remoteurl is absolute. Is that\ncombination an invalid use case?\n\nI think that you strike a good balance between a direct rewrite\nof the shell function and possible optimizations. Therefore,\nfurther improvements should go into separate patches.\n\n> diff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh\n> index 627ef85..2ae1bbd 100755\n> --- a/t/t0060-path-utils.sh\n> +++ b/t/t0060-path-utils.sh\n> @@ -19,6 +19,12 @@ relative_path() {\n>   \t\"test \\\"\\$(test-path-utils relative_path '$1' '$2')\\\" = '$expected'\"\n>   }\n>   \n> +test_submodule_relative_url() {\n> +\texpected=\"$4\"\n> +\ttest_expect_success \"test_submodule_relative_url: $1 $2 $3 => $4\" \\\n> +\t\"test \\\"\\$(git submodule--helper resolve-relative-url-test '$1' '$2' '$3')\\\" = '$expected'\"\n> +}\n\nrelative_url() has an error exit mode that is not unlikely to trigger,\nbut the way this test is written such a failure would be detected\nonly indirectly as a mismatch between actual and expected result. The\nreason is that a failed command substitution does not cause the\nsurrounding command to fail unless it is in the last variable\nassignment. Therefore, I suggest to write the helper function like this:\n\ntest_submodule_relative_url() {\n\ttest_expect_success \"test_submodule_relative_url: $1 $2 $3 => $4\" \"\n\t\tactual=\\$(git submodule--helper resolve-relative-url-test '$1' '$2' '$3') &&\n\t\t\"test \\\"\\$actual\\\" = '$4'\n\t\"\n}\n\n> +\n>   test_git_path() {\n>   \ttest_expect_success \"git-path $1 $2 => $3\" \"\n>   \t\t$1 git rev-parse --git-path $2 >actual &&\n> @@ -286,4 +292,39 @@ test_git_path GIT_COMMON_DIR=bar config                   bar/config\n>   test_git_path GIT_COMMON_DIR=bar packed-refs              bar/packed-refs\n>   test_git_path GIT_COMMON_DIR=bar shallow                  bar/shallow\n>   \n> +test_submodule_relative_url \"(null)\" \"../foo/bar\" \"../bar/a/b/c\" \"../foo/bar/a/b/c\"\n> +test_submodule_relative_url \"../../../\" \"../foo/bar\" \"../bar/a/b/c\" \"../../../../foo/bar/a/b/c\"\n\nIn these two cases, it is unclear whether the \"bar\" in the 4th\nargument is copied from the 2nd or the 3rd argument. I suggest to\nuse a different token:\n\ntest_submodule_relative_url \"(null)\" \"../foo/bar\" \"../sub/a/b/c\" \"../foo/sub/a/b/c\"\ntest_submodule_relative_url \"../../../\" \"../foo/bar\" \"../sub/a/b/c\" \"../../../../foo/sub/a/b/c\"\n\n> +test_submodule_relative_url \"(null)\" \"../foo/bar\" \"../submodule\" \"../foo/submodule\"\n> +test_submodule_relative_url \"../\" \"../foo/bar\" \"../submodule\" \"../../foo/submodule\"\n> +test_submodule_relative_url \"(null)\" \"../foo/submodule\" \"../submodule\" \"../foo/submodule\"\n> +test_submodule_relative_url \"../\" \"../foo/submodule\" \"../submodule\" \"../../foo/submodule\"\n> +test_submodule_relative_url \"(null)\" \"../foo\" \"../submodule\" \"../submodule\"\n> +test_submodule_relative_url \"../\" \"../foo\" \"../submodule\" \"../../submodule\"\n> +test_submodule_relative_url \"(null)\" \"./foo/bar\" \"../submodule\" \"foo/submodule\"\n> +test_submodule_relative_url \"../\" \"./foo/bar\" \"../submodule\" \"../foo/submodule\"\n> +test_submodule_relative_url \"(null)\" \"./foo\" \"../submodule\" \"submodule\"\n> +test_submodule_relative_url \"../\" \"./foo\" \"../submodule\" \"../submodule\"\n> +test_submodule_relative_url \"(null)\" \"//somewhere else/repo\" \"../subrepo\" \"//somewhere else/subrepo\"\n> +test_submodule_relative_url \"(null)\" \"/u//trash directory.t7406-submodule-update/subsuper_update_r\" \"../subsubsuper_update_r\" \"/u//trash directory.t7406-submodule-update/subsubsuper_update_r\"\n> +test_submodule_relative_url \"(null)\" \"/u//trash directory.t7406-submodule-update/super_update_r2\" \"../subsuper_update_r\" \"/u//trash directory.t7406-submodule-update/subsuper_update_r\"\n> +test_submodule_relative_url \"(null)\" \"/u/trash directory.t3600-rm/.\" \"../.\" \"/u/trash directory.t3600-rm/.\"\n> +test_submodule_relative_url \"(null)\" \"/u/trash directory.t3600-rm\" \"./.\" \"/u/trash directory.t3600-rm/.\"\n> +test_submodule_relative_url \"(null)\" \"/u/trash directory.t7400-submodule-basic/addtest\" \"../repo\" \"/u/trash directory.t7400-submodule-basic/repo\"\n> +test_submodule_relative_url \"../\" \"/u/trash directory.t7400-submodule-basic/addtest\" \"../repo\" \"/u/trash directory.t7400-submodule-basic/repo\"\n> +test_submodule_relative_url \"(null)\" \"/u/trash directory.t7400-submodule-basic\" \"./å äö\" \"/u/trash directory.t7400-submodule-basic/å äö\"\n> +test_submodule_relative_url \"(null)\" \"/u/trash directory.t7403-submodule-sync/.\" \"../submodule\" \"/u/trash directory.t7403-submodule-sync/submodule\"\n> +test_submodule_relative_url \"(null)\" \"/u/trash directory.t7407-submodule-foreach/submodule\" \"../submodule\" \"/u/trash directory.t7407-submodule-foreach/submodule\"\n> +test_submodule_relative_url \"(null)\" \"/u/trash directory.t7409-submodule-detached-worktree/home2/../remote\" \"../bundle1\" \"/u/trash directory.t7409-submodule-detached-worktree/home2/../bundle1\"\n> +test_submodule_relative_url \"(null)\" \"/u/trash directory.t7613-merge-submodule/submodule_update_repo\" \"./.\" \"/u/trash directory.t7613-merge-submodule/submodule_update_repo/.\"\n> +test_submodule_relative_url \"(null)\" \"file:///tmp/repo\" \"../subrepo\" \"file:///tmp/subrepo\"\n> +test_submodule_relative_url \"(null)\" \"foo/bar\" \"../submodule\" \"foo/submodule\"\n> +test_submodule_relative_url \"../\" \"foo/bar\" \"../submodule\" \"../foo/submodule\"\n> +test_submodule_relative_url \"(null)\" \"foo\" \"../submodule\" \"submodule\"\n> +test_submodule_relative_url \"../\" \"foo\" \"../submodule\" \"../submodule\"\n> +test_submodule_relative_url \"(null)\" \"helper:://hostname/repo\" \"../subrepo\" \"helper:://hostname/subrepo\"\n> +test_submodule_relative_url \"(null)\" \"ssh://hostname/repo\" \"../subrepo\" \"ssh://hostname/subrepo\"\n> +test_submodule_relative_url \"(null)\" \"ssh://hostname:22/repo\" \"../subrepo\" \"ssh://hostname:22/subrepo\"\n> +test_submodule_relative_url \"(null)\" \"user@host:path/to/repo\" \"../subrepo\" \"user@host:path/to/subrepo\"\n> +test_submodule_relative_url \"(null)\" \"user@host:repo\" \"../subrepo\" \"user@host:subrepo\"\n> +\n>   test_done\n> \n"},{"id":"275923","messageId":"xmqqy4btpgbe.fsf@gitster.mtv.corp.google.com","threadId":"41177","inReplyTo":"5695FFDA.9050409@kdbg.org","subject":"Re: [PATCHv3] submodule: Port resolve_relative_url from shell to C","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-01-13T16:58:13Z","receivedAt":"2016-01-13T16:58:13Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt <j6t@kdbg.org> writes:\n\n> Am 13.01.2016 um 01:10 schrieb Stefan Beller:\n>> Later on we want to deprecate the `git submodule init` command and make\n>> it implicit in other submodule commands. As these other commands are\n>> written in C already, we'd need the init functionality in C, too.\n>> The `resolve_relative_url` function is a major part of that init\n>> functionality, so start by porting this function to C.\n>\n> Maybe tone down the word \"major\" to \"a large and non-trivial function\"?\n> Otherwise, the lack of proof for the claim is irritating. (As we saw,\n> the savings with the port to C are not breath-taking. But we have to\n> start somewhere.)\n> ...\n\nAll good suggestions.  Thanks for a low latency review.  Very much\nappreciated.\n"}]}