Re: [PATCH 1/8] ssh test: make copy_ssh_wrapper_as clean up after itself

[Junio C Hamano]

Jonathan Nieder  writes:

> +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 -f ${1%$X}$X" &&
>   GIT_SSH="${1%$X}$X" &&

As we can clearly see in the context, this is not a new issue, but I
find the users of this helper that benefit from the "${1%$X}$X"
magic somewhat iffy.

There are callers of this helper that pass "somedir/plink" and
"somedir/plink.exe", but behind these callers that _think_ they are
testing the variant with and without the trailing ".exe", the helper
always add ".exe" (after stripping an existing one) on $X=.exe
platforms, ending up in testing the same thing twice.  On platforms
with $X='', testing two different command names may have "some"
value, but I wonder if it is cleaner to use a much less magical
"$1$X" here, and skip the test with a caller that gives ".exe"
variant using a test prerequisite on $X=.exe platforms to avoid
redundant tests?

This is totally outside the scope of this series; I mention this
only because this may be a possible #leftoverbits.

Thanks.

Re: [PATCH 1/8] ssh test: make copy_ssh_wrapper_as clean up after itself

[Brandon Williams]

On 11/20, Jonathan Nieder wrote:
> 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 
> ---
> Thanks to Dscho for tracking this subtle issue down.
> 
>  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..9d007c0f8d 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 -f ${1%$X}$X" &&
>   GIT_SSH="${1%$X}$X" &&
> - export GIT_SSH
> + test_when_finished "GIT_SSH=\"\$TRASH_DIRECTORY/ssh\$X\""

All the escaping!

Patch looks good.

>  }
>  
>  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
> -- 
> 2.15.0.448.gf294e3d99a
> 

-- 
Brandon Williams

[PATCH 1/8] ssh test: make copy_ssh_wrapper_as clean up after itself

[Jonathan Nieder]

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 
---
Thanks to Dscho for tracking this subtle issue down.

 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..9d007c0f8d 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 -f ${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
-- 
2.15.0.448.gf294e3d99a