[PATCH 1/4] difftool: silence uninitialized variable warning

[PATCH 1/4] difftool: silence uninitialized variable warning

[David Aguilar]

Git::config() returns `undef` when given keys that do not exist.
Check that the $guitool value is defined to prevent a noisy
"Use of uninitialized variable $guitool in length" warning.

Signed-off-by: David Aguilar <davvid@gmail.com>
---
 git-difftool.perl | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/git-difftool.perl b/git-difftool.perl
index 0a90de4..12231fb 100755
--- a/git-difftool.perl
+++ b/git-difftool.perl
@@ -336,7 +336,7 @@ sub main
 	}
 	if ($opts{gui}) {
 		my $guitool = Git::config('diff.guitool');
-		if (length($guitool) > 0) {
+		if (defined($guitool) && length($guitool) > 0) {
 			$ENV{GIT_DIFF_TOOL} = $guitool;
 		}
 	}
-- 
1.8.1.3.623.g622c8fc

[PATCH 2/4] t7800: Update copyright notice

[David Aguilar]

Signed-off-by: David Aguilar <davvid@gmail.com>
---
 t/t7800-difftool.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
index eb1d3f8..bb3158a 100755
--- a/t/t7800-difftool.sh
+++ b/t/t7800-difftool.sh
@@ -1,6 +1,6 @@
 #!/bin/sh
 #
-# Copyright (c) 2009, 2010 David Aguilar
+# Copyright (c) 2009, 2010, 2012 David Aguilar
 #
 
 test_description='git-difftool
-- 
1.8.1.3.623.g622c8fc

[PATCH 3/4] t7800: modernize tests

[David Aguilar]

Eliminate a lot of redundant work by using test_config().
Chain everything together by using sane_unset() and a
simpler difftool_test_setup().

The original tests relied upon restore_test_defaults()
from the previous test to provide the next test with a sane
environment.  Make the tests do their own setup so that they
are not dependent on the success of the previous test.
The end result is shorter tests and better test isolation.

Signed-off-by: David Aguilar <davvid@gmail.com>
---
 t/t7800-difftool.sh | 160 +++++++++++++++++++++-------------------------------
 1 file changed, 63 insertions(+), 97 deletions(-)

diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
index bb3158a..2d1ba8d 100755
--- a/t/t7800-difftool.sh
+++ b/t/t7800-difftool.sh
@@ -10,29 +10,11 @@ Testing basic diff tool invocation
 
 . ./test-lib.sh
 
-remove_config_vars()
+difftool_test_setup()
 {
-	# Unset all config variables used by git-difftool
-	git config --unset diff.tool
-	git config --unset diff.guitool
-	git config --unset difftool.test-tool.cmd
-	git config --unset difftool.prompt
-	git config --unset merge.tool
-	git config --unset mergetool.test-tool.cmd
-	git config --unset mergetool.prompt
-	return 0
-}
-
-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
+	test_config diff.tool test-tool &&
+	test_config difftool.test-tool.cmd 'cat $LOCAL' &&
+	test_config difftool.bogus-tool.cmd false
 }
 
 prompt_given()
@@ -65,36 +47,33 @@ test_expect_success PERL 'setup' '
 
 # Configure a custom difftool.<tool>.cmd and use it
 test_expect_success PERL 'custom commands' '
-	restore_test_defaults &&
-	git config difftool.test-tool.cmd "cat \$REMOTE" &&
-
+	difftool_test_setup &&
+	test_config difftool.test-tool.cmd "cat \$REMOTE" &&
 	diff=$(git difftool --no-prompt branch) &&
 	test "$diff" = "master" &&
 
-	restore_test_defaults &&
+	test_config difftool.test-tool.cmd "cat \$LOCAL" &&
 	diff=$(git difftool --no-prompt branch) &&
 	test "$diff" = "branch"
 '
 
 # Ensures that a custom difftool.<tool>.cmd overrides built-ins
 test_expect_success PERL 'custom commands override built-ins' '
-	restore_test_defaults &&
-	git config difftool.defaults.cmd "cat \$REMOTE" &&
+	test_config difftool.defaults.cmd "cat \$REMOTE" &&
 
 	diff=$(git difftool --tool defaults --no-prompt branch) &&
-	test "$diff" = "master" &&
-
-	git config --unset difftool.defaults.cmd
+	test "$diff" = "master"
 '
 
 # 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"
 '
 
 test_expect_success PERL 'difftool forwards arguments to diff' '
+	difftool_test_setup &&
 	>for-diff &&
 	git add for-diff &&
 	echo changes>for-diff &&
@@ -106,178 +85,165 @@ test_expect_success PERL 'difftool forwards arguments to diff' '
 '
 
 test_expect_success PERL 'difftool honors --gui' '
-	git config merge.tool bogus-tool &&
-	git config diff.tool bogus-tool &&
-	git config diff.guitool test-tool &&
+	difftool_test_setup &&
+	test_config merge.tool bogus-tool &&
+	test_config diff.tool bogus-tool &&
+	test_config diff.guitool test-tool &&
 
 	diff=$(git difftool --no-prompt --gui branch) &&
-	test "$diff" = "branch" &&
-
-	restore_test_defaults
+	test "$diff" = "branch"
 '
 
 test_expect_success PERL 'difftool --gui last setting wins' '
-	git config diff.guitool bogus-tool &&
-	git difftool --no-prompt --gui --no-gui &&
+	difftool_test_setup &&
 
-	git config merge.tool bogus-tool &&
-	git config diff.tool bogus-tool &&
-	git config diff.guitool test-tool &&
-	diff=$(git difftool --no-prompt --no-gui --gui branch) &&
-	test "$diff" = "branch" &&
+	diff=$(git difftool --no-prompt --gui --no-gui) &&
+	test -z "$diff" &&
 
-	restore_test_defaults
+	test_config merge.tool bogus-tool &&
+	test_config diff.tool bogus-tool &&
+	test_config diff.guitool test-tool &&
+
+	diff=$(git difftool --no-prompt --no-gui --gui branch) &&
+	test "$diff" = "branch"
 '
 
 test_expect_success PERL 'difftool --gui works without configured diff.guitool' '
-	git config diff.tool test-tool &&
+	difftool_test_setup &&
 
 	diff=$(git difftool --no-prompt --gui branch) &&
-	test "$diff" = "branch" &&
-
-	restore_test_defaults
+	test "$diff" = "branch"
 '
 
 # 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
 '
 
 # Test the $GIT_*_TOOL variables and ensure
 # that $GIT_DIFF_TOOL always wins unless --tool is specified
 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) &&
 	test "$diff" = "branch" &&
 
+	test_config diff.tool bogus-tool &&
+	test_config merge.tool bogus-tool &&
 	GIT_DIFF_TOOL=bogus-tool &&
 	export GIT_DIFF_TOOL &&
 
 	diff=$(git difftool --no-prompt --tool=test-tool branch) &&
 	test "$diff" = "branch" &&
-
-	restore_test_defaults
+	sane_unset GIT_DIFF_TOOL
 '
 
 # Test that we don't have to pass --no-prompt to difftool
 # when $GIT_DIFFTOOL_NO_PROMPT is true
 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) &&
 	test "$diff" = "branch" &&
-
-	restore_test_defaults
+	sane_unset GIT_DIFFTOOL_NO_PROMPT
 '
 
 # git-difftool supports the difftool.prompt variable.
 # Test that GIT_DIFFTOOL_PROMPT can override difftool.prompt = false
 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) &&
 	prompt_given "$prompt" &&
-
-	restore_test_defaults
+	sane_unset GIT_DIFFTOOL_PROMPT
 '
 
 # Test that we don't have to pass --no-prompt when difftool.prompt is false
 test_expect_success PERL 'difftool.prompt config variable is false' '
-	git config difftool.prompt false &&
+	difftool_test_setup &&
+	test_config difftool.prompt false &&
 
 	diff=$(git difftool branch) &&
-	test "$diff" = "branch" &&
-
-	restore_test_defaults
+	test "$diff" = "branch"
 '
 
 # Test that we don't have to pass --no-prompt when mergetool.prompt is false
 test_expect_success PERL 'difftool merge.prompt = false' '
+	difftool_test_setup &&
 	test_might_fail git config --unset difftool.prompt &&
-	git config mergetool.prompt false &&
+	test_config mergetool.prompt false &&
 
 	diff=$(git difftool branch) &&
-	test "$diff" = "branch" &&
-
-	restore_test_defaults
+	test "$diff" = "branch"
 '
 
 # Test that the -y flag can override difftool.prompt = true
 test_expect_success PERL 'difftool.prompt can overridden with -y' '
-	git config difftool.prompt true &&
+	difftool_test_setup &&
+	test_config difftool.prompt true &&
 
 	diff=$(git difftool -y branch) &&
-	test "$diff" = "branch" &&
-
-	restore_test_defaults
+	test "$diff" = "branch"
 '
 
 # Test that the --prompt flag can override difftool.prompt = false
 test_expect_success PERL 'difftool.prompt can overridden with --prompt' '
-	git config difftool.prompt false &&
+	difftool_test_setup &&
+	test_config difftool.prompt false &&
 
 	prompt=$(echo | git difftool --prompt branch | tail -1) &&
-	prompt_given "$prompt" &&
-
-	restore_test_defaults
+	prompt_given "$prompt"
 '
 
 # Test that the last flag passed on the command-line wins
 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) &&
-	prompt_given "$prompt" &&
-
-	restore_test_defaults
+	prompt_given "$prompt"
 '
 
 # git-difftool falls back to git-mergetool config variables
 # so test that behavior here
 test_expect_success PERL 'difftool + mergetool config variables' '
-	remove_config_vars &&
-	git config merge.tool test-tool &&
-	git config mergetool.test-tool.cmd "cat \$LOCAL" &&
+	test_config merge.tool test-tool &&
+	test_config mergetool.test-tool.cmd "cat \$LOCAL" &&
 
 	diff=$(git difftool --no-prompt branch) &&
 	test "$diff" = "branch" &&
 
 	# set merge.tool to something bogus, diff.tool to test-tool
-	git config merge.tool bogus-tool &&
-	git config diff.tool test-tool &&
+	test_config merge.tool bogus-tool &&
+	test_config diff.tool test-tool &&
 
 	diff=$(git difftool --no-prompt branch) &&
-	test "$diff" = "branch" &&
-
-	restore_test_defaults
+	test "$diff" = "branch"
 '
 
 test_expect_success PERL 'difftool.<tool>.path' '
-	git config difftool.tkdiff.path echo &&
+	test_config difftool.tkdiff.path echo &&
 	diff=$(git difftool --tool=tkdiff --no-prompt branch) &&
-	git config --unset difftool.tkdiff.path &&
 	lines=$(echo "$diff" | grep file | wc -l) &&
-	test "$lines" -eq 1 &&
-
-	restore_test_defaults
+	test "$lines" -eq 1
 '
 
 test_expect_success PERL 'difftool --extcmd=cat' '
-- 
1.8.1.3.623.g622c8fc

[PATCH 4/4] t7800: "defaults" is no longer a builtin tool name

[David Aguilar]

073678b8e6324a155fa99f40eee0637941a70a34 reworked the
mergetools/ directory so that every file corresponds to a
difftool-supported tool.  When this happened the "defaults"
file went away as it was no longer needed by mergetool--lib.

t7800 tests that configured commands can override builtins,
but this test was not adjusted when the "defaults" file was
removed because the test continued to pass.

Adjust the test to use the everlasting "vimdiff" tool name
instead of "defaults" so that it correctly tests against a tool
that is known by mergetool--lib.

Signed-off-by: David Aguilar <davvid@gmail.com>
---
 t/t7800-difftool.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
index 2d1ba8d..6307c36 100755
--- a/t/t7800-difftool.sh
+++ b/t/t7800-difftool.sh
@@ -59,9 +59,9 @@ test_expect_success PERL 'custom commands' '
 
 # Ensures that a custom difftool.<tool>.cmd overrides built-ins
 test_expect_success PERL 'custom commands override built-ins' '
-	test_config difftool.defaults.cmd "cat \$REMOTE" &&
+	test_config difftool.vimdiff.cmd "cat \$REMOTE" &&
 
-	diff=$(git difftool --tool defaults --no-prompt branch) &&
+	diff=$(git difftool --tool vimdiff --no-prompt branch) &&
 	test "$diff" = "master"
 '
 
-- 
1.8.1.3.623.g622c8fc

Re: [PATCH 2/4] t7800: Update copyright notice

[David Aguilar]

On Fri, Feb 15, 2013 at 9:47 PM, David Aguilar <davvid@gmail.com> wrote:
> Signed-off-by: David Aguilar <davvid@gmail.com>
> ---
>  t/t7800-difftool.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
> index eb1d3f8..bb3158a 100755
> --- a/t/t7800-difftool.sh
> +++ b/t/t7800-difftool.sh
> @@ -1,6 +1,6 @@
>  #!/bin/sh
>  #
> -# Copyright (c) 2009, 2010 David Aguilar
> +# Copyright (c) 2009, 2010, 2012 David Aguilar

Oh boy, I'm still living in the past.
This should also include 2013.  It gets me every time! ;-)

I'll wait and see if there are other review comments before I resend.
-- 
David

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

[Jonathan Nieder]

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

Re: [PATCH 4/4] t7800: "defaults" is no longer a builtin tool name

[Jonathan Nieder]

David Aguilar wrote:

> t7800 tests that configured commands can override builtins,
> but this test was not adjusted when the "defaults" file was
> removed because the test continued to pass.
>
> Adjust the test to use the everlasting "vimdiff"

Heh. :)

>                                                  tool name
> instead of "defaults" so that it correctly tests against a tool
> that is known by mergetool--lib.

Makes sense.  Thanks for a pleasant read.

Good night,
Jonathan

[PATCH 2/4] t7800: update copyright notice

[David Aguilar]

Signed-off-by: David Aguilar <davvid@gmail.com>
---
Unchanged since v2.

 t/t7800-difftool.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
index eb1d3f8..5b5939b 100755
--- a/t/t7800-difftool.sh
+++ b/t/t7800-difftool.sh
@@ -1,6 +1,6 @@
 #!/bin/sh
 #
-# Copyright (c) 2009, 2010 David Aguilar
+# Copyright (c) 2009, 2010, 2012, 2013 David Aguilar
 #
 
 test_description='git-difftool
-- 
1.8.2.rc0.20.gf548dd7