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

[Brandon Williams <bmwill@google.com>]

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 <johannes.schindelin@gmx.de>
> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
> ---
> 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