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(-) (limited to 't') 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.3 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 --- t/t5601-clone.sh | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) (limited to 't') 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.3 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 --- t/t5601-clone.sh | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) (limited to 't') 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.3 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 --- t/t5601-clone.sh | 10 ++++++++-- t/t5603-clone-dirname.sh | 2 ++ 2 files changed, 10 insertions(+), 2 deletions(-) (limited to 't') 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.3