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

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 <redacted>
---
Unchanged since v1.

 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.2.rc0.20.gf548dd7

Signed-off-by: David Aguilar <redacted>
---
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

Eliminate a lot of redundant work by using test_config().
Catch more return codes by more use of temporary files
and test_cmp.

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 <redacted>
---
v3 includes Junio's review notes to avoid cat with stdin_contains
and to use DQ around $LOCAL.

Another difference from v2 is that it tweaks the function
declarations to keep a SP between the name and parens to better
follow the standard git style.

 t/t7800-difftool.sh | 366 ++++++++++++++++++++++++----------------------------
 1 file changed, 168 insertions(+), 198 deletions(-)

diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
index 5b5939b..fb00273 100755
--- a/t/t7800-difftool.sh
+++ b/t/t7800-difftool.sh

@@ -10,43 +10,25 @@ 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
+	test_config diff.tool test-tool &&
+	test_config difftool.test-tool.cmd 'cat "$LOCAL"' &&
+	test_config difftool.bogus-tool.cmd false
 }
 
-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
-}
-
-prompt_given()
+prompt_given ()
 {
 	prompt="$1"
 	test "$prompt" = "Launch 'test-tool' [Y/n]: branch"
 }
 
-stdin_contains()
+stdin_contains ()
 {
 	grep >/dev/null "$1"
 }
 
-stdin_doesnot_contain()
+stdin_doesnot_contain ()
 {
 	! stdin_contains "$1"
 }

@@ -65,249 +47,237 @@ 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\"" &&
+	echo master >expect &&
+	git difftool --no-prompt branch >actual &&
+	test_cmp expect actual &&
 
-	diff=$(git difftool --no-prompt branch) &&
-	test "$diff" = "master" &&
-
-	restore_test_defaults &&
-	diff=$(git difftool --no-prompt branch) &&
-	test "$diff" = "branch"
+	test_config difftool.test-tool.cmd "cat \"\$LOCAL\"" &&
+	echo branch >expect &&
+	git difftool --no-prompt branch >actual &&
+	test_cmp expect actual
 '
 
-# 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" &&
-
-	diff=$(git difftool --tool defaults --no-prompt branch) &&
-	test "$diff" = "master" &&
-
-	git config --unset difftool.defaults.cmd
+test_expect_success PERL 'custom tool commands override built-ins' '
+	test_config difftool.defaults.cmd "cat \"\$REMOTE\"" &&
+	echo master >expect &&
+	git difftool --tool defaults --no-prompt branch >actual &&
+	test_cmp expect actual
 '
 
-# 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" = ""
+	: >expect &&
+	test_expect_code 1 \
+		git difftool --no-prompt --tool=bad-tool branch >actual &&
+	test_cmp expect actual
 '
 
 test_expect_success PERL 'difftool forwards arguments to diff' '
+	difftool_test_setup &&
 	>for-diff &&
 	git add for-diff &&
 	echo changes>for-diff &&
 	git add for-diff &&
-	diff=$(git difftool --cached --no-prompt -- for-diff) &&
-	test "$diff" = "" &&
+	: >expect &&
+	git difftool --cached --no-prompt -- for-diff >actual &&
+	test_cmp expect actual &&
 	git reset -- for-diff &&
 	rm for-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 &&
-
-	diff=$(git difftool --no-prompt --gui branch) &&
-	test "$diff" = "branch" &&
+	difftool_test_setup &&
+	test_config merge.tool bogus-tool &&
+	test_config diff.tool bogus-tool &&
+	test_config diff.guitool test-tool &&
 
-	restore_test_defaults
+	echo branch >expect &&
+	git difftool --no-prompt --gui branch >actual &&
+	test_cmp expect actual
 '
 
 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 &&
+	: >expect &&
+	git difftool --no-prompt --gui --no-gui >actual &&
+	test_cmp expect actual &&
 
-	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" &&
-
-	restore_test_defaults
+	test_config merge.tool bogus-tool &&
+	test_config diff.tool bogus-tool &&
+	test_config diff.guitool test-tool &&
+	echo branch >expect &&
+	git difftool --no-prompt --no-gui --gui branch >actual &&
+	test_cmp expect actual
 '
 
 test_expect_success PERL 'difftool --gui works without configured diff.guitool' '
-	git config diff.tool test-tool &&
-
-	diff=$(git difftool --no-prompt --gui branch) &&
-	test "$diff" = "branch" &&
-
-	restore_test_defaults
+	difftool_test_setup &&
+	echo branch >expect &&
+	git difftool --no-prompt --gui branch >actual &&
+	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 &&
-	GIT_DIFF_TOOL=test-tool &&
-	export GIT_DIFF_TOOL &&
-
-	diff=$(git difftool --no-prompt branch) &&
-	test "$diff" = "branch" &&
-
-	restore_test_defaults
+	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 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 &&
-
-	GIT_DIFF_TOOL=test-tool &&
-	export GIT_DIFF_TOOL &&
-
-	diff=$(git difftool --no-prompt branch) &&
-	test "$diff" = "branch" &&
+	difftool_test_setup &&
+	test_config diff.tool bogus-tool &&
+	test_config merge.tool bogus-tool &&
 
-	GIT_DIFF_TOOL=bogus-tool &&
-	export GIT_DIFF_TOOL &&
+	echo branch >expect &&
+	GIT_DIFF_TOOL=test-tool git difftool --no-prompt branch >actual &&
+	test_cmp expect actual &&
 
-	diff=$(git difftool --no-prompt --tool=test-tool branch) &&
-	test "$diff" = "branch" &&
-
-	restore_test_defaults
+	test_config diff.tool bogus-tool &&
+	test_config merge.tool bogus-tool &&
+	GIT_DIFF_TOOL=bogus-tool \
+		git difftool --no-prompt --tool=test-tool branch >actual &&
+	test_cmp expect actual
 '
 
 # 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' '
-	GIT_DIFFTOOL_NO_PROMPT=true &&
-	export GIT_DIFFTOOL_NO_PROMPT &&
-
-	diff=$(git difftool branch) &&
-	test "$diff" = "branch" &&
-
-	restore_test_defaults
+	difftool_test_setup &&
+	echo branch >expect &&
+	GIT_DIFFTOOL_NO_PROMPT=true git difftool branch >actual &&
+	test_cmp expect actual
 '
 
 # 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 &&
-	GIT_DIFFTOOL_PROMPT=true &&
-	export GIT_DIFFTOOL_PROMPT &&
-
-	prompt=$(echo | git difftool branch | tail -1) &&
-	prompt_given "$prompt" &&
-
-	restore_test_defaults
+	difftool_test_setup &&
+	test_config difftool.prompt false &&
+	echo >input &&
+	GIT_DIFFTOOL_PROMPT=true git difftool branch <input >output &&
+	prompt=$(tail -1 <output) &&
+	prompt_given "$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 &&
-
-	diff=$(git difftool branch) &&
-	test "$diff" = "branch" &&
-
-	restore_test_defaults
+	difftool_test_setup &&
+	test_config difftool.prompt false &&
+	echo branch >expect &&
+	git difftool branch >actual &&
+	test_cmp expect actual
 '
 
 # 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 &&
-
-	diff=$(git difftool branch) &&
-	test "$diff" = "branch" &&
-
-	restore_test_defaults
+	test_config mergetool.prompt false &&
+	echo branch >expect &&
+	git difftool branch >actual &&
+	test_cmp expect actual
 '
 
 # 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 &&
-
-	diff=$(git difftool -y branch) &&
-	test "$diff" = "branch" &&
-
-	restore_test_defaults
+	difftool_test_setup &&
+	test_config difftool.prompt true &&
+	echo branch >expect &&
+	git difftool -y branch >actual &&
+	test_cmp expect actual
 '
 
 # 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 &&
-
-	prompt=$(echo | git difftool --prompt branch | tail -1) &&
-	prompt_given "$prompt" &&
-
-	restore_test_defaults
+	difftool_test_setup &&
+	test_config difftool.prompt false &&
+	echo >input &&
+	git difftool --prompt branch <input >output &&
+	prompt=$(tail -1 <output) &&
+	prompt_given "$prompt"
 '
 
 # Test that the last flag passed on the command-line wins
 test_expect_success PERL 'difftool last flag wins' '
-	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
+	difftool_test_setup &&
+	echo branch >expect &&
+	git difftool --prompt --no-prompt branch >actual &&
+	test_cmp expect actual &&
+	echo >input &&
+	git difftool --no-prompt --prompt branch <input >output &&
+	prompt=$(tail -1 <output) &&
+	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" &&
-
-	diff=$(git difftool --no-prompt branch) &&
-	test "$diff" = "branch" &&
+	test_config merge.tool test-tool &&
+	test_config mergetool.test-tool.cmd "cat \$LOCAL" &&
+	echo branch >expect &&
+	git difftool --no-prompt branch >actual &&
+	test_cmp expect actual &&
 
 	# set merge.tool to something bogus, diff.tool to test-tool
-	git config merge.tool bogus-tool &&
-	git config diff.tool test-tool &&
-
-	diff=$(git difftool --no-prompt branch) &&
-	test "$diff" = "branch" &&
-
-	restore_test_defaults
+	test_config merge.tool bogus-tool &&
+	test_config diff.tool test-tool &&
+	git difftool --no-prompt branch >actual &&
+	test_cmp expect actual
 '
 
 test_expect_success PERL 'difftool.<tool>.path' '
-	git 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_config difftool.tkdiff.path echo &&
+	git difftool --tool=tkdiff --no-prompt branch >output &&
+	lines=$(grep file output | wc -l) &&
+	test "$lines" -eq 1
 '
 
 test_expect_success PERL 'difftool --extcmd=cat' '
-	diff=$(git difftool --no-prompt --extcmd=cat branch) &&
-	test "$diff" = branch"$LF"master
+	echo branch >expect &&
+	echo master >>expect &&
+	git difftool --no-prompt --extcmd=cat branch >actual &&
+	test_cmp expect actual
 '
 
 test_expect_success PERL 'difftool --extcmd cat' '
-	diff=$(git difftool --no-prompt --extcmd cat branch) &&
-	test "$diff" = branch"$LF"master
+	echo branch >expect &&
+	echo master >>expect &&
+	git difftool --no-prompt --extcmd=cat branch >actual &&
+	test_cmp expect actual
 '
 
 test_expect_success PERL 'difftool -x cat' '
-	diff=$(git difftool --no-prompt -x cat branch) &&
-	test "$diff" = branch"$LF"master
+	echo branch >expect &&
+	echo master >>expect &&
+	git difftool --no-prompt -x cat branch >actual &&
+	test_cmp expect actual
 '
 
 test_expect_success PERL 'difftool --extcmd echo arg1' '
-	diff=$(git difftool --no-prompt --extcmd sh\ -c\ \"echo\ \$1\" branch) &&
-	test "$diff" = file
+	echo file >expect &&
+	git difftool --no-prompt \
+		--extcmd sh\ -c\ \"echo\ \$1\" branch >actual &&
+	test_cmp expect actual
 '
 
 test_expect_success PERL 'difftool --extcmd cat arg1' '
-	diff=$(git difftool --no-prompt --extcmd sh\ -c\ \"cat\ \$1\" branch) &&
-	test "$diff" = master
+	echo master >expect &&
+	git difftool --no-prompt \
+		--extcmd sh\ -c\ \"cat\ \$1\" branch >actual &&
+	test_cmp expect actual
 '
 
 test_expect_success PERL 'difftool --extcmd cat arg2' '
-	diff=$(git difftool --no-prompt --extcmd sh\ -c\ \"cat\ \$2\" branch) &&
-	test "$diff" = branch
+	echo branch >expect &&
+	git difftool --no-prompt \
+		--extcmd sh\ -c\ \"cat\ \$2\" branch >actual &&
+	test_cmp expect actual
 '
 
 # Create a second file on master and a different version on branch

@@ -324,26 +294,26 @@ test_expect_success PERL 'setup with 2 files different' '
 '
 
 test_expect_success PERL 'say no to the first file' '
-	diff=$( (echo n; echo) | git difftool -x cat branch ) &&
-
-	echo "$diff" | stdin_contains m2 &&
-	echo "$diff" | stdin_contains br2 &&
-	echo "$diff" | stdin_doesnot_contain master &&
-	echo "$diff" | stdin_doesnot_contain branch
+	(echo n && echo) >input &&
+	git difftool -x cat branch <input >output &&
+	stdin_contains m2 <output &&
+	stdin_contains br2 <output &&
+	stdin_doesnot_contain master <output &&
+	stdin_doesnot_contain branch <output
 '
 
 test_expect_success PERL 'say no to the second file' '
-	diff=$( (echo; echo n) | git difftool -x cat branch ) &&
-
-	echo "$diff" | stdin_contains master &&
-	echo "$diff" | stdin_contains branch &&
-	echo "$diff" | stdin_doesnot_contain m2 &&
-	echo "$diff" | stdin_doesnot_contain br2
+	(echo && echo n) >input &&
+	git difftool -x cat branch <input >output &&
+	stdin_contains master <output &&
+	stdin_contains branch  <output &&
+	stdin_doesnot_contain m2 <output &&
+	stdin_doesnot_contain br2 <output
 '
 
 test_expect_success PERL 'difftool --tool-help' '
-	tool_help=$(git difftool --tool-help) &&
-	echo "$tool_help" | stdin_contains tool
+	git difftool --tool-help >output &&
+	stdin_contains tool <output
 '
 
 test_expect_success PERL 'setup change in subdirectory' '

@@ -359,29 +329,29 @@ test_expect_success PERL 'setup change in subdirectory' '
 '
 
 test_expect_success PERL 'difftool -d' '
-	diff=$(git difftool -d --extcmd ls branch) &&
-	echo "$diff" | stdin_contains sub &&
-	echo "$diff" | stdin_contains file
+	git difftool -d --extcmd ls branch >output &&
+	stdin_contains sub <output &&
+	stdin_contains file <output
 '
 
 test_expect_success PERL 'difftool --dir-diff' '
-	diff=$(git difftool --dir-diff --extcmd ls branch) &&
-	echo "$diff" | stdin_contains sub &&
-	echo "$diff" | stdin_contains file
+	git difftool --dir-diff --extcmd ls branch >output &&
+	stdin_contains sub <output &&
+	stdin_contains file <output
 '
 
 test_expect_success PERL 'difftool --dir-diff ignores --prompt' '
-	diff=$(git difftool --dir-diff --prompt --extcmd ls branch) &&
-	echo "$diff" | stdin_contains sub &&
-	echo "$diff" | stdin_contains file
+	git difftool --dir-diff --prompt --extcmd ls branch >output &&
+	stdin_contains sub <output &&
+	stdin_contains file <output
 '
 
 test_expect_success PERL 'difftool --dir-diff from subdirectory' '
 	(
 		cd sub &&
-		diff=$(git difftool --dir-diff --extcmd ls branch) &&
-		echo "$diff" | stdin_contains sub &&
-		echo "$diff" | stdin_contains file
+		git difftool --dir-diff --extcmd ls branch >output &&
+		stdin_contains sub <output &&
+		stdin_contains file <output
 	)
 '
 

-- 
1.8.2.rc0.20.gf548dd7

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 <redacted>
---
Rebased against PATCH v3 3/4.

 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 fb00273..21fbba9 100755
--- a/t/t7800-difftool.sh
+++ b/t/t7800-difftool.sh

@@ -60,9 +60,9 @@ test_expect_success PERL 'custom commands' '
 '
 
 test_expect_success PERL 'custom tool commands override built-ins' '
-	test_config difftool.defaults.cmd "cat \"\$REMOTE\"" &&
+	test_config difftool.vimdiff "cat \"\$REMOTE\"" &&
 	echo master >expect &&
-	git difftool --tool defaults --no-prompt branch >actual &&
+	git difftool --tool vimdiff --no-prompt branch >actual &&
 	test_cmp expect actual
 '
 

-- 
1.8.2.rc0.20.gf548dd7

Am 2/21/2013 5:03, schrieb David Aguilar:

 test_expect_success PERL 'difftool -d' '
-	diff=$(git difftool -d --extcmd ls branch) &&
-	echo "$diff" | stdin_contains sub &&
-	echo "$diff" | stdin_contains file
+	git difftool -d --extcmd ls branch >output &&
+	stdin_contains sub <output &&
+	stdin_contains file <output
 '

This test is broken on Windows. There is this code in git-difftool.perl

	for my $file (@worktree) {
...
			copy("$b/$file", "$workdir/$file") or
			exit_cleanup($tmpdir, 1);
...
	}

@worktree is populated with all files in the worktree. At this point,
"output" is among them. Then follows an attempt to copy a file over
"$workdir/$file". I guess that is some link+remove magic going on behind
the scenes. At any rate, this fails on Windows with
"D:/Src/mingw-git/t/trash directory.t7800-difftool/../../git-difftool line
408: Bad file number", because files that are open cannot be written from
outside (the file is open due to the redirection in the test snippet).

What is going on here? Why can this ever succeed even on Unix?

Same for some later tests.

BTW, while debugging this, I found the use of the helper function
stdin_contains() highly unhelpful; it just resolves to a 'grep' that on
top of all hides stdout. Please don't do that. Just use unadorned grep
like we do everywhere else.

-- Hannes

On Wed, Mar 20, 2013 at 2:48 AM, Johannes Sixt [off-list ref] wrote:

Am 2/21/2013 5:03, schrieb David Aguilar:

quoted

 test_expect_success PERL 'difftool -d' '
-     diff=$(git difftool -d --extcmd ls branch) &&
-     echo "$diff" | stdin_contains sub &&
-     echo "$diff" | stdin_contains file
+     git difftool -d --extcmd ls branch >output &&
+     stdin_contains sub <output &&
+     stdin_contains file <output
 '

This test is broken on Windows. There is this code in git-difftool.perl

        for my $file (@worktree) {
...
                        copy("$b/$file", "$workdir/$file") or
                        exit_cleanup($tmpdir, 1);
...
        }

@worktree is populated with all files in the worktree. At this point,
"output" is among them. Then follows an attempt to copy a file over
"$workdir/$file". I guess that is some link+remove magic going on behind
the scenes. At any rate, this fails on Windows with
"D:/Src/mingw-git/t/trash directory.t7800-difftool/../../git-difftool line
408: Bad file number", because files that are open cannot be written from
outside (the file is open due to the redirection in the test snippet).

What is going on here? Why can this ever succeed even on Unix?

Thanks for the report.  Yes, these do pass on Unix.

Hmm I wonder what's going on here?

I started digging in and the @worktree_files (aka @worktree above)
is populated from the output of "git diff --raw ...".

Seeing the "output" filename in "diff --raw" implies that one of the
tests added "output" to the index somehow.  I do not see that
happening anywhere, though, so I do not know how it would end up in
the @worktree array if it is not reported by "diff --raw".


My current understanding of how it could possibly be open twice:

1. via the >output redirect
2. via the copy() perl code which is fed by @worktree

So I'm confused.  Why would we get different results on Windows?

I just re-ran these tests from "next" to check my sanity and they
passed on both Linux and OS X.

Same for some later tests.

Ditto.

BTW, while debugging this, I found the use of the helper function
stdin_contains() highly unhelpful; it just resolves to a 'grep' that on
top of all hides stdout. Please don't do that. Just use unadorned grep
like we do everywhere else.

I'm not too opposed to that.
The one small advantage to the helper is that you can tweak the redirect
in one central place, so it's not all for naught.

Sitaram, you added this back in:

ba959de1 git-difftool: allow skipping file by typing 'n' at prompt

Do you have any thoughts?

It seems like removing the stdout redirect could be helpful for debugging,
and if we did that then there's really no point in having the helper
(aside from the indirection which can sometimes help during debugging).
I don't really feel too strongly either way, but it did bother you
while debugging the test script, so unadorned grep seems like the way to go.

I'll wait and see if anybody else has any Windows-specific clues that
we can use to narrow down this problem.

In lieu of an immediate fix, are there any test prerequisite we can
use to skip these tests on windows?  One or both of NOT_CYGWIN,NOT_MINGW?
-- 
David

Am 3/20/2013 23:59, schrieb David Aguilar:

I started digging in and the @worktree_files (aka @worktree above)
is populated from the output of "git diff --raw ...".

Seeing the "output" filename in "diff --raw" implies that one of the
tests added "output" to the index somehow.  I do not see that
happening anywhere, though, so I do not know how it would end up in
the @worktree array if it is not reported by "diff --raw".


My current understanding of how it could possibly be open twice:

1. via the >output redirect
2. via the copy() perl code which is fed by @worktree

So I'm confused.  Why would we get different results on Windows?

I tracked down the difference between Windows and Linux, and it is...

	for my $file (@worktree) {
		next if $symlinks && -l "$b/$file";

... this line in sub dir_diff. On Linux, we take the short-cut, but on
Windows we proceed through the rest of the loop, which ultimately finds a
difference here:

		my $diff = compare("$b/$file", "$workdir/$file");

and attempts to copy a file here:

			copy("$b/$file", "$workdir/$file") or

where one of the files is the locked "output" file.

I don't know how essential symlinks are for the operation of git-difftool
and whether something can be done about it. The immediate fix is
apparently to protect the tests with SYMLINKS.

-- Hannes

Am 3/21/2013 8:41, schrieb Johannes Sixt:

Am 3/20/2013 23:59, schrieb David Aguilar:

quoted

I started digging in and the @worktree_files (aka @worktree above)
is populated from the output of "git diff --raw ...".

Seeing the "output" filename in "diff --raw" implies that one of the
tests added "output" to the index somehow.  I do not see that
happening anywhere, though, so I do not know how it would end up in
the @worktree array if it is not reported by "diff --raw".


My current understanding of how it could possibly be open twice:

1. via the >output redirect
2. via the copy() perl code which is fed by @worktree

So I'm confused.  Why would we get different results on Windows?

I tracked down the difference between Windows and Linux, and it is...

	for my $file (@worktree) {
		next if $symlinks && -l "$b/$file";

... this line in sub dir_diff. On Linux, we take the short-cut, but on
Windows we proceed through the rest of the loop,

And that is likely by design. From the docs:

--symlinks
--no-symlinks

    git difftool's default behavior is create symlinks to the working
    tree when run in --dir-diff mode.

    Specifying `--no-symlinks` instructs 'git difftool' to create
    copies instead.  `--no-symlinks` is the default on Windows.

And indeed, we have this initialization:

	my %opts = (
		...
		symlinks => $^O ne 'cygwin' &&
				$^O ne 'MSWin32' && $^O ne 'msys',
		...
	);

Can the --dir-diff tests case pass on Cygwin when neither --symlinks nor
--no-symlinks is passed?

Perhaps the right solution is this:

diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
index c6d6b1c..19238f6 100755
--- a/t/t7800-difftool.sh
+++ b/t/t7800-difftool.sh

@@ -328,14 +328,16 @@ test_expect_success PERL 'setup change in subdirectory' '
 	git commit -m "modified both"
 '
 
-test_expect_success PERL 'difftool -d' '
-	git difftool -d --extcmd ls branch >output &&
+# passing --symlinks helps Cygwin, which defaults to --no-symlinks
+
+test_expect_success PERL,SYMLINKS 'difftool -d' '
+	git difftool -d --symlinks --extcmd ls branch >output &&
 	stdin_contains sub <output &&
 	stdin_contains file <output
 '
 
-test_expect_success PERL 'difftool --dir-diff' '
-	git difftool --dir-diff --extcmd ls branch >output &&
+test_expect_success PERL,SYMLINKS 'difftool --dir-diff' '
+	git difftool --dir-diff --symlinks --extcmd ls branch >output &&
 	stdin_contains sub <output &&
 	stdin_contains file <output
 '

@@ -362,16 +364,16 @@ test_expect_success PERL,SYMLINKS 'difftool --dir-diff --symlink without unstage
 	test_cmp actual expect
 '
 
-test_expect_success PERL 'difftool --dir-diff ignores --prompt' '
-	git difftool --dir-diff --prompt --extcmd ls branch >output &&
+test_expect_success PERL,SYMLINKS 'difftool --dir-diff ignores --prompt' '
+	git difftool --dir-diff --symlinks --prompt --extcmd ls branch >output &&
 	stdin_contains sub <output &&
 	stdin_contains file <output
 '
 
-test_expect_success PERL 'difftool --dir-diff from subdirectory' '
+test_expect_success PERL,SYMLINKS 'difftool --dir-diff from subdirectory' '
 	(
 		cd sub &&
-		git difftool --dir-diff --extcmd ls branch >output &&
+		git difftool --dir-diff --symlinks --extcmd ls branch >output &&
 		stdin_contains sub <output &&
 		stdin_contains file <output
 	)

(Only tested on MinGW, which skips the tests.) I leave it to you
to write --no-symlinks tests.

-- Hannes

On Fri, Mar 22, 2013 at 08:13:46AM +0100, Johannes Sixt wrote:

Am 3/21/2013 8:41, schrieb Johannes Sixt:

quoted

Am 3/20/2013 23:59, schrieb David Aguilar:

quoted

I started digging in and the @worktree_files (aka @worktree above)
is populated from the output of "git diff --raw ...".

Seeing the "output" filename in "diff --raw" implies that one of the
tests added "output" to the index somehow.  I do not see that
happening anywhere, though, so I do not know how it would end up in
the @worktree array if it is not reported by "diff --raw".


My current understanding of how it could possibly be open twice:

1. via the >output redirect
2. via the copy() perl code which is fed by @worktree

So I'm confused.  Why would we get different results on Windows?

I tracked down the difference between Windows and Linux, and it is...

	for my $file (@worktree) {
		next if $symlinks && -l "$b/$file";

... this line in sub dir_diff. On Linux, we take the short-cut, but on
Windows we proceed through the rest of the loop,

And that is likely by design. From the docs:

--symlinks
--no-symlinks

    git difftool's default behavior is create symlinks to the working
    tree when run in --dir-diff mode.

    Specifying `--no-symlinks` instructs 'git difftool' to create
    copies instead.  `--no-symlinks` is the default on Windows.

And indeed, we have this initialization:

	my %opts = (
		...
		symlinks => $^O ne 'cygwin' &&
				$^O ne 'MSWin32' && $^O ne 'msys',
		...
	);

Can the --dir-diff tests case pass on Cygwin when neither --symlinks nor
--no-symlinks is passed?

Perhaps the right solution is this:

We already have tests that explicitly pass '--symlinks'.  I wonder if it
would be better to change "output" to ".git/output", which should avoid
the problem by moving the output file out of the working tree.

Am 3/22/2013 11:00, schrieb John Keeping:

On Fri, Mar 22, 2013 at 08:13:46AM +0100, Johannes Sixt wrote:

quoted

Am 3/21/2013 8:41, schrieb Johannes Sixt:

quoted

Am 3/20/2013 23:59, schrieb David Aguilar:

quoted

I started digging in and the @worktree_files (aka @worktree above)
is populated from the output of "git diff --raw ...".

Seeing the "output" filename in "diff --raw" implies that one of the
tests added "output" to the index somehow.  I do not see that
happening anywhere, though, so I do not know how it would end up in
the @worktree array if it is not reported by "diff --raw".


My current understanding of how it could possibly be open twice:

1. via the >output redirect
2. via the copy() perl code which is fed by @worktree

So I'm confused.  Why would we get different results on Windows?

I tracked down the difference between Windows and Linux, and it is...

	for my $file (@worktree) {
		next if $symlinks && -l "$b/$file";

... this line in sub dir_diff. On Linux, we take the short-cut, but on
Windows we proceed through the rest of the loop,

And that is likely by design. From the docs:

--symlinks
--no-symlinks

    git difftool's default behavior is create symlinks to the working
    tree when run in --dir-diff mode.

    Specifying `--no-symlinks` instructs 'git difftool' to create
    copies instead.  `--no-symlinks` is the default on Windows.

And indeed, we have this initialization:

	my %opts = (
		...
		symlinks => $^O ne 'cygwin' &&
				$^O ne 'MSWin32' && $^O ne 'msys',
		...
	);

Can the --dir-diff tests case pass on Cygwin when neither --symlinks nor
--no-symlinks is passed?

Perhaps the right solution is this:

We already have tests that explicitly pass '--symlinks'.  I wonder if it
would be better to change "output" to ".git/output", which should avoid
the problem by moving the output file out of the working tree.

The point of using --symlinks is not to test the effect of the option, but
to test the same thing on Unix and on Cygwin, because the latter uses
--no-symlinks by default. Therefore, I think that this sketch is the right
thing to do.

But the real problem seems to be that "output" should not be among the
files treated in the cited pieces of code (unless I'm wrong, of course; I
know next to nothing about git-difftool). It should not matter where the
file lives. Just add --no-symlinks to the difftool invocation of test
"difftool -d" and watch it fail on Linux, too.

-- Hannes

On Fri, Mar 22, 2013 at 12:14:27PM +0100, Johannes Sixt wrote:

Am 3/22/2013 11:00, schrieb John Keeping:

quoted

On Fri, Mar 22, 2013 at 08:13:46AM +0100, Johannes Sixt wrote:

quoted

Am 3/21/2013 8:41, schrieb Johannes Sixt:

quoted

Am 3/20/2013 23:59, schrieb David Aguilar:

quoted

I started digging in and the @worktree_files (aka @worktree above)
is populated from the output of "git diff --raw ...".

Seeing the "output" filename in "diff --raw" implies that one of the
tests added "output" to the index somehow.  I do not see that
happening anywhere, though, so I do not know how it would end up in
the @worktree array if it is not reported by "diff --raw".


My current understanding of how it could possibly be open twice:

1. via the >output redirect
2. via the copy() perl code which is fed by @worktree

So I'm confused.  Why would we get different results on Windows?

I tracked down the difference between Windows and Linux, and it is...

	for my $file (@worktree) {
		next if $symlinks && -l "$b/$file";

... this line in sub dir_diff. On Linux, we take the short-cut, but on
Windows we proceed through the rest of the loop,

And that is likely by design. From the docs:

--symlinks
--no-symlinks

    git difftool's default behavior is create symlinks to the working
    tree when run in --dir-diff mode.

    Specifying `--no-symlinks` instructs 'git difftool' to create
    copies instead.  `--no-symlinks` is the default on Windows.

And indeed, we have this initialization:

	my %opts = (
		...
		symlinks => $^O ne 'cygwin' &&
				$^O ne 'MSWin32' && $^O ne 'msys',
		...
	);

Can the --dir-diff tests case pass on Cygwin when neither --symlinks nor
--no-symlinks is passed?

Perhaps the right solution is this:

We already have tests that explicitly pass '--symlinks'.  I wonder if it
would be better to change "output" to ".git/output", which should avoid
the problem by moving the output file out of the working tree.

The point of using --symlinks is not to test the effect of the option, but
to test the same thing on Unix and on Cygwin, because the latter uses
--no-symlinks by default. Therefore, I think that this sketch is the right
thing to do.

So shouldn't we be running all of the tests with both --symlinks and
--no-symlinks (except those which explicitly check that these options do
the right thing)?

But the real problem seems to be that "output" should not be among the
files treated in the cited pieces of code (unless I'm wrong, of course; I
know next to nothing about git-difftool). It should not matter where the
file lives. Just add --no-symlinks to the difftool invocation of test
"difftool -d" and watch it fail on Linux, too.

I fired up strace and it looks like difftool sees it as a working tree
file (i.e. the working tree content matches the RHS of the diff) in the
output of "git diff --raw --no-abbrev -z branch" and copies it over to
the temporary directories.  Then when difftool completes it copies back
the working tree files from there.

When the file is copied to the temporary directory, the diff command
hasn't yet run so "output" is empty.  When it's copied back it is after
the "ls" has run and so we overwrite the output of the command with the
original empty file.

So sticking the output file under .git does solve this issue since it
then is not treated as a working tree file.

Whether the current behaviour of difftool is entirely sensible is a
different question.  I think we should at the very least by refusing to
overwrite working tree files that have been modified since they were
copied to the temporary directory

If I ever end up on a system using --no-symlinks I can see myself being
bitten by this by editing the original working tree file while
inspecting the diff in a separate window.  I suspect this is just as
common a workflow as people editing the file in their diff tool and
wanting the changes copied back to the working tree.


John

How about doing this?

The first patch is a cleanup as suggested by Johannes[1], the second
fixes the test failure on Windows and the third makes the test behaviour
more explicit and would have helped to detect this issue earlier.

  [1/3] t7800: don't hide grep output
  [2/3] t7800: fix tests when difftool uses --no-symlinks
  [3/3] t7800: run --dir-diff tests with and without symlinks

 t/t7800-difftool.sh | 71 +++++++++++++++++++++++++++--------------------------
 1 file changed, 36 insertions(+), 35 deletions(-)

-- 
1.8.2.324.ga64ebd9

Remove the stdin_contains and stdin_doesnt_contain helper functions
which add nothing but hide the output of grep, hurting debugging.

Suggested-by: Johannes Sixt <redacted>
Signed-off-by: John Keeping <redacted>
---
 t/t7800-difftool.sh | 44 +++++++++++++++++---------------------------
 1 file changed, 17 insertions(+), 27 deletions(-)

diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
index 3aab6e1..e694972 100755
--- a/t/t7800-difftool.sh
+++ b/t/t7800-difftool.sh

@@ -23,16 +23,6 @@ prompt_given ()
 	test "$prompt" = "Launch 'test-tool' [Y/n]: branch"
 }
 
-stdin_contains ()
-{
-	grep >/dev/null "$1"
-}
-
-stdin_doesnot_contain ()
-{
-	! stdin_contains "$1"
-}
-
 # Create a file on master and change it on branch
 test_expect_success PERL 'setup' '
 	echo master >file &&

@@ -296,24 +286,24 @@ test_expect_success PERL 'setup with 2 files different' '
 test_expect_success PERL 'say no to the first file' '
 	(echo n && echo) >input &&
 	git difftool -x cat branch <input >output &&
-	stdin_contains m2 <output &&
-	stdin_contains br2 <output &&
-	stdin_doesnot_contain master <output &&
-	stdin_doesnot_contain branch <output
+	grep m2 output &&
+	grep br2 output &&
+	! grep master output &&
+	! grep branch output
 '
 
 test_expect_success PERL 'say no to the second file' '
 	(echo && echo n) >input &&
 	git difftool -x cat branch <input >output &&
-	stdin_contains master <output &&
-	stdin_contains branch  <output &&
-	stdin_doesnot_contain m2 <output &&
-	stdin_doesnot_contain br2 <output
+	grep master output &&
+	grep branch output &&
+	! grep m2 output &&
+	! grep br2 output
 '
 
 test_expect_success PERL 'difftool --tool-help' '
 	git difftool --tool-help >output &&
-	stdin_contains tool <output
+	grep tool output
 '
 
 test_expect_success PERL 'setup change in subdirectory' '

@@ -330,28 +320,28 @@ test_expect_success PERL 'setup change in subdirectory' '
 
 test_expect_success PERL 'difftool -d' '
 	git difftool -d --extcmd ls branch >output &&
-	stdin_contains sub <output &&
-	stdin_contains file <output
+	grep sub output &&
+	grep file output
 '
 
 test_expect_success PERL 'difftool --dir-diff' '
 	git difftool --dir-diff --extcmd ls branch >output &&
-	stdin_contains sub <output &&
-	stdin_contains file <output
+	grep sub output &&
+	grep file output
 '
 
 test_expect_success PERL 'difftool --dir-diff ignores --prompt' '
 	git difftool --dir-diff --prompt --extcmd ls branch >output &&
-	stdin_contains sub <output &&
-	stdin_contains file <output
+	grep sub output &&
+	grep file output
 '
 
 test_expect_success PERL 'difftool --dir-diff from subdirectory' '
 	(
 		cd sub &&
 		git difftool --dir-diff --extcmd ls branch >output &&
-		stdin_contains sub <output &&
-		stdin_contains file <output
+		grep sub output &&
+		grep file output
 	)
 '
 

-- 
1.8.2.324.ga64ebd9

When 'git difftool --dir-diff' is using --no-symlinks (either explicitly
or implicitly because it's running on Windows), any working tree files
that have been copied to the temporary directory are copied back after
the difftool completes.  This includes untracked files in the working
tree.

During the tests, this means that the following sequence occurs:

1) the shell opens "output" to redirect the difftool output
2) difftool copies the empty "output" to the temporary directory
3) difftool runs "ls" which writes to "output"
4) difftool copies the empty "output" file back over the output of the
   command
5) the output files doesn't contain the expected output, causing the
   test to fail

Avoid this by writing the output into .git/ which will not be copied or
overwritten.

In the longer term, difftool probably needs to learn to warn the user
instead of overwrite any changes that have been made to the working tree
file.

Signed-off-by: John Keeping <redacted>
---
 t/t7800-difftool.sh | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
index e694972..1eed439 100755
--- a/t/t7800-difftool.sh
+++ b/t/t7800-difftool.sh

@@ -319,29 +319,29 @@ test_expect_success PERL 'setup change in subdirectory' '
 '
 
 test_expect_success PERL 'difftool -d' '
-	git difftool -d --extcmd ls branch >output &&
-	grep sub output &&
-	grep file output
+	git difftool -d --extcmd ls branch >.git/output &&
+	grep sub .git/output &&
+	grep file .git/output
 '
 
 test_expect_success PERL 'difftool --dir-diff' '
-	git difftool --dir-diff --extcmd ls branch >output &&
-	grep sub output &&
-	grep file output
+	git difftool --dir-diff --extcmd ls branch >.git/output &&
+	grep sub .git/output &&
+	grep file .git/output
 '
 
 test_expect_success PERL 'difftool --dir-diff ignores --prompt' '
-	git difftool --dir-diff --prompt --extcmd ls branch >output &&
-	grep sub output &&
-	grep file output
+	git difftool --dir-diff --prompt --extcmd ls branch >.git/output &&
+	grep sub .git/output &&
+	grep file .git/output
 '
 
 test_expect_success PERL 'difftool --dir-diff from subdirectory' '
 	(
 		cd sub &&
-		git difftool --dir-diff --extcmd ls branch >output &&
-		grep sub output &&
-		grep file output
+		git difftool --dir-diff --extcmd ls branch >../.git/output &&
+		grep sub ../.git/output &&
+		grep file ../.git/output
 	)
 '
 

-- 
1.8.2.324.ga64ebd9

Currently the difftool --dir-diff tests may or may not use symlinks
depending on the operating system on which they are run.  In one case
this has caused a test failure to be noticed only on Windows when the
test also fails on Linux when difftool is invoked with --no-symlinks.

Rewrite these tests so that they do not depend on the environment but
run explicitly with both --symlinks and --no-symlinks, protecting the
--symlinks version with a SYMLINKS prerequisite.

Signed-off-by: John Keeping <redacted>
---
 t/t7800-difftool.sh | 19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
index 1eed439..4a70508 100755
--- a/t/t7800-difftool.sh
+++ b/t/t7800-difftool.sh

@@ -318,25 +318,36 @@ test_expect_success PERL 'setup change in subdirectory' '
 	git commit -m "modified both"
 '
 
-test_expect_success PERL 'difftool -d' '
+run_dir_diff_test () {
+	test_expect_success PERL "$1 --no-symlinks" "
+		symlinks=--no-symlinks
+		$2
+	"
+	test_expect_success PERL,SYMLINKS "$1 --symlinks" "
+		symlinks=--symlinks
+		$2
+	"
+}
+
+run_dir_diff_test 'difftool -d' '
 	git difftool -d --extcmd ls branch >.git/output &&
 	grep sub .git/output &&
 	grep file .git/output
 '
 
-test_expect_success PERL 'difftool --dir-diff' '
+run_dir_diff_test 'difftool --dir-diff' '
 	git difftool --dir-diff --extcmd ls branch >.git/output &&
 	grep sub .git/output &&
 	grep file .git/output
 '
 
-test_expect_success PERL 'difftool --dir-diff ignores --prompt' '
+run_dir_diff_test 'difftool --dir-diff ignores --prompt' '
 	git difftool --dir-diff --prompt --extcmd ls branch >.git/output &&
 	grep sub .git/output &&
 	grep file .git/output
 '
 
-test_expect_success PERL 'difftool --dir-diff from subdirectory' '
+run_dir_diff_test 'difftool --dir-diff from subdirectory' '
 	(
 		cd sub &&
 		git difftool --dir-diff --extcmd ls branch >../.git/output &&

-- 
1.8.2.324.ga64ebd9

Currently the difftool --dir-diff tests may or may not use symlinks
depending on the operating system on which they are run.  In one case
this has caused a test failure to be noticed only on Windows when the
test also fails on Linux when difftool is invoked with --no-symlinks.

Rewrite these tests so that they do not depend on the environment but
run explicitly with both --symlinks and --no-symlinks, protecting the
--symlinks version with a SYMLINKS prerequisite.

Signed-off-by: John Keeping <redacted>
---
The previous version of this was missing half the intended change :-(
Sorry for the noise.

 t/t7800-difftool.sh | 27 +++++++++++++++++++--------
 1 file changed, 19 insertions(+), 8 deletions(-)

diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
index 1eed439..bba8a9d 100755
--- a/t/t7800-difftool.sh
+++ b/t/t7800-difftool.sh

@@ -318,28 +318,39 @@ test_expect_success PERL 'setup change in subdirectory' '
 	git commit -m "modified both"
 '
 
-test_expect_success PERL 'difftool -d' '
-	git difftool -d --extcmd ls branch >.git/output &&
+run_dir_diff_test () {
+	test_expect_success PERL "$1 --no-symlinks" "
+		symlinks=--no-symlinks
+		$2
+	"
+	test_expect_success PERL,SYMLINKS "$1 --symlinks" "
+		symlinks=--symlinks
+		$2
+	"
+}
+
+run_dir_diff_test 'difftool -d' '
+	git difftool -d $symlinks --extcmd ls branch >.git/output &&
 	grep sub .git/output &&
 	grep file .git/output
 '
 
-test_expect_success PERL 'difftool --dir-diff' '
-	git difftool --dir-diff --extcmd ls branch >.git/output &&
+run_dir_diff_test 'difftool --dir-diff' '
+	git difftool --dir-diff $symlinks --extcmd ls branch >.git/output &&
 	grep sub .git/output &&
 	grep file .git/output
 '
 
-test_expect_success PERL 'difftool --dir-diff ignores --prompt' '
-	git difftool --dir-diff --prompt --extcmd ls branch >.git/output &&
+run_dir_diff_test 'difftool --dir-diff ignores --prompt' '
+	git difftool --dir-diff $symlinks --prompt --extcmd ls branch >.git/output &&
 	grep sub .git/output &&
 	grep file .git/output
 '
 
-test_expect_success PERL 'difftool --dir-diff from subdirectory' '
+run_dir_diff_test 'difftool --dir-diff from subdirectory' '
 	(
 		cd sub &&
-		git difftool --dir-diff --extcmd ls branch >../.git/output &&
+		git difftool --dir-diff $symlinks --extcmd ls branch >../.git/output &&
 		grep sub ../.git/output &&
 		grep file ../.git/output
 	)

-- 
1.8.2.324.ga64ebd9

Am 22.03.2013 20:36, schrieb John Keeping:

When 'git difftool --dir-diff' is using --no-symlinks (either explicitly
or implicitly because it's running on Windows), any working tree files
that have been copied to the temporary directory are copied back after
the difftool completes.  This includes untracked files in the working
tree.

During the tests, this means that the following sequence occurs:

1) the shell opens "output" to redirect the difftool output
2) difftool copies the empty "output" to the temporary directory

But this should not happen, should it?

3) difftool runs "ls" which writes to "output"
4) difftool copies the empty "output" file back over the output of the
   command
5) the output files doesn't contain the expected output, causing the
   test to fail

Avoid this by writing the output into .git/ which will not be copied or
overwritten.

Isn't this just painting over the bug that "output" is incorrectly copied?

In the longer term, difftool probably needs to learn to warn the user
instead of overwrite any changes that have been made to the working tree
file.

Sure, but this is an independent issue.

Am 22.03.2013 20:36, schrieb John Keeping:

Remove the stdin_contains and stdin_doesnt_contain helper functions
which add nothing but hide the output of grep, hurting debugging.

Thanks. Patch looks good.

-- Hannes

This version fixes the actual cause of the test failure (not being
specific enough when adding files).  This is done with a new version of
patch 2.

Patch 1 is unchanged and patch 3 only contains a minor change.

This is built on da/difftool-fixes.  There may be a small textual
conflict with jk/difftool-dir-diff-edit-fix in the context lines but I
don't believe there is any logical conflict.

John Keeping (3):
  t7800: don't hide grep output
  t7800: fix tests when difftool uses --no-symlinks
  t7800: run --dir-diff tests with and without symlinks

 t/t7800-difftool.sh | 73 +++++++++++++++++++++++++++--------------------------
 1 file changed, 37 insertions(+), 36 deletions(-)

-- 
1.8.2.324.ga64ebd9

Remove the stdin_contains and stdin_doesnt_contain helper functions
which add nothing but hide the output of grep, hurting debugging.

Suggested-by: Johannes Sixt <redacted>
Signed-off-by: John Keeping <redacted>
---
 t/t7800-difftool.sh | 44 +++++++++++++++++---------------------------
 1 file changed, 17 insertions(+), 27 deletions(-)

diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
index 3aab6e1..e694972 100755
--- a/t/t7800-difftool.sh
+++ b/t/t7800-difftool.sh

@@ -23,16 +23,6 @@ prompt_given ()
 	test "$prompt" = "Launch 'test-tool' [Y/n]: branch"
 }
 
-stdin_contains ()
-{
-	grep >/dev/null "$1"
-}
-
-stdin_doesnot_contain ()
-{
-	! stdin_contains "$1"
-}
-
 # Create a file on master and change it on branch
 test_expect_success PERL 'setup' '
 	echo master >file &&

@@ -296,24 +286,24 @@ test_expect_success PERL 'setup with 2 files different' '
 test_expect_success PERL 'say no to the first file' '
 	(echo n && echo) >input &&
 	git difftool -x cat branch <input >output &&
-	stdin_contains m2 <output &&
-	stdin_contains br2 <output &&
-	stdin_doesnot_contain master <output &&
-	stdin_doesnot_contain branch <output
+	grep m2 output &&
+	grep br2 output &&
+	! grep master output &&
+	! grep branch output
 '
 
 test_expect_success PERL 'say no to the second file' '
 	(echo && echo n) >input &&
 	git difftool -x cat branch <input >output &&
-	stdin_contains master <output &&
-	stdin_contains branch  <output &&
-	stdin_doesnot_contain m2 <output &&
-	stdin_doesnot_contain br2 <output
+	grep master output &&
+	grep branch output &&
+	! grep m2 output &&
+	! grep br2 output
 '
 
 test_expect_success PERL 'difftool --tool-help' '
 	git difftool --tool-help >output &&
-	stdin_contains tool <output
+	grep tool output
 '
 
 test_expect_success PERL 'setup change in subdirectory' '

@@ -330,28 +320,28 @@ test_expect_success PERL 'setup change in subdirectory' '
 
 test_expect_success PERL 'difftool -d' '
 	git difftool -d --extcmd ls branch >output &&
-	stdin_contains sub <output &&
-	stdin_contains file <output
+	grep sub output &&
+	grep file output
 '
 
 test_expect_success PERL 'difftool --dir-diff' '
 	git difftool --dir-diff --extcmd ls branch >output &&
-	stdin_contains sub <output &&
-	stdin_contains file <output
+	grep sub output &&
+	grep file output
 '
 
 test_expect_success PERL 'difftool --dir-diff ignores --prompt' '
 	git difftool --dir-diff --prompt --extcmd ls branch >output &&
-	stdin_contains sub <output &&
-	stdin_contains file <output
+	grep sub output &&
+	grep file output
 '
 
 test_expect_success PERL 'difftool --dir-diff from subdirectory' '
 	(
 		cd sub &&
 		git difftool --dir-diff --extcmd ls branch >output &&
-		stdin_contains sub <output &&
-		stdin_contains file <output
+		grep sub output &&
+		grep file output
 	)
 '
 

-- 
1.8.2.324.ga64ebd9

When 'git difftool --dir-diff' is using --no-symlinks (either explicitly
or implicitly because it's running on Windows), any working tree files
that have been copied to the temporary directory are copied back after
the difftool completes.

Because an earlier test uses "git add .", the "output" file used by
tests is tracked by Git and the following sequence occurs during some
tests:

1) the shell opens "output" to redirect the difftool output
2) difftool copies the empty "output" to the temporary directory
3) difftool runs "ls" which writes to "output"
4) difftool copies the empty "output" file back over the output of the
   command
5) the output files doesn't contain the expected output, causing the
   test to fail

Instead of adding all changes, explicitly add only the files that the
test is using, allowing later tests to write their result files into the
working tree.

In the longer term, difftool probably needs to learn to warn the user
instead of overwrite any changes that have been made to the working tree
file.

Signed-off-by: John Keeping <redacted>

---
Changes since v1:
- Fix the actual cause of the issue in the test instead of masking it by
  moving the output file under .git/

 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 e694972..a0b8042 100755
--- a/t/t7800-difftool.sh
+++ b/t/t7800-difftool.sh

@@ -314,7 +314,7 @@ test_expect_success PERL 'setup change in subdirectory' '
 	git commit -m "added sub/sub" &&
 	echo test >>file &&
 	echo test >>sub/sub &&
-	git add . &&
+	git add file sub/sub &&
 	git commit -m "modified both"
 '
 

-- 
1.8.2.324.ga64ebd9

Currently the difftool --dir-diff tests may or may not use symlinks
depending on the operating system on which they are run.  In one case
this has caused a test failure to be noticed only on Windows when the
test also fails on Linux when difftool is invoked with --no-symlinks.

Rewrite these tests so that they do not depend on the environment but
run explicitly with both --symlinks and --no-symlinks, protecting the
--symlinks version with a SYMLINKS prerequisite.

Signed-off-by: John Keeping <redacted>

---
Changes since v1:
- &&-chain from the symlinks=... line

 t/t7800-difftool.sh | 27 +++++++++++++++++++--------
 1 file changed, 19 insertions(+), 8 deletions(-)

diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
index a0b8042..398e033 100755
--- a/t/t7800-difftool.sh
+++ b/t/t7800-difftool.sh

@@ -318,28 +318,39 @@ test_expect_success PERL 'setup change in subdirectory' '
 	git commit -m "modified both"
 '
 
-test_expect_success PERL 'difftool -d' '
-	git difftool -d --extcmd ls branch >output &&
+run_dir_diff_test () {
+	test_expect_success PERL "$1 --no-symlinks" "
+		symlinks=--no-symlinks &&
+		$2
+	"
+	test_expect_success PERL,SYMLINKS "$1 --symlinks" "
+		symlinks=--symlinks &&
+		$2
+	"
+}
+
+run_dir_diff_test 'difftool -d' '
+	git difftool -d $symlinks --extcmd ls branch >output &&
 	grep sub output &&
 	grep file output
 '
 
-test_expect_success PERL 'difftool --dir-diff' '
-	git difftool --dir-diff --extcmd ls branch >output &&
+run_dir_diff_test 'difftool --dir-diff' '
+	git difftool --dir-diff $symlinks --extcmd ls branch >output &&
 	grep sub output &&
 	grep file output
 '
 
-test_expect_success PERL 'difftool --dir-diff ignores --prompt' '
-	git difftool --dir-diff --prompt --extcmd ls branch >output &&
+run_dir_diff_test 'difftool --dir-diff ignores --prompt' '
+	git difftool --dir-diff $symlinks --prompt --extcmd ls branch >output &&
 	grep sub output &&
 	grep file output
 '
 
-test_expect_success PERL 'difftool --dir-diff from subdirectory' '
+run_dir_diff_test 'difftool --dir-diff from subdirectory' '
 	(
 		cd sub &&
-		git difftool --dir-diff --extcmd ls branch >output &&
+		git difftool --dir-diff $symlinks --extcmd ls branch >output &&
 		grep sub output &&
 		grep file output
 	)

-- 
1.8.2.324.ga64ebd9

On Sat, Mar 23, 2013 at 9:31 AM, John Keeping [off-list ref] wrote:

1) the shell opens "output" to redirect the difftool output
2) difftool copies the empty "output" to the temporary directory
3) difftool runs "ls" which writes to "output"
4) difftool copies the empty "output" file back over the output of the
   command
5) the output files doesn't contain the expected output, causing the

s/files doesn't/file doesn't/

   test to fail

Instead of adding all changes, explicitly add only the files that the
test is using, allowing later tests to write their result files into the
working tree.

In the longer term, difftool probably needs to learn to warn the user
instead of overwrite any changes that have been made to the working tree

s/overwrite/overwriting/

file.

Signed-off-by: John Keeping <redacted>

The series looks good, but I can't test it because it does not apply
anywhere here.

Am 3/23/2013 14:31, schrieb John Keeping:

Currently the difftool --dir-diff tests may or may not use symlinks
depending on the operating system on which they are run.  In one case
this has caused a test failure to be noticed only on Windows when the
test also fails on Linux when difftool is invoked with --no-symlinks.

Rewrite these tests so that they do not depend on the environment but
run explicitly with both --symlinks and --no-symlinks, protecting the
--symlinks version with a SYMLINKS prerequisite.

At first, I wondered what the point of having --symlinks and --no-symlinks
was when there is no discernable difference. But 1f229345 (difftool: Use
symlinks when diffing against the worktree) makes it pretty clear: It's an
optimization, and --no-symlinks is only intended as an escape hatch.

-- Hannes

On Mon, Mar 25, 2013 at 08:26:52AM +0100, Johannes Sixt wrote:

The series looks good, but I can't test it because it does not apply
anywhere here.

It's built on top of da/difftool-fixes, is there some problem that stops
it applying cleanly on top of that?

Am 3/23/2013 14:31, schrieb John Keeping:

quoted

Currently the difftool --dir-diff tests may or may not use symlinks
depending on the operating system on which they are run.  In one case
this has caused a test failure to be noticed only on Windows when the
test also fails on Linux when difftool is invoked with --no-symlinks.

Rewrite these tests so that they do not depend on the environment but
run explicitly with both --symlinks and --no-symlinks, protecting the
--symlinks version with a SYMLINKS prerequisite.

At first, I wondered what the point of having --symlinks and --no-symlinks
was when there is no discernable difference. But 1f229345 (difftool: Use
symlinks when diffing against the worktree) makes it pretty clear: It's an
optimization, and --no-symlinks is only intended as an escape hatch.

Am 3/25/2013 11:35, schrieb John Keeping:

On Mon, Mar 25, 2013 at 08:26:52AM +0100, Johannes Sixt wrote:

quoted

The series looks good, but I can't test it because it does not apply
anywhere here.

It's built on top of da/difftool-fixes, is there some problem that stops
it applying cleanly on top of that?

Thanks. I had only tried trees that were "contaminated" by
jk/difftool-dir-diff-edit-fix, which is in conflict with da/difftool-fixes.

t7800 passes on Windows with these patches.

-- Hannes

On Mon, Mar 25, 2013 at 11:59:12AM +0100, Johannes Sixt wrote:

Am 3/25/2013 11:35, schrieb John Keeping:

quoted

On Mon, Mar 25, 2013 at 08:26:52AM +0100, Johannes Sixt wrote:

quoted

The series looks good, but I can't test it because it does not apply
anywhere here.

It's built on top of da/difftool-fixes, is there some problem that stops
it applying cleanly on top of that?

Thanks. I had only tried trees that were "contaminated" by
jk/difftool-dir-diff-edit-fix, which is in conflict with da/difftool-fixes.

I think they merge OK, but I suspect git-am won't apply the patches
cleanly on top of the result.

t7800 passes on Windows with these patches.

Thanks.