{"thread":{"id":"42410","subject":"[PATCH v7 6/9] connect: make parse_connect_url() return the user part of the url as a separate value","startedAt":"2016-05-21T23:17:23Z","lastAt":"2016-05-26T05:35:23Z","messageCount":19,"participants":["Mike Hommey","Eric Sunshine","Torsten Bögershausen","Junio C Hamano"],"isPatch":true,"patchVersion":7,"patchTotal":9},"messages":[{"id":"287163","messageId":"20160521231732.4888-1-mh@glandium.org","threadId":"42410","inReplyTo":null,"subject":"[PATCH v7 0/9] connect: various cleanups","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2016-05-21T23:17:23Z","receivedAt":"2016-05-21T23:17:23Z","isPatch":true,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"Difference with v6:\n- Dropped the core.gitProxy change as per discussion.\n- Added a comment about the extra get_port.\n\nNote that after this series, parse_connect_url can be further refactored to\navoid the kind of back-and-forth with host_end, get_host_and_port and\nget_port.\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             | 231 +++++++++++++++++++++++++++++---------------------\n t/t5500-fetch-pack.sh |  42 ++++++---\n 2 files changed, 166 insertions(+), 107 deletions(-)\n\n-- \n2.8.3.401.ga81c606.dirty\n"},{"id":"287168","messageId":"20160521231732.4888-2-mh@glandium.org","threadId":"42410","inReplyTo":"20160521231732.4888-1-mh@glandium.org","subject":"[PATCH v7 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-21T23:17:24Z","receivedAt":"2016-05-21T23:17:24Z","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 1 file changed, 6 insertions(+)\n\ndiff --git a/connect.c b/connect.c\nindex c53f3f1..caa2a3c 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -742,6 +742,12 @@ 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/* get_host_and_port may not return a port even when\n+\t\t\t * there is one: In the [host:port]:path case,\n+\t\t\t * get_host_and_port is called with \"[host:port]\" and\n+\t\t\t * returns \"host:port\" and NULL.\n+\t\t\t * In that specific case, we still need to split the\n+\t\t\t * port. */\n \t\t\tif (!port)\n \t\t\t\tport = get_port(ssh_host);\n \n-- \n2.8.3.401.ga81c606.dirty\n"},{"id":"287167","messageId":"20160521231732.4888-3-mh@glandium.org","threadId":"42410","inReplyTo":"20160521231732.4888-1-mh@glandium.org","subject":"[PATCH v7 2/9] connect: call get_host_and_port() earlier","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2016-05-21T23:17:25Z","receivedAt":"2016-05-21T23:17:25Z","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 caa2a3c..0cdbeb2 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/* get_host_and_port may not return a port even when\n \t\t\t * there is one: In the [host:port]:path case,\n@@ -749,16 +746,17 @@ struct child_process *git_connect(int fd[2], const char *url,\n \t\t\t * In that specific case, we still need to split the\n \t\t\t * port. */\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@@ -804,7 +802,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@@ -818,6 +816,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.401.ga81c606.dirty\n"},{"id":"287166","messageId":"20160521231732.4888-4-mh@glandium.org","threadId":"42410","inReplyTo":"20160521231732.4888-1-mh@glandium.org","subject":"[PATCH v7 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-21T23:17:26Z","receivedAt":"2016-05-21T23:17:26Z","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 0cdbeb2..7cdaed1 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.401.ga81c606.dirty\n"},{"id":"287171","messageId":"20160521231732.4888-5-mh@glandium.org","threadId":"42410","inReplyTo":"20160521231732.4888-1-mh@glandium.org","subject":"[PATCH v7 4/9] connect: make parse_connect_url() return separated host and port","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2016-05-21T23:17:27Z","receivedAt":"2016-05-21T23:17:27Z","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             | 43 +++++++++++++++++++++++++------------------\n t/t5500-fetch-pack.sh | 32 +++++++++++++++++++++-----------\n 2 files changed, 46 insertions(+), 29 deletions(-)\n\ndiff --git a/connect.c b/connect.c\nindex 7cdaed1..edbf0e2 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,24 @@ 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/* get_host_and_port may not return a port even when there is\n+\t\t * one: In the [host:port]:path case, get_host_and_port is\n+\t\t * called with \"[host:port]\" and returns \"host:port\" and NULL.\n+\t\t * In that specific case, we still need to split 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\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 +687,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 +697,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,15 +770,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/* get_host_and_port may not return a port even when\n-\t\t\t * there is one: In the [host:port]:path case,\n-\t\t\t * get_host_and_port is called with \"[host:port]\" and\n-\t\t\t * returns \"host:port\" and NULL.\n-\t\t\t * In that specific case, we still need to split the\n-\t\t\t * port. */\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@@ -770,8 +777,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@@ -830,8 +837,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.401.ga81c606.dirty\n"},{"id":"287170","messageId":"20160521231732.4888-6-mh@glandium.org","threadId":"42410","inReplyTo":"20160521231732.4888-1-mh@glandium.org","subject":"[PATCH v7 5/9] connect: group CONNECT_DIAG_URL handling code","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2016-05-21T23:17:28Z","receivedAt":"2016-05-21T23:17:28Z","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 edbf0e2..c0fad4f 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -698,7 +698,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@@ -770,20 +770,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.401.ga81c606.dirty\n"},{"id":"287162","messageId":"20160521231732.4888-7-mh@glandium.org","threadId":"42410","inReplyTo":"20160521231732.4888-1-mh@glandium.org","subject":"[PATCH v7 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-21T23:17:29Z","receivedAt":"2016-05-21T23:17:29Z","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 | 51 ++++++++++++++++++++++++++++++++++-----------------\n 1 file changed, 34 insertions(+), 17 deletions(-)\n\ndiff --git a/connect.c b/connect.c\nindex c0fad4f..48d9cd2 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,20 +652,27 @@ 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-\t\t/* get_host_and_port may not return a port even when there is\n-\t\t * one: In the [host:port]:path case, get_host_and_port is\n-\t\t * called with \"[host:port]\" and returns \"host:port\" and NULL.\n-\t\t * In that specific case, we still need to split 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+\tif (*host) {\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\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/* get_host_and_port may not have returned a port even when there is\n+\t * one: In the [host:port]:path case, get_host_and_port is called\n+\t * with \"[host:port]\" and returns \"host:port\" and NULL.\n+\t * In that specific case, we still need to split the port. */\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@@ -687,7 +696,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@@ -697,11 +706,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@@ -810,7 +822,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@@ -823,6 +839,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.401.ga81c606.dirty\n"},{"id":"287164","messageId":"20160521231732.4888-8-mh@glandium.org","threadId":"42410","inReplyTo":"20160521231732.4888-1-mh@glandium.org","subject":"[PATCH v7 7/9] connect: change the --diag-url output to separate user and host","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2016-05-21T23:17:30Z","receivedAt":"2016-05-21T23:17:30Z","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 48d9cd2..52d34c7 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -710,10 +710,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.401.ga81c606.dirty\n"},{"id":"287165","messageId":"20160521231732.4888-9-mh@glandium.org","threadId":"42410","inReplyTo":"20160521231732.4888-1-mh@glandium.org","subject":"[PATCH v7 8/9] connect: actively reject git:// urls with a user part","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2016-05-21T23:17:31Z","receivedAt":"2016-05-21T23:17:31Z","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 52d34c7..04ce210 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -726,6 +726,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.401.ga81c606.dirty\n"},{"id":"287169","messageId":"20160521231732.4888-10-mh@glandium.org","threadId":"42410","inReplyTo":"20160521231732.4888-1-mh@glandium.org","subject":"[PATCH v7 9/9] connect: move ssh command line preparation to a separate function","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2016-05-21T23:17:32Z","receivedAt":"2016-05-21T23:17:32Z","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 04ce210..ddfda4e 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -680,6 +680,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@@ -776,59 +831,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.401.ga81c606.dirty\n"},{"id":"287177","messageId":"CAPig+cQPpX7w7gJc_F_Jz=1PpiK_Q+tSSBrCwmhW84QuwX20kw@mail.gmail.com","threadId":"42410","inReplyTo":"20160521231732.4888-2-mh@glandium.org","subject":"Re: [PATCH v7 1/9] connect: document why we sometimes call get_port after get_host_and_port","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-05-22T02:40:21Z","receivedAt":"2016-05-22T02:40:21Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sat, May 21, 2016 at 7:17 PM, Mike Hommey <mh@glandium.org> wrote:\n> Signed-off-by: Mike Hommey <mh@glandium.org>\n> ---\n> diff --git a/connect.c b/connect.c\n> @@ -742,6 +742,12 @@ struct child_process *git_connect(int fd[2], const char *url,\n>                         transport_check_allowed(\"ssh\");\n>                         get_host_and_port(&ssh_host, &port);\n>\n> +                       /* get_host_and_port may not return a port even when\n> +                        * there is one: In the [host:port]:path case,\n> +                        * get_host_and_port is called with \"[host:port]\" and\n> +                        * returns \"host:port\" and NULL.\n> +                        * In that specific case, we still need to split the\n> +                        * port. */\n\nStyle:\n\n    /*\n     * This is a multi-\n     * line comment.\n     */\n\n>                         if (!port)\n>                                 port = get_port(ssh_host);\n"},{"id":"287178","messageId":"399331a6-dadb-c318-b0e0-c83e0f81ecb0@web.de","threadId":"42410","inReplyTo":"20160521231732.4888-2-mh@glandium.org","subject":"Re: [PATCH v7 1/9] connect: document why we sometimes call get_port after get_host_and_port","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2016-05-22T06:07:05Z","receivedAt":"2016-05-22T06:07:05Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On 22.05.16 01:17, Mike Hommey wrote:\n> Signed-off-by: Mike Hommey <mh@glandium.org>\n> ---\n>  connect.c | 6 ++++++\n>  1 file changed, 6 insertions(+)\n> \n> diff --git a/connect.c b/connect.c\n> index c53f3f1..caa2a3c 100644\n> --- a/connect.c\n> +++ b/connect.c\n> @@ -742,6 +742,12 @@ 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/* get_host_and_port may not return a port even when\n> +\t\t\t * there is one: In the [host:port]:path case,\n> +\t\t\t * get_host_and_port is called with \"[host:port]\" and\n> +\t\t\t * returns \"host:port\" and NULL.\n> +\t\t\t * In that specific case, we still need to split the\n> +\t\t\t * port. */\nIs it worth to mention that this case is \"still supported legacy\" ?\n"},{"id":"287179","messageId":"20160522080316.GA19790@glandium.org","threadId":"42410","inReplyTo":"399331a6-dadb-c318-b0e0-c83e0f81ecb0@web.de","subject":"Re: [PATCH v7 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-22T08:03:16Z","receivedAt":"2016-05-22T08:03:16Z","isPatch":true,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"On Sun, May 22, 2016 at 08:07:05AM +0200, Torsten Bögershausen wrote:\n> On 22.05.16 01:17, Mike Hommey wrote:\n> > Signed-off-by: Mike Hommey <mh@glandium.org>\n> > ---\n> >  connect.c | 6 ++++++\n> >  1 file changed, 6 insertions(+)\n> > \n> > diff --git a/connect.c b/connect.c\n> > index c53f3f1..caa2a3c 100644\n> > --- a/connect.c\n> > +++ b/connect.c\n> > @@ -742,6 +742,12 @@ 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/* get_host_and_port may not return a port even when\n> > +\t\t\t * there is one: In the [host:port]:path case,\n> > +\t\t\t * get_host_and_port is called with \"[host:port]\" and\n> > +\t\t\t * returns \"host:port\" and NULL.\n> > +\t\t\t * In that specific case, we still need to split the\n> > +\t\t\t * port. */\n> Is it worth to mention that this case is \"still supported legacy\" ?\n\nIf it's worth mentioning anywhere, it seems to me it would start with\nurls.txt?\n\nMike\n"},{"id":"287226","messageId":"574287B9.4090307@web.de","threadId":"42410","inReplyTo":"20160522080316.GA19790@glandium.org","subject":"Re: [PATCH v7 1/9] connect: document why we sometimes call get_port after get_host_and_port","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2016-05-23T04:31:53Z","receivedAt":"2016-05-23T04:31:53Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On 05/22/2016 10:03 AM, Mike Hommey wrote:\n> On Sun, May 22, 2016 at 08:07:05AM +0200, Torsten Bögershausen wrote:\n>> On 22.05.16 01:17, Mike Hommey wrote:\n>>> Signed-off-by: Mike Hommey <mh@glandium.org>\n>>> ---\n>>>   connect.c | 6 ++++++\n>>>   1 file changed, 6 insertions(+)\n>>>\n>>> diff --git a/connect.c b/connect.c\n>>> index c53f3f1..caa2a3c 100644\n>>> --- a/connect.c\n>>> +++ b/connect.c\n>>> @@ -742,6 +742,12 @@ 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/* get_host_and_port may not return a port even when\n>>> +\t\t\t * there is one: In the [host:port]:path case,\n>>> +\t\t\t * get_host_and_port is called with \"[host:port]\" and\n>>> +\t\t\t * returns \"host:port\" and NULL.\n>>> +\t\t\t * In that specific case, we still need to split the\n>>> +\t\t\t * port. */\n>> Is it worth to mention that this case is \"still supported legacy\" ?\n> If it's worth mentioning anywhere, it seems to me it would start with\n> urls.txt?\n>\n> Mike\n>\nI don't know.\nurls.txt is for Git users, and connect.c is for Git developers.\nurls.txt does not mention that Git follows any RFC when parsing the\nURLS', it doesn't claim to be 100% compliant.\nEven if it makes sense to do so, as many user simply expect Git to accept\nRFC compliant URL's, and it makes the development easier, if there is an \nalready\nwritten specification, that describes all the details.\nThe parser is not 100% RFC compliant, one example:\n- old-style usgage like \"git clone [host:222]:~/path/to/repo are supported\nTo easy the live for developers, it could make sense why the code is as \nit is,\nsimply to avoid that people around the world suddenly run into problems,\nwhen they upgrade the Git version.\nSo if there is a comment in the code, it could help new developers to \nunderstand\nthings easier.\n"},{"id":"287317","messageId":"xmqqwpmk797y.fsf@gitster.mtv.corp.google.com","threadId":"42410","inReplyTo":"574287B9.4090307@web.de","subject":"Re: [PATCH v7 1/9] connect: document why we sometimes call get_port after get_host_and_port","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-23T21:30:57Z","receivedAt":"2016-05-23T21:30:57Z","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>>>>   \t\t\tget_host_and_port(&ssh_host, &port);\n>>>>   +\t\t\t/* get_host_and_port may not return a port\n>>>> even when\n>>>> +\t\t\t * there is one: In the [host:port]:path case,\n>>>> +\t\t\t * get_host_and_port is called with \"[host:port]\" and\n>>>> +\t\t\t * returns \"host:port\" and NULL.\n>>>> +\t\t\t * In that specific case, we still need to split the\n>>>> +\t\t\t * port. */\n>>> Is it worth to mention that this case is \"still supported legacy\" ?\n>> If it's worth mentioning anywhere, it seems to me it would start with\n>> urls.txt?\n>>\n>> Mike\n>>\n> I don't know.\n> urls.txt is for Git users, and connect.c is for Git developers.\n> urls.txt does not mention that Git follows any RFC when parsing the\n> URLS', it doesn't claim to be 100% compliant.\n> Even if it makes sense to do so, as many user simply expect Git to accept\n> RFC compliant URL's, and it makes the development easier, if there is\n> an already\n> written specification, that describes all the details.\n> The parser is not 100% RFC compliant, one example:\n> - old-style usgage like \"git clone [host:222]:~/path/to/repo are supported\n\nIs it an option to fix get_host_and_port() so that it returns what\nthe caller expects even when it is given \"[host:port]\"?  When the\ncaller passes other forms like \"host:port\", it expects to get \"host\"\nand \"port\" parsed out into two variables.  Why can't the caller\nexpect to see the same happen when feeding \"[host:port]\" to the\nfunction?\n"},{"id":"287322","messageId":"20160523215016.GA20618@glandium.org","threadId":"42410","inReplyTo":"xmqqwpmk797y.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v7 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-23T21:50:16Z","receivedAt":"2016-05-23T21:50:16Z","isPatch":true,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"On Mon, May 23, 2016 at 02:30:57PM -0700, Junio C Hamano wrote:\n> Torsten Bögershausen <tboegi@web.de> writes:\n> \n> >>>>   \t\t\tget_host_and_port(&ssh_host, &port);\n> >>>>   +\t\t\t/* get_host_and_port may not return a port\n> >>>> even when\n> >>>> +\t\t\t * there is one: In the [host:port]:path case,\n> >>>> +\t\t\t * get_host_and_port is called with \"[host:port]\" and\n> >>>> +\t\t\t * returns \"host:port\" and NULL.\n> >>>> +\t\t\t * In that specific case, we still need to split the\n> >>>> +\t\t\t * port. */\n> >>> Is it worth to mention that this case is \"still supported legacy\" ?\n> >> If it's worth mentioning anywhere, it seems to me it would start with\n> >> urls.txt?\n> >>\n> >> Mike\n> >>\n> > I don't know.\n> > urls.txt is for Git users, and connect.c is for Git developers.\n> > urls.txt does not mention that Git follows any RFC when parsing the\n> > URLS', it doesn't claim to be 100% compliant.\n> > Even if it makes sense to do so, as many user simply expect Git to accept\n> > RFC compliant URL's, and it makes the development easier, if there is\n> > an already\n> > written specification, that describes all the details.\n> > The parser is not 100% RFC compliant, one example:\n> > - old-style usgage like \"git clone [host:222]:~/path/to/repo are supported\n\n\"This parser handles the urls described in urls.txt\" is about as much as\nthere is to say IMHO.\n\n> Is it an option to fix get_host_and_port() so that it returns what\n> the caller expects even when it is given \"[host:port]\"?  When the\n> caller passes other forms like \"host:port\", it expects to get \"host\"\n> and \"port\" parsed out into two variables.  Why can't the caller\n> expect to see the same happen when feeding \"[host:port]\" to the\n> function?\n\nAfter this series, there's only one use of get_host_and_port(). As well\nas one use of host_end() and get_port(). They become implementation\ndetails of the parse_connect_url function. And as I mentioned in the\ncover letter this round, parse_connect_url could be further modified to\navoid the kind of back-and-forth that's going on between these\nfunctions. But I'd rather leave that for later.\n\nMike\n"},{"id":"287336","messageId":"5743DC2A.6030808@web.de","threadId":"42410","inReplyTo":"xmqqwpmk797y.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v7 1/9] connect: document why we sometimes call get_port after get_host_and_port","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2016-05-24T04:44:26Z","receivedAt":"2016-05-24T04:44:26Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On 05/23/2016 11:30 PM, Junio C Hamano wrote:\n> Torsten Bögershausen <tboegi@web.de> writes:\n>\n>>>>>    \t\t\tget_host_and_port(&ssh_host, &port);\n>>>>>    +\t\t\t/* get_host_and_port may not return a port\n>>>>> even when\n>>>>> +\t\t\t * there is one: In the [host:port]:path case,\n>>>>> +\t\t\t * get_host_and_port is called with \"[host:port]\" and\n>>>>> +\t\t\t * returns \"host:port\" and NULL.\n>>>>> +\t\t\t * In that specific case, we still need to split the\n>>>>> +\t\t\t * port. */\n>>>> Is it worth to mention that this case is \"still supported legacy\" ?\n>>> If it's worth mentioning anywhere, it seems to me it would start with\n>>> urls.txt?\n>>>\n>>> Mike\n>>>\n>> I don't know.\n>> urls.txt is for Git users, and connect.c is for Git developers.\n>> urls.txt does not mention that Git follows any RFC when parsing the\n>> URLS', it doesn't claim to be 100% compliant.\n>> Even if it makes sense to do so, as many user simply expect Git to accept\n>> RFC compliant URL's, and it makes the development easier, if there is\n>> an already\n>> written specification, that describes all the details.\n>> The parser is not 100% RFC compliant, one example:\n>> - old-style usgage like \"git clone [host:222]:~/path/to/repo are supported\n> Is it an option to fix get_host_and_port() so that it returns what\n> the caller expects even when it is given \"[host:port]\"?  When the\n> caller passes other forms like \"host:port\", it expects to get \"host\"\n> and \"port\" parsed out into two variables.  Why can't the caller\n> expect to see the same happen when feeding \"[host:port]\" to the\n> function?\n>\nThis is somewhat out of my head:\ngit clone   git://[example.com:123]:/test        #illegal, malformated URL\ngit clone   [example.com:123]:/test               #scp-like URL, legacy\ngit clone   ssh://[example.com:123]:/test       #illegal, but supported \nas legacy, because\ngit clone  ssh://[user@::1]/test                       # was the only \nway to specify a user name at a literal IPv6 address\n\nMay be we should have some  more test cases for malformated git:// URLs?\n"},{"id":"287540","messageId":"20160525233403.GA23405@glandium.org","threadId":"42410","inReplyTo":"5743DC2A.6030808@web.de","subject":"Re: [PATCH v7 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-25T23:34:03Z","receivedAt":"2016-05-25T23:34:03Z","isPatch":true,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"On Tue, May 24, 2016 at 06:44:26AM +0200, Torsten Bögershausen wrote:\n> On 05/23/2016 11:30 PM, Junio C Hamano wrote:\n> > Torsten Bögershausen <tboegi@web.de> writes:\n> > \n> > > > > >    \t\t\tget_host_and_port(&ssh_host, &port);\n> > > > > >    +\t\t\t/* get_host_and_port may not return a port\n> > > > > > even when\n> > > > > > +\t\t\t * there is one: In the [host:port]:path case,\n> > > > > > +\t\t\t * get_host_and_port is called with \"[host:port]\" and\n> > > > > > +\t\t\t * returns \"host:port\" and NULL.\n> > > > > > +\t\t\t * In that specific case, we still need to split the\n> > > > > > +\t\t\t * port. */\n> > > > > Is it worth to mention that this case is \"still supported legacy\" ?\n> > > > If it's worth mentioning anywhere, it seems to me it would start with\n> > > > urls.txt?\n> > > > \n> > > > Mike\n> > > > \n> > > I don't know.\n> > > urls.txt is for Git users, and connect.c is for Git developers.\n> > > urls.txt does not mention that Git follows any RFC when parsing the\n> > > URLS', it doesn't claim to be 100% compliant.\n> > > Even if it makes sense to do so, as many user simply expect Git to accept\n> > > RFC compliant URL's, and it makes the development easier, if there is\n> > > an already\n> > > written specification, that describes all the details.\n> > > The parser is not 100% RFC compliant, one example:\n> > > - old-style usgage like \"git clone [host:222]:~/path/to/repo are supported\n> > Is it an option to fix get_host_and_port() so that it returns what\n> > the caller expects even when it is given \"[host:port]\"?  When the\n> > caller passes other forms like \"host:port\", it expects to get \"host\"\n> > and \"port\" parsed out into two variables.  Why can't the caller\n> > expect to see the same happen when feeding \"[host:port]\" to the\n> > function?\n> > \n> This is somewhat out of my head:\n> git clone   git://[example.com:123]:/test        #illegal, malformated URL\n> git clone   [example.com:123]:/test               #scp-like URL, legacy\n> git clone   ssh://[example.com:123]:/test       #illegal, but supported as\n> legacy, because\n> git clone  ssh://[user@::1]/test                       # was the only way to\n> specify a user name at a literal IPv6 address\n> \n> May be we should have some  more test cases for malformated git:// URLs?\n\nNone of these malformed urls are rejected with or without my series\napplied:\n\nWithout:\n$ git fetch-pack --diag-url git://[example.com:123]:/test \nDiag: url=git://[example.com:123]:/test\nDiag: protocol=git\nDiag: hostandport=[example.com:123]:\nDiag: path=/test\n$ git fetch-pack --diag-url\nssh://[example.com:123]:/test\nDiag: url=ssh://[example.com:123]:/test\nDiag: protocol=ssh\nDiag: userandhost=example.com\nDiag: port=123\nDiag: path=/test\n\nWith:\n$ git fetch-pack --diag-url git://[example.com:123]:/test \nDiag: url=git://[example.com:123]:/test\nDiag: protocol=git\nDiag: user=NULL\nDiag: host=example.com\nDiag: port=123\nDiag: path=/test\n$ git fetch-pack --diag-url ssh://[example.com:123]:/test\nDiag: url=ssh://[example.com:123]:/test\nDiag: protocol=ssh\nDiag: user=NULL\nDiag: host=example.com\nDiag: port=123\nDiag: path=/test\n\nNote in the first case, hostandport is \"[example.com:123]:\", and that\nis treated as host=example.com:123 and port=NULL further down, so my\nseries is changing something here, but arguably, it makes it less worse.\n(note that both with and without my series,\n\"git://[example.com:123]:42/path\" is treated the same, so only a corner\ncase changed)\n\nCan we go forward with the current series (modulo the comment style\nthing Eric noted, and maybe adding a note about the parser handling urls\nas per urls.txt), and not bloat scope it? If anything, the state of the\ncode after the series should make further parser changes easier.\n\nCheers,\n\nMike\n"},{"id":"287554","messageId":"57468B1B.4080604@web.de","threadId":"42410","inReplyTo":"20160525233403.GA23405@glandium.org","subject":"Re: [PATCH v7 1/9] connect: document why we sometimes call get_port after get_host_and_port","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2016-05-26T05:35:23Z","receivedAt":"2016-05-26T05:35:23Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On 05/26/2016 01:34 AM, Mike Hommey wrote:\n> On Tue, May 24, 2016 at 06:44:26AM +0200, Torsten Bögershausen wrote:\n>> On 05/23/2016 11:30 PM, Junio C Hamano wrote:\n>>> Torsten Bögershausen <tboegi@web.de> writes:\n>>>\n>>>>>>>     \t\t\tget_host_and_port(&ssh_host, &port);\n>>>>>>>     +\t\t\t/* get_host_and_port may not return a port\n>>>>>>> even when\n>>>>>>> +\t\t\t * there is one: In the [host:port]:path case,\n>>>>>>> +\t\t\t * get_host_and_port is called with \"[host:port]\" and\n>>>>>>> +\t\t\t * returns \"host:port\" and NULL.\n>>>>>>> +\t\t\t * In that specific case, we still need to split the\n>>>>>>> +\t\t\t * port. */\n>>>>>> Is it worth to mention that this case is \"still supported legacy\" ?\n>>>>> If it's worth mentioning anywhere, it seems to me it would start with\n>>>>> urls.txt?\n>>>>>\n>>>>> Mike\n>>>>>\n>>>> I don't know.\n>>>> urls.txt is for Git users, and connect.c is for Git developers.\n>>>> urls.txt does not mention that Git follows any RFC when parsing the\n>>>> URLS', it doesn't claim to be 100% compliant.\n>>>> Even if it makes sense to do so, as many user simply expect Git to accept\n>>>> RFC compliant URL's, and it makes the development easier, if there is\n>>>> an already\n>>>> written specification, that describes all the details.\n>>>> The parser is not 100% RFC compliant, one example:\n>>>> - old-style usgage like \"git clone [host:222]:~/path/to/repo are supported\n>>> Is it an option to fix get_host_and_port() so that it returns what\n>>> the caller expects even when it is given \"[host:port]\"?  When the\n>>> caller passes other forms like \"host:port\", it expects to get \"host\"\n>>> and \"port\" parsed out into two variables.  Why can't the caller\n>>> expect to see the same happen when feeding \"[host:port]\" to the\n>>> function?\n>>>\n>> This is somewhat out of my head:\n>> git clone   git://[example.com:123]:/test        #illegal, malformated URL\n>> git clone   [example.com:123]:/test               #scp-like URL, legacy\n>> git clone   ssh://[example.com:123]:/test       #illegal, but supported as\n>> legacy, because\n>> git clone  ssh://[user@::1]/test                       # was the only way to\n>> specify a user name at a literal IPv6 address\n>>\n>> May be we should have some  more test cases for malformated git:// URLs?\n>\n> None of these malformed urls are rejected with or without my series\n> applied:\n>\n> Without:\n> $ git fetch-pack --diag-url git://[example.com:123]:/test\n> Diag: url=git://[example.com:123]:/test\n> Diag: protocol=git\n> Diag: hostandport=[example.com:123]:\n> Diag: path=/test\n> $ git fetch-pack --diag-url\n> ssh://[example.com:123]:/test\n> Diag: url=ssh://[example.com:123]:/test\n> Diag: protocol=ssh\n> Diag: userandhost=example.com\n> Diag: port=123\n> Diag: path=/test\n>\n> With:\n> $ git fetch-pack --diag-url git://[example.com:123]:/test\n> Diag: url=git://[example.com:123]:/test\n> Diag: protocol=git\n> Diag: user=NULL\n> Diag: host=example.com\n> Diag: port=123\n> Diag: path=/test\n> $ git fetch-pack --diag-url ssh://[example.com:123]:/test\n> Diag: url=ssh://[example.com:123]:/test\n> Diag: protocol=ssh\n> Diag: user=NULL\n> Diag: host=example.com\n> Diag: port=123\n> Diag: path=/test\n>\n> Note in the first case, hostandport is \"[example.com:123]:\", and that\n> is treated as host=example.com:123 and port=NULL further down, so my\n> series is changing something here, but arguably, it makes it less worse.\n> (note that both with and without my series,\n> \"git://[example.com:123]:42/path\" is treated the same, so only a corner\n> case changed)\n>\n> Can we go forward with the current series (modulo the comment style\n> thing Eric noted, and maybe adding a note about the parser handling urls\n> as per urls.txt), and not bloat scope it? If anything, the state of the\n> code after the series should make further parser changes easier.\n>\n> Cheers,\n>\n> Mike\n>\n\n\nThanks for digging.\n\nHow about something like this:\n\n/*\n  * get_host_and_port may not return a port in the [host:port]:path case.\n  * To support this undocumented legacy we still need to split the port.\n*/\n"}]}