threads / patch / 32916

patch, 4 partsdifftool: silence uninitialized variable warning

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

## tl;dr

7 messages between Feb 16, 2013 and Feb 17, 2013. Diffs are folded; open one to read it.

replies: 6people: 2as markdown or json

David Aguilar· Feb 16, 2013, 05:47 UTC · lore

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(-)
Show changes to git-difftool.perl +1 −1
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
David Aguilar· Feb 16, 2013, 05:47 UTC · re: David Aguilar · lore

[PATCH 2/4] t7800: Update copyright notice

Signed-off-by: David Aguilar <davvid@gmail.com>
---
 t/t7800-difftool.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Show changes to t/t7800-difftool.sh +1 −1
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
David Aguilar· Feb 16, 2013, 05:47 UTC · re: David Aguilar · lore

[PATCH 3/4] t7800: modernize tests

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(-)
Show changes to t/t7800-difftool.sh +63 −97
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
David Aguilar· Feb 16, 2013, 05:47 UTC · re: David Aguilar · lore

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

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(-)
Show changes to t/t7800-difftool.sh +2 −2
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
Jonathan Nieder· Feb 17, 2013, 11:23 UTC · re: David Aguilar · lore

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

David Aguilar wrote:
Show 5 quoted lines
> 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

Jonathan Nieder· Feb 17, 2013, 11:21 UTC · re: David Aguilar · lore

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

David Aguilar wrote:
> --- a/t/t7800-difftool.sh
> +++ b/t/t7800-difftool.sh
> @@ -10,29 +10,11 @@ Testing basic diff tool invocation
[...]
Show 10 quoted lines
> -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. :)
[...]
Show 7 quoted lines
>  # 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
[...]
Show 14 quoted lines
>  # 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
[...]
Show 11 quoted lines
>  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.
[...]
Show 6 quoted lines
>  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.
[...]
Show 8 quoted lines
>  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) &&
[...]
Show 8 quoted lines
>  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

David Aguilar· Feb 16, 2013, 06:35 UTC · re: David Aguilar · lore

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

On Fri, Feb 15, 2013 at 9:47 PM, David Aguilar <davvid@gmail.com> wrote:
Show 14 quoted lines
> 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

← back to recent threads