{"thread":{"id":"42188","subject":"[PATCH 1/6] connect: remove get_port()","startedAt":"2016-05-01T06:02:47Z","lastAt":"2016-05-06T15:52:14Z","messageCount":46,"participants":["Mike Hommey","Torsten Bögershausen","Junio C Hamano","Jeff King","Eric Sunshine"],"isPatch":true,"patchVersion":1,"patchTotal":6},"messages":[{"id":"285065","messageId":"1462082573-17992-1-git-send-email-mh@glandium.org","threadId":"42188","inReplyTo":null,"subject":"[PATCH v3 0/6] connect: various cleanups","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2016-05-01T06:02:47Z","receivedAt":"2016-05-01T06:02:47Z","isPatch":true,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"\nOn Fri, Apr 29, 2016 at 09:58:37AM -0700, Junio C Hamano wrote:\n> So I would suggest restructuring this series to do\n> \n> * 2/3 (DIAG consolidation)\n> * refactoring in 3/3 but not s/static/extern/\n> \n> and in optional follow(s)-up, do\n> \n> * s/static/extern/ and update to *.h in 3/3\n> \n> * 1/3, but I do not think it is necessary for users of\n> prepare_ssh_command()\n\nI went a little further on the restructuring and did some more cleanup.\nThis could probably go even further, but that'd be well out of the scope\nI started those patches for :)\n\nThe order in which the changes have been done in this new series is\ndebatable, but I thought the split in smaller parts made things easier\nto review, even if they could have been done in different orders.\n\nI'm leaving the optional follows-up for now. I'll work around the functions\nbeing static somehow on my end.\n\n\nMike Hommey (6):\n connect: remove get_port()\n connect: uniformize and group CONNECT_DIAG_URL handling code\n connect: only match the host with core.gitProxy\n connect: pass separate host and port to git_tcp_connect and\n git_proxy_connect\n connect: don't xstrdup target_host\n connect: move ssh command line preparation to a separate function\n\n connect.c | 184 ++++++++++++++++++++++----------------------------\n t/t5500-fetch-pack.sh | 25 ++++---\n 2 files changed, 95 insertions(+), 114 deletions(-)\n\n-- \n2.8.1.16.g58dac65.dirty\n"},{"id":"285059","messageId":"1462082573-17992-2-git-send-email-mh@glandium.org","threadId":"42188","inReplyTo":"1462082573-17992-1-git-send-email-mh@glandium.org","subject":"[PATCH 1/6] connect: remove get_port()","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2016-05-01T06:02:48Z","receivedAt":"2016-05-01T06:02:48Z","isPatch":true,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"get_port() is only used as a fallback when get_host_and_port() does not\nreturn a port. But get_port() does the same search as\nget_host_and_port(), except get_host_and_port() starts from the end of\nthe host, respecting square brackets for ipv6 addresses, and get_port(),\noperating after get_host_and_port(), works on a modified host string\nthat has square brackes removed if there were any.\n\nI cannot think of any legal host:port string that would not have a port\nreturned by get_host_and_port() *and* have one returned by get_port().\nSo just remove get_port().\n\nSigned-off-by: Mike Hommey \n---\n connect.c | 19 -------------------\n 1 file changed, 19 deletions(-)\n\ndiff --git a/connect.c b/connect.c\nindex c53f3f1..45308ef 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -571,22 +571,6 @@ static struct child_process *git_proxy_connect(int fd[2], char *host)\n \treturn proxy;\n }\n \n-static char *get_port(char *host)\n-{\n-\tchar *end;\n-\tchar *p = strchr(host, ':');\n-\n-\tif (p) {\n-\t\tlong port = strtol(p + 1, &end, 10);\n-\t\tif (end != p + 1 && *end == '\\0' && 0 <= port && port < 65536) {\n-\t\t\t*p = '\\0';\n-\t\t\treturn p+1;\n-\t\t}\n-\t}\n-\n-\treturn NULL;\n-}\n-\n /*\n * Extract protocol and relevant parts from the specified connection URL.\n * The caller must free() the returned strings.\n@@ -742,9 +726,6 @@ struct child_process *git_connect(int fd[2], const char *url,\n \t\t\ttransport_check_allowed(\"ssh\");\n \t\t\tget_host_and_port(&ssh_host, &port);\n \n-\t\t\tif (!port)\n-\t\t\t\tport = get_port(ssh_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-- \n2.8.1.16.g58dac65.dirty\n"},{"id":"285064","messageId":"1462082573-17992-3-git-send-email-mh@glandium.org","threadId":"42188","inReplyTo":"1462082573-17992-1-git-send-email-mh@glandium.org","subject":"[PATCH 2/6] connect: uniformize and group CONNECT_DIAG_URL handling code","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2016-05-01T06:02:49Z","receivedAt":"2016-05-01T06:02:49Z","isPatch":true,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"The CONNECT_DIAG_URL code for PROTO_GIT and PROTO_SSH were different in\nsubtle ways. Those differences are not significant enough to justify the\nduplication of code, while this change also avoids starting to\ninitialize a connection at all when handling CONNECT_DIAG_URL.\n\nThis also moves the get_host_and_port() call earlier than it was\nhappening, while preserving hostandport intact (get_host_and_port()\npossibly altering the string it's passed), because all or hostandport,\nhost and port may be used.\n\nFurther changes are going to remove several uses of hostandport where\nget_host_and_port() is used further down the line.\n\nSigned-off-by: Mike Hommey \n---\n connect.c | 28 +++++++++-------------------\n t/t5500-fetch-pack.sh | 25 +++++++++++++++----------\n 2 files changed, 24 insertions(+), 29 deletions(-)\n\ndiff --git a/connect.c b/connect.c\nindex 45308ef..98449bc 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -656,7 +656,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@@ -667,10 +668,14 @@ 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-\tif ((flags & CONNECT_DIAG_URL) && (protocol != PROTO_SSH)) {\n+\thost = xstrdup(hostandport);\n+\tget_host_and_port(&host, &port);\n+\tif (flags & CONNECT_DIAG_URL) {\n \t\tprintf(\"Diag: url=%s\\n\", url ? url : \"NULL\");\n \t\tprintf(\"Diag: protocol=%s\\n\", prot_name(protocol));\n \t\tprintf(\"Diag: hostandport=%s\\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@@ -721,23 +726,7 @@ struct child_process *git_connect(int fd[2], const char *url,\n \t\tif (protocol == PROTO_SSH) {\n \t\t\tconst char *ssh;\n \t\t\tint putty = 0, tortoiseplink = 0;\n-\t\t\tchar *ssh_host = hostandport;\n-\t\t\tconst char *port = NULL;\n \t\t\ttransport_check_allowed(\"ssh\");\n-\t\t\tget_host_and_port(&ssh_host, &port);\n-\n-\t\t\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: 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(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@@ -779,7 +768,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@@ -793,6 +782,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 }\ndiff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh\nindex e5f83bf..360c59d 100755\n--- a/t/t5500-fetch-pack.sh\n+++ b/t/t5500-fetch-pack.sh\n@@ -537,7 +537,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@@ -546,22 +546,27 @@ 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+\tcase \"$4\" in\n+\t\tNONE)\n+\t\thostandport=$3\n+\t\t;;\n+\t\t*)\n+\t\thostandport=$3:$4\n+\t\t;;\n+\tesac\n+\tehost=$(echo $3 | tr -d \"[]\")\n+\tdiagport=\"Diag: port=$4\"\n \tcat >exp <<-EOF &&\n \tDiag: url=$1\n \tDiag: protocol=$pp\n-\tDiag: $uah=$ehost\n-\t$diagport\n+\tDiag: hostandport=$hostandport\n+\tDiag: userandhost=$ehost\n+\tDiag: port=$4\n \tDiag: path=$5\n \tEOF\n \tgrep -v \"^$\" exp >expected\n-- \n2.8.1.16.g58dac65.dirty\n"},{"id":"285060","messageId":"1462082573-17992-4-git-send-email-mh@glandium.org","threadId":"42188","inReplyTo":"1462082573-17992-1-git-send-email-mh@glandium.org","subject":"[PATCH 3/6] connect: only match the host with core.gitProxy","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2016-05-01T06:02:50Z","receivedAt":"2016-05-01T06:02:50Z","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 \n---\n connect.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/connect.c b/connect.c\nindex 98449bc..1c8ed6b 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -695,7 +695,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, hostandport);\n \t\telse\n \t\t\tgit_tcp_connect(fd, hostandport, flags);\n-- \n2.8.1.16.g58dac65.dirty\n"},{"id":"285063","messageId":"1462082573-17992-5-git-send-email-mh@glandium.org","threadId":"42188","inReplyTo":"1462082573-17992-1-git-send-email-mh@glandium.org","subject":"[PATCH 4/6] connect: pass separate host and port to git_tcp_connect and git_proxy_connect","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2016-05-01T06:02:51Z","receivedAt":"2016-05-01T06:02:51Z","isPatch":true,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"Instead of having each of them (and their callees) take a host:port\nstring and call get_host_and_port on it on their own, rely on the\nget_host_and_port that already happens in git_connect and pass down\nthe separate host and port strings we got out of it.\n\nNote that in git_tcp_connect_sock, the port was set to \"\" in a\ncase that never can happen, so that code path was removed.\n\nSigned-off-by: Mike Hommey \n---\n connect.c | 31 ++++++++++++++-----------------\n 1 file changed, 14 insertions(+), 17 deletions(-)\n\ndiff --git a/connect.c b/connect.c\nindex 1c8ed6b..b3fce84 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 = \"\";\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@@ -696,9 +693,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(host))\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-- \n2.8.1.16.g58dac65.dirty\n"},{"id":"285062","messageId":"1462082573-17992-6-git-send-email-mh@glandium.org","threadId":"42188","inReplyTo":"1462082573-17992-1-git-send-email-mh@glandium.org","subject":"[PATCH 5/6] connect: don't xstrdup target_host","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2016-05-01T06:02:52Z","receivedAt":"2016-05-01T06:02:52Z","isPatch":true,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"Now that hostandport is left unmodified in git_connect (we don't pass\nit to get_host_and_port() anymore), we can avoid xstrdup'ing it.\n\nSigned-off-by: Mike Hommey \n---\n connect.c | 5 ++---\n 1 file changed, 2 insertions(+), 3 deletions(-)\n\ndiff --git a/connect.c b/connect.c\nindex b3fce84..e2b976e 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -683,9 +683,9 @@ struct child_process *git_connect(int fd[2], const char *url,\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\t\ttarget_host = target_host;\n \t\telse\n-\t\t\ttarget_host = xstrdup(hostandport);\n+\t\t\ttarget_host = hostandport;\n \n \t\ttransport_check_allowed(\"git\");\n \n@@ -707,7 +707,6 @@ struct child_process *git_connect(int fd[2], const char *url,\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} else {\n \t\tconn = xmalloc(sizeof(*conn));\n \t\tchild_process_init(conn);\n-- \n2.8.1.16.g58dac65.dirty\n"},{"id":"285061","messageId":"1462082573-17992-7-git-send-email-mh@glandium.org","threadId":"42188","inReplyTo":"1462082573-17992-1-git-send-email-mh@glandium.org","subject":"[PATCH 6/6] connect: move ssh command line preparation to a separate function","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2016-05-01T06:02:53Z","receivedAt":"2016-05-01T06:02:53Z","isPatch":true,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"Signed-off-by: Mike Hommey \n---\n connect.c | 101 ++++++++++++++++++++++++++++++++++----------------------------\n 1 file changed, 55 insertions(+), 46 deletions(-)\n\ndiff --git a/connect.c b/connect.c\nindex e2b976e..db78eb2 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -639,6 +639,58 @@ static enum protocol parse_connect_url(const char *url_orig, char **ret_host,\n \n static struct child_process no_fork = CHILD_PROCESS_INIT;\n \n+static int prepare_ssh_command(struct argv_array *cmd, const char *host,\n+\t\t\t 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+\targv_array_push(cmd, host);\n+\n+\treturn use_shell;\n+}\n+\n /*\n * This returns a dummy child_process if the transport protocol does not\n * need fork(2), or a struct child_process object if it does. Once done,\n@@ -717,55 +769,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\targv_array_push(&conn->args, host);\n+\t\t\tconn->use_shell = prepare_ssh_command(\n+\t\t\t\t&conn->args, 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.1.16.g58dac65.dirty\n"},{"id":"285069","messageId":"e1056272-8c0e-0a0b-2295-4653a47cf86f@web.de","threadId":"42188","inReplyTo":"1462082573-17992-2-git-send-email-mh@glandium.org","subject":"Re: [PATCH 1/6] connect: remove get_port()","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2016-05-01T10:10:09Z","receivedAt":"2016-05-01T10:10:09Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On 2016-05-01 08.02, Mike Hommey wrote:\n> get_port() is only used as a fallback when get_host_and_port() does not\n> return a port. But get_port() does the same search as\n> get_host_and_port(), except get_host_and_port() starts from the end of\n> the host, respecting square brackets for ipv6 addresses, and get_port(),\n> operating after get_host_and_port(), works on a modified host string\n> that has square brackes removed if there were any.\ntypo: brackets.\n> \n> I cannot think of any legal host:port string that would not have a port\n> returned by get_host_and_port() *and* have one returned by get_port().\n> So just remove get_port().\n> \n> Signed-off-by: Mike Hommey \nDoes this pass the test-suite ?\nIt doesn't pass here, t5601:\n\nnot ok 39 - bracketed hostnames are still ssh\n#\n# git clone \"[myhost:123]:src\" ssh-bracket-clone &&\n# expect_ssh \"-p 123\" myhost src\n#\nnot ok 40 - uplink is not treated as putty\n#\n# copy_ssh_wrapper_as \"$TRASH_DIRECTORY/uplink\" &&\n# git clone \"[myhost:123]:src\" ssh-bracket-clone-uplink &&\n# expect_ssh \"-p 123\" myhost src\n#\nnot ok 41 - plink is treated specially (as putty)\n#\n# copy_ssh_wrapper_as \"$TRASH_DIRECTORY/plink\" &&\n# git clone \"[myhost:123]:src\" ssh-bracket-clone-plink-0 &&\n# expect_ssh \"-P 123\" myhost src\n#\nnot ok 42 - plink.exe is treated specially (as putty)\n#\n# copy_ssh_wrapper_as \"$TRASH_DIRECTORY/plink.exe\" &&\n# git clone \"[myhost:123]:src\" ssh-bracket-clone-plink-1 &&\n# expect_ssh \"-P 123\" myhost src\n#\nnot ok 43 - tortoiseplink is like putty, with extra arguments\n"},{"id":"285115","messageId":"ee3d1928-38c5-372c-223f-bd50bfb14930@web.de","threadId":"42188","inReplyTo":"1462082573-17992-3-git-send-email-mh@glandium.org","subject":"Re: [PATCH 2/6] connect: uniformize and group CONNECT_DIAG_URL handling code","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2016-05-01T13:37:24Z","receivedAt":"2016-05-01T13:37:24Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"\n\nOn 01.05.16 08:02, Mike Hommey wrote:\n> The CONNECT_DIAG_URL code for PROTO_GIT and PROTO_SSH were different in\n> subtle ways.\nYes, and there (historical) reasons for that.\nThe first implementation did support IPV6 with SSH:\n\n\ncommit 5ba884483fe1a5f9ce1ce5e3c5e1c37c0fd296c4\n [PATCH] GIT: Try all addresses for given remote name\n \n Try all addresses for given remote name until it succeeds. Also\n supports IPv6.\n\nThis lead to a regression, which was fixed here:\ncommit ce6f8e7ec2bbebe2472e23b684cae0a4adf325ad\n Fix git protocol connection 'port' override\n \n It was broken by the IPv6 patches - we need to remove the \":\" part from\n the hostname for a successful name lookup.\n\n\nLater the ssh:// syntax was added:\n\ncommit c05186cc38ca4605bff1f275619d7d0faeaf2fa5\n Support git+ssh:// and ssh+git:// URL\n \nLater support for [] was added:\ncommit 356bece0a2725818191b12f6e991490d52baa1d1\n GIT: Support [address] in URLs\n Allow IPv6address/IPvFuture enclosed by [] in URLs, like:\n git push '[3ffe:ffff:...:1]:GIT/git'\n or\n git push 'ssh://[3ffe:ffff:...:1]/GIT/git'\n\nLater, support for a trailing ':' was added:\ncommit 608d48b2207a6152839a9762c7a66f217bceb440\n Fix \"getaddrinfo()\" buglet\n So when somebody passes me a \"please pull\" request pointing to something\n like the following\n \tgit://git.kernel.org:/pub/scm/linux/kernel/git/mchehab/v4l-dvb.git\n (note the extraneous colon at the end of the host name), git would happily\n try to connect to port 0, which would generally just cause the remote to\n not even answer, and the \"connect()\" will take a long time to time out.\n\nThen it was possible to use ssh:// with a port number:\ncommit 2e7766655abd0312a6bf4db6a6ff141e7e4ac8b6\n URL: allow port specification in ssh:// URLs\n\n\nParsing of the port number was improved here:\ncommit 8f1482536ad680fcd738158e76e254a534f2e690\n connect.c: stricter port validation, silence compiler warning\n\n\nThen, it was possible to pass the port number to putty or plink:\ncommit 36ad53ffee6ed5b7c277cde660f526fd8ce3d68f\n connect.c: Support PuTTY plink and TortoisePlink as SSH on Windows\n\nThis caused a regression, see below.\n\nLater, there was a fix for [] together with a port number:\ncommit 7acf438215d1b0e8e47a707f3585de8486a2e5fe\n git: Wrong parsing of ssh urls with IPv6 literals ignores port\n\n\nA better way to distinguish \"local file system\" URLS from scp like URLs,\n(None of them has a scheme, but they are nice to use)\ncommit 60003340cda05f5ecd79ee8522b21eda038b994b\n clone: allow cloning local paths with colons in them\n\nThis fix lead to another regression, which was partly resolved here\n(And that's where test cases came in)\ncommit 8d3d28f5dba94a15a79975e4adc909c295c80d80\n clone: tighten \"local paths with colons\" check a bit\n \n\n In order to allow diagnostics in the parser, the diag-url feature was added:\ncommit 5610b7c0c6957cf0b236b6fac087c1f4dc209376\n git fetch-pack: add --diag-url\n \nThe the parser was improved:\ncommit 6a59974869c0a6e9399101bc02223b4c00a8aff2\n git fetch: support host:/~repo\n\nand refactored:\ncommit 83b058752707a6ba4af51ebc98c47913bc7d2d25\n git_connect(): refactor the port handling for ssh\n\nAnd more refactored:\ncommit c59ab2e52a64abd7fded97e0983a9b7f3d0508a0\n connect.c: refactor url parsing\n\nAnd more refactored:\ncommit a2036d7e00ad8aa16ba010a80078e10f0e4568a3\n git_connect(): use common return point\n\nLater the parser was improved:\ncommit 86ceb337ec340c7db9b060b90bfab05a08b8251b\n connect.c: allow ssh://user@[2001:db8::1]/repo.git\ncommit 3f55ccab8e0fec73c8e38b909e9bb4963bfb8f6a\n t5500: show user name and host in diag-url\n\nHowever, one of the refactoring broke a use case,\nwhich was valid before, leading to regressions around the world:\n\ncommit 6b6c5f7a2f66751a93afce54277a1f30ab0dc521\n connect.c: ignore extra colon after hostname\n\nNow back to the regression introduced by supporting plink/putty with\nports, it was improved by a preparing commit, followed by the fix:\n\ncommit baaf233755f71c057d28b9e8692e24d4fca7d22f\n connect: improve check for plink to reduce false positives\n\ncommit 37ee646e72d7f39d61a538e21a4c2721e32cb444\n connect: simplify SSH connection code path\n\nLater IPV6/IPV4 only connections had been supported:\ncommit c915f11eb4922e154e29cf62d3b549d65c06a170\n connect & http: support -4 and -6 switches for remote operations\n\nI skipped the dates and names (I was responsible for one regression)\nI hope this gives a half-correct overview,\nwhy I am reluctant to change any code in connect.c\nunless there is a fix for a real world problem.\n\nAnd even here, the test cases should be changed first (and reviewed) in an own commit.\nMarked as test_must_failure.\nThe c-code can be changed in the next commit, and the TC are marked as \"test_expect_success\"\n\n\nBack to another topic:\nIf you want to support the native Git-support for native HG via hg-serve,\nI will be happy to assist with reviews.\nPlease, if possible, don't touch connect.c at all.\n(Beside the memory leak fix).\n\nIt may be better to start with a real remote-helper and copy the code from connect.c\nAnd, later, if there are common code paths, refactor stuff.\n"},{"id":"285137","messageId":"20160501214322.GA13916@glandium.org","threadId":"42188","inReplyTo":"e1056272-8c0e-0a0b-2295-4653a47cf86f@web.de","subject":"Re: [PATCH 1/6] connect: remove get_port()","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2016-05-01T21:43:22Z","receivedAt":"2016-05-01T21:43:22Z","isPatch":true,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"On Sun, May 01, 2016 at 12:10:09PM +0200, Torsten Bögershausen wrote:\n> On 2016-05-01 08.02, Mike Hommey wrote:\n> > get_port() is only used as a fallback when get_host_and_port() does not\n> > return a port. But get_port() does the same search as\n> > get_host_and_port(), except get_host_and_port() starts from the end of\n> > the host, respecting square brackets for ipv6 addresses, and get_port(),\n> > operating after get_host_and_port(), works on a modified host string\n> > that has square brackes removed if there were any.\n> typo: brackets.\n> > \n> > I cannot think of any legal host:port string that would not have a port\n> > returned by get_host_and_port() *and* have one returned by get_port().\n> > So just remove get_port().\n> > \n> > Signed-off-by: Mike Hommey \n> Does this pass the test-suite ?\n> It doesn't pass here, t5601:\n> \n> not ok 39 - bracketed hostnames are still ssh\n> #\n> # git clone \"[myhost:123]:src\" ssh-bracket-clone &&\n> # expect_ssh \"-p 123\" myhost src\n> #\n> not ok 40 - uplink is not treated as putty\n> #\n> # copy_ssh_wrapper_as \"$TRASH_DIRECTORY/uplink\" &&\n> # git clone \"[myhost:123]:src\" ssh-bracket-clone-uplink &&\n> # expect_ssh \"-p 123\" myhost src\n> #\n> not ok 41 - plink is treated specially (as putty)\n> #\n> # copy_ssh_wrapper_as \"$TRASH_DIRECTORY/plink\" &&\n> # git clone \"[myhost:123]:src\" ssh-bracket-clone-plink-0 &&\n> # expect_ssh \"-P 123\" myhost src\n> #\n> not ok 42 - plink.exe is treated specially (as putty)\n> #\n> # copy_ssh_wrapper_as \"$TRASH_DIRECTORY/plink.exe\" &&\n> # git clone \"[myhost:123]:src\" ssh-bracket-clone-plink-1 &&\n> # expect_ssh \"-P 123\" myhost src\n> #\n> not ok 43 - tortoiseplink is like putty, with extra arguments\n> \n\nsigh. I was assuming all the relevant cases were tested in t5500, but\napparently they aren't.\n\nMike\n"},{"id":"285139","messageId":"20160501232007.GA18006@glandium.org","threadId":"42188","inReplyTo":"ee3d1928-38c5-372c-223f-bd50bfb14930@web.de","subject":"Re: [PATCH 2/6] connect: uniformize and group CONNECT_DIAG_URL handling code","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2016-05-01T23:20:07Z","receivedAt":"2016-05-01T23:20:07Z","isPatch":true,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"On Sun, May 01, 2016 at 03:37:24PM +0200, Torsten Bögershausen wrote:\n> I skipped the dates and names (I was responsible for one regression)\n> I hope this gives a half-correct overview,\n> why I am reluctant to change any code in connect.c\n> unless there is a fix for a real world problem.\n\nI don't agree that connect.c should be left untouched because it's\nfragile. On the contrary, if anything, it should be made less fragile.\n\nAnd the more I look at it, the more I'm tempted to just change the\nparse_connect_url function itself to do _all_ the parsing, instead of\nhaving some code paths invoke get_host_and_port and some others invoke\nget_port on top of that, for some effect that, when you look at the\ncode, you can't know what it's supposed to be. If anything, my attempts\nat cleaning up the code, and partially failing are a demonstration that\nthe code *does* need some clean up.\n\n> And even here, the test cases should be changed first (and reviewed) in an own commit.\n> Marked as test_must_failure.\n> The c-code can be changed in the next commit, and the TC are marked as \"test_expect_success\"\n\nWhat is the benefit from doing so?\n\n> Back to another topic:\n> If you want to support the native Git-support for native HG via hg-serve,\n> I will be happy to assist with reviews.\n> Please, if possible, don't touch connect.c at all.\n> (Beside the memory leak fix).\n> \n> It may be better to start with a real remote-helper and copy the code from connect.c\n> And, later, if there are common code paths, refactor stuff.\n\nThere *are* common code paths. Which is exactly why I'm trying to reuse\nconnect.c without copying it.\n\nMike\n"},{"id":"285140","messageId":"5726DE16.3030402@web.de","threadId":"42188","inReplyTo":"1462082573-17992-3-git-send-email-mh@glandium.org","subject":"Re: [PATCH 2/6] connect: uniformize and group CONNECT_DIAG_URL handling code","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2016-05-02T04:56:54Z","receivedAt":"2016-05-02T04:56:54Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On 05/01/2016 08:02 AM, Mike Hommey wrote:\n> +\tif (flags & CONNECT_DIAG_URL) {\n> \t\tprintf(\"Diag: url=%s\\n\", url ? url : \"NULL\");\n> \t\tprintf(\"Diag: protocol=%s\\n\", prot_name(protocol));\n> \t\tprintf(\"Diag: hostandport=%s\\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;\nDoes it make sense to write the host twice?\nIf things are cleaned up, would something like this make sense ?\n\n\t\tprintf(\"Diag: url=%s\\n\", url ? url : \"NULL\");\n \t\tprintf(\"Diag: protocol=%s\\n\", prot_name(protocol));\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\t\nor\n\t\tprintf(\"Diag: url=%s\\n\", url ? url : \"NULL\");\n \t\tprintf(\"Diag: protocol=%s\\n\", prot_name(protocol));\n if (user)\n\t\t\tprintf(\"Diag: user=%s\\n\", user);\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"},{"id":"285153","messageId":"20160502083133.GA20929@glandium.org","threadId":"42188","inReplyTo":"5726DE16.3030402@web.de","subject":"Re: [PATCH 2/6] connect: uniformize and group CONNECT_DIAG_URL handling code","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2016-05-02T08:31:33Z","receivedAt":"2016-05-02T08:31:33Z","isPatch":true,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"On Mon, May 02, 2016 at 06:56:54AM +0200, Torsten Bögershausen wrote:\n> On 05/01/2016 08:02 AM, Mike Hommey wrote:\n> > +\tif (flags & CONNECT_DIAG_URL) {\n> > \t\tprintf(\"Diag: url=%s\\n\", url ? url : \"NULL\");\n> > \t\tprintf(\"Diag: protocol=%s\\n\", prot_name(protocol));\n> > \t\tprintf(\"Diag: hostandport=%s\\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> Does it make sense to write the host twice?\n> If things are cleaned up, would something like this make sense ?\n> \n> \t\tprintf(\"Diag: url=%s\\n\", url ? url : \"NULL\");\n> \t\tprintf(\"Diag: protocol=%s\\n\", prot_name(protocol));\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\nThat's what I'm converging towards, in the end. There is one thing that\nneeds hostandport, though: the git protocol host header. But I don't\nreally like that parse_connect_url would return user, host, port, *and*\nhostandport. How about \"reconstructing\" hostandport in that case, adding\nsquare brackets when the host contains colons?\n\nBTW, the git protocol currently doesn't reject urls with users and\ndoesn't seem to handle them properly either. My changes would fix this\nby separating user and host at the parse_connect_url level, but the\nquestion then is what to do when there is a user part? die()?\n\nMike\n"},{"id":"285156","messageId":"57273A0B.5050409@web.de","threadId":"42188","inReplyTo":"20160502083133.GA20929@glandium.org","subject":"Re: [PATCH 2/6] connect: uniformize and group CONNECT_DIAG_URL handling code","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2016-05-02T11:29:15Z","receivedAt":"2016-05-02T11:29:15Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On 05/02/2016 10:31 AM, Mike Hommey wrote:\n> On Mon, May 02, 2016 at 06:56:54AM +0200, Torsten Bögershausen wrote:\n>> On 05/01/2016 08:02 AM, Mike Hommey wrote:\n>>> +\tif (flags & CONNECT_DIAG_URL) {\n>>> \t\tprintf(\"Diag: url=%s\\n\", url ? url : \"NULL\");\n>>> \t\tprintf(\"Diag: protocol=%s\\n\", prot_name(protocol));\n>>> \t\tprintf(\"Diag: hostandport=%s\\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>> Does it make sense to write the host twice?\n>> If things are cleaned up, would something like this make sense ?\n>>\n>> \t\tprintf(\"Diag: url=%s\\n\", url ? url : \"NULL\");\n>> \t\tprintf(\"Diag: protocol=%s\\n\", prot_name(protocol));\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>\n> That's what I'm converging towards, in the end. There is one thing that\n> needs hostandport, though: the git protocol host header. But I don't\n> really like that parse_connect_url would return user, host, port, *and*\n> hostandport. How about \"reconstructing\" hostandport in that case, adding\n> square brackets when the host contains colons?\n>\n> BTW, the git protocol currently doesn't reject urls with users and\n> doesn't seem to handle them properly either. My changes would fix this\n> by separating user and host at the parse_connect_url level, but the\n> question then is what to do when there is a user part? die()?\n>\n> Mike\n>\nThat is what happening:\ngit clone git://xx@git.kernel.org/pub/scm/git/git.git\nCloning into 'git'...\nfatal: Unable to look up xx@git.kernel.org (port 9418) (Name or service \nnot known)\n\nAnd that may explain, why we have different handling of ssh vs git:\nThe URL-scheme for git:// simply doesn't specify a user name at all.\n\ngit://host:[port]/path/to/repo\nKnowing that, the \"@\" will be feed into the name resolver,\nand that's OK.\n"},{"id":"285157","messageId":"20160502123858.GA26091@glandium.org","threadId":"42188","inReplyTo":"57273A0B.5050409@web.de","subject":"Re: [PATCH 2/6] connect: uniformize and group CONNECT_DIAG_URL handling code","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2016-05-02T12:38:58Z","receivedAt":"2016-05-02T12:38:58Z","isPatch":true,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"On Mon, May 02, 2016 at 01:29:15PM +0200, Torsten Bögershausen wrote:\n> On 05/02/2016 10:31 AM, Mike Hommey wrote:\n> > On Mon, May 02, 2016 at 06:56:54AM +0200, Torsten Bögershausen wrote:\n> > > On 05/01/2016 08:02 AM, Mike Hommey wrote:\n> > > > +\tif (flags & CONNECT_DIAG_URL) {\n> > > > \t\tprintf(\"Diag: url=%s\\n\", url ? url : \"NULL\");\n> > > > \t\tprintf(\"Diag: protocol=%s\\n\", prot_name(protocol));\n> > > > \t\tprintf(\"Diag: hostandport=%s\\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> > > Does it make sense to write the host twice?\n> > > If things are cleaned up, would something like this make sense ?\n> > > \n> > > \t\tprintf(\"Diag: url=%s\\n\", url ? url : \"NULL\");\n> > > \t\tprintf(\"Diag: protocol=%s\\n\", prot_name(protocol));\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> > \n> > That's what I'm converging towards, in the end. There is one thing that\n> > needs hostandport, though: the git protocol host header. But I don't\n> > really like that parse_connect_url would return user, host, port, *and*\n> > hostandport. How about \"reconstructing\" hostandport in that case, adding\n> > square brackets when the host contains colons?\n> > \n> > BTW, the git protocol currently doesn't reject urls with users and\n> > doesn't seem to handle them properly either. My changes would fix this\n> > by separating user and host at the parse_connect_url level, but the\n> > question then is what to do when there is a user part? die()?\n> > \n> > Mike\n> > \n> That is what happening:\n> git clone git://xx@git.kernel.org/pub/scm/git/git.git\n> Cloning into 'git'...\n> fatal: Unable to look up xx@git.kernel.org (port 9418) (Name or service not\n> known)\n> \n> And that may explain, why we have different handling of ssh vs git:\n> The URL-scheme for git:// simply doesn't specify a user name at all.\n> \n> git://host:[port]/path/to/repo\n> Knowing that, the \"@\" will be feed into the name resolver,\n> and that's OK.\n\nBut we might as well throw an error before, because @ is not going to\nappear in a valid hostname.\n\nMike\n"},{"id":"285216","messageId":"xmqqfuu0qfo4.fsf@gitster.mtv.corp.google.com","threadId":"42188","inReplyTo":"57273A0B.5050409@web.de","subject":"Re: [PATCH 2/6] connect: uniformize and group CONNECT_DIAG_URL handling code","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-02T22:05:31Z","receivedAt":"2016-05-02T22:05:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Torsten Bögershausen writes:\n\n> git://host:[port]/path/to/repo\n> Knowing that, the \"@\" will be feed into the name resolver,\n> and that's OK.\n\nIs it OK? It is plausible that our client side may even want to\naccept git://user:pass@host:port/local/part, and as an anonymous\nservice, allow it to go to git://host:port/local/part without\nsending user:pass part over the wire. Or with the same knowledge\nthat git:// is an anonymous service, it is also a plausible policy\nto error such a request out. To implement either needs a robust\nparsing of the URL, doesn't it?\n"},{"id":"285223","messageId":"xmqq37q0qcge.fsf@gitster.mtv.corp.google.com","threadId":"42188","inReplyTo":"xmqqfuu0qfo4.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 2/6] connect: uniformize and group CONNECT_DIAG_URL handling code","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-02T23:14:57Z","receivedAt":"2016-05-02T23:14:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Torsten Bögershausen writes:\n>\n>> git://host:[port]/path/to/repo\n>> Knowing that, the \"@\" will be feed into the name resolver,\n>> and that's OK.\n>\n> Is it OK? It is plausible that our client side may even want to\n> accept git://user:pass@host:port/local/part, and as an anonymous\n> service, allow it to go to git://host:port/local/part without\n> sending user:pass part over the wire. Or with the same knowledge\n> that git:// is an anonymous service, it is also a plausible policy\n> to error such a request out. To implement either needs a robust\n> parsing of the URL, doesn't it?\n\nTo put it differently, there is a vast difference between\n\n (1) knowing that xx@git.kernel.org is asking to access\n git.kernel.org as user 'xx' and failing because of a policy\n that says \"we do not send auth material over the wire when we\n know we are doing anonymous access\"; and\n\n (2) not caring the distinction between xx@git.kernel.org and\n git.kernel.org, and implicitly relying on the DNS to forbid '@'\n in the hostname and to return a look-up failure when the whole\n string of the former is taken as a hostname in order to fail\n that request.\n\nIf we want to fail the request, we should be in control of the\npolicy to fail. Even in an unlikely (and impossible) world where\nsuddenly resolver starts allowing \"xx@git.kernel.org\" as a host, we\ndo not want to be making a connection to a bogus host that is not\nwhat the end user requested, i.e. git.kernel.org.\n\nOtherwise we cannot change the policy to allow it in the future if\nit turns out to be necessary.\n"},{"id":"285247","messageId":"20160503050338.GD32259@sigill.intra.peff.net","threadId":"42188","inReplyTo":"e1056272-8c0e-0a0b-2295-4653a47cf86f@web.de","subject":"Re: [PATCH 1/6] connect: remove get_port()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-05-03T05:03:38Z","receivedAt":"2016-05-03T05:03:38Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, May 01, 2016 at 12:10:09PM +0200, Torsten Bögershausen wrote:\n\n> On 2016-05-01 08.02, Mike Hommey wrote:\n> > get_port() is only used as a fallback when get_host_and_port() does not\n> > return a port. But get_port() does the same search as\n> > get_host_and_port(), except get_host_and_port() starts from the end of\n> > the host, respecting square brackets for ipv6 addresses, and get_port(),\n> > operating after get_host_and_port(), works on a modified host string\n> > that has square brackes removed if there were any.\n> typo: brackets.\n> > \n> > I cannot think of any legal host:port string that would not have a port\n> > returned by get_host_and_port() *and* have one returned by get_port().\n> > So just remove get_port().\n> > \n> > Signed-off-by: Mike Hommey \n> Does this pass the test-suite ?\n> It doesn't pass here, t5601:\n\nHmm. I do notice that Mike's patch only touches the ssh code path. The\nnormal TCP code path _just_ uses get_host_and_port. Does that mean the\nTCP side is missing some cases, or is the extra parsing just for\nhandling bracketed \"[host:port]:path\" cases for ssh, like:\n\n> not ok 39 - bracketed hostnames are still ssh\n> #\n> # git clone \"[myhost:123]:src\" ssh-bracket-clone &&\n> # expect_ssh \"-p 123\" myhost src\n> #\n\n?\n\n-Peff\n"},{"id":"285248","messageId":"20160503051108.GA5696@glandium.org","threadId":"42188","inReplyTo":"20160503050338.GD32259@sigill.intra.peff.net","subject":"Re: [PATCH 1/6] connect: remove get_port()","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2016-05-03T05:11:08Z","receivedAt":"2016-05-03T05:11:08Z","isPatch":true,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"On Tue, May 03, 2016 at 01:03:38AM -0400, Jeff King wrote:\n> On Sun, May 01, 2016 at 12:10:09PM +0200, Torsten Bögershausen wrote:\n> \n> > On 2016-05-01 08.02, Mike Hommey wrote:\n> > > get_port() is only used as a fallback when get_host_and_port() does not\n> > > return a port. But get_port() does the same search as\n> > > get_host_and_port(), except get_host_and_port() starts from the end of\n> > > the host, respecting square brackets for ipv6 addresses, and get_port(),\n> > > operating after get_host_and_port(), works on a modified host string\n> > > that has square brackes removed if there were any.\n> > typo: brackets.\n> > > \n> > > I cannot think of any legal host:port string that would not have a port\n> > > returned by get_host_and_port() *and* have one returned by get_port().\n> > > So just remove get_port().\n> > > \n> > > Signed-off-by: Mike Hommey \n> > Does this pass the test-suite ?\n> > It doesn't pass here, t5601:\n> \n> Hmm. I do notice that Mike's patch only touches the ssh code path. The\n> normal TCP code path _just_ uses get_host_and_port. Does that mean the\n> TCP side is missing some cases, or is the extra parsing just for\n> handling bracketed \"[host:port]:path\" cases for ssh, like:\n> \n> > not ok 39 - bracketed hostnames are still ssh\n> > #\n> > # git clone \"[myhost:123]:src\" ssh-bracket-clone &&\n> > # expect_ssh \"-p 123\" myhost src\n> > #\n> \n> ?\n\nThe latter.\n\nMike\n"},{"id":"285263","messageId":"1462265452-32360-1-git-send-email-mh@glandium.org","threadId":"42188","inReplyTo":"1462082573-17992-1-git-send-email-mh@glandium.org","subject":"[PATCH v4 00/11] connect: various cleanups","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2016-05-03T08:50:41Z","receivedAt":"2016-05-03T08:50:41Z","isPatch":true,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"I went even further this time around. I'm not totally satistifed with\nthe resulting parse_connect_url function, but at least it feels to me\nthis series puts us in a better place to actually improve it further.\n\nMike Hommey (11):\n add fetch-pack --diag-url tests for some corner cases\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: use \"-l user\" instead of \"user@\" on ssh command line\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 | 223 ++++++++++++++++++++++++++++----------------------\n t/t5500-fetch-pack.sh | 46 ++++++++---\n t/t5601-clone.sh | 52 ++++++++++--\n 3 files changed, 204 insertions(+), 117 deletions(-)\n\n-- \n2.8.1.16.gaa70619.dirty\n"},{"id":"285268","messageId":"1462265452-32360-2-git-send-email-mh@glandium.org","threadId":"42188","inReplyTo":"1462265452-32360-1-git-send-email-mh@glandium.org","subject":"[PATCH v4 01/11] add fetch-pack --diag-url tests for some corner cases","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2016-05-03T08:50:42Z","receivedAt":"2016-05-03T08:50:42Z","isPatch":true,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"t5603-clone-dirname uses url patterns that are not tested with\nfetch-pack --diag-url, and it would be useful if they were.\n\nInterestingly, some of those tests, involving both a port and a\nuser:password pair, don't currently pass. Note that even if a\nuser:password pair is actually not supported by git, the values used\ncould be valid user names (user names can actually contain colons\nand at signs), and are still worth testing the url parser for.\n\nSigned-off-by: Mike Hommey \n---\n t/t5500-fetch-pack.sh | 38 ++++++++++++++++++++++++++++++++++----\n 1 file changed, 34 insertions(+), 4 deletions(-)\n\nNote I'm not /entirely/ sure about colons in user names, but ssh happily\nsends requests to authenticate with logins containing colons. I however\n*do* know it works with at signs (hg.mozilla.org ssh accounts are email\naddresses).\n\ndiff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh\nindex e5f83bf..1f0133f 100755\n--- a/t/t5500-fetch-pack.sh\n+++ b/t/t5500-fetch-pack.sh\n@@ -569,12 +569,27 @@ check_prot_host_port_path () {\n \ttest_cmp expected actual\n }\n \n-for r in repo re:po re/po\n+test_maybe_fail () {\n+\thost=$1; shift\n+\tcase $host in\n+\t\tgit=*)\n+\t\ttest_expect_success \"$@\"\n+\t\t;;\n+\t\t*:*@*)\n+\t\ttest_expect_failure \"$@\"\n+\t\t;;\n+\t\t*)\n+\t\ttest_expect_success \"$@\"\n+\t\t;;\n+\tesac\n+}\n+\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 \tdo\n-\t\tfor h in host user@host user@[::1] user@::1\n+\t\tfor h in host user@host user@[::1] user@::1 user:password@host user:passw@rd@host\n \t\tdo\n \t\t\tfor c in \"\" :\n \t\t\tdo\n@@ -587,9 +602,12 @@ do\n \t\t\t\t'\n \t\t\tdone\n \t\tdone\n-\t\tfor h in host User@host User@[::1]\n+\t\tfor h in host User@host User@[::1] User:password@host User:passw@rd@host\n \t\tdo\n-\t\t\ttest_expect_success \"fetch-pack --diag-url $p://$h:22/$r\" '\n+\t\t\ttest_maybe_fail \"$p=$h\" \"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_maybe_fail \"$p=$h\" \"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@@ -628,6 +646,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 user:password@host user:passw@rd@host\n+\tdo\n+\t\ttest_maybe_fail \"$h\" \"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_maybe_fail \"$h\" \"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.1.16.gaa70619.dirty\n"},{"id":"285258","messageId":"1462265452-32360-3-git-send-email-mh@glandium.org","threadId":"42188","inReplyTo":"1462265452-32360-1-git-send-email-mh@glandium.org","subject":"[PATCH v4 02/11] connect: call get_host_and_port() earlier","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2016-05-03T08:50:43Z","receivedAt":"2016-05-03T08:50:43Z","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 \"\" in a\ncase that never can happen, so that code path was removed.\n\nSigned-off-by: Mike Hommey \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 = \"\";\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.1.16.gaa70619.dirty\n"},{"id":"285262","messageId":"1462265452-32360-4-git-send-email-mh@glandium.org","threadId":"42188","inReplyTo":"1462265452-32360-1-git-send-email-mh@glandium.org","subject":"[PATCH v4 03/11] connect: only match the host with core.gitProxy","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2016-05-03T08:50:44Z","receivedAt":"2016-05-03T08:50:44Z","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 \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.1.16.gaa70619.dirty\n"},{"id":"285264","messageId":"1462265452-32360-5-git-send-email-mh@glandium.org","threadId":"42188","inReplyTo":"1462265452-32360-1-git-send-email-mh@glandium.org","subject":"[PATCH v4 04/11] 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-03T08:50:45Z","receivedAt":"2016-05-03T08:50:45Z","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 \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.1.16.gaa70619.dirty\n"},{"id":"285261","messageId":"1462265452-32360-6-git-send-email-mh@glandium.org","threadId":"42188","inReplyTo":"1462265452-32360-1-git-send-email-mh@glandium.org","subject":"[PATCH v4 05/11] connect: make parse_connect_url() return separated host and port","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2016-05-03T08:50:46Z","receivedAt":"2016-05-03T08:50:46Z","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. This also fixes the tests added 4 commits ago.\n\nSigned-off-by: Mike Hommey \n---\n connect.c | 30 ++++++++++++++++++------------\n t/t5500-fetch-pack.sh | 38 +++++++++-----------------------------\n 2 files changed, 27 insertions(+), 41 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 1f0133f..09d46c3 100755\n--- a/t/t5500-fetch-pack.sh\n+++ b/t/t5500-fetch-pack.sh\n@@ -537,7 +537,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@@ -546,22 +546,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@@ -569,21 +564,6 @@ check_prot_host_port_path () {\n \ttest_cmp expected actual\n }\n \n-test_maybe_fail () {\n-\thost=$1; shift\n-\tcase $host in\n-\t\tgit=*)\n-\t\ttest_expect_success \"$@\"\n-\t\t;;\n-\t\t*:*@*)\n-\t\ttest_expect_failure \"$@\"\n-\t\t;;\n-\t\t*)\n-\t\ttest_expect_success \"$@\"\n-\t\t;;\n-\tesac\n-}\n-\n for r in repo re:po re/po re@po\n do\n \t# git or ssh with scheme\n@@ -604,10 +584,10 @@ do\n \t\tdone\n \t\tfor h in host User@host User@[::1] User:password@host User:passw@rd@host\n \t\tdo\n-\t\t\ttest_maybe_fail \"$p=$h\" \"fetch-pack --diag-url $p://$h:22/$r\" '\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_maybe_fail \"$p=$h\" \"fetch-pack --diag-url $p://$h:22/$r\" '\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@@ -650,11 +630,11 @@ do\n \tp=ssh\n \tfor h in host user@host user:password@host user:passw@rd@host\n \tdo\n-\t\ttest_maybe_fail \"$h\" \"fetch-pack --diag-url [$h:22]:$r\" '\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_maybe_fail \"$h\" \"fetch-pack --diag-url [$h:22]:/~$r\" '\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-- \n2.8.1.16.gaa70619.dirty\n"},{"id":"285257","messageId":"1462265452-32360-7-git-send-email-mh@glandium.org","threadId":"42188","inReplyTo":"1462265452-32360-1-git-send-email-mh@glandium.org","subject":"[PATCH v4 06/11] connect: group CONNECT_DIAG_URL handling code","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2016-05-03T08:50:47Z","receivedAt":"2016-05-03T08:50:47Z","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 \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.1.16.gaa70619.dirty\n"},{"id":"285266","messageId":"1462265452-32360-8-git-send-email-mh@glandium.org","threadId":"42188","inReplyTo":"1462265452-32360-1-git-send-email-mh@glandium.org","subject":"[PATCH v4 07/11] 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-03T08:50:48Z","receivedAt":"2016-05-03T08:50:48Z","isPatch":true,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"Signed-off-by: Mike Hommey \n---\n connect.c | 37 +++++++++++++++++++++++++++++--------\n 1 file changed, 29 insertions(+), 8 deletions(-)\n\ndiff --git a/connect.c b/connect.c\nindex 8813f90..e95e385 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,15 @@ 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\tstruct strbuf userandhost = STRBUF_INIT;\n+\t\t\t\tstrbuf_addstr(&userandhost, user);\n+\t\t\t\tstrbuf_addch(&userandhost, '@');\n+\t\t\t\tstrbuf_addstr(&userandhost, host);\n+\t\t\t\targv_array_push(&conn->args, userandhost.buf);\n+\t\t\t\tstrbuf_release(&userandhost);\n+\t\t\t} else\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 +834,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.1.16.gaa70619.dirty\n"},{"id":"285267","messageId":"1462265452-32360-9-git-send-email-mh@glandium.org","threadId":"42188","inReplyTo":"1462265452-32360-1-git-send-email-mh@glandium.org","subject":"[PATCH v4 08/11] connect: change the --diag-url output to separate user and host","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2016-05-03T08:50:49Z","receivedAt":"2016-05-03T08:50:49Z","isPatch":true,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"Signed-off-by: Mike Hommey \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 e95e385..2c5b722 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 09d46c3..0c2f79f 100755\n--- a/t/t5500-fetch-pack.sh\n+++ b/t/t5500-fetch-pack.sh\n@@ -537,7 +537,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\" | grep -v user= | grep -v host= | grep -v port= >actual &&\n \ttest_cmp expected actual\n }\n \n@@ -552,10 +552,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.1.16.gaa70619.dirty\n"},{"id":"285260","messageId":"1462265452-32360-10-git-send-email-mh@glandium.org","threadId":"42188","inReplyTo":"1462265452-32360-1-git-send-email-mh@glandium.org","subject":"[PATCH v4 09/11] connect: use \"-l user\" instead of \"user@\" on ssh command line","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2016-05-03T08:50:50Z","receivedAt":"2016-05-03T08:50:50Z","isPatch":true,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"While it is not strictly necessary, it makes the connect code simpler\nwhen there is user.\n\nSigned-off-by: Mike Hommey \n---\n connect.c | 12 ++++--------\n t/t5601-clone.sh | 52 ++++++++++++++++++++++++++++++++++++++++++++--------\n 2 files changed, 48 insertions(+), 16 deletions(-)\n\nThis is entirely optional.\n\ndiff --git a/connect.c b/connect.c\nindex 2c5b722..0cec822 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -812,14 +812,10 @@ struct child_process *git_connect(int fd[2], const char *url,\n \t\t\t\targv_array_push(&conn->args, port);\n \t\t\t}\n \t\t\tif (user) {\n-\t\t\t\tstruct strbuf userandhost = STRBUF_INIT;\n-\t\t\t\tstrbuf_addstr(&userandhost, user);\n-\t\t\t\tstrbuf_addch(&userandhost, '@');\n-\t\t\t\tstrbuf_addstr(&userandhost, host);\n-\t\t\t\targv_array_push(&conn->args, userandhost.buf);\n-\t\t\t\tstrbuf_release(&userandhost);\n-\t\t\t} else\n-\t\t\t\targv_array_push(&conn->args, host);\n+\t\t\t\targv_array_push(&conn->args, \"-l\");\n+\t\t\t\targv_array_push(&conn->args, user);\n+\t\t\t}\n+\t\t\targv_array_push(&conn->args, host);\n \t\t} else {\n \t\t\ttransport_check_allowed(\"file\");\n \t\t}\ndiff --git a/t/t5601-clone.sh b/t/t5601-clone.sh\nindex c1efb8e..98fe861 100755\n--- a/t/t5601-clone.sh\n+++ b/t/t5601-clone.sh\n@@ -464,38 +464,74 @@ test_expect_success 'clone ssh://host.xz:22/~repo' '\n '\n \n #IPv6\n-for tuah in ::1 [::1] [::1]: user@::1 user@[::1] user@[::1]: [user@::1] [user@::1]:\n+for tah in ::1 [::1] [::1]:\n+do\n+\tehost=$(echo $tah | sed -e \"s/1]:/1]/ \"| tr -d \"[]\")\n+\ttest_expect_success \"clone ssh://$tah/home/user/repo\" \"\n+\t test_clone_url ssh://$tah/home/user/repo $ehost /home/user/repo\n+\t\"\n+done\n+\n+for tuah in user@::1 user@[::1] user@[::1]: [user@::1] [user@::1]:\n do\n \tehost=$(echo $tuah | sed -e \"s/1]:/1]/ \"| tr -d \"[]\")\n+\tehost=${ehost#user@}\n \ttest_expect_success \"clone ssh://$tuah/home/user/repo\" \"\n-\t test_clone_url ssh://$tuah/home/user/repo $ehost /home/user/repo\n+\t test_clone_url ssh://$tuah/home/user/repo '-l user $ehost' /home/user/repo\n \t\"\n done\n \n #IPv6 from home directory\n-for tuah in ::1 [::1] user@::1 user@[::1] [user@::1]\n+for tah in ::1 [::1]\n+do\n+\teah=$(echo $tah | tr -d \"[]\")\n+\ttest_expect_success \"clone ssh://$tah/~repo\" \"\n+\t test_clone_url ssh://$tah/~repo $eah '~repo'\n+\t\"\n+done\n+\n+for tuah in user@::1 user@[::1] [user@::1]\n do\n \teuah=$(echo $tuah | tr -d \"[]\")\n+\teah=${euah#user@}\n \ttest_expect_success \"clone ssh://$tuah/~repo\" \"\n-\t test_clone_url ssh://$tuah/~repo $euah '~repo'\n+\t test_clone_url ssh://$tuah/~repo '-l user' $eah '~repo'\n \t\"\n done\n \n #IPv6 with port number\n-for tuah in [::1] user@[::1] [user@::1]\n+for tah in [::1]\n+do\n+\teah=$(echo $tah | tr -d \"[]\")\n+\ttest_expect_success \"clone ssh://$tah:22/home/user/repo\" \"\n+\t test_clone_url ssh://$tah:22/home/user/repo '-p 22' $eah /home/user/repo\n+\t\"\n+done\n+\n+for tuah in user@[::1] [user@::1]\n do\n \teuah=$(echo $tuah | tr -d \"[]\")\n+\teah=${euah#user@}\n \ttest_expect_success \"clone ssh://$tuah:22/home/user/repo\" \"\n-\t test_clone_url ssh://$tuah:22/home/user/repo '-p 22' $euah /home/user/repo\n+\t test_clone_url ssh://$tuah:22/home/user/repo '-p 22 -l user' $eah /home/user/repo\n \t\"\n done\n \n #IPv6 from home directory with port number\n-for tuah in [::1] user@[::1] [user@::1]\n+for tah in [::1]\n+do\n+\teah=$(echo $tah | tr -d \"[]\")\n+\ttest_expect_success \"clone ssh://$tah:22/~repo\" \"\n+\t test_clone_url ssh://$tah:22/~repo '-p 22' $eah '~repo'\n+\t\"\n+done\n+\n+for tuah in user@[::1] [user@::1]\n do\n \teuah=$(echo $tuah | tr -d \"[]\")\n+\teah=${euah#user@}\n \ttest_expect_success \"clone ssh://$tuah:22/~repo\" \"\n-\t test_clone_url ssh://$tuah:22/~repo '-p 22' $euah '~repo'\n+\t test_clone_url ssh://$tuah:22/~repo '-p 22 -l user' $eah '~repo'\n \t\"\n done\n \n-- \n2.8.1.16.gaa70619.dirty\n"},{"id":"285265","messageId":"1462265452-32360-11-git-send-email-mh@glandium.org","threadId":"42188","inReplyTo":"1462265452-32360-1-git-send-email-mh@glandium.org","subject":"[PATCH v4 10/11] connect: actively reject git:// urls with a user part","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2016-05-03T08:50:51Z","receivedAt":"2016-05-03T08:50:51Z","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 \n---\n connect.c | 3 +++\n 1 file changed, 3 insertions(+)\n\ndiff --git a/connect.c b/connect.c\nindex 0cec822..215d6d9 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.1.16.gaa70619.dirty\n"},{"id":"285259","messageId":"1462265452-32360-12-git-send-email-mh@glandium.org","threadId":"42188","inReplyTo":"1462265452-32360-1-git-send-email-mh@glandium.org","subject":"[PATCH v4 11/11] connect: move ssh command line preparation to a separate function","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2016-05-03T08:50:52Z","receivedAt":"2016-05-03T08:50:52Z","isPatch":true,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"Signed-off-by: Mike Hommey \n---\n connect.c | 109 ++++++++++++++++++++++++++++++++++----------------------------\n 1 file changed, 59 insertions(+), 50 deletions(-)\n\ndiff --git a/connect.c b/connect.c\nindex 215d6d9..37b3140 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -673,6 +673,62 @@ 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_push(cmd, \"-l\");\n+\t\targv_array_push(cmd, user);\n+\t}\n+\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 +823,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_push(&conn->args, \"-l\");\n-\t\t\t\targv_array_push(&conn->args, user);\n-\t\t\t}\n-\t\t\targv_array_push(&conn->args, host);\n+\t\t\tconn->use_shell = prepare_ssh_command(\n+\t\t\t\t&conn->args, 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.1.16.gaa70619.dirty\n"},{"id":"285317","messageId":"1462278615-5925-1-git-send-email-mh@glandium.org","threadId":"42188","inReplyTo":"1462265452-32360-12-git-send-email-mh@glandium.org","subject":"[PATCH v4.1 11/11] connect: move ssh command line preparation to a separate function","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2016-05-03T12:30:15Z","receivedAt":"2016-05-03T12:30:15Z","isPatch":true,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"---\n connect.c | 109 ++++++++++++++++++++++++++++++++++----------------------------\n 1 file changed, 59 insertions(+), 50 deletions(-)\n\nErr, I had forgotten to commit --amend to add a missing argument.\n\ndiff --git a/connect.c b/connect.c\nindex 215d6d9..5f86983 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -673,6 +673,62 @@ 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_push(cmd, \"-l\");\n+\t\targv_array_push(cmd, user);\n+\t}\n+\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 +823,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_push(&conn->args, \"-l\");\n-\t\t\t\targv_array_push(&conn->args, user);\n-\t\t\t}\n-\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.1.18.gbe709d1.dirty\n"},{"id":"285329","messageId":"8cd2cf72-6e13-baa0-8bdf-d69293ba661f@web.de","threadId":"42188","inReplyTo":"1462265452-32360-2-git-send-email-mh@glandium.org","subject":"Re: [PATCH v4 01/11] add fetch-pack --diag-url tests for some corner cases","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2016-05-03T16:07:04Z","receivedAt":"2016-05-03T16:07:04Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On 2016-05-03 10.50, Mike Hommey wrote:\n> diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh\n> index e5f83bf..1f0133f 100755\n> --- a/t/t5500-fetch-pack.sh\n> +++ b/t/t5500-fetch-pack.sh\n> @@ -569,12 +569,27 @@ check_prot_host_port_path () {\n> \ttest_cmp expected actual\n> }\n> \n> -for r in repo re:po re/po\n> +test_maybe_fail () {\ntest_may_fail sounds non-deterministic or so ;-)\nhow about test_git_and_colon() or similar ?\n> +\thost=$1; shift\n> +\tcase $host in\n> +\t\tgit=*)\n> +\t\ttest_expect_success \"$@\"\n> +\t\t;;\n> +\t\t*:*@*)\n> +\t\ttest_expect_failure \"$@\"\n> +\t\t;;\n> +\t\t*)\n> +\t\ttest_expect_success \"$@\"\n> +\t\t;;\n> +\tesac\n> +}\n"},{"id":"285330","messageId":"xmqqzis7nn02.fsf@gitster.mtv.corp.google.com","threadId":"42188","inReplyTo":"1462265452-32360-2-git-send-email-mh@glandium.org","subject":"Re: [PATCH v4 01/11] add fetch-pack --diag-url tests for some corner cases","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-03T16:07:41Z","receivedAt":"2016-05-03T16:07:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Mike Hommey writes:\n\n> t5603-clone-dirname uses url patterns that are not tested with\n> fetch-pack --diag-url, and it would be useful if they were.\n>\n> Interestingly, some of those tests, involving both a port and a\n> user:password pair, don't currently pass. Note that even if a\n> user:password pair is actually not supported by git, the values used\n> could be valid user names (user names can actually contain colons\n> and at signs), and are still worth testing the url parser for.\n\nI am not sure about the last part of this (and the tests in the\npatch for them). When you are constrained by the Common Internet\nScheme Syntax, i.e.\n\n ://:@:/\n\nyou cannot have arbitrary characters in these parts; within the user\nand password field, any \":\", \"@\", or \"/\" must be encoded.\n\nWhich maens that for the purpose of the parser you are modifying,\nyou can rely on these three special characters to parse things out\n(decoding after the code determines which part is user and which\npart is password is a separate issue).\n\n> Signed-off-by: Mike Hommey \n> ---\n> t/t5500-fetch-pack.sh | 38 ++++++++++++++++++++++++++++++++++----\n> 1 file changed, 34 insertions(+), 4 deletions(-)\n>\n> Note I'm not /entirely/ sure about colons in user names, but ssh happily\n> sends requests to authenticate with logins containing colons. I however\n> *do* know it works with at signs (hg.mozilla.org ssh accounts are email\n> addresses).\n>\n> diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh\n> index e5f83bf..1f0133f 100755\n> --- a/t/t5500-fetch-pack.sh\n> +++ b/t/t5500-fetch-pack.sh\n> @@ -569,12 +569,27 @@ check_prot_host_port_path () {\n> \ttest_cmp expected actual\n> }\n> \n> -for r in repo re:po re/po\n> +test_maybe_fail () {\n\nThat is way too confusing a name when reading the caller of it by\nbeing too close to generic test helpers like test_might_fail,\ntest_must_fail, etc.\n\n> +\thost=$1; shift\n> +\tcase $host in\n> +\t\tgit=*)\n\nDedent this line by one level.\n\n> +\t\ttest_expect_success \"$@\"\n> +\t\t;;\n\nThese two lines are indented correctly.\n"},{"id":"285334","messageId":"c1cc0220-d239-1589-9957-1ef8a6245d54@web.de","threadId":"42188","inReplyTo":"1462265452-32360-9-git-send-email-mh@glandium.org","subject":"Re: [PATCH v4 08/11] connect: change the --diag-url output to separate user and host","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2016-05-03T16:20:01Z","receivedAt":"2016-05-03T16:20:01Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On 2016-05-03 10.50, Mike Hommey wrote:\n> Signed-off-by: Mike Hommey \n> ---\n> connect.c | 6 ++----\n> t/t5500-fetch-pack.sh | 14 ++++++++++++--\n> 2 files changed, 14 insertions(+), 6 deletions(-)\n> \n> diff --git a/connect.c b/connect.c\n> index e95e385..2c5b722 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;\n> diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh\n> index 09d46c3..0c2f79f 100755\n> --- a/t/t5500-fetch-pack.sh\n> +++ b/t/t5500-fetch-pack.sh\n> @@ -537,7 +537,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\" | grep -v user= | grep -v host= | grep -v port= >actual &&\nRunning grep a couple of times is probably not optimal in terms of spawning a\nprocess....\nDoes\n\ngit fetch-pack --diag-url \"$1\" | egrep -v \"user=|host=|port=\" >actual &&\nwork ?\nor the version like this:\ngit fetch-pack --diag-url \"$1\" | egrep -v \"(user|host|port)=\" >actual &&\n\n\n(And before I forget it: The whole series makes sense, thanks for that,\nit may be good if I review the final result as well as all the small changes)\n"},{"id":"285336","messageId":"8f470378-07a9-525b-ff8b-f0de011019cb@web.de","threadId":"42188","inReplyTo":"1462265452-32360-10-git-send-email-mh@glandium.org","subject":"Re: [PATCH v4 09/11] connect: use \"-l user\" instead of \"user@\" on ssh command line","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2016-05-03T16:25:05Z","receivedAt":"2016-05-03T16:25:05Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On 2016-05-03 10.50, Mike Hommey wrote:\n> While it is not strictly necessary, it makes the connect code simpler\n> when there is user.\n> \n\nThat commit message does't tell too much, I think.\n\nBesides that, I'm sure it will break (at least) my ssh wrapper scripts,\nwhich rely on user@host to be passed into the script.\n\nThe thing is that some hosts don't have a DNS entry, but can be reached via\nhost.local, if avahi is running.\nAnd my wrapper script parses the url, looks up the host via netlookup,\nor avahi using host.local, and feeds the result into /usr/bin/ssh.\n"},{"id":"285337","messageId":"f9da0d57-2d85-6a1c-5a28-13956102f37b@web.de","threadId":"42188","inReplyTo":"xmqqzis7nn02.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v4 01/11] add fetch-pack --diag-url tests for some corner cases","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2016-05-03T16:30:26Z","receivedAt":"2016-05-03T16:30:26Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On 2016-05-03 18.07, Junio C Hamano wrote:\n> Mike Hommey writes:\n> \n>> t5603-clone-dirname uses url patterns that are not tested with\n>> fetch-pack --diag-url, and it would be useful if they were.\n>>\n>> Interestingly, some of those tests, involving both a port and a\n>> user:password pair, don't currently pass. Note that even if a\n>> user:password pair is actually not supported by git, the values used\n>> could be valid user names (user names can actually contain colons\n>> and at signs), and are still worth testing the url parser for.\n> \n> I am not sure about the last part of this (and the tests in the\n> patch for them). When you are constrained by the Common Internet\n> Scheme Syntax, i.e.\n> \n> ://:@:/\n> \n> you cannot have arbitrary characters in these parts; within the user\n> and password field, any \":\", \"@\", or \"/\" must be encoded.\n> \nI thinnk we have an old bug here:\n\tif (is_url(url_orig))\n\t\turl = url_decode(url_orig);\n\telse\n\t\turl = xstrdup(url_orig);\n\nThe the url should be separated into the components first,\nand afther that url-path should got into url_decode,\nand may be password, username....\n(That's out of my head)\n"},{"id":"285342","messageId":"CAPig+cTYjY7=8YqGy_4dNKK7JWinShjTR-nEMY2V6gudzS2Bgw@mail.gmail.com","threadId":"42188","inReplyTo":"c1cc0220-d239-1589-9957-1ef8a6245d54@web.de","subject":"Re: [PATCH v4 08/11] connect: change the --diag-url output to separate user and host","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-05-03T17:23:37Z","receivedAt":"2016-05-03T17:23:37Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, May 3, 2016 at 12:20 PM, Torsten Bögershausen wrote:\n> On 2016-05-03 10.50, Mike Hommey wrote:\n>> - git fetch-pack --diag-url \"$1\" | grep -v host= | grep -v port= >actual &&\n>> + git fetch-pack --diag-url \"$1\" | grep -v user= | grep -v host= | grep -v port= >actual &&\n> Running grep a couple of times is probably not optimal in terms of spawning a\n> process....\n> Does\n>\n> git fetch-pack --diag-url \"$1\" | egrep -v \"user=|host=|port=\" >actual &&\n> work ?\n> or the version like this:\n> git fetch-pack --diag-url \"$1\" | egrep -v \"(user|host|port)=\" >actual &&\n\nI always worry about portability problems with these \"advanced\"\nexpressions in grep and sed, however, both of these work fine under\nMac OS X and FreeBSD (which is where problems often manifest).\n\nAn alterante expression which shouldn't raise any portability alarms would be:\n\n sed -e /user=/d -e /host=/d -e /port=/d\n"},{"id":"285343","messageId":"CAPig+cTTeJf+ACfJ8yDj3tbiwUEhcjgn=MiDkzgDk7sYu4FA+w@mail.gmail.com","threadId":"42188","inReplyTo":"1462265452-32360-10-git-send-email-mh@glandium.org","subject":"Re: [PATCH v4 09/11] connect: use \"-l user\" instead of \"user@\" on ssh command line","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-05-03T17:33:24Z","receivedAt":"2016-05-03T17:33:24Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, May 3, 2016 at 4:50 AM, Mike Hommey wrote:\n> While it is not strictly necessary, it makes the connect code simpler\n> when there is user.\n>\n> Signed-off-by: Mike Hommey \n> ---\n> diff --git a/connect.c b/connect.c\n> @@ -812,14 +812,10 @@ struct child_process *git_connect(int fd[2], const char *url,\n> if (user) {\n> - struct strbuf userandhost = STRBUF_INIT;\n> - strbuf_addstr(&userandhost, user);\n> - strbuf_addch(&userandhost, '@');\n> - strbuf_addstr(&userandhost, host);\n> - argv_array_push(&conn->args, userandhost.buf);\n> - strbuf_release(&userandhost);\n> - } else\n> - argv_array_push(&conn->args, host);\n> + argv_array_push(&conn->args, \"-l\");\n> + argv_array_push(&conn->args, user);\n> + }\n> + argv_array_push(&conn->args, host);\n\nEven simpler would be a one-liner for the user case:\n\n if (user)\n argv_array_pushf(&conn->args, \"%s@%s\"\", user, host);\n else\n argv_array_push(&conn->args, host);\n\n> } else {\n> transport_check_allowed(\"file\");\n> }\n"},{"id":"285344","messageId":"xmqqk2jbni85.fsf@gitster.mtv.corp.google.com","threadId":"42188","inReplyTo":"8f470378-07a9-525b-ff8b-f0de011019cb@web.de","subject":"Re: [PATCH v4 09/11] connect: use \"-l user\" instead of \"user@\" on ssh command line","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-03T17:50:50Z","receivedAt":"2016-05-03T17:50:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Torsten Bögershausen writes:\n\n> On 2016-05-03 10.50, Mike Hommey wrote:\n>> While it is not strictly necessary, it makes the connect code simpler\n>> when there is user.\n>> \n>\n> That commit message does't tell too much, I think.\n\n\"Doesn't tell too much\" is not necessarily bad, but \"tells too\nlittle\" is, and I think this tells me enough to say it is not a good\nchange ;-)\n\n> Besides that, I'm sure it will break (at least) my ssh wrapper scripts,\n> which rely on user@host to be passed into the script.\n\nThanks for bringing it up. \"By reducing the language we accept it\nmakes my coding simpler\" is not a good excuse to break existing\nusers, and \"While it is not strictly necessary, \" is a good hint\nthat the author _knows_ that the change can either (1) be done\nwithout, or (2) be done in a way that does not break existing users\nand yet make the end result easier to read.\n"},{"id":"285394","messageId":"20160503224830.GA21973@glandium.org","threadId":"42188","inReplyTo":"xmqqzis7nn02.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v4 01/11] add fetch-pack --diag-url tests for some corner cases","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2016-05-03T22:48:30Z","receivedAt":"2016-05-03T22:48:30Z","isPatch":true,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"On Tue, May 03, 2016 at 09:07:41AM -0700, Junio C Hamano wrote:\n> Mike Hommey writes:\n> \n> > t5603-clone-dirname uses url patterns that are not tested with\n> > fetch-pack --diag-url, and it would be useful if they were.\n> >\n> > Interestingly, some of those tests, involving both a port and a\n> > user:password pair, don't currently pass. Note that even if a\n> > user:password pair is actually not supported by git, the values used\n> > could be valid user names (user names can actually contain colons\n> > and at signs), and are still worth testing the url parser for.\n> \n> I am not sure about the last part of this (and the tests in the\n> patch for them). When you are constrained by the Common Internet\n> Scheme Syntax, i.e.\n> \n> ://:@:/\n> \n> you cannot have arbitrary characters in these parts; within the user\n> and password field, any \":\", \"@\", or \"/\" must be encoded.\n> \n> Which maens that for the purpose of the parser you are modifying,\n> you can rely on these three special characters to parse things out\n> (decoding after the code determines which part is user and which\n> part is password is a separate issue).\n\nt5603-clone-dirname contains a test for e.g. ssh://user:passw@rd@host:1234/\nThat's the basis for these additions. Whether that should work or not is\nbesides what I was interested in, which was to have a single test file to\nrun to test my changes, instead of several.\n\nStrictly speaking, this patch is not necessary, because it only covers\nthings that I found while breaking other tests.\n\nSo, there are multiple possible ways forward here:\n- Completely remove this patch for v5 of the series.\n- Remove the user:passw@rd cases because of the @.\n- Remove the user:password cases because we do nothing with the password\n anyways.\n- A combination of both of the above.\n\nI don't really care which is picked, at this point I just want to get\nover with this series ;)\n\nMike\n"},{"id":"285396","messageId":"20160503225051.GB21973@glandium.org","threadId":"42188","inReplyTo":"CAPig+cTYjY7=8YqGy_4dNKK7JWinShjTR-nEMY2V6gudzS2Bgw@mail.gmail.com","subject":"Re: [PATCH v4 08/11] connect: change the --diag-url output to separate user and host","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2016-05-03T22:50:51Z","receivedAt":"2016-05-03T22:50:51Z","isPatch":true,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"On Tue, May 03, 2016 at 01:23:37PM -0400, Eric Sunshine wrote:\n> On Tue, May 3, 2016 at 12:20 PM, Torsten Bögershausen wrote:\n> > On 2016-05-03 10.50, Mike Hommey wrote:\n> >> - git fetch-pack --diag-url \"$1\" | grep -v host= | grep -v port= >actual &&\n> >> + git fetch-pack --diag-url \"$1\" | grep -v user= | grep -v host= | grep -v port= >actual &&\n> > Running grep a couple of times is probably not optimal in terms of spawning a\n> > process....\n> > Does\n> >\n> > git fetch-pack --diag-url \"$1\" | egrep -v \"user=|host=|port=\" >actual &&\n> > work ?\n> > or the version like this:\n> > git fetch-pack --diag-url \"$1\" | egrep -v \"(user|host|port)=\" >actual &&\n> \n> I always worry about portability problems with these \"advanced\"\n> expressions in grep and sed, however, both of these work fine under\n> Mac OS X and FreeBSD (which is where problems often manifest).\n\nThat was my concern. But it looks like we already rely on the\negrep \"(|)\" form working in some other test, so I guess it's fine to use\nthat.\n\nMike\n"},{"id":"285398","messageId":"20160503225248.GC21973@glandium.org","threadId":"42188","inReplyTo":"CAPig+cTTeJf+ACfJ8yDj3tbiwUEhcjgn=MiDkzgDk7sYu4FA+w@mail.gmail.com","subject":"Re: [PATCH v4 09/11] connect: use \"-l user\" instead of \"user@\" on ssh command line","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2016-05-03T22:52:48Z","receivedAt":"2016-05-03T22:52:48Z","isPatch":true,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"On Tue, May 03, 2016 at 01:33:24PM -0400, Eric Sunshine wrote:\n> On Tue, May 3, 2016 at 4:50 AM, Mike Hommey wrote:\n> > While it is not strictly necessary, it makes the connect code simpler\n> > when there is user.\n> >\n> > Signed-off-by: Mike Hommey \n> > ---\n> > diff --git a/connect.c b/connect.c\n> > @@ -812,14 +812,10 @@ struct child_process *git_connect(int fd[2], const char *url,\n> > if (user) {\n> > - struct strbuf userandhost = STRBUF_INIT;\n> > - strbuf_addstr(&userandhost, user);\n> > - strbuf_addch(&userandhost, '@');\n> > - strbuf_addstr(&userandhost, host);\n> > - argv_array_push(&conn->args, userandhost.buf);\n> > - strbuf_release(&userandhost);\n> > - } else\n> > - argv_array_push(&conn->args, host);\n> > + argv_array_push(&conn->args, \"-l\");\n> > + argv_array_push(&conn->args, user);\n> > + }\n> > + argv_array_push(&conn->args, host);\n> \n> Even simpler would be a one-liner for the user case:\n> \n> if (user)\n> argv_array_pushf(&conn->args, \"%s@%s\"\", user, host);\n\nOh, I should have read the argv-array.h header. Thanks.\n\nMike\n"},{"id":"285632","messageId":"20160505215229.GA31157@glandium.org","threadId":"42188","inReplyTo":"20160503224830.GA21973@glandium.org","subject":"Re: [PATCH v4 01/11] add fetch-pack --diag-url tests for some corner cases","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2016-05-05T21:52:29Z","receivedAt":"2016-05-05T21:52:29Z","isPatch":true,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"On Wed, May 04, 2016 at 07:48:30AM +0900, Mike Hommey wrote:\n> On Tue, May 03, 2016 at 09:07:41AM -0700, Junio C Hamano wrote:\n> > Mike Hommey writes:\n> > \n> > > t5603-clone-dirname uses url patterns that are not tested with\n> > > fetch-pack --diag-url, and it would be useful if they were.\n> > >\n> > > Interestingly, some of those tests, involving both a port and a\n> > > user:password pair, don't currently pass. Note that even if a\n> > > user:password pair is actually not supported by git, the values used\n> > > could be valid user names (user names can actually contain colons\n> > > and at signs), and are still worth testing the url parser for.\n> > \n> > I am not sure about the last part of this (and the tests in the\n> > patch for them). When you are constrained by the Common Internet\n> > Scheme Syntax, i.e.\n> > \n> > ://:@:/\n> > \n> > you cannot have arbitrary characters in these parts; within the user\n> > and password field, any \":\", \"@\", or \"/\" must be encoded.\n> > \n> > Which maens that for the purpose of the parser you are modifying,\n> > you can rely on these three special characters to parse things out\n> > (decoding after the code determines which part is user and which\n> > part is password is a separate issue).\n> \n> t5603-clone-dirname contains a test for e.g. ssh://user:passw@rd@host:1234/\n> That's the basis for these additions. Whether that should work or not is\n> besides what I was interested in, which was to have a single test file to\n> run to test my changes, instead of several.\n> \n> Strictly speaking, this patch is not necessary, because it only covers\n> things that I found while breaking other tests.\n> \n> So, there are multiple possible ways forward here:\n> - Completely remove this patch for v5 of the series.\n> - Remove the user:passw@rd cases because of the @.\n> - Remove the user:password cases because we do nothing with the password\n> anyways.\n> - A combination of both of the above.\n\nAny opinions on this?\n\nMike\n"},{"id":"285659","messageId":"4349487f-716e-dadf-795b-cf790b03e02d@web.de","threadId":"42188","inReplyTo":"20160505215229.GA31157@glandium.org","subject":"Re: [PATCH v4 01/11] add fetch-pack --diag-url tests for some corner cases","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2016-05-06T04:17:04Z","receivedAt":"2016-05-06T04:17:04Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On 05.05.16 23:52, Mike Hommey wrote:\n> On Wed, May 04, 2016 at 07:48:30AM +0900, Mike Hommey wrote:\n>> On Tue, May 03, 2016 at 09:07:41AM -0700, Junio C Hamano wrote:\n>>> Mike Hommey writes:\n>>>\n>>>> t5603-clone-dirname uses url patterns that are not tested with\n>>>> fetch-pack --diag-url, and it would be useful if they were.\n>>>>\n>>>> Interestingly, some of those tests, involving both a port and a\n>>>> user:password pair, don't currently pass. Note that even if a\n>>>> user:password pair is actually not supported by git, the values used\n>>>> could be valid user names (user names can actually contain colons\n>>>> and at signs), and are still worth testing the url parser for.\n>>>\n>>> I am not sure about the last part of this (and the tests in the\n>>> patch for them). When you are constrained by the Common Internet\n>>> Scheme Syntax, i.e.\n>>>\n>>> ://:@:/\n>>>\n>>> you cannot have arbitrary characters in these parts; within the user\n>>> and password field, any \":\", \"@\", or \"/\" must be encoded.\n>>>\n>>> Which maens that for the purpose of the parser you are modifying,\n>>> you can rely on these three special characters to parse things out\n>>> (decoding after the code determines which part is user and which\n>>> part is password is a separate issue).\n>>\n>> t5603-clone-dirname contains a test for e.g. ssh://user:passw@rd@host:1234/\n>> That's the basis for these additions. Whether that should work or not is\n>> besides what I was interested in, which was to have a single test file to\n>> run to test my changes, instead of several.\n>>\n>> Strictly speaking, this patch is not necessary, because it only covers\n>> things that I found while breaking other tests.\n>>\n>> So, there are multiple possible ways forward here:\n>> - Completely remove this patch for v5 of the series.\n>> - Remove the user:passw@rd cases because of the @.\n>> - Remove the user:password cases because we do nothing with the password\n>> anyways.\n>> - A combination of both of the above.\n> \n> Any opinions on this?\n\nssh itself does not use a password:\n\nSSH(1) BSD General Commands Manual SSH(1)\n\nNAME\n ssh -- OpenSSH SSH client (remote login program)\n\nSYNOPSIS\n ssh [-1246AaCfgKkMNnqsTtVvXxYy] [-b bind_address] [-c cipher_spec]\n [-D [bind_address:]port] [-e escape_char] [-F configfile] [-I pkcs11]\n [-i identity_file] [-L [bind_address:]port:host:hostport]\n [-l login_name] [-m mac_spec] [-O ctl_cmd] [-o option] [-p port]\n [-R [bind_address:]port:host:hostport] [-S ctl_path] [-W host:port]\n [-w local_tun[:remote_tun]] [user@]hostname [command]\n\n\nNeither does Git.\nA user name must not include a ':'\n\nThe user:password came in here:\nCommit 92722efec01f67a54b\nclone: do not use port number as dir name\n\nActually, looking back, it may have been better to say\ngit clone ssh://aaaa:bbbb@host:/path\nis illegal and simply die() out.\n\nBack to your question and looking at the offered alternatives. I would vote for\n\"Completely remove this patch for v5 of the series.\"\n"},{"id":"285714","messageId":"xmqqoa8j5glt.fsf@gitster.mtv.corp.google.com","threadId":"42188","inReplyTo":"4349487f-716e-dadf-795b-cf790b03e02d@web.de","subject":"Re: [PATCH v4 01/11] add fetch-pack --diag-url tests for some corner cases","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-06T15:52:14Z","receivedAt":"2016-05-06T15:52:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Torsten Bögershausen writes:\n\n> ssh itself does not use a password:\n> ...\n> Neither does Git.\n> ...\n> The user:password came in here:\n> Commit 92722efec01f67a54b\n> clone: do not use port number as dir name\n>\n> Actually, looking back, it may have been better to say\n> git clone ssh://aaaa:bbbb@host:/path\n> is illegal and simply die() out.\n\nRFC2396, which updated RFC1738, discourages the use of :password\nin \"3.2.2 Server-based Naming Authority\", for obvious reasons.\n\n Some URL schemes use the format \"user:password\" in the userinfo\n field. This practice is NOT RECOMMENDED ...\n\nand then this is marked as deprecated in RFC3986 \"3.2.1. User\nInformation\".\n\n Use of the format \"user:password\" in the userinfo field is\n deprecated. Applications should not render as clear text any\n data after the first colon (\":\") character found within a\n userinfo subcomponent unless the data after the colon is the\n empty string (indicating no password).\n\nHowever, at the parser level that _knows_ the syntax, you shouldn't\nbe unilaterally turning these \"not recommended\" and \"deprecated\" to\n\"forbidden\". It should be prepared to see ':' to its input, if only\nto correctly recognize that as an attempt to express :password, in\norder to be able to hide the data after the first colon when running\nin verbose mode for example.\n\nI'd recommend that the parser to allow :@, and\nat least notice ':' that appears before the first '@' as having a\ndepreated form of . After stripping :// from the\nfront, it is OK to assume that everything before the first '@' is\n (in RFC2396 lingo), and everything in that is\nbefore the first ':' is when doing so. \n\n>>> ... When you are constrained by the Common Internet\n>>> Scheme Syntax, i.e.\n>>>\n>>> ://:@:/\n>>>\n>>> you cannot have arbitrary characters in these parts; within the user\n>>> and password field, any \":\", \"@\", or \"/\" must be encoded.\n"}]}