From 8339805b467ca5b2d9314fdbfdd75a6e96c6b39a Mon Sep 17 00:00:00 2001 From: Jonathan Nieder Date: Mon, 20 Nov 2017 17:49:19 -0800 Subject: ssh test: make copy_ssh_wrapper_as clean up after itself Simplify by not allowing the copied ssh wrapper to persist between tests. This way, tests can be safely reordered, added, and removed with less fear of hidden side effects. This also avoids having to call setup_ssh_wrapper to restore the value of GIT_SSH after this battery of tests, since it means each test will restore it individually. Noticed because on Windows, if `uplink.exe` exists, the MSYS2 Bash will overwrite that when redirecting via `>uplink`. A proposed test wrote a script to 'uplink' after a previous test created uplink.exe using copy_ssh_wrapper_as, so the script written with '>uplink' had the wrong filename and failed. Reported-by: Johannes Schindelin Signed-off-by: Jonathan Nieder Acked-by: Brandon Williams Signed-off-by: Junio C Hamano --- t/t5601-clone.sh | 28 ++++++++++++---------------- 1 file changed, 12 insertions(+), 16 deletions(-) diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh index 86811a0c35..4a16a0b7dd 100755 --- a/t/t5601-clone.sh +++ b/t/t5601-clone.sh @@ -306,21 +306,20 @@ test_expect_success 'clone checking out a tag' ' test_cmp fetch.expected fetch.actual ' -setup_ssh_wrapper () { - test_expect_success 'setup ssh wrapper' ' - cp "$GIT_BUILD_DIR/t/helper/test-fake-ssh$X" \ - "$TRASH_DIRECTORY/ssh$X" && - GIT_SSH="$TRASH_DIRECTORY/ssh$X" && - export GIT_SSH && - export TRASH_DIRECTORY && - >"$TRASH_DIRECTORY"/ssh-output - ' -} +test_expect_success 'set up ssh wrapper' ' + cp "$GIT_BUILD_DIR/t/helper/test-fake-ssh$X" \ + "$TRASH_DIRECTORY/ssh$X" && + GIT_SSH="$TRASH_DIRECTORY/ssh$X" && + export GIT_SSH && + export TRASH_DIRECTORY && + >"$TRASH_DIRECTORY"/ssh-output +' copy_ssh_wrapper_as () { cp "$TRASH_DIRECTORY/ssh$X" "${1%$X}$X" && + test_when_finished "rm $(git rev-parse --sq-quote "${1%$X}$X")" && GIT_SSH="${1%$X}$X" && - export GIT_SSH + test_when_finished "GIT_SSH=\"\$TRASH_DIRECTORY/ssh\$X\"" } expect_ssh () { @@ -344,8 +343,6 @@ expect_ssh () { (cd "$TRASH_DIRECTORY" && test_cmp ssh-expect ssh-output) } -setup_ssh_wrapper - test_expect_success 'clone myhost:src uses ssh' ' git clone myhost:src ssh-clone && expect_ssh myhost src @@ -432,12 +429,14 @@ test_expect_success 'ssh.variant overrides plink detection' ' ' test_expect_success 'GIT_SSH_VARIANT overrides plink detection to plink' ' + copy_ssh_wrapper_as "$TRASH_DIRECTORY/plink" && GIT_SSH_VARIANT=plink \ git clone "[myhost:123]:src" ssh-bracket-clone-variant-3 && expect_ssh "-P 123" myhost src ' test_expect_success 'GIT_SSH_VARIANT overrides plink to tortoiseplink' ' + copy_ssh_wrapper_as "$TRASH_DIRECTORY/plink" && GIT_SSH_VARIANT=tortoiseplink \ git clone "[myhost:123]:src" ssh-bracket-clone-variant-4 && expect_ssh "-batch -P 123" myhost src @@ -449,9 +448,6 @@ test_expect_success 'clean failure on broken quoting' ' git clone "[myhost:123]:src" sq-failure ' -# Reset the GIT_SSH environment variable for clone tests. -setup_ssh_wrapper - counter=0 # $1 url # $2 none|host -- cgit v1.2.1 From 8e349780ecb9bcec52c7df22fcbfb4afd0d7936c Mon Sep 17 00:00:00 2001 From: Jonathan Nieder Date: Mon, 20 Nov 2017 13:22:54 -0800 Subject: connect: move no_fork fallback to git_tcp_connect git_connect has the structure struct child_process *conn = &no_fork; ... switch (protocol) { case PROTO_GIT: if (git_use_proxy(hostandport)) conn = git_proxy_connect(fd, hostandport); else git_tcp_connect(fd, hostandport, flags); ... break; case PROTO_SSH: conn = xmalloc(sizeof(*conn)); child_process_init(conn); argv_array_push(&conn->args, ssh); ... break; ... return conn; In all cases except the git_tcp_connect case, conn is explicitly assigned a value. Make the code clearer by explicitly assigning 'conn = &no_fork' in the tcp case and eliminating the default so the compiler can ensure conn is always correctly assigned. Noticed-by: Junio C Hamano Signed-off-by: Jonathan Nieder Signed-off-by: Junio C Hamano --- connect.c | 36 +++++++++++++++++++++--------------- 1 file changed, 21 insertions(+), 15 deletions(-) diff --git a/connect.c b/connect.c index 7fbd396b35..aa994d1518 100644 --- a/connect.c +++ b/connect.c @@ -582,12 +582,25 @@ static int git_tcp_connect_sock(char *host, int flags) #endif /* NO_IPV6 */ -static void git_tcp_connect(int fd[2], char *host, int flags) +/* + * Dummy child_process returned by git_connect() if the transport protocol + * does not need fork(2). + */ +static struct child_process no_fork = CHILD_PROCESS_INIT; + +int git_connection_is_socket(struct child_process *conn) +{ + return conn == &no_fork; +} + +static struct child_process *git_tcp_connect(int fd[2], char *host, int flags) { int sockfd = git_tcp_connect_sock(host, flags); fd[0] = sockfd; fd[1] = dup(sockfd); + + return &no_fork; } @@ -761,8 +774,6 @@ static enum protocol parse_connect_url(const char *url_orig, char **ret_host, return protocol; } -static struct child_process no_fork = CHILD_PROCESS_INIT; - static const char *get_ssh_command(void) { const char *ssh; @@ -851,11 +862,11 @@ static enum ssh_variant determine_ssh_variant(const char *ssh_command, } /* - * This returns a dummy child_process if the transport protocol does not - * need fork(2), or a struct child_process object if it does. Once done, - * finish the connection with finish_connect() with the value returned from - * this function (it is safe to call finish_connect() with NULL to support - * the former case). + * This returns the dummy child_process `no_fork` if the transport protocol + * does not need fork(2), or a struct child_process object if it does. Once + * done, finish the connection with finish_connect() with the value returned + * from this function (it is safe to call finish_connect() with NULL to + * support the former case). * * If it returns, the connect is successful; it just dies on errors (this * will hopefully be changed in a libification effort, to return NULL when @@ -865,7 +876,7 @@ struct child_process *git_connect(int fd[2], const char *url, const char *prog, int flags) { char *hostandport, *path; - struct child_process *conn = &no_fork; + struct child_process *conn; enum protocol protocol; /* Without this we cannot rely on waitpid() to tell @@ -901,7 +912,7 @@ struct child_process *git_connect(int fd[2], const char *url, if (git_use_proxy(hostandport)) conn = git_proxy_connect(fd, hostandport); else - git_tcp_connect(fd, hostandport, flags); + conn = git_tcp_connect(fd, hostandport, flags); /* * Separate original protocol components prog and path * from extended host header with a NUL byte. @@ -1041,11 +1052,6 @@ struct child_process *git_connect(int fd[2], const char *url, return conn; } -int git_connection_is_socket(struct child_process *conn) -{ - return conn == &no_fork; -} - int finish_connect(struct child_process *conn) { int code; -- cgit v1.2.1 From 2ac67cb63b715989657cee97b3181455b1380b3f Mon Sep 17 00:00:00 2001 From: Jonathan Nieder Date: Mon, 20 Nov 2017 13:23:27 -0800 Subject: connect: split git:// setup into a separate function The git_connect function is growing long. Split the PROTO_GIT-specific portion to a separate function to make it easier to read. No functional change intended. Signed-off-by: Jonathan Nieder Reviewed-by: Stefan Beller Signed-off-by: Junio C Hamano --- connect.c | 103 +++++++++++++++++++++++++++++++++++--------------------------- 1 file changed, 59 insertions(+), 44 deletions(-) diff --git a/connect.c b/connect.c index aa994d1518..9425229206 100644 --- a/connect.c +++ b/connect.c @@ -861,6 +861,64 @@ static enum ssh_variant determine_ssh_variant(const char *ssh_command, return ssh_variant; } +/* + * Open a connection using Git's native protocol. + * + * The caller is responsible for freeing hostandport, but this function may + * modify it (for example, to truncate it to remove the port part). + */ +static struct child_process *git_connect_git(int fd[2], char *hostandport, + const char *path, const char *prog, + int flags) +{ + struct child_process *conn; + struct strbuf request = STRBUF_INIT; + /* + * Set up virtual host information based on where we will + * connect, unless the user has overridden us in + * the environment. + */ + char *target_host = getenv("GIT_OVERRIDE_VIRTUAL_HOST"); + if (target_host) + target_host = xstrdup(target_host); + else + target_host = xstrdup(hostandport); + + transport_check_allowed("git"); + + /* These underlying connection commands die() if they + * cannot connect. + */ + if (git_use_proxy(hostandport)) + conn = git_proxy_connect(fd, hostandport); + else + conn = git_tcp_connect(fd, hostandport, flags); + /* + * Separate original protocol components prog and path + * from extended host header with a NUL byte. + * + * Note: Do not add any other headers here! Doing so + * will cause older git-daemon servers to crash. + */ + strbuf_addf(&request, + "%s %s%chost=%s%c", + prog, path, 0, + target_host, 0); + + /* If using a new version put that stuff here after a second null byte */ + if (get_protocol_version_config() > 0) { + strbuf_addch(&request, '\0'); + strbuf_addf(&request, "version=%d%c", + get_protocol_version_config(), '\0'); + } + + packet_write(fd[1], request.buf, request.len); + + free(target_host); + strbuf_release(&request); + return conn; +} + /* * This returns the dummy child_process `no_fork` if the transport protocol * does not need fork(2), or a struct child_process object if it does. Once @@ -892,50 +950,7 @@ struct child_process *git_connect(int fd[2], const char *url, printf("Diag: path=%s\n", path ? path : "NULL"); conn = NULL; } else if (protocol == PROTO_GIT) { - struct strbuf request = STRBUF_INIT; - /* - * Set up virtual host information based on where we will - * connect, unless the user has overridden us in - * the environment. - */ - char *target_host = getenv("GIT_OVERRIDE_VIRTUAL_HOST"); - if (target_host) - target_host = xstrdup(target_host); - else - target_host = xstrdup(hostandport); - - transport_check_allowed("git"); - - /* These underlying connection commands die() if they - * cannot connect. - */ - if (git_use_proxy(hostandport)) - conn = git_proxy_connect(fd, hostandport); - else - conn = git_tcp_connect(fd, hostandport, flags); - /* - * Separate original protocol components prog and path - * from extended host header with a NUL byte. - * - * Note: Do not add any other headers here! Doing so - * will cause older git-daemon servers to crash. - */ - strbuf_addf(&request, - "%s %s%chost=%s%c", - prog, path, 0, - target_host, 0); - - /* If using a new version put that stuff here after a second null byte */ - if (get_protocol_version_config() > 0) { - strbuf_addch(&request, '\0'); - strbuf_addf(&request, "version=%d%c", - get_protocol_version_config(), '\0'); - } - - packet_write(fd[1], request.buf, request.len); - - free(target_host); - strbuf_release(&request); + conn = git_connect_git(fd, hostandport, path, prog, flags); } else { struct strbuf cmd = STRBUF_INIT; const char *const *var; -- cgit v1.2.1 From fce54ce422befdeb69d94f7576d3c23e94e41572 Mon Sep 17 00:00:00 2001 From: Jonathan Nieder Date: Mon, 20 Nov 2017 14:19:43 -0800 Subject: connect: split ssh command line options into separate function The git_connect function is growing long. Split the portion that discovers an ssh command and options it accepts before the service name and path to a separate function to make it easier to read. No functional change intended. Signed-off-by: Jonathan Nieder Reviewed-by: Stefan Beller Signed-off-by: Junio C Hamano --- connect.c | 113 +++++++++++++++++++++++++++++++++----------------------------- 1 file changed, 60 insertions(+), 53 deletions(-) diff --git a/connect.c b/connect.c index 9425229206..2113feb4f8 100644 --- a/connect.c +++ b/connect.c @@ -919,6 +919,65 @@ static struct child_process *git_connect_git(int fd[2], char *hostandport, return conn; } +/* Prepare a child_process for use by Git's SSH-tunneled transport. */ +static void fill_ssh_args(struct child_process *conn, const char *ssh_host, + const char *port, int flags) +{ + const char *ssh; + enum ssh_variant variant; + + if (looks_like_command_line_option(ssh_host)) + die("strange hostname '%s' blocked", ssh_host); + + ssh = get_ssh_command(); + if (ssh) { + variant = determine_ssh_variant(ssh, 1); + } else { + /* + * GIT_SSH is the no-shell version of + * GIT_SSH_COMMAND (and must remain so for + * historical compatibility). + */ + conn->use_shell = 0; + + ssh = getenv("GIT_SSH"); + if (!ssh) + ssh = "ssh"; + variant = determine_ssh_variant(ssh, 0); + } + + argv_array_push(&conn->args, ssh); + + if (variant == VARIANT_SSH && + get_protocol_version_config() > 0) { + argv_array_push(&conn->args, "-o"); + argv_array_push(&conn->args, "SendEnv=" GIT_PROTOCOL_ENVIRONMENT); + argv_array_pushf(&conn->env_array, GIT_PROTOCOL_ENVIRONMENT "=version=%d", + get_protocol_version_config()); + } + + if (variant != VARIANT_SIMPLE) { + if (flags & CONNECT_IPV4) + argv_array_push(&conn->args, "-4"); + else if (flags & CONNECT_IPV6) + argv_array_push(&conn->args, "-6"); + } + + if (variant == VARIANT_TORTOISEPLINK) + argv_array_push(&conn->args, "-batch"); + + if (port && variant != VARIANT_SIMPLE) { + if (variant == VARIANT_SSH) + argv_array_push(&conn->args, "-p"); + else + argv_array_push(&conn->args, "-P"); + + argv_array_push(&conn->args, port); + } + + argv_array_push(&conn->args, ssh_host); +} + /* * This returns the dummy child_process `no_fork` if the transport protocol * does not need fork(2), or a struct child_process object if it does. Once @@ -972,8 +1031,6 @@ struct child_process *git_connect(int fd[2], const char *url, conn->use_shell = 1; conn->in = conn->out = -1; if (protocol == PROTO_SSH) { - const char *ssh; - enum ssh_variant variant; char *ssh_host = hostandport; const char *port = NULL; transport_check_allowed("ssh"); @@ -995,57 +1052,7 @@ struct child_process *git_connect(int fd[2], const char *url, strbuf_release(&cmd); return NULL; } - - if (looks_like_command_line_option(ssh_host)) - die("strange hostname '%s' blocked", ssh_host); - - ssh = get_ssh_command(); - if (ssh) { - variant = determine_ssh_variant(ssh, 1); - } else { - /* - * GIT_SSH is the no-shell version of - * GIT_SSH_COMMAND (and must remain so for - * historical compatibility). - */ - conn->use_shell = 0; - - ssh = getenv("GIT_SSH"); - if (!ssh) - ssh = "ssh"; - variant = determine_ssh_variant(ssh, 0); - } - - argv_array_push(&conn->args, ssh); - - if (variant == VARIANT_SSH && - get_protocol_version_config() > 0) { - argv_array_push(&conn->args, "-o"); - argv_array_push(&conn->args, "SendEnv=" GIT_PROTOCOL_ENVIRONMENT); - argv_array_pushf(&conn->env_array, GIT_PROTOCOL_ENVIRONMENT "=version=%d", - get_protocol_version_config()); - } - - if (variant != VARIANT_SIMPLE) { - if (flags & CONNECT_IPV4) - argv_array_push(&conn->args, "-4"); - else if (flags & CONNECT_IPV6) - argv_array_push(&conn->args, "-6"); - } - - if (variant == VARIANT_TORTOISEPLINK) - argv_array_push(&conn->args, "-batch"); - - if (port && variant != VARIANT_SIMPLE) { - if (variant == VARIANT_SSH) - argv_array_push(&conn->args, "-p"); - else - argv_array_push(&conn->args, "-P"); - - argv_array_push(&conn->args, port); - } - - argv_array_push(&conn->args, ssh_host); + fill_ssh_args(conn, ssh_host, port, flags); } else { transport_check_allowed("file"); if (get_protocol_version_config() > 0) { -- cgit v1.2.1 From 957e2ad28290076fffe3bf28ae8609c637cf8151 Mon Sep 17 00:00:00 2001 From: Jonathan Nieder Date: Mon, 20 Nov 2017 13:26:19 -0800 Subject: connect: split ssh option computation to its own function This puts the determination of options to pass to each ssh variant (see ssh.variant in git-config(1)) in one place. A follow-up patch will use this in an initial dry run to detect which variant to use when the ssh command is ambiguous. No functional change intended yet. Signed-off-by: Jonathan Nieder Signed-off-by: Junio C Hamano --- connect.c | 65 ++++++++++++++++++++++++++++++++++++--------------------------- 1 file changed, 37 insertions(+), 28 deletions(-) diff --git a/connect.c b/connect.c index 2113feb4f8..f3370510a6 100644 --- a/connect.c +++ b/connect.c @@ -919,6 +919,42 @@ static struct child_process *git_connect_git(int fd[2], char *hostandport, return conn; } +/* + * Append the appropriate environment variables to `env` and options to + * `args` for running ssh in Git's SSH-tunneled transport. + */ +static void push_ssh_options(struct argv_array *args, struct argv_array *env, + enum ssh_variant variant, const char *port, + int flags) +{ + if (variant == VARIANT_SSH && + get_protocol_version_config() > 0) { + argv_array_push(args, "-o"); + argv_array_push(args, "SendEnv=" GIT_PROTOCOL_ENVIRONMENT); + argv_array_pushf(env, GIT_PROTOCOL_ENVIRONMENT "=version=%d", + get_protocol_version_config()); + } + + if (variant != VARIANT_SIMPLE) { + if (flags & CONNECT_IPV4) + argv_array_push(args, "-4"); + else if (flags & CONNECT_IPV6) + argv_array_push(args, "-6"); + } + + if (variant == VARIANT_TORTOISEPLINK) + argv_array_push(args, "-batch"); + + if (port && variant != VARIANT_SIMPLE) { + if (variant == VARIANT_SSH) + argv_array_push(args, "-p"); + else + argv_array_push(args, "-P"); + + argv_array_push(args, port); + } +} + /* Prepare a child_process for use by Git's SSH-tunneled transport. */ static void fill_ssh_args(struct child_process *conn, const char *ssh_host, const char *port, int flags) @@ -947,34 +983,7 @@ static void fill_ssh_args(struct child_process *conn, const char *ssh_host, } argv_array_push(&conn->args, ssh); - - if (variant == VARIANT_SSH && - get_protocol_version_config() > 0) { - argv_array_push(&conn->args, "-o"); - argv_array_push(&conn->args, "SendEnv=" GIT_PROTOCOL_ENVIRONMENT); - argv_array_pushf(&conn->env_array, GIT_PROTOCOL_ENVIRONMENT "=version=%d", - get_protocol_version_config()); - } - - if (variant != VARIANT_SIMPLE) { - if (flags & CONNECT_IPV4) - argv_array_push(&conn->args, "-4"); - else if (flags & CONNECT_IPV6) - argv_array_push(&conn->args, "-6"); - } - - if (variant == VARIANT_TORTOISEPLINK) - argv_array_push(&conn->args, "-batch"); - - if (port && variant != VARIANT_SIMPLE) { - if (variant == VARIANT_SSH) - argv_array_push(&conn->args, "-p"); - else - argv_array_push(&conn->args, "-P"); - - argv_array_push(&conn->args, port); - } - + push_ssh_options(&conn->args, &conn->env_array, variant, port, flags); argv_array_push(&conn->args, ssh_host); } -- cgit v1.2.1 From 0da0e49ba12225684b75e86a4c9344ad121652cb Mon Sep 17 00:00:00 2001 From: Jonathan Nieder Date: Mon, 20 Nov 2017 13:30:04 -0800 Subject: ssh: 'auto' variant to select between 'ssh' and 'simple' Android's "repo" tool is a tool for managing a large codebase consisting of multiple smaller repositories, similar to Git's submodule feature. Starting with Git 94b8ae5a (ssh: introduce a 'simple' ssh variant, 2017-10-16), users noticed that it stopped handling the port in ssh:// URLs. The cause: when it encounters ssh:// URLs, repo pre-connects to the server and sets GIT_SSH to a helper ".repo/repo/git_ssh" that reuses that connection. Before 94b8ae5a, the helper was assumed to support OpenSSH options for lack of a better guess and got passed a -p option to set the port. After that patch, it uses the new default of a simple helper that does not accept an option to set the port. The next release of "repo" will set GIT_SSH_VARIANT to "ssh" to avoid that. But users of old versions and of other similar GIT_SSH implementations would not get the benefit of that fix. So update the default to use OpenSSH options again, with a twist. As observed in 94b8ae5a, we cannot assume that $GIT_SSH always handles OpenSSH options: common helpers such as travis-ci's dpl[*] are configured using GIT_SSH and do not accept OpenSSH options. So make the default a new variant "auto", with the following behavior: 1. First, check for a recognized basename, like today. 2. If the basename is not recognized, check whether $GIT_SSH supports OpenSSH options by running $GIT_SSH -G This returns status 0 and prints configuration in OpenSSH if it recognizes all and returns status 255 if it encounters an unrecognized option. A wrapper script like exec ssh -- "$@" would fail with ssh: Could not resolve hostname -g: Name or service not known , correctly reflecting that it does not support OpenSSH options. The command is run with stdin, stdout, and stderr redirected to /dev/null so even a command that expects a terminal would exit immediately. 3. Based on the result from step (2), behave like "ssh" (if it succeeded) or "simple" (if it failed). This way, the default ssh variant for unrecognized commands can handle both the repo and dpl cases as intended. This autodetection has been running on Google workstations since 2017-10-23 with no reported negative effects. [*] https://github.com/travis-ci/dpl/blob/6c3fddfda1f2a85944c544446b068bac0a77c049/lib/dpl/provider.rb#L215 Reported-by: William Yan Improved-by: Jonathan Tan Signed-off-by: Jonathan Nieder Signed-off-by: Junio C Hamano --- Documentation/config.txt | 26 ++++++++++++++++---------- connect.c | 32 +++++++++++++++++++++++++------- t/t5601-clone.sh | 21 +++++++++++++++++++++ 3 files changed, 62 insertions(+), 17 deletions(-) diff --git a/Documentation/config.txt b/Documentation/config.txt index 0460af37e2..0c371ad786 100644 --- a/Documentation/config.txt +++ b/Documentation/config.txt @@ -2081,16 +2081,22 @@ matched against are those given directly to Git commands. This means any URLs visited as a result of a redirection do not participate in matching. ssh.variant:: - Depending on the value of the environment variables `GIT_SSH` or - `GIT_SSH_COMMAND`, or the config setting `core.sshCommand`, Git - auto-detects whether to adjust its command-line parameters for use - with ssh (OpenSSH), plink or tortoiseplink, as opposed to the default - (simple). -+ -The config variable `ssh.variant` can be set to override this auto-detection; -valid values are `ssh`, `simple`, `plink`, `putty` or `tortoiseplink`. Any -other value will be treated as normal ssh. This setting can be overridden via -the environment variable `GIT_SSH_VARIANT`. + By default, Git determines the command line arguments to use + based on the basename of the configured SSH command (configured + using the environment variable `GIT_SSH` or `GIT_SSH_COMMAND` or + the config setting `core.sshCommand`). If the basename is + unrecognized, Git will attempt to detect support of OpenSSH + options by first invoking the configured SSH command with the + `-G` (print configuration) option and will subsequently use + OpenSSH options (if that is successful) or no options besides + the host and remote command (if it fails). ++ +The config variable `ssh.variant` can be set to override this detection. +Valid values are `ssh` (to use OpenSSH options), `plink`, `putty`, +`tortoiseplink`, `simple` (no options except the host and remote command). +The default auto-detection can be explicitly requested using the value +`auto`. Any other value is treated as `ssh`. This setting can also be +overridden via the environment variable `GIT_SSH_VARIANT`. + The current command-line parameters used for each variant are as follows: diff --git a/connect.c b/connect.c index f3370510a6..a939323d05 100644 --- a/connect.c +++ b/connect.c @@ -788,6 +788,7 @@ static const char *get_ssh_command(void) } enum ssh_variant { + VARIANT_AUTO, VARIANT_SIMPLE, VARIANT_SSH, VARIANT_PLINK, @@ -795,14 +796,16 @@ enum ssh_variant { VARIANT_TORTOISEPLINK, }; -static int override_ssh_variant(enum ssh_variant *ssh_variant) +static void override_ssh_variant(enum ssh_variant *ssh_variant) { const char *variant = getenv("GIT_SSH_VARIANT"); if (!variant && git_config_get_string_const("ssh.variant", &variant)) - return 0; + return; - if (!strcmp(variant, "plink")) + if (!strcmp(variant, "auto")) + *ssh_variant = VARIANT_AUTO; + else if (!strcmp(variant, "plink")) *ssh_variant = VARIANT_PLINK; else if (!strcmp(variant, "putty")) *ssh_variant = VARIANT_PUTTY; @@ -812,18 +815,18 @@ static int override_ssh_variant(enum ssh_variant *ssh_variant) *ssh_variant = VARIANT_SIMPLE; else *ssh_variant = VARIANT_SSH; - - return 1; } static enum ssh_variant determine_ssh_variant(const char *ssh_command, int is_cmdline) { - enum ssh_variant ssh_variant = VARIANT_SIMPLE; + enum ssh_variant ssh_variant = VARIANT_AUTO; const char *variant; char *p = NULL; - if (override_ssh_variant(&ssh_variant)) + override_ssh_variant(&ssh_variant); + + if (ssh_variant != VARIANT_AUTO) return ssh_variant; if (!is_cmdline) { @@ -982,6 +985,21 @@ static void fill_ssh_args(struct child_process *conn, const char *ssh_host, variant = determine_ssh_variant(ssh, 0); } + if (variant == VARIANT_AUTO) { + struct child_process detect = CHILD_PROCESS_INIT; + + detect.use_shell = conn->use_shell; + detect.no_stdin = detect.no_stdout = detect.no_stderr = 1; + + argv_array_push(&detect.args, ssh); + argv_array_push(&detect.args, "-G"); + push_ssh_options(&detect.args, &detect.env_array, + VARIANT_SSH, port, flags); + argv_array_push(&detect.args, ssh_host); + + variant = run_command(&detect) ? VARIANT_SIMPLE : VARIANT_SSH; + } + argv_array_push(&conn->args, ssh); push_ssh_options(&conn->args, &conn->env_array, variant, port, flags); argv_array_push(&conn->args, ssh_host); diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh index 4a16a0b7dd..d96b8e7379 100755 --- a/t/t5601-clone.sh +++ b/t/t5601-clone.sh @@ -369,6 +369,12 @@ test_expect_success 'variant can be overriden' ' expect_ssh myhost src ' +test_expect_success 'variant=auto picks based on basename' ' + copy_ssh_wrapper_as "$TRASH_DIRECTORY/plink" && + git -c ssh.variant=auto clone -4 "[myhost:123]:src" ssh-auto-clone && + expect_ssh "-4 -P 123" myhost src +' + test_expect_success 'simple is treated as simple' ' copy_ssh_wrapper_as "$TRASH_DIRECTORY/simple" && git clone -4 "[myhost:123]:src" ssh-bracket-clone-simple && @@ -381,6 +387,21 @@ test_expect_success 'uplink is treated as simple' ' expect_ssh myhost src ' +test_expect_success 'OpenSSH-like uplink is treated as ssh' ' + write_script "$TRASH_DIRECTORY/uplink" <<-EOF && + if test "\$1" = "-G" + then + exit 0 + fi && + exec "\$TRASH_DIRECTORY/ssh$X" "\$@" + EOF + test_when_finished "rm -f \"\$TRASH_DIRECTORY/uplink\"" && + GIT_SSH="$TRASH_DIRECTORY/uplink" && + test_when_finished "GIT_SSH=\"\$TRASH_DIRECTORY/ssh\$X\"" && + git clone "[myhost:123]:src" ssh-bracket-clone-sshlike-uplink && + expect_ssh "-p 123" myhost src +' + test_expect_success 'plink is treated specially (as putty)' ' copy_ssh_wrapper_as "$TRASH_DIRECTORY/plink" && git clone "[myhost:123]:src" ssh-bracket-clone-plink-0 && -- cgit v1.2.1 From a3f5b66fac3b5f3b2c352c8086dbc3d476f9e3d4 Mon Sep 17 00:00:00 2001 From: Jonathan Nieder Date: Mon, 20 Nov 2017 13:30:30 -0800 Subject: ssh: 'simple' variant does not support -4/-6 If the user passes -4/--ipv4 or -6/--ipv6 to "git fetch" or "git push" and the ssh command configured with GIT_SSH does not support such a setting, error out instead of ignoring the option and continuing. Signed-off-by: Jonathan Nieder Acked-by: Stefan Beller Signed-off-by: Junio C Hamano --- connect.c | 25 ++++++++++++++++++++++--- t/t5601-clone.sh | 12 ++++++------ 2 files changed, 28 insertions(+), 9 deletions(-) diff --git a/connect.c b/connect.c index a939323d05..c3d1cd3787 100644 --- a/connect.c +++ b/connect.c @@ -938,11 +938,30 @@ static void push_ssh_options(struct argv_array *args, struct argv_array *env, get_protocol_version_config()); } - if (variant != VARIANT_SIMPLE) { - if (flags & CONNECT_IPV4) + if (flags & CONNECT_IPV4) { + switch (variant) { + case VARIANT_AUTO: + BUG("VARIANT_AUTO passed to push_ssh_options"); + case VARIANT_SIMPLE: + die("ssh variant 'simple' does not support -4"); + case VARIANT_SSH: + case VARIANT_PLINK: + case VARIANT_PUTTY: + case VARIANT_TORTOISEPLINK: argv_array_push(args, "-4"); - else if (flags & CONNECT_IPV6) + } + } else if (flags & CONNECT_IPV6) { + switch (variant) { + case VARIANT_AUTO: + BUG("VARIANT_AUTO passed to push_ssh_options"); + case VARIANT_SIMPLE: + die("ssh variant 'simple' does not support -6"); + case VARIANT_SSH: + case VARIANT_PLINK: + case VARIANT_PUTTY: + case VARIANT_TORTOISEPLINK: argv_array_push(args, "-6"); + } } if (variant == VARIANT_TORTOISEPLINK) diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh index d96b8e7379..9ae1606ee5 100755 --- a/t/t5601-clone.sh +++ b/t/t5601-clone.sh @@ -364,9 +364,10 @@ test_expect_success 'OpenSSH variant passes -4' ' expect_ssh "-4 -p 123" myhost src ' -test_expect_success 'variant can be overriden' ' - git -c ssh.variant=simple clone -4 "[myhost:123]:src" ssh-simple-clone && - expect_ssh myhost src +test_expect_success 'variant can be overridden' ' + copy_ssh_wrapper_as "$TRASH_DIRECTORY/putty" && + git -c ssh.variant=putty clone -4 "[myhost:123]:src" ssh-putty-clone && + expect_ssh "-4 -P 123" myhost src ' test_expect_success 'variant=auto picks based on basename' ' @@ -375,10 +376,9 @@ test_expect_success 'variant=auto picks based on basename' ' expect_ssh "-4 -P 123" myhost src ' -test_expect_success 'simple is treated as simple' ' +test_expect_success 'simple does not support -4/-6' ' copy_ssh_wrapper_as "$TRASH_DIRECTORY/simple" && - git clone -4 "[myhost:123]:src" ssh-bracket-clone-simple && - expect_ssh myhost src + test_must_fail git clone -4 "[myhost:123]:src" ssh-bracket-clone-simple ' test_expect_success 'uplink is treated as simple' ' -- cgit v1.2.1 From 3fa5e0d07a979dfd1a1095a9dda4904c62189e00 Mon Sep 17 00:00:00 2001 From: Jonathan Nieder Date: Mon, 20 Nov 2017 13:31:01 -0800 Subject: ssh: 'simple' variant does not support --port When trying to connect to an ssh:// URL with port explicitly specified and the ssh command configured with GIT_SSH does not support such a setting, it is less confusing to error out than to silently suppress the port setting and continue. This requires updating the GIT_SSH setting in t5603-clone-dirname.sh. That test is about the directory name produced when cloning various URLs. It uses an ssh wrapper that ignores all its arguments but does not declare that it supports a port argument; update it to set GIT_SSH_VARIANT=ssh to do so. (Real-life ssh wrappers that pass a port argument to OpenSSH would also support -G and would not require such an update.) Reported-by: William Yan Signed-off-by: Jonathan Nieder Acked-by: Stefan Beller Signed-off-by: Junio C Hamano --- connect.c | 15 ++++++++++++--- t/t5601-clone.sh | 10 ++++++++-- t/t5603-clone-dirname.sh | 2 ++ 3 files changed, 22 insertions(+), 5 deletions(-) diff --git a/connect.c b/connect.c index c3d1cd3787..d6033861b3 100644 --- a/connect.c +++ b/connect.c @@ -967,11 +967,20 @@ static void push_ssh_options(struct argv_array *args, struct argv_array *env, if (variant == VARIANT_TORTOISEPLINK) argv_array_push(args, "-batch"); - if (port && variant != VARIANT_SIMPLE) { - if (variant == VARIANT_SSH) + if (port) { + switch (variant) { + case VARIANT_AUTO: + BUG("VARIANT_AUTO passed to push_ssh_options"); + case VARIANT_SIMPLE: + die("ssh variant 'simple' does not support setting port"); + case VARIANT_SSH: argv_array_push(args, "-p"); - else + break; + case VARIANT_PLINK: + case VARIANT_PUTTY: + case VARIANT_TORTOISEPLINK: argv_array_push(args, "-P"); + } argv_array_push(args, port); } diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh index 9ae1606ee5..66784fc8ff 100755 --- a/t/t5601-clone.sh +++ b/t/t5601-clone.sh @@ -378,12 +378,18 @@ test_expect_success 'variant=auto picks based on basename' ' test_expect_success 'simple does not support -4/-6' ' copy_ssh_wrapper_as "$TRASH_DIRECTORY/simple" && - test_must_fail git clone -4 "[myhost:123]:src" ssh-bracket-clone-simple + test_must_fail git clone -4 "myhost:src" ssh-4-clone-simple +' + +test_expect_success 'simple does not support port' ' + copy_ssh_wrapper_as "$TRASH_DIRECTORY/simple" && + test_must_fail git clone "[myhost:123]:src" ssh-bracket-clone-simple ' test_expect_success 'uplink is treated as simple' ' copy_ssh_wrapper_as "$TRASH_DIRECTORY/uplink" && - git clone "[myhost:123]:src" ssh-bracket-clone-uplink && + test_must_fail git clone "[myhost:123]:src" ssh-bracket-clone-uplink && + git clone "myhost:src" ssh-clone-uplink && expect_ssh myhost src ' diff --git a/t/t5603-clone-dirname.sh b/t/t5603-clone-dirname.sh index d5af758129..13b5e5eb9b 100755 --- a/t/t5603-clone-dirname.sh +++ b/t/t5603-clone-dirname.sh @@ -11,7 +11,9 @@ test_expect_success 'setup ssh wrapper' ' git upload-pack "$TRASH_DIRECTORY" EOF GIT_SSH="$TRASH_DIRECTORY/ssh-wrapper" && + GIT_SSH_VARIANT=ssh && export GIT_SSH && + export GIT_SSH_VARIANT && export TRASH_DIRECTORY ' -- cgit v1.2.1 From 233cd282ad71e667082bae45b3a73e947daa158b Mon Sep 17 00:00:00 2001 From: Jonathan Nieder Date: Mon, 20 Nov 2017 14:04:58 -0800 Subject: connect: correct style of C-style comment Documentation/CodingGuidelines explains: - Multi-line comments include their delimiters on separate lines from the text. E.g. /* * A very long * multi-line comment. */ Reported-by: Brandon Williams Signed-off-by: Jonathan Nieder Signed-off-by: Junio C Hamano --- connect.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/connect.c b/connect.c index d6033861b3..c3a014c5ba 100644 --- a/connect.c +++ b/connect.c @@ -889,7 +889,8 @@ static struct child_process *git_connect_git(int fd[2], char *hostandport, transport_check_allowed("git"); - /* These underlying connection commands die() if they + /* + * These underlying connection commands die() if they * cannot connect. */ if (git_use_proxy(hostandport)) -- cgit v1.2.1