From 37ee646e72d7f39d61a538e21a4c2721e32cb444 Mon Sep 17 00:00:00 2001 From: "brian m. carlson" Date: Sun, 26 Apr 2015 20:30:10 +0000 Subject: [PATCH 1/3] connect: simplify SSH connection code path The code path used in git_connect pushed the majority of the SSH connection code into an else block, even though the if block returns. Simplify the code by eliminating the else block, as it is unneeded. Signed-off-by: Jeff King Signed-off-by: brian m. carlson Signed-off-by: Junio C Hamano --- connect.c | 40 ++++++++++++++++++++-------------------- 1 file changed, 20 insertions(+), 20 deletions(-) diff --git a/connect.c b/connect.c index ce0e121423..170e391221 100644 --- a/connect.c +++ b/connect.c @@ -740,28 +740,28 @@ struct child_process *git_connect(int fd[2], const char *url, free(hostandport); free(path); return NULL; + } + + ssh = getenv("GIT_SSH_COMMAND"); + if (ssh) { + conn->use_shell = 1; + putty = 0; } else { - ssh = getenv("GIT_SSH_COMMAND"); - if (ssh) { - conn->use_shell = 1; - putty = 0; - } else { - ssh = getenv("GIT_SSH"); - if (!ssh) - ssh = "ssh"; - putty = !!strcasestr(ssh, "plink"); - } - - argv_array_push(&conn->args, ssh); - if (putty && !strcasestr(ssh, "tortoiseplink")) - argv_array_push(&conn->args, "-batch"); - if (port) { - /* P is for PuTTY, p is for OpenSSH */ - argv_array_push(&conn->args, putty ? "-P" : "-p"); - argv_array_push(&conn->args, port); - } - argv_array_push(&conn->args, ssh_host); + ssh = getenv("GIT_SSH"); + if (!ssh) + ssh = "ssh"; + putty = !!strcasestr(ssh, "plink"); + } + + argv_array_push(&conn->args, ssh); + if (putty && !strcasestr(ssh, "tortoiseplink")) + argv_array_push(&conn->args, "-batch"); + if (port) { + /* P is for PuTTY, p is for OpenSSH */ + argv_array_push(&conn->args, putty ? "-P" : "-p"); + argv_array_push(&conn->args, port); } + argv_array_push(&conn->args, ssh_host); } else { /* remove repo-local variables from the environment */ conn->env = local_repo_env; From d1018c2494c05e6a27ec5945902306cfb9e45b0c Mon Sep 17 00:00:00 2001 From: "brian m. carlson" Date: Sun, 26 Apr 2015 20:30:11 +0000 Subject: [PATCH 2/3] t5601: fix quotation error leading to skipped tests One of the tests in t5601 used single quotes to delimit an argument containing spaces. However, this caused test_expect_success to be passed three arguments instead of two, which in turn caused the test name to be treated as a prerequisite instead of a test name. As there was no prerequisite called "bracketed hostnames are still ssh", the test was always skipped. Because this test was always skipped, the fact that it passed the arguments in the wrong order was obscured. Use double quotes inside the test and reorder the arguments so that the test runs and properly reflects the arguments that are passed to ssh. Signed-off-by: brian m. carlson Signed-off-by: Junio C Hamano --- t/t5601-clone.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh index 02b40b117f..5efc177917 100755 --- a/t/t5601-clone.sh +++ b/t/t5601-clone.sh @@ -332,7 +332,7 @@ test_expect_success !MINGW,!CYGWIN 'clone local path foo:bar' ' test_expect_success 'bracketed hostnames are still ssh' ' git clone "[myhost:123]:src" ssh-bracket-clone && - expect_ssh myhost '-p 123' src + expect_ssh "-p 123" myhost src ' counter=0 From baaf233755f71c057d28b9e8692e24d4fca7d22f Mon Sep 17 00:00:00 2001 From: "brian m. carlson" Date: Sun, 26 Apr 2015 20:30:12 +0000 Subject: [PATCH 3/3] connect: improve check for plink to reduce false positives The git_connect function has code to handle plink and tortoiseplink specially, as they require different command line arguments from OpenSSH (-P instead of -p for ports; tortoiseplink additionally requires -batch). However, the match was done by checking for "plink" anywhere in the string, which led to a GIT_SSH value containing "uplink" being treated as an invocation of putty's plink. Improve the check by looking for "plink" or "tortoiseplink" (or those names suffixed with ".exe") only in the final component of the path. This has the downside that a program such as "plink-0.63" would no longer be recognized, but the increased robustness is likely worth it. Add tests to cover these cases to avoid regressions. Signed-off-by: brian m. carlson Acked-by: Johannes Schindelin Signed-off-by: Junio C Hamano --- connect.c | 18 +++++++++++++++--- t/t5601-clone.sh | 33 +++++++++++++++++++++++++++++++++ 2 files changed, 48 insertions(+), 3 deletions(-) diff --git a/connect.c b/connect.c index 170e391221..1f7b7694f6 100644 --- a/connect.c +++ b/connect.c @@ -722,7 +722,7 @@ struct child_process *git_connect(int fd[2], const char *url, conn->in = conn->out = -1; if (protocol == PROTO_SSH) { const char *ssh; - int putty; + int putty, tortoiseplink = 0; char *ssh_host = hostandport; const char *port = NULL; get_host_and_port(&ssh_host, &port); @@ -747,14 +747,26 @@ struct child_process *git_connect(int fd[2], const char *url, conn->use_shell = 1; putty = 0; } else { + const char *base; + char *ssh_dup; + ssh = getenv("GIT_SSH"); if (!ssh) ssh = "ssh"; - putty = !!strcasestr(ssh, "plink"); + + ssh_dup = xstrdup(ssh); + base = basename(ssh_dup); + + tortoiseplink = !strcasecmp(base, "tortoiseplink") || + !strcasecmp(base, "tortoiseplink.exe"); + putty = !strcasecmp(base, "plink") || + !strcasecmp(base, "plink.exe") || tortoiseplink; + + free(ssh_dup); } argv_array_push(&conn->args, ssh); - if (putty && !strcasestr(ssh, "tortoiseplink")) + if (tortoiseplink) argv_array_push(&conn->args, "-batch"); if (port) { /* P is for PuTTY, p is for OpenSSH */ diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh index 5efc177917..731c09c418 100755 --- a/t/t5601-clone.sh +++ b/t/t5601-clone.sh @@ -296,6 +296,12 @@ setup_ssh_wrapper () { ' } +copy_ssh_wrapper_as () { + cp "$TRASH_DIRECTORY/ssh-wrapper" "$1" && + GIT_SSH="$1" && + export GIT_SSH +} + expect_ssh () { test_when_finished ' (cd "$TRASH_DIRECTORY" && rm -f ssh-expect && >ssh-output) @@ -335,6 +341,33 @@ test_expect_success 'bracketed hostnames are still ssh' ' expect_ssh "-p 123" myhost src ' +test_expect_success 'uplink is not treated as putty' ' + copy_ssh_wrapper_as "$TRASH_DIRECTORY/uplink" && + git clone "[myhost:123]:src" ssh-bracket-clone-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 && + expect_ssh "-P 123" myhost src +' + +test_expect_success 'plink.exe is treated specially (as putty)' ' + copy_ssh_wrapper_as "$TRASH_DIRECTORY/plink.exe" && + git clone "[myhost:123]:src" ssh-bracket-clone-plink-1 && + expect_ssh "-P 123" myhost src +' + +test_expect_success 'tortoiseplink is like putty, with extra arguments' ' + copy_ssh_wrapper_as "$TRASH_DIRECTORY/tortoiseplink" && + git clone "[myhost:123]:src" ssh-bracket-clone-plink-2 && + expect_ssh "-batch -P 123" myhost src +' + +# Reset the GIT_SSH environment variable for clone tests. +setup_ssh_wrapper + counter=0 # $1 url # $2 none|host