* [PATCH v3 1/4] difftool: silence uninitialized variable warning @ 2013-02-21 4:03 David Aguilar 2013-02-21 4:03 ` [PATCH 2/4] t7800: update copyright notice David Aguilar 0 siblings, 1 reply; 55+ messages in thread From: David Aguilar @ 2013-02-21 4:03 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, Jonathan Nieder 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 ^ permalink raw reply related [flat|nested] 55+ messages in thread
* [PATCH 2/4] t7800: update copyright notice 2013-02-21 4:03 [PATCH v3 1/4] difftool: silence uninitialized variable warning David Aguilar @ 2013-02-21 4:03 ` David Aguilar 2013-02-21 4:03 ` [PATCH v3 3/4] t7800: modernize tests David Aguilar 0 siblings, 1 reply; 55+ messages in thread From: David Aguilar @ 2013-02-21 4:03 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, Jonathan Nieder 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 ^ permalink raw reply related [flat|nested] 55+ messages in thread
* [PATCH v3 3/4] t7800: modernize tests 2013-02-21 4:03 ` [PATCH 2/4] t7800: update copyright notice David Aguilar @ 2013-02-21 4:03 ` David Aguilar 2013-02-21 4:03 ` [PATCH v3 4/4] t7800: "defaults" is no longer a builtin tool name David Aguilar 2013-03-20 9:48 ` [PATCH v3 3/4] t7800: modernize tests Johannes Sixt 0 siblings, 2 replies; 55+ messages in thread From: David Aguilar @ 2013-02-21 4:03 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, Jonathan Nieder 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 ^ permalink raw reply related [flat|nested] 55+ messages in thread
* [PATCH v3 4/4] t7800: "defaults" is no longer a builtin tool name 2013-02-21 4:03 ` [PATCH v3 3/4] t7800: modernize tests David Aguilar @ 2013-02-21 4:03 ` David Aguilar 2013-02-21 4:55 ` Junio C Hamano 2013-02-21 5:00 ` Junio C Hamano 2013-03-20 9:48 ` [PATCH v3 3/4] t7800: modernize tests Johannes Sixt 1 sibling, 2 replies; 55+ messages in thread From: David Aguilar @ 2013-02-21 4:03 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, Jonathan Nieder 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 ^ permalink raw reply related [flat|nested] 55+ messages in thread
* Re: [PATCH v3 4/4] t7800: "defaults" is no longer a builtin tool name 2013-02-21 4:03 ` [PATCH v3 4/4] t7800: "defaults" is no longer a builtin tool name David Aguilar @ 2013-02-21 4:55 ` Junio C Hamano 2013-02-21 5:00 ` Junio C Hamano 1 sibling, 0 replies; 55+ messages in thread From: Junio C Hamano @ 2013-02-21 4:55 UTC (permalink / raw) To: David Aguilar; +Cc: git, Jonathan Nieder Thanks; will replace and queue. ^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH v3 4/4] t7800: "defaults" is no longer a builtin tool name 2013-02-21 4:03 ` [PATCH v3 4/4] t7800: "defaults" is no longer a builtin tool name David Aguilar 2013-02-21 4:55 ` Junio C Hamano @ 2013-02-21 5:00 ` Junio C Hamano 2013-02-21 23:31 ` David Aguilar 1 sibling, 1 reply; 55+ messages in thread From: Junio C Hamano @ 2013-02-21 5:00 UTC (permalink / raw) To: David Aguilar; +Cc: git, Jonathan Nieder 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 ^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH v3 4/4] t7800: "defaults" is no longer a builtin tool name 2013-02-21 5:00 ` Junio C Hamano @ 2013-02-21 23:31 ` David Aguilar 0 siblings, 0 replies; 55+ messages in thread From: David Aguilar @ 2013-02-21 23:31 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, Jonathan Nieder 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 ^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH v3 3/4] t7800: modernize tests 2013-02-21 4:03 ` [PATCH v3 3/4] t7800: modernize tests David Aguilar 2013-02-21 4:03 ` [PATCH v3 4/4] t7800: "defaults" is no longer a builtin tool name David Aguilar @ 2013-03-20 9:48 ` Johannes Sixt 2013-03-20 22:59 ` David Aguilar 1 sibling, 1 reply; 55+ messages in thread From: Johannes Sixt @ 2013-03-20 9:48 UTC (permalink / raw) To: David Aguilar; +Cc: Junio C Hamano, git, Jonathan Nieder 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 ^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH v3 3/4] t7800: modernize tests 2013-03-20 9:48 ` [PATCH v3 3/4] t7800: modernize tests Johannes Sixt @ 2013-03-20 22:59 ` David Aguilar 2013-03-21 7:41 ` Johannes Sixt 0 siblings, 1 reply; 55+ messages in thread From: David Aguilar @ 2013-03-20 22:59 UTC (permalink / raw) To: Johannes Sixt, Sitaram Chamarty; +Cc: Junio C Hamano, git, Jonathan Nieder 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 ^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH v3 3/4] t7800: modernize tests 2013-03-20 22:59 ` David Aguilar @ 2013-03-21 7:41 ` Johannes Sixt 2013-03-22 7:13 ` Johannes Sixt 0 siblings, 1 reply; 55+ messages in thread From: Johannes Sixt @ 2013-03-21 7:41 UTC (permalink / raw) To: David Aguilar; +Cc: Sitaram Chamarty, Junio C Hamano, git, Jonathan Nieder 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 ^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH v3 3/4] t7800: modernize tests 2013-03-21 7:41 ` Johannes Sixt @ 2013-03-22 7:13 ` Johannes Sixt 2013-03-22 10:00 ` John Keeping 0 siblings, 1 reply; 55+ messages in thread From: Johannes Sixt @ 2013-03-22 7:13 UTC (permalink / raw) To: David Aguilar; +Cc: Sitaram Chamarty, Junio C Hamano, git, Jonathan Nieder 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 ^ permalink raw reply related [flat|nested] 55+ messages in thread
* Re: [PATCH v3 3/4] t7800: modernize tests 2013-03-22 7:13 ` Johannes Sixt @ 2013-03-22 10:00 ` John Keeping 2013-03-22 11:14 ` Johannes Sixt 0 siblings, 1 reply; 55+ messages in thread From: John Keeping @ 2013-03-22 10:00 UTC (permalink / raw) To: Johannes Sixt Cc: David Aguilar, Sitaram Chamarty, Junio C Hamano, git, Jonathan Nieder 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 ^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH v3 3/4] t7800: modernize tests 2013-03-22 10:00 ` John Keeping @ 2013-03-22 11:14 ` Johannes Sixt 2013-03-22 11:53 ` John Keeping 0 siblings, 1 reply; 55+ messages in thread From: Johannes Sixt @ 2013-03-22 11:14 UTC (permalink / raw) To: John Keeping Cc: David Aguilar, Sitaram Chamarty, Junio C Hamano, git, Jonathan Nieder 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 ^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH v3 3/4] t7800: modernize tests 2013-03-22 11:14 ` Johannes Sixt @ 2013-03-22 11:53 ` John Keeping 2013-03-22 19:36 ` [PATCH 0/3] Improve difftool --dir-diff tests John Keeping 0 siblings, 1 reply; 55+ messages in thread From: John Keeping @ 2013-03-22 11:53 UTC (permalink / raw) To: Johannes Sixt Cc: David Aguilar, Sitaram Chamarty, Junio C Hamano, git, Jonathan Nieder 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 ^ permalink raw reply [flat|nested] 55+ messages in thread
* [PATCH 0/3] Improve difftool --dir-diff tests 2013-03-22 11:53 ` John Keeping @ 2013-03-22 19:36 ` John Keeping 2013-03-22 19:36 ` [PATCH 1/3] t7800: don't hide grep output John Keeping ` (3 more replies) 0 siblings, 4 replies; 55+ messages in thread From: John Keeping @ 2013-03-22 19:36 UTC (permalink / raw) To: git Cc: Johannes Sixt, David Aguilar, Sitaram Chamarty, Junio C Hamano, Jonathan Nieder, 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 ^ permalink raw reply [flat|nested] 55+ messages in thread
* [PATCH 1/3] t7800: don't hide grep output 2013-03-22 19:36 ` [PATCH 0/3] Improve difftool --dir-diff tests John Keeping @ 2013-03-22 19:36 ` John Keeping 2013-03-22 22:32 ` Johannes Sixt 2013-03-22 22:45 ` Junio C Hamano 2013-03-22 19:36 ` [PATCH 2/3] t7800: fix tests when difftool uses --no-symlinks John Keeping ` (2 subsequent siblings) 3 siblings, 2 replies; 55+ messages in thread From: John Keeping @ 2013-03-22 19:36 UTC (permalink / raw) To: git Cc: Johannes Sixt, David Aguilar, Sitaram Chamarty, Junio C Hamano, Jonathan Nieder, 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 ^ permalink raw reply related [flat|nested] 55+ messages in thread
* Re: [PATCH 1/3] t7800: don't hide grep output 2013-03-22 19:36 ` [PATCH 1/3] t7800: don't hide grep output John Keeping @ 2013-03-22 22:32 ` Johannes Sixt 2013-03-22 22:45 ` Junio C Hamano 1 sibling, 0 replies; 55+ messages in thread From: Johannes Sixt @ 2013-03-22 22:32 UTC (permalink / raw) To: John Keeping Cc: git, David Aguilar, Sitaram Chamarty, Junio C Hamano, Jonathan Nieder 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 ^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH 1/3] t7800: don't hide grep output 2013-03-22 19:36 ` [PATCH 1/3] t7800: don't hide grep output John Keeping 2013-03-22 22:32 ` Johannes Sixt @ 2013-03-22 22:45 ` Junio C Hamano 1 sibling, 0 replies; 55+ messages in thread From: Junio C Hamano @ 2013-03-22 22:45 UTC (permalink / raw) To: John Keeping Cc: git, Johannes Sixt, David Aguilar, Sitaram Chamarty, Jonathan Nieder Looks good from a cursory read. Thanks. ^ permalink raw reply [flat|nested] 55+ messages in thread
* [PATCH 2/3] t7800: fix tests when difftool uses --no-symlinks 2013-03-22 19:36 ` [PATCH 0/3] Improve difftool --dir-diff tests John Keeping 2013-03-22 19:36 ` [PATCH 1/3] t7800: don't hide grep output John Keeping @ 2013-03-22 19:36 ` John Keeping 2013-03-22 22:27 ` Johannes Sixt 2013-03-22 22:53 ` Junio C Hamano 2013-03-22 19:36 ` [PATCH 3/3] t7800: run --dir-diff tests with and without symlinks John Keeping 2013-03-23 13:31 ` [PATCH v2 0/3] difftool --dir-diff test improvements John Keeping 3 siblings, 2 replies; 55+ messages in thread From: John Keeping @ 2013-03-22 19:36 UTC (permalink / raw) To: git Cc: Johannes Sixt, David Aguilar, Sitaram Chamarty, Junio C Hamano, Jonathan Nieder, 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 ^ permalink raw reply related [flat|nested] 55+ messages in thread
* Re: [PATCH 2/3] t7800: fix tests when difftool uses --no-symlinks 2013-03-22 19:36 ` [PATCH 2/3] t7800: fix tests when difftool uses --no-symlinks John Keeping @ 2013-03-22 22:27 ` Johannes Sixt 2013-03-22 22:53 ` Junio C Hamano 1 sibling, 0 replies; 55+ messages in thread From: Johannes Sixt @ 2013-03-22 22:27 UTC (permalink / raw) To: John Keeping Cc: git, David Aguilar, Sitaram Chamarty, Junio C Hamano, Jonathan Nieder 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 > ' > ... ^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH 2/3] t7800: fix tests when difftool uses --no-symlinks 2013-03-22 19:36 ` [PATCH 2/3] t7800: fix tests when difftool uses --no-symlinks John Keeping 2013-03-22 22:27 ` Johannes Sixt @ 2013-03-22 22:53 ` Junio C Hamano 2013-03-22 23:05 ` John Keeping 1 sibling, 1 reply; 55+ messages in thread From: Junio C Hamano @ 2013-03-22 22:53 UTC (permalink / raw) To: John Keeping Cc: git, Johannes Sixt, David Aguilar, Sitaram Chamarty, Jonathan Nieder 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 > ) > ' ^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH 2/3] t7800: fix tests when difftool uses --no-symlinks 2013-03-22 22:53 ` Junio C Hamano @ 2013-03-22 23:05 ` John Keeping 2013-03-23 3:24 ` David Aguilar 0 siblings, 1 reply; 55+ messages in thread From: John Keeping @ 2013-03-22 23:05 UTC (permalink / raw) To: Junio C Hamano Cc: git, Johannes Sixt, David Aguilar, Sitaram Chamarty, Jonathan Nieder 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 ^ permalink raw reply related [flat|nested] 55+ messages in thread
* Re: [PATCH 2/3] t7800: fix tests when difftool uses --no-symlinks 2013-03-22 23:05 ` John Keeping @ 2013-03-23 3:24 ` David Aguilar 0 siblings, 0 replies; 55+ messages in thread From: David Aguilar @ 2013-03-23 3:24 UTC (permalink / raw) To: John Keeping Cc: Junio C Hamano, git, Johannes Sixt, Sitaram Chamarty, Jonathan Nieder 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 ^ permalink raw reply [flat|nested] 55+ messages in thread
* [PATCH 3/3] t7800: run --dir-diff tests with and without symlinks 2013-03-22 19:36 ` [PATCH 0/3] Improve difftool --dir-diff tests John Keeping 2013-03-22 19:36 ` [PATCH 1/3] t7800: don't hide grep output John Keeping 2013-03-22 19:36 ` [PATCH 2/3] t7800: fix tests when difftool uses --no-symlinks John Keeping @ 2013-03-22 19:36 ` John Keeping 2013-03-22 21:05 ` [PATCH 3/3 v2] " John Keeping 2013-03-23 13:31 ` [PATCH v2 0/3] difftool --dir-diff test improvements John Keeping 3 siblings, 1 reply; 55+ messages in thread From: John Keeping @ 2013-03-22 19:36 UTC (permalink / raw) To: git Cc: Johannes Sixt, David Aguilar, Sitaram Chamarty, Junio C Hamano, Jonathan Nieder, 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 ^ permalink raw reply related [flat|nested] 55+ messages in thread
* [PATCH 3/3 v2] t7800: run --dir-diff tests with and without symlinks 2013-03-22 19:36 ` [PATCH 3/3] t7800: run --dir-diff tests with and without symlinks John Keeping @ 2013-03-22 21:05 ` John Keeping 0 siblings, 0 replies; 55+ messages in thread From: John Keeping @ 2013-03-22 21:05 UTC (permalink / raw) To: git Cc: Johannes Sixt, David Aguilar, Sitaram Chamarty, Junio C Hamano, Jonathan Nieder 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 ^ permalink raw reply related [flat|nested] 55+ messages in thread
* [PATCH v2 0/3] difftool --dir-diff test improvements 2013-03-22 19:36 ` [PATCH 0/3] Improve difftool --dir-diff tests John Keeping ` (2 preceding siblings ...) 2013-03-22 19:36 ` [PATCH 3/3] t7800: run --dir-diff tests with and without symlinks John Keeping @ 2013-03-23 13:31 ` John Keeping 2013-03-23 13:31 ` [PATCH v2 1/3] t7800: don't hide grep output John Keeping ` (2 more replies) 3 siblings, 3 replies; 55+ messages in thread From: John Keeping @ 2013-03-23 13:31 UTC (permalink / raw) To: git Cc: Johannes Sixt, David Aguilar, Sitaram Chamarty, Junio C Hamano, Jonathan Nieder, 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 ^ permalink raw reply [flat|nested] 55+ messages in thread
* [PATCH v2 1/3] t7800: don't hide grep output 2013-03-23 13:31 ` [PATCH v2 0/3] difftool --dir-diff test improvements John Keeping @ 2013-03-23 13:31 ` John Keeping 2013-03-23 13:31 ` [PATCH v2 2/3] t7800: fix tests when difftool uses --no-symlinks John Keeping 2013-03-23 13:31 ` [PATCH v2 3/3] t7800: run --dir-diff tests with and without symlinks John Keeping 2 siblings, 0 replies; 55+ messages in thread From: John Keeping @ 2013-03-23 13:31 UTC (permalink / raw) To: git Cc: Johannes Sixt, David Aguilar, Sitaram Chamarty, Junio C Hamano, Jonathan Nieder, 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 ^ permalink raw reply related [flat|nested] 55+ messages in thread
* [PATCH v2 2/3] t7800: fix tests when difftool uses --no-symlinks 2013-03-23 13:31 ` [PATCH v2 0/3] difftool --dir-diff test improvements John Keeping 2013-03-23 13:31 ` [PATCH v2 1/3] t7800: don't hide grep output John Keeping @ 2013-03-23 13:31 ` John Keeping 2013-03-24 5:19 ` Junio C Hamano 2013-03-24 6:20 ` Eric Sunshine 2013-03-23 13:31 ` [PATCH v2 3/3] t7800: run --dir-diff tests with and without symlinks John Keeping 2 siblings, 2 replies; 55+ messages in thread From: John Keeping @ 2013-03-23 13:31 UTC (permalink / raw) To: git Cc: Johannes Sixt, David Aguilar, Sitaram Chamarty, Junio C Hamano, Jonathan Nieder, 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 ^ permalink raw reply related [flat|nested] 55+ messages in thread
* Re: [PATCH v2 2/3] t7800: fix tests when difftool uses --no-symlinks 2013-03-23 13:31 ` [PATCH v2 2/3] t7800: fix tests when difftool uses --no-symlinks John Keeping @ 2013-03-24 5:19 ` Junio C Hamano 2013-03-24 12:36 ` John Keeping 2013-03-24 13:24 ` Matt McClure 2013-03-24 6:20 ` Eric Sunshine 1 sibling, 2 replies; 55+ messages in thread From: Junio C Hamano @ 2013-03-24 5:19 UTC (permalink / raw) To: John Keeping Cc: git, Johannes Sixt, David Aguilar, Sitaram Chamarty, Jonathan Nieder 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" > ' ^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH v2 2/3] t7800: fix tests when difftool uses --no-symlinks 2013-03-24 5:19 ` Junio C Hamano @ 2013-03-24 12:36 ` John Keeping 2013-03-24 13:31 ` Matt McClure 2013-03-24 21:29 ` David Aguilar 2013-03-24 13:24 ` Matt McClure 1 sibling, 2 replies; 55+ messages in thread From: John Keeping @ 2013-03-24 12:36 UTC (permalink / raw) To: Junio C Hamano Cc: git, Johannes Sixt, David Aguilar, Sitaram Chamarty, Jonathan Nieder 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. ^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH v2 2/3] t7800: fix tests when difftool uses --no-symlinks 2013-03-24 12:36 ` John Keeping @ 2013-03-24 13:31 ` Matt McClure 2013-03-24 15:15 ` John Keeping 2013-03-24 21:29 ` David Aguilar 1 sibling, 1 reply; 55+ messages in thread From: Matt McClure @ 2013-03-24 13:31 UTC (permalink / raw) To: John Keeping Cc: Junio C Hamano, git, Johannes Sixt, David Aguilar, Sitaram Chamarty, Jonathan Nieder 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 ^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH v2 2/3] t7800: fix tests when difftool uses --no-symlinks 2013-03-24 13:31 ` Matt McClure @ 2013-03-24 15:15 ` John Keeping 2013-03-25 7:41 ` Johannes Sixt 0 siblings, 1 reply; 55+ messages in thread From: John Keeping @ 2013-03-24 15:15 UTC (permalink / raw) To: Matt McClure Cc: Junio C Hamano, git, Johannes Sixt, David Aguilar, Sitaram Chamarty, Jonathan Nieder 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 ^ permalink raw reply related [flat|nested] 55+ messages in thread
* Re: [PATCH v2 2/3] t7800: fix tests when difftool uses --no-symlinks 2013-03-24 15:15 ` John Keeping @ 2013-03-25 7:41 ` Johannes Sixt 2013-03-25 10:42 ` John Keeping 2013-03-25 16:15 ` [PATCH v2 2/3] t7800: fix tests when difftool uses --no-symlinks Junio C Hamano 0 siblings, 2 replies; 55+ messages in thread From: Johannes Sixt @ 2013-03-25 7:41 UTC (permalink / raw) To: John Keeping Cc: Matt McClure, Junio C Hamano, git, David Aguilar, Sitaram Chamarty, Jonathan Nieder 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 ^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH v2 2/3] t7800: fix tests when difftool uses --no-symlinks 2013-03-25 7:41 ` Johannes Sixt @ 2013-03-25 10:42 ` John Keeping 2013-03-25 21:44 ` [PATCH v2] difftool: don't overwrite modified files John Keeping 2013-03-25 16:15 ` [PATCH v2 2/3] t7800: fix tests when difftool uses --no-symlinks Junio C Hamano 1 sibling, 1 reply; 55+ messages in thread From: John Keeping @ 2013-03-25 10:42 UTC (permalink / raw) To: Johannes Sixt Cc: Matt McClure, Junio C Hamano, git, David Aguilar, Sitaram Chamarty, Jonathan Nieder 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); ^ permalink raw reply [flat|nested] 55+ messages in thread
* [PATCH v2] difftool: don't overwrite modified files 2013-03-25 10:42 ` John Keeping @ 2013-03-25 21:44 ` John Keeping 2013-03-26 8:38 ` Johannes Sixt 2013-03-26 20:52 ` Matt McClure 0 siblings, 2 replies; 55+ messages in thread From: John Keeping @ 2013-03-25 21:44 UTC (permalink / raw) To: Johannes Sixt Cc: Matt McClure, Junio C Hamano, git, David Aguilar, Sitaram Chamarty, Jonathan Nieder 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 ^ permalink raw reply related [flat|nested] 55+ messages in thread
* Re: [PATCH v2] difftool: don't overwrite modified files 2013-03-25 21:44 ` [PATCH v2] difftool: don't overwrite modified files John Keeping @ 2013-03-26 8:38 ` Johannes Sixt 2013-03-26 8:47 ` Johannes Sixt 2013-03-26 9:31 ` John Keeping 2013-03-26 20:52 ` Matt McClure 1 sibling, 2 replies; 55+ messages in thread From: Johannes Sixt @ 2013-03-26 8:38 UTC (permalink / raw) To: John Keeping Cc: Matt McClure, Junio C Hamano, git, David Aguilar, Sitaram Chamarty, Jonathan Nieder 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 ^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH v2] difftool: don't overwrite modified files 2013-03-26 8:38 ` Johannes Sixt @ 2013-03-26 8:47 ` Johannes Sixt 2013-03-26 9:31 ` John Keeping 1 sibling, 0 replies; 55+ messages in thread From: Johannes Sixt @ 2013-03-26 8:47 UTC (permalink / raw) To: John Keeping Cc: Matt McClure, Junio C Hamano, git, David Aguilar, Sitaram Chamarty, Jonathan Nieder Forgot to mention: The patch passes t7800 on Windows. Thanks, -- Hannes ^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH v2] difftool: don't overwrite modified files 2013-03-26 8:38 ` Johannes Sixt 2013-03-26 8:47 ` Johannes Sixt @ 2013-03-26 9:31 ` John Keeping 2013-03-26 9:53 ` Johannes Sixt 1 sibling, 1 reply; 55+ messages in thread From: John Keeping @ 2013-03-26 9:31 UTC (permalink / raw) To: Johannes Sixt Cc: Matt McClure, Junio C Hamano, git, David Aguilar, Sitaram Chamarty, Jonathan Nieder 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)/.." ^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH v2] difftool: don't overwrite modified files 2013-03-26 9:31 ` John Keeping @ 2013-03-26 9:53 ` Johannes Sixt 2013-03-26 19:34 ` John Keeping 0 siblings, 1 reply; 55+ messages in thread From: Johannes Sixt @ 2013-03-26 9:53 UTC (permalink / raw) To: John Keeping Cc: Matt McClure, Junio C Hamano, git, David Aguilar, Sitaram Chamarty, Jonathan Nieder 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 ^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH v2] difftool: don't overwrite modified files 2013-03-26 9:53 ` Johannes Sixt @ 2013-03-26 19:34 ` John Keeping 0 siblings, 0 replies; 55+ messages in thread From: John Keeping @ 2013-03-26 19:34 UTC (permalink / raw) To: Johannes Sixt Cc: Matt McClure, Junio C Hamano, git, David Aguilar, Sitaram Chamarty, Jonathan Nieder 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. ^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH v2] difftool: don't overwrite modified files 2013-03-25 21:44 ` [PATCH v2] difftool: don't overwrite modified files John Keeping 2013-03-26 8:38 ` Johannes Sixt @ 2013-03-26 20:52 ` Matt McClure 2013-03-26 21:01 ` John Keeping 1 sibling, 1 reply; 55+ messages in thread From: Matt McClure @ 2013-03-26 20:52 UTC (permalink / raw) To: John Keeping Cc: Johannes Sixt, Junio C Hamano, git, David Aguilar, Sitaram Chamarty, Jonathan Nieder 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 ^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH v2] difftool: don't overwrite modified files 2013-03-26 20:52 ` Matt McClure @ 2013-03-26 21:01 ` John Keeping 0 siblings, 0 replies; 55+ messages in thread From: John Keeping @ 2013-03-26 21:01 UTC (permalink / raw) To: Matt McClure Cc: Johannes Sixt, Junio C Hamano, git, David Aguilar, Sitaram Chamarty, Jonathan Nieder 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. ^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH v2 2/3] t7800: fix tests when difftool uses --no-symlinks 2013-03-25 7:41 ` Johannes Sixt 2013-03-25 10:42 ` John Keeping @ 2013-03-25 16:15 ` Junio C Hamano 1 sibling, 0 replies; 55+ messages in thread From: Junio C Hamano @ 2013-03-25 16:15 UTC (permalink / raw) To: Johannes Sixt Cc: John Keeping, Matt McClure, git, David Aguilar, Sitaram Chamarty, Jonathan Nieder 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. ;-). ^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH v2 2/3] t7800: fix tests when difftool uses --no-symlinks 2013-03-24 12:36 ` John Keeping 2013-03-24 13:31 ` Matt McClure @ 2013-03-24 21:29 ` David Aguilar 2013-03-25 10:57 ` John Keeping 2013-03-25 14:54 ` Junio C Hamano 1 sibling, 2 replies; 55+ messages in thread From: David Aguilar @ 2013-03-24 21:29 UTC (permalink / raw) To: John Keeping Cc: Junio C Hamano, git, Johannes Sixt, Sitaram Chamarty, Jonathan Nieder 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 ^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH v2 2/3] t7800: fix tests when difftool uses --no-symlinks 2013-03-24 21:29 ` David Aguilar @ 2013-03-25 10:57 ` John Keeping 2013-03-25 14:54 ` Junio C Hamano 1 sibling, 0 replies; 55+ messages in thread From: John Keeping @ 2013-03-25 10:57 UTC (permalink / raw) To: David Aguilar Cc: Junio C Hamano, git, Johannes Sixt, Sitaram Chamarty, Jonathan Nieder 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 ^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH v2 2/3] t7800: fix tests when difftool uses --no-symlinks 2013-03-24 21:29 ` David Aguilar 2013-03-25 10:57 ` John Keeping @ 2013-03-25 14:54 ` Junio C Hamano 1 sibling, 0 replies; 55+ messages in thread From: Junio C Hamano @ 2013-03-25 14:54 UTC (permalink / raw) To: David Aguilar Cc: John Keeping, git, Johannes Sixt, Sitaram Chamarty, Jonathan Nieder 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). ^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH v2 2/3] t7800: fix tests when difftool uses --no-symlinks 2013-03-24 5:19 ` Junio C Hamano 2013-03-24 12:36 ` John Keeping @ 2013-03-24 13:24 ` Matt McClure 1 sibling, 0 replies; 55+ messages in thread From: Matt McClure @ 2013-03-24 13:24 UTC (permalink / raw) To: Junio C Hamano Cc: John Keeping, git, Johannes Sixt, David Aguilar, Sitaram Chamarty, Jonathan Nieder 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 ^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH v2 2/3] t7800: fix tests when difftool uses --no-symlinks 2013-03-23 13:31 ` [PATCH v2 2/3] t7800: fix tests when difftool uses --no-symlinks John Keeping 2013-03-24 5:19 ` Junio C Hamano @ 2013-03-24 6:20 ` Eric Sunshine 1 sibling, 0 replies; 55+ messages in thread From: Eric Sunshine @ 2013-03-24 6:20 UTC (permalink / raw) To: John Keeping Cc: Git List, Johannes Sixt, David Aguilar, Sitaram Chamarty, Junio C Hamano, Jonathan Nieder 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> ^ permalink raw reply [flat|nested] 55+ messages in thread
* [PATCH v2 3/3] t7800: run --dir-diff tests with and without symlinks 2013-03-23 13:31 ` [PATCH v2 0/3] difftool --dir-diff test improvements John Keeping 2013-03-23 13:31 ` [PATCH v2 1/3] t7800: don't hide grep output John Keeping 2013-03-23 13:31 ` [PATCH v2 2/3] t7800: fix tests when difftool uses --no-symlinks John Keeping @ 2013-03-23 13:31 ` John Keeping 2013-03-25 7:26 ` Johannes Sixt 2 siblings, 1 reply; 55+ messages in thread From: John Keeping @ 2013-03-23 13:31 UTC (permalink / raw) To: git Cc: Johannes Sixt, David Aguilar, Sitaram Chamarty, Junio C Hamano, Jonathan Nieder, 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 ^ permalink raw reply related [flat|nested] 55+ messages in thread
* Re: [PATCH v2 3/3] t7800: run --dir-diff tests with and without symlinks 2013-03-23 13:31 ` [PATCH v2 3/3] t7800: run --dir-diff tests with and without symlinks John Keeping @ 2013-03-25 7:26 ` Johannes Sixt 2013-03-25 10:35 ` John Keeping 0 siblings, 1 reply; 55+ messages in thread From: Johannes Sixt @ 2013-03-25 7:26 UTC (permalink / raw) To: John Keeping Cc: git, David Aguilar, Sitaram Chamarty, Junio C Hamano, Jonathan Nieder 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 ^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH v2 3/3] t7800: run --dir-diff tests with and without symlinks 2013-03-25 7:26 ` Johannes Sixt @ 2013-03-25 10:35 ` John Keeping 2013-03-25 10:59 ` Johannes Sixt 0 siblings, 1 reply; 55+ messages in thread From: John Keeping @ 2013-03-25 10:35 UTC (permalink / raw) To: Johannes Sixt Cc: git, David Aguilar, Sitaram Chamarty, Junio C Hamano, Jonathan Nieder 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. ^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH v2 3/3] t7800: run --dir-diff tests with and without symlinks 2013-03-25 10:35 ` John Keeping @ 2013-03-25 10:59 ` Johannes Sixt 2013-03-25 11:02 ` John Keeping 2013-03-25 21:50 ` Junio C Hamano 0 siblings, 2 replies; 55+ messages in thread From: Johannes Sixt @ 2013-03-25 10:59 UTC (permalink / raw) To: John Keeping Cc: git, David Aguilar, Sitaram Chamarty, Junio C Hamano, Jonathan Nieder 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 ^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH v2 3/3] t7800: run --dir-diff tests with and without symlinks 2013-03-25 10:59 ` Johannes Sixt @ 2013-03-25 11:02 ` John Keeping 2013-03-25 21:50 ` Junio C Hamano 1 sibling, 0 replies; 55+ messages in thread From: John Keeping @ 2013-03-25 11:02 UTC (permalink / raw) To: Johannes Sixt Cc: git, David Aguilar, Sitaram Chamarty, Junio C Hamano, Jonathan Nieder 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. ^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH v2 3/3] t7800: run --dir-diff tests with and without symlinks 2013-03-25 10:59 ` Johannes Sixt 2013-03-25 11:02 ` John Keeping @ 2013-03-25 21:50 ` Junio C Hamano 2013-03-26 9:22 ` John Keeping 1 sibling, 1 reply; 55+ messages in thread From: Junio C Hamano @ 2013-03-25 21:50 UTC (permalink / raw) To: Johannes Sixt Cc: John Keeping, git, David Aguilar, Sitaram Chamarty, Jonathan Nieder 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. ^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH v2 3/3] t7800: run --dir-diff tests with and without symlinks 2013-03-25 21:50 ` Junio C Hamano @ 2013-03-26 9:22 ` John Keeping 0 siblings, 0 replies; 55+ messages in thread From: John Keeping @ 2013-03-26 9:22 UTC (permalink / raw) To: Junio C Hamano Cc: Johannes Sixt, git, David Aguilar, Sitaram Chamarty, Jonathan Nieder 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. ^ permalink raw reply [flat|nested] 55+ messages in thread
end of thread, other threads:[~2013-03-26 21:02 UTC | newest] Thread overview: 55+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2013-02-21 4:03 [PATCH v3 1/4] difftool: silence uninitialized variable warning David Aguilar 2013-02-21 4:03 ` [PATCH 2/4] t7800: update copyright notice David Aguilar 2013-02-21 4:03 ` [PATCH v3 3/4] t7800: modernize tests David Aguilar 2013-02-21 4:03 ` [PATCH v3 4/4] t7800: "defaults" is no longer a builtin tool name David Aguilar 2013-02-21 4:55 ` Junio C Hamano 2013-02-21 5:00 ` Junio C Hamano 2013-02-21 23:31 ` David Aguilar 2013-03-20 9:48 ` [PATCH v3 3/4] t7800: modernize tests Johannes Sixt 2013-03-20 22:59 ` David Aguilar 2013-03-21 7:41 ` Johannes Sixt 2013-03-22 7:13 ` Johannes Sixt 2013-03-22 10:00 ` John Keeping 2013-03-22 11:14 ` Johannes Sixt 2013-03-22 11:53 ` John Keeping 2013-03-22 19:36 ` [PATCH 0/3] Improve difftool --dir-diff tests John Keeping 2013-03-22 19:36 ` [PATCH 1/3] t7800: don't hide grep output John Keeping 2013-03-22 22:32 ` Johannes Sixt 2013-03-22 22:45 ` Junio C Hamano 2013-03-22 19:36 ` [PATCH 2/3] t7800: fix tests when difftool uses --no-symlinks John Keeping 2013-03-22 22:27 ` Johannes Sixt 2013-03-22 22:53 ` Junio C Hamano 2013-03-22 23:05 ` John Keeping 2013-03-23 3:24 ` David Aguilar 2013-03-22 19:36 ` [PATCH 3/3] t7800: run --dir-diff tests with and without symlinks John Keeping 2013-03-22 21:05 ` [PATCH 3/3 v2] " John Keeping 2013-03-23 13:31 ` [PATCH v2 0/3] difftool --dir-diff test improvements John Keeping 2013-03-23 13:31 ` [PATCH v2 1/3] t7800: don't hide grep output John Keeping 2013-03-23 13:31 ` [PATCH v2 2/3] t7800: fix tests when difftool uses --no-symlinks John Keeping 2013-03-24 5:19 ` Junio C Hamano 2013-03-24 12:36 ` John Keeping 2013-03-24 13:31 ` Matt McClure 2013-03-24 15:15 ` John Keeping 2013-03-25 7:41 ` Johannes Sixt 2013-03-25 10:42 ` John Keeping 2013-03-25 21:44 ` [PATCH v2] difftool: don't overwrite modified files John Keeping 2013-03-26 8:38 ` Johannes Sixt 2013-03-26 8:47 ` Johannes Sixt 2013-03-26 9:31 ` John Keeping 2013-03-26 9:53 ` Johannes Sixt 2013-03-26 19:34 ` John Keeping 2013-03-26 20:52 ` Matt McClure 2013-03-26 21:01 ` John Keeping 2013-03-25 16:15 ` [PATCH v2 2/3] t7800: fix tests when difftool uses --no-symlinks Junio C Hamano 2013-03-24 21:29 ` David Aguilar 2013-03-25 10:57 ` John Keeping 2013-03-25 14:54 ` Junio C Hamano 2013-03-24 13:24 ` Matt McClure 2013-03-24 6:20 ` Eric Sunshine 2013-03-23 13:31 ` [PATCH v2 3/3] t7800: run --dir-diff tests with and without symlinks John Keeping 2013-03-25 7:26 ` Johannes Sixt 2013-03-25 10:35 ` John Keeping 2013-03-25 10:59 ` Johannes Sixt 2013-03-25 11:02 ` John Keeping 2013-03-25 21:50 ` Junio C Hamano 2013-03-26 9:22 ` John Keeping
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).