{"thread":{"id":"35227","subject":"[PATCH V2] git clone: is an URL local or ssh","startedAt":"2013-10-28T20:16:19Z","lastAt":"2013-10-29T01:49:45Z","messageCount":4,"participants":["Torsten Bögershausen","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":2,"patchTotal":null},"messages":[{"id":"229665","messageId":"201310282116.21551.tboegi@web.de","threadId":"35227","inReplyTo":null,"subject":"[PATCH V2] git clone: is an URL local or ssh","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2013-10-28T20:16:19Z","receivedAt":"2013-10-28T20:16:19Z","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 Changes since V1: Comments from Eric Sunshine (thanks)\n connect.c        | 47 +++++++++++++++++----------\n connect.h        |  1 +\n t/t5601-clone.sh | 98 ++++++++++++++++++++++++++++++++++++++++++++------------\n transport.c      |  8 -----\n 4 files changed, 108 insertions(+), 46 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..a126f08 100755\n--- a/t/t5601-clone.sh\n+++ b/t/t5601-clone.sh\n@@ -294,39 +294,95 @@ 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+i5601=0\n+# $1 url\n+# $2 none|host\n+# $3 path\n+test_clone_url () {\n+\ti5601=$(($i5601 + 1))\n+\t>\"$TRASH_DIRECTORY/ssh-output\" &&\n+\ttest_might_fail git clone \"$1\" tmp$i5601 &&\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(\n+\t\tcd \"$TRASH_DIRECTORY\" &&\n+\t\ttest_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$i5601 && 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-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\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.457.g424cb08\n"},{"id":"229667","messageId":"xmqqmwltdsgm.fsf@gitster.dls.corp.google.com","threadId":"35227","inReplyTo":"201310282116.21551.tboegi@web.de","subject":"Re: [PATCH V2] git clone: is an URL local or ssh","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-10-28T20:57:13Z","receivedAt":"2013-10-28T20:57:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Torsten Bögershausen <tboegi@web.de> writes:\n\n> (This does apply on pu, not on master.\n\nHmph.  At least for me, it applies down to cabb411f (Merge branch\n'nd/clone-local-with-colon', 2013-10-14) just fine.  Puzzled.\n\n> diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh\n> index 1d1c875..a126f08 100755\n> --- a/t/t5601-clone.sh\n> +++ b/t/t5601-clone.sh\n> @@ -294,39 +294,95 @@ 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> +i5601=0\n> +# $1 url\n> +# $2 none|host\n> +# $3 path\n> +test_clone_url () {\n> +\ti5601=$(($i5601 + 1))\n> +\t>\"$TRASH_DIRECTORY/ssh-output\" &&\n> +\ttest_might_fail git clone \"$1\" tmp$i5601 &&\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\nThis looks like a strange use of {} (not an issue this patch\nintroduced, though).  Shouldn't this suffice?\n\n\tcase ... in\n        ...\n        esac >\"$TRASH_DIRECTORY/ssh-expect\"\n\n> +\t(\n> +\t\tcd \"$TRASH_DIRECTORY\" &&\n> +\t\ttest_cmp ssh-expect ssh-output &&\n> +\t\trm -rf ssh-expect ssh-output\n\nDrop \"r\", please, when you know these are supposed to be files.\n\n> +\t)\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>  \tcp -R src \"foo:bar\" &&\n> +\ttest_clone_url \"foo:bar\" none &&\n> +\t( cd tmp$i5601 && git log)\n\nHmph.  What is this \"git log\" about?  Leftover from an earlier\ndebugging session?\n\nThe code change to connect.c part seemed to be OK from a cursory\nlook.\n\nThanks.\n"},{"id":"229692","messageId":"20131029014806.GB11861@sigill.intra.peff.net","threadId":"35227","inReplyTo":"201310282116.21551.tboegi@web.de","subject":"Re: [PATCH V2] git clone: is an URL local or ssh","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-10-29T01:48:06Z","receivedAt":"2013-10-29T01:48:06Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Oct 28, 2013 at 09:16:19PM +0100, Torsten Bögershausen wrote:\n\n> Commit 8d3d28f5 added test cases for URLs which should be ssh.\n> \n> Add more tests testing all the combinations:\n>  -IPv4 or IPv6\n>  -path starting with \"/\" or with \"/~\"\n>  -with and without the ssh:// scheme\n> \n> Add tests for ssh:// with port number.\n> \n> When a git repository \"foo:bar\" exist, git clone will call\n> absolute_path() and git_connect() will be called with\n> something like \"/home/user/projects/foo:bar\"\n> \n> Tighten the test and use \"foo:bar\" instead of \"./foo:bar\",\n> refactor 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> \n> Bug 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\nIMHO, this would have been a little easier to follow as separate\npatches, as there are a few things going on. I think the changes to the\ncode are fine, though.\n\n> diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh\n> index 1d1c875..a126f08 100755\n> --- a/t/t5601-clone.sh\n> +++ b/t/t5601-clone.sh\n> @@ -294,39 +294,95 @@ 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\nI'd prefer if you left the expect_ssh interface intact, as it is\npotentially useful outside of t5601 (but your patch ties it very closely\nto a particular set of clone tests). Can you call out to expect_ssh\ninstead from test_clone_url instead?\n\n> +i5601=0\n\nThe name of this variable confused me for a bit. Maybe \"counter\" or\nsomething would be a little more descriptive?\n\n> +# $1 url\n> +# $2 none|host\n> +# $3 path\n> +test_clone_url () {\n> +\ti5601=$(($i5601 + 1))\n> +\t>\"$TRASH_DIRECTORY/ssh-output\" &&\n> +\ttest_might_fail git clone \"$1\" tmp$i5601 &&\n\nDo we always want test_might_fail in these tests? I really would prefer\nthat the test actually accomplish the clone, as then we know that\nnothing funny is going on (e.g., multiple invocations of ssh, one of\nwhich is good and one of which is not). I think I gave more specific\nexamples in the last round of review.\n\n> -\t(cd \"$TRASH_DIRECTORY\" && test_cmp ssh-expect ssh-output)\n> +\t(\n> +\t\tcd \"$TRASH_DIRECTORY\" &&\n> +\t\ttest_cmp ssh-expect ssh-output &&\n> +\t\trm -rf ssh-expect ssh-output\n> +\t)\n\nHere we clean up at the end of the test, which is a good improvement on\nmy existing functions. But I notice that you also clear ssh-output at\nthe beginning of the test, still (which you must do to not leave a\npolluted state after a failing test). I think this might be better as:\n\n  test_when_finished '\n    (cd \"$TRASH_DIRECTORY\" && rm -f ssh-expect ssh-output)\n  '\n\nand then the \"clear_ssh\" can just go away (and your reset of ssh-output,\nwhich is the analogous line), as tests can expect a clean state.\n\n-Peff\n"},{"id":"229693","messageId":"20131029014945.GC11861@sigill.intra.peff.net","threadId":"35227","inReplyTo":"xmqqmwltdsgm.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH V2] git clone: is an URL local or ssh","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-10-29T01:49:45Z","receivedAt":"2013-10-29T01:49:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Oct 28, 2013 at 01:57:13PM -0700, Junio C Hamano wrote:\n\n> > +i5601=0\n> > +# $1 url\n> > +# $2 none|host\n> > +# $3 path\n> > +test_clone_url () {\n> > +\ti5601=$(($i5601 + 1))\n> > +\t>\"$TRASH_DIRECTORY/ssh-output\" &&\n> > +\ttest_might_fail git clone \"$1\" tmp$i5601 &&\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> \n> This looks like a strange use of {} (not an issue this patch\n> introduced, though).  Shouldn't this suffice?\n> \n> \tcase ... in\n>         ...\n>         esac >\"$TRASH_DIRECTORY/ssh-expect\"\n\nYeah, I think you are right. This one is my fault from 8d3d28f; I think\nin an earlier draft I had more complex logic that needed the {}, and\nthen never dropped them when it got simplified.\n\n-Peff\n"}]}