{"thread":{"id":"35209","subject":"[PATCH/RFC] git clone: is an URL local or ssh","startedAt":"2013-10-26T19:03:34Z","lastAt":"2013-10-27T18:31:24Z","messageCount":2,"participants":["Torsten Bögershausen","Eric Sunshine"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"229572","messageId":"201310262103.35770.tboegi@web.de","threadId":"35209","inReplyTo":null,"subject":"[PATCH/RFC] git clone: is an URL local or ssh","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2013-10-26T19:03:34Z","receivedAt":"2013-10-26T19:03:34Z","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(This does apply on pu, not on master.\n I'm almost sure there are more corner cases, but the\n most important things should be covered)\n\n connect.c        | 47 +++++++++++++++++----------\n connect.h        |  1 +\n t/t5601-clone.sh | 96 +++++++++++++++++++++++++++++++++++++++++++-------------\n transport.c      |  8 -----\n 4 files changed, 106 insertions(+), 46 deletions(-)\n\ndiff --git a/connect.c b/connect.c\nindex 06e88b0..903063e 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 seperator;\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\tseperator = '/';\n \t} else {\n \t\thost = url;\n-\t\tc = ':';\n+\t\tseperator = ':';\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, seperator);\n+\tif (seperator == ':') {\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 && seperator == '/') {\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 && seperator == '/')\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..69af007 100755\n--- a/t/t5601-clone.sh\n+++ b/t/t5601-clone.sh\n@@ -294,39 +294,93 @@ test_expect_success 'setup ssh wrapper' '\n \texport TRASH_DIRECTORY\n '\n \n-clear_ssh () {\n-\t>\"$TRASH_DIRECTORY/ssh-output\"\n-}\n-\n-expect_ssh () {\n+i6501=0\n+# $1 url\n+# $2 none|host\n+# $3 path\n+test_clone_url () {\n+\ti6501=$(($i6501 + 1))\n+\t>\"$TRASH_DIRECTORY/ssh-output\" &&\n+\ttest_might_fail git clone \"$1\" tmp$i6501 &&\n \t{\n-\t\tcase \"$1\" in\n+\t\tcase \"$2\" in\n \t\tnone)\n \t\t\t;;\n \t\t*)\n-\t\t\techo \"ssh: $1 git-upload-pack '$2'\"\n+\t\t\techo \"ssh: $2 git-upload-pack '$3'\"\n \t\tesac\n \t} >\"$TRASH_DIRECTORY/ssh-expect\" &&\n-\t(cd \"$TRASH_DIRECTORY\" && test_cmp ssh-expect ssh-output)\n+\t(cd \"$TRASH_DIRECTORY\" && test_cmp ssh-expect ssh-output) && {\n+\t\trm -rf ssh-expect ssh-output\n+\t}\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+# url looks ssh like, and is on disc: should be local\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+\ttest_clone_url \"foo:bar\" none &&\n+\t( cd tmp$i6501 && git log)\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-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+# 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.457.g424cb08\n"},{"id":"229616","messageId":"CAPig+cTCesnmj1z2H1ZkXAcRqZKQu-NLeLxfCYPYUZe45aadtQ@mail.gmail.com","threadId":"35209","inReplyTo":"201310262103.35770.tboegi@web.de","subject":"Re: [PATCH/RFC] git clone: is an URL local or ssh","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2013-10-27T18:31:24Z","receivedAt":"2013-10-27T18:31:24Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Saturday, October 26, 2013, Torsten Bögershausen wrote:\n> diff --git a/connect.c b/connect.c\n> index 06e88b0..903063e 100644\n> --- a/connect.c\n> +++ b/connect.c\n> @@ -564,9 +574,9 @@ struct child_process *git_connect(int fd[2], const char *url_orig,\n>         char *url;\n>         char *host, *path;\n>         char *end;\n> -       int c;\n> +       int seperator;\n\ns/seperator/separator/g\n\n>         struct child_process *conn = &no_fork;\n> -       enum protocol protocol = PROTO_LOCAL;\n> +       enum protocol protocol = PROTO_LOCAL_OR_SSH;\n>         int free_path = 0;\n>         char *port = NULL;\n>         const char **arg;\n> diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh\n> index 1d1c875..69af007 100755\n> --- a/t/t5601-clone.sh\n> +++ b/t/t5601-clone.sh\n> @@ -294,39 +294,93 @@ test_expect_success 'setup ssh wrapper' '\n>         export TRASH_DIRECTORY\n>  '\n>\n> -clear_ssh () {\n> -       >\"$TRASH_DIRECTORY/ssh-output\"\n> -}\n> -\n> -expect_ssh () {\n> +i6501=0\n\nIs this variable meant to be named after the test script t5601? If so:\ns/i6501/i5601/g\n\n> +# $1 url\n> +# $2 none|host\n> +# $3 path\n> +test_clone_url () {\n> +       i6501=$(($i6501 + 1))\n> +       >\"$TRASH_DIRECTORY/ssh-output\" &&\n> +       test_might_fail git clone \"$1\" tmp$i6501 &&\n>         {\n> -               case \"$1\" in\n> +               case \"$2\" in\n>                 none)\n>                         ;;\n>                 *)\n> -                       echo \"ssh: $1 git-upload-pack '$2'\"\n> +                       echo \"ssh: $2 git-upload-pack '$3'\"\n>                 esac\n>         } >\"$TRASH_DIRECTORY/ssh-expect\" &&\n> -       (cd \"$TRASH_DIRECTORY\" && test_cmp ssh-expect ssh-output)\n> +       (cd \"$TRASH_DIRECTORY\" && test_cmp ssh-expect ssh-output) && {\n> +               rm -rf ssh-expect ssh-output\n> +       }\n\nShould the 'rm' be inside the (cd...) subshell? If not, are the braces\nwrapping 'rm' needed, and wouldn't you want to prefix the paths with\n$TRASH_DIRECTORY/?\n\n>  }\n>\n> -test_expect_success 'cloning myhost:src uses ssh' '\n> -       clear_ssh &&\n> -       git clone myhost:src ssh-clone &&\n> -       expect_ssh myhost src\n> -'\n"}]}