{"thread":{"id":"44354","subject":"[PATCH 0/3] Fix submodule url issues","startedAt":"2016-10-22T00:00:00Z","lastAt":"2016-10-24T19:47:29Z","messageCount":11,"participants":["Stefan Beller","Johannes Sixt","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"304657","messageId":"20161021235939.20792-1-sbeller@google.com","threadId":"44354","inReplyTo":null,"subject":"[PATCH 0/3] Fix submodule url issues","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-10-21T23:59:36Z","receivedAt":"2016-10-22T00:00:00Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"previous patch: http://public-inbox.org/git/20161018210623.32696-1-sbeller@google.com/\n\nFirst fix our test suite in patch 2,\nthen patch 3 is a resend of the previous patch to normalize the configured\nremote.\n\nThanks,\nStefan\n\nStefan Beller (3):\n  t7506: fix diff order arguments in test_cmp\n  submodule tests: replace cloning from . by \"$(pwd)\"\n  submodule--helper: normalize funny urls\n\n builtin/submodule--helper.c  | 48 +++++++++++++++++++++++++++++++++-----------\n t/t0060-path-utils.sh        | 11 ++++++----\n t/t3600-rm.sh                |  1 +\n t/t7064-wtstatus-pv2.sh      |  9 ++++++---\n t/t7403-submodule-sync.sh    |  3 ++-\n t/t7406-submodule-update.sh  |  6 ++++--\n t/t7407-submodule-foreach.sh |  3 ++-\n t/t7506-status-submodule.sh  |  7 ++++---\n 8 files changed, 62 insertions(+), 26 deletions(-)\n\n-- \n2.10.1.507.g2a9098a\n\n"},{"id":"304659","messageId":"20161021235939.20792-2-sbeller@google.com","threadId":"44354","inReplyTo":"20161021235939.20792-1-sbeller@google.com","subject":"[PATCH 1/3] t7506: fix diff order arguments in test_cmp","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-10-21T23:59:37Z","receivedAt":"2016-10-22T00:00:09Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Fix the argument order for test_cmp. When given the expected\nresult first the diff shows the actual output with '+' and the\nexpectation with '-', which is the convention for our tests.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n t/t7506-status-submodule.sh | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t7506-status-submodule.sh b/t/t7506-status-submodule.sh\nindex d31b34d..74cb6b8 100755\n--- a/t/t7506-status-submodule.sh\n+++ b/t/t7506-status-submodule.sh\n@@ -260,7 +260,7 @@ test_expect_success 'diff with merge conflict in .gitmodules' '\n \t\tcd super &&\n \t\tgit diff >../diff_actual 2>&1\n \t) &&\n-\ttest_cmp diff_actual diff_expect\n+\ttest_cmp diff_expect diff_actual\n '\n \n test_expect_success 'diff --submodule with merge conflict in .gitmodules' '\n@@ -268,7 +268,7 @@ test_expect_success 'diff --submodule with merge conflict in .gitmodules' '\n \t\tcd super &&\n \t\tgit diff --submodule >../diff_submodule_actual 2>&1\n \t) &&\n-\ttest_cmp diff_submodule_actual diff_submodule_expect\n+\ttest_cmp diff_submodule_expect diff_submodule_actual\n '\n \n test_done\n-- \n2.10.1.507.g2a9098a\n\n"},{"id":"304660","messageId":"20161021235939.20792-4-sbeller@google.com","threadId":"44354","inReplyTo":"20161021235939.20792-1-sbeller@google.com","subject":"[PATCH 3/3] submodule--helper: normalize funny urls","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-10-21T23:59:39Z","receivedAt":"2016-10-22T00:00:14Z","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\nThe test 3600 needs a slight adaption as well, and was not covered by\nthe previous patch, because the setup of the submodule in this test\nis different than the \"clone . super\" pattern that was fixed in the\nprevious patch.\n\nThe url configured for the submodule path submod is \"./.\", which\ngets normalized with this patch, such that the nested submodule\n'subsubmod' doesn't resolve its path properly via '../'.\nIn later tests we do not need the url any more as testing of\nremoval of the config including url happens in early tests, so the\neasieast fix for that test is to just make the submodule its own\nauthoritative source of truth by removing the remote url.\n\nHelped-by: Junio C Hamano <gitster@pobox.com>\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n builtin/submodule--helper.c | 48 +++++++++++++++++++++++++++++++++------------\n t/t0060-path-utils.sh       | 11 +++++++----\n t/t3600-rm.sh               |  1 +\n 3 files changed, 44 insertions(+), 16 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 4beeda5..21e2e2a 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\"\ndiff --git a/t/t3600-rm.sh b/t/t3600-rm.sh\nindex d046d98..7839ccd 100755\n--- a/t/t3600-rm.sh\n+++ b/t/t3600-rm.sh\n@@ -615,6 +615,7 @@ test_expect_success 'setup subsubmodule' '\n \tgit reset --hard &&\n \tgit submodule update &&\n \t(cd submod &&\n+\t\tgit config --unset remote.origin.url &&\n \t\tgit update-index --add --cacheinfo 160000 $(git rev-parse HEAD) subsubmod &&\n \t\tgit config -f .gitmodules submodule.sub.url ../. &&\n \t\tgit config -f .gitmodules submodule.sub.path subsubmod &&\n-- \n2.10.1.507.g2a9098a\n\n"},{"id":"304661","messageId":"20161021235939.20792-3-sbeller@google.com","threadId":"44354","inReplyTo":"20161021235939.20792-1-sbeller@google.com","subject":"[PATCH 2/3] submodule tests: replace cloning from . by \"$(pwd)\"","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-10-21T23:59:38Z","receivedAt":"2016-10-22T00:00:18Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"When adding a submodule via \"git submodule add <relative url>\",\nthe relative url applies to the superprojects remote. When the\nsuperproject was cloned via \"git clone . super\", the remote url\nis ending with '/.'.\n\nThe logic to construct the relative urls is not smart enough to\ndetect that the ending /. is referring to the directory itself\nbut rather treats it like any other relative path, i.e.\n\n    path/to/dir/. + ../relative/path/to/submodule\n\nwould result in\n\n    path/to/dir/relative/path/to/submodule\n\nand not omit the \"dir\" as you may expect.\n\nAs in a later patch we'll normalize the remote url before the\ncomputation of relative urls takes place, we need to first get our\ntest suite in a shape with normalized urls only, which is why we should\nrefrain from cloning from '.'\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n t/t7064-wtstatus-pv2.sh      | 9 ++++++---\n t/t7403-submodule-sync.sh    | 3 ++-\n t/t7406-submodule-update.sh  | 6 ++++--\n t/t7407-submodule-foreach.sh | 3 ++-\n t/t7506-status-submodule.sh  | 3 ++-\n 5 files changed, 16 insertions(+), 8 deletions(-)\n\ndiff --git a/t/t7064-wtstatus-pv2.sh b/t/t7064-wtstatus-pv2.sh\nindex 3012a4d..95514bb 100755\n--- a/t/t7064-wtstatus-pv2.sh\n+++ b/t/t7064-wtstatus-pv2.sh\n@@ -330,7 +330,8 @@ test_expect_success 'verify UU (edit-edit) conflict' '\n test_expect_success 'verify upstream fields in branch header' '\n \tgit checkout master &&\n \ttest_when_finished \"rm -rf sub_repo\" &&\n-\tgit clone . sub_repo &&\n+\tgit clone \"$(pwd)\" sub_repo &&\n+\tgit -C sub_repo config --unset remote.origin.url &&\n \t(\n \t\t## Confirm local master tracks remote master.\n \t\tcd sub_repo &&\n@@ -392,8 +393,10 @@ test_expect_success 'verify upstream fields in branch header' '\n \n test_expect_success 'create and add submodule, submodule appears clean (A. S...)' '\n \tgit checkout master &&\n-\tgit clone . sub_repo &&\n-\tgit clone . super_repo &&\n+\tgit clone \"$(pwd)\" sub_repo &&\n+\tgit -C sub_repo config --unset remote.origin.url &&\n+\tgit clone \"$(pwd)\" super_repo &&\n+\tgit -C super_repo config --unset remote.origin.url &&\n \t(\tcd super_repo &&\n \t\tgit submodule add ../sub_repo sub1 &&\n \ndiff --git a/t/t7403-submodule-sync.sh b/t/t7403-submodule-sync.sh\nindex 0726799..6d85391 100755\n--- a/t/t7403-submodule-sync.sh\n+++ b/t/t7403-submodule-sync.sh\n@@ -15,7 +15,8 @@ test_expect_success setup '\n \tgit add file &&\n \ttest_tick &&\n \tgit commit -m upstream &&\n-\tgit clone . super &&\n+\tgit clone \"$(pwd)\" super &&\n+\tgit -C super config --unset remote.origin.url &&\n \tgit clone super submodule &&\n \t(\n \t\tcd submodule &&\ndiff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh\nindex 64f322c..9430c2a 100755\n--- a/t/t7406-submodule-update.sh\n+++ b/t/t7406-submodule-update.sh\n@@ -26,7 +26,8 @@ test_expect_success 'setup a submodule tree' '\n \tgit add file &&\n \ttest_tick &&\n \tgit commit -m upstream &&\n-\tgit clone . super &&\n+\tgit clone \"$(pwd)\" super &&\n+\tgit -C super config --unset remote.origin.url &&\n \tgit clone super submodule &&\n \tgit clone super rebasing &&\n \tgit clone super merging &&\n@@ -64,7 +65,8 @@ test_expect_success 'setup a submodule tree' '\n \t test_tick &&\n \t git commit -m \"none\"\n \t) &&\n-\tgit clone . recursivesuper &&\n+\tgit clone \"$(pwd)\" recursivesuper &&\n+\tgit -C recursivesuper config --unset remote.origin.url &&\n \t( cd recursivesuper\n \t git submodule add ../super super\n \t)\ndiff --git a/t/t7407-submodule-foreach.sh b/t/t7407-submodule-foreach.sh\nindex 6ba5daf..257e817 100755\n--- a/t/t7407-submodule-foreach.sh\n+++ b/t/t7407-submodule-foreach.sh\n@@ -17,7 +17,8 @@ test_expect_success 'setup a submodule tree' '\n \tgit add file &&\n \ttest_tick &&\n \tgit commit -m upstream &&\n-\tgit clone . super &&\n+\tgit clone \"$(pwd)\" super &&\n+\tgit -C super config --unset remote.origin.url &&\n \tgit clone super submodule &&\n \t(\n \t\tcd super &&\ndiff --git a/t/t7506-status-submodule.sh b/t/t7506-status-submodule.sh\nindex 74cb6b8..62a99bc 100755\n--- a/t/t7506-status-submodule.sh\n+++ b/t/t7506-status-submodule.sh\n@@ -197,7 +197,8 @@ A  sub1\n EOF\n \n test_expect_success 'status with merge conflict in .gitmodules' '\n-\tgit clone . super &&\n+\tgit clone \"$(pwd)\" super &&\n+\tgit -C super config --unset remote.origin.url &&\n \ttest_create_repo_with_commit sub1 &&\n \ttest_tick &&\n \ttest_create_repo_with_commit sub2 &&\n-- \n2.10.1.507.g2a9098a\n\n"},{"id":"304673","messageId":"c6c5ce05-3511-a992-e079-316f0ce90ecd@kdbg.org","threadId":"44354","inReplyTo":"20161021235939.20792-3-sbeller@google.com","subject":"Re: [PATCH 2/3] submodule tests: replace cloning from . by \"$(pwd)\"","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2016-10-22T07:09:25Z","receivedAt":"2016-10-22T07:09:35Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 22.10.2016 um 01:59 schrieb Stefan Beller:\n> When adding a submodule via \"git submodule add <relative url>\",\n> the relative url applies to the superprojects remote. When the\n> superproject was cloned via \"git clone . super\", the remote url\n> is ending with '/.'.\n>\n> The logic to construct the relative urls is not smart enough to\n> detect that the ending /. is referring to the directory itself\n> but rather treats it like any other relative path, i.e.\n>\n>     path/to/dir/. + ../relative/path/to/submodule\n>\n> would result in\n>\n>     path/to/dir/relative/path/to/submodule\n>\n> and not omit the \"dir\" as you may expect.\n>\n> As in a later patch we'll normalize the remote url before the\n> computation of relative urls takes place, we need to first get our\n> test suite in a shape with normalized urls only, which is why we should\n> refrain from cloning from '.'\n\nBut you are removing a valid use case from the tests. Aren't you \nsweeping something under the rug with this patch?\n\n>\n> Signed-off-by: Stefan Beller <sbeller@google.com>\n> ---\n>  t/t7064-wtstatus-pv2.sh      | 9 ++++++---\n>  t/t7403-submodule-sync.sh    | 3 ++-\n>  t/t7406-submodule-update.sh  | 6 ++++--\n>  t/t7407-submodule-foreach.sh | 3 ++-\n>  t/t7506-status-submodule.sh  | 3 ++-\n>  5 files changed, 16 insertions(+), 8 deletions(-)\n>\n> diff --git a/t/t7064-wtstatus-pv2.sh b/t/t7064-wtstatus-pv2.sh\n> index 3012a4d..95514bb 100755\n> --- a/t/t7064-wtstatus-pv2.sh\n> +++ b/t/t7064-wtstatus-pv2.sh\n> @@ -330,7 +330,8 @@ test_expect_success 'verify UU (edit-edit) conflict' '\n>  test_expect_success 'verify upstream fields in branch header' '\n>  \tgit checkout master &&\n>  \ttest_when_finished \"rm -rf sub_repo\" &&\n> -\tgit clone . sub_repo &&\n> +\tgit clone \"$(pwd)\" sub_repo &&\n> +\tgit -C sub_repo config --unset remote.origin.url &&\n\nWhy is it necessary to remove this configuration? Is it because when it \nis present, the submodule does not construct its own reference? And if \nso, should it not be sufficient to only remove the configuration \n(without changing 'git clone' command), but move this patch after the \npatch that fixes the /. treatment?\n\n>  \t(\n>  \t\t## Confirm local master tracks remote master.\n>  \t\tcd sub_repo &&\n...\n\n"},{"id":"304674","messageId":"xmqqh984aldl.fsf@gitster.mtv.corp.google.com","threadId":"44354","inReplyTo":"c6c5ce05-3511-a992-e079-316f0ce90ecd@kdbg.org","subject":"Re: [PATCH 2/3] submodule tests: replace cloning from . by \"$(pwd)\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-10-22T07:33:26Z","receivedAt":"2016-10-22T07:33:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt <j6t@kdbg.org> writes:\n\n>> The logic to construct the relative urls is not smart enough to\n>> detect that the ending /. is referring to the directory itself\n>> but rather treats it like any other relative path, i.e.\n>>\n>>     path/to/dir/. + ../relative/path/to/submodule\n>>\n>> would result in\n>>\n>>     path/to/dir/relative/path/to/submodule\n>>\n>> and not omit the \"dir\" as you may expect.\n>>\n>> As in a later patch we'll normalize the remote url before the\n>> computation of relative urls takes place, we need to first get our\n>> test suite in a shape with normalized urls only, which is why we should\n>> refrain from cloning from '.'\n>\n> But you are removing a valid use case from the tests. Aren't you\n> sweeping something under the rug with this patch?\n\nI share the same reaction.\n\nIf the primary problem being solved is that the combination of a\nrelative URL ../sub and the base URL for the superproject which is\nset to /path/to/dir/. (due to \"clone .\") were incorrectly resolved\nas /path/to/dir/sub (because the buggy relative path logic did not\nknow that removing \"/.\" at the end does not take you to one level\nup), and a topic that fixes the bug would make that relative URL\n../sub to be resolved as /path/to/sub, of course.  Otherwise, the\ntopic did not fix the bug.  \n\nNow if a test that wanted to have a clone of the superproject by\n\"clone .\", which results in the base URL of /path/to/dir/., actually\nwants to refer in its .gitmodules to /path/to/dir/sub (which after\nall was where the submodule the test created with or without the\nbugfix), I would think the right adjustment for the test that used\nto rely on the buggy behaviour would be to stop using ../sub and\ninstead use ./sub as the relative URL, no?  After all, the bug forced\nthe original test writer to write ../sub but the submodule in this\ncase actually is at ./sub relative to its superproject, and that is\nwhat the original test writer would have written if the bug weren't\nthere in the first place, no?\n\nAnother thing I do not quite understand is why this step comes\nbefore the fix.  If the \"clone .\" is adjusted to avoid triggering\nthe buggy behaviour, i.e. making the base URL to /path/to/dir\n(instead of /path/to/dir/.), wouldn't the relative URL ../sub that\nwas written to work around the bug that hasn't been fixed yet in\nthis step need to be adjusted anyway?  It would end up referring to\n/path/to/sub and not /path/to/dir/sub until the fix is put in place.\n\nIs the removal of remote.origin.url a wrong workaround for that\nbreakage, I wonder...  I do not understand that change at all, and I\ndo not think it was explained in the log message.\n\nIf we really wanted to update the test before fixing the bug, I\nwould understand if this step changed ../sub (or whatever relative\nURL that has extra ../ only because the base URL has extra /. at the\nend to compensate for the buggy resolution) to ./sub in the tests\nand marked them to expect failure, and then a later step that fixes\nthe bug turns them to expect success without make any other change.\n\n\n"},{"id":"304683","messageId":"CAGZ79kaukGh2ynkOQcF=skzxTMYr8CFRyGJw6FEmNsTAcaG_VQ@mail.gmail.com","threadId":"44354","inReplyTo":"xmqqh984aldl.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 2/3] submodule tests: replace cloning from . by \"$(pwd)\"","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-10-22T20:46:12Z","receivedAt":"2016-10-22T20:46:20Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Sat, Oct 22, 2016 at 12:33 AM, Junio C Hamano <gitster@pobox.com> wrote:\n> Johannes Sixt <j6t@kdbg.org> writes:\n>\n>>> The logic to construct the relative urls is not smart enough to\n>>> detect that the ending /. is referring to the directory itself\n>>> but rather treats it like any other relative path, i.e.\n>>>\n>>>     path/to/dir/. + ../relative/path/to/submodule\n>>>\n>>> would result in\n>>>\n>>>     path/to/dir/relative/path/to/submodule\n>>>\n>>> and not omit the \"dir\" as you may expect.\n>>>\n>>> As in a later patch we'll normalize the remote url before the\n>>> computation of relative urls takes place, we need to first get our\n>>> test suite in a shape with normalized urls only, which is why we should\n>>> refrain from cloning from '.'\n>>\n>> But you are removing a valid use case from the tests. Aren't you\n>> sweeping something under the rug with this patch?\n>\n> I share the same reaction.\n\nOh I see. I agree.\nI reverted the lines that replace . by \"$(pwd)\" and it still works, but it\nstill works because the code path regarding local paths was not broken\n(both . as well as \"$(pwd)\" are working without the fix)\n\n>\n> If the primary problem being solved is that the combination of a\n> relative URL ../sub and the base URL for the superproject which is\n> set to /path/to/dir/. (due to \"clone .\") were incorrectly resolved\n> as /path/to/dir/sub (because the buggy relative path logic did not\n> know that removing \"/.\" at the end does not take you to one level\n> up), and a topic that fixes the bug would make that relative URL\n> ../sub to be resolved as /path/to/sub, of course.  Otherwise, the\n> topic did not fix the bug.\n>\n> Now if a test that wanted to have a clone of the superproject by\n> \"clone .\", which results in the base URL of /path/to/dir/., actually\n> wants to refer in its .gitmodules to /path/to/dir/sub (which after\n> all was where the submodule the test created with or without the\n> bugfix), I would think the right adjustment for the test that used\n> to rely on the buggy behaviour would be to stop using ../sub and\n> instead use ./sub as the relative URL, no?  After all, the bug forced\n> the original test writer to write ../sub but the submodule in this\n> case actually is at ./sub relative to its superproject, and that is\n> what the original test writer would have written if the bug weren't\n> there in the first place, no?\n\nTrue.\n\nI have looked into it again, and by now I think the bug is a feature,\nactually.\n\nConsider this:\n\n    git clone . super\n    git -C super submodule add ../submodule\n    # we thought the previous line is buggy\n    git clone super super-clone\n\nNow in the super-clone the ../submodule is the correct\nrelative url, because the url where we cloned from doesn't\nend in /.\n\nIf we were to fix the bug with the /. ending, then we would need the\nfollowing:\n\n    git clone . super\n    git -C super submodule add ./submodule\n    # correct relative URL above!\n\n    git clone super super-clone\n    cd superclone && sed s|url =./|url = ../|\n    # make sure we fix the relative URLs to account for a different remote.\n\nAnd this doesn't seem right to me as it burdens the user of the super-clone.\n\n>\n> Another thing I do not quite understand is why this step comes\n> before the fix.  If the \"clone .\" is adjusted to avoid triggering\n> the buggy behaviour, i.e. making the base URL to /path/to/dir\n> (instead of /path/to/dir/.), wouldn't the relative URL ../sub that\n> was written to work around the bug that hasn't been fixed yet in\n> this step need to be adjusted anyway?  It would end up referring to\n> /path/to/sub and not /path/to/dir/sub until the fix is put in place.\n>\n> Is the removal of remote.origin.url a wrong workaround for that\n> breakage, I wonder...  I do not understand that change at all, and I\n> do not think it was explained in the log message.\n\nI think it is wrong, because it is sidestepping the actual issue.\nContinuing from above:\n\n    git clone super-clone super-clone2\n    # this works again, as the remote change doesn't matter.\n\n    mkdir test && git -C test clone ../ .\n    # submodule urls need to be \"undone again to work:\n    cd test && sed s|url =../|url = ./|\n\nSo I think keeping the /. around as it currently is, is a pretty\nuseful bug.\n\n>\n> If we really wanted to update the test before fixing the bug, I\n> would understand if this step changed ../sub (or whatever relative\n> URL that has extra ../ only because the base URL has extra /. at the\n> end to compensate for the buggy resolution) to ./sub in the tests\n> and marked them to expect failure, and then a later step that fixes\n> the bug turns them to expect success without make any other change.\n\nI'll think about this further, but currently I am inclined to declare\nit a nonbug\nand as it eases testing a lot. Also if someone wants to clone a repository\nwith broken relative urls, they can work around that by e.g.\n\n    git clone <url>/.\n\nto fix one level of indentation, though it is not documented and seems\nto be brittle.\n\nThanks,\nStefan\n"},{"id":"304755","messageId":"61637cd9-8f83-c988-15c0-54f948153c07@kdbg.org","threadId":"44354","inReplyTo":"CAGZ79kaukGh2ynkOQcF=skzxTMYr8CFRyGJw6FEmNsTAcaG_VQ@mail.gmail.com","subject":"Re: [PATCH 2/3] submodule tests: replace cloning from . by \"$(pwd)\"","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2016-10-23T10:14:26Z","receivedAt":"2016-10-23T10:14:36Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 22.10.2016 um 22:46 schrieb Stefan Beller:\n> I have looked into it again, and by now I think the bug is a feature,\n> actually.\n>\n> Consider this:\n>\n>     git clone . super\n>     git -C super submodule add ../submodule\n>     # we thought the previous line is buggy\n>     git clone super super-clone\n\nAt this point, we *should* have this if there were no bugs (at least \nthat is my assumption):\n\n   /tmp\n   !\n   + submodule     <- submodule's remote repo\n   !\n   + foo           <- we are here (.), super's remote repo\n     !\n     + super       <- remote.origin.url=/tmp/foo/.\n       !\n       + submodule <- remote.origin.url=/tmp/foo/./../submodule\n                      submodule.submodule.url=../submodule\n\nWhen I test this, 'git submodule add' fails:\n\nfoo@master> git -C super submodule add ../submodule\nfatal: repository '/tmp/foo/submodule' does not exist\nfatal: clone of '/tmp/foo/submodule' into submodule path \n'/tmp/foo/super/submodule' failed\n\n> Now in the super-clone the ../submodule is the correct\n> relative url, because the url where we cloned from doesn't\n> end in /.\n\nI do not understand why this would be relevant. The question is not how \nthe submodule's remote URL ends, but how the submodule's remote URL is \nconstructed from the super-project's URL and the relative path specified \nfor 'git submodule add'.\n\nWhether ../submodule or ./submodule is the correct relative URL depends \non where the origin of the submodule is located relative to the origin \nof the super-project. In the above example, it is ../submodule. However, \nthe error message tells us that git looked in /tmp/foo/submodule, which \nlooks like the /. bug!\n\nI do not understand where you see a feature here. What am I missing?\n\n-- Hannes\n\n"},{"id":"304780","messageId":"xmqqinsh7i86.fsf@gitster.mtv.corp.google.com","threadId":"44354","inReplyTo":"61637cd9-8f83-c988-15c0-54f948153c07@kdbg.org","subject":"Re: [PATCH 2/3] submodule tests: replace cloning from . by \"$(pwd)\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-10-24T17:46:33Z","receivedAt":"2016-10-24T17:46:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt <j6t@kdbg.org> writes:\n\n> Am 22.10.2016 um 22:46 schrieb Stefan Beller:\n>> I have looked into it again, and by now I think the bug is a feature,\n>> actually.\n>>\n>> Consider this:\n>>\n>>     git clone . super\n>>     git -C super submodule add ../submodule\n>>     # we thought the previous line is buggy\n>>     git clone super super-clone\n>\n> At this point, we *should* have this if there were no bugs (at least\n> that is my assumption):\n>\n>   /tmp\n>   !\n>   + submodule     <- submodule's remote repo\n>   !\n>   + foo           <- we are here (.), super's remote repo\n>     !\n>     + super       <- remote.origin.url=/tmp/foo/.\n>       !\n>       + submodule <- remote.origin.url=/tmp/foo/./../submodule\n>                      submodule.submodule.url=../submodule\n>\n> When I test this, 'git submodule add' fails:\n>\n> foo@master> git -C super submodule add ../submodule\n> fatal: repository '/tmp/foo/submodule' does not exist\n> fatal: clone of '/tmp/foo/submodule' into submodule path\n> '/tmp/foo/super/submodule' failed\n>\n>> Now in the super-clone the ../submodule is the correct\n>> relative url, because the url where we cloned from doesn't\n>> end in /.\n>\n> I do not understand why this would be relevant. The question is not\n> how the submodule's remote URL ends, but how the submodule's remote\n> URL is constructed from the super-project's URL and the relative path\n> specified for 'git submodule add'.\n\nFWIW, that matches my understanding.\n\n> Whether ../submodule or ./submodule is the correct relative URL\n> depends on where the origin of the submodule is located relative to\n> the origin of the super-project. In the above example, it is\n> ../submodule. However, the error message tells us that git looked in\n> /tmp/foo/submodule, which looks like the /. bug!\n>\n> I do not understand where you see a feature here. What am I missing?\n>\n> -- Hannes\n"},{"id":"304792","messageId":"CAGZ79kZrsxjSfa=CSs5V56ePabnD3-W7FHC7KkRO6jytX+GrAw@mail.gmail.com","threadId":"44354","inReplyTo":"61637cd9-8f83-c988-15c0-54f948153c07@kdbg.org","subject":"Re: [PATCH 2/3] submodule tests: replace cloning from . by \"$(pwd)\"","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-10-24T19:10:10Z","receivedAt":"2016-10-24T19:10:35Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Sun, Oct 23, 2016 at 3:14 AM, Johannes Sixt <j6t@kdbg.org> wrote:\n> Am 22.10.2016 um 22:46 schrieb Stefan Beller:\n>>\n>> I have looked into it again, and by now I think the bug is a feature,\n>> actually.\n>>\n>> Consider this:\n>>\n>>     git clone . super\n>>     git -C super submodule add ../submodule\n>>     # we thought the previous line is buggy\n>>     git clone super super-clone\n>\n>\n> At this point, we *should* have this if there were no bugs (at least that is\n> my assumption):\n>\n>   /tmp\n>   !\n>   + submodule     <- submodule's remote repo\n>   !\n>   + foo           <- we are here (.), super's remote repo\n>     !\n>     + super       <- remote.origin.url=/tmp/foo/.\n>       !\n>       + submodule <- remote.origin.url=/tmp/foo/./../submodule\n>                      submodule.submodule.url=../submodule\n>\n> When I test this, 'git submodule add' fails:\n\nYes this setup currently fails because the  /tmp/foo/./../submodule\nis computed to be /tmp/foo/submodule.\n\nIn the test suite the directory \"foo\" is usually called\n\"trash\\ directory.tXXXX-description\" and the remote of\nthe submodule is created inside of it, such that:\n\n/tmp\n   !\n   + foo            <- we are here (.), super's remote repo\n     !\n     + submodule    <- submodule's remote repo\n     !\n     + super        <- remote.origin.url=/tmp/foo/.\n       !\n       + submodule  <- remote.origin.url=/tmp/foo/./../submodule\n                       submodule.submodule.url=../submodule\n                       current result=/tmp/foo/submodule\n\nworks.\n\n>\n> foo@master> git -C super submodule add ../submodule\n> fatal: repository '/tmp/foo/submodule' does not exist\n> fatal: clone of '/tmp/foo/submodule' into submodule path\n> '/tmp/foo/super/submodule' failed\n>\n>> Now in the super-clone the ../submodule is the correct\n>> relative url, because the url where we cloned from doesn't\n>> end in /.\n>\n>\n> I do not understand why this would be relevant. The question is not how the\n> submodule's remote URL ends, but how the submodule's remote URL is\n> constructed from the super-project's URL and the relative path specified for\n> 'git submodule add'.\n\nI was not precise here. If you have the working setup as above, you can clone\nsuper to super-clone and it keeps working, because of the current \"bug\".\n\nThe difference between \"super\" that is cloned from . and \"super-clone\" that\nis cloned from \"super\" is only the remote url, and currently\n/tmp/foo/super\n/tmp/foo/.\n\n+ relative path ../submodule resolve to the same submodule remote.\n\n>\n> Whether ../submodule or ./submodule is the correct relative URL depends on\n> where the origin of the submodule is located relative to the origin of the\n> super-project. In the above example, it is ../submodule. However, the error\n> message tells us that git looked in /tmp/foo/submodule, which looks like the\n> /. bug!\n\nCorrect.\n\nAt the time I was trying to fix the urls in the test suite with the\nbug fix and I then\nrealized that this bugfix introduces a lot of pain to our test suite,\nbecause we do\nthese secondary clones quite a few times. The pattern usually is:\n* setup super (cloned from .)\n* clone super to super-clone\n* trash super-clone for testing purposes and delete it.\n\n>\n> I do not understand where you see a feature here. What am I missing?\n\nThe ease of use in our own testing suite is the feature I was talking about.\n\nWhen we would fix the bug, we would not just need to fix\ns|../submodule|./submodule| when setting up super, but we would need to\nfix it every time again in reverse when creating these short lived\n\"super-clones\"\nthat get tested on and deleted.\n\nSo maybe the correct fix for the testing suite is a double clone, i.e. instead\nof\n\n\n    # prepare some stuff\n    git clone . super\n\nwe rather do:\n\n    # mkdir upstream && prepare stuff in upstream\n    git clone upstream super\n\nHowever that way we would end up not exercising the\ncode path to be fixed with the actual bug fix i.e. we'd never clone from /.\nbecause it is silly conceptually. We could add a new test for that though\nthat only tests that cloning from . constructs the correct URL.\nHowever that is already tested in t0060 resolving the relative URLs\nvia the git submodule helper.\n\nThanks for bearing this discussion,\nI feel like I am missing the obvious point here,\n\nStefan\n"},{"id":"304798","messageId":"0dfa14f7-494d-9be3-fcb7-8c16b1a7fdfa@kdbg.org","threadId":"44354","inReplyTo":"CAGZ79kZrsxjSfa=CSs5V56ePabnD3-W7FHC7KkRO6jytX+GrAw@mail.gmail.com","subject":"Re: [PATCH 2/3] submodule tests: replace cloning from . by \"$(pwd)\"","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2016-10-24T19:47:21Z","receivedAt":"2016-10-24T19:47:29Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 24.10.2016 um 21:10 schrieb Stefan Beller:\n> The ease of use in our own testing suite is the feature I was talking about.\n\nOk, thanks for the clarification. Next steps would then be, IMO, to fix \nthe tests to match the future world order and mark tests that fail due \nto the bug with test_expect_failure, and then switch them back to \ntest_expect_success in the patch with the bug fix.\n\n-- Hannes\n\n"}]}