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

[PATCH v3 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>
---
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

[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

[PATCH v3 3/4] t7800: modernize tests

[David Aguilar]

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 <davvid@gmail.com>
---
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

[PATCH v3 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>
---
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

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

[Junio C Hamano]

Thanks; will replace and queue.

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

[Junio C Hamano]

David Aguilar <davvid@gmail.com> writes:

> 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
>  '

Eek.

$ sh t7800-difftool.sh -i
ok 1 - setup
ok 2 - custom commands
not ok 3 - custom tool commands override built-ins
#
#               test_config difftool.vimdiff "cat \"\$REMOTE\"" &&
#               echo master >expect &&
#               git difftool --tool vimdiff --no-prompt branch >actual &&
#               test_cmp expect actual
#

Running the same test with "-v" seems to get stuck with this
forever:

expecting success:
        test_config difftool.vimdiff "cat \"\$REMOTE\"" &&
        echo master >expect &&
        git difftool --tool vimdiff --no-prompt branch >actual &&
        test_cmp expect actual

Vim: Warning: Output is not to a terminal
Vim: Warning: Input is not from a terminal

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

[David Aguilar]

On Wed, Feb 20, 2013 at 9:00 PM, Junio C Hamano <gitster@pobox.com> wrote:
> David Aguilar <davvid@gmail.com> writes:
>
>> 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
>>  '
>
> Eek.
>
> $ sh t7800-difftool.sh -i
> ok 1 - setup
> ok 2 - custom commands
> not ok 3 - custom tool commands override built-ins
> #
> #               test_config difftool.vimdiff "cat \"\$REMOTE\"" &&
> #               echo master >expect &&
> #               git difftool --tool vimdiff --no-prompt branch >actual &&
> #               test_cmp expect actual
> #
>
> Running the same test with "-v" seems to get stuck with this
> forever:
>
> expecting success:
>         test_config difftool.vimdiff "cat \"\$REMOTE\"" &&
>         echo master >expect &&
>         git difftool --tool vimdiff --no-prompt branch >actual &&
>         test_cmp expect actual
>
> Vim: Warning: Output is not to a terminal
> Vim: Warning: Input is not from a terminal
>

Oh boy.  I botched the rebase.  I have a replacement.  I thought I
re-ran the tests!  I'll resend it.
-- 
David

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

[Johannes Sixt]

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

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

[David Aguilar]

On Wed, Mar 20, 2013 at 2:48 AM, Johannes Sixt <j.sixt@viscovery.net> wrote:
> 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?

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

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

[Johannes Sixt]

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

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

[Johannes Sixt]

Am 3/21/2013 8:41, schrieb Johannes Sixt:
> 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,

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

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

[John Keeping]

On Fri, Mar 22, 2013 at 08:13:46AM +0100, Johannes Sixt wrote:
> Am 3/21/2013 8:41, schrieb Johannes Sixt:
> > 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,
> 
> 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.

> 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

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

[Johannes Sixt]

Am 3/22/2013 11:00, schrieb John Keeping:
> On Fri, Mar 22, 2013 at 08:13:46AM +0100, Johannes Sixt wrote:
>> Am 3/21/2013 8:41, schrieb Johannes Sixt:
>>> 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,
>>
>> 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

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

[John Keeping]

On Fri, Mar 22, 2013 at 12:14:27PM +0100, Johannes Sixt wrote:
> Am 3/22/2013 11:00, schrieb John Keeping:
> > On Fri, Mar 22, 2013 at 08:13:46AM +0100, Johannes Sixt wrote:
> >> Am 3/21/2013 8:41, schrieb Johannes Sixt:
> >>> 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,
> >>
> >> 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

[PATCH 0/3] Improve difftool --dir-diff tests

[John Keeping]

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

[PATCH 1/3] t7800: don't hide grep output

[John Keeping]

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 <j.sixt@viscovery.net>
Signed-off-by: John Keeping <john@keeping.me.uk>
---
 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

[PATCH 2/3] t7800: fix tests when difftool uses --no-symlinks

[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
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 <john@keeping.me.uk>
---
 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

[PATCH 3/3] t7800: run --dir-diff tests with and without symlinks

[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.

Signed-off-by: John Keeping <john@keeping.me.uk>
---
 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

[PATCH 3/3 v2] t7800: run --dir-diff tests with and without symlinks

[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.

Signed-off-by: John Keeping <john@keeping.me.uk>
---
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

Re: [PATCH 2/3] t7800: fix tests when difftool uses --no-symlinks

[Johannes Sixt]

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.

> 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
>  '
> ...

Re: [PATCH 1/3] t7800: don't hide grep output

[Johannes Sixt]

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

Re: [PATCH 1/3] t7800: don't hide grep output

[Junio C Hamano]

Looks good from a cursory read.  Thanks.

Re: [PATCH 2/3] t7800: fix tests when difftool uses --no-symlinks

[Junio C Hamano]

John Keeping <john@keeping.me.uk> writes:

> 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.

Hmph.  Why do we populate the temporary directory with a copy of an
untracked path in the first place?  I thought the point of dir-diff
was to materialize only the relevant paths to two temporaries and
compare these temporaries with a tool that knows how to compare two
directories?

Even if you had path F in HEAD that you are no longer tracking in
the working tree, a normal

	$ git diff HEAD

would report the path F to have been deleted, so I would imagine
that the preimage side of the temporary directory should get a copy
of HEAD:F at path F, while the postimage side of the temporary
directory should not even have anything at path F, when dir-diff
runs, no?

Isn't that the real reason why the test fails?  The path 'output' is
not being tracked at any revision or in the index that is involved
in the test, is it?

> 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.

It is a good idea to move these test output and expect test vectore
files to a different place to make it easier to distinguish them
from test input (e.g. "sub", "file", etc.) in general, but the
description of the original problem sounds like it is just working
around a bug to me.  What am I missing?

> 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 <john@keeping.me.uk>
> ---
>  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
>  	)
>  '

Re: [PATCH 2/3] t7800: fix tests when difftool uses --no-symlinks

[John Keeping]

On Fri, Mar 22, 2013 at 03:53:38PM -0700, Junio C Hamano wrote:
> John Keeping <john@keeping.me.uk> writes:
> 
> > 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.
> 
> Hmph.  Why do we populate the temporary directory with a copy of an
> untracked path in the first place?  I thought the point of dir-diff
> was to materialize only the relevant paths to two temporaries and
> compare these temporaries with a tool that knows how to compare two
> directories?
> 
> Even if you had path F in HEAD that you are no longer tracking in
> the working tree, a normal
> 
> 	$ git diff HEAD
> 
> would report the path F to have been deleted, so I would imagine
> that the preimage side of the temporary directory should get a copy
> of HEAD:F at path F, while the postimage side of the temporary
> directory should not even have anything at path F, when dir-diff
> runs, no?
> 
> Isn't that the real reason why the test fails?  The path 'output' is
> not being tracked at any revision or in the index that is involved
> in the test, is it?

Actually it is, which is what I missed earlier.

A couple of tests before this 'setup change in subdirectory' does 'git
add .' which is far more general than it needs.  Perhaps this is a
better change:

diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
index bba8a9d..561c993 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"
 '
 

> > 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.
> 
> It is a good idea to move these test output and expect test vectore
> files to a different place to make it easier to distinguish them
> from test input (e.g. "sub", "file", etc.) in general, but the
> description of the original problem sounds like it is just working
> around a bug to me.  What am I missing?

I think there is a bug, as described in the paragraph below, and this
test should be made independent of that.  In light of the above I think
we can drop this patch and do this with that change instead.

> > 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 <john@keeping.me.uk>
> > ---
> >  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
> >  	)
> >  '
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/3] t7800: fix tests when difftool uses --no-symlinks

[David Aguilar]

On Fri, Mar 22, 2013 at 4:05 PM, John Keeping <john@keeping.me.uk> wrote:
> On Fri, Mar 22, 2013 at 03:53:38PM -0700, Junio C Hamano wrote:
>> John Keeping <john@keeping.me.uk> writes:
>>
>> > 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.
>>
>> Hmph.  Why do we populate the temporary directory with a copy of an
>> untracked path in the first place?  I thought the point of dir-diff
>> was to materialize only the relevant paths to two temporaries and
>> compare these temporaries with a tool that knows how to compare two
>> directories?
>>
>> Even if you had path F in HEAD that you are no longer tracking in
>> the working tree, a normal
>>
>>       $ git diff HEAD
>>
>> would report the path F to have been deleted, so I would imagine
>> that the preimage side of the temporary directory should get a copy
>> of HEAD:F at path F, while the postimage side of the temporary
>> directory should not even have anything at path F, when dir-diff
>> runs, no?
>>
>> Isn't that the real reason why the test fails?  The path 'output' is
>> not being tracked at any revision or in the index that is involved
>> in the test, is it?
>
> Actually it is, which is what I missed earlier.
>
> A couple of tests before this 'setup change in subdirectory' does 'git
> add .' which is far more general than it needs.  Perhaps this is a
> better change:
>
> diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
> index bba8a9d..561c993 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"
>  '
>
>
>> > 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.
>>
>> It is a good idea to move these test output and expect test vectore
>> files to a different place to make it easier to distinguish them
>> from test input (e.g. "sub", "file", etc.) in general, but the
>> description of the original problem sounds like it is just working
>> around a bug to me.  What am I missing?
>
> I think there is a bug, as described in the paragraph below, and this
> test should be made independent of that.  In light of the above I think
> we can drop this patch and do this with that change instead.

I, too, was scratching my head when the Windows issue was first reported.

Thanks for clearing this up; fixing the blind "add ."
certainly seems like the way to go.
-- 
David

[PATCH v2 0/3] difftool --dir-diff test improvements

[John Keeping]

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

[PATCH v2 1/3] t7800: don't hide grep output

[John Keeping]

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 <j.sixt@viscovery.net>
Signed-off-by: John Keeping <john@keeping.me.uk>
---
 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

[PATCH v2 2/3] t7800: fix tests when difftool uses --no-symlinks

[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.

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 <john@keeping.me.uk>

---
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

[PATCH v2 3/3] t7800: run --dir-diff tests with and without symlinks

[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.

Signed-off-by: John Keeping <john@keeping.me.uk>

---
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

Re: [PATCH v2 2/3] t7800: fix tests when difftool uses --no-symlinks

[Junio C Hamano]

John Keeping <john@keeping.me.uk> writes:

> 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.

Good.

> 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.

Questionable.

Admittedly I do not use difftool myself, and I have long assumed
that difftool users are using the tools to _view_ the changes, but
apparently some of the tools let the user muck with what is shown,
and also apparently people seem to like the fact that they can make
changes.  So I've led to believe the "update in difftool, take the
change back to working tree, either by making symbolic links or
copying them back" behaviour was a _feature_.

It is possible that this is not universally considerd as a feature,
but if that is the case, I think the right way to do this is to tell
the tools _not_ to let the user to modify contents they show in the
first place, not letting the user modify and then warning after the
fact.

> Signed-off-by: John Keeping <john@keeping.me.uk>
>
> ---
> 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"
>  '

Re: [PATCH v2 2/3] t7800: fix tests when difftool uses --no-symlinks

[Eric Sunshine]

On Sat, Mar 23, 2013 at 9:31 AM, John Keeping <john@keeping.me.uk> 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 <john@keeping.me.uk>

Re: [PATCH v2 2/3] t7800: fix tests when difftool uses --no-symlinks

[John Keeping]

On Sat, Mar 23, 2013 at 10:19:36PM -0700, Junio C Hamano wrote:
> > 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.
> 
> Questionable.
> 
> Admittedly I do not use difftool myself, and I have long assumed
> that difftool users are using the tools to _view_ the changes, but
> apparently some of the tools let the user muck with what is shown,
> and also apparently people seem to like the fact that they can make
> changes.  So I've led to believe the "update in difftool, take the
> change back to working tree, either by making symbolic links or
> copying them back" behaviour was a _feature_.

Yes it is.  I think my explanation wasn't clear enough here.

What currently happens is that after the user's tool has finished
running the working tree file and temporary file are compared and if
they are different then the temporary file is copied over the working
tree file.

This is good if the user has edited the temporary file, but what if they
edit they working tree file while using the tool to examine the
differences?  I think we need to at the very least look at the mtime of
the files and refuse to copy over the temporary file if that of the
working tree file is newer.

Obviously none of this matters if we can use symlinks, but in the
non-symlink case I think a user might find it surprising if the
(unmodified) file used by their diff tool were suddenly copied over the
working tree wiping out the changes they have just made.

> It is possible that this is not universally considerd as a feature,
> but if that is the case, I think the right way to do this is to tell
> the tools _not_ to let the user to modify contents they show in the
> first place, not letting the user modify and then warning after the
> fact.

Re: [PATCH v2 2/3] t7800: fix tests when difftool uses --no-symlinks

[Matt McClure]

On Sun, Mar 24, 2013 at 1:19 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Admittedly I do not use difftool myself, and I have long assumed
> that difftool users are using the tools to _view_ the changes, but
> apparently some of the tools let the user muck with what is shown,
> and also apparently people seem to like the fact that they can make
> changes.  So I've led to believe the "update in difftool, take the
> change back to working tree, either by making symbolic links or
> copying them back" behaviour was a _feature_.

Definitely. Here's how I use it:
http://matthewlmcclure.com/s/2013/03/14/use-emacs-dircmp-mode-as-a-git-difftool.html

-- 
Matt McClure
http://www.matthewlmcclure.com
http://www.mapmyfitness.com/profile/matthewlmcclure

Re: [PATCH v2 2/3] t7800: fix tests when difftool uses --no-symlinks

[Matt McClure]

On Sun, Mar 24, 2013 at 8:36 AM, John Keeping <john@keeping.me.uk> wrote:
> In the
> non-symlink case I think a user might find it surprising if the
> (unmodified) file used by their diff tool were suddenly copied over the
> working tree wiping out the changes they have just made.

That's exactly what I was describing here:
http://thread.gmane.org/gmane.comp.version-control.git/217979/focus=218006

-- 
Matt McClure
http://www.matthewlmcclure.com
http://www.mapmyfitness.com/profile/matthewlmcclure

Re: [PATCH v2 2/3] t7800: fix tests when difftool uses --no-symlinks

[John Keeping]

On Sun, Mar 24, 2013 at 09:31:45AM -0400, Matt McClure wrote:
> On Sun, Mar 24, 2013 at 8:36 AM, John Keeping <john@keeping.me.uk> wrote:
> > In the
> > non-symlink case I think a user might find it surprising if the
> > (unmodified) file used by their diff tool were suddenly copied over the
> > working tree wiping out the changes they have just made.
> 
> That's exactly what I was describing here:
> http://thread.gmane.org/gmane.comp.version-control.git/217979/focus=218006

Ahh, I guess I didn't fully register the impact of that at the time and
had to rediscover the problem for myself ;-)

How about doing this (on top of jk/difftool-dir-diff-edit-fix)?

-- >8 --
Subject: [PATCH] difftool: don't overwrite modified files

After running the user's diff tool, git-difftool will copy any files
that differ between the working tree and the temporary tree.  This is
useful when the user edits the file in their diff tool but is wrong if
they edit the working tree file while examining the diff.

Instead of copying unconditionally when the files differ, store the
initial hash of the working tree file and only copy the temporary file
back if it was modified and the working tree file was not.  If both
files have been modified, print a warning and exit with an error.

Signed-off-by: John Keeping <john@keeping.me.uk>
---
 git-difftool.perl   | 35 +++++++++++++++++++++--------------
 t/t7800-difftool.sh | 26 ++++++++++++++++++++++++++
 2 files changed, 47 insertions(+), 14 deletions(-)

diff --git a/git-difftool.perl b/git-difftool.perl
index c433e86..be82b5a 100755
--- a/git-difftool.perl
+++ b/git-difftool.perl
@@ -15,7 +15,6 @@ use strict;
 use warnings;
 use File::Basename qw(dirname);
 use File::Copy;
-use File::Compare;
 use File::Find;
 use File::stat;
 use File::Path qw(mkpath rmtree);
@@ -123,7 +122,7 @@ sub setup_dir_diff
 	my $rindex = '';
 	my %submodule;
 	my %symlink;
-	my @working_tree = ();
+	my %working_tree;
 	my @rawdiff = split('\0', $diffrtn);
 
 	my $i = 0;
@@ -175,7 +174,9 @@ EOF
 
 		if ($rmode ne $null_mode) {
 			if (use_wt_file($repo, $workdir, $dst_path, $rsha1, $symlinks)) {
-				push(@working_tree, $dst_path);
+				$working_tree{$dst_path} =
+					$repo->command_oneline('hash-object',
+						"$workdir/$dst_path");
 			} else {
 				$rindex .= "$rmode $rsha1\t$dst_path\0";
 			}
@@ -227,7 +228,7 @@ EOF
 	# not part of the index. Remove any trailing slash from $workdir
 	# before starting to avoid double slashes in symlink targets.
 	$workdir =~ s|/$||;
-	for my $file (@working_tree) {
+	for my $file (keys %working_tree) {
 		my $dir = dirname($file);
 		unless (-d "$rdir/$dir") {
 			mkpath("$rdir/$dir") or
@@ -278,7 +279,7 @@ EOF
 		exit_cleanup($tmpdir, 1) if not $ok;
 	}
 
-	return ($ldir, $rdir, $tmpdir, @working_tree);
+	return ($ldir, $rdir, $tmpdir, %working_tree);
 }
 
 sub write_to_file
@@ -376,7 +377,7 @@ sub dir_diff
 	my $error = 0;
 	my $repo = Git->repository();
 	my $workdir = find_worktree($repo);
-	my ($a, $b, $tmpdir, @worktree) =
+	my ($a, $b, $tmpdir, %worktree) =
 		setup_dir_diff($repo, $workdir, $symlinks);
 
 	if (defined($extcmd)) {
@@ -390,19 +391,25 @@ sub dir_diff
 	# should be copied back to the working tree.
 	# Do not copy back files when symlinks are used and the
 	# external tool did not replace the original link with a file.
-	for my $file (@worktree) {
+	for my $file (keys %worktree) {
 		next if $symlinks && -l "$b/$file";
 		next if ! -f "$b/$file";
 
-		my $diff = compare("$b/$file", "$workdir/$file");
-		if ($diff == 0) {
-			next;
-		} elsif ($diff == -1) {
-			my $errmsg = "warning: Could not compare ";
-			$errmsg += "'$b/$file' with '$workdir/$file'\n";
+		my $wt_hash = $repo->command_oneline('hash-object',
+			"$workdir/$file");
+		my $tmp_hash = $repo->command_oneline('hash-object',
+			"$b/$file");
+		my $wt_modified = $wt_hash ne $worktree{$file};
+		my $tmp_modified = $tmp_hash ne $worktree{$file};
+
+		if ($wt_modified and $tmp_modified) {
+			my $errmsg = "warning: Both files modified: ";
+			$errmsg .= "'$workdir/$file' and '$b/$file'.\n";
+			$errmsg .= "warning: Working tree file has been left.\n";
+			$errmsg .= "warning:\n";
 			warn $errmsg;
 			$error = 1;
-		} elsif ($diff == 1) {
+		} elsif ($tmp_modified) {
 			my $mode = stat("$b/$file")->mode;
 			copy("$b/$file", "$workdir/$file") or
 			exit_cleanup($tmpdir, 1);
diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
index db3d3d6..be2042d 100755
--- a/t/t7800-difftool.sh
+++ b/t/t7800-difftool.sh
@@ -407,4 +407,30 @@ test_expect_success PERL 'difftool --dir-diff from subdirectory' '
 	)
 '
 
+write_script modify-file <<\EOF
+echo "new content" >file
+EOF
+
+test_expect_success PERL 'difftool --no-symlinks does not overwrite working tree file ' '
+	echo "orig content" >file &&
+	git difftool --dir-diff --no-symlinks --extcmd "$(pwd)/modify-file" branch &&
+	echo "new content" >expect &&
+	test_cmp expect file
+'
+
+write_script modify-both-files <<\EOF
+echo "wt content" >file &&
+echo "tmp content" >"$2/file" &&
+echo "$2" >tmpdir
+EOF
+
+test_expect_success PERL 'difftool --no-symlinks detects conflict ' '
+	echo "orig content" >file &&
+	test_must_fail git difftool --dir-diff --no-symlinks --extcmd "$(pwd)/modify-both-files" branch &&
+	echo "wt content" >expect &&
+	test_cmp expect file &&
+	echo "tmp content" >expect &&
+	test_cmp expect "$(cat tmpdir)/file"
+'
+
 test_done
-- 
1.8.2.411.g65a544e

Re: [PATCH v2 2/3] t7800: fix tests when difftool uses --no-symlinks

[David Aguilar]

On Sun, Mar 24, 2013 at 5:36 AM, John Keeping <john@keeping.me.uk> wrote:
> On Sat, Mar 23, 2013 at 10:19:36PM -0700, Junio C Hamano wrote:
>> > 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.
>>
>> Questionable.
>>
>> Admittedly I do not use difftool myself, and I have long assumed
>> that difftool users are using the tools to _view_ the changes, but
>> apparently some of the tools let the user muck with what is shown,
>> and also apparently people seem to like the fact that they can make
>> changes.  So I've led to believe the "update in difftool, take the
>> change back to working tree, either by making symbolic links or
>> copying them back" behaviour was a _feature_.
>
> Yes it is.  I think my explanation wasn't clear enough here.
>
> What currently happens is that after the user's tool has finished
> running the working tree file and temporary file are compared and if
> they are different then the temporary file is copied over the working
> tree file.
>
> This is good if the user has edited the temporary file, but what if they
> edit they working tree file while using the tool to examine the
> differences?  I think we need to at the very least look at the mtime of
> the files and refuse to copy over the temporary file if that of the
> working tree file is newer.
>
> Obviously none of this matters if we can use symlinks, but in the
> non-symlink case I think a user might find it surprising if the
> (unmodified) file used by their diff tool were suddenly copied over the
> working tree wiping out the changes they have just made.

Thanks, this adds a little more safety to the operation, which is good.
The downside is that it's a performance hit since we end up running
an additional hash-object on every worktree file.
I would definitely choose safety/correctness in this situation.

This makes me wonder whether the modifiable mode should be made
more explicit, either in the documentation or via a flag.

Imagine if --dir-diff also honored --edit and --no-edit flags.

Right now --edit is the default.  If we had foreseen these various
edge cases and unintended copy-backs then we may have initially
chosen --no-edit as the default, but that's not really my point.

What I'm thinking is that it might be good for the tool to
learn --edit/--no-edit so that the symlink/copy-back heuristic
can be documented alongside that option.  Users can then know
what to expect when using this mode.  --no-edit would also be
faster since it can avoid all these extra steps.

It could also learn "difftool.dirDiffEditable" to control the
default, which would eliminate the pain in needing to supply
the flag on every invocation.

What do you think about officially supporting a read-only mode?
-- 
David

Re: [PATCH v2 3/3] t7800: run --dir-diff tests with and without symlinks

[Johannes Sixt]

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

Re: [PATCH v2 2/3] t7800: fix tests when difftool uses --no-symlinks

[Johannes Sixt]

Am 3/24/2013 16:15, schrieb John Keeping:
> Subject: [PATCH] difftool: don't overwrite modified files
> 
> After running the user's diff tool, git-difftool will copy any files
> that differ between the working tree and the temporary tree.  This is
> useful when the user edits the file in their diff tool but is wrong if
> they edit the working tree file while examining the diff.
> 
> Instead of copying unconditionally when the files differ, store the
> initial hash of the working tree file and only copy the temporary file
> back if it was modified and the working tree file was not.  If both
> files have been modified, print a warning and exit with an error.
> 
> Signed-off-by: John Keeping <john@keeping.me.uk>
> ---
>  git-difftool.perl   | 35 +++++++++++++++++++++--------------
>  t/t7800-difftool.sh | 26 ++++++++++++++++++++++++++
>  2 files changed, 47 insertions(+), 14 deletions(-)
> 
> diff --git a/git-difftool.perl b/git-difftool.perl
> index c433e86..be82b5a 100755
> --- a/git-difftool.perl
> +++ b/git-difftool.perl
> @@ -15,7 +15,6 @@ use strict;
>  use warnings;
>  use File::Basename qw(dirname);
>  use File::Copy;
> -use File::Compare;
>  use File::Find;
>  use File::stat;
>  use File::Path qw(mkpath rmtree);
> @@ -123,7 +122,7 @@ sub setup_dir_diff
>  	my $rindex = '';
>  	my %submodule;
>  	my %symlink;
> -	my @working_tree = ();
> +	my %working_tree;
>  	my @rawdiff = split('\0', $diffrtn);
>  
>  	my $i = 0;
> @@ -175,7 +174,9 @@ EOF
>  
>  		if ($rmode ne $null_mode) {
>  			if (use_wt_file($repo, $workdir, $dst_path, $rsha1, $symlinks)) {
> -				push(@working_tree, $dst_path);
> +				$working_tree{$dst_path} =
> +					$repo->command_oneline('hash-object',
> +						"$workdir/$dst_path");
>  			} else {
>  				$rindex .= "$rmode $rsha1\t$dst_path\0";
>  			}
> @@ -227,7 +228,7 @@ EOF
>  	# not part of the index. Remove any trailing slash from $workdir
>  	# before starting to avoid double slashes in symlink targets.
>  	$workdir =~ s|/$||;
> -	for my $file (@working_tree) {
> +	for my $file (keys %working_tree) {
>  		my $dir = dirname($file);
>  		unless (-d "$rdir/$dir") {
>  			mkpath("$rdir/$dir") or
> @@ -278,7 +279,7 @@ EOF
>  		exit_cleanup($tmpdir, 1) if not $ok;
>  	}
>  
> -	return ($ldir, $rdir, $tmpdir, @working_tree);
> +	return ($ldir, $rdir, $tmpdir, %working_tree);
>  }
>  
>  sub write_to_file
> @@ -376,7 +377,7 @@ sub dir_diff
>  	my $error = 0;
>  	my $repo = Git->repository();
>  	my $workdir = find_worktree($repo);
> -	my ($a, $b, $tmpdir, @worktree) =
> +	my ($a, $b, $tmpdir, %worktree) =
>  		setup_dir_diff($repo, $workdir, $symlinks);
>  
>  	if (defined($extcmd)) {
> @@ -390,19 +391,25 @@ sub dir_diff
>  	# should be copied back to the working tree.
>  	# Do not copy back files when symlinks are used and the
>  	# external tool did not replace the original link with a file.
> -	for my $file (@worktree) {
> +	for my $file (keys %worktree) {
>  		next if $symlinks && -l "$b/$file";
>  		next if ! -f "$b/$file";
>  
> -		my $diff = compare("$b/$file", "$workdir/$file");
> -		if ($diff == 0) {
> -			next;
> -		} elsif ($diff == -1) {
> -			my $errmsg = "warning: Could not compare ";
> -			$errmsg += "'$b/$file' with '$workdir/$file'\n";
> +		my $wt_hash = $repo->command_oneline('hash-object',
> +			"$workdir/$file");
> +		my $tmp_hash = $repo->command_oneline('hash-object',
> +			"$b/$file");

This is gross. Can't we do much better here? Difftool already keeps a
GIT_INDEX of the files in the temporary tree ($tmpdir/rindex). Running
git-diff-files should be sufficient to tell which ones where edited via
the users's diff-tool. Then you can restrict calling hash-object to only
those worktree files where an "edit collision" needs to be checked for.

You could also keep a parallel index that keeps the state of the same set
of files in the worktree. Then another git-diff-files call could replace
the other half of hash-object calls.

> +		my $wt_modified = $wt_hash ne $worktree{$file};
> +		my $tmp_modified = $tmp_hash ne $worktree{$file};
> +
> +		if ($wt_modified and $tmp_modified) {
> +			my $errmsg = "warning: Both files modified: ";
> +			$errmsg .= "'$workdir/$file' and '$b/$file'.\n";
> +			$errmsg .= "warning: Working tree file has been left.\n";
> +			$errmsg .= "warning:\n";
>  			warn $errmsg;
>  			$error = 1;
> -		} elsif ($diff == 1) {
> +		} elsif ($tmp_modified) {
>  			my $mode = stat("$b/$file")->mode;
>  			copy("$b/$file", "$workdir/$file") or
>  			exit_cleanup($tmpdir, 1);

-- Hannes

Re: [PATCH v2 3/3] t7800: run --dir-diff tests with and without symlinks

[John Keeping]

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:
> > 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.

Re: [PATCH v2 2/3] t7800: fix tests when difftool uses --no-symlinks

[John Keeping]

On Mon, Mar 25, 2013 at 08:41:59AM +0100, Johannes Sixt wrote:
> Am 3/24/2013 16:15, schrieb John Keeping:
> > Subject: [PATCH] difftool: don't overwrite modified files
> > 
> > After running the user's diff tool, git-difftool will copy any files
> > that differ between the working tree and the temporary tree.  This is
> > useful when the user edits the file in their diff tool but is wrong if
> > they edit the working tree file while examining the diff.
> > 
> > Instead of copying unconditionally when the files differ, store the
> > initial hash of the working tree file and only copy the temporary file
> > back if it was modified and the working tree file was not.  If both
> > files have been modified, print a warning and exit with an error.
> > 
> > Signed-off-by: John Keeping <john@keeping.me.uk>
> > ---
> >  git-difftool.perl   | 35 +++++++++++++++++++++--------------
> >  t/t7800-difftool.sh | 26 ++++++++++++++++++++++++++
> >  2 files changed, 47 insertions(+), 14 deletions(-)
> > 
> > diff --git a/git-difftool.perl b/git-difftool.perl
> > index c433e86..be82b5a 100755
> > --- a/git-difftool.perl
> > +++ b/git-difftool.perl
> > @@ -15,7 +15,6 @@ use strict;
> >  use warnings;
> >  use File::Basename qw(dirname);
> >  use File::Copy;
> > -use File::Compare;
> >  use File::Find;
> >  use File::stat;
> >  use File::Path qw(mkpath rmtree);
> > @@ -123,7 +122,7 @@ sub setup_dir_diff
> >  	my $rindex = '';
> >  	my %submodule;
> >  	my %symlink;
> > -	my @working_tree = ();
> > +	my %working_tree;
> >  	my @rawdiff = split('\0', $diffrtn);
> >  
> >  	my $i = 0;
> > @@ -175,7 +174,9 @@ EOF
> >  
> >  		if ($rmode ne $null_mode) {
> >  			if (use_wt_file($repo, $workdir, $dst_path, $rsha1, $symlinks)) {
> > -				push(@working_tree, $dst_path);
> > +				$working_tree{$dst_path} =
> > +					$repo->command_oneline('hash-object',
> > +						"$workdir/$dst_path");
> >  			} else {
> >  				$rindex .= "$rmode $rsha1\t$dst_path\0";
> >  			}
> > @@ -227,7 +228,7 @@ EOF
> >  	# not part of the index. Remove any trailing slash from $workdir
> >  	# before starting to avoid double slashes in symlink targets.
> >  	$workdir =~ s|/$||;
> > -	for my $file (@working_tree) {
> > +	for my $file (keys %working_tree) {
> >  		my $dir = dirname($file);
> >  		unless (-d "$rdir/$dir") {
> >  			mkpath("$rdir/$dir") or
> > @@ -278,7 +279,7 @@ EOF
> >  		exit_cleanup($tmpdir, 1) if not $ok;
> >  	}
> >  
> > -	return ($ldir, $rdir, $tmpdir, @working_tree);
> > +	return ($ldir, $rdir, $tmpdir, %working_tree);
> >  }
> >  
> >  sub write_to_file
> > @@ -376,7 +377,7 @@ sub dir_diff
> >  	my $error = 0;
> >  	my $repo = Git->repository();
> >  	my $workdir = find_worktree($repo);
> > -	my ($a, $b, $tmpdir, @worktree) =
> > +	my ($a, $b, $tmpdir, %worktree) =
> >  		setup_dir_diff($repo, $workdir, $symlinks);
> >  
> >  	if (defined($extcmd)) {
> > @@ -390,19 +391,25 @@ sub dir_diff
> >  	# should be copied back to the working tree.
> >  	# Do not copy back files when symlinks are used and the
> >  	# external tool did not replace the original link with a file.
> > -	for my $file (@worktree) {
> > +	for my $file (keys %worktree) {
> >  		next if $symlinks && -l "$b/$file";
> >  		next if ! -f "$b/$file";
> >  
> > -		my $diff = compare("$b/$file", "$workdir/$file");
> > -		if ($diff == 0) {
> > -			next;
> > -		} elsif ($diff == -1) {
> > -			my $errmsg = "warning: Could not compare ";
> > -			$errmsg += "'$b/$file' with '$workdir/$file'\n";
> > +		my $wt_hash = $repo->command_oneline('hash-object',
> > +			"$workdir/$file");
> > +		my $tmp_hash = $repo->command_oneline('hash-object',
> > +			"$b/$file");
> 
> This is gross. Can't we do much better here? Difftool already keeps a
> GIT_INDEX of the files in the temporary tree ($tmpdir/rindex). Running
> git-diff-files should be sufficient to tell which ones where edited via
> the users's diff-tool. Then you can restrict calling hash-object to only
> those worktree files where an "edit collision" needs to be checked for.

That's only the case for files that are not copied from the working
tree, so the temporary index doesn't contain the files that are of
interest here.

> You could also keep a parallel index that keeps the state of the same set
> of files in the worktree. Then another git-diff-files call could replace
> the other half of hash-object calls.

I like the idea of creating an index from the working tree files and
using it here.  If we create a "starting state" index for these files,
we should be able to run git-diff-files against both the working tree
and the temporary tree at this point and compare the output.  I'll try
this approach this evening.

> > +		my $wt_modified = $wt_hash ne $worktree{$file};
> > +		my $tmp_modified = $tmp_hash ne $worktree{$file};
> > +
> > +		if ($wt_modified and $tmp_modified) {
> > +			my $errmsg = "warning: Both files modified: ";
> > +			$errmsg .= "'$workdir/$file' and '$b/$file'.\n";
> > +			$errmsg .= "warning: Working tree file has been left.\n";
> > +			$errmsg .= "warning:\n";
> >  			warn $errmsg;
> >  			$error = 1;
> > -		} elsif ($diff == 1) {
> > +		} elsif ($tmp_modified) {
> >  			my $mode = stat("$b/$file")->mode;
> >  			copy("$b/$file", "$workdir/$file") or
> >  			exit_cleanup($tmpdir, 1);

Re: [PATCH v2 2/3] t7800: fix tests when difftool uses --no-symlinks

[John Keeping]

On Sun, Mar 24, 2013 at 02:29:40PM -0700, David Aguilar wrote:
> This makes me wonder whether the modifiable mode should be made
> more explicit, either in the documentation or via a flag.
> 
> Imagine if --dir-diff also honored --edit and --no-edit flags.
> 
> Right now --edit is the default.  If we had foreseen these various
> edge cases and unintended copy-backs then we may have initially
> chosen --no-edit as the default, but that's not really my point.

I view --symlinks as the default, which avoids most of this pain ;-)
I guess we're talking about three different "working tree files" modes
here: symlink, copy-copyback and copy-readonly.

I wonder if anyone uses --no-symlinks when they are not forced to by
their operating system?  What is the use case if they do?

> What I'm thinking is that it might be good for the tool to
> learn --edit/--no-edit so that the symlink/copy-back heuristic
> can be documented alongside that option.  Users can then know
> what to expect when using this mode.  --no-edit would also be
> faster since it can avoid all these extra steps.
> 
> It could also learn "difftool.dirDiffEditable" to control the
> default, which would eliminate the pain in needing to supply
> the flag on every invocation.
> 
> What do you think about officially supporting a read-only mode?

How would that interoperate with symlink mode?  Should --no-edit imply
--no-symlinks or does the --[no-]edit option only have an effect if
--no-symlinks is in effect?

I don't think this is the first time this idea has been suggested, so
that's some indicator that it's a good idea.  I'm not sure about
--edit/--no-edit for this though.  The behaviour isn't really similar to
the way that option works with git-commit, git-merge, etc.  I don't have
a better suggestion at the moment though.


John

Re: [PATCH v2 3/3] t7800: run --dir-diff tests with and without symlinks

[Johannes Sixt]

Am 3/25/2013 11:35, schrieb John Keeping:
> 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?

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

Re: [PATCH v2 3/3] t7800: run --dir-diff tests with and without symlinks

[John Keeping]

On Mon, Mar 25, 2013 at 11:59:12AM +0100, Johannes Sixt wrote:
> Am 3/25/2013 11:35, schrieb John Keeping:
> > 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?
> 
> 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.

Re: [PATCH v2 2/3] t7800: fix tests when difftool uses --no-symlinks

[Junio C Hamano]

David Aguilar <davvid@gmail.com> writes:

> This makes me wonder whether the modifiable mode should be made
> more explicit, either in the documentation or via a flag.
>
> Imagine if --dir-diff also honored --edit and --no-edit flags.
>
> Right now --edit is the default.  If we had foreseen these various
> edge cases and unintended copy-backs then we may have initially
> chosen --no-edit as the default, but that's not really my point.
>
> What I'm thinking is that it might be good for the tool to
> learn --edit/--no-edit so that the symlink/copy-back heuristic
> can be documented alongside that option.  Users can then know
> what to expect when using this mode.  --no-edit would also be
> faster since it can avoid all these extra steps.
>
> It could also learn "difftool.dirDiffEditable" to control the
> default, which would eliminate the pain in needing to supply
> the flag on every invocation.
>
> What do you think about officially supporting a read-only mode?

Yeah, actually that was what I've been assuming the default (not
suggesting to change the default behaviour here).

Re: [PATCH v2 2/3] t7800: fix tests when difftool uses --no-symlinks

[Junio C Hamano]

Johannes Sixt <j.sixt@viscovery.net> writes:

> Am 3/24/2013 16:15, schrieb John Keeping:
> ...
>> +	for my $file (keys %worktree) {
>>  		next if $symlinks && -l "$b/$file";
>>  		next if ! -f "$b/$file";
>>  
>> -		my $diff = compare("$b/$file", "$workdir/$file");
>> -		if ($diff == 0) {
>> -			next;
>> -		} elsif ($diff == -1) {
>> -			my $errmsg = "warning: Could not compare ";
>> -			$errmsg += "'$b/$file' with '$workdir/$file'\n";
>> +		my $wt_hash = $repo->command_oneline('hash-object',
>> +			"$workdir/$file");
>> +		my $tmp_hash = $repo->command_oneline('hash-object',
>> +			"$b/$file");
>
> This is gross. Can't we do much better here? Difftool already keeps a
> GIT_INDEX of the files in the temporary tree ($tmpdir/rindex). Running
> git-diff-files should be sufficient to tell which ones where edited via
> the users's diff-tool. Then you can restrict calling hash-object to only
> those worktree files where an "edit collision" needs to be checked for.

;-).

[PATCH v2] difftool: don't overwrite modified files

[John Keeping]

After running the user's diff tool, git-difftool will copy any files
that differ between the working tree and the temporary tree.  This is
useful when the user edits the file in their diff tool but is wrong if
they edit the working tree file while examining the diff.

Instead of copying unconditionally when the files differ, create and
index from the working tree files and only copy the temporary file back
if it was modified and the working tree file was not.  If both files
have been modified, print a warning and exit with an error.

Note that we cannot use an existing index in git-difftool since those
contain the modified files that need to be checked out but here we are
looking at those files which are copied from the working tree and not
checked out.  These are precisely the files which are not in the
existing indices.

Signed-off-by: John Keeping <john@keeping.me.uk>
---
On Mon, Mar 25, 2013 at 10:42:19AM +0000, John Keeping wrote:
> On Mon, Mar 25, 2013 at 08:41:59AM +0100, Johannes Sixt wrote:
> > This is gross. Can't we do much better here? Difftool already keeps a
> > GIT_INDEX of the files in the temporary tree ($tmpdir/rindex). Running
> > git-diff-files should be sufficient to tell which ones where edited via
> > the users's diff-tool. Then you can restrict calling hash-object to only
> > those worktree files where an "edit collision" needs to be checked for.
> 
> That's only the case for files that are not copied from the working
> tree, so the temporary index doesn't contain the files that are of
> interest here.
> 
> > You could also keep a parallel index that keeps the state of the same set
> > of files in the worktree. Then another git-diff-files call could replace
> > the other half of hash-object calls.
> 
> I like the idea of creating an index from the working tree files and
> using it here.  If we create a "starting state" index for these files,
> we should be able to run git-diff-files against both the working tree
> and the temporary tree at this point and compare the output.

Here's an attempt at taking this approach, built on
jk/difftool-dir-diff-edit-fix.

 git-difftool.perl   | 73 +++++++++++++++++++++++++++++++++++++++++++----------
 t/t7800-difftool.sh | 26 +++++++++++++++++++
 2 files changed, 85 insertions(+), 14 deletions(-)

diff --git a/git-difftool.perl b/git-difftool.perl
index c433e86..d10f7d2 100755
--- a/git-difftool.perl
+++ b/git-difftool.perl
@@ -13,9 +13,9 @@
 use 5.008;
 use strict;
 use warnings;
+use Error qw(:try);
 use File::Basename qw(dirname);
 use File::Copy;
-use File::Compare;
 use File::Find;
 use File::stat;
 use File::Path qw(mkpath rmtree);
@@ -88,14 +88,45 @@ sub use_wt_file
 	my ($repo, $workdir, $file, $sha1, $symlinks) = @_;
 	my $null_sha1 = '0' x 40;
 
-	if ($sha1 eq $null_sha1) {
-		return 1;
-	} elsif (not $symlinks) {
+	if ($sha1 ne $null_sha1 and not $symlinks) {
 		return 0;
 	}
 
 	my $wt_sha1 = $repo->command_oneline('hash-object', "$workdir/$file");
-	return $sha1 eq $wt_sha1;
+	my $use = ($sha1 eq $null_sha1) || ($sha1 eq $wt_sha1);
+	return ($use, $wt_sha1);
+}
+
+sub changed_files
+{
+	my ($repo_path, $index, $worktree) = @_;
+	$ENV{GIT_INDEX_FILE} = $index;
+	$ENV{GIT_WORK_TREE} = $worktree;
+	my $must_unset_git_dir = 0;
+	if (not defined($ENV{GIT_DIR})) {
+		$must_unset_git_dir = 1;
+		$ENV{GIT_DIR} = $repo_path;
+	}
+
+	my @refreshargs = qw/update-index --really-refresh -q --unmerged/;
+	my @gitargs = qw/diff-files --name-only -z/;
+	try {
+		Git::command_oneline(@refreshargs);
+	} catch Git::Error::Command with {};
+
+	my $line = Git::command_oneline(@gitargs);
+	my @files;
+	if (defined $line) {
+		@files = split('\0', $line);
+	} else {
+		@files = ();
+	}
+
+	delete($ENV{GIT_INDEX_FILE});
+	delete($ENV{GIT_WORK_TREE});
+	delete($ENV{GIT_DIR}) if ($must_unset_git_dir);
+
+	return map { $_ => 1 } @files;
 }
 
 sub setup_dir_diff
@@ -121,6 +152,7 @@ sub setup_dir_diff
 	my $null_sha1 = '0' x 40;
 	my $lindex = '';
 	my $rindex = '';
+	my $wtindex = '';
 	my %submodule;
 	my %symlink;
 	my @working_tree = ();
@@ -174,8 +206,12 @@ EOF
 		}
 
 		if ($rmode ne $null_mode) {
-			if (use_wt_file($repo, $workdir, $dst_path, $rsha1, $symlinks)) {
-				push(@working_tree, $dst_path);
+			my ($use, $wt_sha1) = use_wt_file($repo, $workdir,
+							  $dst_path, $rsha1,
+							  $symlinks);
+			if ($use) {
+				push @working_tree, $dst_path;
+				$wtindex .= "$rmode $wt_sha1\t$dst_path\0";
 			} else {
 				$rindex .= "$rmode $rsha1\t$dst_path\0";
 			}
@@ -218,6 +254,12 @@ EOF
 	$rc = system('git', 'checkout-index', '--all', "--prefix=$rdir/");
 	exit_cleanup($tmpdir, $rc) if $rc != 0;
 
+	$ENV{GIT_INDEX_FILE} = "$tmpdir/wtindex";
+	($inpipe, $ctx) =
+		$repo->command_input_pipe(qw(update-index --info-only -z --index-info));
+	print($inpipe $wtindex);
+	$repo->command_close_pipe($inpipe, $ctx);
+
 	# If $GIT_DIR was explicitly set just for the update/checkout
 	# commands, then it should be unset before continuing.
 	delete($ENV{GIT_DIR}) if ($must_unset_git_dir);
@@ -390,19 +432,22 @@ sub dir_diff
 	# should be copied back to the working tree.
 	# Do not copy back files when symlinks are used and the
 	# external tool did not replace the original link with a file.
+	my %wt_modified = changed_files($repo->repo_path(),
+		"$tmpdir/wtindex", "$workdir");
+	my %tmp_modified = changed_files($repo->repo_path(),
+		"$tmpdir/wtindex", "$b");
 	for my $file (@worktree) {
 		next if $symlinks && -l "$b/$file";
 		next if ! -f "$b/$file";
 
-		my $diff = compare("$b/$file", "$workdir/$file");
-		if ($diff == 0) {
-			next;
-		} elsif ($diff == -1) {
-			my $errmsg = "warning: Could not compare ";
-			$errmsg += "'$b/$file' with '$workdir/$file'\n";
+		if (exists $wt_modified{$file} and exists $tmp_modified{$file}) {
+			my $errmsg = "warning: Both files modified: ";
+			$errmsg .= "'$workdir/$file' and '$b/$file'.\n";
+			$errmsg .= "warning: Working tree file has been left.\n";
+			$errmsg .= "warning:\n";
 			warn $errmsg;
 			$error = 1;
-		} elsif ($diff == 1) {
+		} elsif ($tmp_modified{$file}) {
 			my $mode = stat("$b/$file")->mode;
 			copy("$b/$file", "$workdir/$file") or
 			exit_cleanup($tmpdir, 1);
diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
index db3d3d6..be2042d 100755
--- a/t/t7800-difftool.sh
+++ b/t/t7800-difftool.sh
@@ -407,4 +407,30 @@ test_expect_success PERL 'difftool --dir-diff from subdirectory' '
 	)
 '
 
+write_script modify-file <<\EOF
+echo "new content" >file
+EOF
+
+test_expect_success PERL 'difftool --no-symlinks does not overwrite working tree file ' '
+	echo "orig content" >file &&
+	git difftool --dir-diff --no-symlinks --extcmd "$(pwd)/modify-file" branch &&
+	echo "new content" >expect &&
+	test_cmp expect file
+'
+
+write_script modify-both-files <<\EOF
+echo "wt content" >file &&
+echo "tmp content" >"$2/file" &&
+echo "$2" >tmpdir
+EOF
+
+test_expect_success PERL 'difftool --no-symlinks detects conflict ' '
+	echo "orig content" >file &&
+	test_must_fail git difftool --dir-diff --no-symlinks --extcmd "$(pwd)/modify-both-files" branch &&
+	echo "wt content" >expect &&
+	test_cmp expect file &&
+	echo "tmp content" >expect &&
+	test_cmp expect "$(cat tmpdir)/file"
+'
+
 test_done
-- 
1.8.2.411.g65a544e

Re: [PATCH v2 3/3] t7800: run --dir-diff tests with and without symlinks

[Junio C Hamano]

Johannes Sixt <j.sixt@viscovery.net> writes:

> Am 3/25/2013 11:35, schrieb John Keeping:
>> 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?
>
> Thanks. I had only tried trees that were "contaminated" by
> jk/difftool-dir-diff-edit-fix, which is in conflict with da/difftool-fixes.

Yes, the conflict was unpleasant to deal with.  John, I think what
is queued on 'pu' is OK, but please double check after I push it out
in a few hours.

Re: [PATCH v2] difftool: don't overwrite modified files

[Johannes Sixt]

Am 3/25/2013 22:44, schrieb John Keeping:
> After running the user's diff tool, git-difftool will copy any files
> that differ between the working tree and the temporary tree.  This is
> useful when the user edits the file in their diff tool but is wrong if
> they edit the working tree file while examining the diff.
> 
> Instead of copying unconditionally when the files differ, create and
> index from the working tree files and only copy the temporary file back
> if it was modified and the working tree file was not.  If both files
> have been modified, print a warning and exit with an error.
> 
> Note that we cannot use an existing index in git-difftool since those
> contain the modified files that need to be checked out but here we are
> looking at those files which are copied from the working tree and not
> checked out.  These are precisely the files which are not in the
> existing indices.
> 
> Signed-off-by: John Keeping <john@keeping.me.uk>
> ---
> On Mon, Mar 25, 2013 at 10:42:19AM +0000, John Keeping wrote:
>> On Mon, Mar 25, 2013 at 08:41:59AM +0100, Johannes Sixt wrote:
>>> This is gross. Can't we do much better here? Difftool already keeps a
>>> GIT_INDEX of the files in the temporary tree ($tmpdir/rindex). Running
>>> git-diff-files should be sufficient to tell which ones where edited via
>>> the users's diff-tool. Then you can restrict calling hash-object to only
>>> those worktree files where an "edit collision" needs to be checked for.
>>
>> That's only the case for files that are not copied from the working
>> tree, so the temporary index doesn't contain the files that are of
>> interest here.
>>
>>> You could also keep a parallel index that keeps the state of the same set
>>> of files in the worktree. Then another git-diff-files call could replace
>>> the other half of hash-object calls.
>>
>> I like the idea of creating an index from the working tree files and
>> using it here.  If we create a "starting state" index for these files,
>> we should be able to run git-diff-files against both the working tree
>> and the temporary tree at this point and compare the output.
> 
> Here's an attempt at taking this approach, built on
> jk/difftool-dir-diff-edit-fix.
> 
>  git-difftool.perl   | 73 +++++++++++++++++++++++++++++++++++++++++++----------
>  t/t7800-difftool.sh | 26 +++++++++++++++++++
>  2 files changed, 85 insertions(+), 14 deletions(-)
> 
> diff --git a/git-difftool.perl b/git-difftool.perl
> index c433e86..d10f7d2 100755
> --- a/git-difftool.perl
> +++ b/git-difftool.perl
> @@ -13,9 +13,9 @@
>  use 5.008;
>  use strict;
>  use warnings;
> +use Error qw(:try);
>  use File::Basename qw(dirname);
>  use File::Copy;
> -use File::Compare;
>  use File::Find;
>  use File::stat;
>  use File::Path qw(mkpath rmtree);
> @@ -88,14 +88,45 @@ sub use_wt_file
>  	my ($repo, $workdir, $file, $sha1, $symlinks) = @_;
>  	my $null_sha1 = '0' x 40;
>  
> -	if ($sha1 eq $null_sha1) {
> -		return 1;
> -	} elsif (not $symlinks) {
> +	if ($sha1 ne $null_sha1 and not $symlinks) {
>  		return 0;
>  	}
>  
>  	my $wt_sha1 = $repo->command_oneline('hash-object', "$workdir/$file");
> -	return $sha1 eq $wt_sha1;
> +	my $use = ($sha1 eq $null_sha1) || ($sha1 eq $wt_sha1);
> +	return ($use, $wt_sha1);
> +}
> +
> +sub changed_files
> +{
> +	my ($repo_path, $index, $worktree) = @_;
> +	$ENV{GIT_INDEX_FILE} = $index;
> +	$ENV{GIT_WORK_TREE} = $worktree;
> +	my $must_unset_git_dir = 0;
> +	if (not defined($ENV{GIT_DIR})) {
> +		$must_unset_git_dir = 1;
> +		$ENV{GIT_DIR} = $repo_path;
> +	}
> +
> +	my @refreshargs = qw/update-index --really-refresh -q --unmerged/;
> +	my @gitargs = qw/diff-files --name-only -z/;
> +	try {
> +		Git::command_oneline(@refreshargs);
> +	} catch Git::Error::Command with {};
> +
> +	my $line = Git::command_oneline(@gitargs);
> +	my @files;
> +	if (defined $line) {
> +		@files = split('\0', $line);
> +	} else {
> +		@files = ();
> +	}
> +
> +	delete($ENV{GIT_INDEX_FILE});
> +	delete($ENV{GIT_WORK_TREE});
> +	delete($ENV{GIT_DIR}) if ($must_unset_git_dir);
> +
> +	return map { $_ => 1 } @files;
>  }
>  
>  sub setup_dir_diff
> @@ -121,6 +152,7 @@ sub setup_dir_diff
>  	my $null_sha1 = '0' x 40;
>  	my $lindex = '';
>  	my $rindex = '';
> +	my $wtindex = '';
>  	my %submodule;
>  	my %symlink;
>  	my @working_tree = ();
> @@ -174,8 +206,12 @@ EOF
>  		}
>  
>  		if ($rmode ne $null_mode) {
> -			if (use_wt_file($repo, $workdir, $dst_path, $rsha1, $symlinks)) {
> -				push(@working_tree, $dst_path);
> +			my ($use, $wt_sha1) = use_wt_file($repo, $workdir,
> +							  $dst_path, $rsha1,
> +							  $symlinks);
> +			if ($use) {
> +				push @working_tree, $dst_path;
> +				$wtindex .= "$rmode $wt_sha1\t$dst_path\0";
>  			} else {
>  				$rindex .= "$rmode $rsha1\t$dst_path\0";
>  			}
> @@ -218,6 +254,12 @@ EOF
>  	$rc = system('git', 'checkout-index', '--all', "--prefix=$rdir/");
>  	exit_cleanup($tmpdir, $rc) if $rc != 0;
>  
> +	$ENV{GIT_INDEX_FILE} = "$tmpdir/wtindex";
> +	($inpipe, $ctx) =
> +		$repo->command_input_pipe(qw(update-index --info-only -z --index-info));
> +	print($inpipe $wtindex);
> +	$repo->command_close_pipe($inpipe, $ctx);
> +
>  	# If $GIT_DIR was explicitly set just for the update/checkout
>  	# commands, then it should be unset before continuing.
>  	delete($ENV{GIT_DIR}) if ($must_unset_git_dir);
> @@ -390,19 +432,22 @@ sub dir_diff
>  	# should be copied back to the working tree.
>  	# Do not copy back files when symlinks are used and the
>  	# external tool did not replace the original link with a file.
> +	my %wt_modified = changed_files($repo->repo_path(),
> +		"$tmpdir/wtindex", "$workdir");
> +	my %tmp_modified = changed_files($repo->repo_path(),
> +		"$tmpdir/wtindex", "$b");
>  	for my $file (@worktree) {
>  		next if $symlinks && -l "$b/$file";
>  		next if ! -f "$b/$file";
>  
> -		my $diff = compare("$b/$file", "$workdir/$file");
> -		if ($diff == 0) {
> -			next;
> -		} elsif ($diff == -1) {
> -			my $errmsg = "warning: Could not compare ";
> -			$errmsg += "'$b/$file' with '$workdir/$file'\n";
> +		if (exists $wt_modified{$file} and exists $tmp_modified{$file}) {
> +			my $errmsg = "warning: Both files modified: ";
> +			$errmsg .= "'$workdir/$file' and '$b/$file'.\n";
> +			$errmsg .= "warning: Working tree file has been left.\n";
> +			$errmsg .= "warning:\n";
>  			warn $errmsg;
>  			$error = 1;
> -		} elsif ($diff == 1) {
> +		} elsif ($tmp_modified{$file}) {
>  			my $mode = stat("$b/$file")->mode;
>  			copy("$b/$file", "$workdir/$file") or
>  			exit_cleanup($tmpdir, 1);

I don't have a lot to say about the patch text, except that there is
nothing obvious out of the ordinary, but please take this with a large
grain of salt, as I'm lacking context. (It's the first time these days
that I'm looking at difftool.)

BTW, did you know that perl is mostly a write-only language? ;-)

> diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
> index db3d3d6..be2042d 100755
> --- a/t/t7800-difftool.sh
> +++ b/t/t7800-difftool.sh
> @@ -407,4 +407,30 @@ test_expect_success PERL 'difftool --dir-diff from subdirectory' '
>  	)
>  '
>  
> +write_script modify-file <<\EOF
> +echo "new content" >file
> +EOF
> +
> +test_expect_success PERL 'difftool --no-symlinks does not overwrite working tree file ' '
> +	echo "orig content" >file &&
> +	git difftool --dir-diff --no-symlinks --extcmd "$(pwd)/modify-file" branch &&
> +	echo "new content" >expect &&
> +	test_cmp expect file
> +'
> +
> +write_script modify-both-files <<\EOF
> +echo "wt content" >file &&
> +echo "tmp content" >"$2/file" &&
> +echo "$2" >tmpdir
> +EOF
> +
> +test_expect_success PERL 'difftool --no-symlinks detects conflict ' '
> +	echo "orig content" >file &&
> +	test_must_fail git difftool --dir-diff --no-symlinks --extcmd "$(pwd)/modify-both-files" branch &&
> +	echo "wt content" >expect &&
> +	test_cmp expect file &&
> +	echo "tmp content" >expect &&
> +	test_cmp expect "$(cat tmpdir)/file"
> +'

The new tests look good.

One question though: Do I understand correctly that the temporary
directories are leaked in the case of an "edit conflict"? If so, is it
worth a warning for the user to clean up the garbage?

-- Hannes

Re: [PATCH v2] difftool: don't overwrite modified files

[Johannes Sixt]

Forgot to mention: The patch passes t7800 on Windows.

Thanks,
-- Hannes

Re: [PATCH v2 3/3] t7800: run --dir-diff tests with and without symlinks

[John Keeping]

On Mon, Mar 25, 2013 at 02:50:38PM -0700, Junio C Hamano wrote:
> Johannes Sixt <j.sixt@viscovery.net> writes:
> 
> > Am 3/25/2013 11:35, schrieb John Keeping:
> >> 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?
> >
> > Thanks. I had only tried trees that were "contaminated" by
> > jk/difftool-dir-diff-edit-fix, which is in conflict with da/difftool-fixes.
> 
> Yes, the conflict was unpleasant to deal with.  John, I think what
> is queued on 'pu' is OK, but please double check after I push it out
> in a few hours.

The result looks good to me.

Re: [PATCH v2] difftool: don't overwrite modified files

[John Keeping]

On Tue, Mar 26, 2013 at 09:38:42AM +0100, Johannes Sixt wrote:
> Am 3/25/2013 22:44, schrieb John Keeping:
> > After running the user's diff tool, git-difftool will copy any files
> > that differ between the working tree and the temporary tree.  This is
> > useful when the user edits the file in their diff tool but is wrong if
> > they edit the working tree file while examining the diff.
> > 
> > Instead of copying unconditionally when the files differ, create and
> > index from the working tree files and only copy the temporary file back
> > if it was modified and the working tree file was not.  If both files
> > have been modified, print a warning and exit with an error.
> > 
> > Note that we cannot use an existing index in git-difftool since those
> > contain the modified files that need to be checked out but here we are
> > looking at those files which are copied from the working tree and not
> > checked out.  These are precisely the files which are not in the
> > existing indices.
> > 
> > Signed-off-by: John Keeping <john@keeping.me.uk>
> > ---
> > On Mon, Mar 25, 2013 at 10:42:19AM +0000, John Keeping wrote:
> >> On Mon, Mar 25, 2013 at 08:41:59AM +0100, Johannes Sixt wrote:
> >>> This is gross. Can't we do much better here? Difftool already keeps a
> >>> GIT_INDEX of the files in the temporary tree ($tmpdir/rindex). Running
> >>> git-diff-files should be sufficient to tell which ones where edited via
> >>> the users's diff-tool. Then you can restrict calling hash-object to only
> >>> those worktree files where an "edit collision" needs to be checked for.
> >>
> >> That's only the case for files that are not copied from the working
> >> tree, so the temporary index doesn't contain the files that are of
> >> interest here.
> >>
> >>> You could also keep a parallel index that keeps the state of the same set
> >>> of files in the worktree. Then another git-diff-files call could replace
> >>> the other half of hash-object calls.
> >>
> >> I like the idea of creating an index from the working tree files and
> >> using it here.  If we create a "starting state" index for these files,
> >> we should be able to run git-diff-files against both the working tree
> >> and the temporary tree at this point and compare the output.
> > 
> > Here's an attempt at taking this approach, built on
> > jk/difftool-dir-diff-edit-fix.
> > 
> >  git-difftool.perl   | 73 +++++++++++++++++++++++++++++++++++++++++++----------
> >  t/t7800-difftool.sh | 26 +++++++++++++++++++
> >  2 files changed, 85 insertions(+), 14 deletions(-)
> > 
> > diff --git a/git-difftool.perl b/git-difftool.perl
> > index c433e86..d10f7d2 100755
> > --- a/git-difftool.perl
> > +++ b/git-difftool.perl
> > @@ -13,9 +13,9 @@
> >  use 5.008;
> >  use strict;
> >  use warnings;
> > +use Error qw(:try);
> >  use File::Basename qw(dirname);
> >  use File::Copy;
> > -use File::Compare;
> >  use File::Find;
> >  use File::stat;
> >  use File::Path qw(mkpath rmtree);
> > @@ -88,14 +88,45 @@ sub use_wt_file
> >  	my ($repo, $workdir, $file, $sha1, $symlinks) = @_;
> >  	my $null_sha1 = '0' x 40;
> >  
> > -	if ($sha1 eq $null_sha1) {
> > -		return 1;
> > -	} elsif (not $symlinks) {
> > +	if ($sha1 ne $null_sha1 and not $symlinks) {
> >  		return 0;
> >  	}
> >  
> >  	my $wt_sha1 = $repo->command_oneline('hash-object', "$workdir/$file");
> > -	return $sha1 eq $wt_sha1;
> > +	my $use = ($sha1 eq $null_sha1) || ($sha1 eq $wt_sha1);
> > +	return ($use, $wt_sha1);
> > +}
> > +
> > +sub changed_files
> > +{
> > +	my ($repo_path, $index, $worktree) = @_;
> > +	$ENV{GIT_INDEX_FILE} = $index;
> > +	$ENV{GIT_WORK_TREE} = $worktree;
> > +	my $must_unset_git_dir = 0;
> > +	if (not defined($ENV{GIT_DIR})) {
> > +		$must_unset_git_dir = 1;
> > +		$ENV{GIT_DIR} = $repo_path;
> > +	}
> > +
> > +	my @refreshargs = qw/update-index --really-refresh -q --unmerged/;
> > +	my @gitargs = qw/diff-files --name-only -z/;
> > +	try {
> > +		Git::command_oneline(@refreshargs);
> > +	} catch Git::Error::Command with {};
> > +
> > +	my $line = Git::command_oneline(@gitargs);
> > +	my @files;
> > +	if (defined $line) {
> > +		@files = split('\0', $line);
> > +	} else {
> > +		@files = ();
> > +	}
> > +
> > +	delete($ENV{GIT_INDEX_FILE});
> > +	delete($ENV{GIT_WORK_TREE});
> > +	delete($ENV{GIT_DIR}) if ($must_unset_git_dir);
> > +
> > +	return map { $_ => 1 } @files;
> >  }
> >  
> >  sub setup_dir_diff
> > @@ -121,6 +152,7 @@ sub setup_dir_diff
> >  	my $null_sha1 = '0' x 40;
> >  	my $lindex = '';
> >  	my $rindex = '';
> > +	my $wtindex = '';
> >  	my %submodule;
> >  	my %symlink;
> >  	my @working_tree = ();
> > @@ -174,8 +206,12 @@ EOF
> >  		}
> >  
> >  		if ($rmode ne $null_mode) {
> > -			if (use_wt_file($repo, $workdir, $dst_path, $rsha1, $symlinks)) {
> > -				push(@working_tree, $dst_path);
> > +			my ($use, $wt_sha1) = use_wt_file($repo, $workdir,
> > +							  $dst_path, $rsha1,
> > +							  $symlinks);
> > +			if ($use) {
> > +				push @working_tree, $dst_path;
> > +				$wtindex .= "$rmode $wt_sha1\t$dst_path\0";
> >  			} else {
> >  				$rindex .= "$rmode $rsha1\t$dst_path\0";
> >  			}
> > @@ -218,6 +254,12 @@ EOF
> >  	$rc = system('git', 'checkout-index', '--all', "--prefix=$rdir/");
> >  	exit_cleanup($tmpdir, $rc) if $rc != 0;
> >  
> > +	$ENV{GIT_INDEX_FILE} = "$tmpdir/wtindex";
> > +	($inpipe, $ctx) =
> > +		$repo->command_input_pipe(qw(update-index --info-only -z --index-info));
> > +	print($inpipe $wtindex);
> > +	$repo->command_close_pipe($inpipe, $ctx);
> > +
> >  	# If $GIT_DIR was explicitly set just for the update/checkout
> >  	# commands, then it should be unset before continuing.
> >  	delete($ENV{GIT_DIR}) if ($must_unset_git_dir);
> > @@ -390,19 +432,22 @@ sub dir_diff
> >  	# should be copied back to the working tree.
> >  	# Do not copy back files when symlinks are used and the
> >  	# external tool did not replace the original link with a file.
> > +	my %wt_modified = changed_files($repo->repo_path(),
> > +		"$tmpdir/wtindex", "$workdir");
> > +	my %tmp_modified = changed_files($repo->repo_path(),
> > +		"$tmpdir/wtindex", "$b");
> >  	for my $file (@worktree) {
> >  		next if $symlinks && -l "$b/$file";
> >  		next if ! -f "$b/$file";
> >  
> > -		my $diff = compare("$b/$file", "$workdir/$file");
> > -		if ($diff == 0) {
> > -			next;
> > -		} elsif ($diff == -1) {
> > -			my $errmsg = "warning: Could not compare ";
> > -			$errmsg += "'$b/$file' with '$workdir/$file'\n";
> > +		if (exists $wt_modified{$file} and exists $tmp_modified{$file}) {
> > +			my $errmsg = "warning: Both files modified: ";
> > +			$errmsg .= "'$workdir/$file' and '$b/$file'.\n";
> > +			$errmsg .= "warning: Working tree file has been left.\n";
> > +			$errmsg .= "warning:\n";
> >  			warn $errmsg;
> >  			$error = 1;
> > -		} elsif ($diff == 1) {
> > +		} elsif ($tmp_modified{$file}) {
> >  			my $mode = stat("$b/$file")->mode;
> >  			copy("$b/$file", "$workdir/$file") or
> >  			exit_cleanup($tmpdir, 1);
> 
> I don't have a lot to say about the patch text, except that there is
> nothing obvious out of the ordinary, but please take this with a large
> grain of salt, as I'm lacking context. (It's the first time these days
> that I'm looking at difftool.)
> 
> BTW, did you know that perl is mostly a write-only language? ;-)
> 
> > diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
> > index db3d3d6..be2042d 100755
> > --- a/t/t7800-difftool.sh
> > +++ b/t/t7800-difftool.sh
> > @@ -407,4 +407,30 @@ test_expect_success PERL 'difftool --dir-diff from subdirectory' '
> >  	)
> >  '
> >  
> > +write_script modify-file <<\EOF
> > +echo "new content" >file
> > +EOF
> > +
> > +test_expect_success PERL 'difftool --no-symlinks does not overwrite working tree file ' '
> > +	echo "orig content" >file &&
> > +	git difftool --dir-diff --no-symlinks --extcmd "$(pwd)/modify-file" branch &&
> > +	echo "new content" >expect &&
> > +	test_cmp expect file
> > +'
> > +
> > +write_script modify-both-files <<\EOF
> > +echo "wt content" >file &&
> > +echo "tmp content" >"$2/file" &&
> > +echo "$2" >tmpdir
> > +EOF
> > +
> > +test_expect_success PERL 'difftool --no-symlinks detects conflict ' '
> > +	echo "orig content" >file &&
> > +	test_must_fail git difftool --dir-diff --no-symlinks --extcmd "$(pwd)/modify-both-files" branch &&
> > +	echo "wt content" >expect &&
> > +	test_cmp expect file &&
> > +	echo "tmp content" >expect &&
> > +	test_cmp expect "$(cat tmpdir)/file"
> > +'
> 
> The new tests look good.
> 
> One question though: Do I understand correctly that the temporary
> directories are leaked in the case of an "edit conflict"? If so, is it
> worth a warning for the user to clean up the garbage?

Do you mean for normal users or for those running the tests?  In normal
usage we do print a warning - it's in the existing code, triggered by
setting "$error = 1" - you can see that if you run the tests with "-v".

The last test does result in /tmp filling up with temporary directories
though, it would be good if the test could clean up after itself.  The
best I can come up with is adding something like this immediately after
running difftool but I'm not entirely happy with the ".." in the
argument to rm:

	test_when_finished rm -rf "$(cat tmpdir)/.."

Re: [PATCH v2] difftool: don't overwrite modified files

[Johannes Sixt]

Am 3/26/2013 10:31, schrieb John Keeping:
> On Tue, Mar 26, 2013 at 09:38:42AM +0100, Johannes Sixt wrote:
>> One question though: Do I understand correctly that the temporary
>> directories are leaked in the case of an "edit conflict"? If so, is it
>> worth a warning for the user to clean up the garbage?
> 
> Do you mean for normal users or for those running the tests?  In normal
> usage we do print a warning - it's in the existing code, triggered by
> setting "$error = 1" - you can see that if you run the tests with "-v".

I meant for normal users. I see the error now. Thanks.

> The last test does result in /tmp filling up with temporary directories
> though, it would be good if the test could clean up after itself.  The
> best I can come up with is adding something like this immediately after
> running difftool but I'm not entirely happy with the ".." in the
> argument to rm:
> 
> 	test_when_finished rm -rf "$(cat tmpdir)/.."

Wrap the test in

	(
		TMPDIR=$TRASH_DIRECTORY &&
		export TMPDIR &&
	...
	)

It works for me.

-- Hannes

Re: [PATCH v2] difftool: don't overwrite modified files

[John Keeping]

On Tue, Mar 26, 2013 at 10:53:48AM +0100, Johannes Sixt wrote:
> Am 3/26/2013 10:31, schrieb John Keeping:
> > On Tue, Mar 26, 2013 at 09:38:42AM +0100, Johannes Sixt wrote:
> > The last test does result in /tmp filling up with temporary directories
> > though, it would be good if the test could clean up after itself.  The
> > best I can come up with is adding something like this immediately after
> > running difftool but I'm not entirely happy with the ".." in the
> > argument to rm:
> > 
> > 	test_when_finished rm -rf "$(cat tmpdir)/.."
> 
> Wrap the test in
> 
> 	(
> 		TMPDIR=$TRASH_DIRECTORY &&
> 		export TMPDIR &&
> 	...
> 	)
> 
> It works for me.

Nice.  I've reviewed File::Spec and it looks like that TMPDIR takes
priority on every operating system except VMS, and I don't think we care
about that.

Unless Junio says otherwise, I'll hold off sending this until difftool
calms down a bit to avoid too many conflicted merges.

Re: [PATCH v2] difftool: don't overwrite modified files

[Matt McClure]

On Mon, Mar 25, 2013 at 5:44 PM, John Keeping <john@keeping.me.uk> wrote:
> Instead of copying unconditionally when the files differ, create and
> index from the working tree files and only copy the temporary file back
> if it was modified and the working tree file was not.  If both files
> have been modified, print a warning and exit with an error.

When there's a conflict, does difftool save both conflicting files? Or
only the working tree copy? I think it should preserve both copies on
disk.

-- 
Matt McClure
http://www.matthewlmcclure.com
http://www.mapmyfitness.com/profile/matthewlmcclure

Re: [PATCH v2] difftool: don't overwrite modified files

[John Keeping]

On Tue, Mar 26, 2013 at 04:52:02PM -0400, Matt McClure wrote:
> On Mon, Mar 25, 2013 at 5:44 PM, John Keeping <john@keeping.me.uk> wrote:
> > Instead of copying unconditionally when the files differ, create and
> > index from the working tree files and only copy the temporary file back
> > if it was modified and the working tree file was not.  If both files
> > have been modified, print a warning and exit with an error.
> 
> When there's a conflict, does difftool save both conflicting files? Or
> only the working tree copy? I think it should preserve both copies on
> disk.

It preserves both copies - the "clean the temporary directory" step is
just skipped.

This isn't ideal since the temporary copy will be under a temporary
directory somewhere but is better than the current behaviour.  It might
be nice to move the temporary file back with an extension so that the
files are at least near each other but I don't think that's needed in
the first version of this change.

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

[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