{"thread":{"id":"41183","subject":"[PATCH] submodule: Port resolve_relative_url from shell to C","startedAt":"2016-01-13T18:15:27Z","lastAt":"2016-01-15T23:03:17Z","messageCount":12,"participants":["Stefan Beller","Junio C Hamano","Eric Sunshine","Jens Lehmann","Johannes Sixt"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"275947","messageId":"1452708927-9401-1-git-send-email-sbeller@google.com","threadId":"41183","inReplyTo":null,"subject":"[PATCH] submodule: Port resolve_relative_url from shell to C","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-01-13T18:15:27Z","receivedAt":"2016-01-13T18:15:27Z","isPatch":true,"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 rather large 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> up_path seems to be ignored when remoteurl is absolute. Is that\n> combination an invalid use case?\n\nYes, that is invalid. See the original:\n \n    echo \"${is_relative:+${up_path}}${remoteurl#./}\"\n\nThis also only adds up_path in case of is_relative being true.\nDid you mean to say that fact should be documented?\n\n> I think that you strike a good balance between a direct rewrite\n> of the shell function and possible optimizations. Therefore,\n> further improvements should go into separate patches.\n\nok.\n\n> In these two cases, it is unclear whether the \"bar\" in the 4th\n> argument is copied from the 2nd or the 3rd argument. I suggest to\n> use a different token:\n> ...\n\nI just produced all possible test cases I could find in the test suite,\nwhich looked like they are a good fit. (I did some deduplication there already,\nthe test suite produced over 100 different mostly subtly reworded test cases)\n\nI wonder if I should slim down this more as the test suite already takes very long\nto run. However these tests don't take very long on their own.\n\nThanks for the review,\nStefan\n\ninterdiff to v2:\n\tdiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\n\tindex 3dd8008..3e58b5d 100644\n\t--- a/builtin/submodule--helper.c\n\t+++ b/builtin/submodule--helper.c\n\t@@ -117,9 +117,9 @@ static char *relative_url(const char *remote_url,\n\t\t\t\t\t\tcolonsep = 1;\n\t\t\t\t\t} else {\n\t\t\t\t\t\tif (is_relative || !strcmp(\".\", remoteurl))\n\t-\t\t\t\t\t\tdie(N_(\"cannot strip one component off url '%s'\"), remoteurl);\n\t+\t\t\t\t\t\tdie(_(\"cannot strip one component off url '%s'\"), remoteurl);\n\t\t\t\t\t\telse\n\t-\t\t\t\t\t\tremoteurl = \".\";\n\t+\t\t\t\t\t\tremoteurl = xstrdup(\".\");\n\t\t\t\t\t}\n\t\t\t\t}\n\t\t\t} else if (starts_with_dot_slash(url)) {\n\t@@ -139,12 +139,10 @@ static char *relative_url(const char *remote_url,\n\t\tfree(remoteurl);\n\t\tif (!up_path || !is_relative)\n\t\t\treturn out;\n\t-\telse {\n\t-\t\tstrbuf_addf(&sb, \"%s%s\", up_path, out);\n\t \n\t-\t\tfree(out);\n\t-\t\treturn strbuf_detach(&sb, NULL);\n\t-\t}\n\t+\tstrbuf_addf(&sb, \"%s%s\", up_path, out);\n\t+\tfree(out);\n\t+\treturn strbuf_detach(&sb, NULL);\n\t }\n\t \n\t static int resolve_relative_url(int argc, const char **argv, const char *prefix)\n\tdiff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh\n\tindex 2ae1bbd..8a1579c 100755\n\t--- a/t/t0060-path-utils.sh\n\t+++ b/t/t0060-path-utils.sh\n\t@@ -20,9 +20,10 @@ relative_path() {\n\t }\n\t \n\t test_submodule_relative_url() {\n\t-\texpected=\"$4\"\n\t-\ttest_expect_success \"test_submodule_relative_url: $1 $2 $3 => $4\" \\\n\t-\t\"test \\\"\\$(git submodule--helper resolve-relative-url-test '$1' '$2' '$3')\\\" = '$expected'\"\n\t+\ttest_expect_success \"test_submodule_relative_url: $1 $2 $3 => $4\" \"\n\t+\t\tactual=\\$(git submodule--helper resolve-relative-url-test '$1' '$2' '$3') &&\n\t+\t\ttest \\\"\\$actual\\\" = '$4'\n\t+\t\"\n\t }\n\t \n\t test_git_path() {\n\t@@ -292,8 +293,8 @@ test_git_path GIT_COMMON_DIR=bar config                   bar/config\n\t test_git_path GIT_COMMON_DIR=bar packed-refs              bar/packed-refs\n\t test_git_path GIT_COMMON_DIR=bar shallow                  bar/shallow\n\t \n\t-test_submodule_relative_url \"(null)\" \"../foo/bar\" \"../bar/a/b/c\" \"../foo/bar/a/b/c\"\n\t-test_submodule_relative_url \"../../../\" \"../foo/bar\" \"../bar/a/b/c\" \"../../../../foo/bar/a/b/c\"\n\t+test_submodule_relative_url \"(null)\" \"../foo/bar\" \"../sub/a/b/c\" \"../foo/sub/a/b/c\"\n\t+test_submodule_relative_url \"../../../\" \"../foo/bar\" \"../sub/a/b/c\" \"../../../../foo/sub/a/b/c\"\n\t test_submodule_relative_url \"(null)\" \"../foo/bar\" \"../submodule\" \"../foo/submodule\"\n\t test_submodule_relative_url \"../\" \"../foo/bar\" \"../submodule\" \"../../foo/submodule\"\n\t test_submodule_relative_url \"(null)\" \"../foo/submodule\" \"../submodule\" \"../foo/submodule\"\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\n---\n builtin/submodule--helper.c | 189 ++++++++++++++++++++++++++++++++++++++++++++\n git-submodule.sh            |  81 +------------------\n t/t0060-path-utils.sh       |  42 ++++++++++\n 3 files changed, 235 insertions(+), 77 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex f4c3eff..3e58b5d 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -9,6 +9,193 @@\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(_(\"cannot strip one component off url '%s'\"), remoteurl);\n+\t\t\t\t\telse\n+\t\t\t\t\t\tremoteurl = xstrdup(\".\");\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+\n+\tstrbuf_addf(&sb, \"%s%s\", up_path, out);\n+\tfree(out);\n+\treturn strbuf_detach(&sb, NULL);\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 +451,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..8a1579c 100755\n--- a/t/t0060-path-utils.sh\n+++ b/t/t0060-path-utils.sh\n@@ -19,6 +19,13 @@ relative_path() {\n \t\"test \\\"\\$(test-path-utils relative_path '$1' '$2')\\\" = '$expected'\"\n }\n \n+test_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\ttest \\\"\\$actual\\\" = '$4'\n+\t\"\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 +293,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\" \"../sub/a/b/c\" \"../foo/sub/a/b/c\"\n+test_submodule_relative_url \"../../../\" \"../foo/bar\" \"../sub/a/b/c\" \"../../../../foo/sub/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.1.g33e69b5.dirty\n"},{"id":"275979","messageId":"xmqq4mehm92b.fsf@gitster.mtv.corp.google.com","threadId":"41183","inReplyTo":"1452708927-9401-1-git-send-email-sbeller@google.com","subject":"Re: [PATCH] submodule: Port resolve_relative_url from shell to C","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-01-13T22:03:08Z","receivedAt":"2016-01-13T22:03:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> Later on we want to deprecate the `git submodule init` command and make\n> it implicit in other submodule commands.\n\nI doubt there is a concensus for \"deprecate\" part to warrant the use\nof \"we want\" here.  I tend to think that the latter half of the\nsentence is uncontroversial, i.e. it is a good idea to make other\n\"submodule\" subcommands internally call it when it makes sense, and\nalso make knobs available to other commands like \"clone\" and\npossibly \"checkout\" so that the users do not have to do the\n\"submodule init\" as a separate step, though.\n\n> As I was porting the functionality I noticed some odds with the inputs.\n\nI can parse but cannot quite grok.  You found some strange things in\nthe input?  Whose input, that comes from where given by whom?\n\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\nI can also parse this, but it is unclear what you did to the\ntemporary debugging help at the end.  If you left it, then that is\nno longer a temporary but is part of the final product.  It is also\nunclear what \"Duplicates\" you are talking about here.\n\nDo you mean that you found some of the existing tests were odd, and\nafter examination with help from a temporary hack which does not\nremain in this patch, you determined that some tests were duplicated,\nwhich you removed, while adding new ones?\n\n>  builtin/submodule--helper.c | 189 ++++++++++++++++++++++++++++++++++++++++++++\n>  git-submodule.sh            |  81 +------------------\n>  t/t0060-path-utils.sh       |  42 ++++++++++\n>  3 files changed, 235 insertions(+), 77 deletions(-)\n>\n> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\n> index f4c3eff..3e58b5d 100644\n> --- a/builtin/submodule--helper.c\n> +++ b/builtin/submodule--helper.c\n> @@ -9,6 +9,193 @@\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\nIs it correct to use shorten_unambiguous_ref() here like this?  The\nfunction is meant to be used when you want heads/frotz because you\nhave both refs/heads/frotz and refs/tags/frotz at the same time.  I\nthink you want to say branch.frotz.remote even in such a case.  IOW,\nshouldn't it be skip_prefix() with refs/heads/, together with die()\nif the prefix is something else?\n\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\nAsterisk sticks to the variable, not the type.\n\n> +\twhile (p-- != str)\n\nIt is preferable to use '>' not '!=' here, because you know p\napproaches str from the larger side, for readability.\n\n> +\t\tif (is_dir_sep(*p))\n> +\t\t\treturn p;\n> +\treturn NULL;\n> +}\n\n(a useless comment) This is one of the rare places where I wish\nthere were a version of strcspn() that scans from the right.\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\nNothing wrong here, but it looked somewhat inconsistent to see this\nassignment, while remoteurl is done as an initialization [*1*]\n\n\n[Footnote]\n\n*1* as a personal preference, I tend to prefer seeing only simple\noperations in initialization and heavyweight ones as a separate\nassignment to an otherwise uninitialized variable, and strlen() is\nlighter-weight than xstrdup() in my dictionary.\n\n\n\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\nAdjust the style, and perhaps remove the final full-stop to make the\nlast string literal easier to see?  I.e.\n\n\t/*\n         * Prepend a './' to ensure all relative remoteurls\n         * start with './' or '../'\n         */\n\nwould be easier to see what it is said.\n\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\nStyle.\n\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(_(\"cannot strip one component off url '%s'\"), remoteurl);\n> +\t\t\t\t\telse\n> +\t\t\t\t\t\tremoteurl = xstrdup(\".\");\n> +\t\t\t\t}\n> +\t\t\t}\n\nIt is somewhat hard to see how this avoids stripping one (or both)\nslashes just after \"http:\" in remoteurl=\"http://site/path/\", leaving\njust \"http:/\" (or \"http:\").\n\nThis codepath has overly deep nesting levels.  Is this the simplest\nwe can do?\n\nThe final else { if .. else } can be made into else if .. else to\ndedent the overlong die() by one level, but I am wondering if the\ndeep nesting is just a symptom of logic being unnecessarily complex.\n\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> +\n> +\tstrbuf_addf(&sb, \"%s%s\", up_path, out);\n> +\tfree(out);\n> +\treturn strbuf_detach(&sb, NULL);\n> +}\n\nThanks.\n"},{"id":"275980","messageId":"CAPig+cTpghLgRdCCHu6CdM0v4TzytsOFFuE5p9=Z0myZ7+5xLQ@mail.gmail.com","threadId":"41183","inReplyTo":"1452708927-9401-1-git-send-email-sbeller@google.com","subject":"Re: [PATCH] submodule: Port resolve_relative_url from shell to C","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-01-13T22:03:27Z","receivedAt":"2016-01-13T22:03:27Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Jan 13, 2016 at 1:15 PM, Stefan Beller <sbeller@google.com> wrote:\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 rather large part of that init\n> functionality, so start by porting this function to C.\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\nNo time presently for a proper review, so just a few superficial comments...\n\n> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\n> @@ -9,6 +9,193 @@\n> +static int resolve_relative_url(int argc, const char **argv, const char *prefix)\n> +{\n> +       char *remoteurl = NULL;\n> +       char *remote = get_default_remote();\n> +       const char *up_path = NULL;\n> +       char *res;\n> +       const char *url;\n> +       struct strbuf sb = STRBUF_INIT;\n> +\n> +       if (argc != 2 && argc != 3)\n> +               die(\"BUG: resolve_relative_url only accepts one or two arguments\");\n\nThis is diagnosing incorrect arguments, so:\ns/resolve_relative_url/resolve-relative-url/\n\nAlso, I'm not convinced that this deserves a \"BUG:\" prefix, as this is\nnow a user-accessible command (even though it's plumbing).\n\n> +       url = argv[1];\n> +       strbuf_addf(&sb, \"remote.%s.url\", remote);\n> +       free(remote);\n> +\n> +       if (git_config_get_string(sb.buf, &remoteurl))\n> +               /* the repository is its own authoritative upstream */\n> +               remoteurl = xgetcwd();\n> +\n> +       if (argc == 3)\n> +               up_path = argv[2];\n> +\n> +       res = relative_url(remoteurl, url, up_path);\n> +       printf(\"%s\\n\", res);\n> +\n> +       free(res);\n> +       return 0;\n> +}\n> +\n> +static int resolve_relative_url_test(int argc, const char **argv, const char *prefix)\n> +{\n> +       char *remoteurl, *res;\n> +       const char *up_path, *url;\n> +\n> +       if (argc != 4)\n> +               die(\"BUG: resolve_relative_url only accepts three arguments: <up_path> <remoteurl> <url>\");\n\ns/resolve_relative_url/resolve-relative-url/\n\nDitto observation about \"BUG:\" prefix.\n\n> +       up_path = argv[1];\n> +       remoteurl = xstrdup(argv[2]);\n> +       url = argv[3];\n> +\n> +       if (!strcmp(up_path, \"(null)\"))\n> +               up_path = NULL;\n> +\n> +       res = relative_url(remoteurl, url, up_path);\n> +       printf(\"%s\\n\", res);\n\nThis could be:\n\n     puts(res);\n\nthough I don't care strongly.\n\n> +       free(res);\n> +       return 0;\n> +}\n"},{"id":"275982","messageId":"CAGZ79ka0rxYK7GRSjh13XOsg887EgqYtc5B60z9qU=tAoJGERQ@mail.gmail.com","threadId":"41183","inReplyTo":"xmqq4mehm92b.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] submodule: Port resolve_relative_url from shell to C","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-01-13T22:47:20Z","receivedAt":"2016-01-13T22:47:20Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, Jan 13, 2016 at 2:03 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Stefan Beller <sbeller@google.com> writes:\n>\n>> Later on we want to deprecate the `git submodule init` command and make\n>> it implicit in other submodule commands.\n>\n> I doubt there is a concensus for \"deprecate\" part to warrant the use\n> of \"we want\" here.  I tend to think that the latter half of the\n> sentence is uncontroversial, i.e. it is a good idea to make other\n> \"submodule\" subcommands internally call it when it makes sense, and\n> also make knobs available to other commands like \"clone\" and\n> possibly \"checkout\" so that the users do not have to do the\n> \"submodule init\" as a separate step, though.\n\nMaybe I need to rethink my strategy here and deliver a patch series\nwhich includes a complete port of `submodule init`, and maybe even\noptions in checkout (and clone) to run `submodule init`. That way the\nimmediate benefit would be clear on why the series is a good idea.\n\nThe current wording is mostly arguing to Jens, how to do the submodule\ngroups thing later on, but skipping the immediate steps.\n\n>\n>> As I was porting the functionality I noticed some odds with the inputs.\n>\n> I can parse but cannot quite grok.  You found some strange things in\n> the input?  Whose input, that comes from where given by whom?\n>\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> I can also parse this, but it is unclear what you did to the\n> temporary debugging help at the end.  If you left it, then that is\n> no longer a temporary but is part of the final product.  It is also\n> unclear what \"Duplicates\" you are talking about here.\n\nSo in v1 somebody complained it's not clear what kind of input you'd get into\nthe relative_url(up_path, remoteurl, url) function. I did not know either, as it\nwas a straight port, passing the test suite. So I wanted to add tests.\n\nTo come up with reasonable tests I added a section to the code similar as this:\n\n    {\n        FILE *f = fopen(\"/tmp/testcases\", \"a\");\n        fprintf(f, \"%s|%s|%s|%s\\n\", up_path, remoteurl, url, result);\n        fclose(f);\n    }\n\nThen I run the whole test suite with the relative_url instrumented.\nThis gave me a file \"/tmp/testcases\" containing 500 lines with valid\nin and output for the `relative_url` function.\nHowever I run these 500 lines through sort|uniq to get about 90 lines\nof unduplicated tests.\n\nbut in these 90 lines there were still syntactic duplicates where one\nline may look like the other line just with\n    s/trash directory.tXXXX/trash directory.tYYYY/\nso I removed these lines manually, too.\n\nAnd that's how I came up with the set of tests.\nThe logging function to \"/tmp/testcases\" was temporary and is not part\nof the final product, but by mentioning that, some issues may be clear\nto the reader, such as:\n * why there are tests with /u/trash directory-t7400.../...\n * the tests are as exhaustive as the test suite before.\n * there are no tests to test failure though, only test for good tests\n\n>\n> Do you mean that you found some of the existing tests were odd, and\n> after examination with help from a temporary hack which does not\n> remain in this patch, you determined that some tests were duplicated,\n> which you removed, while adding new ones?\n\nYes, this.\n\n>\n>>  builtin/submodule--helper.c | 189 ++++++++++++++++++++++++++++++++++++++++++++\n>>  git-submodule.sh            |  81 +------------------\n>>  t/t0060-path-utils.sh       |  42 ++++++++++\n>>  3 files changed, 235 insertions(+), 77 deletions(-)\n>>\n>> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\n>> index f4c3eff..3e58b5d 100644\n>> --- a/builtin/submodule--helper.c\n>> +++ b/builtin/submodule--helper.c\n>> @@ -9,6 +9,193 @@\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>> +     char *dest = NULL, *ret;\n>> +     unsigned char sha1[20];\n>> +     int flag;\n>> +     struct strbuf sb = STRBUF_INIT;\n>> +     const char *refname = resolve_ref_unsafe(\"HEAD\", 0, sha1, &flag);\n>> +\n>> +     if (!refname)\n>> +             die(\"No such ref: HEAD\");\n>> +\n>> +     refname = shorten_unambiguous_ref(refname, 0);\n>> +     strbuf_addf(&sb, \"branch.%s.remote\", refname);\n>\n> Is it correct to use shorten_unambiguous_ref() here like this?  The\n> function is meant to be used when you want heads/frotz because you\n> have both refs/heads/frotz and refs/tags/frotz at the same time.  I\n> think you want to say branch.frotz.remote even in such a case.  IOW,\n> shouldn't it be skip_prefix() with refs/heads/, together with die()\n> if the prefix is something else?\n\nRight.\n\n>\n>> +     if (git_config_get_string(sb.buf, &dest))\n>> +             ret = xstrdup(\"origin\");\n>> +     else\n>> +             ret = xstrdup(dest);\n>> +\n>> +     strbuf_release(&sb);\n>> +     return ret;\n>> +}\n>> +\n>> +static int starts_with_dot_slash(const char *str)\n>> +{\n>> +     return str[0] == '.' && is_dir_sep(str[1]);\n>> +}\n>> +\n>> +static int starts_with_dot_dot_slash(const char *str)\n>> +{\n>> +     return str[0] == '.' && str[1] == '.' && is_dir_sep(str[2]);\n>> +}\n>> +\n>> +static char *last_dir_separator(char *str)\n>> +{\n>> +     char* p = str + strlen(str);\n>\n> Asterisk sticks to the variable, not the type.\n\nok\n\n>\n>> +     while (p-- != str)\n>\n> It is preferable to use '>' not '!=' here, because you know p\n> approaches str from the larger side, for readability.\n\nAlso known as the limes operator (p--> str, \"p goes to str\")\njust kidding :)\n\n>\n>> +             if (is_dir_sep(*p))\n>> +                     return p;\n>> +     return NULL;\n>> +}\n>\n> (a useless comment) This is one of the rare places where I wish\n> there were a version of strcspn() that scans from the right.\n>\n>> +static char *relative_url(const char *remote_url,\n>> +                             const char *url,\n>> +                             const char *up_path)\n>> +{\n>> +     int is_relative = 0;\n>> +     int colonsep = 0;\n>> +     char *out;\n>> +     char *remoteurl = xstrdup(remote_url);\n>> +     struct strbuf sb = STRBUF_INIT;\n>> +     size_t len;\n>> +\n>> +     len = strlen(remoteurl);\n>\n> Nothing wrong here, but it looked somewhat inconsistent to see this\n> assignment, while remoteurl is done as an initialization [*1*]\n\nok, noted.\n\n>\n>\n> [Footnote]\n>\n> *1* as a personal preference, I tend to prefer seeing only simple\n> operations in initialization and heavyweight ones as a separate\n> assignment to an otherwise uninitialized variable, and strlen() is\n> lighter-weight than xstrdup() in my dictionary.\n>\n>\n>\n>> +     if (is_dir_sep(remoteurl[len]))\n>> +             remoteurl[len] = '\\0';\n>> +\n>> +     if (!url_is_local_not_ssh(remoteurl) || is_absolute_path(remoteurl))\n>> +             is_relative = 0;\n>> +     else {\n>> +             is_relative = 1;\n>> +\n>> +             /* Prepend a './' to ensure all relative remoteurls start\n>> +              * with './' or '../'. */\n>\n> Adjust the style, and perhaps remove the final full-stop to make the\n> last string literal easier to see?  I.e.\n>\n>         /*\n>          * Prepend a './' to ensure all relative remoteurls\n>          * start with './' or '../'\n>          */\n>\n> would be easier to see what it is said.\n\nok\n\n>\n>> +             if (!starts_with_dot_slash(remoteurl) &&\n>> +                 !starts_with_dot_dot_slash(remoteurl)) {\n>> +                     strbuf_reset(&sb);\n>> +                     strbuf_addf(&sb, \"./%s\", remoteurl);\n>> +                     free(remoteurl);\n>> +                     remoteurl = strbuf_detach(&sb, NULL);\n>> +             }\n>> +     }\n>> +     /* When the url starts with '../', remove that and the\n>> +      * last directory in remoteurl. */\n>\n> Style.\n\nok\n\n>\n>> +     while (url) {\n>> +             if (starts_with_dot_dot_slash(url)) {\n>> +                     char *rfind;\n>> +                     url += 3;\n>> +\n>> +                     rfind = last_dir_separator(remoteurl);\n>> +                     if (rfind)\n>> +                             *rfind = '\\0';\n>> +                     else {\n>> +                             rfind = strrchr(remoteurl, ':');\n>> +                             if (rfind) {\n>> +                                     *rfind = '\\0';\n>> +                                     colonsep = 1;\n>> +                             } else {\n>> +                                     if (is_relative || !strcmp(\".\", remoteurl))\n>> +                                             die(_(\"cannot strip one component off url '%s'\"), remoteurl);\n>> +                                     else\n>> +                                             remoteurl = xstrdup(\".\");\n>> +                             }\n>> +                     }\n>\n> It is somewhat hard to see how this avoids stripping one (or both)\n> slashes just after \"http:\" in remoteurl=\"http://site/path/\", leaving\n> just \"http:/\" (or \"http:\").\n\nit would leave just 'http:/' if url were to be ../../some/where/else,\nsuch that the constructed url below would be http://some/where/else.\n\n>\n> This codepath has overly deep nesting levels.  Is this the simplest\n> we can do?\n\nit's a direct translation from shell. I could imagine the inside of\n    if (starts_with_dot_dot_slash(url)) {\n        ...\n    }\n\nmay go to its own function, such that it becomes:\n\n    while (url) {\n        if (starts_with_dot_dot_slash(url)) {\n            adjust_remoteurl_and_url(&url, &remoteurl)\n        else if (starts_with_dot_slash(url))\n            url += 2;\n        else\n            break;\n    }\n\n\nwith a proper name for adjust_remoteurl_and_url of course.\n\n>\n> The final else { if .. else } can be made into else if .. else to\n> dedent the overlong die() by one level, but I am wondering if the\n> deep nesting is just a symptom of logic being unnecessarily complex.\n\nI don't think it's unnecessary complex, but results from a direct\nshell->C translation.\n\n>\n>> +             } else if (starts_with_dot_slash(url)) {\n>> +                     url += 2;\n>> +             } else\n>> +                     break;\n>> +     }\n>> +     strbuf_reset(&sb);\n>> +     strbuf_addf(&sb, \"%s%s%s\", remoteurl, colonsep ? \":\" : \"/\", url);\n>> +\n>> +     if (starts_with_dot_slash(sb.buf))\n>> +             out = xstrdup(sb.buf + 2);\n>> +     else\n>> +             out = xstrdup(sb.buf);\n>> +     strbuf_reset(&sb);\n>> +\n>> +     free(remoteurl);\n>> +     if (!up_path || !is_relative)\n>> +             return out;\n>> +\n>> +     strbuf_addf(&sb, \"%s%s\", up_path, out);\n>> +     free(out);\n>> +     return strbuf_detach(&sb, NULL);\n>> +}\n>\n> Thanks.\n"},{"id":"275985","messageId":"CAGZ79kYtb-m6evvnVAvFiHccOSpLwVCT7GcLRuSG=HFtrRqg6w@mail.gmail.com","threadId":"41183","inReplyTo":"CAPig+cTpghLgRdCCHu6CdM0v4TzytsOFFuE5p9=Z0myZ7+5xLQ@mail.gmail.com","subject":"Re: [PATCH] submodule: Port resolve_relative_url from shell to C","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-01-13T23:37:35Z","receivedAt":"2016-01-13T23:37:35Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, Jan 13, 2016 at 2:03 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:\n> No time presently for a proper review, so just a few superficial comments...\n\nOk, I incorporated all your suggestions, too. Thanks for the superficial review!\n"},{"id":"276078","messageId":"56980A14.1060605@web.de","threadId":"41183","inReplyTo":"CAGZ79ka0rxYK7GRSjh13XOsg887EgqYtc5B60z9qU=tAoJGERQ@mail.gmail.com","subject":"Re: [PATCH] submodule: Port resolve_relative_url from shell to C","fromName":"Jens Lehmann","fromEmail":"jens.lehmann@web.de","sentAt":"2016-01-14T20:50:28Z","receivedAt":"2016-01-14T20:50:28Z","isPatch":true,"sender":{"key":"jens.lehmann@web.de","avatar":"https://avatars.githubusercontent.com/u/135220?v=4"},"body":"Am 13.01.2016 um 23:47 schrieb Stefan Beller:\n> On Wed, Jan 13, 2016 at 2:03 PM, Junio C Hamano <gitster@pobox.com> wrote:\n>> Stefan Beller <sbeller@google.com> writes:\n>>\n>>> Later on we want to deprecate the `git submodule init` command and make\n>>> it implicit in other submodule commands.\n>>\n>> I doubt there is a concensus for \"deprecate\" part to warrant the use\n>> of \"we want\" here.  I tend to think that the latter half of the\n>> sentence is uncontroversial, i.e. it is a good idea to make other\n>> \"submodule\" subcommands internally call it when it makes sense, and\n>> also make knobs available to other commands like \"clone\" and\n>> possibly \"checkout\" so that the users do not have to do the\n>> \"submodule init\" as a separate step, though.\n>\n> Maybe I need to rethink my strategy here and deliver a patch series\n> which includes a complete port of `submodule init`, and maybe even\n> options in checkout (and clone) to run `submodule init`. That way the\n> immediate benefit would be clear on why the series is a good idea.\n\nI think that makes lots of sense. It looks to me like clone already\nhas that option (as --recurse-submodules must init the submodules),\nbut it might make sense to add such an option to checkout to init\n(and then also update) all newly appearing submodules (just like\n\"git submodule update\" has the --init option for the same purpose).\n\n> The current wording is mostly arguing to Jens, how to do the submodule\n> groups thing later on, but skipping the immediate steps.\n\nI really believe that in the future a lot of users will hop on to the\nautomatically-init-and-update-submodules train once we have it (and I\nthink users of the groups feature want to be on that train by default).\n\nBut I also believe we'll have to support the old school init-manually\nand update-when-I-want-to use cases for a very long time, as lots of\nwork flows are built around that.\n"},{"id":"276080","messageId":"56980BC8.90506@kdbg.org","threadId":"41183","inReplyTo":"xmqq4mehm92b.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] submodule: Port resolve_relative_url from shell to C","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2016-01-14T20:57:44Z","receivedAt":"2016-01-14T20:57:44Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 13.01.2016 um 23:03 schrieb Junio C Hamano:\n> Stefan Beller <sbeller@google.com> writes:\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(_(\"cannot strip one component off url '%s'\"), remoteurl);\n>> +\t\t\t\t\telse\n>> +\t\t\t\t\t\tremoteurl = xstrdup(\".\");\n>> +\t\t\t\t}\n>> +\t\t\t}\n>\n> It is somewhat hard to see how this avoids stripping one (or both)\n> slashes just after \"http:\" in remoteurl=\"http://site/path/\", leaving\n> just \"http:/\" (or \"http:\").\n>\n> This codepath has overly deep nesting levels.  Is this the simplest\n> we can do?\n\nThe code as written is quite easy to follow when compared to the \noriginal shell code. I think that is a reasonable goal, and improvements \ncan into separate patches.\n\n>\n> The final else { if .. else } can be made into else if .. else to\n> dedent the overlong die() by one level, but I am wondering if the\n> deep nesting is just a symptom of logic being unnecessarily complex.\n>\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\nFor example, the section that begins here...\n\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>> +\n>> +\tstrbuf_addf(&sb, \"%s%s\", up_path, out);\n>> +\tfree(out);\n>> +\treturn strbuf_detach(&sb, NULL);\n\n\n... and ends here can easily be rewritten to become a single \nstrbuf_addf() without the xstrdup()s and without the early exit (at the \ncost of some additional ?: conditionals in the arguments).\n\n-- Hannes\n"},{"id":"276089","messageId":"CAGZ79kZ0ooOd+rCXwEiBWgpRbfy6fU+_AOjYbqHP_2qTKQG3Xg@mail.gmail.com","threadId":"41183","inReplyTo":"56980BC8.90506@kdbg.org","subject":"Re: [PATCH] submodule: Port resolve_relative_url from shell to C","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-01-14T22:49:53Z","receivedAt":"2016-01-14T22:49:53Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"> (at the cost of some additional ?: conditionals in the arguments).\n\nI tried that once last year and I could not bring a version which was\neasier to read.\nI'll try again.\n"},{"id":"276090","messageId":"CAGZ79kZZxoD=+GJVPOCuQK_oLqR-pOQw2QM98Yxx3XoGRMAXfQ@mail.gmail.com","threadId":"41183","inReplyTo":"56980A14.1060605@web.de","subject":"Re: [PATCH] submodule: Port resolve_relative_url from shell to C","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-01-14T23:43:27Z","receivedAt":"2016-01-14T23:43:27Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Jan 14, 2016 at 12:50 PM, Jens Lehmann <Jens.Lehmann@web.de> wrote:\n> Am 13.01.2016 um 23:47 schrieb Stefan Beller:\n>>\n>> On Wed, Jan 13, 2016 at 2:03 PM, Junio C Hamano <gitster@pobox.com> wrote:\n>>>\n>>> Stefan Beller <sbeller@google.com> writes:\n>>>\n>>>> Later on we want to deprecate the `git submodule init` command and make\n>>>> it implicit in other submodule commands.\n>>>\n>>>\n>>> I doubt there is a concensus for \"deprecate\" part to warrant the use\n>>> of \"we want\" here.  I tend to think that the latter half of the\n>>> sentence is uncontroversial, i.e. it is a good idea to make other\n>>> \"submodule\" subcommands internally call it when it makes sense, and\n>>> also make knobs available to other commands like \"clone\" and\n>>> possibly \"checkout\" so that the users do not have to do the\n>>> \"submodule init\" as a separate step, though.\n>>\n>>\n>> Maybe I need to rethink my strategy here and deliver a patch series\n>> which includes a complete port of `submodule init`, and maybe even\n>> options in checkout (and clone) to run `submodule init`. That way the\n>> immediate benefit would be clear on why the series is a good idea.\n>\n>\n> I think that makes lots of sense. It looks to me like clone already\n> has that option (as --recurse-submodules must init the submodules),\n> but it might make sense to add such an option to checkout to init\n> (and then also update) all newly appearing submodules (just like\n> \"git submodule update\" has the --init option for the same purpose).\n\nThe next series I'll send out will replace the shell part of `git\nsubmodule init`\nwith a small wrapper for `git submodule--helper init` which will then have\nthe functionality to initialize submodules. I'll break that C code in a way\nthat we'll end up having a function like:\n\n    void init_submodule(const char *path);\n\nAfter that is donw, I'll try to call this from all the places which currently\ndo setup a child process for init or `update --init`.\n\n>\n>> The current wording is mostly arguing to Jens, how to do the submodule\n>> groups thing later on, but skipping the immediate steps.\n>\n>\n> I really believe that in the future a lot of users will hop on to the\n> automatically-init-and-update-submodules train once we have it (and I\n> think users of the groups feature want to be on that train by default).\n\nRereading old mail I wonder if we had a miss understanding on the groups\nfeature or rather the  automatically-init-submodules feature.\n\nAs far as I understand initializing git submodules, you can do it multiple times\nwithout hurting yourself, i.e. an implementation of update could look like\n\nupdate()\n{\n    auto-init-subs = { }\n    if groups selected:\n        auto-init-subs = {subs selected by groups}\n    foreach uninitialized submodule:\n        if submodule has set auto-init (in superprojects .gitmodule I'd guess)\n            auto-init-subs += {that submodule}\n    if auto-init-subs not empty:\n        git submodule init <auto-init-subs>\n    update-as-we-know-it\n}\n\nand then multiple calls to update() would not hurt.\nThat way we would not need to add any logic to the init sub command as my\nfirst patch series had. There it was more like:\n\nupdate()\n{\n    if groups selected:\n        git submodule init --groups # have the logic inside of init\n    update-as-we-know-it\n}\n\nI think I'll redo the groups patch series as the former now.\n\n>\n> But I also believe we'll have to support the old school init-manually\n> and update-when-I-want-to use cases for a very long time, as lots of\n> work flows are built around that.\n\nSure, the \"submodule init\" command is not going away. I just want to have\nan easy way to access it from within C code, hence the rewrite effort.\n\nThanks,\nStefan\n"},{"id":"276177","messageId":"xmqqwpraiw15.fsf@gitster.mtv.corp.google.com","threadId":"41183","inReplyTo":"CAGZ79ka0rxYK7GRSjh13XOsg887EgqYtc5B60z9qU=tAoJGERQ@mail.gmail.com","subject":"Re: [PATCH] submodule: Port resolve_relative_url from shell to C","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-01-15T17:37:26Z","receivedAt":"2016-01-15T17:37:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> On Wed, Jan 13, 2016 at 2:03 PM, Junio C Hamano <gitster@pobox.com> wrote:\n>> Stefan Beller <sbeller@google.com> writes:\n>>> +     while (url) {\n>>> +             if (starts_with_dot_dot_slash(url)) {\n>>> +                     char *rfind;\n>>> +                     url += 3;\n>>> +\n>>> +                     rfind = last_dir_separator(remoteurl);\n>>> +                     if (rfind)\n>>> +                             *rfind = '\\0';\n>>> +                     else {\n>>> +                             rfind = strrchr(remoteurl, ':');\n>>> +                             if (rfind) {\n>>> +                                     *rfind = '\\0';\n>>> +                                     colonsep = 1;\n>>> +                             } else {\n>>> +                                     if (is_relative || !strcmp(\".\", remoteurl))\n>>> +                                             die(_(\"cannot strip one component off url '%s'\"), remoteurl);\n>>> +                                     else\n>>> +                                             remoteurl = xstrdup(\".\");\n>>> +                             }\n>>> +                     }\n>>\n>> It is somewhat hard to see how this avoids stripping one (or both)\n>> slashes just after \"http:\" in remoteurl=\"http://site/path/\", leaving\n>> just \"http:/\" (or \"http:\").\n>\n> it would leave just 'http:/' if url were to be ../../some/where/else,\n> such that the constructed url below would be http://some/where/else.\n\nIs that a good outcome, though?  Isn't it something we would want to\ncatch as an error?\n"},{"id":"276214","messageId":"CAGZ79kaBLmwfeMocKP+tQmqNLy0BDYTU9dFtMY6rmiTqNSi_Dg@mail.gmail.com","threadId":"41183","inReplyTo":"xmqqwpraiw15.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] submodule: Port resolve_relative_url from shell to C","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-01-15T22:58:47Z","receivedAt":"2016-01-15T22:58:47Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, Jan 15, 2016 at 9:37 AM, Junio C Hamano <gitster@pobox.com> wrote:\n> Stefan Beller <sbeller@google.com> writes:\n>\n>> On Wed, Jan 13, 2016 at 2:03 PM, Junio C Hamano <gitster@pobox.com> wrote:\n>>> Stefan Beller <sbeller@google.com> writes:\n>>>> +     while (url) {\n>>>> +             if (starts_with_dot_dot_slash(url)) {\n>>>> +                     char *rfind;\n>>>> +                     url += 3;\n>>>> +\n>>>> +                     rfind = last_dir_separator(remoteurl);\n>>>> +                     if (rfind)\n>>>> +                             *rfind = '\\0';\n>>>> +                     else {\n>>>> +                             rfind = strrchr(remoteurl, ':');\n>>>> +                             if (rfind) {\n>>>> +                                     *rfind = '\\0';\n>>>> +                                     colonsep = 1;\n>>>> +                             } else {\n>>>> +                                     if (is_relative || !strcmp(\".\", remoteurl))\n>>>> +                                             die(_(\"cannot strip one component off url '%s'\"), remoteurl);\n>>>> +                                     else\n>>>> +                                             remoteurl = xstrdup(\".\");\n>>>> +                             }\n>>>> +                     }\n>>>\n>>> It is somewhat hard to see how this avoids stripping one (or both)\n>>> slashes just after \"http:\" in remoteurl=\"http://site/path/\", leaving\n>>> just \"http:/\" (or \"http:\").\n>>\n>> it would leave just 'http:/' if url were to be ../../some/where/else,\n>> such that the constructed url below would be http://some/where/else.\n>\n> Is that a good outcome, though?  Isn't it something we would want to\n> catch as an error?\n\nI would want to add theses checks later and for now\njust port over the code from shell to C. (The same issue\nis found in the shell code and nobody seems to bother so far)\n"},{"id":"276215","messageId":"xmqqlh7qfnt6.fsf@gitster.mtv.corp.google.com","threadId":"41183","inReplyTo":"CAGZ79kaBLmwfeMocKP+tQmqNLy0BDYTU9dFtMY6rmiTqNSi_Dg@mail.gmail.com","subject":"Re: [PATCH] submodule: Port resolve_relative_url from shell to C","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-01-15T23:03:17Z","receivedAt":"2016-01-15T23:03:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> On Fri, Jan 15, 2016 at 9:37 AM, Junio C Hamano <gitster@pobox.com> wrote:\n>>>> It is somewhat hard to see how this avoids stripping one (or both)\n>>>> slashes just after \"http:\" in remoteurl=\"http://site/path/\", leaving\n>>>> just \"http:/\" (or \"http:\").\n>>>\n>>> it would leave just 'http:/' if url were to be ../../some/where/else,\n>>> such that the constructed url below would be http://some/where/else.\n>>\n>> Is that a good outcome, though?  Isn't it something we would want to\n>> catch as an error?\n>\n> I would want to add theses checks later and for now\n> just port over the code from shell to C. (The same issue\n> is found in the shell code and nobody seems to bother so far)\n\nUnderstood and I think that is a good direction to go.  Perhaps\nleave a comment in the area to document it as a known bug (or a\nNEEDSWORK) to make it more obvious and to help remember it?\n\nThanks.\n"}]}