Re: [PATCH 3/4] t7800: modernize tests

[Jonathan Nieder <jrnieder@gmail.com>]

David Aguilar wrote:

> --- a/t/t7800-difftool.sh
> +++ b/t/t7800-difftool.sh
> @@ -10,29 +10,11 @@ Testing basic diff tool invocation
[...]
> -restore_test_defaults()
> -{
> -	# Restores the test defaults used by several tests
> -	remove_config_vars
> -	unset GIT_DIFF_TOOL
> -	unset GIT_DIFFTOOL_PROMPT
> -	unset GIT_DIFFTOOL_NO_PROMPT
> -	git config diff.tool test-tool &&
> -	git config difftool.test-tool.cmd 'cat $LOCAL'
> -	git config difftool.bogus-tool.cmd false

Yay. :)

[...]
>  # Ensures that git-difftool ignores bogus --tool values
>  test_expect_success PERL 'difftool ignores bad --tool values' '
>  	diff=$(git difftool --no-prompt --tool=bad-tool branch)
>  	test "$?" = 1 &&
> -	test "$diff" = ""
> +	test -z "$diff"
>  '

Not about this patch: if I add more commands before that "diff",
their exit status would be ignored.  Could this be made more resilient
using test_expect_code?  Something like

	test_expect_code 1 git diff --no-prompt --tool=bad-tool branch >actual &&
	>expect &&
	test_cmp expect actual

[...]
>  # Specify the diff tool using $GIT_DIFF_TOOL
>  test_expect_success PERL 'GIT_DIFF_TOOL variable' '
> -	test_might_fail git config --unset diff.tool &&
> +	difftool_test_setup &&
> +	git config --unset diff.tool &&
> +
>  	GIT_DIFF_TOOL=test-tool &&
>  	export GIT_DIFF_TOOL &&
>  
>  	diff=$(git difftool --no-prompt branch) &&
>  	test "$diff" = "branch" &&
> -
> -	restore_test_defaults
> +	sane_unset GIT_DIFF_TOOL

If this test fails, GIT_DIFF_TOOL would remain set which could take
down later tests, too.  Could it be set in a subprocess (e.g., a
subshell) to avoid that?

	difftool_test_setup &&
	git config --unset diff.tool &&

	echo branch >expect &&
	GIT_DIFF_TOOL=test-tool git difftool --no-prompt branch >actual &&
	test_cmp expect actual

[...]
>  test_expect_success PERL 'GIT_DIFF_TOOL overrides' '
> -	git config diff.tool bogus-tool &&
> -	git config merge.tool bogus-tool &&
> -
> +	difftool_test_setup &&
> +	test_config diff.tool bogus-tool &&
> +	test_config merge.tool bogus-tool &&
>  	GIT_DIFF_TOOL=test-tool &&
>  	export GIT_DIFF_TOOL &&
>  
>  	diff=$(git difftool --no-prompt branch) &&

Likewise.

[...]
>  	GIT_DIFF_TOOL=bogus-tool &&
>  	export GIT_DIFF_TOOL &&
>  
>  	diff=$(git difftool --no-prompt --tool=test-tool branch) &&

Likewise.

[...]
>  test_expect_success PERL 'GIT_DIFFTOOL_NO_PROMPT variable' '
> +	difftool_test_setup &&
>  	GIT_DIFFTOOL_NO_PROMPT=true &&
>  	export GIT_DIFFTOOL_NO_PROMPT &&
>  
>  	diff=$(git difftool branch) &&

Likewise.

[...]
>  test_expect_success PERL 'GIT_DIFFTOOL_PROMPT variable' '
> -	git config difftool.prompt false &&
> +	difftool_test_setup &&
> +	test_config difftool.prompt false &&
>  	GIT_DIFFTOOL_PROMPT=true &&
>  	export GIT_DIFFTOOL_PROMPT &&
>  
>  	prompt=$(echo | git difftool branch | tail -1) &&

Likewise.  This one loses the exit status from 'git difftool',
which could be avoided by writing to temporary files:

	echo >input &&
	GIT_DIFFTOOL_PROMPT=true git difftool branch <input >output &&
	prompt=$(tail -1 <output) &&

[...]
>  test_expect_success PERL 'difftool last flag wins' '
> +	difftool_test_setup &&
>  	diff=$(git difftool --prompt --no-prompt branch) &&
>  	test "$diff" = "branch" &&
>  
> -	restore_test_defaults &&
> -
>  	prompt=$(echo | git difftool --no-prompt --prompt branch | tail -1) &&
[...]

Likewise.

Thanks for cleaning up, and sorry I don't have anything more
substantial to offer.

Hope that helps,
Jonathan