{"thread":{"id":"47257","subject":"[PATCH v3 0/8] Coping with unrecognized ssh wrapper scripts in GIT_SSH","startedAt":"2017-11-20T21:21:43Z","lastAt":"2017-11-22T01:52:24Z","messageCount":27,"participants":["Jonathan Nieder","Brandon Williams","Junio C Hamano","Stefan Beller"],"isPatch":true,"patchVersion":3,"patchTotal":8},"messages":[{"id":"332948","messageId":"20171120212134.lh2l4drdzu6fh5g2@aiede.mtv.corp.google.com","threadId":"47257","inReplyTo":null,"subject":"[PATCH v3 0/8] Coping with unrecognized ssh wrapper scripts in GIT_SSH","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-11-20T21:21:34Z","receivedAt":"2017-11-20T21:21:43Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Previously: [1].\n\nThis version should be essentially identical to v2. Changes:\n- patch 1 is new and should fix the test failure on Windows\n- patch 2 is new, discussed at [2]\n- patch 5 split off from patch 6 as suggested at [3]\n- patch 6 commit message got two new notes to address the worries\n from [3]\n\nThanks for the helpful reviews, and sorry to take so long to get this\nout. Thoughts of all kinds welcome, as always.\n\nSincerely,\nJonathan Nieder (8):\n ssh test: make copy_ssh_wrapper_as clean up after itself\n connect: move no_fork fallback to git_tcp_connect\n connect: split git:// setup into a separate function\n connect: split ssh command line options into separate function\n connect: split ssh option computation to its own function\n ssh: 'auto' variant to select between 'ssh' and 'simple'\n ssh: 'simple' variant does not support -4/-6\n ssh: 'simple' variant does not support --port\n\n Documentation/config.txt | 24 ++--\n connect.c | 322 +++++++++++++++++++++++++++++------------------\n t/t5601-clone.sh | 69 ++++++----\n t/t5603-clone-dirname.sh | 2 +\n 4 files changed, 265 insertions(+), 152 deletions(-)\n\n[1] https://public-inbox.org/git/20171023231625.6mhcyqti7vdg6yot@aiede.mtv.corp.google.com/\n[2] https://public-inbox.org/git/20171115202516.hduhzsgeoff5a22b@aiede.mtv.corp.google.com/\n[3] https://public-inbox.org/git/xmqq60b59toe.fsf@gitster.mtv.corp.google.com/\n"},{"id":"332949","messageId":"20171120212223.wquyxbmz34foynrk@aiede.mtv.corp.google.com","threadId":"47257","inReplyTo":"20171120212134.lh2l4drdzu6fh5g2@aiede.mtv.corp.google.com","subject":"[PATCH 1/8] ssh test: make copy_ssh_wrapper_as clean up after itself","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-11-20T21:22:23Z","receivedAt":"2017-11-20T21:22:35Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Simplify by not allowing the copied ssh wrapper to persist between\ntests. This way, tests can be safely reordered, added, and removed\nwith less fear of hidden side effects.\n\nThis also avoids having to call setup_ssh_wrapper to restore the value\nof GIT_SSH after this battery of tests, since it means each test will\nrestore it individually.\n\nNoticed because on Windows, if `uplink.exe` exists, the MSYS2 Bash\nwill overwrite that when redirecting via `>uplink`. A proposed test\nwrote a script to 'uplink' after a previous test created uplink.exe\nusing copy_ssh_wrapper_as, so the script written with '>uplink' had\nthe wrong filename and failed.\n\nReported-by: Johannes Schindelin \nSigned-off-by: Jonathan Nieder \n---\nThanks to Dscho for tracking this subtle issue down.\n\n t/t5601-clone.sh | 28 ++++++++++++----------------\n 1 file changed, 12 insertions(+), 16 deletions(-)\n\ndiff --git a/t/t5601-clone.sh b/t/t5601-clone.sh\nindex 86811a0c35..9d007c0f8d 100755\n--- a/t/t5601-clone.sh\n+++ b/t/t5601-clone.sh\n@@ -306,21 +306,20 @@ test_expect_success 'clone checking out a tag' '\n \ttest_cmp fetch.expected fetch.actual\n '\n \n-setup_ssh_wrapper () {\n-\ttest_expect_success 'setup ssh wrapper' '\n-\t\tcp \"$GIT_BUILD_DIR/t/helper/test-fake-ssh$X\" \\\n-\t\t\t\"$TRASH_DIRECTORY/ssh$X\" &&\n-\t\tGIT_SSH=\"$TRASH_DIRECTORY/ssh$X\" &&\n-\t\texport GIT_SSH &&\n-\t\texport TRASH_DIRECTORY &&\n-\t\t>\"$TRASH_DIRECTORY\"/ssh-output\n-\t'\n-}\n+test_expect_success 'set up ssh wrapper' '\n+\tcp \"$GIT_BUILD_DIR/t/helper/test-fake-ssh$X\" \\\n+\t\t\"$TRASH_DIRECTORY/ssh$X\" &&\n+\tGIT_SSH=\"$TRASH_DIRECTORY/ssh$X\" &&\n+\texport GIT_SSH &&\n+\texport TRASH_DIRECTORY &&\n+\t>\"$TRASH_DIRECTORY\"/ssh-output\n+'\n \n copy_ssh_wrapper_as () {\n \tcp \"$TRASH_DIRECTORY/ssh$X\" \"${1%$X}$X\" &&\n+\ttest_when_finished \"rm -f ${1%$X}$X\" &&\n \tGIT_SSH=\"${1%$X}$X\" &&\n-\texport GIT_SSH\n+\ttest_when_finished \"GIT_SSH=\\\"\\$TRASH_DIRECTORY/ssh\\$X\\\"\"\n }\n \n expect_ssh () {\n@@ -344,8 +343,6 @@ expect_ssh () {\n \t(cd \"$TRASH_DIRECTORY\" && test_cmp ssh-expect ssh-output)\n }\n \n-setup_ssh_wrapper\n-\n test_expect_success 'clone myhost:src uses ssh' '\n \tgit clone myhost:src ssh-clone &&\n \texpect_ssh myhost src\n@@ -432,12 +429,14 @@ test_expect_success 'ssh.variant overrides plink detection' '\n '\n \n test_expect_success 'GIT_SSH_VARIANT overrides plink detection to plink' '\n+\tcopy_ssh_wrapper_as \"$TRASH_DIRECTORY/plink\" &&\n \tGIT_SSH_VARIANT=plink \\\n \tgit clone \"[myhost:123]:src\" ssh-bracket-clone-variant-3 &&\n \texpect_ssh \"-P 123\" myhost src\n '\n \n test_expect_success 'GIT_SSH_VARIANT overrides plink to tortoiseplink' '\n+\tcopy_ssh_wrapper_as \"$TRASH_DIRECTORY/plink\" &&\n \tGIT_SSH_VARIANT=tortoiseplink \\\n \tgit clone \"[myhost:123]:src\" ssh-bracket-clone-variant-4 &&\n \texpect_ssh \"-batch -P 123\" myhost src\n@@ -449,9 +448,6 @@ test_expect_success 'clean failure on broken quoting' '\n \t\tgit clone \"[myhost:123]:src\" sq-failure\n '\n \n-# Reset the GIT_SSH environment variable for clone tests.\n-setup_ssh_wrapper\n-\n counter=0\n # $1 url\n # $2 none|host\n-- \n2.15.0.448.gf294e3d99a\n\n"},{"id":"332950","messageId":"20171120212254.ab6ata2mwvdbbdmw@aiede.mtv.corp.google.com","threadId":"47257","inReplyTo":"20171120212134.lh2l4drdzu6fh5g2@aiede.mtv.corp.google.com","subject":"[PATCH 2/8] connect: move no_fork fallback to git_tcp_connect","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-11-20T21:22:54Z","receivedAt":"2017-11-20T21:23:08Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"git_connect has the structure\n\n\tstruct child_process *conn = &no_fork;\n\n\t...\n\tswitch (protocol) {\n\tcase PROTO_GIT:\n\t\tif (git_use_proxy(hostandport))\n\t\t\tconn = git_proxy_connect(fd, hostandport);\n\t\telse\n\t\t\tgit_tcp_connect(fd, hostandport, flags);\n\t\t...\n\t\tbreak;\n\tcase PROTO_SSH:\n\t\tconn = xmalloc(sizeof(*conn));\n\t\tchild_process_init(conn);\n\t\targv_array_push(&conn->args, ssh);\n\t\t...\n\t\tbreak;\n\t...\n\treturn conn;\n\nIn all cases except the git_tcp_connect case, conn is explicitly\nassigned a value. Make the code clearer by explicitly assigning\n'conn = &no_fork' in the tcp case and eliminating the default so the\ncompiler can ensure conn is always correctly assigned.\n\nNoticed-by: Junio C Hamano \nSigned-off-by: Jonathan Nieder \n---\n connect.c | 36 +++++++++++++++++++++---------------\n 1 file changed, 21 insertions(+), 15 deletions(-)\n\ndiff --git a/connect.c b/connect.c\nindex 7fbd396b35..aa994d1518 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -582,12 +582,25 @@ 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+/*\n+ * Dummy child_process returned by git_connect() if the transport protocol\n+ * does not need fork(2).\n+ */\n+static struct child_process no_fork = CHILD_PROCESS_INIT;\n+\n+int git_connection_is_socket(struct child_process *conn)\n+{\n+\treturn conn == &no_fork;\n+}\n+\n+static struct child_process *git_tcp_connect(int fd[2], char *host, int flags)\n {\n \tint sockfd = git_tcp_connect_sock(host, flags);\n \n \tfd[0] = sockfd;\n \tfd[1] = dup(sockfd);\n+\n+\treturn &no_fork;\n }\n \n \n@@ -761,8 +774,6 @@ static enum protocol parse_connect_url(const char *url_orig, char **ret_host,\n \treturn protocol;\n }\n \n-static struct child_process no_fork = CHILD_PROCESS_INIT;\n-\n static const char *get_ssh_command(void)\n {\n \tconst char *ssh;\n@@ -851,11 +862,11 @@ static enum ssh_variant determine_ssh_variant(const char *ssh_command,\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- * finish the connection with finish_connect() with the value returned from\n- * this function (it is safe to call finish_connect() with NULL to support\n- * the former case).\n+ * This returns the dummy child_process `no_fork` if the transport protocol\n+ * does not need fork(2), or a struct child_process object if it does. Once\n+ * done, finish the connection with finish_connect() with the value returned\n+ * from this function (it is safe to call finish_connect() with NULL to\n+ * support the former case).\n *\n * If it returns, the connect is successful; it just dies on errors (this\n * will hopefully be changed in a libification effort, to return NULL when\n@@ -865,7 +876,7 @@ 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-\tstruct child_process *conn = &no_fork;\n+\tstruct child_process *conn;\n \tenum protocol protocol;\n \n \t/* Without this we cannot rely on waitpid() to tell\n@@ -901,7 +912,7 @@ struct child_process *git_connect(int fd[2], const char *url,\n \t\tif (git_use_proxy(hostandport))\n \t\t\tconn = git_proxy_connect(fd, hostandport);\n \t\telse\n-\t\t\tgit_tcp_connect(fd, hostandport, flags);\n+\t\t\tconn = git_tcp_connect(fd, hostandport, 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@@ -1041,11 +1052,6 @@ struct child_process *git_connect(int fd[2], const char *url,\n \treturn conn;\n }\n \n-int git_connection_is_socket(struct child_process *conn)\n-{\n-\treturn conn == &no_fork;\n-}\n-\n int finish_connect(struct child_process *conn)\n {\n \tint code;\n-- \n2.15.0.448.gf294e3d99a\n\n"},{"id":"332951","messageId":"20171120212327.ssk6vmw2hd5jwbi5@aiede.mtv.corp.google.com","threadId":"47257","inReplyTo":"20171120212134.lh2l4drdzu6fh5g2@aiede.mtv.corp.google.com","subject":"[PATCH 3/8] connect: split git:// setup into a separate function","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-11-20T21:23:27Z","receivedAt":"2017-11-20T21:23:39Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"The git_connect function is growing long. Split the\nPROTO_GIT-specific portion to a separate function to make it easier to\nread.\n\nNo functional change intended.\n\nSigned-off-by: Jonathan Nieder \nReviewed-by: Stefan Beller \n---\nAs before.\n\n connect.c | 103 +++++++++++++++++++++++++++++++++++---------------------------\n 1 file changed, 59 insertions(+), 44 deletions(-)\n\ndiff --git a/connect.c b/connect.c\nindex aa994d1518..9425229206 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -861,6 +861,64 @@ static enum ssh_variant determine_ssh_variant(const char *ssh_command,\n \treturn ssh_variant;\n }\n \n+/*\n+ * Open a connection using Git's native protocol.\n+ *\n+ * The caller is responsible for freeing hostandport, but this function may\n+ * modify it (for example, to truncate it to remove the port part).\n+ */\n+static struct child_process *git_connect_git(int fd[2], char *hostandport,\n+\t\t\t\t\t const char *path, const char *prog,\n+\t\t\t\t\t int flags)\n+{\n+\tstruct child_process *conn;\n+\tstruct strbuf request = STRBUF_INIT;\n+\t/*\n+\t * Set up virtual host information based on where we will\n+\t * connect, unless the user has overridden us in\n+\t * the environment.\n+\t */\n+\tchar *target_host = getenv(\"GIT_OVERRIDE_VIRTUAL_HOST\");\n+\tif (target_host)\n+\t\ttarget_host = xstrdup(target_host);\n+\telse\n+\t\ttarget_host = xstrdup(hostandport);\n+\n+\ttransport_check_allowed(\"git\");\n+\n+\t/* These underlying connection commands die() if they\n+\t * cannot connect.\n+\t */\n+\tif (git_use_proxy(hostandport))\n+\t\tconn = git_proxy_connect(fd, hostandport);\n+\telse\n+\t\tconn = git_tcp_connect(fd, hostandport, flags);\n+\t/*\n+\t * Separate original protocol components prog and path\n+\t * from extended host header with a NUL byte.\n+\t *\n+\t * Note: Do not add any other headers here! Doing so\n+\t * will cause older git-daemon servers to crash.\n+\t */\n+\tstrbuf_addf(&request,\n+\t\t \"%s %s%chost=%s%c\",\n+\t\t prog, path, 0,\n+\t\t target_host, 0);\n+\n+\t/* If using a new version put that stuff here after a second null byte */\n+\tif (get_protocol_version_config() > 0) {\n+\t\tstrbuf_addch(&request, '\\0');\n+\t\tstrbuf_addf(&request, \"version=%d%c\",\n+\t\t\t get_protocol_version_config(), '\\0');\n+\t}\n+\n+\tpacket_write(fd[1], request.buf, request.len);\n+\n+\tfree(target_host);\n+\tstrbuf_release(&request);\n+\treturn conn;\n+}\n+\n /*\n * This returns the dummy child_process `no_fork` if the transport protocol\n * does not need fork(2), or a struct child_process object if it does. Once\n@@ -892,50 +950,7 @@ struct child_process *git_connect(int fd[2], const char *url,\n \t\tprintf(\"Diag: path=%s\\n\", path ? path : \"NULL\");\n \t\tconn = NULL;\n \t} else if (protocol == PROTO_GIT) {\n-\t\tstruct strbuf request = STRBUF_INIT;\n-\t\t/*\n-\t\t * Set up virtual host information based on where we will\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-\n-\t\ttransport_check_allowed(\"git\");\n-\n-\t\t/* These underlying connection commands die() if they\n-\t\t * cannot connect.\n-\t\t */\n-\t\tif (git_use_proxy(hostandport))\n-\t\t\tconn = git_proxy_connect(fd, hostandport);\n-\t\telse\n-\t\t\tconn = git_tcp_connect(fd, hostandport, 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-\t\t *\n-\t\t * Note: Do not add any other headers here! Doing so\n-\t\t * will cause older git-daemon servers to crash.\n-\t\t */\n-\t\tstrbuf_addf(&request,\n-\t\t\t \"%s %s%chost=%s%c\",\n-\t\t\t prog, path, 0,\n-\t\t\t target_host, 0);\n-\n-\t\t/* If using a new version put that stuff here after a second null byte */\n-\t\tif (get_protocol_version_config() > 0) {\n-\t\t\tstrbuf_addch(&request, '\\0');\n-\t\t\tstrbuf_addf(&request, \"version=%d%c\",\n-\t\t\t\t get_protocol_version_config(), '\\0');\n-\t\t}\n-\n-\t\tpacket_write(fd[1], request.buf, request.len);\n-\n-\t\tfree(target_host);\n-\t\tstrbuf_release(&request);\n+\t\tconn = git_connect_git(fd, hostandport, path, prog, flags);\n \t} else {\n \t\tstruct strbuf cmd = STRBUF_INIT;\n \t\tconst char *const *var;\n-- \n2.15.0.448.gf294e3d99a\n\n"},{"id":"332952","messageId":"20171120212505.5t6vzvkfvhmjqwsd@aiede.mtv.corp.google.com","threadId":"47257","inReplyTo":"20171120212134.lh2l4drdzu6fh5g2@aiede.mtv.corp.google.com","subject":"[PATCH 4/8] connect: split ssh command line options into separate function","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-11-20T21:25:05Z","receivedAt":"2017-11-20T21:25:14Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"The git_connect function is growing long. Split the portion that\ndiscovers an ssh command and options it accepts before the service\nname and path to a separate function to make it easier to read.\n\nNo functional change intended.\n\nSigned-off-by: Jonathan Nieder \nReviewed-by: Stefan Beller \n---\nAs before.\n\n connect.c | 116 +++++++++++++++++++++++++++++++++-----------------------------\n 1 file changed, 61 insertions(+), 55 deletions(-)\n\ndiff --git a/connect.c b/connect.c\nindex 9425229206..a9dc493db2 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -919,6 +919,65 @@ static struct child_process *git_connect_git(int fd[2], char *hostandport,\n \treturn conn;\n }\n \n+/* Prepare a child_process for use by Git's SSH-tunneled transport. */\n+static void fill_ssh_args(struct child_process *conn, const char *ssh_host,\n+\t\t\t const char *port, int flags)\n+{\n+\tconst char *ssh;\n+\tenum ssh_variant variant;\n+\n+\tif (looks_like_command_line_option(ssh_host))\n+\t\tdie(\"strange hostname '%s' blocked\", ssh_host);\n+\n+\tssh = get_ssh_command();\n+\tif (ssh) {\n+\t\tvariant = determine_ssh_variant(ssh, 1);\n+\t} else {\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\tconn->use_shell = 0;\n+\n+\t\tssh = getenv(\"GIT_SSH\");\n+\t\tif (!ssh)\n+\t\t\tssh = \"ssh\";\n+\t\tvariant = determine_ssh_variant(ssh, 0);\n+\t}\n+\n+\targv_array_push(&conn->args, ssh);\n+\n+\tif (variant == VARIANT_SSH &&\n+\t get_protocol_version_config() > 0) {\n+\t\targv_array_push(&conn->args, \"-o\");\n+\t\targv_array_push(&conn->args, \"SendEnv=\" GIT_PROTOCOL_ENVIRONMENT);\n+\t\targv_array_pushf(&conn->env_array, GIT_PROTOCOL_ENVIRONMENT \"=version=%d\",\n+\t\t\t\t get_protocol_version_config());\n+\t}\n+\n+\tif (variant != VARIANT_SIMPLE) {\n+\t\tif (flags & CONNECT_IPV4)\n+\t\t\targv_array_push(&conn->args, \"-4\");\n+\t\telse if (flags & CONNECT_IPV6)\n+\t\t\targv_array_push(&conn->args, \"-6\");\n+\t}\n+\n+\tif (variant == VARIANT_TORTOISEPLINK)\n+\t\targv_array_push(&conn->args, \"-batch\");\n+\n+\tif (port && variant != VARIANT_SIMPLE) {\n+\t\tif (variant == VARIANT_SSH)\n+\t\t\targv_array_push(&conn->args, \"-p\");\n+\t\telse\n+\t\t\targv_array_push(&conn->args, \"-P\");\n+\n+\t\targv_array_push(&conn->args, port);\n+\t}\n+\n+\targv_array_push(&conn->args, ssh_host);\n+}\n+\n /*\n * This returns the dummy child_process `no_fork` if the transport protocol\n * does not need fork(2), or a struct child_process object if it does. Once\n@@ -972,16 +1031,13 @@ struct child_process *git_connect(int fd[2], const char *url,\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\tenum ssh_variant variant;\n \t\t\tchar *ssh_host = hostandport;\n \t\t\tconst char *port = NULL;\n+\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@@ -995,57 +1051,7 @@ struct child_process *git_connect(int fd[2], const char *url,\n \t\t\t\tstrbuf_release(&cmd);\n \t\t\t\treturn NULL;\n \t\t\t}\n-\n-\t\t\tif (looks_like_command_line_option(ssh_host))\n-\t\t\t\tdie(\"strange hostname '%s' blocked\", ssh_host);\n-\n-\t\t\tssh = get_ssh_command();\n-\t\t\tif (ssh) {\n-\t\t\t\tvariant = determine_ssh_variant(ssh, 1);\n-\t\t\t} else {\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-\t\t\t\tvariant = determine_ssh_variant(ssh, 0);\n-\t\t\t}\n-\n-\t\t\targv_array_push(&conn->args, ssh);\n-\n-\t\t\tif (variant == VARIANT_SSH &&\n-\t\t\t get_protocol_version_config() > 0) {\n-\t\t\t\targv_array_push(&conn->args, \"-o\");\n-\t\t\t\targv_array_push(&conn->args, \"SendEnv=\" GIT_PROTOCOL_ENVIRONMENT);\n-\t\t\t\targv_array_pushf(&conn->env_array, GIT_PROTOCOL_ENVIRONMENT \"=version=%d\",\n-\t\t\t\t\t\t get_protocol_version_config());\n-\t\t\t}\n-\n-\t\t\tif (variant != VARIANT_SIMPLE) {\n-\t\t\t\tif (flags & CONNECT_IPV4)\n-\t\t\t\t\targv_array_push(&conn->args, \"-4\");\n-\t\t\t\telse if (flags & CONNECT_IPV6)\n-\t\t\t\t\targv_array_push(&conn->args, \"-6\");\n-\t\t\t}\n-\n-\t\t\tif (variant == VARIANT_TORTOISEPLINK)\n-\t\t\t\targv_array_push(&conn->args, \"-batch\");\n-\n-\t\t\tif (port && variant != VARIANT_SIMPLE) {\n-\t\t\t\tif (variant == VARIANT_SSH)\n-\t\t\t\t\targv_array_push(&conn->args, \"-p\");\n-\t\t\t\telse\n-\t\t\t\t\targv_array_push(&conn->args, \"-P\");\n-\n-\t\t\t\targv_array_push(&conn->args, port);\n-\t\t\t}\n-\n-\t\t\targv_array_push(&conn->args, ssh_host);\n+\t\t\tfill_ssh_args(conn, ssh_host, port, flags);\n \t\t} else {\n \t\t\ttransport_check_allowed(\"file\");\n \t\t\tif (get_protocol_version_config() > 0) {\n-- \n2.15.0.448.gf294e3d99a\n\n"},{"id":"332953","messageId":"20171120212619.nqtijn5ygypixmw6@aiede.mtv.corp.google.com","threadId":"47257","inReplyTo":"20171120212134.lh2l4drdzu6fh5g2@aiede.mtv.corp.google.com","subject":"[PATCH 5/8] connect: split ssh option computation to its own function","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-11-20T21:26:19Z","receivedAt":"2017-11-20T21:26:28Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"This puts the determination of options to pass to each ssh variant\n(see ssh.variant in git-config(1)) in one place.\n\nA follow-up patch will use this in an initial dry run to detect which\nvariant to use when the ssh command is ambiguous.\n\nNo functional change intended yet.\n\nSigned-off-by: Jonathan Nieder \n---\nSplit out to make patch 6 easier to read, as suggested at\nhttps://public-inbox.org/git/xmqq60b59toe.fsf@gitster.mtv.corp.google.com/.\n\nAdded a function comment to make the purpose and API of this internal\nhelper clearer.\n\n connect.c | 65 ++++++++++++++++++++++++++++++++++++---------------------------\n 1 file changed, 37 insertions(+), 28 deletions(-)\n\ndiff --git a/connect.c b/connect.c\nindex a9dc493db2..d2fbb15cc5 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -919,6 +919,42 @@ static struct child_process *git_connect_git(int fd[2], char *hostandport,\n \treturn conn;\n }\n \n+/*\n+ * Append the appropriate environment variables to `env` and options to\n+ * `args` for running ssh in Git's SSH-tunneled transport.\n+ */\n+static void push_ssh_options(struct argv_array *args, struct argv_array *env,\n+\t\t\t enum ssh_variant variant, const char *port,\n+\t\t\t int flags)\n+{\n+\tif (variant == VARIANT_SSH &&\n+\t get_protocol_version_config() > 0) {\n+\t\targv_array_push(args, \"-o\");\n+\t\targv_array_push(args, \"SendEnv=\" GIT_PROTOCOL_ENVIRONMENT);\n+\t\targv_array_pushf(env, GIT_PROTOCOL_ENVIRONMENT \"=version=%d\",\n+\t\t\t\t get_protocol_version_config());\n+\t}\n+\n+\tif (variant != VARIANT_SIMPLE) {\n+\t\tif (flags & CONNECT_IPV4)\n+\t\t\targv_array_push(args, \"-4\");\n+\t\telse if (flags & CONNECT_IPV6)\n+\t\t\targv_array_push(args, \"-6\");\n+\t}\n+\n+\tif (variant == VARIANT_TORTOISEPLINK)\n+\t\targv_array_push(args, \"-batch\");\n+\n+\tif (port && variant != VARIANT_SIMPLE) {\n+\t\tif (variant == VARIANT_SSH)\n+\t\t\targv_array_push(args, \"-p\");\n+\t\telse\n+\t\t\targv_array_push(args, \"-P\");\n+\n+\t\targv_array_push(args, port);\n+\t}\n+}\n+\n /* Prepare a child_process for use by Git's SSH-tunneled transport. */\n static void fill_ssh_args(struct child_process *conn, const char *ssh_host,\n \t\t\t const char *port, int flags)\n@@ -947,34 +983,7 @@ static void fill_ssh_args(struct child_process *conn, const char *ssh_host,\n \t}\n \n \targv_array_push(&conn->args, ssh);\n-\n-\tif (variant == VARIANT_SSH &&\n-\t get_protocol_version_config() > 0) {\n-\t\targv_array_push(&conn->args, \"-o\");\n-\t\targv_array_push(&conn->args, \"SendEnv=\" GIT_PROTOCOL_ENVIRONMENT);\n-\t\targv_array_pushf(&conn->env_array, GIT_PROTOCOL_ENVIRONMENT \"=version=%d\",\n-\t\t\t\t get_protocol_version_config());\n-\t}\n-\n-\tif (variant != VARIANT_SIMPLE) {\n-\t\tif (flags & CONNECT_IPV4)\n-\t\t\targv_array_push(&conn->args, \"-4\");\n-\t\telse if (flags & CONNECT_IPV6)\n-\t\t\targv_array_push(&conn->args, \"-6\");\n-\t}\n-\n-\tif (variant == VARIANT_TORTOISEPLINK)\n-\t\targv_array_push(&conn->args, \"-batch\");\n-\n-\tif (port && variant != VARIANT_SIMPLE) {\n-\t\tif (variant == VARIANT_SSH)\n-\t\t\targv_array_push(&conn->args, \"-p\");\n-\t\telse\n-\t\t\targv_array_push(&conn->args, \"-P\");\n-\n-\t\targv_array_push(&conn->args, port);\n-\t}\n-\n+\tpush_ssh_options(&conn->args, &conn->env_array, variant, port, flags);\n \targv_array_push(&conn->args, ssh_host);\n }\n \n-- \n2.15.0.448.gf294e3d99a\n\n"},{"id":"332954","messageId":"20171120213004.57552ja3nmxy6pmc@aiede.mtv.corp.google.com","threadId":"47257","inReplyTo":"20171120212134.lh2l4drdzu6fh5g2@aiede.mtv.corp.google.com","subject":"[PATCH 6/8] ssh: 'auto' variant to select between 'ssh' and 'simple'","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-11-20T21:30:04Z","receivedAt":"2017-11-20T21:30:13Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Android's \"repo\" tool is a tool for managing a large codebase\nconsisting of multiple smaller repositories, similar to Git's\nsubmodule feature. Starting with Git 94b8ae5a (ssh: introduce a\n'simple' ssh variant, 2017-10-16), users noticed that it stopped\nhandling the port in ssh:// URLs.\n\nThe cause: when it encounters ssh:// URLs, repo pre-connects to the\nserver and sets GIT_SSH to a helper \".repo/repo/git_ssh\" that reuses\nthat connection. Before 94b8ae5a, the helper was assumed to support\nOpenSSH options for lack of a better guess and got passed a -p option\nto set the port. After that patch, it uses the new default of a\nsimple helper that does not accept an option to set the port.\n\nThe next release of \"repo\" will set GIT_SSH_VARIANT to \"ssh\" to avoid\nthat. But users of old versions and of other similar GIT_SSH\nimplementations would not get the benefit of that fix.\n\nSo update the default to use OpenSSH options again, with a twist. As\nobserved in 94b8ae5a, we cannot assume that $GIT_SSH always handles\nOpenSSH options: common helpers such as travis-ci's dpl[*] are\nconfigured using GIT_SSH and do not accept OpenSSH options. So make\nthe default a new variant \"auto\", with the following behavior:\n\n 1. First, check for a recognized basename, like today.\n\n 2. If the basename is not recognized, check whether $GIT_SSH supports\n OpenSSH options by running\n\n\t$GIT_SSH -G \n\n This returns status 0 and prints configuration in OpenSSH if it\n recognizes all and returns status 255 if it encounters\n an unrecognized option. A wrapper script like\n\n\texec ssh -- \"$@\"\n\n would fail with\n\n\tssh: Could not resolve hostname -g: Name or service not known\n\n , correctly reflecting that it does not support OpenSSH options.\n The command is run with stdin, stdout, and stderr redirected to\n /dev/null so even a command that expects a terminal would exit\n immediately.\n\n 3. Based on the result from step (2), behave like \"ssh\" (if it\n succeeded) or \"simple\" (if it failed).\n\nThis way, the default ssh variant for unrecognized commands can handle\nboth the repo and dpl cases as intended.\n\nThis autodetection has been running on Google workstations since\n2017-10-23 with no reported negative effects.\n\n[*] https://github.com/travis-ci/dpl/blob/6c3fddfda1f2a85944c544446b068bac0a77c049/lib/dpl/provider.rb#L215\n\nReported-by: William Yan \nImproved-by: Jonathan Tan \nSigned-off-by: Jonathan Nieder \n---\nAdded two notes to the commit message:\n - describing the real-world testing this patch has undergone\n - stdin, stdout, and stderr go to /dev/null, preventing a\n hypothetical ssh variant that *ignores* -G from hanging waiting for\n input from the terminal.\n\nThis is to address the worries at\nhttps://public-inbox.org/git/xmqq60b59toe.fsf@gitster.mtv.corp.google.com/\nand https://public-inbox.org/git/CAGZ79kZTjUvcq_hKHCqTDoaBxt2x+9XcqYc6ao1bhcET2SM-PQ@mail.gmail.com/\nabout hanging.\n\nNo change to the code from last time.\n\n Documentation/config.txt | 24 +++++++++++++++---------\n connect.c | 32 +++++++++++++++++++++++++-------\n t/t5601-clone.sh | 21 +++++++++++++++++++++\n 3 files changed, 61 insertions(+), 16 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 0460af37e2..0c371ad786 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -2081,16 +2081,22 @@ matched against are those given directly to Git commands. This means any URLs\n visited as a result of a redirection do not participate in matching.\n \n ssh.variant::\n-\tDepending on the value of the environment variables `GIT_SSH` or\n-\t`GIT_SSH_COMMAND`, or the config setting `core.sshCommand`, Git\n-\tauto-detects whether to adjust its command-line parameters for use\n-\twith ssh (OpenSSH), plink or tortoiseplink, as opposed to the default\n-\t(simple).\n+\tBy default, Git determines the command line arguments to use\n+\tbased on the basename of the configured SSH command (configured\n+\tusing the environment variable `GIT_SSH` or `GIT_SSH_COMMAND` or\n+\tthe config setting `core.sshCommand`). If the basename is\n+\tunrecognized, Git will attempt to detect support of OpenSSH\n+\toptions by first invoking the configured SSH command with the\n+\t`-G` (print configuration) option and will subsequently use\n+\tOpenSSH options (if that is successful) or no options besides\n+\tthe host and remote command (if it fails).\n +\n-The config variable `ssh.variant` can be set to override this auto-detection;\n-valid values are `ssh`, `simple`, `plink`, `putty` or `tortoiseplink`. Any\n-other value will be treated as normal ssh. This setting can be overridden via\n-the environment variable `GIT_SSH_VARIANT`.\n+The config variable `ssh.variant` can be set to override this detection.\n+Valid values are `ssh` (to use OpenSSH options), `plink`, `putty`,\n+`tortoiseplink`, `simple` (no options except the host and remote command).\n+The default auto-detection can be explicitly requested using the value\n+`auto`. Any other value is treated as `ssh`. This setting can also be\n+overridden via the environment variable `GIT_SSH_VARIANT`.\n +\n The current command-line parameters used for each variant are as\n follows:\ndiff --git a/connect.c b/connect.c\nindex d2fbb15cc5..455c54a2ec 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -788,6 +788,7 @@ static const char *get_ssh_command(void)\n }\n \n enum ssh_variant {\n+\tVARIANT_AUTO,\n \tVARIANT_SIMPLE,\n \tVARIANT_SSH,\n \tVARIANT_PLINK,\n@@ -795,14 +796,16 @@ enum ssh_variant {\n \tVARIANT_TORTOISEPLINK,\n };\n \n-static int override_ssh_variant(enum ssh_variant *ssh_variant)\n+static void override_ssh_variant(enum ssh_variant *ssh_variant)\n {\n \tconst char *variant = getenv(\"GIT_SSH_VARIANT\");\n \n \tif (!variant && git_config_get_string_const(\"ssh.variant\", &variant))\n-\t\treturn 0;\n+\t\treturn;\n \n-\tif (!strcmp(variant, \"plink\"))\n+\tif (!strcmp(variant, \"auto\"))\n+\t\t*ssh_variant = VARIANT_AUTO;\n+\telse if (!strcmp(variant, \"plink\"))\n \t\t*ssh_variant = VARIANT_PLINK;\n \telse if (!strcmp(variant, \"putty\"))\n \t\t*ssh_variant = VARIANT_PUTTY;\n@@ -812,18 +815,18 @@ static int override_ssh_variant(enum ssh_variant *ssh_variant)\n \t\t*ssh_variant = VARIANT_SIMPLE;\n \telse\n \t\t*ssh_variant = VARIANT_SSH;\n-\n-\treturn 1;\n }\n \n static enum ssh_variant determine_ssh_variant(const char *ssh_command,\n \t\t\t\t\t int is_cmdline)\n {\n-\tenum ssh_variant ssh_variant = VARIANT_SIMPLE;\n+\tenum ssh_variant ssh_variant = VARIANT_AUTO;\n \tconst char *variant;\n \tchar *p = NULL;\n \n-\tif (override_ssh_variant(&ssh_variant))\n+\toverride_ssh_variant(&ssh_variant);\n+\n+\tif (ssh_variant != VARIANT_AUTO)\n \t\treturn ssh_variant;\n \n \tif (!is_cmdline) {\n@@ -982,6 +985,21 @@ static void fill_ssh_args(struct child_process *conn, const char *ssh_host,\n \t\tvariant = determine_ssh_variant(ssh, 0);\n \t}\n \n+\tif (variant == VARIANT_AUTO) {\n+\t\tstruct child_process detect = CHILD_PROCESS_INIT;\n+\n+\t\tdetect.use_shell = conn->use_shell;\n+\t\tdetect.no_stdin = detect.no_stdout = detect.no_stderr = 1;\n+\n+\t\targv_array_push(&detect.args, ssh);\n+\t\targv_array_push(&detect.args, \"-G\");\n+\t\tpush_ssh_options(&detect.args, &detect.env_array,\n+\t\t\t\t VARIANT_SSH, port, flags);\n+\t\targv_array_push(&detect.args, ssh_host);\n+\n+\t\tvariant = run_command(&detect) ? VARIANT_SIMPLE : VARIANT_SSH;\n+\t}\n+\n \targv_array_push(&conn->args, ssh);\n \tpush_ssh_options(&conn->args, &conn->env_array, variant, port, flags);\n \targv_array_push(&conn->args, ssh_host);\ndiff --git a/t/t5601-clone.sh b/t/t5601-clone.sh\nindex 9d007c0f8d..209e2d5604 100755\n--- a/t/t5601-clone.sh\n+++ b/t/t5601-clone.sh\n@@ -369,6 +369,12 @@ test_expect_success 'variant can be overriden' '\n \texpect_ssh myhost src\n '\n \n+test_expect_success 'variant=auto picks based on basename' '\n+\tcopy_ssh_wrapper_as \"$TRASH_DIRECTORY/plink\" &&\n+\tgit -c ssh.variant=auto clone -4 \"[myhost:123]:src\" ssh-auto-clone &&\n+\texpect_ssh \"-4 -P 123\" myhost src\n+'\n+\n test_expect_success 'simple is treated as simple' '\n \tcopy_ssh_wrapper_as \"$TRASH_DIRECTORY/simple\" &&\n \tgit clone -4 \"[myhost:123]:src\" ssh-bracket-clone-simple &&\n@@ -381,6 +387,21 @@ test_expect_success 'uplink is treated as simple' '\n \texpect_ssh myhost src\n '\n \n+test_expect_success 'OpenSSH-like uplink is treated as ssh' '\n+\twrite_script \"$TRASH_DIRECTORY/uplink\" <<-EOF &&\n+\tif test \"\\$1\" = \"-G\"\n+\tthen\n+\t\texit 0\n+\tfi &&\n+\texec \"\\$TRASH_DIRECTORY/ssh$X\" \"\\$@\"\n+\tEOF\n+\ttest_when_finished \"rm -f \\\"\\$TRASH_DIRECTORY/uplink\\\"\" &&\n+\tGIT_SSH=\"$TRASH_DIRECTORY/uplink\" &&\n+\ttest_when_finished \"GIT_SSH=\\\"\\$TRASH_DIRECTORY/ssh\\$X\\\"\" &&\n+\tgit clone \"[myhost:123]:src\" ssh-bracket-clone-sshlike-uplink &&\n+\texpect_ssh \"-p 123\" myhost src\n+'\n+\n test_expect_success 'plink is treated specially (as putty)' '\n \tcopy_ssh_wrapper_as \"$TRASH_DIRECTORY/plink\" &&\n \tgit clone \"[myhost:123]:src\" ssh-bracket-clone-plink-0 &&\n-- \n2.15.0.448.gf294e3d99a\n\n"},{"id":"332955","messageId":"20171120213030.ya5kxw7vldgid34g@aiede.mtv.corp.google.com","threadId":"47257","inReplyTo":"20171120212134.lh2l4drdzu6fh5g2@aiede.mtv.corp.google.com","subject":"[PATCH 7/8] ssh: 'simple' variant does not support -4/-6","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-11-20T21:30:30Z","receivedAt":"2017-11-20T21:30:40Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"If the user passes -4/--ipv4 or -6/--ipv6 to \"git fetch\" or \"git push\"\nand the ssh command configured with GIT_SSH does not support such a\nsetting, error out instead of ignoring the option and continuing.\n\nSigned-off-by: Jonathan Nieder \nAcked-by: Stefan Beller \n---\nAs before.\n\n connect.c | 25 ++++++++++++++++++++++---\n t/t5601-clone.sh | 12 ++++++------\n 2 files changed, 28 insertions(+), 9 deletions(-)\n\ndiff --git a/connect.c b/connect.c\nindex 455c54a2ec..be106d1868 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -938,11 +938,30 @@ static void push_ssh_options(struct argv_array *args, struct argv_array *env,\n \t\t\t\t get_protocol_version_config());\n \t}\n \n-\tif (variant != VARIANT_SIMPLE) {\n-\t\tif (flags & CONNECT_IPV4)\n+\tif (flags & CONNECT_IPV4) {\n+\t\tswitch (variant) {\n+\t\tcase VARIANT_AUTO:\n+\t\t\tBUG(\"VARIANT_AUTO passed to push_ssh_options\");\n+\t\tcase VARIANT_SIMPLE:\n+\t\t\tdie(\"ssh variant 'simple' does not support -4\");\n+\t\tcase VARIANT_SSH:\n+\t\tcase VARIANT_PLINK:\n+\t\tcase VARIANT_PUTTY:\n+\t\tcase VARIANT_TORTOISEPLINK:\n \t\t\targv_array_push(args, \"-4\");\n-\t\telse if (flags & CONNECT_IPV6)\n+\t\t}\n+\t} else if (flags & CONNECT_IPV6) {\n+\t\tswitch (variant) {\n+\t\tcase VARIANT_AUTO:\n+\t\t\tBUG(\"VARIANT_AUTO passed to push_ssh_options\");\n+\t\tcase VARIANT_SIMPLE:\n+\t\t\tdie(\"ssh variant 'simple' does not support -6\");\n+\t\tcase VARIANT_SSH:\n+\t\tcase VARIANT_PLINK:\n+\t\tcase VARIANT_PUTTY:\n+\t\tcase VARIANT_TORTOISEPLINK:\n \t\t\targv_array_push(args, \"-6\");\n+\t\t}\n \t}\n \n \tif (variant == VARIANT_TORTOISEPLINK)\ndiff --git a/t/t5601-clone.sh b/t/t5601-clone.sh\nindex 209e2d5604..ad910ae9fa 100755\n--- a/t/t5601-clone.sh\n+++ b/t/t5601-clone.sh\n@@ -364,9 +364,10 @@ test_expect_success 'OpenSSH variant passes -4' '\n \texpect_ssh \"-4 -p 123\" myhost src\n '\n \n-test_expect_success 'variant can be overriden' '\n-\tgit -c ssh.variant=simple clone -4 \"[myhost:123]:src\" ssh-simple-clone &&\n-\texpect_ssh myhost src\n+test_expect_success 'variant can be overridden' '\n+\tcopy_ssh_wrapper_as \"$TRASH_DIRECTORY/putty\" &&\n+\tgit -c ssh.variant=putty clone -4 \"[myhost:123]:src\" ssh-putty-clone &&\n+\texpect_ssh \"-4 -P 123\" myhost src\n '\n \n test_expect_success 'variant=auto picks based on basename' '\n@@ -375,10 +376,9 @@ test_expect_success 'variant=auto picks based on basename' '\n \texpect_ssh \"-4 -P 123\" myhost src\n '\n \n-test_expect_success 'simple is treated as simple' '\n+test_expect_success 'simple does not support -4/-6' '\n \tcopy_ssh_wrapper_as \"$TRASH_DIRECTORY/simple\" &&\n-\tgit clone -4 \"[myhost:123]:src\" ssh-bracket-clone-simple &&\n-\texpect_ssh myhost src\n+\ttest_must_fail git clone -4 \"[myhost:123]:src\" ssh-bracket-clone-simple\n '\n \n test_expect_success 'uplink is treated as simple' '\n-- \n2.15.0.448.gf294e3d99a\n\n"},{"id":"332956","messageId":"20171120213101.yquv2ywyjajjtr5x@aiede.mtv.corp.google.com","threadId":"47257","inReplyTo":"20171120212134.lh2l4drdzu6fh5g2@aiede.mtv.corp.google.com","subject":"[PATCH 8/8] ssh: 'simple' variant does not support --port","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-11-20T21:31:01Z","receivedAt":"2017-11-20T21:31:12Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"When trying to connect to an ssh:// URL with port explicitly specified\nand the ssh command configured with GIT_SSH does not support such a\nsetting, it is less confusing to error out than to silently suppress\nthe port setting and continue.\n\nThis requires updating the GIT_SSH setting in t5603-clone-dirname.sh.\nThat test is about the directory name produced when cloning various\nURLs. It uses an ssh wrapper that ignores all its arguments but does\nnot declare that it supports a port argument; update it to set\nGIT_SSH_VARIANT=ssh to do so. (Real-life ssh wrappers that pass a\nport argument to OpenSSH would also support -G and would not require\nsuch an update.)\n\nReported-by: William Yan \nSigned-off-by: Jonathan Nieder \nAcked-by: Stefan Beller \n---\nAs before.\n\nThat's the end of the series. Thanks for reading.\n\n connect.c | 15 ++++++++++++---\n t/t5601-clone.sh | 10 ++++++++--\n t/t5603-clone-dirname.sh | 2 ++\n 3 files changed, 22 insertions(+), 5 deletions(-)\n\ndiff --git a/connect.c b/connect.c\nindex be106d1868..20ed1d9574 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -967,11 +967,20 @@ static void push_ssh_options(struct argv_array *args, struct argv_array *env,\n \tif (variant == VARIANT_TORTOISEPLINK)\n \t\targv_array_push(args, \"-batch\");\n \n-\tif (port && variant != VARIANT_SIMPLE) {\n-\t\tif (variant == VARIANT_SSH)\n+\tif (port) {\n+\t\tswitch (variant) {\n+\t\tcase VARIANT_AUTO:\n+\t\t\tBUG(\"VARIANT_AUTO passed to push_ssh_options\");\n+\t\tcase VARIANT_SIMPLE:\n+\t\t\tdie(\"ssh variant 'simple' does not support setting port\");\n+\t\tcase VARIANT_SSH:\n \t\t\targv_array_push(args, \"-p\");\n-\t\telse\n+\t\t\tbreak;\n+\t\tcase VARIANT_PLINK:\n+\t\tcase VARIANT_PUTTY:\n+\t\tcase VARIANT_TORTOISEPLINK:\n \t\t\targv_array_push(args, \"-P\");\n+\t\t}\n \n \t\targv_array_push(args, port);\n \t}\ndiff --git a/t/t5601-clone.sh b/t/t5601-clone.sh\nindex ad910ae9fa..78dab81d87 100755\n--- a/t/t5601-clone.sh\n+++ b/t/t5601-clone.sh\n@@ -378,12 +378,18 @@ test_expect_success 'variant=auto picks based on basename' '\n \n test_expect_success 'simple does not support -4/-6' '\n \tcopy_ssh_wrapper_as \"$TRASH_DIRECTORY/simple\" &&\n-\ttest_must_fail git clone -4 \"[myhost:123]:src\" ssh-bracket-clone-simple\n+\ttest_must_fail git clone -4 \"myhost:src\" ssh-4-clone-simple\n+'\n+\n+test_expect_success 'simple does not support port' '\n+\tcopy_ssh_wrapper_as \"$TRASH_DIRECTORY/simple\" &&\n+\ttest_must_fail git clone \"[myhost:123]:src\" ssh-bracket-clone-simple\n '\n \n test_expect_success 'uplink is treated as simple' '\n \tcopy_ssh_wrapper_as \"$TRASH_DIRECTORY/uplink\" &&\n-\tgit clone \"[myhost:123]:src\" ssh-bracket-clone-uplink &&\n+\ttest_must_fail git clone \"[myhost:123]:src\" ssh-bracket-clone-uplink &&\n+\tgit clone \"myhost:src\" ssh-clone-uplink &&\n \texpect_ssh myhost src\n '\n \ndiff --git a/t/t5603-clone-dirname.sh b/t/t5603-clone-dirname.sh\nindex d5af758129..13b5e5eb9b 100755\n--- a/t/t5603-clone-dirname.sh\n+++ b/t/t5603-clone-dirname.sh\n@@ -11,7 +11,9 @@ test_expect_success 'setup ssh wrapper' '\n \tgit upload-pack \"$TRASH_DIRECTORY\"\n \tEOF\n \tGIT_SSH=\"$TRASH_DIRECTORY/ssh-wrapper\" &&\n+\tGIT_SSH_VARIANT=ssh &&\n \texport GIT_SSH &&\n+\texport GIT_SSH_VARIANT &&\n \texport TRASH_DIRECTORY\n '\n \n-- \n2.15.0.448.gf294e3d99a\n\n"},{"id":"332957","messageId":"20171120214720.GA92506@google.com","threadId":"47257","inReplyTo":"20171120212223.wquyxbmz34foynrk@aiede.mtv.corp.google.com","subject":"Re: [PATCH 1/8] ssh test: make copy_ssh_wrapper_as clean up after itself","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-11-20T21:47:20Z","receivedAt":"2017-11-20T21:47:29Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 11/20, Jonathan Nieder wrote:\n> Simplify by not allowing the copied ssh wrapper to persist between\n> tests. This way, tests can be safely reordered, added, and removed\n> with less fear of hidden side effects.\n> \n> This also avoids having to call setup_ssh_wrapper to restore the value\n> of GIT_SSH after this battery of tests, since it means each test will\n> restore it individually.\n> \n> Noticed because on Windows, if `uplink.exe` exists, the MSYS2 Bash\n> will overwrite that when redirecting via `>uplink`. A proposed test\n> wrote a script to 'uplink' after a previous test created uplink.exe\n> using copy_ssh_wrapper_as, so the script written with '>uplink' had\n> the wrong filename and failed.\n> \n> Reported-by: Johannes Schindelin \n> Signed-off-by: Jonathan Nieder \n> ---\n> Thanks to Dscho for tracking this subtle issue down.\n> \n> t/t5601-clone.sh | 28 ++++++++++++----------------\n> 1 file changed, 12 insertions(+), 16 deletions(-)\n> \n> diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh\n> index 86811a0c35..9d007c0f8d 100755\n> --- a/t/t5601-clone.sh\n> +++ b/t/t5601-clone.sh\n> @@ -306,21 +306,20 @@ test_expect_success 'clone checking out a tag' '\n> \ttest_cmp fetch.expected fetch.actual\n> '\n> \n> -setup_ssh_wrapper () {\n> -\ttest_expect_success 'setup ssh wrapper' '\n> -\t\tcp \"$GIT_BUILD_DIR/t/helper/test-fake-ssh$X\" \\\n> -\t\t\t\"$TRASH_DIRECTORY/ssh$X\" &&\n> -\t\tGIT_SSH=\"$TRASH_DIRECTORY/ssh$X\" &&\n> -\t\texport GIT_SSH &&\n> -\t\texport TRASH_DIRECTORY &&\n> -\t\t>\"$TRASH_DIRECTORY\"/ssh-output\n> -\t'\n> -}\n> +test_expect_success 'set up ssh wrapper' '\n> +\tcp \"$GIT_BUILD_DIR/t/helper/test-fake-ssh$X\" \\\n> +\t\t\"$TRASH_DIRECTORY/ssh$X\" &&\n> +\tGIT_SSH=\"$TRASH_DIRECTORY/ssh$X\" &&\n> +\texport GIT_SSH &&\n> +\texport TRASH_DIRECTORY &&\n> +\t>\"$TRASH_DIRECTORY\"/ssh-output\n> +'\n> \n> copy_ssh_wrapper_as () {\n> \tcp \"$TRASH_DIRECTORY/ssh$X\" \"${1%$X}$X\" &&\n> +\ttest_when_finished \"rm -f ${1%$X}$X\" &&\n> \tGIT_SSH=\"${1%$X}$X\" &&\n> -\texport GIT_SSH\n> +\ttest_when_finished \"GIT_SSH=\\\"\\$TRASH_DIRECTORY/ssh\\$X\\\"\"\n\nAll the escaping!\n\nPatch looks good.\n\n> }\n> \n> expect_ssh () {\n> @@ -344,8 +343,6 @@ expect_ssh () {\n> \t(cd \"$TRASH_DIRECTORY\" && test_cmp ssh-expect ssh-output)\n> }\n> \n> -setup_ssh_wrapper\n> -\n> test_expect_success 'clone myhost:src uses ssh' '\n> \tgit clone myhost:src ssh-clone &&\n> \texpect_ssh myhost src\n> @@ -432,12 +429,14 @@ test_expect_success 'ssh.variant overrides plink detection' '\n> '\n> \n> test_expect_success 'GIT_SSH_VARIANT overrides plink detection to plink' '\n> +\tcopy_ssh_wrapper_as \"$TRASH_DIRECTORY/plink\" &&\n> \tGIT_SSH_VARIANT=plink \\\n> \tgit clone \"[myhost:123]:src\" ssh-bracket-clone-variant-3 &&\n> \texpect_ssh \"-P 123\" myhost src\n> '\n> \n> test_expect_success 'GIT_SSH_VARIANT overrides plink to tortoiseplink' '\n> +\tcopy_ssh_wrapper_as \"$TRASH_DIRECTORY/plink\" &&\n> \tGIT_SSH_VARIANT=tortoiseplink \\\n> \tgit clone \"[myhost:123]:src\" ssh-bracket-clone-variant-4 &&\n> \texpect_ssh \"-batch -P 123\" myhost src\n> @@ -449,9 +448,6 @@ test_expect_success 'clean failure on broken quoting' '\n> \t\tgit clone \"[myhost:123]:src\" sq-failure\n> '\n> \n> -# Reset the GIT_SSH environment variable for clone tests.\n> -setup_ssh_wrapper\n> -\n> counter=0\n> # $1 url\n> # $2 none|host\n> -- \n> 2.15.0.448.gf294e3d99a\n> \n\n-- \nBrandon Williams\n"},{"id":"332959","messageId":"20171120215218.GB92506@google.com","threadId":"47257","inReplyTo":"20171120212327.ssk6vmw2hd5jwbi5@aiede.mtv.corp.google.com","subject":"Re: [PATCH 3/8] connect: split git:// setup into a separate function","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-11-20T21:52:18Z","receivedAt":"2017-11-20T21:52:26Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 11/20, Jonathan Nieder wrote:\n> The git_connect function is growing long. Split the\n> PROTO_GIT-specific portion to a separate function to make it easier to\n> read.\n> \n> No functional change intended.\n> \n> Signed-off-by: Jonathan Nieder \n> Reviewed-by: Stefan Beller \n> ---\n> As before.\n> \n> connect.c | 103 +++++++++++++++++++++++++++++++++++---------------------------\n> 1 file changed, 59 insertions(+), 44 deletions(-)\n> \n> diff --git a/connect.c b/connect.c\n> index aa994d1518..9425229206 100644\n> --- a/connect.c\n> +++ b/connect.c\n> @@ -861,6 +861,64 @@ static enum ssh_variant determine_ssh_variant(const char *ssh_command,\n> \treturn ssh_variant;\n> }\n> \n> +/*\n> + * Open a connection using Git's native protocol.\n> + *\n> + * The caller is responsible for freeing hostandport, but this function may\n> + * modify it (for example, to truncate it to remove the port part).\n> + */\n> +static struct child_process *git_connect_git(int fd[2], char *hostandport,\n> +\t\t\t\t\t const char *path, const char *prog,\n> +\t\t\t\t\t int flags)\n> +{\n> +\tstruct child_process *conn;\n> +\tstruct strbuf request = STRBUF_INIT;\n> +\t/*\n> +\t * Set up virtual host information based on where we will\n> +\t * connect, unless the user has overridden us in\n> +\t * the environment.\n> +\t */\n> +\tchar *target_host = getenv(\"GIT_OVERRIDE_VIRTUAL_HOST\");\n> +\tif (target_host)\n> +\t\ttarget_host = xstrdup(target_host);\n> +\telse\n> +\t\ttarget_host = xstrdup(hostandport);\n> +\n> +\ttransport_check_allowed(\"git\");\n> +\n> +\t/* These underlying connection commands die() if they\n> +\t * cannot connect.\n> +\t */\n\nI know this is really just code motion but maybe we can fix the style of\nthe comment here?\n\n> +\tif (git_use_proxy(hostandport))\n> +\t\tconn = git_proxy_connect(fd, hostandport);\n> +\telse\n> +\t\tconn = git_tcp_connect(fd, hostandport, flags);\n> +\t/*\n> +\t * Separate original protocol components prog and path\n> +\t * from extended host header with a NUL byte.\n> +\t *\n> +\t * Note: Do not add any other headers here! Doing so\n> +\t * will cause older git-daemon servers to crash.\n> +\t */\n> +\tstrbuf_addf(&request,\n> +\t\t \"%s %s%chost=%s%c\",\n> +\t\t prog, path, 0,\n> +\t\t target_host, 0);\n> +\n> +\t/* If using a new version put that stuff here after a second null byte */\n> +\tif (get_protocol_version_config() > 0) {\n> +\t\tstrbuf_addch(&request, '\\0');\n> +\t\tstrbuf_addf(&request, \"version=%d%c\",\n> +\t\t\t get_protocol_version_config(), '\\0');\n> +\t}\n> +\n> +\tpacket_write(fd[1], request.buf, request.len);\n> +\n> +\tfree(target_host);\n> +\tstrbuf_release(&request);\n> +\treturn conn;\n> +}\n> +\n> /*\n> * This returns the dummy child_process `no_fork` if the transport protocol\n> * does not need fork(2), or a struct child_process object if it does. Once\n> @@ -892,50 +950,7 @@ struct child_process *git_connect(int fd[2], const char *url,\n> \t\tprintf(\"Diag: path=%s\\n\", path ? path : \"NULL\");\n> \t\tconn = NULL;\n> \t} else if (protocol == PROTO_GIT) {\n> -\t\tstruct strbuf request = STRBUF_INIT;\n> -\t\t/*\n> -\t\t * Set up virtual host information based on where we will\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> -\n> -\t\ttransport_check_allowed(\"git\");\n> -\n> -\t\t/* These underlying connection commands die() if they\n> -\t\t * cannot connect.\n> -\t\t */\n> -\t\tif (git_use_proxy(hostandport))\n> -\t\t\tconn = git_proxy_connect(fd, hostandport);\n> -\t\telse\n> -\t\t\tconn = git_tcp_connect(fd, hostandport, 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> -\t\t *\n> -\t\t * Note: Do not add any other headers here! Doing so\n> -\t\t * will cause older git-daemon servers to crash.\n> -\t\t */\n> -\t\tstrbuf_addf(&request,\n> -\t\t\t \"%s %s%chost=%s%c\",\n> -\t\t\t prog, path, 0,\n> -\t\t\t target_host, 0);\n> -\n> -\t\t/* If using a new version put that stuff here after a second null byte */\n> -\t\tif (get_protocol_version_config() > 0) {\n> -\t\t\tstrbuf_addch(&request, '\\0');\n> -\t\t\tstrbuf_addf(&request, \"version=%d%c\",\n> -\t\t\t\t get_protocol_version_config(), '\\0');\n> -\t\t}\n> -\n> -\t\tpacket_write(fd[1], request.buf, request.len);\n> -\n> -\t\tfree(target_host);\n> -\t\tstrbuf_release(&request);\n> +\t\tconn = git_connect_git(fd, hostandport, path, prog, flags);\n> \t} else {\n> \t\tstruct strbuf cmd = STRBUF_INIT;\n> \t\tconst char *const *var;\n> -- \n> 2.15.0.448.gf294e3d99a\n> \n\n-- \nBrandon Williams\n"},{"id":"332960","messageId":"20171120215404.GC92506@google.com","threadId":"47257","inReplyTo":"20171120212505.5t6vzvkfvhmjqwsd@aiede.mtv.corp.google.com","subject":"Re: [PATCH 4/8] connect: split ssh command line options into separate function","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-11-20T21:54:04Z","receivedAt":"2017-11-20T21:54:11Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 11/20, Jonathan Nieder wrote:\n> The git_connect function is growing long. Split the portion that\n> discovers an ssh command and options it accepts before the service\n> name and path to a separate function to make it easier to read.\n> \n> No functional change intended.\n> \n> Signed-off-by: Jonathan Nieder \n> Reviewed-by: Stefan Beller \n> ---\n> As before.\n> \n> connect.c | 116 +++++++++++++++++++++++++++++++++-----------------------------\n> 1 file changed, 61 insertions(+), 55 deletions(-)\n> \n> diff --git a/connect.c b/connect.c\n> index 9425229206..a9dc493db2 100644\n> --- a/connect.c\n> +++ b/connect.c\n> @@ -919,6 +919,65 @@ static struct child_process *git_connect_git(int fd[2], char *hostandport,\n> \treturn conn;\n> }\n> \n> +/* Prepare a child_process for use by Git's SSH-tunneled transport. */\n> +static void fill_ssh_args(struct child_process *conn, const char *ssh_host,\n> +\t\t\t const char *port, int flags)\n> +{\n> +\tconst char *ssh;\n> +\tenum ssh_variant variant;\n> +\n> +\tif (looks_like_command_line_option(ssh_host))\n> +\t\tdie(\"strange hostname '%s' blocked\", ssh_host);\n> +\n> +\tssh = get_ssh_command();\n> +\tif (ssh) {\n> +\t\tvariant = determine_ssh_variant(ssh, 1);\n> +\t} else {\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\tconn->use_shell = 0;\n> +\n> +\t\tssh = getenv(\"GIT_SSH\");\n> +\t\tif (!ssh)\n> +\t\t\tssh = \"ssh\";\n> +\t\tvariant = determine_ssh_variant(ssh, 0);\n> +\t}\n> +\n> +\targv_array_push(&conn->args, ssh);\n> +\n> +\tif (variant == VARIANT_SSH &&\n> +\t get_protocol_version_config() > 0) {\n> +\t\targv_array_push(&conn->args, \"-o\");\n> +\t\targv_array_push(&conn->args, \"SendEnv=\" GIT_PROTOCOL_ENVIRONMENT);\n> +\t\targv_array_pushf(&conn->env_array, GIT_PROTOCOL_ENVIRONMENT \"=version=%d\",\n> +\t\t\t\t get_protocol_version_config());\n> +\t}\n> +\n> +\tif (variant != VARIANT_SIMPLE) {\n> +\t\tif (flags & CONNECT_IPV4)\n> +\t\t\targv_array_push(&conn->args, \"-4\");\n> +\t\telse if (flags & CONNECT_IPV6)\n> +\t\t\targv_array_push(&conn->args, \"-6\");\n> +\t}\n> +\n> +\tif (variant == VARIANT_TORTOISEPLINK)\n> +\t\targv_array_push(&conn->args, \"-batch\");\n> +\n> +\tif (port && variant != VARIANT_SIMPLE) {\n> +\t\tif (variant == VARIANT_SSH)\n> +\t\t\targv_array_push(&conn->args, \"-p\");\n> +\t\telse\n> +\t\t\targv_array_push(&conn->args, \"-P\");\n> +\n> +\t\targv_array_push(&conn->args, port);\n> +\t}\n> +\n> +\targv_array_push(&conn->args, ssh_host);\n> +}\n> +\n> /*\n> * This returns the dummy child_process `no_fork` if the transport protocol\n> * does not need fork(2), or a struct child_process object if it does. Once\n> @@ -972,16 +1031,13 @@ struct child_process *git_connect(int fd[2], const char *url,\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\tenum ssh_variant variant;\n> \t\t\tchar *ssh_host = hostandport;\n> \t\t\tconst char *port = NULL;\n> +\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\nAre these random additions and deletions intentional?\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> @@ -995,57 +1051,7 @@ struct child_process *git_connect(int fd[2], const char *url,\n> \t\t\t\tstrbuf_release(&cmd);\n> \t\t\t\treturn NULL;\n> \t\t\t}\n> -\n> -\t\t\tif (looks_like_command_line_option(ssh_host))\n> -\t\t\t\tdie(\"strange hostname '%s' blocked\", ssh_host);\n> -\n> -\t\t\tssh = get_ssh_command();\n> -\t\t\tif (ssh) {\n> -\t\t\t\tvariant = determine_ssh_variant(ssh, 1);\n> -\t\t\t} else {\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> -\t\t\t\tvariant = determine_ssh_variant(ssh, 0);\n> -\t\t\t}\n> -\n> -\t\t\targv_array_push(&conn->args, ssh);\n> -\n> -\t\t\tif (variant == VARIANT_SSH &&\n> -\t\t\t get_protocol_version_config() > 0) {\n> -\t\t\t\targv_array_push(&conn->args, \"-o\");\n> -\t\t\t\targv_array_push(&conn->args, \"SendEnv=\" GIT_PROTOCOL_ENVIRONMENT);\n> -\t\t\t\targv_array_pushf(&conn->env_array, GIT_PROTOCOL_ENVIRONMENT \"=version=%d\",\n> -\t\t\t\t\t\t get_protocol_version_config());\n> -\t\t\t}\n> -\n> -\t\t\tif (variant != VARIANT_SIMPLE) {\n> -\t\t\t\tif (flags & CONNECT_IPV4)\n> -\t\t\t\t\targv_array_push(&conn->args, \"-4\");\n> -\t\t\t\telse if (flags & CONNECT_IPV6)\n> -\t\t\t\t\targv_array_push(&conn->args, \"-6\");\n> -\t\t\t}\n> -\n> -\t\t\tif (variant == VARIANT_TORTOISEPLINK)\n> -\t\t\t\targv_array_push(&conn->args, \"-batch\");\n> -\n> -\t\t\tif (port && variant != VARIANT_SIMPLE) {\n> -\t\t\t\tif (variant == VARIANT_SSH)\n> -\t\t\t\t\targv_array_push(&conn->args, \"-p\");\n> -\t\t\t\telse\n> -\t\t\t\t\targv_array_push(&conn->args, \"-P\");\n> -\n> -\t\t\t\targv_array_push(&conn->args, port);\n> -\t\t\t}\n> -\n> -\t\t\targv_array_push(&conn->args, ssh_host);\n> +\t\t\tfill_ssh_args(conn, ssh_host, port, flags);\n> \t\t} else {\n> \t\t\ttransport_check_allowed(\"file\");\n> \t\t\tif (get_protocol_version_config() > 0) {\n> -- \n> 2.15.0.448.gf294e3d99a\n> \n\n-- \nBrandon Williams\n"},{"id":"332996","messageId":"20171120220458.sxltsyheixgkunyh@aiede.mtv.corp.google.com","threadId":"47257","inReplyTo":"20171120215218.GB92506@google.com","subject":"Re: [PATCH 3/8] connect: split git:// setup into a separate function","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-11-20T22:04:58Z","receivedAt":"2017-11-20T22:05:06Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Brandon Williams wrote:\n> On 11/20, Jonathan Nieder wrote:\n\n>> +\t/* These underlying connection commands die() if they\n>> +\t * cannot connect.\n>> +\t */\n>\n> I know this is really just code motion but maybe we can fix the style of\n> the comment here?\n\nHow about doing that as a separate commit?\n\n-- >8 --\nSubject: connect: correct style of C-style comment\n\nDocumentation/CodingGuidelines explains:\n\n - Multi-line comments include their delimiters on separate lines from\n the text. E.g.\n\n\t/*\n\t * A very long\n\t * multi-line comment.\n\t */\n\nReported-by: Brandon Williams \nSigned-off-by: Jonathan Nieder \n---\n connect.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git i/connect.c w/connect.c\nindex 20ed1d9574..e544a5e1dd 100644\n--- i/connect.c\n+++ w/connect.c\n@@ -889,7 +889,8 @@ static struct child_process *git_connect_git(int fd[2], char *hostandport,\n \n \ttransport_check_allowed(\"git\");\n \n-\t/* These underlying connection commands die() if they\n+\t/*\n+\t * These underlying connection commands die() if they\n \t * cannot connect.\n \t */\n \tif (git_use_proxy(hostandport))\n"},{"id":"332998","messageId":"20171120220950.f5ipri354x6squck@aiede.mtv.corp.google.com","threadId":"47257","inReplyTo":"20171120215404.GC92506@google.com","subject":"Re: [PATCH 4/8] connect: split ssh command line options into separate function","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-11-20T22:09:50Z","receivedAt":"2017-11-20T22:09:57Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nBrandon Williams wrote:\n> On 11/20, Jonathan Nieder wrote:\n[long stream of quoted context snipped; please cut down the quoted\n text to what you are replying to in the future]\n>> @@ -972,16 +1031,13 @@ struct child_process *git_connect(int fd[2], const char *url,\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\tenum ssh_variant variant;\n>> \t\t\tchar *ssh_host = hostandport;\n>> \t\t\tconst char *port = NULL;\n>> +\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>\n> Are these random additions and deletions intentional?\n\nSorry about that. It was to make the code easier to read, but I can\nsee how it's jarring during review. I can resend without the removed\nblank lines if you like.\n\nFor context, here's the code in question after the current patch:\n\n\tif (protocol == PROTO_SSH) {\n\t\tchar *ssh_host = hostandport;\n\t\tconst char *port = NULL;\n\n\t\ttransport_check_allowed(\"ssh\");\n\t\tget_host_and_port(&ssh_host, &port);\n\t\tif (!port)\n\t\t\tport = get_port(ssh_host);\n\t\tif (flags & CONNECT_DIAG_URL) {\n\t\t\tprintf(\"Diag: url=%s\\n\", url ? url : \"NULL\");\n\t\t\tprintf(\"Diag: protocol=%s\\n\", prot_name(protocol));\n\t\t\tprintf(\"Diag: userandhost=%s\\n\", ssh_host ? ssh_host : \"NULL\");\n\t\t\tprintf(\"Diag: port=%s\\n\", port ? port : \"NONE\");\n\t\t\tprintf(\"Diag: path=%s\\n\", path ? path : \"NULL\");\n\n\t\t\tfree(hostandport);\n\t\t\tfree(path);\n\t\t\tfree(conn);\n\t\t\tstrbuf_release(&cmd);\n\t\t\treturn NULL;\n\t\t}\n\t\tfill_ssh_args(conn, ssh_host, port, flags);\n\t} else {\n\t\ttransport_check_allowed(\"file\");\n\t\tif (get_protocol_version_config() > 0) {\n\t\t\targv_array_pushf(&conn->env_array, GIT_PROTOCOL_ENVIRONMENT \"=version=%d\",\n\t\t\t\t\t get_protocol_version_config());\n\t\t}\n\t}\n\nJonathan\n"},{"id":"333005","messageId":"20171120221943.5slenc6lovg73kif@aiede.mtv.corp.google.com","threadId":"47257","inReplyTo":"20171120215404.GC92506@google.com","subject":"[PATCH v4 4/8] connect: split ssh command line options into separate function","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-11-20T22:19:43Z","receivedAt":"2017-11-20T22:20:05Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"The git_connect function is growing long. Split the portion that\ndiscovers an ssh command and options it accepts before the service\nname and path to a separate function to make it easier to read.\n\nNo functional change intended.\n\nSigned-off-by: Jonathan Nieder \nReviewed-by: Stefan Beller \n---\nBrandon Williams wrote:\n> On 11/20, Jonathan Nieder wrote:\n\n>> @@ -972,16 +1031,13 @@ struct child_process *git_connect(int fd[2], const char *url,\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\tenum ssh_variant variant;\n>> \t\t\tchar *ssh_host = hostandport;\n>> \t\t\tconst char *port = NULL;\n>> +\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>\n> Are these random additions and deletions intentional?\n\nThanks again for noticing this. After looking more closely, I don't\nsee any reason for these whitespace changes. Here's a corrected\npatch.\n\n connect.c | 113 +++++++++++++++++++++++++++++++++-----------------------------\n 1 file changed, 60 insertions(+), 53 deletions(-)\n\ndiff --git a/connect.c b/connect.c\nindex 9425229206..2113feb4f8 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -919,6 +919,65 @@ static struct child_process *git_connect_git(int fd[2], char *hostandport,\n \treturn conn;\n }\n \n+/* Prepare a child_process for use by Git's SSH-tunneled transport. */\n+static void fill_ssh_args(struct child_process *conn, const char *ssh_host,\n+\t\t\t const char *port, int flags)\n+{\n+\tconst char *ssh;\n+\tenum ssh_variant variant;\n+\n+\tif (looks_like_command_line_option(ssh_host))\n+\t\tdie(\"strange hostname '%s' blocked\", ssh_host);\n+\n+\tssh = get_ssh_command();\n+\tif (ssh) {\n+\t\tvariant = determine_ssh_variant(ssh, 1);\n+\t} else {\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\tconn->use_shell = 0;\n+\n+\t\tssh = getenv(\"GIT_SSH\");\n+\t\tif (!ssh)\n+\t\t\tssh = \"ssh\";\n+\t\tvariant = determine_ssh_variant(ssh, 0);\n+\t}\n+\n+\targv_array_push(&conn->args, ssh);\n+\n+\tif (variant == VARIANT_SSH &&\n+\t get_protocol_version_config() > 0) {\n+\t\targv_array_push(&conn->args, \"-o\");\n+\t\targv_array_push(&conn->args, \"SendEnv=\" GIT_PROTOCOL_ENVIRONMENT);\n+\t\targv_array_pushf(&conn->env_array, GIT_PROTOCOL_ENVIRONMENT \"=version=%d\",\n+\t\t\t\t get_protocol_version_config());\n+\t}\n+\n+\tif (variant != VARIANT_SIMPLE) {\n+\t\tif (flags & CONNECT_IPV4)\n+\t\t\targv_array_push(&conn->args, \"-4\");\n+\t\telse if (flags & CONNECT_IPV6)\n+\t\t\targv_array_push(&conn->args, \"-6\");\n+\t}\n+\n+\tif (variant == VARIANT_TORTOISEPLINK)\n+\t\targv_array_push(&conn->args, \"-batch\");\n+\n+\tif (port && variant != VARIANT_SIMPLE) {\n+\t\tif (variant == VARIANT_SSH)\n+\t\t\targv_array_push(&conn->args, \"-p\");\n+\t\telse\n+\t\t\targv_array_push(&conn->args, \"-P\");\n+\n+\t\targv_array_push(&conn->args, port);\n+\t}\n+\n+\targv_array_push(&conn->args, ssh_host);\n+}\n+\n /*\n * This returns the dummy child_process `no_fork` if the transport protocol\n * does not need fork(2), or a struct child_process object if it does. Once\n@@ -972,8 +1031,6 @@ struct child_process *git_connect(int fd[2], const char *url,\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\tenum ssh_variant variant;\n \t\t\tchar *ssh_host = hostandport;\n \t\t\tconst char *port = NULL;\n \t\t\ttransport_check_allowed(\"ssh\");\n@@ -995,57 +1052,7 @@ struct child_process *git_connect(int fd[2], const char *url,\n \t\t\t\tstrbuf_release(&cmd);\n \t\t\t\treturn NULL;\n \t\t\t}\n-\n-\t\t\tif (looks_like_command_line_option(ssh_host))\n-\t\t\t\tdie(\"strange hostname '%s' blocked\", ssh_host);\n-\n-\t\t\tssh = get_ssh_command();\n-\t\t\tif (ssh) {\n-\t\t\t\tvariant = determine_ssh_variant(ssh, 1);\n-\t\t\t} else {\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-\t\t\t\tvariant = determine_ssh_variant(ssh, 0);\n-\t\t\t}\n-\n-\t\t\targv_array_push(&conn->args, ssh);\n-\n-\t\t\tif (variant == VARIANT_SSH &&\n-\t\t\t get_protocol_version_config() > 0) {\n-\t\t\t\targv_array_push(&conn->args, \"-o\");\n-\t\t\t\targv_array_push(&conn->args, \"SendEnv=\" GIT_PROTOCOL_ENVIRONMENT);\n-\t\t\t\targv_array_pushf(&conn->env_array, GIT_PROTOCOL_ENVIRONMENT \"=version=%d\",\n-\t\t\t\t\t\t get_protocol_version_config());\n-\t\t\t}\n-\n-\t\t\tif (variant != VARIANT_SIMPLE) {\n-\t\t\t\tif (flags & CONNECT_IPV4)\n-\t\t\t\t\targv_array_push(&conn->args, \"-4\");\n-\t\t\t\telse if (flags & CONNECT_IPV6)\n-\t\t\t\t\targv_array_push(&conn->args, \"-6\");\n-\t\t\t}\n-\n-\t\t\tif (variant == VARIANT_TORTOISEPLINK)\n-\t\t\t\targv_array_push(&conn->args, \"-batch\");\n-\n-\t\t\tif (port && variant != VARIANT_SIMPLE) {\n-\t\t\t\tif (variant == VARIANT_SSH)\n-\t\t\t\t\targv_array_push(&conn->args, \"-p\");\n-\t\t\t\telse\n-\t\t\t\t\targv_array_push(&conn->args, \"-P\");\n-\n-\t\t\t\targv_array_push(&conn->args, port);\n-\t\t\t}\n-\n-\t\t\targv_array_push(&conn->args, ssh_host);\n+\t\t\tfill_ssh_args(conn, ssh_host, port, flags);\n \t\t} else {\n \t\t\ttransport_check_allowed(\"file\");\n \t\t\tif (get_protocol_version_config() > 0) {\n-- \n2.15.0.448.gf294e3d99a\n\n"},{"id":"333011","messageId":"20171120222521.GD92506@google.com","threadId":"47257","inReplyTo":"20171120213004.57552ja3nmxy6pmc@aiede.mtv.corp.google.com","subject":"Re: [PATCH 6/8] ssh: 'auto' variant to select between 'ssh' and 'simple'","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-11-20T22:25:21Z","receivedAt":"2017-11-20T22:25:30Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 11/20, Jonathan Nieder wrote:\n> Android's \"repo\" tool is a tool for managing a large codebase\n> consisting of multiple smaller repositories, similar to Git's\n> submodule feature. Starting with Git 94b8ae5a (ssh: introduce a\n> 'simple' ssh variant, 2017-10-16), users noticed that it stopped\n> handling the port in ssh:// URLs.\n> \n> The cause: when it encounters ssh:// URLs, repo pre-connects to the\n> server and sets GIT_SSH to a helper \".repo/repo/git_ssh\" that reuses\n> that connection. Before 94b8ae5a, the helper was assumed to support\n> OpenSSH options for lack of a better guess and got passed a -p option\n> to set the port. After that patch, it uses the new default of a\n> simple helper that does not accept an option to set the port.\n> \n> The next release of \"repo\" will set GIT_SSH_VARIANT to \"ssh\" to avoid\n> that. But users of old versions and of other similar GIT_SSH\n> implementations would not get the benefit of that fix.\n> \n> So update the default to use OpenSSH options again, with a twist. As\n> observed in 94b8ae5a, we cannot assume that $GIT_SSH always handles\n> OpenSSH options: common helpers such as travis-ci's dpl[*] are\n> configured using GIT_SSH and do not accept OpenSSH options. So make\n> the default a new variant \"auto\", with the following behavior:\n> \n> 1. First, check for a recognized basename, like today.\n> \n> 2. If the basename is not recognized, check whether $GIT_SSH supports\n> OpenSSH options by running\n> \n> \t$GIT_SSH -G \n> \n> This returns status 0 and prints configuration in OpenSSH if it\n> recognizes all and returns status 255 if it encounters\n> an unrecognized option. A wrapper script like\n> \n> \texec ssh -- \"$@\"\n> \n> would fail with\n> \n> \tssh: Could not resolve hostname -g: Name or service not known\n> \n> , correctly reflecting that it does not support OpenSSH options.\n> The command is run with stdin, stdout, and stderr redirected to\n> /dev/null so even a command that expects a terminal would exit\n> immediately.\n> \n> 3. Based on the result from step (2), behave like \"ssh\" (if it\n> succeeded) or \"simple\" (if it failed).\n> \n> This way, the default ssh variant for unrecognized commands can handle\n> both the repo and dpl cases as intended.\n> \n> This autodetection has been running on Google workstations since\n> 2017-10-23 with no reported negative effects.\n> \n> [*] https://github.com/travis-ci/dpl/blob/6c3fddfda1f2a85944c544446b068bac0a77c049/lib/dpl/provider.rb#L215\n> \n> Reported-by: William Yan \n> Improved-by: Jonathan Tan \n> Signed-off-by: Jonathan Nieder \n> ---\n> Added two notes to the commit message:\n> - describing the real-world testing this patch has undergone\n> - stdin, stdout, and stderr go to /dev/null, preventing a\n> hypothetical ssh variant that *ignores* -G from hanging waiting for\n> input from the terminal.\n> \n> This is to address the worries at\n> https://public-inbox.org/git/xmqq60b59toe.fsf@gitster.mtv.corp.google.com/\n> and https://public-inbox.org/git/CAGZ79kZTjUvcq_hKHCqTDoaBxt2x+9XcqYc6ao1bhcET2SM-PQ@mail.gmail.com/\n> about hanging.\n> \n> No change to the code from last time.\n> \n\nThanks a lot for getting this patch out. It's a much more robust\nsolution than I had originally and should hopefully avoid any more issues\nwith detecting different ssh programs. My only concern is there may be\nsome program out there that uses -G in a way different from OpenSSH.\nThough realistically i don't think that is an issue because if that ends\nup being a problem we can just have the authors of the offending ssh\nclient send a patch to us to fix it! :)\n\n> Documentation/config.txt | 24 +++++++++++++++---------\n> connect.c | 32 +++++++++++++++++++++++++-------\n> t/t5601-clone.sh | 21 +++++++++++++++++++++\n> 3 files changed, 61 insertions(+), 16 deletions(-)\n> \n> diff --git a/Documentation/config.txt b/Documentation/config.txt\n> index 0460af37e2..0c371ad786 100644\n> --- a/Documentation/config.txt\n> +++ b/Documentation/config.txt\n> @@ -2081,16 +2081,22 @@ matched against are those given directly to Git commands. This means any URLs\n> visited as a result of a redirection do not participate in matching.\n> \n> ssh.variant::\n> -\tDepending on the value of the environment variables `GIT_SSH` or\n> -\t`GIT_SSH_COMMAND`, or the config setting `core.sshCommand`, Git\n> -\tauto-detects whether to adjust its command-line parameters for use\n> -\twith ssh (OpenSSH), plink or tortoiseplink, as opposed to the default\n> -\t(simple).\n> +\tBy default, Git determines the command line arguments to use\n> +\tbased on the basename of the configured SSH command (configured\n> +\tusing the environment variable `GIT_SSH` or `GIT_SSH_COMMAND` or\n> +\tthe config setting `core.sshCommand`). If the basename is\n> +\tunrecognized, Git will attempt to detect support of OpenSSH\n> +\toptions by first invoking the configured SSH command with the\n> +\t`-G` (print configuration) option and will subsequently use\n> +\tOpenSSH options (if that is successful) or no options besides\n> +\tthe host and remote command (if it fails).\n> +\n> -The config variable `ssh.variant` can be set to override this auto-detection;\n> -valid values are `ssh`, `simple`, `plink`, `putty` or `tortoiseplink`. Any\n> -other value will be treated as normal ssh. This setting can be overridden via\n> -the environment variable `GIT_SSH_VARIANT`.\n> +The config variable `ssh.variant` can be set to override this detection.\n> +Valid values are `ssh` (to use OpenSSH options), `plink`, `putty`,\n> +`tortoiseplink`, `simple` (no options except the host and remote command).\n> +The default auto-detection can be explicitly requested using the value\n> +`auto`. Any other value is treated as `ssh`. This setting can also be\n> +overridden via the environment variable `GIT_SSH_VARIANT`.\n> +\n> The current command-line parameters used for each variant are as\n> follows:\n> diff --git a/connect.c b/connect.c\n> index d2fbb15cc5..455c54a2ec 100644\n> --- a/connect.c\n> +++ b/connect.c\n> @@ -788,6 +788,7 @@ static const char *get_ssh_command(void)\n> }\n> \n> enum ssh_variant {\n> +\tVARIANT_AUTO,\n> \tVARIANT_SIMPLE,\n> \tVARIANT_SSH,\n> \tVARIANT_PLINK,\n> @@ -795,14 +796,16 @@ enum ssh_variant {\n> \tVARIANT_TORTOISEPLINK,\n> };\n> \n> -static int override_ssh_variant(enum ssh_variant *ssh_variant)\n> +static void override_ssh_variant(enum ssh_variant *ssh_variant)\n> {\n> \tconst char *variant = getenv(\"GIT_SSH_VARIANT\");\n> \n> \tif (!variant && git_config_get_string_const(\"ssh.variant\", &variant))\n> -\t\treturn 0;\n> +\t\treturn;\n> \n> -\tif (!strcmp(variant, \"plink\"))\n> +\tif (!strcmp(variant, \"auto\"))\n> +\t\t*ssh_variant = VARIANT_AUTO;\n> +\telse if (!strcmp(variant, \"plink\"))\n> \t\t*ssh_variant = VARIANT_PLINK;\n> \telse if (!strcmp(variant, \"putty\"))\n> \t\t*ssh_variant = VARIANT_PUTTY;\n> @@ -812,18 +815,18 @@ static int override_ssh_variant(enum ssh_variant *ssh_variant)\n> \t\t*ssh_variant = VARIANT_SIMPLE;\n> \telse\n> \t\t*ssh_variant = VARIANT_SSH;\n> -\n> -\treturn 1;\n> }\n> \n> static enum ssh_variant determine_ssh_variant(const char *ssh_command,\n> \t\t\t\t\t int is_cmdline)\n> {\n> -\tenum ssh_variant ssh_variant = VARIANT_SIMPLE;\n> +\tenum ssh_variant ssh_variant = VARIANT_AUTO;\n> \tconst char *variant;\n> \tchar *p = NULL;\n> \n> -\tif (override_ssh_variant(&ssh_variant))\n> +\toverride_ssh_variant(&ssh_variant);\n> +\n> +\tif (ssh_variant != VARIANT_AUTO)\n> \t\treturn ssh_variant;\n> \n> \tif (!is_cmdline) {\n> @@ -982,6 +985,21 @@ static void fill_ssh_args(struct child_process *conn, const char *ssh_host,\n> \t\tvariant = determine_ssh_variant(ssh, 0);\n> \t}\n> \n> +\tif (variant == VARIANT_AUTO) {\n> +\t\tstruct child_process detect = CHILD_PROCESS_INIT;\n> +\n> +\t\tdetect.use_shell = conn->use_shell;\n> +\t\tdetect.no_stdin = detect.no_stdout = detect.no_stderr = 1;\n> +\n> +\t\targv_array_push(&detect.args, ssh);\n> +\t\targv_array_push(&detect.args, \"-G\");\n> +\t\tpush_ssh_options(&detect.args, &detect.env_array,\n> +\t\t\t\t VARIANT_SSH, port, flags);\n> +\t\targv_array_push(&detect.args, ssh_host);\n> +\n> +\t\tvariant = run_command(&detect) ? VARIANT_SIMPLE : VARIANT_SSH;\n> +\t}\n> +\n> \targv_array_push(&conn->args, ssh);\n> \tpush_ssh_options(&conn->args, &conn->env_array, variant, port, flags);\n> \targv_array_push(&conn->args, ssh_host);\n> diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh\n> index 9d007c0f8d..209e2d5604 100755\n> --- a/t/t5601-clone.sh\n> +++ b/t/t5601-clone.sh\n> @@ -369,6 +369,12 @@ test_expect_success 'variant can be overriden' '\n> \texpect_ssh myhost src\n> '\n> \n> +test_expect_success 'variant=auto picks based on basename' '\n> +\tcopy_ssh_wrapper_as \"$TRASH_DIRECTORY/plink\" &&\n> +\tgit -c ssh.variant=auto clone -4 \"[myhost:123]:src\" ssh-auto-clone &&\n> +\texpect_ssh \"-4 -P 123\" myhost src\n> +'\n> +\n> test_expect_success 'simple is treated as simple' '\n> \tcopy_ssh_wrapper_as \"$TRASH_DIRECTORY/simple\" &&\n> \tgit clone -4 \"[myhost:123]:src\" ssh-bracket-clone-simple &&\n> @@ -381,6 +387,21 @@ test_expect_success 'uplink is treated as simple' '\n> \texpect_ssh myhost src\n> '\n> \n> +test_expect_success 'OpenSSH-like uplink is treated as ssh' '\n> +\twrite_script \"$TRASH_DIRECTORY/uplink\" <<-EOF &&\n> +\tif test \"\\$1\" = \"-G\"\n> +\tthen\n> +\t\texit 0\n> +\tfi &&\n> +\texec \"\\$TRASH_DIRECTORY/ssh$X\" \"\\$@\"\n> +\tEOF\n> +\ttest_when_finished \"rm -f \\\"\\$TRASH_DIRECTORY/uplink\\\"\" &&\n> +\tGIT_SSH=\"$TRASH_DIRECTORY/uplink\" &&\n> +\ttest_when_finished \"GIT_SSH=\\\"\\$TRASH_DIRECTORY/ssh\\$X\\\"\" &&\n> +\tgit clone \"[myhost:123]:src\" ssh-bracket-clone-sshlike-uplink &&\n> +\texpect_ssh \"-p 123\" myhost src\n> +'\n> +\n> test_expect_success 'plink is treated specially (as putty)' '\n> \tcopy_ssh_wrapper_as \"$TRASH_DIRECTORY/plink\" &&\n> \tgit clone \"[myhost:123]:src\" ssh-bracket-clone-plink-0 &&\n> -- \n> 2.15.0.448.gf294e3d99a\n> \n\n-- \nBrandon Williams\n"},{"id":"333014","messageId":"20171120222820.GE92506@google.com","threadId":"47257","inReplyTo":"20171120220950.f5ipri354x6squck@aiede.mtv.corp.google.com","subject":"Re: [PATCH 4/8] connect: split ssh command line options into separate function","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-11-20T22:28:20Z","receivedAt":"2017-11-20T22:28:28Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 11/20, Jonathan Nieder wrote:\n> Hi,\n> \n> Brandon Williams wrote:\n> > On 11/20, Jonathan Nieder wrote:\n> [long stream of quoted context snipped; please cut down the quoted\n> text to what you are replying to in the future]\n> >> @@ -972,16 +1031,13 @@ struct child_process *git_connect(int fd[2], const char *url,\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\tenum ssh_variant variant;\n> >> \t\t\tchar *ssh_host = hostandport;\n> >> \t\t\tconst char *port = NULL;\n> >> +\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> >\n> > Are these random additions and deletions intentional?\n> \n> Sorry about that. It was to make the code easier to read, but I can\n> see how it's jarring during review. I can resend without the removed\n> blank lines if you like.\n\nIf its for better code readability I'm all for the additions and\ndeletions. I just wanted to make sure they were intentional :)\n\nSo I'm fine with either the original or the modified patch.\n\n> \n> For context, here's the code in question after the current patch:\n> \n> \tif (protocol == PROTO_SSH) {\n> \t\tchar *ssh_host = hostandport;\n> \t\tconst char *port = NULL;\n> \n> \t\ttransport_check_allowed(\"ssh\");\n> \t\tget_host_and_port(&ssh_host, &port);\n> \t\tif (!port)\n> \t\t\tport = get_port(ssh_host);\n> \t\tif (flags & CONNECT_DIAG_URL) {\n> \t\t\tprintf(\"Diag: url=%s\\n\", url ? url : \"NULL\");\n> \t\t\tprintf(\"Diag: protocol=%s\\n\", prot_name(protocol));\n> \t\t\tprintf(\"Diag: userandhost=%s\\n\", ssh_host ? ssh_host : \"NULL\");\n> \t\t\tprintf(\"Diag: port=%s\\n\", port ? port : \"NONE\");\n> \t\t\tprintf(\"Diag: path=%s\\n\", path ? path : \"NULL\");\n> \n> \t\t\tfree(hostandport);\n> \t\t\tfree(path);\n> \t\t\tfree(conn);\n> \t\t\tstrbuf_release(&cmd);\n> \t\t\treturn NULL;\n> \t\t}\n> \t\tfill_ssh_args(conn, ssh_host, port, flags);\n> \t} else {\n> \t\ttransport_check_allowed(\"file\");\n> \t\tif (get_protocol_version_config() > 0) {\n> \t\t\targv_array_pushf(&conn->env_array, GIT_PROTOCOL_ENVIRONMENT \"=version=%d\",\n> \t\t\t\t\t get_protocol_version_config());\n> \t\t}\n> \t}\n> \n> Jonathan\n\n-- \nBrandon Williams\n"},{"id":"333015","messageId":"20171120222925.GF92506@google.com","threadId":"47257","inReplyTo":"20171120220458.sxltsyheixgkunyh@aiede.mtv.corp.google.com","subject":"Re: [PATCH 3/8] connect: split git:// setup into a separate function","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-11-20T22:29:25Z","receivedAt":"2017-11-20T22:29:32Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 11/20, Jonathan Nieder wrote:\n> Brandon Williams wrote:\n> > On 11/20, Jonathan Nieder wrote:\n> \n> >> +\t/* These underlying connection commands die() if they\n> >> +\t * cannot connect.\n> >> +\t */\n> >\n> > I know this is really just code motion but maybe we can fix the style of\n> > the comment here?\n> \n> How about doing that as a separate commit?\n\nLooks good!\n\n> \n> -- >8 --\n> Subject: connect: correct style of C-style comment\n> \n> Documentation/CodingGuidelines explains:\n> \n> - Multi-line comments include their delimiters on separate lines from\n> the text. E.g.\n> \n> \t/*\n> \t * A very long\n> \t * multi-line comment.\n> \t */\n> \n> Reported-by: Brandon Williams \n> Signed-off-by: Jonathan Nieder \n> ---\n> connect.c | 3 ++-\n> 1 file changed, 2 insertions(+), 1 deletion(-)\n> \n> diff --git i/connect.c w/connect.c\n> index 20ed1d9574..e544a5e1dd 100644\n> --- i/connect.c\n> +++ w/connect.c\n> @@ -889,7 +889,8 @@ static struct child_process *git_connect_git(int fd[2], char *hostandport,\n> \n> \ttransport_check_allowed(\"git\");\n> \n> -\t/* These underlying connection commands die() if they\n> +\t/*\n> +\t * These underlying connection commands die() if they\n> \t * cannot connect.\n> \t */\n> \tif (git_use_proxy(hostandport))\n\n-- \nBrandon Williams\n"},{"id":"333016","messageId":"20171120223232.GG92506@google.com","threadId":"47257","inReplyTo":"20171120212134.lh2l4drdzu6fh5g2@aiede.mtv.corp.google.com","subject":"Re: [PATCH v3 0/8] Coping with unrecognized ssh wrapper scripts in GIT_SSH","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-11-20T22:32:32Z","receivedAt":"2017-11-20T22:32:39Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 11/20, Jonathan Nieder wrote:\n> Previously: [1].\n> \n> This version should be essentially identical to v2. Changes:\n> - patch 1 is new and should fix the test failure on Windows\n> - patch 2 is new, discussed at [2]\n> - patch 5 split off from patch 6 as suggested at [3]\n> - patch 6 commit message got two new notes to address the worries\n> from [3]\n> \n> Thanks for the helpful reviews, and sorry to take so long to get this\n> out. Thoughts of all kinds welcome, as always.\n\nJust finished looking through the series. Looks good overall!\n\nThanks again for getting this out!\n\n> \n> Sincerely,\n> Jonathan Nieder (8):\n> ssh test: make copy_ssh_wrapper_as clean up after itself\n> connect: move no_fork fallback to git_tcp_connect\n> connect: split git:// setup into a separate function\n> connect: split ssh command line options into separate function\n> connect: split ssh option computation to its own function\n> ssh: 'auto' variant to select between 'ssh' and 'simple'\n> ssh: 'simple' variant does not support -4/-6\n> ssh: 'simple' variant does not support --port\n> \n> Documentation/config.txt | 24 ++--\n> connect.c | 322 +++++++++++++++++++++++++++++------------------\n> t/t5601-clone.sh | 69 ++++++----\n> t/t5603-clone-dirname.sh | 2 +\n> 4 files changed, 265 insertions(+), 152 deletions(-)\n> \n> [1] https://public-inbox.org/git/20171023231625.6mhcyqti7vdg6yot@aiede.mtv.corp.google.com/\n> [2] https://public-inbox.org/git/20171115202516.hduhzsgeoff5a22b@aiede.mtv.corp.google.com/\n> [3] https://public-inbox.org/git/xmqq60b59toe.fsf@gitster.mtv.corp.google.com/\n\n-- \nBrandon Williams\n"},{"id":"333037","messageId":"xmqqtvxojufd.fsf@gitster.mtv.corp.google.com","threadId":"47257","inReplyTo":"20171120212223.wquyxbmz34foynrk@aiede.mtv.corp.google.com","subject":"Re: [PATCH 1/8] ssh test: make copy_ssh_wrapper_as clean up after itself","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-11-21T01:24:06Z","receivedAt":"2017-11-21T01:24:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n> +test_expect_success 'set up ssh wrapper' '\n> +\tcp \"$GIT_BUILD_DIR/t/helper/test-fake-ssh$X\" \\\n> +\t\t\"$TRASH_DIRECTORY/ssh$X\" &&\n> +\tGIT_SSH=\"$TRASH_DIRECTORY/ssh$X\" &&\n> +\texport GIT_SSH &&\n> +\texport TRASH_DIRECTORY &&\n> +\t>\"$TRASH_DIRECTORY\"/ssh-output\n> +'\n> \n> copy_ssh_wrapper_as () {\n> \tcp \"$TRASH_DIRECTORY/ssh$X\" \"${1%$X}$X\" &&\n> +\ttest_when_finished \"rm -f ${1%$X}$X\" &&\n> \tGIT_SSH=\"${1%$X}$X\" &&\n\nAs we can clearly see in the context, this is not a new issue, but I\nfind the users of this helper that benefit from the \"${1%$X}$X\"\nmagic somewhat iffy.\n\nThere are callers of this helper that pass \"somedir/plink\" and\n\"somedir/plink.exe\", but behind these callers that _think_ they are\ntesting the variant with and without the trailing \".exe\", the helper\nalways add \".exe\" (after stripping an existing one) on $X=.exe\nplatforms, ending up in testing the same thing twice. On platforms\nwith $X='', testing two different command names may have \"some\"\nvalue, but I wonder if it is cleaner to use a much less magical\n\"$1$X\" here, and skip the test with a caller that gives \".exe\"\nvariant using a test prerequisite on $X=.exe platforms to avoid\nredundant tests?\n\nThis is totally outside the scope of this series; I mention this\nonly because this may be a possible #leftoverbits.\n\nThanks.\n"},{"id":"333038","messageId":"xmqqpo8cju2s.fsf@gitster.mtv.corp.google.com","threadId":"47257","inReplyTo":"20171120212619.nqtijn5ygypixmw6@aiede.mtv.corp.google.com","subject":"Re: [PATCH 5/8] connect: split ssh option computation to its own function","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-11-21T01:31:39Z","receivedAt":"2017-11-21T01:31:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n> This puts the determination of options to pass to each ssh variant\n> (see ssh.variant in git-config(1)) in one place.\n>\n> A follow-up patch will use this in an initial dry run to detect which\n> variant to use when the ssh command is ambiguous.\n>\n> No functional change intended yet.\n>\n> Signed-off-by: Jonathan Nieder \n> ---\n> Split out to make patch 6 easier to read, as suggested at\n> https://public-inbox.org/git/xmqq60b59toe.fsf@gitster.mtv.corp.google.com/.\n>\n> Added a function comment to make the purpose and API of this internal\n> helper clearer.\n\nThe resulting fill-ssh-args reads a lot nicer. Good.\n\n>\n> connect.c | 65 ++++++++++++++++++++++++++++++++++++---------------------------\n> 1 file changed, 37 insertions(+), 28 deletions(-)\n>\n> diff --git a/connect.c b/connect.c\n> index a9dc493db2..d2fbb15cc5 100644\n> --- a/connect.c\n> +++ b/connect.c\n> @@ -919,6 +919,42 @@ static struct child_process *git_connect_git(int fd[2], char *hostandport,\n> \treturn conn;\n> }\n> \n> +/*\n> + * Append the appropriate environment variables to `env` and options to\n> + * `args` for running ssh in Git's SSH-tunneled transport.\n> + */\n> +static void push_ssh_options(struct argv_array *args, struct argv_array *env,\n> +\t\t\t enum ssh_variant variant, const char *port,\n> +\t\t\t int flags)\n> +{\n> +\tif (variant == VARIANT_SSH &&\n> +\t get_protocol_version_config() > 0) {\n> +\t\targv_array_push(args, \"-o\");\n> +\t\targv_array_push(args, \"SendEnv=\" GIT_PROTOCOL_ENVIRONMENT);\n> +\t\targv_array_pushf(env, GIT_PROTOCOL_ENVIRONMENT \"=version=%d\",\n> +\t\t\t\t get_protocol_version_config());\n> +\t}\n> +\n> +\tif (variant != VARIANT_SIMPLE) {\n> +\t\tif (flags & CONNECT_IPV4)\n> +\t\t\targv_array_push(args, \"-4\");\n> +\t\telse if (flags & CONNECT_IPV6)\n> +\t\t\targv_array_push(args, \"-6\");\n> +\t}\n> +\n> +\tif (variant == VARIANT_TORTOISEPLINK)\n> +\t\targv_array_push(args, \"-batch\");\n> +\n> +\tif (port && variant != VARIANT_SIMPLE) {\n> +\t\tif (variant == VARIANT_SSH)\n> +\t\t\targv_array_push(args, \"-p\");\n> +\t\telse\n> +\t\t\targv_array_push(args, \"-P\");\n> +\n> +\t\targv_array_push(args, port);\n> +\t}\n> +}\n> +\n> /* Prepare a child_process for use by Git's SSH-tunneled transport. */\n> static void fill_ssh_args(struct child_process *conn, const char *ssh_host,\n> \t\t\t const char *port, int flags)\n> @@ -947,34 +983,7 @@ static void fill_ssh_args(struct child_process *conn, const char *ssh_host,\n> \t}\n> \n> \targv_array_push(&conn->args, ssh);\n> -\n> -\tif (variant == VARIANT_SSH &&\n> -\t get_protocol_version_config() > 0) {\n> -\t\targv_array_push(&conn->args, \"-o\");\n> -\t\targv_array_push(&conn->args, \"SendEnv=\" GIT_PROTOCOL_ENVIRONMENT);\n> -\t\targv_array_pushf(&conn->env_array, GIT_PROTOCOL_ENVIRONMENT \"=version=%d\",\n> -\t\t\t\t get_protocol_version_config());\n> -\t}\n> -\n> -\tif (variant != VARIANT_SIMPLE) {\n> -\t\tif (flags & CONNECT_IPV4)\n> -\t\t\targv_array_push(&conn->args, \"-4\");\n> -\t\telse if (flags & CONNECT_IPV6)\n> -\t\t\targv_array_push(&conn->args, \"-6\");\n> -\t}\n> -\n> -\tif (variant == VARIANT_TORTOISEPLINK)\n> -\t\targv_array_push(&conn->args, \"-batch\");\n> -\n> -\tif (port && variant != VARIANT_SIMPLE) {\n> -\t\tif (variant == VARIANT_SSH)\n> -\t\t\targv_array_push(&conn->args, \"-p\");\n> -\t\telse\n> -\t\t\targv_array_push(&conn->args, \"-P\");\n> -\n> -\t\targv_array_push(&conn->args, port);\n> -\t}\n> -\n> +\tpush_ssh_options(&conn->args, &conn->env_array, variant, port, flags);\n> \targv_array_push(&conn->args, ssh_host);\n> }\n"},{"id":"333039","messageId":"xmqqlgj0jtai.fsf@gitster.mtv.corp.google.com","threadId":"47257","inReplyTo":"20171120213004.57552ja3nmxy6pmc@aiede.mtv.corp.google.com","subject":"Re: [PATCH 6/8] ssh: 'auto' variant to select between 'ssh' and 'simple'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-11-21T01:48:37Z","receivedAt":"2017-11-21T01:48:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n> Android's \"repo\" tool is a tool for managing a large codebase\n> consisting of multiple smaller repositories, similar to Git's\n> submodule feature. Starting with Git 94b8ae5a (ssh: introduce a\n> 'simple' ssh variant, 2017-10-16), users noticed that it stopped\n> handling the port in ssh:// URLs.\n>\n> ...\n> Reported-by: William Yan \n> Improved-by: Jonathan Tan \n> Signed-off-by: Jonathan Nieder \n> ---\n\nNot a big issue, but the above made me wonder, due to lack of any\nsigned-off-by before improved-by, what \"base\" was improved by JTan.\nIf you were writing a change before formally passing it around with\nyour sign-off and somebody had a valuable input to improve it, it\nseems that people say helped-by around here.\n\n> ssh.variant::\n> -\tDepending on the value of the environment variables `GIT_SSH` or\n> -\t`GIT_SSH_COMMAND`, or the config setting `core.sshCommand`, Git\n> -\tauto-detects whether to adjust its command-line parameters for use\n> -\twith ssh (OpenSSH), plink or tortoiseplink, as opposed to the default\n> -\t(simple).\n> +\tBy default, Git determines the command line arguments to use\n> +\tbased on the basename of the configured SSH command (configured\n> +\tusing the environment variable `GIT_SSH` or `GIT_SSH_COMMAND` or\n> +\tthe config setting `core.sshCommand`). If the basename is\n> +\tunrecognized, Git will attempt to detect support of OpenSSH\n> +\toptions by first invoking the configured SSH command with the\n> +\t`-G` (print configuration) option and will subsequently use\n> +\tOpenSSH options (if that is successful) or no options besides\n> +\tthe host and remote command (if it fails).\n> +\n> -The config variable `ssh.variant` can be set to override this auto-detection;\n> -valid values are `ssh`, `simple`, `plink`, `putty` or `tortoiseplink`. Any\n> -other value will be treated as normal ssh. This setting can be overridden via\n> -the environment variable `GIT_SSH_VARIANT`.\n> +The config variable `ssh.variant` can be set to override this detection.\n> +Valid values are `ssh` (to use OpenSSH options), `plink`, `putty`,\n> +`tortoiseplink`, `simple` (no options except the host and remote command).\n> +The default auto-detection can be explicitly requested using the value\n> +`auto`. Any other value is treated as `ssh`. This setting can also be\n> +overridden via the environment variable `GIT_SSH_VARIANT`.\n> +\n\nCleanly written and easily read. Good.\n\n> @@ -982,6 +985,21 @@ static void fill_ssh_args(struct child_process *conn, const char *ssh_host,\n> \t\tvariant = determine_ssh_variant(ssh, 0);\n> \t}\n> \n> +\tif (variant == VARIANT_AUTO) {\n> +\t\tstruct child_process detect = CHILD_PROCESS_INIT;\n> +\n> +\t\tdetect.use_shell = conn->use_shell;\n> +\t\tdetect.no_stdin = detect.no_stdout = detect.no_stderr = 1;\n> +\n> +\t\targv_array_push(&detect.args, ssh);\n> +\t\targv_array_push(&detect.args, \"-G\");\n> +\t\tpush_ssh_options(&detect.args, &detect.env_array,\n> +\t\t\t\t VARIANT_SSH, port, flags);\n> +\t\targv_array_push(&detect.args, ssh_host);\n> +\n> +\t\tvariant = run_command(&detect) ? VARIANT_SIMPLE : VARIANT_SSH;\n> +\t}\n\nI briefly wondered if it safe to simply append \"-G\" in both cases,\ni.e. where \"ssh\" came from GIT_SSH and it came from GIT_SSH_COMMAND,\nbut in the real invocation we see in the post-context of this hunk,\nwe'd push options the same way regardless, so it should be fine.\n\n> \targv_array_push(&conn->args, ssh);\n> \tpush_ssh_options(&conn->args, &conn->env_array, variant, port, flags);\n> \targv_array_push(&conn->args, ssh_host);\n\n> +test_expect_success 'OpenSSH-like uplink is treated as ssh' '\n> +\twrite_script \"$TRASH_DIRECTORY/uplink\" <<-EOF &&\n> +\tif test \"\\$1\" = \"-G\"\n> +\tthen\n> +\t\texit 0\n> +\tfi &&\n> +\texec \"\\$TRASH_DIRECTORY/ssh$X\" \"\\$@\"\n> +\tEOF\n\nOK.\n\n> +\ttest_when_finished \"rm -f \\\"\\$TRASH_DIRECTORY/uplink\\\"\" &&\n> +\tGIT_SSH=\"$TRASH_DIRECTORY/uplink\" &&\n> +\ttest_when_finished \"GIT_SSH=\\\"\\$TRASH_DIRECTORY/ssh\\$X\\\"\" &&\n> +\tgit clone \"[myhost:123]:src\" ssh-bracket-clone-sshlike-uplink &&\n> +\texpect_ssh \"-p 123\" myhost src\n> +'\n> +\n> test_expect_success 'plink is treated specially (as putty)' '\n> \tcopy_ssh_wrapper_as \"$TRASH_DIRECTORY/plink\" &&\n> \tgit clone \"[myhost:123]:src\" ssh-bracket-clone-plink-0 &&\n\nThanks.\n"},{"id":"333040","messageId":"20171121014919.p4cl5esf7evptzbk@aiede.mtv.corp.google.com","threadId":"47257","inReplyTo":"20171120212223.wquyxbmz34foynrk@aiede.mtv.corp.google.com","subject":"[PATCH 1/8 v2] ssh test: make copy_ssh_wrapper_as clean up after itself","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-11-21T01:49:19Z","receivedAt":"2017-11-21T01:49:27Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Simplify by not allowing the copied ssh wrapper to persist between\ntests. This way, tests can be safely reordered, added, and removed\nwith less fear of hidden side effects.\n\nThis also avoids having to call setup_ssh_wrapper to restore the value\nof GIT_SSH after this battery of tests, since it means each test will\nrestore it individually.\n\nNoticed because on Windows, if `uplink.exe` exists, the MSYS2 Bash\nwill overwrite that when redirecting via `>uplink`. A proposed test\nwrote a script to 'uplink' after a previous test created uplink.exe\nusing copy_ssh_wrapper_as, so the script written with '>uplink' had\nthe wrong filename and failed.\n\nReported-by: Johannes Schindelin \nSigned-off-by: Jonathan Nieder \nAcked-by: Brandon Williams \n---\nJonathan Nieder wrote:\n\n> Thanks to Dscho for tracking this subtle issue down.\n>\n> t/t5601-clone.sh | 28 ++++++++++++----------------\n> 1 file changed, 12 insertions(+), 16 deletions(-)\n[...]\n> +++ b/t/t5601-clone.sh\n> @@ -306,21 +306,20 @@ test_expect_success 'clone checking out a tag' '\n[...]\n> copy_ssh_wrapper_as () {\n> \tcp \"$TRASH_DIRECTORY/ssh$X\" \"${1%$X}$X\" &&\n> +\ttest_when_finished \"rm -f ${1%$X}$X\" &&\n\nI noticed while looking this over again that it is not exactly right.\n$1 is likely to include spaces, causing the \"rm\" command not to delete\nanything. Removing the \"-f\" confirms the bug.\n\nA small tweak fixes it:\n\n diff --git i/t/t5601-clone.sh w/t/t5601-clone.sh\n index 9d007c0f8d..4a16a0b7dd 100755\n --- i/t/t5601-clone.sh\n +++ w/t/t5601-clone.sh\n @@ -317,7 +317,7 @@ test_expect_success 'set up ssh wrapper' '\n \n copy_ssh_wrapper_as () {\n \tcp \"$TRASH_DIRECTORY/ssh$X\" \"${1%$X}$X\" &&\n -\ttest_when_finished \"rm -f ${1%$X}$X\" &&\n +\ttest_when_finished \"rm $(git rev-parse --sq-quote \"${1%$X}$X\")\" &&\n \tGIT_SSH=\"${1%$X}$X\" &&\n \ttest_when_finished \"GIT_SSH=\\\"\\$TRASH_DIRECTORY/ssh\\$X\\\"\"\n }\n\n t/t5601-clone.sh | 28 ++++++++++++----------------\n 1 file changed, 12 insertions(+), 16 deletions(-)\n\ndiff --git a/t/t5601-clone.sh b/t/t5601-clone.sh\nindex 86811a0c35..4a16a0b7dd 100755\n--- a/t/t5601-clone.sh\n+++ b/t/t5601-clone.sh\n@@ -306,21 +306,20 @@ test_expect_success 'clone checking out a tag' '\n \ttest_cmp fetch.expected fetch.actual\n '\n \n-setup_ssh_wrapper () {\n-\ttest_expect_success 'setup ssh wrapper' '\n-\t\tcp \"$GIT_BUILD_DIR/t/helper/test-fake-ssh$X\" \\\n-\t\t\t\"$TRASH_DIRECTORY/ssh$X\" &&\n-\t\tGIT_SSH=\"$TRASH_DIRECTORY/ssh$X\" &&\n-\t\texport GIT_SSH &&\n-\t\texport TRASH_DIRECTORY &&\n-\t\t>\"$TRASH_DIRECTORY\"/ssh-output\n-\t'\n-}\n+test_expect_success 'set up ssh wrapper' '\n+\tcp \"$GIT_BUILD_DIR/t/helper/test-fake-ssh$X\" \\\n+\t\t\"$TRASH_DIRECTORY/ssh$X\" &&\n+\tGIT_SSH=\"$TRASH_DIRECTORY/ssh$X\" &&\n+\texport GIT_SSH &&\n+\texport TRASH_DIRECTORY &&\n+\t>\"$TRASH_DIRECTORY\"/ssh-output\n+'\n \n copy_ssh_wrapper_as () {\n \tcp \"$TRASH_DIRECTORY/ssh$X\" \"${1%$X}$X\" &&\n+\ttest_when_finished \"rm $(git rev-parse --sq-quote \"${1%$X}$X\")\" &&\n \tGIT_SSH=\"${1%$X}$X\" &&\n-\texport GIT_SSH\n+\ttest_when_finished \"GIT_SSH=\\\"\\$TRASH_DIRECTORY/ssh\\$X\\\"\"\n }\n \n expect_ssh () {\n@@ -344,8 +343,6 @@ expect_ssh () {\n \t(cd \"$TRASH_DIRECTORY\" && test_cmp ssh-expect ssh-output)\n }\n \n-setup_ssh_wrapper\n-\n test_expect_success 'clone myhost:src uses ssh' '\n \tgit clone myhost:src ssh-clone &&\n \texpect_ssh myhost src\n@@ -432,12 +429,14 @@ test_expect_success 'ssh.variant overrides plink detection' '\n '\n \n test_expect_success 'GIT_SSH_VARIANT overrides plink detection to plink' '\n+\tcopy_ssh_wrapper_as \"$TRASH_DIRECTORY/plink\" &&\n \tGIT_SSH_VARIANT=plink \\\n \tgit clone \"[myhost:123]:src\" ssh-bracket-clone-variant-3 &&\n \texpect_ssh \"-P 123\" myhost src\n '\n \n test_expect_success 'GIT_SSH_VARIANT overrides plink to tortoiseplink' '\n+\tcopy_ssh_wrapper_as \"$TRASH_DIRECTORY/plink\" &&\n \tGIT_SSH_VARIANT=tortoiseplink \\\n \tgit clone \"[myhost:123]:src\" ssh-bracket-clone-variant-4 &&\n \texpect_ssh \"-batch -P 123\" myhost src\n@@ -449,9 +448,6 @@ test_expect_success 'clean failure on broken quoting' '\n \t\tgit clone \"[myhost:123]:src\" sq-failure\n '\n \n-# Reset the GIT_SSH environment variable for clone tests.\n-setup_ssh_wrapper\n-\n counter=0\n # $1 url\n # $2 none|host\n-- \n2.15.0.448.gf294e3d99a\n\n"},{"id":"333042","messageId":"20171121020152.b35kxjyzdveclsu6@aiede.mtv.corp.google.com","threadId":"47257","inReplyTo":"xmqqlgj0jtai.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 6/8] ssh: 'auto' variant to select between 'ssh' and 'simple'","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-11-21T02:01:52Z","receivedAt":"2017-11-21T02:01:59Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Junio C Hamano wrote:\n> Jonathan Nieder writes:\n\n>> Android's \"repo\" tool is a tool for managing a large codebase\n>> consisting of multiple smaller repositories, similar to Git's\n>> submodule feature. Starting with Git 94b8ae5a (ssh: introduce a\n>> 'simple' ssh variant, 2017-10-16), users noticed that it stopped\n>> handling the port in ssh:// URLs.\n>>\n>> ...\n>> Reported-by: William Yan \n>> Improved-by: Jonathan Tan \n>> Signed-off-by: Jonathan Nieder \n>> ---\n>\n> Not a big issue, but the above made me wonder, due to lack of any\n> signed-off-by before improved-by, what \"base\" was improved by JTan.\n> If you were writing a change before formally passing it around with\n> your sign-off and somebody had a valuable input to improve it, it\n> seems that people say helped-by around here.\n\nYep, I should have put the Improved-by after my sign-off.\n\nJonathan Tan's contribution was at\nhttps://public-inbox.org/git/20171023151929.67165aea67353e5c24a15229@google.com/\n\n[...]\n>> +The config variable `ssh.variant` can be set to override this detection.\n>> +Valid values are `ssh` (to use OpenSSH options), `plink`, `putty`,\n>> +`tortoiseplink`, `simple` (no options except the host and remote command).\n>> +The default auto-detection can be explicitly requested using the value\n>> +`auto`. Any other value is treated as `ssh`. This setting can also be\n>> +overridden via the environment variable `GIT_SSH_VARIANT`.\n>> +\n>\n> Cleanly written and easily read. Good.\n\ni.e. that part is all thanks to him. :)\n\nJonathan\n"},{"id":"333198","messageId":"CAGZ79ka6HVtF10K9_t30E0bjRzc+Ac-GZB99fqoU5CVvUjooug@mail.gmail.com","threadId":"47257","inReplyTo":"20171121014919.p4cl5esf7evptzbk@aiede.mtv.corp.google.com","subject":"Re: [PATCH 1/8 v2] ssh test: make copy_ssh_wrapper_as clean up after itself","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-11-21T23:42:44Z","receivedAt":"2017-11-21T23:42:51Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Mon, Nov 20, 2017 at 5:49 PM, Jonathan Nieder wrote:\n\n> @@ -317,7 +317,7 @@ test_expect_success 'set up ssh wrapper' '\n>\n> copy_ssh_wrapper_as () {\n> cp \"$TRASH_DIRECTORY/ssh$X\" \"${1%$X}$X\" &&\n> - test_when_finished \"rm -f ${1%$X}$X\" &&\n> + test_when_finished \"rm $(git rev-parse --sq-quote \"${1%$X}$X\")\" &&\n\nI wondered why the line above doesn't need the same treatment, but there\nthe argument is quoted, in this line we cannot use quotation as we are already\nusing it for bundle up the argument for test_when_finished.\n\nThe patch looks good.\nStefan\n"},{"id":"333201","messageId":"CAGZ79kaR3Jk++mQKhJVk9dK8_usLGWTAgEdkF1K061TKwiFv5w@mail.gmail.com","threadId":"47257","inReplyTo":"20171120223232.GG92506@google.com","subject":"Re: [PATCH v3 0/8] Coping with unrecognized ssh wrapper scripts in GIT_SSH","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-11-22T00:00:40Z","receivedAt":"2017-11-22T00:00:50Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Mon, Nov 20, 2017 at 2:32 PM, Brandon Williams wrote:\n> On 11/20, Jonathan Nieder wrote:\n>> Previously: [1].\n>>\n>> This version should be essentially identical to v2. Changes:\n>> - patch 1 is new and should fix the test failure on Windows\n>> - patch 2 is new, discussed at [2]\n>> - patch 5 split off from patch 6 as suggested at [3]\n>> - patch 6 commit message got two new notes to address the worries\n>> from [3]\n>>\n>> Thanks for the helpful reviews, and sorry to take so long to get this\n>> out. Thoughts of all kinds welcome, as always.\n>\n> Just finished looking through the series. Looks good overall!\n>\n> Thanks again for getting this out!\n\nSame here,\n\nThanks,\nStefan\n"},{"id":"333218","messageId":"xmqqmv3fxep9.fsf@gitster.mtv.corp.google.com","threadId":"47257","inReplyTo":"20171120212134.lh2l4drdzu6fh5g2@aiede.mtv.corp.google.com","subject":"Re: [PATCH v3 0/8] Coping with unrecognized ssh wrapper scripts in GIT_SSH","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-11-22T01:52:18Z","receivedAt":"2017-11-22T01:52:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n> Jonathan Nieder (8):\n> ssh test: make copy_ssh_wrapper_as clean up after itself\n> connect: move no_fork fallback to git_tcp_connect\n> connect: split git:// setup into a separate function\n> connect: split ssh command line options into separate function\n> connect: split ssh option computation to its own function\n> ssh: 'auto' variant to select between 'ssh' and 'simple'\n> ssh: 'simple' variant does not support -4/-6\n> ssh: 'simple' variant does not support --port\n\nThanks. All looked sensible. With this, we can unblock both topics\n;-)\n"}]}