{"thread":{"id":"44322","subject":"[PATCH] submodule--helper: normalize funny urls","startedAt":"2016-10-18T17:53:01Z","lastAt":"2016-10-18T19:49:49Z","messageCount":2,"participants":["Stefan Beller","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"304354","messageId":"20161018175247.28326-1-sbeller@google.com","threadId":"44322","inReplyTo":null,"subject":"[PATCH] submodule--helper: normalize funny urls","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-10-18T17:52:47Z","receivedAt":"2016-10-18T17:53:01Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"The remote URL for the submodule can be specified relative\nto the URL of the superproject in .gitmodules.  A top-level\ngit://site.xz/toplevel.git can specify in its .gitmodules\n\n        [submodule \"sub\"]\n                url = ../submodule.git\n                path = sub\n\nto say that git://site.xz/submodule.git is where the\nsubmodule bound at its \"sub/\" is found.\n\nHowever, when the toplevel is cloned like this:\n\n        git clone git://site.xz/toplevel.git/. top\n\ni.e. the toplevel specifies its URL with trailing \"/.\", the\ncode set the URL to git://site.xz/toplevel.git/submodule.git\nfor the submodule, which is nonsense.  This was because the\nlogic failed to treat trailing \"/.\" any differently from\ntrailing \"/<anything-without-slash>\" when resolving a\nrelative URL \"../<something>\" off of it.  Stripping \"/.\" at\nthe end does *not* take you one level up, even though\nstripping \"/<anything-without-slash>\" does!\n\nSome users may rely on this by always cloning with '/.' and having\nan additional '../' in the relative path for the submodule, and this\npatch breaks them. So why introduce this patch?\n\nThe fix in c12922024 (submodule: ignore trailing slash on superproject\nURL, 2016-10-10) and prior discussion revealed, that Git and Git\nfor Windows treat URLs differently, as currently Git for Windows\nstrips off a trailing dot from paths when calling a Git binary\nunlike when running a shell. Which means Git for Windows is already\ndoing the right thing for the case mentioned above, but it would fail\nour current tests, that test for the broken behavior and it would\nconfuse users working across platforms. So we'd rather fix it\nin Git to ignore any of these trailing no ops in the path properly.\n\nWe never produce the URLs with a trailing '/.' in Git ourselves,\nthey come to us, because the user used it as the URL for cloning\na superproject. Normalize these paths.\n\nHelped-by: Junio C Hamano <gitster@pobox.com>\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n\n * reworded the commit message, taken from Junio, but added more explanation\n   why we want to introduce this patch. \n * added the length check\n * use an infinite loop with break instead of a variable\n   to determine the ending condition.\n\n builtin/submodule--helper.c | 48 +++++++++++++++++++++++++++++++++------------\n t/t0060-path-utils.sh       | 11 +++++++----\n 2 files changed, 43 insertions(+), 16 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 260f46f..ac03cb3 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -76,6 +76,29 @@ static int chop_last_dir(char **remoteurl, int is_relative)\n \treturn 0;\n }\n \n+static void strip_url_ending(char *url, size_t *_len)\n+{\n+\tsize_t len = _len ? *_len : strlen(url);\n+\n+\tfor (;;) {\n+\t\tif (len > 1 && is_dir_sep(url[len-2]) && url[len-1] == '.') {\n+\t\t\turl[len-2] = '\\0';\n+\t\t\tlen -= 2;\n+\t\t\tcontinue;\n+\t\t}\n+\t\tif (len > 0 && is_dir_sep(url[len-1])) {\n+\t\t\turl[len-1] = '\\0';\n+\t\t\tlen --;\n+\t\t\tcontinue;\n+\t\t}\n+\n+\t\tbreak;\n+\t}\n+\n+\tif (_len)\n+\t\t*_len = len;\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@@ -93,14 +116,16 @@ static int chop_last_dir(char **remoteurl, int is_relative)\n  * the superproject working tree otherwise.\n  *\n  * NEEDSWORK: This works incorrectly on the domain and protocol part.\n- * remote_url      url              outcome          expectation\n- * http://a.com/b  ../c             http://a.com/c   as is\n- * http://a.com/b/ ../c             http://a.com/c   same as previous line, but\n- *                                                   ignore trailing slash in url\n- * http://a.com/b  ../../c          http://c         error out\n- * http://a.com/b  ../../../c       http:/c          error out\n- * http://a.com/b  ../../../../c    http:c           error out\n- * http://a.com/b  ../../../../../c    .:c           error out\n+ * remote_url       url              outcome          expectation\n+ * http://a.com/b   ../c             http://a.com/c   as is\n+ * http://a.com/b/  ../c             http://a.com/c   same as previous line, but\n+ *                                                    ignore trailing '/' in url\n+ * http://a.com/b/. ../c             http://a.com/c   same as previous line, but\n+ *                                                    ignore trailing '/.' in url\n+ * http://a.com/b   ../../c          http://c         error out\n+ * http://a.com/b   ../../../c       http:/c          error out\n+ * http://a.com/b   ../../../../c    http:c           error out\n+ * http://a.com/b   ../../../../../c    .:c           error out\n  * NEEDSWORK: Given how chop_last_dir() works, this function is broken\n  * when a local part has a colon in its path component, too.\n  */\n@@ -115,8 +140,7 @@ static char *relative_url(const char *remote_url,\n \tstruct strbuf sb = STRBUF_INIT;\n \tsize_t len = strlen(remoteurl);\n \n-\tif (is_dir_sep(remoteurl[len-1]))\n-\t\tremoteurl[len-1] = '\\0';\n+\tstrip_url_ending(remoteurl, &len);\n \n \tif (!url_is_local_not_ssh(remoteurl) || is_absolute_path(remoteurl))\n \t\tis_relative = 0;\n@@ -149,10 +173,10 @@ static char *relative_url(const char *remote_url,\n \t}\n \tstrbuf_reset(&sb);\n \tstrbuf_addf(&sb, \"%s%s%s\", remoteurl, colonsep ? \":\" : \"/\", url);\n-\tif (ends_with(url, \"/\"))\n-\t\tstrbuf_setlen(&sb, sb.len - 1);\n \tfree(remoteurl);\n \n+\tstrip_url_ending(sb.buf, &sb.len);\n+\n \tif (starts_with_dot_slash(sb.buf))\n \t\tout = xstrdup(sb.buf + 2);\n \telse\ndiff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh\nindex 25b48e5..e154e5f 100755\n--- a/t/t0060-path-utils.sh\n+++ b/t/t0060-path-utils.sh\n@@ -329,14 +329,17 @@ test_submodule_relative_url \"(null)\" \"./foo\" \"../submodule\" \"submodule\"\n test_submodule_relative_url \"(null)\" \"//somewhere else/repo\" \"../subrepo\" \"//somewhere else/subrepo\"\n test_submodule_relative_url \"(null)\" \"$PWD/subsuper_update_r\" \"../subsubsuper_update_r\" \"$(pwd)/subsubsuper_update_r\"\n test_submodule_relative_url \"(null)\" \"$PWD/super_update_r2\" \"../subsuper_update_r\" \"$(pwd)/subsuper_update_r\"\n-test_submodule_relative_url \"(null)\" \"$PWD/.\" \"../.\" \"$(pwd)/.\"\n-test_submodule_relative_url \"(null)\" \"$PWD\" \"./.\" \"$(pwd)/.\"\n+test_submodule_relative_url \"(null)\" \"$PWD/sub/.\" \"../.\" \"$(pwd)\"\n+test_submodule_relative_url \"(null)\" \"$PWD/sub/./.\" \"../.\" \"$(pwd)\"\n+test_submodule_relative_url \"(null)\" \"$PWD/sub/.////././/./.\" \"../.\" \"$(pwd)\"\n+test_submodule_relative_url \"(null)\" \"$PWD\" \"./.\" \"$(pwd)\"\n test_submodule_relative_url \"(null)\" \"$PWD/addtest\" \"../repo\" \"$(pwd)/repo\"\n test_submodule_relative_url \"(null)\" \"$PWD\" \"./å äö\" \"$(pwd)/å äö\"\n-test_submodule_relative_url \"(null)\" \"$PWD/.\" \"../submodule\" \"$(pwd)/submodule\"\n+test_submodule_relative_url \"(null)\" \"$PWD/sub\" \"../submodule\" \"$(pwd)/submodule\"\n+test_submodule_relative_url \"(null)\" \"$PWD/sub/.\" \"../submodule\" \"$(pwd)/submodule\"\n test_submodule_relative_url \"(null)\" \"$PWD/submodule\" \"../submodule\" \"$(pwd)/submodule\"\n test_submodule_relative_url \"(null)\" \"$PWD/home2/../remote\" \"../bundle1\" \"$(pwd)/home2/../bundle1\"\n-test_submodule_relative_url \"(null)\" \"$PWD/submodule_update_repo\" \"./.\" \"$(pwd)/submodule_update_repo/.\"\n+test_submodule_relative_url \"(null)\" \"$PWD/submodule_update_repo\" \"./.\" \"$(pwd)/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 \"(null)\" \"foo\" \"../submodule\" \"submodule\"\n-- \n2.10.1.480.g573bd76\n\n"},{"id":"304358","messageId":"xmqq8ttlphd7.fsf@gitster.mtv.corp.google.com","threadId":"44322","inReplyTo":"20161018175247.28326-1-sbeller@google.com","subject":"Re: [PATCH] submodule--helper: normalize funny urls","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-10-18T19:49:40Z","receivedAt":"2016-10-18T19:49:49Z","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> Some users may rely on this by always cloning with '/.' and having\n> an additional '../' in the relative path for the submodule, and this\n> patch breaks them. So why introduce this patch?\n>\n> The fix in c12922024 (submodule: ignore trailing slash on superproject\n> URL, 2016-10-10) and prior discussion revealed, that Git and Git\n> for Windows treat URLs differently, as currently Git for Windows\n> strips off a trailing dot from paths when calling a Git binary\n> unlike when running a shell. Which means Git for Windows is already\n> doing the right thing for the case mentioned above, but it would fail\n> our current tests, that test for the broken behavior and it would\n> confuse users working across platforms. So we'd rather fix it\n> in Git to ignore any of these trailing no ops in the path properly.\n>\n> We never produce the URLs with a trailing '/.' in Git ourselves,\n> they come to us, because the user used it as the URL for cloning\n> a superproject. Normalize these paths.\n>\n> Helped-by: Junio C Hamano <gitster@pobox.com>\n> Signed-off-by: Stefan Beller <sbeller@google.com>\n> ---\n>\n>  * reworded the commit message, taken from Junio, but added more explanation\n>    why we want to introduce this patch. \n\nThe additional explanation is very good.\n\n> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\n> index 260f46f..ac03cb3 100644\n> --- a/builtin/submodule--helper.c\n> +++ b/builtin/submodule--helper.c\n> @@ -76,6 +76,29 @@ static int chop_last_dir(char **remoteurl, int is_relative)\n>  \treturn 0;\n>  }\n>  \n> +static void strip_url_ending(char *url, size_t *_len)\n> +{\n> +\tsize_t len = _len ? *_len : strlen(url);\n\nStare at our codebase and you'd notice that we avoid using names\nwith leading underscore deliberately and use trailing one instead\nwhen we name a throw-away name like this.  Let's do the same here.\nI.e.\n\n\tstatic void strip_url_ending(char *url, size_t *len_)\n\t{\n\t\tsize_t len = len_ ? *len_ : strlen(url);\n\n> +\tfor (;;) {\n> +\t\tif (len > 1 && is_dir_sep(url[len-2]) && url[len-1] == '.') {\n> +\t\t\turl[len-2] = '\\0';\n\n\"len-1\" and \"len-2\" are usually spelled with SP on both sides of\nbinary operators.\n\n> +\t\t\tlen -= 2;\n> +\t\t\tcontinue;\n> +\t\t}\n> +\t\tif (len > 0 && is_dir_sep(url[len-1])) {\n> +\t\t\turl[len-1] = '\\0';\n> +\t\t\tlen --;\n\nAnd post-decrement sticks to whatever it is decrementing without SP.\n\n> +\t\t\tcontinue;\n> +\t\t}\n"}]}