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