{"thread":{"id":"42462","subject":"[PATCH v8 5/9] connect: group CONNECT_DIAG_URL handling code","startedAt":"2016-05-27T02:27:47Z","lastAt":"2016-05-28T08:47:15Z","messageCount":16,"participants":["Mike Hommey","Torsten Bögershausen"],"isPatch":true,"patchVersion":8,"patchTotal":9},"messages":[{"id":"287634","messageId":"20160527022756.22904-1-mh@glandium.org","threadId":"42462","inReplyTo":null,"subject":"[PATCH v8 0/9] connect: various cleanups","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2016-05-27T02:27:47Z","receivedAt":"2016-05-27T02:27:47Z","isPatch":true,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"Changes from v7:\n- Fixed comments.\n\nMike Hommey (9):\n  connect: document why we sometimes call get_port after\n    get_host_and_port\n  connect: call get_host_and_port() earlier\n  connect: re-derive a host:port string from the separate host and port\n    variables\n  connect: make parse_connect_url() return separated host and port\n  connect: group CONNECT_DIAG_URL handling code\n  connect: make parse_connect_url() return the user part of the url as a\n    separate value\n  connect: change the --diag-url output to separate user and host\n  connect: actively reject git:// urls with a user part\n  connect: move ssh command line preparation to a separate function\n\n connect.c             | 235 +++++++++++++++++++++++++++++---------------------\n t/t5500-fetch-pack.sh |  42 ++++++---\n 2 files changed, 170 insertions(+), 107 deletions(-)\n\n-- \n2.8.3\n"},{"id":"287631","messageId":"20160527022756.22904-2-mh@glandium.org","threadId":"42462","inReplyTo":"20160527022756.22904-1-mh@glandium.org","subject":"[PATCH v8 1/9] connect: document why we sometimes call get_port after get_host_and_port","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2016-05-27T02:27:48Z","receivedAt":"2016-05-27T02:27:48Z","isPatch":true,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"Signed-off-by: Mike Hommey <mh@glandium.org>\n---\n connect.c | 7 +++++++\n 1 file changed, 7 insertions(+)\n\ndiff --git a/connect.c b/connect.c\nindex c53f3f1..6e520c3 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -742,6 +742,13 @@ struct child_process *git_connect(int fd[2], const char *url,\n \t\t\ttransport_check_allowed(\"ssh\");\n \t\t\tget_host_and_port(&ssh_host, &port);\n \n+\t\t\t/*\n+\t\t\t * get_host_and_port does not return a port in the\n+\t\t\t * [host:port]:path case. In that case, it is called\n+\t\t\t * with \"[host:port]\" and returns \"host:port\" and NULL.\n+\t\t\t * To support this undocumented legacy we still need\n+\t\t\t * to split the port.\n+\t\t\t */\n \t\t\tif (!port)\n \t\t\t\tport = get_port(ssh_host);\n \n-- \n2.8.3\n"},{"id":"287638","messageId":"20160527022756.22904-3-mh@glandium.org","threadId":"42462","inReplyTo":"20160527022756.22904-1-mh@glandium.org","subject":"[PATCH v8 2/9] connect: call get_host_and_port() earlier","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2016-05-27T02:27:49Z","receivedAt":"2016-05-27T02:27:49Z","isPatch":true,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"Currently, get_host_and_port() is called in git_connect() for the ssh\nprotocol, and in git_tcp_connect_sock() for the git protocol. Instead\nof doing this, just call it from a single place, right after\nparse_connect_url(), and pass the host and port separately to\ngit_*_connect() functions.\n\nWe however preserve hostandport, at least for now.\n\nNote that in git_tcp_connect_sock, the port was set to \"<none>\" in a\ncase that never can happen, so that code path was removed.\n\nSigned-off-by: Mike Hommey <mh@glandium.org>\n---\n connect.c | 47 +++++++++++++++++++++++------------------------\n 1 file changed, 23 insertions(+), 24 deletions(-)\n\ndiff --git a/connect.c b/connect.c\nindex 6e520c3..d5af65f 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -343,18 +343,16 @@ static const char *ai_name(const struct addrinfo *ai)\n /*\n  * Returns a connected socket() fd, or else die()s.\n  */\n-static int git_tcp_connect_sock(char *host, int flags)\n+static int git_tcp_connect_sock(const char *host, const char *port, int flags)\n {\n \tstruct strbuf error_message = STRBUF_INIT;\n \tint sockfd = -1;\n-\tconst char *port = STR(DEFAULT_GIT_PORT);\n \tstruct addrinfo hints, *ai0, *ai;\n \tint gai;\n \tint cnt = 0;\n \n-\tget_host_and_port(&host, &port);\n-\tif (!*port)\n-\t\tport = \"<none>\";\n+\tif (!port)\n+\t\tport = STR(DEFAULT_GIT_PORT);\n \n \tmemset(&hints, 0, sizeof(hints));\n \tif (flags & CONNECT_IPV4)\n@@ -411,11 +409,10 @@ static int git_tcp_connect_sock(char *host, int flags)\n /*\n  * Returns a connected socket() fd, or else die()s.\n  */\n-static int git_tcp_connect_sock(char *host, int flags)\n+static int git_tcp_connect_sock(const char *host, const char *port, int flags)\n {\n \tstruct strbuf error_message = STRBUF_INIT;\n \tint sockfd = -1;\n-\tconst char *port = STR(DEFAULT_GIT_PORT);\n \tchar *ep;\n \tstruct hostent *he;\n \tstruct sockaddr_in sa;\n@@ -423,7 +420,8 @@ static int git_tcp_connect_sock(char *host, int flags)\n \tunsigned int nport;\n \tint cnt;\n \n-\tget_host_and_port(&host, &port);\n+\tif (!port)\n+\t\tport = STR(DEFAULT_GIT_PORT);\n \n \tif (flags & CONNECT_VERBOSE)\n \t\tfprintf(stderr, \"Looking up %s ... \", host);\n@@ -482,9 +480,10 @@ static int git_tcp_connect_sock(char *host, int flags)\n #endif /* NO_IPV6 */\n \n \n-static void git_tcp_connect(int fd[2], char *host, int flags)\n+static void git_tcp_connect(int fd[2], const char *host, const char *port,\n+\t\t\t    int flags)\n {\n-\tint sockfd = git_tcp_connect_sock(host, flags);\n+\tint sockfd = git_tcp_connect_sock(host, port, flags);\n \n \tfd[0] = sockfd;\n \tfd[1] = dup(sockfd);\n@@ -550,18 +549,16 @@ static int git_use_proxy(const char *host)\n \treturn (git_proxy_command && *git_proxy_command);\n }\n \n-static struct child_process *git_proxy_connect(int fd[2], char *host)\n+static struct child_process *git_proxy_connect(int fd[2], const char *host,\n+\t\t\t\t\t       const char *port)\n {\n-\tconst char *port = STR(DEFAULT_GIT_PORT);\n \tstruct child_process *proxy;\n \n-\tget_host_and_port(&host, &port);\n-\n \tproxy = xmalloc(sizeof(*proxy));\n \tchild_process_init(proxy);\n \targv_array_push(&proxy->args, git_proxy_command);\n \targv_array_push(&proxy->args, host);\n-\targv_array_push(&proxy->args, port);\n+\targv_array_push(&proxy->args, port ? port : STR(DEFAULT_GIT_PORT));\n \tproxy->in = -1;\n \tproxy->out = -1;\n \tif (start_command(proxy))\n@@ -672,7 +669,8 @@ static struct child_process no_fork = CHILD_PROCESS_INIT;\n struct child_process *git_connect(int fd[2], const char *url,\n \t\t\t\t  const char *prog, int flags)\n {\n-\tchar *hostandport, *path;\n+\tchar *hostandport, *path, *host;\n+\tconst char *port = NULL;\n \tstruct child_process *conn = &no_fork;\n \tenum protocol protocol;\n \tstruct strbuf cmd = STRBUF_INIT;\n@@ -683,6 +681,8 @@ struct child_process *git_connect(int fd[2], const char *url,\n \tsignal(SIGCHLD, SIG_DFL);\n \n \tprotocol = parse_connect_url(url, &hostandport, &path);\n+\thost = xstrdup(hostandport);\n+\tget_host_and_port(&host, &port);\n \tif ((flags & CONNECT_DIAG_URL) && (protocol != PROTO_SSH)) {\n \t\tprintf(\"Diag: url=%s\\n\", url ? url : \"NULL\");\n \t\tprintf(\"Diag: protocol=%s\\n\", prot_name(protocol));\n@@ -707,9 +707,9 @@ struct child_process *git_connect(int fd[2], const char *url,\n \t\t * cannot connect.\n \t\t */\n \t\tif (git_use_proxy(hostandport))\n-\t\t\tconn = git_proxy_connect(fd, hostandport);\n+\t\t\tconn = git_proxy_connect(fd, host, port);\n \t\telse\n-\t\t\tgit_tcp_connect(fd, hostandport, flags);\n+\t\t\tgit_tcp_connect(fd, host, port, flags);\n \t\t/*\n \t\t * Separate original protocol components prog and path\n \t\t * from extended host header with a NUL byte.\n@@ -737,10 +737,7 @@ struct child_process *git_connect(int fd[2], const char *url,\n \t\tif (protocol == PROTO_SSH) {\n \t\t\tconst char *ssh;\n \t\t\tint putty = 0, tortoiseplink = 0;\n-\t\t\tchar *ssh_host = hostandport;\n-\t\t\tconst char *port = NULL;\n \t\t\ttransport_check_allowed(\"ssh\");\n-\t\t\tget_host_and_port(&ssh_host, &port);\n \n \t\t\t/*\n \t\t\t * get_host_and_port does not return a port in the\n@@ -750,16 +747,17 @@ struct child_process *git_connect(int fd[2], const char *url,\n \t\t\t * to split the port.\n \t\t\t */\n \t\t\tif (!port)\n-\t\t\t\tport = get_port(ssh_host);\n+\t\t\t\tport = get_port(host);\n \n \t\t\tif (flags & CONNECT_DIAG_URL) {\n \t\t\t\tprintf(\"Diag: url=%s\\n\", url ? url : \"NULL\");\n \t\t\t\tprintf(\"Diag: protocol=%s\\n\", prot_name(protocol));\n-\t\t\t\tprintf(\"Diag: userandhost=%s\\n\", ssh_host ? ssh_host : \"NULL\");\n+\t\t\t\tprintf(\"Diag: userandhost=%s\\n\", host ? host : \"NULL\");\n \t\t\t\tprintf(\"Diag: port=%s\\n\", port ? port : \"NONE\");\n \t\t\t\tprintf(\"Diag: path=%s\\n\", path ? path : \"NULL\");\n \n \t\t\t\tfree(hostandport);\n+\t\t\t\tfree(host);\n \t\t\t\tfree(path);\n \t\t\t\tfree(conn);\n \t\t\t\treturn NULL;\n@@ -805,7 +803,7 @@ struct child_process *git_connect(int fd[2], const char *url,\n \t\t\t\targv_array_push(&conn->args, putty ? \"-P\" : \"-p\");\n \t\t\t\targv_array_push(&conn->args, port);\n \t\t\t}\n-\t\t\targv_array_push(&conn->args, ssh_host);\n+\t\t\targv_array_push(&conn->args, host);\n \t\t} else {\n \t\t\ttransport_check_allowed(\"file\");\n \t\t}\n@@ -819,6 +817,7 @@ struct child_process *git_connect(int fd[2], const char *url,\n \t\tstrbuf_release(&cmd);\n \t}\n \tfree(hostandport);\n+\tfree(host);\n \tfree(path);\n \treturn conn;\n }\n-- \n2.8.3\n"},{"id":"287637","messageId":"20160527022756.22904-4-mh@glandium.org","threadId":"42462","inReplyTo":"20160527022756.22904-1-mh@glandium.org","subject":"[PATCH v8 3/9] connect: re-derive a host:port string from the separate host and port variables","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2016-05-27T02:27:50Z","receivedAt":"2016-05-27T02:27:50Z","isPatch":true,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"The last uses of the hostandport variable, besides being strdup'ed\nbefore being split into host and port, is to fill the host header in the\ngit protocol and to test whether to proxy the request.\n\nInstead of relying on parse_connect_url() to return a host:port string\nthat makes sense there, re-derive one from the host and port variables.\nThis will allow to refactor parse_connect_url() to return separate host\nand port strings.\n\nSigned-off-by: Mike Hommey <mh@glandium.org>\n---\n connect.c | 31 +++++++++++++++++++++++--------\n 1 file changed, 23 insertions(+), 8 deletions(-)\n\ndiff --git a/connect.c b/connect.c\nindex d5af65f..3d7bd8e 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -695,18 +695,32 @@ struct child_process *git_connect(int fd[2], const char *url,\n \t\t * connect, unless the user has overridden us in\n \t\t * the environment.\n \t\t */\n-\t\tchar *target_host = getenv(\"GIT_OVERRIDE_VIRTUAL_HOST\");\n-\t\tif (target_host)\n-\t\t\ttarget_host = xstrdup(target_host);\n-\t\telse\n-\t\t\ttarget_host = xstrdup(hostandport);\n+\t\tstruct strbuf target_host = STRBUF_INIT;\n+\t\tstruct strbuf virtual_host = STRBUF_INIT;\n+\t\tconst char *colon = strchr(host, ':');\n+\t\tchar *override_vhost = getenv(\"GIT_OVERRIDE_VIRTUAL_HOST\");\n+\n+\t\t/* If the host contains a colon (ipv6 address), it needs to\n+\t\t * be enclosed with square brackets. */\n+\t\tif (colon)\n+\t\t\tstrbuf_addch(&target_host, '[');\n+\t\tstrbuf_addstr(&target_host, host);\n+\t\tif (colon)\n+\t\t\tstrbuf_addch(&target_host, ']');\n+\t\tif (port) {\n+\t\t\tstrbuf_addch(&target_host, ':');\n+\t\t\tstrbuf_addstr(&target_host, port);\n+\t\t}\n+\n+\t\tstrbuf_addstr(&virtual_host, override_vhost ? override_vhost\n+\t\t\t\t\t\t\t    : target_host.buf);\n \n \t\ttransport_check_allowed(\"git\");\n \n \t\t/* These underlying connection commands die() if they\n \t\t * cannot connect.\n \t\t */\n-\t\tif (git_use_proxy(hostandport))\n+\t\tif (git_use_proxy(target_host.buf))\n \t\t\tconn = git_proxy_connect(fd, host, port);\n \t\telse\n \t\t\tgit_tcp_connect(fd, host, port, flags);\n@@ -720,8 +734,9 @@ struct child_process *git_connect(int fd[2], const char *url,\n \t\tpacket_write(fd[1],\n \t\t\t     \"%s %s%chost=%s%c\",\n \t\t\t     prog, path, 0,\n-\t\t\t     target_host, 0);\n-\t\tfree(target_host);\n+\t\t\t     virtual_host.buf, 0);\n+\t\tstrbuf_release(&virtual_host);\n+\t\tstrbuf_release(&target_host);\n \t} else {\n \t\tconn = xmalloc(sizeof(*conn));\n \t\tchild_process_init(conn);\n-- \n2.8.3\n"},{"id":"287639","messageId":"20160527022756.22904-5-mh@glandium.org","threadId":"42462","inReplyTo":"20160527022756.22904-1-mh@glandium.org","subject":"[PATCH v8 4/9] connect: make parse_connect_url() return separated host and port","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2016-05-27T02:27:51Z","receivedAt":"2016-05-27T02:27:51Z","isPatch":true,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"Now that nothing besides CONNECT_DIAG_URL is using hostandport, we can\nhave parse_connect_url() itself do the host and port splitting.\n\nThis still leaves \"user@\" part of the host, if there is one, which will\nbe addressed in a subsequent change. This however does add /some/\nhandling of the \"user@\" part of the host, in order to pick the port\nproperly.\n\nSigned-off-by: Mike Hommey <mh@glandium.org>\n---\n connect.c             | 47 ++++++++++++++++++++++++++++-------------------\n t/t5500-fetch-pack.sh | 32 +++++++++++++++++++++-----------\n 2 files changed, 49 insertions(+), 30 deletions(-)\n\ndiff --git a/connect.c b/connect.c\nindex 3d7bd8e..de7419e 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -589,10 +589,11 @@ 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_path)\n+\t\t\t\t       char **ret_port, char **ret_path)\n {\n \tchar *url;\n \tchar *host, *path;\n+\tconst char *port = NULL;\n \tchar *end;\n \tint separator = '/';\n \tenum protocol protocol = PROTO_LOCAL;\n@@ -647,7 +648,27 @@ static enum protocol parse_connect_url(const char *url_orig, char **ret_host,\n \tpath = xstrdup(path);\n \t*end = '\\0';\n \n+\tget_host_and_port(&host, &port);\n+\n+\tif (*host && !port) {\n+\t\t/*\n+\t\t * get_host_and_port does not return a port in the\n+\t\t * [host:port]:path case. In that case, it is called with\n+\t\t * \"[host:port]\" and returns \"host:port\" and NULL.\n+\t\t * To support this undocumented legacy we still need to split\n+\t\t * the port.\n+\t\t * \"host:port\" may also look like \"user@host:port\". As the\n+\t\t * `user` portion tends to be less strict than `host:port`,\n+\t\t * we first put it out of the equation: since a hostname\n+\t\t * cannot contain a '@', we start from the last '@' in the\n+\t\t * string.\n+\t\t */\n+\t\tchar *end_user = strrchr(host, '@');\n+\t\tport = get_port(end_user ? end_user : host);\n+\t}\n+\n \t*ret_host = xstrdup(host);\n+\t*ret_port = port ? xstrdup(port) : NULL;\n \t*ret_path = path;\n \tfree(url);\n \treturn protocol;\n@@ -669,8 +690,7 @@ static struct child_process no_fork = CHILD_PROCESS_INIT;\n struct child_process *git_connect(int fd[2], const char *url,\n \t\t\t\t  const char *prog, int flags)\n {\n-\tchar *hostandport, *path, *host;\n-\tconst char *port = NULL;\n+\tchar *host, *port, *path;\n \tstruct child_process *conn = &no_fork;\n \tenum protocol protocol;\n \tstruct strbuf cmd = STRBUF_INIT;\n@@ -680,13 +700,12 @@ 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, &hostandport, &path);\n-\thost = xstrdup(hostandport);\n-\tget_host_and_port(&host, &port);\n+\tprotocol = parse_connect_url(url, &host, &port, &path);\n \tif ((flags & CONNECT_DIAG_URL) && (protocol != PROTO_SSH)) {\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\\n\", hostandport ? hostandport : \"NULL\");\n+\t\tprintf(\"Diag: userandhost=%s\\n\", host ? host : \"NULL\");\n+\t\tprintf(\"Diag: port=%s\\n\", port ? port : \"NONE\");\n \t\tprintf(\"Diag: path=%s\\n\", path ? path : \"NULL\");\n \t\tconn = NULL;\n \t} else if (protocol == PROTO_GIT) {\n@@ -754,16 +773,6 @@ struct child_process *git_connect(int fd[2], const char *url,\n \t\t\tint putty = 0, tortoiseplink = 0;\n \t\t\ttransport_check_allowed(\"ssh\");\n \n-\t\t\t/*\n-\t\t\t * get_host_and_port does not return a port in the\n-\t\t\t * [host:port]:path case. In that case, it is called\n-\t\t\t * with \"[host:port]\" and returns \"host:port\" and NULL.\n-\t\t\t * To support this undocumented legacy we still need\n-\t\t\t * to split the port.\n-\t\t\t */\n-\t\t\tif (!port)\n-\t\t\t\tport = get_port(host);\n-\n \t\t\tif (flags & CONNECT_DIAG_URL) {\n \t\t\t\tprintf(\"Diag: url=%s\\n\", url ? url : \"NULL\");\n \t\t\t\tprintf(\"Diag: protocol=%s\\n\", prot_name(protocol));\n@@ -771,8 +780,8 @@ struct child_process *git_connect(int fd[2], const char *url,\n \t\t\t\tprintf(\"Diag: port=%s\\n\", port ? port : \"NONE\");\n \t\t\t\tprintf(\"Diag: path=%s\\n\", path ? path : \"NULL\");\n \n-\t\t\t\tfree(hostandport);\n \t\t\t\tfree(host);\n+\t\t\t\tfree(port);\n \t\t\t\tfree(path);\n \t\t\t\tfree(conn);\n \t\t\t\treturn NULL;\n@@ -831,8 +840,8 @@ struct child_process *git_connect(int fd[2], const char *url,\n \t\tfd[1] = conn->in;  /* write to child's stdin */\n \t\tstrbuf_release(&cmd);\n \t}\n-\tfree(hostandport);\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 91a69fc..739c6b1 100755\n--- a/t/t5500-fetch-pack.sh\n+++ b/t/t5500-fetch-pack.sh\n@@ -553,7 +553,7 @@ check_prot_path () {\n \tDiag: protocol=$2\n \tDiag: path=$3\n \tEOF\n-\tgit fetch-pack --diag-url \"$1\" | grep -v hostandport= >actual &&\n+\tgit fetch-pack --diag-url \"$1\" | egrep -v '(host|port)=' >actual &&\n \ttest_cmp expected actual\n }\n \n@@ -562,22 +562,17 @@ check_prot_host_port_path () {\n \tcase \"$2\" in\n \t\t*ssh*)\n \t\tpp=ssh\n-\t\tuah=userandhost\n-\t\tehost=$(echo $3 | tr -d \"[]\")\n-\t\tdiagport=\"Diag: port=$4\"\n \t\t;;\n \t\t*)\n-\t\tpp=$p\n-\t\tuah=hostandport\n-\t\tehost=$(echo $3$4 | sed -e \"s/22$/:22/\" -e \"s/NONE//\")\n-\t\tdiagport=\"\"\n+\t\tpp=$2\n \t\t;;\n \tesac\n+\tehost=$(echo $3 | tr -d \"[]\")\n \tcat >exp <<-EOF &&\n \tDiag: url=$1\n \tDiag: protocol=$pp\n-\tDiag: $uah=$ehost\n-\t$diagport\n+\tDiag: userandhost=$ehost\n+\tDiag: port=$4\n \tDiag: path=$5\n \tEOF\n \tgrep -v \"^$\" exp >expected\n@@ -585,7 +580,7 @@ check_prot_host_port_path () {\n \ttest_cmp expected actual\n }\n \n-for r in repo re:po re/po\n+for r in repo re:po re/po re@po\n do\n \t# git or ssh with scheme\n \tfor p in \"ssh+git\" \"git+ssh\" git ssh\n@@ -608,6 +603,9 @@ do\n \t\t\ttest_expect_success \"fetch-pack --diag-url $p://$h:22/$r\" '\n \t\t\t\tcheck_prot_host_port_path $p://$h:22/$r $p \"$h\" 22 \"/$r\"\n \t\t\t'\n+\t\t\ttest_expect_success \"fetch-pack --diag-url $p://$h:22/$r\" '\n+\t\t\t\tcheck_prot_host_port_path $p://$h:22/$r $p \"$h\" 22 \"/$r\"\n+\t\t\t'\n \t\tdone\n \tdone\n \t# file with scheme\n@@ -644,6 +642,18 @@ do\n \t\t\tcheck_prot_host_port_path $h:/~$r $p \"$h\" NONE \"~$r\"\n \t\t'\n \tdone\n+\t#ssh without scheme with port\n+\tp=ssh\n+\tfor h in host user@host\n+\tdo\n+\t\ttest_expect_success \"fetch-pack --diag-url [$h:22]:$r\" '\n+\t\t\tcheck_prot_host_port_path [$h:22]:$r $p $h 22 \"$r\"\n+\t\t'\n+\t\t# Do \"/~\" -> \"~\" conversion\n+\t\ttest_expect_success \"fetch-pack --diag-url [$h:22]:/~$r\" '\n+\t\t\tcheck_prot_host_port_path [$h:22]:/~$r $p $h 22 \"~$r\"\n+\t\t'\n+\tdone\n done\n \n test_expect_success MINGW 'fetch-pack --diag-url file://c:/repo' '\n-- \n2.8.3\n"},{"id":"287630","messageId":"20160527022756.22904-6-mh@glandium.org","threadId":"42462","inReplyTo":"20160527022756.22904-1-mh@glandium.org","subject":"[PATCH v8 5/9] connect: group CONNECT_DIAG_URL handling code","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2016-05-27T02:27:52Z","receivedAt":"2016-05-27T02:27:52Z","isPatch":true,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"Previous changes made both branches handling CONNECT_DIAG_URL identical.\nWe can now remove one of those branches and have CONNECT_DIAG_URL be\nhandled in one place.\n\nSigned-off-by: Mike Hommey <mh@glandium.org>\n---\n connect.c | 16 +---------------\n 1 file changed, 1 insertion(+), 15 deletions(-)\n\ndiff --git a/connect.c b/connect.c\nindex de7419e..4be06f4 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -701,7 +701,7 @@ struct child_process *git_connect(int fd[2], const char *url,\n \tsignal(SIGCHLD, SIG_DFL);\n \n \tprotocol = parse_connect_url(url, &host, &port, &path);\n-\tif ((flags & CONNECT_DIAG_URL) && (protocol != PROTO_SSH)) {\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: userandhost=%s\\n\", host ? host : \"NULL\");\n@@ -773,20 +773,6 @@ struct child_process *git_connect(int fd[2], const char *url,\n \t\t\tint putty = 0, tortoiseplink = 0;\n \t\t\ttransport_check_allowed(\"ssh\");\n \n-\t\t\tif (flags & CONNECT_DIAG_URL) {\n-\t\t\t\tprintf(\"Diag: url=%s\\n\", url ? url : \"NULL\");\n-\t\t\t\tprintf(\"Diag: protocol=%s\\n\", prot_name(protocol));\n-\t\t\t\tprintf(\"Diag: userandhost=%s\\n\", host ? host : \"NULL\");\n-\t\t\t\tprintf(\"Diag: port=%s\\n\", port ? port : \"NONE\");\n-\t\t\t\tprintf(\"Diag: path=%s\\n\", path ? path : \"NULL\");\n-\n-\t\t\t\tfree(host);\n-\t\t\t\tfree(port);\n-\t\t\t\tfree(path);\n-\t\t\t\tfree(conn);\n-\t\t\t\treturn NULL;\n-\t\t\t}\n-\n \t\t\tssh = getenv(\"GIT_SSH_COMMAND\");\n \t\t\tif (!ssh) {\n \t\t\t\tconst char *base;\n-- \n2.8.3\n"},{"id":"287636","messageId":"20160527022756.22904-7-mh@glandium.org","threadId":"42462","inReplyTo":"20160527022756.22904-1-mh@glandium.org","subject":"[PATCH v8 6/9] connect: make parse_connect_url() return the user part of the url as a separate value","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2016-05-27T02:27:53Z","receivedAt":"2016-05-27T02:27:53Z","isPatch":true,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"Signed-off-by: Mike Hommey <mh@glandium.org>\n---\n connect.c | 54 ++++++++++++++++++++++++++++++++++++------------------\n 1 file changed, 36 insertions(+), 18 deletions(-)\n\ndiff --git a/connect.c b/connect.c\nindex 4be06f4..0819c25 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -588,11 +588,13 @@ static char *get_port(char *host)\n  * Extract protocol and relevant parts from the specified connection URL.\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+static enum protocol parse_connect_url(const char *url_orig, char **ret_user,\n+\t\t\t\t       char **ret_host, char **ret_port,\n+\t\t\t\t       char **ret_path)\n {\n \tchar *url;\n \tchar *host, *path;\n+\tconst char *user = NULL;\n \tconst char *port = NULL;\n \tchar *end;\n \tint separator = '/';\n@@ -650,23 +652,31 @@ static enum protocol parse_connect_url(const char *url_orig, char **ret_host,\n \n \tget_host_and_port(&host, &port);\n \n-\tif (*host && !port) {\n+\tif (*host) {\n \t\t/*\n-\t\t * get_host_and_port does not return a port in the\n-\t\t * [host:port]:path case. In that case, it is called with\n-\t\t * \"[host:port]\" and returns \"host:port\" and NULL.\n-\t\t * To support this undocumented legacy we still need to split\n-\t\t * the port.\n-\t\t * \"host:port\" may also look like \"user@host:port\". As the\n-\t\t * `user` portion tends to be less strict than `host:port`,\n-\t\t * we first put it out of the equation: since a hostname\n-\t\t * cannot contain a '@', we start from the last '@' in the\n-\t\t * string.\n+\t\t * At this point, the host part may look like user@host:port.\n+\t\t * As the `user` portion tends to be less strict than\n+\t\t * `host:port`, we first put it out of the equation: since a\n+\t\t * hostname cannot contain a '@', we start from the last '@'\n+\t\t * in the string.\n \t\t */\n \t\tchar *end_user = strrchr(host, '@');\n-\t\tport = get_port(end_user ? end_user : host);\n+\t\tif (end_user) {\n+\t\t\t*end_user = '\\0';\n+\t\t\tuser = host;\n+\t\t\thost = end_user + 1;\n+\t\t}\n \t}\n+\t/*\n+\t * get_host_and_port does not return a port in the [host:port]:path\n+\t * case. In that case, it is called with \"[host:port]\" and returns\n+\t * \"host:port\" and NULL.\n+\t * To support this undocumented legacy we still need to split the port.\n+\t */\n+\tif (!port)\n+\t\tport = get_port(host);\n \n+\t*ret_user = user ? xstrdup(user) : NULL;\n \t*ret_host = xstrdup(host);\n \t*ret_port = port ? xstrdup(port) : NULL;\n \t*ret_path = path;\n@@ -690,7 +700,7 @@ static struct child_process no_fork = CHILD_PROCESS_INIT;\n struct child_process *git_connect(int fd[2], const char *url,\n \t\t\t\t  const char *prog, int flags)\n {\n-\tchar *host, *port, *path;\n+\tchar *user, *host, *port, *path;\n \tstruct child_process *conn = &no_fork;\n \tenum protocol protocol;\n \tstruct strbuf cmd = STRBUF_INIT;\n@@ -700,11 +710,14 @@ 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, &user, &host, &port, &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: userandhost=%s\\n\", host ? host : \"NULL\");\n+\t\tif (user)\n+\t\t\tprintf(\"Diag: userandhost=%s@%s\\n\", user, host);\n+\t\telse\n+\t\t\tprintf(\"Diag: userandhost=%s\\n\", host ? host : \"NULL\");\n \t\tprintf(\"Diag: port=%s\\n\", port ? port : \"NONE\");\n \t\tprintf(\"Diag: path=%s\\n\", path ? path : \"NULL\");\n \t\tconn = NULL;\n@@ -813,7 +826,11 @@ struct child_process *git_connect(int fd[2], const char *url,\n \t\t\t\targv_array_push(&conn->args, putty ? \"-P\" : \"-p\");\n \t\t\t\targv_array_push(&conn->args, port);\n \t\t\t}\n-\t\t\targv_array_push(&conn->args, host);\n+\t\t\tif (user)\n+\t\t\t\targv_array_pushf(&conn->args, \"%s@%s\",\n+\t\t\t\t\t\t user, host);\n+\t\t\telse\n+\t\t\t\targv_array_push(&conn->args, host);\n \t\t} else {\n \t\t\ttransport_check_allowed(\"file\");\n \t\t}\n@@ -826,6 +843,7 @@ struct child_process *git_connect(int fd[2], const char *url,\n \t\tfd[1] = conn->in;  /* write to child's stdin */\n \t\tstrbuf_release(&cmd);\n \t}\n+\tfree(user);\n \tfree(host);\n \tfree(port);\n \tfree(path);\n-- \n2.8.3\n"},{"id":"287633","messageId":"20160527022756.22904-8-mh@glandium.org","threadId":"42462","inReplyTo":"20160527022756.22904-1-mh@glandium.org","subject":"[PATCH v8 7/9] connect: change the --diag-url output to separate user and host","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2016-05-27T02:27:54Z","receivedAt":"2016-05-27T02:27:54Z","isPatch":true,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"Signed-off-by: Mike Hommey <mh@glandium.org>\n---\n connect.c             |  6 ++----\n t/t5500-fetch-pack.sh | 14 ++++++++++++--\n 2 files changed, 14 insertions(+), 6 deletions(-)\n\ndiff --git a/connect.c b/connect.c\nindex 0819c25..0c4d23b 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -714,10 +714,8 @@ struct child_process *git_connect(int fd[2], const char *url,\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\tif (user)\n-\t\t\tprintf(\"Diag: userandhost=%s@%s\\n\", user, host);\n-\t\telse\n-\t\t\tprintf(\"Diag: userandhost=%s\\n\", host ? host : \"NULL\");\n+\t\tprintf(\"Diag: user=%s\\n\", user ? user : \"NULL\");\n+\t\tprintf(\"Diag: host=%s\\n\", host ? host : \"NULL\");\n \t\tprintf(\"Diag: port=%s\\n\", port ? port : \"NONE\");\n \t\tprintf(\"Diag: path=%s\\n\", path ? path : \"NULL\");\n \t\tconn = NULL;\ndiff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh\nindex 739c6b1..2d9c4be 100755\n--- a/t/t5500-fetch-pack.sh\n+++ b/t/t5500-fetch-pack.sh\n@@ -553,7 +553,7 @@ check_prot_path () {\n \tDiag: protocol=$2\n \tDiag: path=$3\n \tEOF\n-\tgit fetch-pack --diag-url \"$1\" | egrep -v '(host|port)=' >actual &&\n+\tgit fetch-pack --diag-url \"$1\" | egrep -v \"(user|host|port)=\" >actual &&\n \ttest_cmp expected actual\n }\n \n@@ -568,10 +568,20 @@ check_prot_host_port_path () {\n \t\t;;\n \tesac\n \tehost=$(echo $3 | tr -d \"[]\")\n+\tcase \"$ehost\" in\n+\t\t*@*)\n+\t\tuser=${ehost%@*}\n+\t\tehost=${ehost#$user@}\n+\t\t;;\n+\t\t*)\n+\t\tuser=NULL\n+\t\t;;\n+\tesac\n \tcat >exp <<-EOF &&\n \tDiag: url=$1\n \tDiag: protocol=$pp\n-\tDiag: userandhost=$ehost\n+\tDiag: user=$user\n+\tDiag: host=$ehost\n \tDiag: port=$4\n \tDiag: path=$5\n \tEOF\n-- \n2.8.3\n"},{"id":"287635","messageId":"20160527022756.22904-9-mh@glandium.org","threadId":"42462","inReplyTo":"20160527022756.22904-1-mh@glandium.org","subject":"[PATCH v8 8/9] connect: actively reject git:// urls with a user part","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2016-05-27T02:27:55Z","receivedAt":"2016-05-27T02:27:55Z","isPatch":true,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"Currently, urls of the for git://user@host don't work because user@host\nis not resolving at the DNS level, but we shouldn't be relying on it\nbeing an invalid host name, and actively reject it for containing a\nusername in the first place.\n\nSigned-off-by: Mike Hommey <mh@glandium.org>\n---\n connect.c | 3 +++\n 1 file changed, 3 insertions(+)\n\ndiff --git a/connect.c b/connect.c\nindex 0c4d23b..9aea3cd 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -730,6 +730,9 @@ struct child_process *git_connect(int fd[2], const char *url,\n \t\tconst char *colon = strchr(host, ':');\n \t\tchar *override_vhost = getenv(\"GIT_OVERRIDE_VIRTUAL_HOST\");\n \n+\t\tif (user)\n+\t\t\tdie(\"user@host is not allowed in git:// urls\");\n+\n \t\t/* If the host contains a colon (ipv6 address), it needs to\n \t\t * be enclosed with square brackets. */\n \t\tif (colon)\n-- \n2.8.3\n"},{"id":"287632","messageId":"20160527022756.22904-10-mh@glandium.org","threadId":"42462","inReplyTo":"20160527022756.22904-1-mh@glandium.org","subject":"[PATCH v8 9/9] connect: move ssh command line preparation to a separate function","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2016-05-27T02:27:56Z","receivedAt":"2016-05-27T02:27:56Z","isPatch":true,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"Signed-off-by: Mike Hommey <mh@glandium.org>\n---\n connect.c | 108 +++++++++++++++++++++++++++++++++-----------------------------\n 1 file changed, 58 insertions(+), 50 deletions(-)\n\ndiff --git a/connect.c b/connect.c\nindex 9aea3cd..076ae09 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -684,6 +684,61 @@ static enum protocol parse_connect_url(const char *url_orig, char **ret_user,\n \treturn protocol;\n }\n \n+static int prepare_ssh_command(struct argv_array *cmd, const char *user,\n+\t\t\t       const char *host, const char *port, int flags)\n+{\n+\tconst char *ssh;\n+\tint putty = 0, tortoiseplink = 0, use_shell = 1;\n+\ttransport_check_allowed(\"ssh\");\n+\n+\tssh = getenv(\"GIT_SSH_COMMAND\");\n+\tif (!ssh) {\n+\t\tconst char *base;\n+\t\tchar *ssh_dup;\n+\n+\t\t/*\n+\t\t * GIT_SSH is the no-shell version of\n+\t\t * GIT_SSH_COMMAND (and must remain so for\n+\t\t * historical compatibility).\n+\t\t */\n+\t\tuse_shell = 0;\n+\n+\t\tssh = getenv(\"GIT_SSH\");\n+\t\tif (!ssh)\n+\t\t\tssh = \"ssh\";\n+\n+\t\tssh_dup = xstrdup(ssh);\n+\t\tbase = basename(ssh_dup);\n+\n+\t\ttortoiseplink = !strcasecmp(base, \"tortoiseplink\") ||\n+\t\t\t!strcasecmp(base, \"tortoiseplink.exe\");\n+\t\tputty = tortoiseplink ||\n+\t\t\t!strcasecmp(base, \"plink\") ||\n+\t\t\t!strcasecmp(base, \"plink.exe\");\n+\n+\t\tfree(ssh_dup);\n+\t}\n+\n+\targv_array_push(cmd, ssh);\n+\tif (flags & CONNECT_IPV4)\n+\t\targv_array_push(cmd, \"-4\");\n+\telse if (flags & CONNECT_IPV6)\n+\t\targv_array_push(cmd, \"-6\");\n+\tif (tortoiseplink)\n+\t\targv_array_push(cmd, \"-batch\");\n+\tif (port) {\n+\t\t/* P is for PuTTY, p is for OpenSSH */\n+\t\targv_array_push(cmd, putty ? \"-P\" : \"-p\");\n+\t\targv_array_push(cmd, port);\n+\t}\n+\tif (user)\n+\t\targv_array_pushf(cmd, \"%s@%s\", user, host);\n+\telse\n+\t\targv_array_push(cmd, host);\n+\n+\treturn use_shell;\n+}\n+\n static struct child_process no_fork = CHILD_PROCESS_INIT;\n \n /*\n@@ -780,59 +835,12 @@ struct child_process *git_connect(int fd[2], const char *url,\n \n \t\t/* remove repo-local variables from the environment */\n \t\tconn->env = local_repo_env;\n-\t\tconn->use_shell = 1;\n \t\tconn->in = conn->out = -1;\n \t\tif (protocol == PROTO_SSH) {\n-\t\t\tconst char *ssh;\n-\t\t\tint putty = 0, tortoiseplink = 0;\n-\t\t\ttransport_check_allowed(\"ssh\");\n-\n-\t\t\tssh = getenv(\"GIT_SSH_COMMAND\");\n-\t\t\tif (!ssh) {\n-\t\t\t\tconst char *base;\n-\t\t\t\tchar *ssh_dup;\n-\n-\t\t\t\t/*\n-\t\t\t\t * GIT_SSH is the no-shell version of\n-\t\t\t\t * GIT_SSH_COMMAND (and must remain so for\n-\t\t\t\t * historical compatibility).\n-\t\t\t\t */\n-\t\t\t\tconn->use_shell = 0;\n-\n-\t\t\t\tssh = getenv(\"GIT_SSH\");\n-\t\t\t\tif (!ssh)\n-\t\t\t\t\tssh = \"ssh\";\n-\n-\t\t\t\tssh_dup = xstrdup(ssh);\n-\t\t\t\tbase = basename(ssh_dup);\n-\n-\t\t\t\ttortoiseplink = !strcasecmp(base, \"tortoiseplink\") ||\n-\t\t\t\t\t!strcasecmp(base, \"tortoiseplink.exe\");\n-\t\t\t\tputty = tortoiseplink ||\n-\t\t\t\t\t!strcasecmp(base, \"plink\") ||\n-\t\t\t\t\t!strcasecmp(base, \"plink.exe\");\n-\n-\t\t\t\tfree(ssh_dup);\n-\t\t\t}\n-\n-\t\t\targv_array_push(&conn->args, ssh);\n-\t\t\tif (flags & CONNECT_IPV4)\n-\t\t\t\targv_array_push(&conn->args, \"-4\");\n-\t\t\telse if (flags & CONNECT_IPV6)\n-\t\t\t\targv_array_push(&conn->args, \"-6\");\n-\t\t\tif (tortoiseplink)\n-\t\t\t\targv_array_push(&conn->args, \"-batch\");\n-\t\t\tif (port) {\n-\t\t\t\t/* P is for PuTTY, p is for OpenSSH */\n-\t\t\t\targv_array_push(&conn->args, putty ? \"-P\" : \"-p\");\n-\t\t\t\targv_array_push(&conn->args, port);\n-\t\t\t}\n-\t\t\tif (user)\n-\t\t\t\targv_array_pushf(&conn->args, \"%s@%s\",\n-\t\t\t\t\t\t user, host);\n-\t\t\telse\n-\t\t\t\targv_array_push(&conn->args, host);\n+\t\t\tconn->use_shell = prepare_ssh_command(\n+\t\t\t\t&conn->args, user, host, port, flags);\n \t\t} else {\n+\t\t\tconn->use_shell = 1;\n \t\t\ttransport_check_allowed(\"file\");\n \t\t}\n \t\targv_array_push(&conn->args, cmd.buf);\n-- \n2.8.3\n"},{"id":"287678","messageId":"a6f4bf06-a52b-dcdf-9407-0c03b8b1dfe1@web.de","threadId":"42462","inReplyTo":"20160527022756.22904-1-mh@glandium.org","subject":"Re: [PATCH v8 0/9] connect: various cleanups","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2016-05-27T14:24:20Z","receivedAt":"2016-05-27T14:24:20Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On 27.05.16 04:27, Mike Hommey wrote:\n> Changes from v7:\n> - Fixed comments.\n> \n> Mike Hommey (9):\n>   connect: document why we sometimes call get_port after\n>     get_host_and_port\n>   connect: call get_host_and_port() earlier\n>   connect: re-derive a host:port string from the separate host and port\n>     variables\n>   connect: make parse_connect_url() return separated host and port\n>   connect: group CONNECT_DIAG_URL handling code\n>   connect: make parse_connect_url() return the user part of the url as a\n>     separate value\n>   connect: change the --diag-url output to separate user and host\n>   connect: actively reject git:// urls with a user part\n>   connect: move ssh command line preparation to a separate function\n> \n>  connect.c             | 235 +++++++++++++++++++++++++++++---------------------\n>  t/t5500-fetch-pack.sh |  42 ++++++---\n>  2 files changed, 170 insertions(+), 107 deletions(-)\n> \nIs the whole series somewhere on a public repo ?\nNo major remarks so far, if possible, I would like\nto have a look at the resulting connect.c\n"},{"id":"287719","messageId":"20160527215943.GA21018@glandium.org","threadId":"42462","inReplyTo":"a6f4bf06-a52b-dcdf-9407-0c03b8b1dfe1@web.de","subject":"Re: [PATCH v8 0/9] connect: various cleanups","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2016-05-27T21:59:43Z","receivedAt":"2016-05-27T21:59:43Z","isPatch":true,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"On Fri, May 27, 2016 at 04:24:20PM +0200, Torsten Bögershausen wrote:\n> On 27.05.16 04:27, Mike Hommey wrote:\n> > Changes from v7:\n> > - Fixed comments.\n> > \n> > Mike Hommey (9):\n> >   connect: document why we sometimes call get_port after\n> >     get_host_and_port\n> >   connect: call get_host_and_port() earlier\n> >   connect: re-derive a host:port string from the separate host and port\n> >     variables\n> >   connect: make parse_connect_url() return separated host and port\n> >   connect: group CONNECT_DIAG_URL handling code\n> >   connect: make parse_connect_url() return the user part of the url as a\n> >     separate value\n> >   connect: change the --diag-url output to separate user and host\n> >   connect: actively reject git:// urls with a user part\n> >   connect: move ssh command line preparation to a separate function\n> > \n> >  connect.c             | 235 +++++++++++++++++++++++++++++---------------------\n> >  t/t5500-fetch-pack.sh |  42 ++++++---\n> >  2 files changed, 170 insertions(+), 107 deletions(-)\n> > \n> Is the whole series somewhere on a public repo ?\n\nIs it now :)\n\n> No major remarks so far, if possible, I would like\n> to have a look at the resulting connect.c\n\nhttps://github.com/glandium/git/blob/connect/connect.c\n\nMike\n"},{"id":"287721","messageId":"5071966e-6bce-fa91-8837-81cb3c9157f7@web.de","threadId":"42462","inReplyTo":"20160527215943.GA21018@glandium.org","subject":"Re: [PATCH v8 0/9] connect: various cleanups","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2016-05-28T05:02:01Z","receivedAt":"2016-05-28T05:02:01Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On 27.05.16 23:59, Mike Hommey wrote:\n> On Fri, May 27, 2016 at 04:24:20PM +0200, Torsten Bögershausen wrote:\n>> On 27.05.16 04:27, Mike Hommey wrote:\n>>> Changes from v7:\n>>> - Fixed comments.\n>>>\n>>> Mike Hommey (9):\nAll in all, a great improvement.\n2 things left.\n- The comment about [] is now stale, isn't it ?\n- The legacy support should only be active for ssh, and not\n  be used by other schemes.\n\n\n\ndiff --git a/connect.c b/connect.c\nindex 076ae09..79b8dae 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -618,10 +618,6 @@ static enum protocol parse_connect_url(const char *url_orig, char **ret_user,\n                }\n        }\n \n-       /*\n-        * Don't do destructive transforms as protocol code does\n-        * '[]' unwrapping in get_host_and_port()\n-        */\n        end = host_end(&host, 0);\n \n        if (protocol == PROTO_LOCAL)\n@@ -673,7 +669,7 @@ static enum protocol parse_connect_url(const char *url_orig, char **ret_user,\n         * \"host:port\" and NULL.\n         * To support this undocumented legacy we still need to split the port.\n         */\n-       if (!port)\n+       if (!port && protocol == PROTO_SSH)\n                port = get_port(host);\n \n        *ret_user = user ? xstrdup(user) : NULL;\n"},{"id":"287722","messageId":"20160528053354.GA3860@glandium.org","threadId":"42462","inReplyTo":"5071966e-6bce-fa91-8837-81cb3c9157f7@web.de","subject":"Re: [PATCH v8 0/9] connect: various cleanups","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2016-05-28T05:33:54Z","receivedAt":"2016-05-28T05:33:54Z","isPatch":true,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"On Sat, May 28, 2016 at 07:02:01AM +0200, Torsten Bögershausen wrote:\n> On 27.05.16 23:59, Mike Hommey wrote:\n> > On Fri, May 27, 2016 at 04:24:20PM +0200, Torsten Bögershausen wrote:\n> >> On 27.05.16 04:27, Mike Hommey wrote:\n> >>> Changes from v7:\n> >>> - Fixed comments.\n> >>>\n> >>> Mike Hommey (9):\n> All in all, a great improvement.\n> 2 things left.\n> - The comment about [] is now stale, isn't it ?\n\nNo, it's still valid at this point, that's what the 2nd argument to\nhost_end (0) does.\n\nMike\n"},{"id":"287726","messageId":"5ba52c0a-2b8f-91af-d823-9519cdb9f67c@web.de","threadId":"42462","inReplyTo":"20160528053354.GA3860@glandium.org","subject":"Re: [PATCH v8 0/9] connect: various cleanups","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2016-05-28T08:17:19Z","receivedAt":"2016-05-28T08:17:19Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On 28.05.16 07:33, Mike Hommey wrote:\n> On Sat, May 28, 2016 at 07:02:01AM +0200, Torsten Bögershausen wrote:\n>> On 27.05.16 23:59, Mike Hommey wrote:\n>>> On Fri, May 27, 2016 at 04:24:20PM +0200, Torsten Bögershausen wrote:\n>>>> On 27.05.16 04:27, Mike Hommey wrote:\n>>>>> Changes from v7:\n>>>>> - Fixed comments.\n>>>>>\n>>>>> Mike Hommey (9):\n>> All in all, a great improvement.\n>> 2 things left.\n>> - The comment about [] is now stale, isn't it ?\n> No, it's still valid at this point, that's what the 2nd argument to\n> host_end (0) does.\n>\n> Mike\n\nThe protocol (specific) code doesn't do this anymore, \nbecause that is now common to all protocols.\n\n\n       /*\n        * Don't do destructive transforms now, the\n        * '[]' unwrapping is done in get_host_and_port()\n        */\n"},{"id":"287729","messageId":"20160528084715.GA13770@glandium.org","threadId":"42462","inReplyTo":"5ba52c0a-2b8f-91af-d823-9519cdb9f67c@web.de","subject":"Re: [PATCH v8 0/9] connect: various cleanups","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2016-05-28T08:47:15Z","receivedAt":"2016-05-28T08:47:15Z","isPatch":true,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"On Sat, May 28, 2016 at 10:17:19AM +0200, Torsten Bögershausen wrote:\n> On 28.05.16 07:33, Mike Hommey wrote:\n> > On Sat, May 28, 2016 at 07:02:01AM +0200, Torsten Bögershausen wrote:\n> >> On 27.05.16 23:59, Mike Hommey wrote:\n> >>> On Fri, May 27, 2016 at 04:24:20PM +0200, Torsten Bögershausen wrote:\n> >>>> On 27.05.16 04:27, Mike Hommey wrote:\n> >>>>> Changes from v7:\n> >>>>> - Fixed comments.\n> >>>>>\n> >>>>> Mike Hommey (9):\n> >> All in all, a great improvement.\n> >> 2 things left.\n> >> - The comment about [] is now stale, isn't it ?\n> > No, it's still valid at this point, that's what the 2nd argument to\n> > host_end (0) does.\n> >\n> > Mike\n> \n> The protocol (specific) code doesn't do this anymore, \n> because that is now common to all protocols.\n> \n> \n>        /*\n>         * Don't do destructive transforms now, the\n>         * '[]' unwrapping is done in get_host_and_port()\n>         */\n> \n\nI'm not following what you're saying. The code explicitly calls host_end\nso that it doesn't remove the square brackets from the string it's\npassed. That's what the comment says, and that's what happens.\n\nMike\n"}]}