{"thread":{"id":"42351","subject":"[PATCH v6 5/9] connect: group CONNECT_DIAG_URL handling code","startedAt":"2016-05-17T01:35:45Z","lastAt":"2016-05-20T22:56:35Z","messageCount":17,"participants":["Mike Hommey","Junio C Hamano"],"isPatch":true,"patchVersion":6,"patchTotal":9},"messages":[{"id":"286750","messageId":"20160517013554.22578-1-mh@glandium.org","threadId":"42351","inReplyTo":null,"subject":"[PATCH v6 0/9] connect: various cleanups","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2016-05-17T01:35:45Z","receivedAt":"2016-05-17T01:35:45Z","isPatch":true,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"The main difference here is patch 2/9 now throwing an error in user's face\nwhen they have a core.gitProxy configuration with a port number. There might\nbe some bikeshedding to do on the content of the error message.\n\nMike Hommey (9):\n  connect: call get_host_and_port() earlier\n  connect: only match the host with core.gitProxy\n  connect: fill the host header in the git protocol with the host and\n    port 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              | 243 +++++++++++++++++++++++++++++--------------------\n t/t5500-fetch-pack.sh  |  42 ++++++---\n t/t5532-fetch-proxy.sh |   8 +-\n 3 files changed, 183 insertions(+), 110 deletions(-)\n\n-- \n2.8.2.411.ga570dec.dirty\n"},{"id":"286747","messageId":"20160517013554.22578-2-mh@glandium.org","threadId":"42351","inReplyTo":"20160517013554.22578-1-mh@glandium.org","subject":"[PATCH v6 1/9] connect: call get_host_and_port() earlier","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2016-05-17T01:35:46Z","receivedAt":"2016-05-17T01:35:46Z","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 c53f3f1..d3448c2 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,22 +737,20 @@ 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\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@@ -798,7 +796,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@@ -812,6 +810,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.2.411.ga570dec.dirty\n"},{"id":"286748","messageId":"20160517013554.22578-3-mh@glandium.org","threadId":"42351","inReplyTo":"20160517013554.22578-1-mh@glandium.org","subject":"[PATCH v6 2/9] connect: only match the host with core.gitProxy","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2016-05-17T01:35:47Z","receivedAt":"2016-05-17T01:35:47Z","isPatch":true,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"Currently, core.gitProxy doesn't actually match purely on domain names\nas documented: it also matches ports.\n\nSo a core.gitProxy value like \"script for kernel.org\" doesn't make the\nscript called for an url like git://kernel.org:port/path, while it is\ncalled for git://kernel.org/path.\n\nThis per-port behavior seems like an oversight rather than a deliberate\nchoice, so, make git://kernel.org:port/path call the gitProxy script in\nthe case described above.\n\nHowever, if people have been relying on this behavior, git now fails\nwith an error message inviting for a configuration change.\n\nSigned-off-by: Mike Hommey <mh@glandium.org>\n---\n connect.c              | 20 +++++++++++++++++---\n t/t5532-fetch-proxy.sh |  8 +++++++-\n 2 files changed, 24 insertions(+), 4 deletions(-)\n\ndiff --git a/connect.c b/connect.c\nindex d3448c2..0f48cde 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -490,6 +490,8 @@ static void git_tcp_connect(int fd[2], const char *host, const char *port,\n }\n \n \n+static char *get_port(char *host);\n+\n static char *git_proxy_command;\n \n static int git_proxy_command_options(const char *var, const char *value,\n@@ -517,10 +519,14 @@ static int git_proxy_command_options(const char *var, const char *value,\n \t\t\t/* matches everybody */\n \t\t\tmatchlen = strlen(value);\n \t\telse {\n-\t\t\thostlen = strlen(for_pos + 5);\n+\t\t\tstruct strbuf host = STRBUF_INIT;\n+\t\t\tchar *port;\n+\t\t\tstrbuf_addstr(&host, for_pos + 5);\n+\t\t\tport = get_port(host.buf);\n+\t\t\thostlen = strlen(host.buf);\n \t\t\tif (rhost_len < hostlen)\n \t\t\t\tmatchlen = -1;\n-\t\t\telse if (!strncmp(for_pos + 5,\n+\t\t\telse if (!strncmp(host.buf,\n \t\t\t\t\t  rhost_name + rhost_len - hostlen,\n \t\t\t\t\t  hostlen) &&\n \t\t\t\t ((rhost_len == hostlen) ||\n@@ -528,6 +534,14 @@ static int git_proxy_command_options(const char *var, const char *value,\n \t\t\t\tmatchlen = for_pos - value;\n \t\t\telse\n \t\t\t\tmatchlen = -1;\n+\t\t\tif (matchlen > 0 && port)\n+\t\t\t\tdie(\"core.gitProxy only supports \\\"for \"\n+\t\t\t\t    \"host/domain\\\", not \\\"for host:port\\\".\\n\"\n+\t\t\t\t    \"Please change your configuration \"\n+\t\t\t\t    \"accordingly.\\nPlease note that the port \"\n+\t\t\t\t    \"is given as second argument to the proxy \"\n+\t\t\t\t    \"script.\\n\");\n+\t\t\tstrbuf_release(&host);\n \t\t}\n \t\tif (0 <= matchlen) {\n \t\t\t/* core.gitproxy = none for kernel.org */\n@@ -706,7 +720,7 @@ struct child_process *git_connect(int fd[2], const char *url,\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(host))\n \t\t\tconn = git_proxy_connect(fd, host, port);\n \t\telse\n \t\t\tgit_tcp_connect(fd, host, port, flags);\ndiff --git a/t/t5532-fetch-proxy.sh b/t/t5532-fetch-proxy.sh\nindex 51c9669..efa80f8 100755\n--- a/t/t5532-fetch-proxy.sh\n+++ b/t/t5532-fetch-proxy.sh\n@@ -33,7 +33,9 @@ test_expect_success 'setup proxy script' '\n \n test_expect_success 'setup local repo' '\n \tgit remote add fake git://example.com/remote &&\n-\tgit config core.gitproxy ./proxy\n+\tgit remote add fake_b git://example.org/remote &&\n+\tgit config core.gitproxy \"./proxy for example.com\" &&\n+\tgit config --add core.gitproxy \"./proxy for example.org:9418\"\n '\n \n test_expect_success 'fetch through proxy works' '\n@@ -43,4 +45,8 @@ test_expect_success 'fetch through proxy works' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'core.gitProxy with port fails' '\n+\ttest_must_fail git fetch fake_b\n+'\n+\n test_done\n-- \n2.8.2.411.ga570dec.dirty\n"},{"id":"286754","messageId":"20160517013554.22578-4-mh@glandium.org","threadId":"42351","inReplyTo":"20160517013554.22578-1-mh@glandium.org","subject":"[PATCH v6 3/9] connect: fill the host header in the git protocol with the host and port variables","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2016-05-17T01:35:48Z","receivedAt":"2016-05-17T01:35:48Z","isPatch":true,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"The last use of the hostandport variable, besides being strdup'ed before\nbeing split into host and port, is to fill the host header in the git\nprotocol.\n\nInstead of relying on parse_connect_url() to return a host:port string\nthat makes sense there, construct one from the host and port variables.\n\nSigned-off-by: Mike Hommey <mh@glandium.org>\n---\n connect.c | 27 ++++++++++++++++++++-------\n 1 file changed, 20 insertions(+), 7 deletions(-)\n\ndiff --git a/connect.c b/connect.c\nindex 0f48cde..0676ee0 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -709,11 +709,24 @@ 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\tchar *override_vhost = getenv(\"GIT_OVERRIDE_VIRTUAL_HOST\");\n+\t\tif (override_vhost)\n+\t\t\tstrbuf_addstr(&target_host, override_vhost);\n+\t\telse {\n+\t\t\t/* If the host contains a colon (ipv6 address), it\n+\t\t\t * needs to be enclosed with square brackets. */\n+\t\t\tconst char *colon = strchr(host, ':');\n+\t\t\tif (colon)\n+\t\t\t\tstrbuf_addch(&target_host, '[');\n+\t\t\tstrbuf_addstr(&target_host, host);\n+\t\t\tif (colon)\n+\t\t\t\tstrbuf_addch(&target_host, ']');\n+\t\t\tif (port) {\n+\t\t\t\tstrbuf_addch(&target_host, ':');\n+\t\t\t\tstrbuf_addstr(&target_host, port);\n+\t\t\t}\n+\t\t}\n \n \t\ttransport_check_allowed(\"git\");\n \n@@ -734,8 +747,8 @@ 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     target_host.buf, 0);\n+\t\tstrbuf_release(&target_host);\n \t} else {\n \t\tconn = xmalloc(sizeof(*conn));\n \t\tchild_process_init(conn);\n-- \n2.8.2.411.ga570dec.dirty\n"},{"id":"286749","messageId":"20160517013554.22578-5-mh@glandium.org","threadId":"42351","inReplyTo":"20160517013554.22578-1-mh@glandium.org","subject":"[PATCH v6 4/9] connect: make parse_connect_url() return separated host and port","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2016-05-17T01:35:49Z","receivedAt":"2016-05-17T01:35:49Z","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             | 33 +++++++++++++++++++++------------\n t/t5500-fetch-pack.sh | 32 +++++++++++++++++++++-----------\n 2 files changed, 42 insertions(+), 23 deletions(-)\n\ndiff --git a/connect.c b/connect.c\nindex 0676ee0..4ce83f7 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -603,10 +603,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@@ -661,7 +662,20 @@ 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/* 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}\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@@ -683,8 +697,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@@ -694,13 +707,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@@ -766,9 +778,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 (!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@@ -776,8 +785,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@@ -836,8 +845,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.2.411.ga570dec.dirty\n"},{"id":"286746","messageId":"20160517013554.22578-6-mh@glandium.org","threadId":"42351","inReplyTo":"20160517013554.22578-1-mh@glandium.org","subject":"[PATCH v6 5/9] connect: group CONNECT_DIAG_URL handling code","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2016-05-17T01:35:50Z","receivedAt":"2016-05-17T01:35:50Z","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 4ce83f7..3a77b2f 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -708,7 +708,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@@ -778,20 +778,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.2.411.ga570dec.dirty\n"},{"id":"286752","messageId":"20160517013554.22578-7-mh@glandium.org","threadId":"42351","inReplyTo":"20160517013554.22578-1-mh@glandium.org","subject":"[PATCH v6 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-17T01:35:51Z","receivedAt":"2016-05-17T01:35:51Z","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 | 33 +++++++++++++++++++++++++--------\n 1 file changed, 25 insertions(+), 8 deletions(-)\n\ndiff --git a/connect.c b/connect.c\nindex 3a77b2f..2659b40 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -602,11 +602,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@@ -664,16 +666,23 @@ 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/* 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+\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@@ -697,7 +706,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@@ -707,11 +716,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@@ -818,7 +830,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                 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@@ -831,6 +847,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.2.411.ga570dec.dirty\n"},{"id":"286755","messageId":"20160517013554.22578-8-mh@glandium.org","threadId":"42351","inReplyTo":"20160517013554.22578-1-mh@glandium.org","subject":"[PATCH v6 7/9] connect: change the --diag-url output to separate user and host","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2016-05-17T01:35:52Z","receivedAt":"2016-05-17T01:35:52Z","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 2659b40..dcaf32f 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -720,10 +720,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.2.411.ga570dec.dirty\n"},{"id":"286753","messageId":"20160517013554.22578-9-mh@glandium.org","threadId":"42351","inReplyTo":"20160517013554.22578-1-mh@glandium.org","subject":"[PATCH v6 8/9] connect: actively reject git:// urls with a user part","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2016-05-17T01:35:53Z","receivedAt":"2016-05-17T01:35:53Z","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 dcaf32f..7360d57 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -733,6 +733,9 @@ struct child_process *git_connect(int fd[2], const char *url,\n \t\t */\n \t\tstruct strbuf target_host = STRBUF_INIT;\n \t\tchar *override_vhost = getenv(\"GIT_OVERRIDE_VIRTUAL_HOST\");\n+\t\tif (user)\n+\t\t\tdie(\"user@host is not allowed in git:// urls\");\n+\n \t\tif (override_vhost)\n \t\t\tstrbuf_addstr(&target_host, override_vhost);\n \t\telse {\n-- \n2.8.2.411.ga570dec.dirty\n"},{"id":"286751","messageId":"20160517013554.22578-10-mh@glandium.org","threadId":"42351","inReplyTo":"20160517013554.22578-1-mh@glandium.org","subject":"[PATCH v6 9/9] connect: move ssh command line preparation to a separate function","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2016-05-17T01:35:54Z","receivedAt":"2016-05-17T01:35: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 | 108 +++++++++++++++++++++++++++++++++-----------------------------\n 1 file changed, 58 insertions(+), 50 deletions(-)\n\ndiff --git a/connect.c b/connect.c\nindex 7360d57..38cdffc 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -690,6 +690,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@@ -784,59 +839,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                 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.2.411.ga570dec.dirty\n"},{"id":"287130","messageId":"xmqqfutccuq5.fsf@gitster.mtv.corp.google.com","threadId":"42351","inReplyTo":"20160517013554.22578-2-mh@glandium.org","subject":"Re: [PATCH v6 1/9] connect: call get_host_and_port() earlier","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-20T20:58:26Z","receivedAt":"2016-05-20T20:58:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Mike Hommey <mh@glandium.org> writes:\n\n> Currently, get_host_and_port() is called in git_connect() for the ssh\n> protocol, and in git_tcp_connect_sock() for the git protocol. Instead\n> of doing this, just call it from a single place, right after\n> parse_connect_url(), and pass the host and port separately to\n> git_*_connect() functions.\n>\n> We however preserve hostandport, at least for now.\n>\n> Note that in git_tcp_connect_sock, the port was set to \"<none>\" in a\n> case that never can happen, so that code path was removed.\n\nCan never happen because?\n\n  !*port means get_host_and_port() made the \"port\" pointer point at\n  a NUL byte.  That does not happen because the only case port is\n  moved by that function is to have it point at a byte after we\n  found ':', and the \"port\" string is a decimal integer whose value\n  is between 0 and 65535, so there is no way port points at an empty\n  string.\n\nOK.\n\n>  struct child_process *git_connect(int fd[2], const char *url,\n>  \t\t\t\t  const char *prog, int flags)\n> ...\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> ...\n> @@ -737,22 +737,20 @@ 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\tif (!port)\n> -\t\t\t\tport = get_port(ssh_host);\n> +\t\t\t\tport = get_port(host);\n\nThis looks strange.  We asked get_host_and_port() to split\nhostandport into host and port already, and learned that port is\nempty, i.e. it wasn't <host>:<port> where <port> is a decimal\ninteger between 0 and 65535.  It could have been \"<host>:\" in which\ncase get_host_and_port() would have turned that colon into NUL.\n\nWould there be a case where this get_port() call does anthing\nuseful?\n"},{"id":"287132","messageId":"xmqqbn40cser.fsf@gitster.mtv.corp.google.com","threadId":"42351","inReplyTo":"20160517013554.22578-3-mh@glandium.org","subject":"Re: [PATCH v6 2/9] connect: only match the host with core.gitProxy","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-20T21:48:28Z","receivedAt":"2016-05-20T21:48:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Mike Hommey <mh@glandium.org> writes:\n\n> Currently, core.gitProxy doesn't actually match purely on domain names\n> as documented: it also matches ports.\n>\n> So a core.gitProxy value like \"script for kernel.org\" doesn't make the\n> script called for an url like git://kernel.org:port/path, while it is\n> called for git://kernel.org/path.\n\nLet's recap.  When you are accessing \"git://kernel.org:9418/path\"\n\n - \"script1 for kernel.org\" does not match, which may be\n   counter-intuitive;\n\n - \"script2 for kernel.org:9418\" matches and gets used.\n\nIsn't that what the current code does?  If so, I suspect that\nallowing the first one to match may be an improvement with slight\nbackward incompatibility that we agreed that we do not mind.\n\n\tNote for those other than Mike: The reason why we do not\n\tcare about the breakage of the backward compatibility is\n\tbecause it would only matter when you configured both\n\t\"script1 for kernel.org\" and \"script2 for kernel.org:9418\",\n\tso that URL can be used to differenciate which proxy script\n\tto use.  In such a configuration, we used to call script2,\n\tbut \"fixing\" it would make it call script1.  Given that the\n\tproxy script takes the port number as its parameter, this\n\tcan be worked around easily by introducing a new script0\n\tthat switches between script1 and script2 based on the port\n\tnumber parameter.  IOW, we accept the compatibility breakage\n\tbecause adjusting is easy.\n\nWe of course need to warn the user if we do this.  That is\n\n - If the URL we access has :<port>, and we apply <host>-only\n   core.gitProxy entry to it, and\n\n - If the user has core.gitProxy entry for <host>:<port>,\n\nwe definitely need to warn so that the user is told that the \"easy\nadjustment\" is necessary.\n\nThinking aloud a bit more, if the user has \"script1 for kernel.org\"\nwithout \"script2 for kernel.org:9418\", and relied on the fact that\nhaving :9418 that is not necessary (as it is the default port) in\nthe URL lets her bypass the proxy script, the user also needs to be\ntold that we have changed the rule of the game and her configuration\nneeds to be updated.  So the above condition to warn would need to\nbe loosened, i.e. \"if the URL being accessed has :<port>, and if we\napply gitProxy specified with only <host>\" we need to warn.\n\nA proper transition plan is necessary; if the long term ideal is to\nuse \"script1 for kernel.org\" for \"git://kernel.org:9418/path\", we do\nnot want to keep giving this warning forever.\n\nAnd if we need a transition plan _anyway_, we probably should have a\nperiod during which those who have been relying on the fact that\n\"script2 for kernel.org:9418\" was a supported way to specify proxy\nfor \"git://kernel.org:9418/path\" would get a warning but will still\nuse \"script2 for kernel.org:9418\" as a valid core.gitProxy\nspecification.\n\n> This per-port behavior seems like an oversight rather than a deliberate\n> choice, so, make git://kernel.org:port/path call the gitProxy script in\n> the case described above.\n\nSo, even if we agree that per-port behaviour is not something we\nwould use if we were building the system without any existing users\ntoday, I do not think we want \"git now fails with an error\" at all.\nIt goes against the approach Git takes to give smooth transtion to\nusers when we must break backward compatibility.\n\n> However, if people have been relying on this behavior, git now fails\n> with an error message inviting for a configuration change.\n"},{"id":"287135","messageId":"20160520221430.GA28273@glandium.org","threadId":"42351","inReplyTo":"xmqqfutccuq5.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v6 1/9] connect: call get_host_and_port() earlier","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2016-05-20T22:14:30Z","receivedAt":"2016-05-20T22:14:30Z","isPatch":true,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"On Fri, May 20, 2016 at 01:58:26PM -0700, Junio C Hamano wrote:\n> Mike Hommey <mh@glandium.org> writes:\n> \n> > Currently, get_host_and_port() is called in git_connect() for the ssh\n> > protocol, and in git_tcp_connect_sock() for the git protocol. Instead\n> > of doing this, just call it from a single place, right after\n> > parse_connect_url(), and pass the host and port separately to\n> > git_*_connect() functions.\n> >\n> > We however preserve hostandport, at least for now.\n> >\n> > Note that in git_tcp_connect_sock, the port was set to \"<none>\" in a\n> > case that never can happen, so that code path was removed.\n> \n> Can never happen because?\n> \n>   !*port means get_host_and_port() made the \"port\" pointer point at\n>   a NUL byte.  That does not happen because the only case port is\n>   moved by that function is to have it point at a byte after we\n>   found ':', and the \"port\" string is a decimal integer whose value\n>   is between 0 and 65535, so there is no way port points at an empty\n>   string.\n> \n> OK.\n\nDo you want me to add this to the commit message in a possible v7?\n\n> >  struct child_process *git_connect(int fd[2], const char *url,\n> >  \t\t\t\t  const char *prog, int flags)\n> > ...\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> > ...\n> > @@ -737,22 +737,20 @@ 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\tif (!port)\n> > -\t\t\t\tport = get_port(ssh_host);\n> > +\t\t\t\tport = get_port(host);\n> \n> This looks strange.  We asked get_host_and_port() to split\n> hostandport into host and port already, and learned that port is\n> empty, i.e. it wasn't <host>:<port> where <port> is a decimal\n> integer between 0 and 65535.  It could have been \"<host>:\" in which\n> case get_host_and_port() would have turned that colon into NUL.\n> \n> Would there be a case where this get_port() call does anthing\n> useful?\n\nv3 of this series did remove this get_port(), and broke the\n'[host:port]:path' syntax as a consequence. The reason this happens is\nthat get_host_and_port, in that case, is called with [host:port], sees\nthe square brackets, and searches the port *after* the closing bracket,\nbecause the usual case where square brackets appear is ipv6 addresses,\nwhich contain colons, and the brackets in that case are used to separate\nthe host and the port.\n\nIn that case, get_host_and_port returns \"host:port\" and null.\n\nMike\n"},{"id":"287137","messageId":"xmqqwpmobcd4.fsf@gitster.mtv.corp.google.com","threadId":"42351","inReplyTo":"20160520221430.GA28273@glandium.org","subject":"Re: [PATCH v6 1/9] connect: call get_host_and_port() earlier","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-20T22:20:23Z","receivedAt":"2016-05-20T22:20:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Mike Hommey <mh@glandium.org> writes:\n\n>> Can never happen because?\n>> \n>>   !*port means get_host_and_port() made the \"port\" pointer point at\n>>   a NUL byte.  That does not happen because the only case port is\n>>   moved by that function is to have it point at a byte after we\n>>   found ':', and the \"port\" string is a decimal integer whose value\n>>   is between 0 and 65535, so there is no way port points at an empty\n>>   string.\n>> \n>> OK.\n>\n> Do you want me to add this to the commit message in a possible v7?\n\nNo.\n\nI was merely thinking aloud to see if \"in a case that never can\nhappen\" is sufficient decsription.  I think it is ;-)\n\n>> This looks strange....\n> v3 of this series did remove this get_port(), and broke the\n> '[host:port]:path' syntax as a consequence. The reason this happens is\n> that get_host_and_port, in that case, is called with [host:port], sees\n> the square brackets, and searches the port *after* the closing bracket,\n> because the usual case where square brackets appear is ipv6 addresses,\n> which contain colons, and the brackets in that case are used to separate\n> the host and the port.\n>\n> In that case, get_host_and_port returns \"host:port\" and null.\n\nDoesn't that indicate that this codepath deserves some in-code\ncomment?\n"},{"id":"287138","messageId":"20160520222831.GA7752@glandium.org","threadId":"42351","inReplyTo":"xmqqwpmobcd4.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v6 1/9] connect: call get_host_and_port() earlier","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2016-05-20T22:28:31Z","receivedAt":"2016-05-20T22:28:31Z","isPatch":true,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"On Fri, May 20, 2016 at 03:20:23PM -0700, Junio C Hamano wrote:\n> Mike Hommey <mh@glandium.org> writes:\n> \n> >> Can never happen because?\n> >> \n> >>   !*port means get_host_and_port() made the \"port\" pointer point at\n> >>   a NUL byte.  That does not happen because the only case port is\n> >>   moved by that function is to have it point at a byte after we\n> >>   found ':', and the \"port\" string is a decimal integer whose value\n> >>   is between 0 and 65535, so there is no way port points at an empty\n> >>   string.\n> >> \n> >> OK.\n> >\n> > Do you want me to add this to the commit message in a possible v7?\n> \n> No.\n> \n> I was merely thinking aloud to see if \"in a case that never can\n> happen\" is sufficient decsription.  I think it is ;-)\n> \n> >> This looks strange....\n> > v3 of this series did remove this get_port(), and broke the\n> > '[host:port]:path' syntax as a consequence. The reason this happens is\n> > that get_host_and_port, in that case, is called with [host:port], sees\n> > the square brackets, and searches the port *after* the closing bracket,\n> > because the usual case where square brackets appear is ipv6 addresses,\n> > which contain colons, and the brackets in that case are used to separate\n> > the host and the port.\n> >\n> > In that case, get_host_and_port returns \"host:port\" and null.\n> \n> Doesn't that indicate that this codepath deserves some in-code\n> comment?\n\nWhat would be the usual way you'd do this? separate patch? or just doing\nit in one of the patches that touches the surrounding code?\n\nMike\n"},{"id":"287139","messageId":"20160520223054.GB7752@glandium.org","threadId":"42351","inReplyTo":"xmqqbn40cser.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v6 2/9] connect: only match the host with core.gitProxy","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2016-05-20T22:30:54Z","receivedAt":"2016-05-20T22:30:54Z","isPatch":true,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"On Fri, May 20, 2016 at 02:48:28PM -0700, Junio C Hamano wrote:\n> So, even if we agree that per-port behaviour is not something we\n> would use if we were building the system without any existing users\n> today, I do not think we want \"git now fails with an error\" at all.\n> It goes against the approach Git takes to give smooth transtion to\n> users when we must break backward compatibility.\n\nI don't disagree. I went with a hard fail because it was easier. I'm\nnot too keen blocking this series on this transition happening. So I'll\ntry to finish this series without this change, and we can separate this\ntransition discussion from the rest of the connect.c changes.\n\nMike\n"},{"id":"287141","messageId":"xmqqoa80baos.fsf@gitster.mtv.corp.google.com","threadId":"42351","inReplyTo":"20160520223054.GB7752@glandium.org","subject":"Re: [PATCH v6 2/9] connect: only match the host with core.gitProxy","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-20T22:56:35Z","receivedAt":"2016-05-20T22:56:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Mike Hommey <mh@glandium.org> writes:\n\n> On Fri, May 20, 2016 at 02:48:28PM -0700, Junio C Hamano wrote:\n>> So, even if we agree that per-port behaviour is not something we\n>> would use if we were building the system without any existing users\n>> today, I do not think we want \"git now fails with an error\" at all.\n>> It goes against the approach Git takes to give smooth transtion to\n>> users when we must break backward compatibility.\n>\n> I don't disagree. I went with a hard fail because it was easier. I'm\n> not too keen blocking this series on this transition happening. So I'll\n> try to finish this series without this change, and we can separate this\n> transition discussion from the rest of the connect.c changes.\n\nYeah, I was thinking about the same thing as a short-term\ndirection.\n\nThanks.\n"}]}