{"thread":{"id":"35429","subject":"[PATCH v7 08/10] git_connect(): Refactor the port handling for ssh","startedAt":"2013-11-28T19:49:54Z","lastAt":"2013-11-28T19:49:54Z","messageCount":1,"participants":["Torsten Bögershausen"],"isPatch":true,"patchVersion":7,"patchTotal":10},"messages":[{"id":"231259","messageId":"201311282049.55160.tboegi@web.de","threadId":"35429","inReplyTo":null,"subject":"[PATCH v7 08/10] git_connect(): Refactor the port handling for ssh","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2013-11-28T19:49:54Z","receivedAt":"2013-11-28T19:49:54Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"Use get_host_and_port() even for ssh.\nRemove the variable port git_connect(), and simplify parse_connect_url()\nUse only one return point in git_connect(), doing the free() and return conn.\n\nt5601 had 2 corner test cases which now pass.\n\nSigned-off-by: Torsten Bögershausen <tboegi@web.de>\n---\n connect.c             | 47 +++++++++++++----------------------------------\n t/t5500-fetch-pack.sh |  9 +++------\n t/t5601-clone.sh      | 10 +---------\n 3 files changed, 17 insertions(+), 49 deletions(-)\n\ndiff --git a/connect.c b/connect.c\nindex 7e5f608..7874530 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -541,16 +541,13 @@ static struct child_process *git_proxy_connect(int fd[2], char *host)\n \treturn proxy;\n }\n \n-static char *get_port(char *host)\n+static const char *get_port_numeric(const char *p)\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\t*p = '\\0';\n-\t\t\treturn p+1;\n+\t\t\treturn p;\n \t\t}\n \t}\n \n@@ -562,7 +559,7 @@ static char *get_port(char *host)\n  * The caller must free() the returned strings.\n  */\n static enum protocol parse_connect_url(const char *url_orig, char **ret_host,\n-\t\t\t\t       char **ret_port, char **ret_path)\n+\t\t\t\t       char **ret_path)\n {\n \tchar *url;\n \tchar *host, *path;\n@@ -570,7 +567,6 @@ static enum protocol parse_connect_url(const char *url_orig, char **ret_host,\n \tint separator;\n \tenum protocol protocol = PROTO_LOCAL;\n \tint free_path = 0;\n-\tchar *port = NULL;\n \n \tif (is_url(url_orig))\n \t\turl = url_decode(url_orig);\n@@ -589,16 +585,12 @@ static enum protocol parse_connect_url(const char *url_orig, char **ret_host,\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 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\tif (protocol != PROTO_GIT) {\n-\t\t\t\t*end = 0;\n-\t\t\t\thost++;\n-\t\t\t}\n \t\t\tend++;\n \t\t} else\n \t\t\tend = host;\n@@ -636,17 +628,7 @@ static enum protocol parse_connect_url(const char *url_orig, char **ret_host,\n \t\t*ptr = '\\0';\n \t}\n \n-\t/*\n-\t * Add support for ssh port: ssh://host.xy:<port>/...\n-\t */\n-\tif (protocol == PROTO_SSH && separator == '/')\n-\t\tport = get_port(end);\n-\n \t*ret_host = xstrdup(host);\n-\tif (port)\n-\t\t*ret_port = xstrdup(port);\n-\telse\n-\t\t*ret_port = NULL;\n \tif (free_path)\n \t\t*ret_path = path;\n \telse\n@@ -674,7 +656,6 @@ struct child_process *git_connect(int fd[2], const char *url,\n \tchar *host, *path;\n \tstruct child_process *conn = &no_fork;\n \tenum protocol protocol;\n-\tchar *port;\n \tconst char **arg;\n \tstruct strbuf cmd = STRBUF_INIT;\n \n@@ -683,18 +664,13 @@ struct child_process *git_connect(int fd[2], const char *url,\n \t */\n \tsignal(SIGCHLD, SIG_DFL);\n \n-\tprotocol = parse_connect_url(url, &host, &port, &path);\n+\tprotocol = parse_connect_url(url, &host, &path);\n \tif (flags & CONNECT_DIAG_URL) {\n \t\tprintf(\"Diag: url=%s\\n\", url ? url : \"NULL\");\n \t\tprintf(\"Diag: protocol=%s\\n\", prot_name(protocol));\n-\t\tprintf(\"Diag: hostandport=%s\", host ? host : \"NULL\");\n-\t\tif (port)\n-\t\t\tprintf(\":%s\\n\", port);\n-\t\telse\n-\t\t\tprintf(\"\\n\");\n+\t\tprintf(\"Diag: hostandport=%s\\n\", host ? host : \"NULL\");\n \t\tprintf(\"Diag: path=%s\\n\", path ? path : \"NULL\");\n \t\tfree(host);\n-\t\tfree(port);\n \t\tfree(path);\n \t\treturn NULL;\n \t}\n@@ -721,7 +697,6 @@ struct child_process *git_connect(int fd[2], const char *url,\n \t\t\t     target_host, 0);\n \t\tfree(target_host);\n \t\tfree(host);\n-\t\tfree(port);\n \t\tfree(path);\n \t\treturn conn;\n \t}\n@@ -737,6 +712,11 @@ struct child_process *git_connect(int fd[2], const char *url,\n \tif (protocol == PROTO_SSH) {\n \t\tconst char *ssh = getenv(\"GIT_SSH\");\n \t\tint putty = ssh && strcasestr(ssh, \"plink\");\n+\t\tchar *ssh_host = host; /* keep host for the free() below */\n+\t\tconst char *port = NULL;\n+\t\tget_host_and_port(&ssh_host, &port);\n+\t\tport = get_port_numeric(port);\n+\n \t\tif (!ssh) ssh = \"ssh\";\n \n \t\t*arg++ = ssh;\n@@ -747,7 +727,7 @@ struct child_process *git_connect(int fd[2], const char *url,\n \t\t\t*arg++ = putty ? \"-P\" : \"-p\";\n \t\t\t*arg++ = port;\n \t\t}\n-\t\t*arg++ = host;\n+\t\t*arg++ = ssh_host;\n \t}\n \telse {\n \t\t/* remove repo-local variables from the environment */\n@@ -764,7 +744,6 @@ struct child_process *git_connect(int fd[2], const char *url,\n \tfd[1] = conn->in;  /* write to child's stdin */\n \tstrbuf_release(&cmd);\n \tfree(host);\n-\tfree(port);\n \tfree(path);\n \treturn conn;\n }\ndiff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh\nindex 2d3cdaa..f4a2a38 100755\n--- a/t/t5500-fetch-pack.sh\n+++ b/t/t5500-fetch-pack.sh\n@@ -561,20 +561,18 @@ do\n \t\tdo\n \t\t\tcase \"$p\" in\n \t\t\t*ssh*)\n-\t\t\t\thh=$(echo $h | tr -d \"[]\")\n \t\t\t\tpp=ssh\n \t\t\t\t;;\n \t\t\t*)\n-\t\t\t\thh=$h\n \t\t\t\tpp=$p\n \t\t\t;;\n \t\t\tesac\n \t\t\ttest_expect_success \"fetch-pack --diag-url $p://$h/$r\" '\n-\t\t\t\tcheck_prot_host_path $p://$h/$r $pp \"$hh\" \"/$r\"\n+\t\t\t\tcheck_prot_host_path $p://$h/$r $pp \"$h\" \"/$r\"\n \t\t\t'\n \t\t\t# \"/~\" -> \"~\" conversion\n \t\t\ttest_expect_success \"fetch-pack --diag-url $p://$h/~$r\" '\n-\t\t\t\tcheck_prot_host_path $p://$h/~$r $pp \"$hh\" \"~$r\"\n+\t\t\t\tcheck_prot_host_path $p://$h/~$r $pp \"$h\" \"~$r\"\n \t\t\t'\n \t\tdone\n \tdone\n@@ -604,13 +602,12 @@ do\n \tp=ssh\n \tfor h in host [::1]\n \tdo\n-\t\thh=$(echo $h | tr -d \"[]\")\n \t\ttest_expect_success \"fetch-pack --diag-url $h:$r\" '\n \t\t\tcheck_prot_path $h:$r $p \"$r\"\n \t\t'\n \t\t# Do \"/~\" -> \"~\" conversion\n \t\ttest_expect_success \"fetch-pack --diag-url $h:/~$r\" '\n-\t\t\tcheck_prot_host_path $h:/~$r $p \"$hh\" \"~$r\"\n+\t\t\tcheck_prot_host_path $h:/~$r $p \"$h\" \"~$r\"\n \t\t'\n \tdone\n done\ndiff --git a/t/t5601-clone.sh b/t/t5601-clone.sh\nindex 4db0c0b..53a1de9 100755\n--- a/t/t5601-clone.sh\n+++ b/t/t5601-clone.sh\n@@ -364,15 +364,7 @@ do\n done\n \n # Corner cases\n-# failing\n-for url in [foo]bar/baz:qux [foo/bar]:baz\n-do\n-\ttest_expect_failure \"clone $url is not ssh\" '\n-\t\ttest_clone_url $url none\n-\t'\n-done\n-\n-for url in foo/bar:baz\n+for url in foo/bar:baz [foo]bar/baz:qux [foo/bar]:baz\n do\n \ttest_expect_success \"clone $url is not ssh\" '\n \t\ttest_clone_url $url none\n-- \n1.8.5.rc0.23.gaa27064\n"}]}