{"thread":{"id":"35035","subject":"[PATCH] clone: tighten \"local paths with colons\" check a bit","startedAt":"2013-09-27T13:48:13Z","lastAt":"2013-09-28T13:14:42Z","messageCount":4,"participants":["Nguyễn Thái Ngọc Duy","Jeff King","Jonathan Nieder","Torsten Bögershausen"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"228316","messageId":"1380289693-593-1-git-send-email-pclouds@gmail.com","threadId":"35035","inReplyTo":null,"subject":"[PATCH] clone: tighten \"local paths with colons\" check a bit","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2013-09-27T13:48:13Z","receivedAt":"2013-09-27T13:48:13Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"commit 6000334 (clone: allow cloning local paths with colons in them -\n2013-05-04) is added to make it possible to specify a path that has\ncolons in it without file://, e.g. ../foo:bar/somewhere. But the check\nis a bit loose.\n\nConsider the url '[foo]:bar', the '[]' unwrapping code will turn the\nstring to 'foo\\0:bar'. The effect of this new string is the same as\n'foo/:bar' to the expression \"path < strchrnul(host, '/')\", which\nmistakes it as a sign of local paths while it's actually not.\n\nMake sure we only check so when no protocol is specified and the url\nis not started with '['.\n\nNoticed-by: Morten Stenshorne <mstensho@opera.com>\nSigned-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n I wanted to add a test then realized there were no ssh tests in the\n test suite. So laziness won :p\n\n connect.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/connect.c b/connect.c\nindex a0783d4..303f850 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -551,7 +551,7 @@ struct child_process *git_connect(int fd[2], const char *url_orig,\n \tpath = strchr(end, c);\n \tif (path && !has_dos_drive_prefix(end)) {\n \t\tif (c == ':') {\n-\t\t\tif (path < strchrnul(host, '/')) {\n+\t\t\tif (host != url || path < strchrnul(host, '/')) {\n \t\t\t\tprotocol = PROTO_SSH;\n \t\t\t\t*path++ = '\\0';\n \t\t\t} else /* '/' in the host part, assume local path */\n-- \n1.8.2.83.gc99314b\n"},{"id":"228330","messageId":"20130927185603.GA2917@sigill.intra.peff.net","threadId":"35035","inReplyTo":"1380289693-593-1-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH] clone: tighten \"local paths with colons\" check a bit","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-09-27T18:56:03Z","receivedAt":"2013-09-27T18:56:03Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Sep 27, 2013 at 08:48:13PM +0700, Nguyen Thai Ngoc Duy wrote:\n\n> ---\n>  I wanted to add a test then realized there were no ssh tests in the\n>  test suite. So laziness won :p\n\nThere is one in t5602, but it's not very reusable. How about squashing\nin the patch below, which does a basic ssh-works test, and confirms your\nfix?\n\n---\ndiff --git a/t/t5601-clone.sh b/t/t5601-clone.sh\nindex 0629149..a3e3d48 100755\n--- a/t/t5601-clone.sh\n+++ b/t/t5601-clone.sh\n@@ -280,9 +280,53 @@ test_expect_success NOT_MINGW,NOT_CYGWIN 'clone local path foo:bar' '\n \ttest_cmp fetch.expected fetch.actual\n '\n \n+test_expect_success 'setup ssh wrapper' '\n+\twrite_script \"$TRASH_DIRECTORY/ssh-wrapper\" <<-\\EOF &&\n+\techo >>\"$TRASH_DIRECTORY/ssh-output\" \"ssh: $*\" &&\n+\t# throw away all but the last argument, which should be the\n+\t# command\n+\twhile test $# -gt 1; do shift; done\n+\teval \"$1\"\n+\tEOF\n+\n+\tGIT_SSH=\"$TRASH_DIRECTORY/ssh-wrapper\" &&\n+\texport GIT_SSH &&\n+\texport TRASH_DIRECTORY\n+'\n+\n+clear_ssh () {\n+\t>\"$TRASH_DIRECTORY/ssh-output\"\n+}\n+\n+expect_ssh () {\n+\t{\n+\t\tcase \"$1\" in\n+\t\tnone)\n+\t\t\t;;\n+\t\t*)\n+\t\t\techo \"ssh: $1 git-upload-pack '$2'\"\n+\t\tesac\n+\t} >\"$TRASH_DIRECTORY/ssh-expect\" &&\n+\t(cd \"$TRASH_DIRECTORY\" && test_cmp ssh-expect ssh-output)\n+}\n+\n+test_expect_success 'cloning myhost:src uses ssh' '\n+\tclear_ssh &&\n+\tgit clone myhost:src ssh-clone &&\n+\texpect_ssh myhost src\n+'\n+\n test_expect_success NOT_MINGW,NOT_CYGWIN 'clone local path foo:bar' '\n+\tclear_ssh &&\n \tcp -R src \"foo:bar\" &&\n-\tgit clone \"./foo:bar\" foobar\n+\tgit clone \"./foo:bar\" foobar &&\n+\texpect_ssh none\n+'\n+\n+test_expect_success 'bracketed hostnames are still ssh' '\n+\tclear_ssh &&\n+\tgit clone \"[myhost:123]:src\" ssh-bracket-clone &&\n+\texpect_ssh myhost:123 src\n '\n \n test_done\n"},{"id":"228333","messageId":"20130927215631.GN9464@google.com","threadId":"35035","inReplyTo":"1380289693-593-1-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH] clone: tighten \"local paths with colons\" check a bit","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-09-27T21:56:31Z","receivedAt":"2013-09-27T21:56:31Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Nguyễn Thái Ngọc Duy wrote:\n\n> commit 6000334 (clone: allow cloning local paths with colons in them -\n> 2013-05-04) is added to make it possible to specify a path that has\n> colons in it without file://, e.g. ../foo:bar/somewhere. But the check\n> is a bit loose.\n[...]\n> Make sure we only check so when no protocol is specified and the url\n> is not started with '['.\n\nMore precisely, this disables the \"'/' before ':'\" check when the\nurl has been mangled by '[]' unwrapping (which only happens if the\nURL starts with '[' and contains an ']' at some point later).\n\nIf I try to clone \"[foo]bar/baz:qux\", after this change it will act as\nthough I specified the remote repository \"foo:qux\" instead of the local\nrepository \"./foo:qux\" as before this change.  Both are wrong ---\nthat's a bug for another day.\n\nThanks, both.\n"},{"id":"228360","messageId":"5246D642.7060203@web.de","threadId":"35035","inReplyTo":"20130927215631.GN9464@google.com","subject":"Re: [PATCH] clone: tighten \"local paths with colons\" check a bit","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2013-09-28T13:14:42Z","receivedAt":"2013-09-28T13:14:42Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On 2013-09-27 23.56, Jonathan Nieder wrote:\n> Nguyễn Thái Ngọc Duy wrote:\n> \n>> commit 6000334 (clone: allow cloning local paths with colons in them -\n>> 2013-05-04) is added to make it possible to specify a path that has\n>> colons in it without file://, e.g. ../foo:bar/somewhere. But the check\n>> is a bit loose.\n> [...]\n>> Make sure we only check so when no protocol is specified and the url\n>> is not started with '['.\n> \n> More precisely, this disables the \"'/' before ':'\" check when the\n> url has been mangled by '[]' unwrapping (which only happens if the\n> URL starts with '[' and contains an ']' at some point later).\n> \n> If I try to clone \"[foo]bar/baz:qux\", after this change it will act as\n> though I specified the remote repository \"foo:qux\" instead of the local\n> repository \"./foo:qux\" as before this change.  Both are wrong ---\n> that's a bug for another day.\n(Loud thinking)\nCould it make sense to disable the SSH autodection logic\nwhenever the url starts with '.' (like in \"../XX.git\") \nor with \"/\" like in /home/USER/projects/XX.git ?\n\ndiff --git a/connect.c b/connect.c\nindex a80ebd3..b382032 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -550,7 +550,8 @@ struct child_process *git_connect(int fd[2], const char *url_orig,\n                end = host;\n \n        path = strchr(end, c);\n-       if (path && !has_dos_drive_prefix(end)) {\n+       if (path && !has_dos_drive_prefix(end) &&\n+           url[0] != '/' && url[0] != '.' ) {\n                if (c == ':') {\n                        if (path < strchrnul(host, '/')) {\n                                protocol = PROTO_SSH;\n"}]}