{"thread":{"id":"44316","subject":"[PATCH] submodule--helper: normalize funny urls","startedAt":"2016-10-17T22:17:23Z","lastAt":"2016-10-18T17:51:12Z","messageCount":8,"participants":["Stefan Beller","Junio C Hamano","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"304313","messageId":"20161017221623.7299-1-sbeller@google.com","threadId":"44316","inReplyTo":null,"subject":"[PATCH] submodule--helper: normalize funny urls","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-10-17T22:16:23Z","receivedAt":"2016-10-17T22:17:23Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Currently a URL for the superproject ending in\n\n(A) .../path/to/dir\n(B) .../path/to/dir/\n(C) .../path/to/dir/.\n(D) .../path/to/dir/./.\n(E) .../path/to/dir/.///.//.\n\nis treated the same in (A) and (B), but (C, D, E) are different.\n\nWe never produce the URLs in (C,D,E) ourselves, they come to use, because\nthe user used it as the URL for cloning a superproject.\nNormalize these paths.\n\nSigned-off-by: Stefan Beller \n---\n\n By being strict in Git, I think we also fix the Git for Windows painpoints.\n \n This goes on top of origin/sb/submodule-ignore-trailing-slash.\n \n Thanks,\n Stefan\n\n builtin/submodule--helper.c | 49 ++++++++++++++++++++++++++++++++++-----------\n t/t0060-path-utils.sh | 11 ++++++----\n 2 files changed, 44 insertions(+), 16 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 260f46f..ca90763 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -76,6 +76,30 @@ 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+\tint check_url_stripping = 1;\n+\tsize_t len = _len ? *_len : strlen(url);\n+\n+\twhile (check_url_stripping) {\n+\t\tcheck_url_stripping = 0;\n+\t\tif (is_dir_sep(url[len-2]) && url[len-1] == '.') {\n+\t\t\turl[len-2] = '\\0';\n+\t\t\tlen -= 2;\n+\t\t\tcheck_url_stripping = 1;\n+\t\t}\n+\n+\t\tif (is_dir_sep(url[len-1])) {\n+\t\t\turl[len-1] = '\\0';\n+\t\t\tlen --;\n+\t\t\tcheck_url_stripping = 1;\n+\t\t}\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 +117,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 +141,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 +174,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":"304318","messageId":"xmqqbmyisiae.fsf@gitster.mtv.corp.google.com","threadId":"44316","inReplyTo":"20161017221623.7299-1-sbeller@google.com","subject":"Re: [PATCH] submodule--helper: normalize funny urls","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-10-17T22:49:13Z","receivedAt":"2016-10-17T22:49:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> +static void strip_url_ending(char *url, size_t *_len)\n> +{\n> +\tint check_url_stripping = 1;\n> +\tsize_t len = _len ? *_len : strlen(url);\n> +\n> +\twhile (check_url_stripping) {\n> +\t\tcheck_url_stripping = 0;\n> +\t\tif (is_dir_sep(url[len-2]) && url[len-1] == '.') {\n\nThis is \"strip /. at the end\" it seems.\n\nDoes anything in the loop control guarantees 2 <= len at this point?\n\n> +\t\t\turl[len-2] = '\\0';\n> +\t\t\tlen -= 2;\n> +\t\t\tcheck_url_stripping = 1;\n> +\t\t}\n> +\n> +\t\tif (is_dir_sep(url[len-1])) {\n\nThis is \"strip / at the end\" it seems.\n\nDoes anything in the loop control guarantees 1 <= len at this point?\n\n> +\t\t\turl[len-1] = '\\0';\n> +\t\t\tlen--;\n> +\t\t\tcheck_url_stripping = 1;\n> +\t\t}\n> +\t}\n"},{"id":"304320","messageId":"CAGZ79kaE=KAWcDTEvAqGO=-zzjh3VeSwHH82SnhK6DO9THOyPA@mail.gmail.com","threadId":"44316","inReplyTo":"xmqqbmyisiae.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] submodule--helper: normalize funny urls","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-10-17T22:50:41Z","receivedAt":"2016-10-17T22:50:46Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Mon, Oct 17, 2016 at 3:49 PM, Junio C Hamano wrote:\n> Stefan Beller writes:\n>\n>> +static void strip_url_ending(char *url, size_t *_len)\n>> +{\n>> + int check_url_stripping = 1;\n>> + size_t len = _len ? *_len : strlen(url);\n>> +\n>> + while (check_url_stripping) {\n>> + check_url_stripping = 0;\n>> + if (is_dir_sep(url[len-2]) && url[len-1] == '.') {\n>\n> This is \"strip /. at the end\" it seems.\n>\n> Does anything in the loop control guarantees 2 <= len at this point?\n\nOh, thanks for pointing that out. I thought about that and missed to add it.\nI'll reroll with the length check once we hear back from Windows folks,\nthat this is a viable strategy for them, too.\n\nThanks,\nStefan\n\n>\n>> + url[len-2] = '\\0';\n>> + len -= 2;\n>> + check_url_stripping = 1;\n>> + }\n>> +\n>> + if (is_dir_sep(url[len-1])) {\n>\n> This is \"strip / at the end\" it seems.\n>\n> Does anything in the loop control guarantees 1 <= len at this point?\n>\n>> + url[len-1] = '\\0';\n>> + len--;\n>> + check_url_stripping = 1;\n>> + }\n>> + }\n"},{"id":"304329","messageId":"alpine.DEB.2.20.1610181323401.197091@virtualbox","threadId":"44316","inReplyTo":"CAGZ79kaE=KAWcDTEvAqGO=-zzjh3VeSwHH82SnhK6DO9THOyPA@mail.gmail.com","subject":"Re: [PATCH] submodule--helper: normalize funny urls","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-10-18T11:23:56Z","receivedAt":"2016-10-18T11:24:49Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Stefan,\n\nOn Mon, 17 Oct 2016, Stefan Beller wrote:\n\n> On Mon, Oct 17, 2016 at 3:49 PM, Junio C Hamano wrote:\n> > Stefan Beller writes:\n> >\n> >> +static void strip_url_ending(char *url, size_t *_len)\n> >> +{\n> >> + int check_url_stripping = 1;\n> >> + size_t len = _len ? *_len : strlen(url);\n> >> +\n> >> + while (check_url_stripping) {\n> >> + check_url_stripping = 0;\n> >> + if (is_dir_sep(url[len-2]) && url[len-1] == '.') {\n> >\n> > This is \"strip /. at the end\" it seems.\n> >\n> > Does anything in the loop control guarantees 2 <= len at this point?\n> \n> Oh, thanks for pointing that out. I thought about that and missed to add it.\n> I'll reroll with the length check once we hear back from Windows folks,\n> that this is a viable strategy for them, too.\n\nIt is a viable strategy for me, too.\n\nThanks,\nDscho\n"},{"id":"304340","messageId":"xmqqy41lr87v.fsf@gitster.mtv.corp.google.com","threadId":"44316","inReplyTo":"20161017221623.7299-1-sbeller@google.com","subject":"Re: [PATCH] submodule--helper: normalize funny urls","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-10-18T15:24:20Z","receivedAt":"2016-10-18T15:24:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> Currently a URL for the superproject ending in\n>\n> (A) .../path/to/dir\n> (B) .../path/to/dir/\n> (C) .../path/to/dir/.\n> (D) .../path/to/dir/./.\n> (E) .../path/to/dir/.///.//.\n>\n> is treated the same in (A) and (B), but (C, D, E) are different.\n\nYou may know what you meant to say with \"treated\", but the readers\ndo not know \"treated\" in what situation you are talking about. We\nneed to tell the readers that the bug being fixed is about resolving\na relative URL \"../\" off of the URL of the superproject\nto compute the remote URL for a submodule repository.\n\n> We never produce the URLs in (C,D,E) ourselves, they come to use, because\n> the user used it as the URL for cloning a superproject.\n> Normalize these paths.\n\nAs you know the externally-visible impact of this change (which I\nasked you, but didn't see an on-list answer to, by the way), please\ndescribe what this means to the end user in the log message. It is\nnormally done in an earlier part of the log message to describe the\nproblem being solved and its background.\n\nIf I understand the issue correctly, it may go like this:\n\n\tThe remote URL for the submodule can be specified relative\n\tto the URL of the superproject in .gitmodules. A top-level\n\tgit://site.xz/toplevel.git can specify in its .gitmodules\n\n\t\t[submodule \"sub\"]\n\t\t\turl = ../submodule.git\n\t\t\tpath = sub\n\n\tto say that git://site.xz/submodule.git is where the\n\tsubmodule bound at its \"sub/\" is found.\n\n\tHowever, when the toplevel is cloned like this:\n\n\t\tgit clone git://site.xz/toplevel.git/. top\n\n\ti.e. the toplevel specifies its URL with trailing \"/.\", the\n\tcode set the URL to git://site.xz/toplevel.git/submodule.git\n\tfor the submodule, which is nonsense. This was because the\n\tlogic failed to treat trailing \"/.\" any differently from\n\ttrailing \"/\" when resolving a\n\trelative URL \"../\" off of it. Stripping \"/.\" at\n\tthe end does *not* take you one level up, even though\n\tstripping \"/\" does!\n\t\nAnd then describe the solution/bugfix, listing A-C (or A-E) and\ntelling that these will be treated the same way.\n\n"},{"id":"304350","messageId":"xmqqr37dpoip.fsf@gitster.mtv.corp.google.com","threadId":"44316","inReplyTo":"xmqqbmyisiae.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] submodule--helper: normalize funny urls","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-10-18T17:15:10Z","receivedAt":"2016-10-18T17:15:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Stefan Beller writes:\n>\n>> +static void strip_url_ending(char *url, size_t *_len)\n>> +{\n>> +\tint check_url_stripping = 1;\n>> +\tsize_t len = _len ? *_len : strlen(url);\n>> +\n>> +\twhile (check_url_stripping) {\n>> +\t\tcheck_url_stripping = 0;\n>> +\t\tif (is_dir_sep(url[len-2]) && url[len-1] == '.') {\n>\n> This is \"strip /. at the end\" it seems.\n>\n> Does anything in the loop control guarantees 2 <= len at this point?\n>\n>> +\t\t\turl[len-2] = '\\0';\n>> +\t\t\tlen -= 2;\n>> +\t\t\tcheck_url_stripping = 1;\n>> +\t\t}\n>> +\n>> +\t\tif (is_dir_sep(url[len-1])) {\n>\n> This is \"strip / at the end\" it seems.\n>\n> Does anything in the loop control guarantees 1 <= len at this point?\n>\n>> +\t\t\turl[len-1] = '\\0';\n>> +\t\t\tlen--;\n>> +\t\t\tcheck_url_stripping = 1;\n>> +\t\t}\n>> +\t}\n\nI also somehow find the \"check-url-stripping\" variable ugly.\n\n\twhile (URL still has something that could be stripped) {\n\t\tif (ends with \"/.\") {\n\t\t\tstrip \"/.\";\n\t\t\tcontinue;\n\t\t}\n\t\tif (ends with \"/\") {\n\t\t\tstrip \"/\";\n\t\t\tcontinue;\n\t\t}\n\t\tbreak;\n\t}\n\nperhaps?\n"},{"id":"304351","messageId":"CAGZ79kZEFanPpdrE4sD64LJHmyrBiHafo1VzoGtoGh+wO_HCuw@mail.gmail.com","threadId":"44316","inReplyTo":"xmqqr37dpoip.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] submodule--helper: normalize funny urls","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-10-18T17:17:44Z","receivedAt":"2016-10-18T17:17:50Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Oct 18, 2016 at 10:15 AM, Junio C Hamano wrote:\n>\n> I also somehow find the \"check-url-stripping\" variable ugly.\n>\n> while (URL still has something that could be stripped) {\n\nfor(;;) {\n\nhere ? (this code would not need a variable, and\nfor wins over while:\n$ git grep \"while (1)\" |wc -l\n107\n$ git grep \"for (;;)\" |wc -l\n128\n)\n\n> if (ends with \"/.\") {\n> strip \"/.\";\n> continue;\n> }\n> if (ends with \"/\") {\n> strip \"/\";\n> continue;\n> }\n> break;\n> }\n>\n> perhaps?\n"},{"id":"304353","messageId":"xmqqinsppmuv.fsf@gitster.mtv.corp.google.com","threadId":"44316","inReplyTo":"CAGZ79kZEFanPpdrE4sD64LJHmyrBiHafo1VzoGtoGh+wO_HCuw@mail.gmail.com","subject":"Re: [PATCH] submodule--helper: normalize funny urls","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-10-18T17:51:04Z","receivedAt":"2016-10-18T17:51:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> for(;;) {\n>\n> here ? (this code would not need a variable, and\n> for wins over while:\n> $ git grep \"while (1)\" |wc -l\n> 107\n> $ git grep \"for (;;)\" |wc -l\n> 128\n> )\n\nI dunno; the numbers tells me there is no strong preference by wide\nmargin either way.\n\nI am not sure if the end shape does not really need an exit\ncondition. If there is a need for one, \"for (; condition;)\" would\nlook strange. If there isn't, \"for (;;)\" is actually my personal\npreference over \"while (1)\".\n\n"}]}