{"thread":{"id":"44325","subject":"[PATCHv3] submodule--helper: normalize funny urls","startedAt":"2016-10-18T21:06:41Z","lastAt":"2016-10-21T20:56:26Z","messageCount":12,"participants":["Stefan Beller","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"304362","messageId":"20161018210623.32696-1-sbeller@google.com","threadId":"44325","inReplyTo":null,"subject":"[PATCHv3] submodule--helper: normalize funny urls","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-10-18T21:06:23Z","receivedAt":"2016-10-18T21:06:41Z","isPatch":false,"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---\nv3:\n * fixed the coding style.\n\nv2:\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..4f11082 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":"304363","messageId":"xmqqzim1nyz0.fsf@gitster.mtv.corp.google.com","threadId":"44325","inReplyTo":"20161018210623.32696-1-sbeller@google.com","subject":"Re: [PATCHv3] submodule--helper: normalize funny urls","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-10-18T21:12:19Z","receivedAt":"2016-10-18T21:12:31Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> The remote URL for the submodule can be specified relative\n> ...\n> v3:\n>  * fixed the coding style.\n\nAh, thanks.  I had a squash queued on top but will replace with this\none.\n"},{"id":"304365","messageId":"xmqqshrtnynj.fsf@gitster.mtv.corp.google.com","threadId":"44325","inReplyTo":"xmqqzim1nyz0.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCHv3] submodule--helper: normalize funny urls","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-10-18T21:19:12Z","receivedAt":"2016-10-18T21:19:20Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Stefan Beller <sbeller@google.com> writes:\n>\n>> The remote URL for the submodule can be specified relative\n>> ...\n>> v3:\n>>  * fixed the coding style.\n>\n> Ah, thanks.  I had a squash queued on top but will replace with this\n> one.\n\nHeh, I guess I shouldn't have responded before seeing what this\nbreaks.  Applied on top of sb/submodule-ignore-trailing-slash, these\nseem to break.\n\n    t/trash directory.t3600-rm\n    t/trash directory.t7403-submodule-sync\n    t/trash directory.t7406-submodule-update\n    t/trash directory.t7407-submodule-foreach\n    t/trash directory.t7506-status-submodule\n\nSome may be showing broken assumptions of the downstream, two wrongs\ncompensating each other and correcting one exposing breakage of the\nother.  I didn't look at them deeply.\n"},{"id":"304366","messageId":"CAGZ79kZHLVpxbJ_C-dM2LDA64-_TJNyY+52fTWkOvLvvAq2XDg@mail.gmail.com","threadId":"44325","inReplyTo":"xmqqshrtnynj.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCHv3] submodule--helper: normalize funny urls","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-10-18T23:25:08Z","receivedAt":"2016-10-18T23:25:15Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Oct 18, 2016 at 2:19 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Junio C Hamano <gitster@pobox.com> writes:\n>\n>> Stefan Beller <sbeller@google.com> writes:\n>>\n>>> The remote URL for the submodule can be specified relative\n>>> ...\n>>> v3:\n>>>  * fixed the coding style.\n>>\n>> Ah, thanks.  I had a squash queued on top but will replace with this\n>> one.\n>\n> Heh, I guess I shouldn't have responded before seeing what this\n> breaks.  Applied on top of sb/submodule-ignore-trailing-slash, these\n> seem to break.\n\nUgh. (I should have tested more than just t0060).\n\nThe underlying issue is two fold:\n\n* in t3600 we'd need\ndiff --git a/t/t3600-rm.sh b/t/t3600-rm.sh\nindex d046d98..545d32f 100755\n--- a/t/t3600-rm.sh\n+++ b/t/t3600-rm.sh\n@@ -616,7 +616,7 @@ test_expect_success 'setup subsubmodule' '\n        git submodule update &&\n        (cd submod &&\n                git update-index --add --cacheinfo 160000 $(git\nrev-parse HEAD) subsubmod &&\n-               git config -f .gitmodules submodule.sub.url ../. &&\n+               git config -f .gitmodules submodule.sub.url ./. &&\n                git config -f .gitmodules submodule.sub.path subsubmod &&\n                git submodule init &&\n                git add .gitmodules &&\n\nbecause the sub-submodule URL is actually the same as the submodule\n(because we'd test lazily)\n\nThis looks ok from a bug fixers perspective.\n\nHowever in t7403, we have a construct like:\n\n    git clone . super\n\nwhich then results in\n\n    git -C super remote -v\n...../git/t/trash directory.t7403-submodule-sync/. (fetch)\n\nAnd the commit message of this patch claimed we'd never use\nthe /. syntax ourselves. (We could argue the stupid users in the test\nsuite are doing it wrong, because in practice nobody would use clone\nto create a nested repository? Not sure I agree.)\n\nHowever instead of fixing the levels of nesting, the fix is as easy as:\ndiff --git a/t/t7403-submodule-sync.sh b/t/t7403-submodule-sync.sh\nindex 0726799..525d32b 100755\n--- a/t/t7403-submodule-sync.sh\n+++ b/t/t7403-submodule-sync.sh\n@@ -15,7 +15,9 @@ test_expect_success setup '\n        git add file &&\n        test_tick &&\n        git commit -m upstream &&\n-       git clone . super &&\n+       # avoid cloning a repository with a url ending in /.\n+       git clone . root &&\n+       git clone root super &&\n        git clone super submodule &&\n        (\n                cd submodule &&\n\nSame goes for t740{6,7} as well as t7506.\n\nI think this change to the test suite is not warranted, because\nwe want to have the current behavior as-is as it seems like a nice\nhack:\n\n* Maybe we'd want to think about checking for the URL in git clone\n  normalize the URL before configuring remote.origin.URL\n\n* an often observed work flow for submodule tests seems:\n\n    mkdir sub1 &&\n    git -C sub1 init  &&\n    ...\n\n    git clone . super &&\n    git -C super submodule add ../sub1\n    ... # the ../sub1 looks intuitively correct\n    # because from the current directory which is\n    # super the relative path is ../sub1\n    #\n    # However in reality this ought to be a relative URL,\n    # and as super sits in the same directory as sub1\n    # ./sub1 would be \"correct\" according to the documentation\n    # However as the super remote URL ends with /.\n    # we had a bug that we needed to add one layer of unnesting\n    # and that is how ../sub1 worked.\n\n\nNot sure about this patch any more.\n\nStefan\n"},{"id":"304371","messageId":"xmqq60opnolz.fsf@gitster.mtv.corp.google.com","threadId":"44325","inReplyTo":"CAGZ79kZHLVpxbJ_C-dM2LDA64-_TJNyY+52fTWkOvLvvAq2XDg@mail.gmail.com","subject":"Re: [PATCHv3] submodule--helper: normalize funny urls","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-10-19T00:56:08Z","receivedAt":"2016-10-19T00:56:17Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> The underlying issue is two fold:\n>\n> * in t3600 we'd need\n> diff --git a/t/t3600-rm.sh b/t/t3600-rm.sh\n> index d046d98..545d32f 100755\n> --- a/t/t3600-rm.sh\n> +++ b/t/t3600-rm.sh\n> @@ -616,7 +616,7 @@ test_expect_success 'setup subsubmodule' '\n>         git submodule update &&\n>         (cd submod &&\n>                 git update-index --add --cacheinfo 160000 $(git\n> rev-parse HEAD) subsubmod &&\n> -               git config -f .gitmodules submodule.sub.url ../. &&\n> +               git config -f .gitmodules submodule.sub.url ./. &&\n>                 git config -f .gitmodules submodule.sub.path subsubmod &&\n>                 git submodule init &&\n>                 git add .gitmodules &&\n>\n> because the sub-submodule URL is actually the same as the submodule\n> (because we'd test lazily)\n\nThis fix sounds entirely sane.  The \"../.\" you touched was depending\non the buggy behaviour; it is exactly the case of \"there were two\nwrongs that were covering each other; after one of them gets fixed,\nthe other one's brokenness is exposed\", right?\n\n> However in t7403, we have a construct like:\n>\n>     git clone . super\n>\n> which then results in\n>\n>     git -C super remote -v\n> ...../git/t/trash directory.t7403-submodule-sync/. (fetch)\n\nThat sounds expected.  We do not have to add trailing \"/.\", but the\nsystem ought to work with or without it correctly and the same way.\n\n> However instead of fixing the levels of nesting, the fix is as easy as:\n> diff --git a/t/t7403-submodule-sync.sh b/t/t7403-submodule-sync.sh\n> index 0726799..525d32b 100755\n> --- a/t/t7403-submodule-sync.sh\n> +++ b/t/t7403-submodule-sync.sh\n> @@ -15,7 +15,9 @@ test_expect_success setup '\n>         git add file &&\n>         test_tick &&\n>         git commit -m upstream &&\n> -       git clone . super &&\n> +       # avoid cloning a repository with a url ending in /.\n> +       git clone . root &&\n> +       git clone root super &&\n>         git clone super submodule &&\n>         (\n>                 cd submodule &&\n>\n> Same goes for t740{6,7} as well as t7506.\n\nIsn't the issue the same as that \"3600-rm\" one?  I know you said\ntwofold upfront, but I am not sure I agree.\n\nThe \"super\" repository refers to its submodules with \"../submodule\"\nin the test code we have, even though the submodule referred to\nlives inside $TRASH, and by fixing the \"trailing /. and trailing\n/root are treated the same way\" bug, its reference created in the\ntest ends up referring to one level above, perhaps in t/submodule,\ninstead of the intended place t/trash/submodule.  By using \"root\",\nyou are making their wrong references point at the right place.\n\nAdmittedly, the updated test above tests something different from\nwhat it originally wanted to test, which feels somewhat distasteful\nbut I do not think it is wrong.\n\n> I think this change to the test suite is not warranted, because\n> we want to have the current behavior as-is ...\n\nI am not sure.  Certainly we would want to make sure that the normal\ncase (i.e. no funny trailing junk) to work correctly, but we do want\nto protect the fix from future breakage as well, no?\n\nPerhaps we can do a preliminary step to update tests to primarily\ncheck the cases that do not involve URL with trailing \"/.\" by either\ndoing that double clone, or more preferrably, clone from \"$(pwd)\"\nand adjust the incorrect submodule reference that have been relying\non the buggy behaviour.  With that \"root\" trick, the test would pass\nwith or without the fix under discussion, right?\n\nThen do the fix under discussion and introduce a test that clones\nfrom \".\" refers to submodules with relative path and makes sure that\ntrailing \"/.\" is interpreted correctly.\n"},{"id":"304372","messageId":"CAGZ79kYrKGLEOO72aWuX5OOM-AecdFZFXRqBkRzhdAM-VbPFxA@mail.gmail.com","threadId":"44325","inReplyTo":"xmqq60opnolz.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCHv3] submodule--helper: normalize funny urls","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-10-19T01:04:09Z","receivedAt":"2016-10-19T01:04:17Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Oct 18, 2016 at 5:56 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Stefan Beller <sbeller@google.com> writes:\n>\n>> The underlying issue is two fold:\n>>\n>> * in t3600 we'd need\n>> diff --git a/t/t3600-rm.sh b/t/t3600-rm.sh\n>> index d046d98..545d32f 100755\n>> --- a/t/t3600-rm.sh\n>> +++ b/t/t3600-rm.sh\n>> @@ -616,7 +616,7 @@ test_expect_success 'setup subsubmodule' '\n>>         git submodule update &&\n>>         (cd submod &&\n>>                 git update-index --add --cacheinfo 160000 $(git\n>> rev-parse HEAD) subsubmod &&\n>> -               git config -f .gitmodules submodule.sub.url ../. &&\n>> +               git config -f .gitmodules submodule.sub.url ./. &&\n>>                 git config -f .gitmodules submodule.sub.path subsubmod &&\n>>                 git submodule init &&\n>>                 git add .gitmodules &&\n>>\n>> because the sub-submodule URL is actually the same as the submodule\n>> (because we'd test lazily)\n>\n> This fix sounds entirely sane.  The \"../.\" you touched was depending\n> on the buggy behaviour; it is exactly the case of \"there were two\n> wrongs that were covering each other; after one of them gets fixed,\n> the other one's brokenness is exposed\", right?\n>\n>> However in t7403, we have a construct like:\n>>\n>>     git clone . super\n>>\n>> which then results in\n>>\n>>     git -C super remote -v\n>> ...../git/t/trash directory.t7403-submodule-sync/. (fetch)\n>\n> That sounds expected.  We do not have to add trailing \"/.\", but the\n> system ought to work with or without it correctly and the same way.\n>\n>> However instead of fixing the levels of nesting, the fix is as easy as:\n>> diff --git a/t/t7403-submodule-sync.sh b/t/t7403-submodule-sync.sh\n>> index 0726799..525d32b 100755\n>> --- a/t/t7403-submodule-sync.sh\n>> +++ b/t/t7403-submodule-sync.sh\n>> @@ -15,7 +15,9 @@ test_expect_success setup '\n>>         git add file &&\n>>         test_tick &&\n>>         git commit -m upstream &&\n>> -       git clone . super &&\n>> +       # avoid cloning a repository with a url ending in /.\n>> +       git clone . root &&\n>> +       git clone root super &&\n>>         git clone super submodule &&\n>>         (\n>>                 cd submodule &&\n>>\n>> Same goes for t740{6,7} as well as t7506.\n>\n> Isn't the issue the same as that \"3600-rm\" one?  I know you said\n> twofold upfront, but I am not sure I agree.\n\nI took a couple of hours trying to get the same fix applied to the t7* tests,\nbut that doesn't seem to be as easy. I'll try again.\n\n>\n> The \"super\" repository refers to its submodules with \"../submodule\"\n> in the test code we have, even though the submodule referred to\n> lives inside $TRASH, and by fixing the \"trailing /. and trailing\n> /root are treated the same way\" bug, its reference created in the\n> test ends up referring to one level above, perhaps in t/submodule,\n> instead of the intended place t/trash/submodule.  By using \"root\",\n> you are making their wrong references point at the right place.\n\nCorrect.\n\n>\n> Admittedly, the updated test above tests something different from\n> what it originally wanted to test, which feels somewhat distasteful\n> but I do not think it is wrong.\n\nI think it is. I was just showing how to quick fix the issue, and how wide the\nimpact was. More like assessing the situation of what is broken with\nthat patch, rather than proposing a way to go for fixing.\n\n>\n>> I think this change to the test suite is not warranted, because\n>> we want to have the current behavior as-is ...\n>\n> I am not sure.  Certainly we would want to make sure that the normal\n> case (i.e. no funny trailing junk) to work correctly, but we do want\n> to protect the fix from future breakage as well, no?\n\nExactly. So not intermediate \"root\" that we clone from, but adapting the\nrelative URLs. Maybe half the broken tests can switch to 'root' and the others\ngo with the current behavior of cloning . to super.\n\n>\n> Perhaps we can do a preliminary step to update tests to primarily\n> check the cases that do not involve URL with trailing \"/.\" by either\n> doing that double clone, or more preferrably, clone from \"$(pwd)\"\n> and adjust the incorrect submodule reference that have been relying\n> on the buggy behaviour.  With that \"root\" trick, the test would pass\n> with or without the fix under discussion, right?\n\nI assume so, (not tested).\n\n>\n> Then do the fix under discussion and introduce a test that clones\n> from \".\" refers to submodules with relative path and makes sure that\n> trailing \"/.\" is interpreted correctly.\n"},{"id":"304374","messageId":"xmqqwph5m6th.fsf@gitster.mtv.corp.google.com","threadId":"44325","inReplyTo":"CAGZ79kYrKGLEOO72aWuX5OOM-AecdFZFXRqBkRzhdAM-VbPFxA@mail.gmail.com","subject":"Re: [PATCHv3] submodule--helper: normalize funny urls","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-10-19T02:05:46Z","receivedAt":"2016-10-19T02:05:57Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n>> I am not sure.  Certainly we would want to make sure that the normal\n>> case (i.e. no funny trailing junk) to work correctly, but we do want\n>> to protect the fix from future breakage as well, no?\n>\n> Exactly. So not intermediate \"root\" that we clone from, but adapting the\n> relative URLs. Maybe half the broken tests can switch to 'root' and the others\n> go with the current behavior of cloning . to super.\n>>\n>> Perhaps we can do a preliminary step to update tests to primarily\n>> check the cases that do not involve URL with trailing \"/.\" by either\n>> doing that double clone, or more preferrably, clone from \"$(pwd)\"\n>> and adjust the incorrect submodule reference that have been relying\n>> on the buggy behaviour.  With that \"root\" trick, the test would pass\n>> with or without the fix under discussion, right?\n>\n> I assume so, (not tested).\n\nOK.  Thanks for sticking with it.\n"},{"id":"304500","messageId":"CAGZ79kY4u-k8v3g5Jmod4acwJ81AnJYeYMQZ2tarx6UMGj8Xew@mail.gmail.com","threadId":"44325","inReplyTo":"xmqqwph5m6th.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCHv3] submodule--helper: normalize funny urls","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-10-20T19:15:57Z","receivedAt":"2016-10-20T19:16:03Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Oct 18, 2016 at 7:05 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Stefan Beller <sbeller@google.com> writes:\n>\n>>> I am not sure.  Certainly we would want to make sure that the normal\n>>> case (i.e. no funny trailing junk) to work correctly, but we do want\n>>> to protect the fix from future breakage as well, no?\n>>\n>> Exactly. So not intermediate \"root\" that we clone from, but adapting the\n>> relative URLs. Maybe half the broken tests can switch to 'root' and the others\n>> go with the current behavior of cloning . to super.\n>>>\n>>> Perhaps we can do a preliminary step to update tests to primarily\n>>> check the cases that do not involve URL with trailing \"/.\" by either\n>>> doing that double clone, or more preferrably, clone from \"$(pwd)\"\n>>> and adjust the incorrect submodule reference that have been relying\n>>> on the buggy behaviour.  With that \"root\" trick, the test would pass\n>>> with or without the fix under discussion, right?\n>>\n>> I assume so, (not tested).\n>\n> OK.  Thanks for sticking with it.\n\nDo we actually want to fix git-clone as well?\nI tried and then I see breakage in 5603-clone-dirname\nas ssh://host seems to be an invalid url; it has to end with a slash?\n\nIf we were to fix clone as well, then we'd still have a lot of possible stale\ndata (ending in /.) out there, so maybe we want to not fix clone for\nnow and only\nfix it when computing the submodule url.\n\nSo I'll first fix up the test suite to not rely on buggy behavior and\nthen send this patch\nwith no change in tests? That sounds strange to me as it hides away the cause.\n"},{"id":"304501","messageId":"xmqqzilyg6tk.fsf@gitster.mtv.corp.google.com","threadId":"44325","inReplyTo":"CAGZ79kY4u-k8v3g5Jmod4acwJ81AnJYeYMQZ2tarx6UMGj8Xew@mail.gmail.com","subject":"Re: [PATCHv3] submodule--helper: normalize funny urls","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-10-20T19:26:47Z","receivedAt":"2016-10-20T19:27:10Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> Do we actually want to fix git-clone as well?\n\nIf I understand correctly, the point of this fix is to make it not\nto matter whether the original URL the end user gives or recorded as\nthe remote by \"git clone\" in the repository is any one of:\n\n\t$any_leading_part/path/to/dir\n\t$any_leading_part/path/to/dir/\n\t$any_leading_part/path/to/dir/.\n\nSo I do not think there is anything to \"fix\", as long as \"git clone\"\nthat is given any one of the above three records any one of the\nabove three as the result.  It _may_ be desirable if the result is\nidentical what was given as input, but I do not offhand think that\nis required.\n\n> I tried and then I see breakage in 5603-clone-dirname\n> as ssh://host seems to be an invalid url; it has to end with a slash?\n\nThat is a separate issue, isn't it?  We shouldn't be touching the\nleading \"<scheme>://<host>/\" part, I would think.  \n\nFor example, a URL \"../another\" relative to \"ssh://host/path\" may be\n\"ssh://host/another\", but shouldn't it be an error to take\n\"../../outside\" relative to \"ssh://host/path\"?\n"},{"id":"304502","messageId":"CAGZ79kZyCXKnt9kTbnD8KrnsjJVb_7juW2epmdP9XqcRGfg=GA@mail.gmail.com","threadId":"44325","inReplyTo":"xmqqzilyg6tk.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCHv3] submodule--helper: normalize funny urls","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-10-20T19:34:13Z","receivedAt":"2016-10-20T19:34:36Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Oct 20, 2016 at 12:26 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Stefan Beller <sbeller@google.com> writes:\n>\n>> Do we actually want to fix git-clone as well?\n>\n> If I understand correctly, the point of this fix is to make it not\n> to matter whether the original URL the end user gives or recorded as\n> the remote by \"git clone\" in the repository is any one of:\n>\n>         $any_leading_part/path/to/dir\n>         $any_leading_part/path/to/dir/\n>         $any_leading_part/path/to/dir/.\n>\n> So I do not think there is anything to \"fix\", as long as \"git clone\"\n\nYes \"git clone\" works with any of the three above.\n\nMy thought was to fix it nevertheless, such that the url recorded as\nremote.origin.url is always the first case (no l or /. at the end).\n\nIf we were to add this fix to clone, then it may be easier to debug\nsubmodule url schemes for users as the submodule url would then\nbe a concatenation of remote.origin.url and the relative part.\n\nThat seems easier to understand than ${remote.origin.url%%/.} +\nrelative path, maybe? (Because then the user doesn't need to guess\nor remember historical behavior that is wrong on how this)\n\n> that is given any one of the above three records any one of the\n> above three as the result.  It _may_ be desirable if the result is\n> identical what was given as input, but I do not offhand think that\n> is required.\n>\n>> I tried and then I see breakage in 5603-clone-dirname\n>> as ssh://host seems to be an invalid url; it has to end with a slash?\n>\n> That is a separate issue, isn't it?  We shouldn't be touching the\n> leading \"<scheme>://<host>/\" part, I would think.\n\nI agree, So I'll first fix the submodule parts only.\n\n>\n> For example, a URL \"../another\" relative to \"ssh://host/path\" may be\n> \"ssh://host/another\", but shouldn't it be an error to take\n> \"../../outside\" relative to \"ssh://host/path\"?\n\nThat is correct. I'll stop looking at clone code.\n"},{"id":"304503","messageId":"xmqqvawmg5kh.fsf@gitster.mtv.corp.google.com","threadId":"44325","inReplyTo":"CAGZ79kZyCXKnt9kTbnD8KrnsjJVb_7juW2epmdP9XqcRGfg=GA@mail.gmail.com","subject":"Re: [PATCHv3] submodule--helper: normalize funny urls","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-10-20T19:53:50Z","receivedAt":"2016-10-20T19:53:58Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> My thought was to fix it nevertheless, such that the url recorded as\n> remote.origin.url is always the first case (no l or /. at the end).\n>\n> If we were to add this fix to clone, then it may be easier to debug\n> submodule url schemes for users as the submodule url would then\n> be a concatenation of remote.origin.url and the relative part.\n>\n> That seems easier to understand than ${remote.origin.url%%/.} +\n> relative path, maybe? (Because then the user doesn't need to guess\n> or remember historical behavior that is wrong on how this)\n\nAre you declaring that trailing / or /. will now be illegal?  If you\nare declaring that, then I agree that new codepaths no longer have\nto worry about \"strip / or /. before concatenating\" and will\nsimplify things for them.  But otherwise, such a \"fix\" also would\nhave an effect of hiding bugs from codepaths.  They still need to be\nprepared to see any of the three variants and cope with them\ncorrectly, right?\n"},{"id":"304643","messageId":"CAGZ79kYXyUr9qets_xkHU4NekSz2J67=Pheu=fTq5ztx0gRu2w@mail.gmail.com","threadId":"44325","inReplyTo":"xmqqwph5m6th.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCHv3] submodule--helper: normalize funny urls","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-10-21T20:56:19Z","receivedAt":"2016-10-21T20:56:26Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Oct 18, 2016 at 7:05 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Stefan Beller <sbeller@google.com> writes:\n>\n>>> I am not sure.  Certainly we would want to make sure that the normal\n>>> case (i.e. no funny trailing junk) to work correctly, but we do want\n>>> to protect the fix from future breakage as well, no?\n>>\n>> Exactly. So not intermediate \"root\" that we clone from, but adapting the\n>> relative URLs. Maybe half the broken tests can switch to 'root' and the others\n>> go with the current behavior of cloning . to super.\n>>>\n>>> Perhaps we can do a preliminary step to update tests to primarily\n>>> check the cases that do not involve URL with trailing \"/.\" by either\n>>> doing that double clone, or more preferrably, clone from \"$(pwd)\"\n>>> and adjust the incorrect submodule reference that have been relying\n>>> on the buggy behaviour.  With that \"root\" trick, the test would pass\n>>> with or without the fix under discussion, right?\n>>\n>> I assume so, (not tested).\n>\n> OK.  Thanks for sticking with it.\n\nOk, the root trick works fine without the fix, however we preferrably\nwant to fix it\nwithout double cloning, then the fix becomes a bit harder to follow:\n\ninstead of\n\n    git clone . super\n\nwe do\n\n    git clone \"$(pwd)\" super &&\n    git -C super config --unset remote.origin.url &&\n\ninstead, such that the relative urls work the same way.\n(The super becomes its own authoritative source of truth with no upstream.\nIn that case the url is relative to the superproject, e.g.\n\n    git -C super submodule add ../submodule\n\nwill then resolve the relative path from super/../submodule to be just\nas before, where\nwe bugily used $(git config remote.origin.url =\n...super/.)/../submodule, which due to the\nbut resolved to the submodule as well (as /. was counted as a directory).\n"}]}