{"thread":{"id":"35241","subject":"[PATCH V3] git clone: is an URL local or ssh","startedAt":"2013-10-29T21:07:50Z","lastAt":"2013-10-30T07:11:49Z","messageCount":4,"participants":["Torsten Bögershausen","Jeff King","Johannes Sixt"],"isPatch":true,"patchVersion":3,"patchTotal":null},"messages":[{"id":"229747","messageId":"201310292207.50869.tboegi@web.de","threadId":"35241","inReplyTo":null,"subject":"[PATCH V3] git clone: is an URL local or ssh","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2013-10-29T21:07:50Z","receivedAt":"2013-10-29T21:07:50Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"Commit 8d3d28f5 added test cases for URLs which should be ssh.\n\nAdd more tests testing all the combinations:\n -IPv4 or IPv6\n -path starting with \"/\" or with \"/~\"\n -with and without the ssh:// scheme\n\nAdd tests for ssh:// with port number.\n\nWhen a git repository \"foo:bar\" exist, git clone will call\nabsolute_path() and git_connect() will be called with\nsomething like \"/home/user/projects/foo:bar\"\n\nTighten the test and use \"foo:bar\" instead of \"./foo:bar\",\nrefactor clear_ssh() and expect_ssh() into test_clone_url().\n\n\"git clone foo/bar:baz\" should not be ssh:\n  Make the rule\n  \"if there is a slash before the first colon, it is not ssh\"\n  more strict by using the same is_local() in both connect.c\n  and transport.c. Add a test case.\n\nBug fixes for corner cases:\n- Handle clone of [::1]:/~repo correctly (2e7766655):\n  Git should call \"ssh ::1 ~repo\", not ssh ::1 /~repo\n  This was caused by looking at (host != url), which never\n  worked for host names with literal IPv6 adresses, embedded by []\n  Support for the [] URLs was introduced in 356bece0a.\n\n- Do not tamper local URLs starting with '[' which have a ']'\n\nSigned-off-by: Torsten Bögershausen <tboegi@web.de>\n---\n (Thanks for the reviews)\n Changes since V2:\n clear_ssh and expect_ssh did come back\n And I couldn't get it working without the\n >\"$TRASH_DIRECTORY/ssh-output\"\n\n test_when_finished:\n I could not get that to work. Probably because of the\n battle between the quotings: '\"' \"'\" '\"'\n \n Other note about test_might_fail:\n The first version did not need it, git clone did\n always succeed.\n After a while it always failed.\n According to my understanding, git clone ssh://xxx.yy\n should fail (as we can not clone) ??\n But it seems to be a timing problem, anybody wants to comment\n on this ?\n \n\n connect.c        | 47 ++++++++++++++++++++------------\n connect.h        |  1 +\n t/t5601-clone.sh | 81 +++++++++++++++++++++++++++++++++++++++++++++++++-------\n transport.c      |  8 ------\n 4 files changed, 102 insertions(+), 35 deletions(-)\n\ndiff --git a/connect.c b/connect.c\nindex 06e88b0..d61adc9 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -231,6 +231,7 @@ int server_supports(const char *feature)\n }\n \n enum protocol {\n+\tPROTO_LOCAL_OR_SSH = 0,\n \tPROTO_LOCAL = 1,\n \tPROTO_SSH,\n \tPROTO_GIT\n@@ -547,6 +548,15 @@ static char *get_port(char *host)\n \n static struct child_process no_fork;\n \n+int is_local(const char *url)\n+{\n+\tconst char *colon = strchr(url, ':');\n+\tconst char *slash = strchr(url, '/');\n+\treturn !colon || (slash && slash < colon) ||\n+\t\thas_dos_drive_prefix(url);\n+}\n+\n+\n /*\n  * This returns a dummy child_process if the transport protocol does not\n  * need fork(2), or a struct child_process object if it does.  Once done,\n@@ -564,9 +574,9 @@ struct child_process *git_connect(int fd[2], const char *url_orig,\n \tchar *url;\n \tchar *host, *path;\n \tchar *end;\n-\tint c;\n+\tint separator;\n \tstruct child_process *conn = &no_fork;\n-\tenum protocol protocol = PROTO_LOCAL;\n+\tenum protocol protocol = PROTO_LOCAL_OR_SSH;\n \tint free_path = 0;\n \tchar *port = NULL;\n \tconst char **arg;\n@@ -587,20 +597,23 @@ struct child_process *git_connect(int fd[2], const char *url_orig,\n \t\t*host = '\\0';\n \t\tprotocol = get_protocol(url);\n \t\thost += 3;\n-\t\tc = '/';\n+\t\tseparator = '/';\n \t} else {\n \t\thost = url;\n-\t\tc = ':';\n+\t\tseparator = ':';\n+\t\tif (is_local(url))\n+\t\t\tprotocol = PROTO_LOCAL;\n \t}\n \n \t/*\n \t * Don't do destructive transforms with git:// as that\n \t * protocol code does '[]' unwrapping of its own.\n+\t * Don't change local URLs\n \t */\n \tif (host[0] == '[') {\n \t\tend = strchr(host + 1, ']');\n \t\tif (end) {\n-\t\t\tif (protocol != PROTO_GIT) {\n+\t\t\tif (protocol != PROTO_GIT && protocol != PROTO_LOCAL) {\n \t\t\t\t*end = 0;\n \t\t\t\thost++;\n \t\t\t}\n@@ -610,17 +623,17 @@ struct child_process *git_connect(int fd[2], const char *url_orig,\n \t} else\n \t\tend = host;\n \n-\tpath = strchr(end, c);\n-\tif (path && !has_dos_drive_prefix(end)) {\n-\t\tif (c == ':') {\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-\t\t\t\tpath = end;\n+\tpath = strchr(end, separator);\n+\tif (separator == ':') {\n+\t\tif (path && protocol == PROTO_LOCAL_OR_SSH) {\n+\t\t\t/* We have a ':' */\n+\t\t\tprotocol = PROTO_SSH;\n+\t\t\t*path++ = '\\0';\n+\t\t} else {/* assume local path */\n+\t\t\tprotocol = PROTO_LOCAL;\n+\t\t\tpath = end;\n \t\t}\n-\t} else\n-\t\tpath = end;\n+\t}\n \n \tif (!path || !*path)\n \t\tdie(\"No path specified. See 'man git-pull' for valid url syntax\");\n@@ -629,7 +642,7 @@ struct child_process *git_connect(int fd[2], const char *url_orig,\n \t * null-terminate hostname and point path to ~ for URL's like this:\n \t *    ssh://host.xz/~user/repo\n \t */\n-\tif (protocol != PROTO_LOCAL && host != url) {\n+\tif (protocol != PROTO_LOCAL && separator == '/') {\n \t\tchar *ptr = path;\n \t\tif (path[1] == '~')\n \t\t\tpath++;\n@@ -644,7 +657,7 @@ struct child_process *git_connect(int fd[2], const char *url_orig,\n \t/*\n \t * Add support for ssh port: ssh://host.xy:<port>/...\n \t */\n-\tif (protocol == PROTO_SSH && host != url)\n+\tif (protocol == PROTO_SSH && separator == '/')\n \t\tport = get_port(end);\n \n \tif (protocol == PROTO_GIT) {\ndiff --git a/connect.h b/connect.h\nindex 64fb7db..fb2de9b 100644\n--- a/connect.h\n+++ b/connect.h\n@@ -5,6 +5,7 @@\n extern struct child_process *git_connect(int fd[2], const char *url, const char *prog, int flags);\n extern int finish_connect(struct child_process *conn);\n extern int git_connection_is_socket(struct child_process *conn);\n+extern int is_local(const char *url);\n extern int server_supports(const char *feature);\n extern int parse_feature_request(const char *features, const char *feature);\n extern const char *server_feature_value(const char *feature, int *len_ret);\ndiff --git a/t/t5601-clone.sh b/t/t5601-clone.sh\nindex 1d1c875..3aa2ce2 100755\n--- a/t/t5601-clone.sh\n+++ b/t/t5601-clone.sh\n@@ -310,23 +310,84 @@ expect_ssh () {\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+counter=0\n+# $1 url\n+# $2 none|host\n+# $3 path\n+test_clone_url () {\n+\tcounter=$(($counter + 1))\n \tclear_ssh &&\n-\tgit clone \"[myhost:123]:src\" ssh-bracket-clone &&\n-\texpect_ssh myhost:123 src\n+\ttest_might_fail git clone \"$1\" tmp$counter &&\n+\texpect_ssh \"$2\" \"$3\"\n+}\n+\n+#ip v4\n+for repo in rep rep/home/project /~proj 123\n+do\n+\ttest_expect_success \"cloning host:$repo\" '\n+\t\ttest_clone_url host:$repo host $repo\n+\t'\n+done\n+\n+#ipv6\n+for repo in rep rep/home/project /~proj 123\n+do\n+\ttest_expect_success \"cloning [::1]:$repo\" '\n+\t\ttest_clone_url [::1]:$repo ::1 $repo\n+\t'\n+done\n+\n+# Corner cases\n+for url in [foo]bar/baz:qux [foo/bar]:baz foo/bar:baz\n+do\n+\ttest_expect_success \"cloning $url is not ssh\" '\n+\t\t\ttest_clone_url $url none\n+\t'\n+done\n+\n+#with ssh:// scheme\n+test_expect_success 'ssh://host.xz/home/user/repo' '\n+\ttest_clone_url \"ssh://host.xz/home/user/repo\" host.xz \"/home/user/repo\"\n+'\n+\n+# from home directory\n+test_expect_success 'ssh://host.xz/~repo' '\n+\ttest_clone_url \"ssh://host.xz/~repo\" host.xz \"~repo\"\n+'\n+# with port number\n+test_expect_success 'ssh://host.xz:22/home/user/repo' '\n+\ttest_clone_url \"ssh://host.xz:22/home/user/repo\" \"-p 22 host.xz\" \"/home/user/repo\"\n+'\n+\n+# from home directory with port number\n+test_expect_success 'ssh://host.xz:22/~repo' '\n+\ttest_clone_url \"ssh://host.xz:22/~repo\" \"-p 22 host.xz\" \"~repo\"\n+'\n+\n+#IPv6\n+test_expect_success 'ssh://[::1]/home/user/repo' '\n+\ttest_clone_url \"ssh://[::1]/home/user/repo\" \"::1\" \"/home/user/repo\"\n+'\n+\n+#IPv6 from home directory\n+test_expect_success 'ssh://[::1]/~repo' '\n+\ttest_clone_url \"ssh://[::1]/~repo\" \"::1\" \"~repo\"\n+'\n+\n+#IPv6 with port number\n+test_expect_success 'ssh://[::1]:22/home/user/repo' '\n+\ttest_clone_url \"ssh://[::1]:22/home/user/repo\" \"-p 22 ::1\" \"/home/user/repo\"\n+'\n+#IPv6 from home directory with port number\n+test_expect_success 'ssh://[::1]:22/~repo' '\n+\ttest_clone_url \"ssh://[::1]:22/~repo\" \"-p 22 ::1\" \"~repo\"\n '\n \n test_expect_success 'clone from a repository with two identical branches' '\ndiff --git a/transport.c b/transport.c\nindex 7202b77..a09ba95 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -885,14 +885,6 @@ void transport_take_over(struct transport *transport,\n \ttransport->cannot_reuse = 1;\n }\n \n-static int is_local(const char *url)\n-{\n-\tconst char *colon = strchr(url, ':');\n-\tconst char *slash = strchr(url, '/');\n-\treturn !colon || (slash && slash < colon) ||\n-\t\thas_dos_drive_prefix(url);\n-}\n-\n static int is_file(const char *url)\n {\n \tstruct stat buf;\n-- \n1.8.4.499.g8798c92\n"},{"id":"229782","messageId":"20131030064231.GA11317@sigill.intra.peff.net","threadId":"35241","inReplyTo":"201310292207.50869.tboegi@web.de","subject":"Re: [PATCH V3] git clone: is an URL local or ssh","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-10-30T06:42:31Z","receivedAt":"2013-10-30T06:42:31Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Oct 29, 2013 at 10:07:50PM +0100, Torsten Bögershausen wrote:\n\n>  Changes since V2:\n>  clear_ssh and expect_ssh did come back\n>  And I couldn't get it working without the\n>  >\"$TRASH_DIRECTORY/ssh-output\"\n\nThanks.\n\n>  test_when_finished:\n>  I could not get that to work. Probably because of the\n>  battle between the quotings: '\"' \"'\" '\"'\n\nThe quoting should be straight-forward, since you can do it in\nexpect_ssh, outside of the regular test eval. But what is tricky is that\nyou do not actually want \"ssh-output\" to disappear, but rather you want\nit to be an empty file, so that tests which do not trigger ssh can\ncompare against it using test_cmp.\n\nThe patch below makes it work, but I'm thinking that it does not\nactually improve readability. While the \"clear_ssh\" call is something\neach test needs to remember, at least it is obvious there that the test\nis clearing the state before running the clone.\n\n>  Other note about test_might_fail:\n>  The first version did not need it, git clone did\n>  always succeed.\n>  After a while it always failed.\n>  According to my understanding, git clone ssh://xxx.yy\n>  should fail (as we can not clone) ??\n\nCloning over ssh via our fake wrapper should work, as the wrapper finds\nthe shell command and execs it.  So ssh://host/path should end up\nrunning:\n\n  ssh host 'git-upload-pack path'\n\nand the ssh wrapper converts that to:\n\n  git-upload-pack path\n\nIf we want to make it more robust, we could cd into a \"hosts/$host\"\ndirectory that simulates the remote host, but I don't know if that is\nnecessary.\n\n---\nThis is the test_when_finished patch.\n\ndiff --git a/t/t5601-clone.sh b/t/t5601-clone.sh\nindex 7db7f48..05afe5a 100755\n--- a/t/t5601-clone.sh\n+++ b/t/t5601-clone.sh\n@@ -291,14 +291,14 @@ test_expect_success 'setup ssh wrapper' '\n \n \tGIT_SSH=\"$TRASH_DIRECTORY/ssh-wrapper\" &&\n \texport GIT_SSH &&\n-\texport TRASH_DIRECTORY\n+\texport TRASH_DIRECTORY &&\n+\t>\"$TRASH_DIRECTORY\"/ssh-output\n '\n \n-clear_ssh () {\n-\t>\"$TRASH_DIRECTORY/ssh-output\"\n-}\n-\n expect_ssh () {\n+\ttest_when_finished '\n+\t  (cd \"$TRASH_DIRECTORY\" && rm -f ssh-expect && >ssh-output)\n+\t' &&\n \t{\n \t\tcase \"$1\" in\n \t\tnone)\n@@ -311,7 +311,6 @@ expect_ssh () {\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 \texpect_ssh none\n@@ -323,7 +322,6 @@ counter=0\n # $3 path\n test_clone_url () {\n \tcounter=$(($counter + 1))\n-\tclear_ssh &&\n \ttest_might_fail git clone \"$1\" tmp$counter &&\n \texpect_ssh \"$2\" \"$3\"\n }\n"},{"id":"229784","messageId":"5270ACB7.6090608@viscovery.net","threadId":"35241","inReplyTo":"201310292207.50869.tboegi@web.de","subject":"Re: [PATCH V3] git clone: is an URL local or ssh","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2013-10-30T06:52:39Z","receivedAt":"2013-10-30T06:52:39Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Just a heads-up: This patch breaks t5601 totally on Windows. Test #4, a\nlocal clone via file: protocol, fails already. I'm investigating now.\n\n-- Hannes\n"},{"id":"229788","messageId":"5270B135.8030501@viscovery.net","threadId":"35241","inReplyTo":"201310292207.50869.tboegi@web.de","subject":"Re: [PATCH V3] git clone: is an URL local or ssh","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2013-10-30T07:11:49Z","receivedAt":"2013-10-30T07:11:49Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 10/29/2013 22:07, schrieb Torsten Bögershausen:\n> @@ -610,17 +623,17 @@ struct child_process *git_connect(int fd[2], const char *url_orig,\n>  \t} else\n>  \t\tend = host;\n>  \n> -\tpath = strchr(end, c);\n> -\tif (path && !has_dos_drive_prefix(end)) {\n> -\t\tif (c == ':') {\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> -\t\t\t\tpath = end;\n> +\tpath = strchr(end, separator);\n> +\tif (separator == ':') {\n> +\t\tif (path && protocol == PROTO_LOCAL_OR_SSH) {\n> +\t\t\t/* We have a ':' */\n> +\t\t\tprotocol = PROTO_SSH;\n> +\t\t\t*path++ = '\\0';\n> +\t\t} else {/* assume local path */\n> +\t\t\tprotocol = PROTO_LOCAL;\n> +\t\t\tpath = end;\n>  \t\t}\n> -\t} else\n> -\t\tpath = end;\n> +\t}\n>  \n>  \tif (!path || !*path)\n>  \t\tdie(\"No path specified. See 'man git-pull' for valid url syntax\");\n\nThis hunk breaks on Windows. You removed the has_dos_drive_prefix check.\n\nThe check for has_dos_drive_prefix check must happen *before* you further\ninvestigate the path/url/host for ssh protocol, and if it returns true,\nthen the path is local, no matter what follows.\n\n-- Hannes\n"}]}