git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
BWBrandon Williams <bmwill@google.com>
Date
Nov 20, 2017, 21:47 UTC
Message-ID
<20171120214720.GA92506@google.com>
In-Reply-To
<20171120212223.wquyxbmz34foynrk@aiede.mtv.corp.google.com>
On 11/20, Jonathan Nieder wrote:
Show 55 quoted lines
> 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.
Show 40 quoted lines
>  }
>  
>  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
Previous: Jonathan NiederNext: Junio C Hamano
Message 3 of 27 in “Coping with unrecognized ssh wrapper scripts in GIT_SSH”
  1. 0/8 Coping with unrecognized ssh wrapper scripts in GIT_SSHJonathan Nieder, Nov 20, 2017
  2. 1/8 ssh test: make copy_ssh_wrapper_as clean up after itselfJonathan Nieder, Nov 20, 2017
  3. Brandon WilliamsNov 20, 2017
  4. Junio C HamanoNov 21, 2017
  5. 1/8 ssh test: make copy_ssh_wrapper_as clean up after itselfJonathan Nieder, Nov 21, 2017
  6. Stefan BellerNov 21, 2017
  7. 2/8 connect: move no_fork fallback to git_tcp_connectJonathan Nieder, Nov 20, 2017
  8. 3/8 connect: split git:// setup into a separate functionJonathan Nieder, Nov 20, 2017
  9. Brandon WilliamsNov 20, 2017
  10. Jonathan NiederNov 20, 2017
  11. Brandon WilliamsNov 20, 2017
  12. 4/8 connect: split ssh command line options into separate functionJonathan Nieder, Nov 20, 2017
  13. Brandon WilliamsNov 20, 2017
  14. Jonathan NiederNov 20, 2017
  15. Brandon WilliamsNov 20, 2017
  16. 4/8 connect: split ssh command line options into separate functionJonathan Nieder, Nov 20, 2017
  17. 5/8 connect: split ssh option computation to its own functionJonathan Nieder, Nov 20, 2017
  18. Junio C HamanoNov 21, 2017
  19. 6/8 ssh: 'auto' variant to select between 'ssh' and 'simple'Jonathan Nieder, Nov 20, 2017
  20. Brandon WilliamsNov 20, 2017
  21. Junio C HamanoNov 21, 2017
  22. Jonathan NiederNov 21, 2017
  23. 7/8 ssh: 'simple' variant does not support -4/-6Jonathan Nieder, Nov 20, 2017
  24. 8/8 ssh: 'simple' variant does not support --portJonathan Nieder, Nov 20, 2017
  25. Brandon WilliamsNov 20, 2017
  26. Stefan BellerNov 22, 2017
  27. Junio C HamanoNov 22, 2017

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.