{"thread":{"id":"42337","subject":"[PATCH v5 3/9] connect: fill the host header in the git protocol with the host and port variables","startedAt":"2016-05-16T00:07:31Z","lastAt":"2016-05-16T22:54:25Z","messageCount":16,"participants":["Mike Hommey","Junio C Hamano"],"isPatch":true,"patchVersion":5,"patchTotal":9},"messages":[{"id":"286620","messageId":"20160516000740.19042-1-mh@glandium.org","threadId":"42337","inReplyTo":null,"subject":"[PATCH v5 0/9] connect: various cleanups","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2016-05-16T00:07:31Z","receivedAt":"2016-05-16T00:07:31Z","isPatch":true,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"I removed the two controvertial patches, and applied the various suggestions\nfrom the last cycle.\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             | 222 ++++++++++++++++++++++++++++----------------------\n t/t5500-fetch-pack.sh |  42 +++++++---\n 2 files changed, 157 insertions(+), 107 deletions(-)\n\n-- \n2.8.2.411.ga331486\n"},{"id":"286627","messageId":"20160516000740.19042-2-mh@glandium.org","threadId":"42337","inReplyTo":"20160516000740.19042-1-mh@glandium.org","subject":"[PATCH v5 1/9] connect: call get_host_and_port() earlier","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2016-05-16T00:07:32Z","receivedAt":"2016-05-16T00:07:32Z","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.ga331486\n"},{"id":"286626","messageId":"20160516000740.19042-3-mh@glandium.org","threadId":"42337","inReplyTo":"20160516000740.19042-1-mh@glandium.org","subject":"[PATCH v5 2/9] connect: only match the host with core.gitProxy","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2016-05-16T00:07:33Z","receivedAt":"2016-05-16T00:07:33Z","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\nSigned-off-by: Mike Hommey <mh@glandium.org>\n---\n connect.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/connect.c b/connect.c\nindex d3448c2..2a08318 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -706,7 +706,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);\n-- \n2.8.2.411.ga331486\n"},{"id":"286619","messageId":"20160516000740.19042-4-mh@glandium.org","threadId":"42337","inReplyTo":"20160516000740.19042-1-mh@glandium.org","subject":"[PATCH v5 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-16T00:07:34Z","receivedAt":"2016-05-16T00:07:34Z","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 2a08318..ed1a00d 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -695,11 +695,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@@ -720,8 +733,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.ga331486\n"},{"id":"286624","messageId":"20160516000740.19042-5-mh@glandium.org","threadId":"42337","inReplyTo":"20160516000740.19042-1-mh@glandium.org","subject":"[PATCH v5 4/9] connect: make parse_connect_url() return separated host and port","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2016-05-16T00:07:35Z","receivedAt":"2016-05-16T00:07:35Z","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             | 30 ++++++++++++++++++------------\n t/t5500-fetch-pack.sh | 32 +++++++++++++++++++++-----------\n 2 files changed, 39 insertions(+), 23 deletions(-)\n\ndiff --git a/connect.c b/connect.c\nindex ed1a00d..3428149 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,17 @@ 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/* The host might contain a user:password string, ignore it\n+\t\t * when searching for the port again */\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 +680,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 +690,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@@ -752,9 +761,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@@ -762,8 +768,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@@ -822,8 +828,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..9acba2b 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\" | grep -v host= | grep -v 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.ga331486\n"},{"id":"286628","messageId":"20160516000740.19042-6-mh@glandium.org","threadId":"42337","inReplyTo":"20160516000740.19042-1-mh@glandium.org","subject":"[PATCH v5 5/9] connect: group CONNECT_DIAG_URL handling code","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2016-05-16T00:07:36Z","receivedAt":"2016-05-16T00:07:36Z","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 3428149..8813f90 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -691,7 +691,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@@ -761,20 +761,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.ga331486\n"},{"id":"286621","messageId":"20160516000740.19042-7-mh@glandium.org","threadId":"42337","inReplyTo":"20160516000740.19042-1-mh@glandium.org","subject":"[PATCH v5 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-16T00:07:37Z","receivedAt":"2016-05-16T00:07:37Z","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 8813f90..c40ff35 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,13 +652,20 @@ 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/* The host might contain a user:password string, ignore it\n \t\t * when searching for the port again */\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@@ -680,7 +689,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@@ -690,11 +699,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@@ -801,7 +813,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@@ -814,6 +830,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.ga331486\n"},{"id":"286623","messageId":"20160516000740.19042-8-mh@glandium.org","threadId":"42337","inReplyTo":"20160516000740.19042-1-mh@glandium.org","subject":"[PATCH v5 7/9] connect: change the --diag-url output to separate user and host","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2016-05-16T00:07:38Z","receivedAt":"2016-05-16T00:07:38Z","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 c40ff35..df15ff3 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -703,10 +703,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 9acba2b..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\" | grep -v host= | grep -v 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.ga331486\n"},{"id":"286625","messageId":"20160516000740.19042-9-mh@glandium.org","threadId":"42337","inReplyTo":"20160516000740.19042-1-mh@glandium.org","subject":"[PATCH v5 8/9] connect: actively reject git:// urls with a user part","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2016-05-16T00:07:39Z","receivedAt":"2016-05-16T00:07:39Z","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 df15ff3..fdd40b0 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -716,6 +716,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.ga331486\n"},{"id":"286622","messageId":"20160516000740.19042-10-mh@glandium.org","threadId":"42337","inReplyTo":"20160516000740.19042-1-mh@glandium.org","subject":"[PATCH v5 9/9] connect: move ssh command line preparation to a separate function","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2016-05-16T00:07:40Z","receivedAt":"2016-05-16T00:07:40Z","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 fdd40b0..ae9ef7b 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -673,6 +673,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@@ -767,59 +822,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.ga331486\n"},{"id":"286730","messageId":"xmqqvb2dzlee.fsf@gitster.mtv.corp.google.com","threadId":"42337","inReplyTo":"20160516000740.19042-3-mh@glandium.org","subject":"Re: [PATCH v5 2/9] connect: only match the host with core.gitProxy","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-16T22:30:01Z","receivedAt":"2016-05-16T22:30:01Z","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> 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\nHmph.  The fact that hostandport, not just host after stripping\npossible \":port\" part, is passed to the function smells like a\ndeliberate design to allow people to use different proxy for\ndifferent port, so I am not sure everybody agrees with your \"seems\nlike an oversight\".\n\nDon't existing users depend on the behaviour?  Isn't the change\nrobbing Peter to pay Paul?\n"},{"id":"286731","messageId":"20160516223639.GA16182@glandium.org","threadId":"42337","inReplyTo":"xmqqvb2dzlee.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v5 2/9] connect: only match the host with core.gitProxy","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2016-05-16T22:36:39Z","receivedAt":"2016-05-16T22:36:39Z","isPatch":true,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"On Mon, May 16, 2016 at 03:30:01PM -0700, Junio C Hamano wrote:\n> 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> > 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> \n> Hmph.  The fact that hostandport, not just host after stripping\n> possible \":port\" part, is passed to the function smells like a\n> deliberate design to allow people to use different proxy for\n> different port, so I am not sure everybody agrees with your \"seems\n> like an oversight\".\n> \n> Don't existing users depend on the behaviour?  Isn't the change\n> robbing Peter to pay Paul?\n\nThe gitProxy script gets the port passed. Why would you need different\nscripts for different ports if the port is passed as an argument? Also,\nif it's deliberate, it's widely undocumented.\n\nMike\n"},{"id":"286732","messageId":"xmqqr3d1zkz7.fsf@gitster.mtv.corp.google.com","threadId":"42337","inReplyTo":"20160516000740.19042-5-mh@glandium.org","subject":"Re: [PATCH v5 4/9] connect: make parse_connect_url() return separated host and port","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-16T22:39:08Z","receivedAt":"2016-05-16T22:39:08Z","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> +\tget_host_and_port(&host, &port);\n> +\n> +\tif (*host && !port) {\n> +\t\t/* The host might contain a user:password string, ignore it\n> +\t\t * when searching for the port again */\n> +\t\tchar *end_user = strrchr(host, '@');\n> +\t\tport = get_port(end_user ? end_user : host);\n\nScanning from the right because host part would never have '@', but\nthere could be an invalid URL with an unquoted '@' in userinfo part?\nThen this makes sense.\n\n> diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh\n> index 91a69fc..9acba2b 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\" | grep -v host= | grep -v port= >actual &&\n\nA single process:\n\n\t... | grep -v -e '^host=' -e '^port='\n\nperhaps?\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\nThis makes the diag output simpler and allows the caller to expect\nthe same set of variables, which is good.\n"},{"id":"286733","messageId":"xmqqmvnpzkvj.fsf@gitster.mtv.corp.google.com","threadId":"42337","inReplyTo":"20160516000740.19042-9-mh@glandium.org","subject":"Re: [PATCH v5 8/9] connect: actively reject git:// urls with a user part","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-16T22:41:20Z","receivedAt":"2016-05-16T22:41:20Z","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, urls of the for git://user@host don't work because user@host\n> is not resolving at the DNS level, but we shouldn't be relying on it\n> being an invalid host name, and actively reject it for containing a\n> username in the first place.\n\nMakes sense.  Connecting to host by stripping user@ would probably\ngive us a better behaviour, but this is a good first step even if we\nare aiming for that endgame state.\n\n>\n> Signed-off-by: Mike Hommey <mh@glandium.org>\n> ---\n>  connect.c | 3 +++\n>  1 file changed, 3 insertions(+)\n>\n> diff --git a/connect.c b/connect.c\n> index df15ff3..fdd40b0 100644\n> --- a/connect.c\n> +++ b/connect.c\n> @@ -716,6 +716,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"},{"id":"286735","messageId":"20160516225341.GA19595@glandium.org","threadId":"42337","inReplyTo":"xmqqr3d1zkz7.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v5 4/9] connect: make parse_connect_url() return separated host and port","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2016-05-16T22:53:41Z","receivedAt":"2016-05-16T22:53:41Z","isPatch":true,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"On Mon, May 16, 2016 at 03:39:08PM -0700, Junio C Hamano wrote:\n> Mike Hommey <mh@glandium.org> writes:\n> \n> > +\tget_host_and_port(&host, &port);\n> > +\n> > +\tif (*host && !port) {\n> > +\t\t/* The host might contain a user:password string, ignore it\n> > +\t\t * when searching for the port again */\n> > +\t\tchar *end_user = strrchr(host, '@');\n> > +\t\tport = get_port(end_user ? end_user : host);\n> \n> Scanning from the right because host part would never have '@', but\n> there could be an invalid URL with an unquoted '@' in userinfo part?\n> Then this makes sense.\n\nIndeed, I forgot to update the comment after the discussion on this\nlist.\n\n> > diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh\n> > index 91a69fc..9acba2b 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\" | grep -v host= | grep -v port= >actual &&\n> \n> A single process:\n> \n> \t... | grep -v -e '^host=' -e '^port='\n\nheh, I'm actually replacing it with a single egrep in a subsequent\npatch, following feedback from previous round, but missed that there was\nthis intermediate step with multiple greps still. Do you want an update\nanyways, or does only the final result really matter? I guess I can\nupdate it in any case, since I'll have to update the patch for the\ncomment above anyways.\n\nMike\n"},{"id":"286736","messageId":"xmqqinydzk9q.fsf@gitster.mtv.corp.google.com","threadId":"42337","inReplyTo":"20160516223639.GA16182@glandium.org","subject":"Re: [PATCH v5 2/9] connect: only match the host with core.gitProxy","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-16T22:54:25Z","receivedAt":"2016-05-16T22:54:25Z","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> The gitProxy script gets the port passed. Why would you need different\n> scripts for different ports if the port is passed as an argument? Also,\n> if it's deliberate, it's widely undocumented.\n\nFair enough.\n\nA user who has been working around thsi \"oversight\", would have\nrelied on her proxy configured with 'script for myhost.xz:9111' to\nbe called for git://myhost.xz:9111/path, right?  We'd need to\nsomehow let her know that her configuration will be broken, but as\nlong as we can find a way to do so to ease the transition, I think\nthe updated \"if you want to use a different behaviour depending on\nport, use the port parameter\" is a lot more sensible behaviour than\nwhat we had traditionally.\n"}]}