We already handle PuTTY's plink and TortoiseGit's tortoiseplink in GIT_SSH by automatically using the -P option to specify ports, and in tortoiseplink's case by passing the --batch option. For users who need to pass additional command-line options to plink, this poses a problem: the only way to do that is to use GIT_SSH_COMMAND, but Git does not handle that specifically, so those users have to manually parse the command-line options passed via GIT_SSH_COMMAND and replace -p (if present) by -P, and add --batch in the case of tortoiseplink. This is error-prone and a bad user experience. To fix this, the changes proposed in this patch series introduce handling this by splitting the GIT_SSH_COMMAND value and treating the first parameter with the same grace as GIT_SSH. To counter any possible misdetection, the user can also specify explicitly via GIT_SSH_VARIANT or ssh.variant which SSH variant they are using. Changes relative to v2: - touched up the documentation for ssh.variant to make it even easier to understand - free()d the config variable - completely refactored the code to fulfil Junio's burning desire to avoid split_cmdline() when unnecessary It is quite preposterous to call this an "iteration" of the patch series, because the code is so different now. I say this because I want to caution that this code has not been tested as thoroughly, by far, as the first iteration. The primary purpose of code review is correctness, everything else is either a consequence of it, or a means to make reviewing easier. That means that I highly encourage those who pushed for these extensive changes that make the patch series a lot less robust to balance things out by at least *rudimentary* testing. Johannes Schindelin (1): git_connect(): factor out SSH variant handling Junio C Hamano (1): connect: rename tortoiseplink and putty variables Segev Finer (2): connect: handle putty/plink also in GIT_SSH_COMMAND connect: Add the envvar GIT_SSH_VARIANT and ssh.variant config Documentation/config.txt | 11 +++++++ Documentation/git.txt | 6 ++++ connect.c | 75 ++++++++++++++++++++++++++++++++++++------------ t/t5601-clone.sh | 41 ++++++++++++++++++++++++++ 4 files changed, 114 insertions(+), 19 deletions(-) base-commit: 8f60064c1f538f06e1c579cbd9840b86b10bcd3d Published-As: https://github.com/dscho/git/releases/tag/putty-w-args-v3 Fetch-It-Via: git fetch https://github.com/dscho/git putty-w-args-v3 Interdiff vs v2: diff --git a/Documentation/config.txt b/Documentation/config.txt index f2c210f0a0..b88df57ab6 100644 --- a/Documentation/config.txt +++ b/Documentation/config.txt @@ -1950,11 +1950,15 @@ 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:: - Override the autodetection of plink/tortoiseplink in the SSH - command that 'git fetch' and 'git push' use. It can be set to - either `ssh`, `plink`, `putty` or `tortoiseplink`. Any other - value will be treated as normal ssh. This is useful in case - that Git gets this wrong. + 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 plink or tortoiseplink, as opposed to the default (OpenSSH). ++ +The config variable `ssh.variant` can be set to override this auto-detection; +valid values are `ssh`, `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`. i18n.commitEncoding:: Character encoding the commit messages are stored in; Git itself diff --git a/Documentation/git.txt b/Documentation/git.txt index c322558aa7..a0c6728d1a 100644 --- a/Documentation/git.txt +++ b/Documentation/git.txt @@ -1021,11 +1021,10 @@ personal `.ssh/config` file. Please consult your ssh documentation for further details. `GIT_SSH_VARIANT`:: - If this environment variable is set, it overrides the autodetection - of plink/tortoiseplink in the SSH command that 'git fetch' and 'git - push' use. It can be set to either `ssh`, `plink`, `putty` or - `tortoiseplink`. Any other value will be treated as normal ssh. This - is useful in case that Git gets this wrong. + If this environment variable is set, it overrides Git's autodetection + whether `GIT_SSH`/`GIT_SSH_COMMAND`/`core.sshCommand` refer to OpenSSH, + plink or tortoiseplink. This variable overrides the config setting + `ssh.variant` that serves the same purpose. `GIT_ASKPASS`:: If this environment variable is set, then Git commands which need to diff --git a/connect.c b/connect.c index 7b4437578b..7f1f802396 100644 --- a/connect.c +++ b/connect.c @@ -691,20 +691,45 @@ static const char *get_ssh_command(void) return NULL; } -static int handle_ssh_variant(int *port_option, int *needs_batch) +static int handle_ssh_variant(const char *ssh_command, int is_cmdline, + int *port_option, int *needs_batch) { - const char *variant; + const char *variant = getenv("GIT_SSH_VARIANT"); + char *p = NULL; + + if (variant) + ; /* okay, fall through */ + else if (!git_config_get_string("ssh.variant", &p)) + variant = p; + else if (!is_cmdline) { + p = xstrdup(ssh_command); + variant = basename(p); + } else { + const char **ssh_argv; - if (!(variant = getenv("GIT_SSH_VARIANT")) && - git_config_get_string_const("ssh.variant", &variant)) - return 0; + p = xstrdup(ssh_command); + if (split_cmdline(p, &ssh_argv)) { + variant = basename((char *)ssh_argv[0]); + /* + * At this point, variant points into the buffer + * referenced by p, hence we do not need ssh_argv + * any longer. + */ + free(ssh_argv); + } else + return 0; + } - if (!strcmp(variant, "plink") || !strcmp(variant, "putty")) + if (!strcasecmp(variant, "plink") || + !strcasecmp(variant, "plink.exe") || + !strcasecmp(variant, "putty")) *port_option = 'P'; - else if (!strcmp(variant, "tortoiseplink")) { + else if (!strcasecmp(variant, "tortoiseplink") || + !strcasecmp(variant, "tortoiseplink.exe")) { *port_option = 'P'; *needs_batch = 1; } + free(p); return 1; } @@ -791,7 +816,6 @@ struct child_process *git_connect(int fd[2], const char *url, int port_option = 'p'; char *ssh_host = hostandport; const char *port = NULL; - char *ssh_argv0 = NULL; transport_check_allowed("ssh"); get_host_and_port(&ssh_host, &port); @@ -812,15 +836,10 @@ struct child_process *git_connect(int fd[2], const char *url, } ssh = get_ssh_command(); - if (ssh) { - char *split_ssh = xstrdup(ssh); - const char **ssh_argv; - - if (split_cmdline(split_ssh, &ssh_argv)) - ssh_argv0 = xstrdup(ssh_argv[0]); - free(split_ssh); - free((void *)ssh_argv); - } else { + if (ssh) + handle_ssh_variant(ssh, 1, &port_option, + &needs_batch); + else { /* * GIT_SSH is the no-shell version of * GIT_SSH_COMMAND (and must remain so for @@ -831,26 +850,12 @@ struct child_process *git_connect(int fd[2], const char *url, ssh = getenv("GIT_SSH"); if (!ssh) ssh = "ssh"; - - ssh_argv0 = xstrdup(ssh); + else + handle_ssh_variant(ssh, 0, + &port_option, + &needs_batch); } - if (!handle_ssh_variant(&port_option, &needs_batch) && - ssh_argv0) { - const char *base = basename(ssh_argv0); - - if (!strcasecmp(base, "tortoiseplink") || - !strcasecmp(base, "tortoiseplink.exe")) { - port_option = 'P'; - needs_batch = 1; - } else if (!strcasecmp(base, "plink") || - !strcasecmp(base, "plink.exe")) { - port_option = 'P'; - } - } - - free(ssh_argv0); - argv_array_push(&conn->args, ssh); if (flags & CONNECT_IPV4) argv_array_push(&conn->args, "-4"); -- 2.11.1.windows.prerelease.2.9.g3014b57