{"thread":{"id":"38617","subject":"[PATCH v2 1/3] connect.c: allow ssh://user@[2001:db8::1]/repo.git","startedAt":"2015-02-21T15:52:48Z","lastAt":"2015-02-23T00:23:16Z","messageCount":4,"participants":["Torsten Bögershausen","Junio C Hamano","brian m. carlson"],"isPatch":true,"patchVersion":2,"patchTotal":3},"messages":[{"id":"256467","messageId":"54E8A9D0.30108@web.de","threadId":"38617","inReplyTo":null,"subject":"[PATCH v2 1/3] connect.c: allow ssh://user@[2001:db8::1]/repo.git","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2015-02-21T15:52:48Z","receivedAt":"2015-02-21T15:52:48Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"The ssh:// syntax was added in 2386d65822c91, it accepted\nssh://user@2001:db8::1/repo.git, which is now legacy.\n\nOver the years the parser was improved to support [] and port numbers,\nbut the combination of ssh://user@[2001:db8::1]:222/repo.git did\nnever work.\n\nThe only only way to use a user name, a literall IPV6 address and a port\nnumber was ssh://[user@2001:db8::1]:222/repo.git\n\n(Thanks to Christian Taube for reporting this long\nstanding issue)\n\nNew users would use ssh://user@[2001:db8::1]:222/repo.git,\nso change the parser to handle it correctly.\n\nSupport the old legacy URL's as well, to be backwards compatible,\nand avoid regressions for users which upgrade an existing installation\nto a later Git version.\n\nSigned-off-by: Torsten Bögershausen \n---\n\nThanks for the reviews\nI hope the intention of being backward compatible is a little bit clearer now,\nas well as the intention to accept URL's conforming to the RFC\n\n\n connect.c | 63 ++++++++++++++++++++++++++++++++++----------------------\n t/t5601-clone.sh | 2 +-\n 2 files changed, 39 insertions(+), 26 deletions(-)\n\ndiff --git a/connect.c b/connect.c\nindex d47d0ec..b608976 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -274,28 +274,44 @@ static enum protocol get_protocol(const char *name)\n \tdie(\"I don't handle protocol '%s'\", name);\n }\n \n+static char *host_end(char **hoststart, int removebrackets)\n+{\n+\tchar *host = *hoststart;\n+\tchar *end;\n+\tchar *start = strstr(host, \"@[\");\n+\tif (start)\n+\t\tstart++; /* Jump over '@' */\n+\telse\n+\t\tstart = host;\n+\tif (start[0] == '[') {\n+\t\tend = strchr(start + 1, ']');\n+\t\tif (end) {\n+\t\t\tif (removebrackets) {\n+\t\t\t\t*end = 0;\n+\t\t\t\tmemmove(start, start + 1, end - start);\n+\t\t\t\tend++;\n+\t\t\t}\n+\t\t} else\n+\t\t\tend = host;\n+\t} else\n+\t\tend = host;\n+\treturn end;\n+}\n+\n #define STR_(s)\t# s\n #define STR(s)\tSTR_(s)\n \n static void get_host_and_port(char **host, const char **port)\n {\n \tchar *colon, *end;\n-\n-\tif (*host[0] == '[') {\n-\t\tend = strchr(*host + 1, ']');\n-\t\tif (end) {\n-\t\t\t*end = 0;\n-\t\t\tend++;\n-\t\t\t(*host)++;\n-\t\t} else\n-\t\t\tend = *host;\n-\t} else\n-\t\tend = *host;\n+\tend = host_end(host, 1);\n \tcolon = strchr(end, ':');\n-\n \tif (colon) {\n-\t\t*colon = 0;\n-\t\t*port = colon + 1;\n+\t\tlong portnr = strtol(colon + 1, &end, 10);\n+\t\tif (end != colon + 1 && *end == '\\0' && 0 <= portnr && portnr < 65536) {\n+\t\t\t*colon = 0;\n+\t\t\t*port = colon + 1;\n+\t\t}\n \t}\n }\n \n@@ -547,13 +563,16 @@ static struct child_process *git_proxy_connect(int fd[2], char *host)\n \treturn proxy;\n }\n \n-static const char *get_port_numeric(const char *p)\n+static char *get_port(char *host)\n {\n \tchar *end;\n+\tchar *p = strchr(host, ':');\n+\n \tif (p) {\n \t\tlong port = strtol(p + 1, &end, 10);\n \t\tif (end != p + 1 && *end == '\\0' && 0 <= port && port < 65536) {\n-\t\t\treturn p;\n+\t\t\t*p = '\\0';\n+\t\t\treturn p+1;\n \t\t}\n \t}\n \n@@ -595,14 +614,7 @@ static enum protocol parse_connect_url(const char *url_orig, char **ret_host,\n \t * Don't do destructive transforms as protocol code does\n \t * '[]' unwrapping in get_host_and_port()\n \t */\n-\tif (host[0] == '[') {\n-\t\tend = strchr(host + 1, ']');\n-\t\tif (end) {\n-\t\t\tend++;\n-\t\t} else\n-\t\t\tend = host;\n-\t} else\n-\t\tend = host;\n+\tend = host_end(&host, 0);\n \n \tif (protocol == PROTO_LOCAL)\n \t\tpath = end;\n@@ -705,7 +717,8 @@ struct child_process *git_connect(int fd[2], const char *url,\n \t\t\tchar *ssh_host = hostandport;\n \t\t\tconst char *port = NULL;\n \t\t\tget_host_and_port(&ssh_host, &port);\n-\t\t\tport = get_port_numeric(port);\n+\t\t\tif (!port)\n+\t\t\t\tport = get_port(ssh_host);\n \n \t\t\tif (!ssh) ssh = \"ssh\";\n \ndiff --git a/t/t5601-clone.sh b/t/t5601-clone.sh\nindex e4f10c0..f901b8a 100755\n--- a/t/t5601-clone.sh\n+++ b/t/t5601-clone.sh\n@@ -326,7 +326,7 @@ test_expect_success !MINGW,!CYGWIN 'clone local path foo:bar' '\n \n test_expect_success 'bracketed hostnames are still ssh' '\n \tgit clone \"[myhost:123]:src\" ssh-bracket-clone &&\n-\texpect_ssh myhost:123 src\n+\texpect_ssh myhost '-p 123' src\n '\n \n counter=0\n-- \n2.2.0.rc1.790.ge19fcd2\n"},{"id":"256482","messageId":"54EA188E.9090601@web.de","threadId":"38617","inReplyTo":"54E8A9D0.30108@web.de","subject":"Re: [PATCH v2 1/3] connect.c: allow ssh://user@[2001:db8::1]/repo.git","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2015-02-22T17:57:34Z","receivedAt":"2015-02-22T17:57:34Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"(Sorry for the spam, a few things need correction already now,\nand forgot cc: Brian)\n\nOn 2015-02-21 16.52, Torsten Bögershausen wrote:\n> The ssh:// syntax was added in 2386d65822c91, it accepted\n> ssh://user@2001:db8::1/repo.git, which is now legacy.\n> \n> Over the years the parser was improved to support [] and port numbers,\n> but the combination of ssh://user@[2001:db8::1]:222/repo.git did\n> never work.\ns/did never work/was never implemented/\n> \n> The only only way to use a user name, a literall IPV6 address and a port\ns/literall/literal/\n> number was ssh://[user@2001:db8::1]:222/repo.git\n> \n> (Thanks to Christian Taube for reporting this long\n> standing issue)\n> \n> New users would use ssh://user@[2001:db8::1]:222/repo.git,\n> so change the parser to handle it correctly.\n> \n> Support the old legacy URL's as well, to be backwards compatible,\n\ns/URL's/URLs/\n"},{"id":"256486","messageId":"xmqqwq39vit7.fsf@gitster.dls.corp.google.com","threadId":"38617","inReplyTo":"54E8A9D0.30108@web.de","subject":"Re: [PATCH v2 1/3] connect.c: allow ssh://user@[2001:db8::1]/repo.git","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-02-22T19:37:40Z","receivedAt":"2015-02-22T19:37:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Torsten Bögershausen writes:\n\n> The ssh:// syntax was added in 2386d65822c91, it accepted\n> ssh://user@2001:db8::1/repo.git, which is now legacy.\n\nPlease do not quote just a raw object name, but annotate it so that\nwe can more easily tell what the change was about and how far back\nin the history it was done without having to run \"git show -s\" on\nit.\n\n> Over the years the parser was improved to support [] and port numbers,\n> but the combination of ssh://user@[2001:db8::1]:222/repo.git did\n> never work.\n>\n> The only only way to use a user name, a literall IPV6 address and a port\n> number was ssh://[user@2001:db8::1]:222/repo.git\n\nLet me double check if I understand Brian and the above correctly:\n\n * The original 2386d658 (Add first cut at \"git protocol\" connect\n logic., 2005-07-13) did not even care that the IPv6 syntax was\n valid or not, and worse yet, it did not correctly handle literal\n addresses in brackets;\n\n * Later we started supporting literal addresses in brackets, but\n \"user@[literal]:port\" form did not work. Instead, a bug in the\n parser allowed \"[user@literal]:port\" (which is not a valid way to\n spell such things) to work as if it were \"user@[literal]:port\",\n which is what users would expect.\n\n * This three-patch series is an attempt to allow that kosher\n syntax, \"user@[literal]:port\", to work.\n\n * It does not terribly matter if we broke the invalid syntax\n \"user@literal:port\" without brackets, and it might even be\n beneficial if we declared such addresses invalid and deprecated\n for the sake of uniformity with other protocols. Making that\n judgement, however, is outside the scope of this series.\n \"user@literal:port\" will continue to behave as it did before.\n\nIs that what is going on?\n\nIf that is the case, I think it addresses Brian's concern well.\n\nThanks.\n\n(I haven't checked the patch text itself yet, though).\n"},{"id":"256492","messageId":"20150223002316.GB514544@vauxhall.crustytoothpaste.net","threadId":"38617","inReplyTo":"xmqqwq39vit7.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v2 1/3] connect.c: allow ssh://user@[2001:db8::1]/repo.git","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2015-02-23T00:23:16Z","receivedAt":"2015-02-23T00:23:16Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On Sun, Feb 22, 2015 at 11:37:40AM -0800, Junio C Hamano wrote:\n>Let me double check if I understand Brian and the above correctly:\n>\n> * The original 2386d658 (Add first cut at \"git protocol\" connect\n> logic., 2005-07-13) did not even care that the IPv6 syntax was\n> valid or not, and worse yet, it did not correctly handle literal\n> addresses in brackets;\n>\n> * Later we started supporting literal addresses in brackets, but\n> \"user@[literal]:port\" form did not work. Instead, a bug in the\n> parser allowed \"[user@literal]:port\" (which is not a valid way to\n> spell such things) to work as if it were \"user@[literal]:port\",\n> which is what users would expect.\n>\n> * This three-patch series is an attempt to allow that kosher\n> syntax, \"user@[literal]:port\", to work.\n\nYes. My overwhelming concern is that this work, because this is the \nsyntax we want people to use and that they will expect to work based on \nthe standards, other software, and Git using HTTPS.\n\n> * It does not terribly matter if we broke the invalid syntax\n> \"user@literal:port\" without brackets, and it might even be\n> beneficial if we declared such addresses invalid and deprecated\n> for the sake of uniformity with other protocols. Making that\n> judgement, however, is outside the scope of this series.\n> \"user@literal:port\" will continue to behave as it did before.\n\nThat's an accurate assessment of my position.\n\n>Is that what is going on?\n>\n>If that is the case, I think it addresses Brian's concern well.\n\nYes, if that's the case, it addresses my concern.\n-- \nbrian m. carlson / brian with sandals: Houston, Texas, US\n+1 832 623 2791 | http://www.crustytoothpaste.net/~bmc | My opinion only\nOpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187\n"}]}