{"thread":{"id":"32012","subject":"git commit/push can fail silently when clone omits \".git\"","startedAt":"2012-11-04T19:50:58Z","lastAt":"2012-11-13T08:35:58Z","messageCount":7,"participants":["Jeffrey S. Haemer","Jeff King","Heiko Voigt"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"202521","messageId":"CAABvdFyn=_2JKHYA_jAduoNAti3U0YFHbdU94esm=m8R0s2LcA@mail.gmail.com","threadId":"32012","inReplyTo":null,"subject":"git commit/push can fail silently when clone omits \".git\"","fromName":"Jeffrey S. Haemer","fromEmail":"jeffrey.haemer@gmail.com","sentAt":"2012-11-04T19:50:58Z","receivedAt":"2012-11-04T19:50:58Z","isPatch":false,"sender":{"key":"jeffrey.haemer@gmail.com","avatar":null},"body":"Ladies and Gentlemen,\n\nI'm running git 1.7.9.5 on Ubuntu 12.04.1 LTS\n\nI got bitten by what follows. Yes, it's an edge case. Yes I now understand\nwhy it does what it does. Yes the right answer is \"Don't do that, Jeff.\" :-)\n\nStill, it took me a little time to figure out what I'd done wrong because\nthe failure is silent, so I thought I'd document it. Perhaps there's even\nsome way to issue an error message for cases like this.\n\nThe attached test script shows the issue in detail, but here's the basic\nfailure:\n\n$ ls\nhello.git\n$ git clone hello # *Mistake!* Succeeds, but should have cloned \"hello.git\"\nor into something else.\n$ cd hello; touch foo; git add foo; git commit -am\"add a new file\"\n$ git status # says I'm a rev ahead of the origin\n$ git push # nothing pushed\n$ git status # says everything's okay\n\nAt this point hello/foo still exists, there's nothing to commit, git diff\norigin/master reports nothing, yet foo was never pushed to hello.git.\n\nHTH!\n\n--\nJeffrey Haemer <jeffrey.haemer@gmail.com>\n720-837-8908 [cell], http://seejeffrun.blogspot.com [blog],\nhttp://www.youtube.com/user/goyishekop [vlog]\nפרייהייט? דאס איז יאַנג דינען וואָרט.\n"},{"id":"202663","messageId":"20121108185643.GN15560@sigill.intra.peff.net","threadId":"32012","inReplyTo":"CAABvdFyn=_2JKHYA_jAduoNAti3U0YFHbdU94esm=m8R0s2LcA@mail.gmail.com","subject":"Re: git commit/push can fail silently when clone omits \".git\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-11-08T18:56:43Z","receivedAt":"2012-11-08T18:56:43Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Nov 04, 2012 at 12:50:58PM -0700, Jeffrey S. Haemer wrote:\n\n> I got bitten by what follows. Yes, it's an edge case. Yes I now understand\n> why it does what it does. Yes the right answer is \"Don't do that, Jeff.\" :-)\n> \n> Still, it took me a little time to figure out what I'd done wrong because\n> the failure is silent, so I thought I'd document it. Perhaps there's even\n> some way to issue an error message for cases like this.\n> \n> The attached test script shows the issue in detail, but here's the basic\n> failure:\n> \n> $ ls\n> hello.git\n> $ git clone hello # *Mistake!* Succeeds, but should have cloned \"hello.git\"\n> or into something else.\n\nIt does clone hello.git into \"hello\", but it sets remote.origin.url in\nthe cloned repository to \"/path/to/hello\". I.e., to itself, rather than\nthe correct hello.git.\n\nThe reason is that \"clone\" sets the config from the repo name you gave\nit, not the path it finds on disk. The name you gave was not ambiguous\nat the time of clone, but it became so during the clone. I am tempted to\nsay that we should set the config to the path we found on disk, not what\nthe user gave us. That includes the ugly \"/.git\" for non-bare repos, but\nwe should be able to safely strip that off without adding any ambiguity\n(i.e, it is only \"foo\" versus \"foo.git\" that is ambiguous).\n\nUnfortunately, the patch below which does that seems to make t7407 very\nunhappy. It looks like the submodule test uses \"git clone .\" and\n\"git-submodule add\" expects the \"/.\" to still be at the end of the\nconfigured URL when processing relative submodule paths. I'm not sure if\nthat is important, or an unnecessary brittleness in the submodule code.\n\nJens, Heiko?\n\n---\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex 0d663e3..687d5c0 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -713,8 +713,12 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \trepo_name = argv[0];\n \n \tpath = get_repo_path(repo_name, &is_bundle);\n-\tif (path)\n-\t\trepo = xstrdup(absolute_path(repo_name));\n+\tif (path) {\n+\t\tif (!suffixcmp(path, \"/.git\"))\n+\t\t\trepo = xstrndup(path, strlen(path) - 5);\n+\t\telse\n+\t\t\trepo = xstrdup(path);\n+\t}\n \telse if (!strchr(repo_name, ':'))\n \t\tdie(_(\"repository '%s' does not exist\"), repo_name);\n \telse\ndiff --git a/t/t5601-clone.sh b/t/t5601-clone.sh\nindex 67869b4..0eeeb2d 100755\n--- a/t/t5601-clone.sh\n+++ b/t/t5601-clone.sh\n@@ -280,4 +280,20 @@ test_expect_success 'clone checking out a tag' '\n \ttest_cmp fetch.expected fetch.actual\n '\n \n+test_expect_success 'clone does not create ambiguous config' '\n+\tgit init --bare ambiguous.git &&\n+\tgit clone ambiguous &&\n+\t(\n+\t\tcd ambiguous &&\n+\t\ttest_commit one &&\n+\t\tgit push --all\n+\t) &&\n+\techo one >expect &&\n+\t(\n+\t\tcd ambiguous.git &&\n+\t\tgit log -1 --format=%s\n+\t) >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n"},{"id":"202713","messageId":"20121109184225.GA1190@book.hvoigt.net","threadId":"32012","inReplyTo":"20121108185643.GN15560@sigill.intra.peff.net","subject":"Re: git commit/push can fail silently when clone omits \".git\"","fromName":"Heiko Voigt","fromEmail":"hvoigt@hvoigt.net","sentAt":"2012-11-09T18:42:27Z","receivedAt":"2012-11-09T18:42:27Z","isPatch":false,"sender":{"key":"hvoigt@hvoigt.net","avatar":"https://avatars.githubusercontent.com/u/184958?v=4"},"body":"Hi,\n\nOn Thu, Nov 08, 2012 at 01:56:43PM -0500, Jeff King wrote:\n> Unfortunately, the patch below which does that seems to make t7407 very\n> unhappy. It looks like the submodule test uses \"git clone .\" and\n> \"git-submodule add\" expects the \"/.\" to still be at the end of the\n> configured URL when processing relative submodule paths. I'm not sure if\n> that is important, or an unnecessary brittleness in the submodule code.\n> \n> Jens, Heiko?\n\nAfter some analysis it seems to me that the test deviates from the\nexpected behavior. For relative urls we have documented that if we have\na remote in the superproject a relative submodule path is relative to that\nremotes url.\n\nIn the test super has been cloned from \".\". So the tests root directory\nshould be the directory the submodule path is relative to. That would\nbe ./submodule (since submodule is also in the root directory) and not\n../submodule.\n\nBefore your patch a \"/.\" was added to the origin of super and \"/.\" is\ncurrently counted as a path component.\n\nSo we have another corner case here: When your superproject was cloned\nfrom \".\" the urls you currently have to specify with submodule add are\nwrong (one \"..\" to much).\n\nSince this is a change in behaviour I would like to further think about\nthe implications this brings if we fix this. Not sure how many people\nclone from \".\". The correct behavior (as documented) is the one you\nintroduce with your patch. If we decide to fix this we should also correct\nthe path calculation in git-submodule.sh.\n\nCheers Heiko\n"},{"id":"203060","messageId":"20121113083233.GA38188@book.hvoigt.net","threadId":"32012","inReplyTo":"20121109184225.GA1190@book.hvoigt.net","subject":"[PATCH 0/3] fix cloning superprojects from \".\"","fromName":"Heiko Voigt","fromEmail":"hvoigt@hvoigt.net","sentAt":"2012-11-13T08:32:33Z","receivedAt":"2012-11-13T08:32:33Z","isPatch":true,"sender":{"key":"hvoigt@hvoigt.net","avatar":"https://avatars.githubusercontent.com/u/184958?v=4"},"body":"Hi,\n\nOn Fri, Nov 09, 2012 at 07:42:26PM +0100, Heiko Voigt wrote:\n> Since this is a change in behaviour I would like to further think about\n> the implications this brings if we fix this. Not sure how many people\n> clone from \".\". The correct behavior (as documented) is the one you\n> introduce with your patch. If we decide to fix this we should also correct\n> the path calculation in git-submodule.sh.\n\nOk I think this corner case is not that commonly used since most people\nwork with remote remotes which you can not cd into to clone from \".\".\n\nHere is a patch series to clean this handling up.\n\nCheers Heiko\n\nHeiko Voigt (3):\n  Fix relative submodule setup of submodule tests\n  ensure that relative submodule url needs ./ or ../\n  fix corner case for relative submodule path calculation\n\n git-submodule.sh             | 22 +++++++++++++++++\n t/t7400-submodule-basic.sh   | 56 ++++++++++++++++++++++++++++++++++++++++++++\n t/t7403-submodule-sync.sh    |  2 ++\n t/t7406-submodule-update.sh  |  2 ++\n t/t7407-submodule-foreach.sh |  2 ++\n t/t7506-status-submodule.sh  |  2 ++\n 6 files changed, 86 insertions(+)\n\n-- \n1.8.0.3.gaed4666\n"},{"id":"203061","messageId":"20121113083428.GB38188@book.hvoigt.net","threadId":"32012","inReplyTo":"20121113083233.GA38188@book.hvoigt.net","subject":"[PATCH 1/3] Fix relative submodule setup of submodule tests","fromName":"Heiko Voigt","fromEmail":"hvoigt@hvoigt.net","sentAt":"2012-11-13T08:34:28Z","receivedAt":"2012-11-13T08:34:28Z","isPatch":true,"sender":{"key":"hvoigt@hvoigt.net","avatar":"https://avatars.githubusercontent.com/u/184958?v=4"},"body":"If a remote is configured in a superproject relative submodule urls\nshould be relative to that remote. Since we have a bug in relative\npath calculation for superproject paths that contain a \"/.\" using\n../submodule was accepted here. We are going to fix this behavior so\nwe first need to correct these tests.\n\nLater tests expect the submodules origin to be in a directory underneath\nthe tests root. Lets remove the origin from super (which points directly\nat the tests root directory) to keep these tests expectations.\n\nSigned-off-by: Heiko Voigt <hvoigt@hvoigt.net>\n---\n t/t7403-submodule-sync.sh    | 2 ++\n t/t7406-submodule-update.sh  | 2 ++\n t/t7407-submodule-foreach.sh | 2 ++\n t/t7506-status-submodule.sh  | 2 ++\n 4 files changed, 8 insertions(+)\n\ndiff --git a/t/t7403-submodule-sync.sh b/t/t7403-submodule-sync.sh\nindex 524d5c1..b310a58 100755\n--- a/t/t7403-submodule-sync.sh\n+++ b/t/t7403-submodule-sync.sh\n@@ -18,6 +18,8 @@ test_expect_success setup '\n \tgit clone . super &&\n \tgit clone super submodule &&\n \t(cd super &&\n+\t # relative submodule urls relate to this folder not the remotes\n+\t git remote rm origin &&\n \t git submodule add ../submodule submodule &&\n \t test_tick &&\n \t git commit -m \"submodule\"\ndiff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh\nindex 1542653..f3628c9 100755\n--- a/t/t7406-submodule-update.sh\n+++ b/t/t7406-submodule-update.sh\n@@ -32,6 +32,8 @@ test_expect_success 'setup a submodule tree' '\n \tgit clone super merging &&\n \tgit clone super none &&\n \t(cd super &&\n+\t # relative submodule urls relate to this folder not the remotes\n+\t git remote rm origin &&\n \t git submodule add ../submodule submodule &&\n \t test_tick &&\n \t git commit -m \"submodule\" &&\ndiff --git a/t/t7407-submodule-foreach.sh b/t/t7407-submodule-foreach.sh\nindex 9b69fe2..99956a6 100755\n--- a/t/t7407-submodule-foreach.sh\n+++ b/t/t7407-submodule-foreach.sh\n@@ -21,6 +21,8 @@ test_expect_success 'setup a submodule tree' '\n \tgit clone super submodule &&\n \t(\n \t\tcd super &&\n+\t\t# relative submodule urls relate to this folder not the remotes\n+\t\tgit remote rm origin &&\n \t\tgit submodule add ../submodule sub1 &&\n \t\tgit submodule add ../submodule sub2 &&\n \t\tgit submodule add ../submodule sub3 &&\ndiff --git a/t/t7506-status-submodule.sh b/t/t7506-status-submodule.sh\nindex d31b34d..9021b1a 100755\n--- a/t/t7506-status-submodule.sh\n+++ b/t/t7506-status-submodule.sh\n@@ -203,6 +203,8 @@ test_expect_success 'status with merge conflict in .gitmodules' '\n \ttest_create_repo_with_commit sub2 &&\n \t(\n \t\tcd super &&\n+\t\t# relative submodule urls relate to this folder not the remotes\n+\t\tgit remote rm origin &&\n \t\tprev=$(git rev-parse HEAD) &&\n \t\tgit checkout -b add_sub1 &&\n \t\tgit submodule add ../sub1 &&\n-- \n1.8.0.3.gaed4666\n"},{"id":"203062","messageId":"20121113083510.GC38188@book.hvoigt.net","threadId":"32012","inReplyTo":"20121113083233.GA38188@book.hvoigt.net","subject":"[PATCH 2/3] ensure that relative submodule url needs ./ or ../","fromName":"Heiko Voigt","fromEmail":"hvoigt@hvoigt.net","sentAt":"2012-11-13T08:35:10Z","receivedAt":"2012-11-13T08:35:10Z","isPatch":true,"sender":{"key":"hvoigt@hvoigt.net","avatar":"https://avatars.githubusercontent.com/u/184958?v=4"},"body":"Even though a relative path can be without them the\ndocumentation explicitely talks about them. Lets ensure\nthat behavior with a test.\n\nSigned-off-by: Heiko Voigt <hvoigt@hvoigt.net>\n---\n t/t7400-submodule-basic.sh | 12 ++++++++++++\n 1 file changed, 12 insertions(+)\n\ndiff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh\nindex 5397037..3c2afa6 100755\n--- a/t/t7400-submodule-basic.sh\n+++ b/t/t7400-submodule-basic.sh\n@@ -506,6 +506,18 @@ test_expect_success 'set up for relative path tests' '\n \t)\n '\n \n+test_expect_success 'subrepo is NOT considered a relative path\"' '\n+\t(\n+\t\tcd reltest &&\n+\t\tcp pristine-.git-config .git/config &&\n+\t\tcp pristine-.gitmodules .gitmodules &&\n+\t\tgit config -f .gitmodules submodule.sub.url \"subrepo\" &&\n+\t\tgit config remote.origin.url \"$submodurl\" &&\n+\t\tgit submodule init &&\n+\t\ttest \"$(git config submodule.sub.url)\" = subrepo\n+\t)\n+'\n+\n test_expect_success '../subrepo works with URL - ssh://hostname/repo' '\n \t(\n \t\tcd reltest &&\n-- \n1.8.0.3.gaed4666\n"},{"id":"203063","messageId":"20121113083558.GD38188@book.hvoigt.net","threadId":"32012","inReplyTo":"20121113083233.GA38188@book.hvoigt.net","subject":"[PATCH 3/3] fix corner case for relative submodule path calculation","fromName":"Heiko Voigt","fromEmail":"hvoigt@hvoigt.net","sentAt":"2012-11-13T08:35:58Z","receivedAt":"2012-11-13T08:35:58Z","isPatch":true,"sender":{"key":"hvoigt@hvoigt.net","avatar":"https://avatars.githubusercontent.com/u/184958?v=4"},"body":"A trailing /. for the superprojects origin is treated as\na full path component. This is wrong. Lets add a test and\nfix this.\n\nSigned-off-by: Heiko Voigt <hvoigt@hvoigt.net>\n---\n git-submodule.sh           | 22 ++++++++++++++++++++++\n t/t7400-submodule-basic.sh | 44 ++++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 66 insertions(+)\n\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex ab6b110..9f61a9c 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -69,6 +69,28 @@ resolve_relative_url ()\n \t\t;;\n \tesac\n \n+\t# strip one dot path components\n+\ttempurl=\"$remoteurl\"\n+\tremoteurl=\n+\tsep=\n+\twhile test -n \"$tempurl\"\n+\tdo\n+\t\tcase \"$tempurl\" in\n+\t\t*/.)\n+\t\t\ttempurl=\"${tempurl%/.}\"\n+\t\t\t;;\n+\t\t?*/*)\n+\t\t\tremoteurl=\"${tempurl##*/}$sep$remoteurl\"\n+\t\t\ttempurl=\"${tempurl%/*}\"\n+\t\t\tsep=/\n+\t\t;;\n+\t\t*)\n+\t\t\tremoteurl=\"$tempurl$sep$remoteurl\"\n+\t\t\ttempurl=\n+\t\t\t;;\n+\t\tesac\n+\tdone\n+\n \twhile test -n \"$url\"\n \tdo\n \t\tcase \"$url\" in\ndiff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh\nindex 3c2afa6..1b4cc00 100755\n--- a/t/t7400-submodule-basic.sh\n+++ b/t/t7400-submodule-basic.sh\n@@ -518,6 +518,50 @@ test_expect_success 'subrepo is NOT considered a relative path\"' '\n \t)\n '\n \n+test_expect_success '../subrepo works with absolute local path - \"$submodurl/repo/.\"' '\n+\t(\n+\t\tcd reltest &&\n+\t\tcp pristine-.git-config .git/config &&\n+\t\tcp pristine-.gitmodules .gitmodules &&\n+\t\tgit config remote.origin.url \"$submodurl/repo/.\" &&\n+\t\tgit submodule init &&\n+\t\ttest \"$(git config submodule.sub.url)\" = \"$submodurl/subrepo\"\n+\t)\n+'\n+\n+test_expect_success '../subrepo works with absolute local path - \"$submodurl/repo/./\"' '\n+\t(\n+\t\tcd reltest &&\n+\t\tcp pristine-.git-config .git/config &&\n+\t\tcp pristine-.gitmodules .gitmodules &&\n+\t\tgit config remote.origin.url \"$submodurl/repo/./\" &&\n+\t\tgit submodule init &&\n+\t\ttest \"$(git config submodule.sub.url)\" = \"$submodurl/subrepo\"\n+\t)\n+'\n+\n+test_expect_success '../subrepo works with absolute local path - \"$submodurl/./repo/.\"' '\n+\t(\n+\t\tcd reltest &&\n+\t\tcp pristine-.git-config .git/config &&\n+\t\tcp pristine-.gitmodules .gitmodules &&\n+\t\tgit config remote.origin.url \"$submodurl/./repo/.\" &&\n+\t\tgit submodule init &&\n+\t\ttest \"$(git config submodule.sub.url)\" = \"$submodurl/subrepo\"\n+\t)\n+'\n+\n+test_expect_success '../subrepo works with absolute local path - \"$submodurl/././repo/.\"' '\n+\t(\n+\t\tcd reltest &&\n+\t\tcp pristine-.git-config .git/config &&\n+\t\tcp pristine-.gitmodules .gitmodules &&\n+\t\tgit config remote.origin.url \"$submodurl/././repo/.\" &&\n+\t\tgit submodule init &&\n+\t\ttest \"$(git config submodule.sub.url)\" = \"$submodurl/subrepo\"\n+\t)\n+'\n+\n test_expect_success '../subrepo works with URL - ssh://hostname/repo' '\n \t(\n \t\tcd reltest &&\n-- \n1.8.0.3.gaed4666\n"}]}