{"thread":{"id":"32954","subject":"[PATCH v3 1/4] difftool: silence uninitialized variable warning","startedAt":"2013-02-21T04:03:45Z","lastAt":"2013-03-26T21:01:42Z","messageCount":55,"participants":["David Aguilar","Junio C Hamano","Johannes Sixt","John Keeping","Eric Sunshine","Matt McClure"],"isPatch":true,"patchVersion":3,"patchTotal":4},"messages":[{"id":"209949","messageId":"1361419428-22410-1-git-send-email-davvid@gmail.com","threadId":"32954","inReplyTo":null,"subject":"[PATCH v3 1/4] difftool: silence uninitialized variable warning","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2013-02-21T04:03:45Z","receivedAt":"2013-02-21T04:03:45Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"Git::config() returns `undef` when given keys that do not exist.\nCheck that the $guitool value is defined to prevent a noisy\n\"Use of uninitialized variable $guitool in length\" warning.\n\nSigned-off-by: David Aguilar \n---\nUnchanged since v1.\n\n git-difftool.perl | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/git-difftool.perl b/git-difftool.perl\nindex 0a90de4..12231fb 100755\n--- a/git-difftool.perl\n+++ b/git-difftool.perl\n@@ -336,7 +336,7 @@ sub main\n \t}\n \tif ($opts{gui}) {\n \t\tmy $guitool = Git::config('diff.guitool');\n-\t\tif (length($guitool) > 0) {\n+\t\tif (defined($guitool) && length($guitool) > 0) {\n \t\t\t$ENV{GIT_DIFF_TOOL} = $guitool;\n \t\t}\n \t}\n-- \n1.8.2.rc0.20.gf548dd7\n"},{"id":"209950","messageId":"1361419428-22410-2-git-send-email-davvid@gmail.com","threadId":"32954","inReplyTo":"1361419428-22410-1-git-send-email-davvid@gmail.com","subject":"[PATCH 2/4] t7800: update copyright notice","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2013-02-21T04:03:46Z","receivedAt":"2013-02-21T04:03:46Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"Signed-off-by: David Aguilar \n---\nUnchanged since v2.\n\n t/t7800-difftool.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh\nindex eb1d3f8..5b5939b 100755\n--- a/t/t7800-difftool.sh\n+++ b/t/t7800-difftool.sh\n@@ -1,6 +1,6 @@\n #!/bin/sh\n #\n-# Copyright (c) 2009, 2010 David Aguilar\n+# Copyright (c) 2009, 2010, 2012, 2013 David Aguilar\n #\n \n test_description='git-difftool\n-- \n1.8.2.rc0.20.gf548dd7\n"},{"id":"209951","messageId":"1361419428-22410-3-git-send-email-davvid@gmail.com","threadId":"32954","inReplyTo":"1361419428-22410-2-git-send-email-davvid@gmail.com","subject":"[PATCH v3 3/4] t7800: modernize tests","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2013-02-21T04:03:47Z","receivedAt":"2013-02-21T04:03:47Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"Eliminate a lot of redundant work by using test_config().\nCatch more return codes by more use of temporary files\nand test_cmp.\n\nThe original tests relied upon restore_test_defaults()\nfrom the previous test to provide the next test with a sane\nenvironment. Make the tests do their own setup so that they\nare not dependent on the success of the previous test.\nThe end result is shorter tests and better test isolation.\n\nSigned-off-by: David Aguilar \n---\nv3 includes Junio's review notes to avoid cat with stdin_contains\nand to use DQ around $LOCAL.\n\nAnother difference from v2 is that it tweaks the function\ndeclarations to keep a SP between the name and parens to better\nfollow the standard git style.\n\n t/t7800-difftool.sh | 366 ++++++++++++++++++++++++----------------------------\n 1 file changed, 168 insertions(+), 198 deletions(-)\n\ndiff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh\nindex 5b5939b..fb00273 100755\n--- a/t/t7800-difftool.sh\n+++ b/t/t7800-difftool.sh\n@@ -10,43 +10,25 @@ Testing basic diff tool invocation\n \n . ./test-lib.sh\n \n-remove_config_vars()\n+difftool_test_setup ()\n {\n-\t# Unset all config variables used by git-difftool\n-\tgit config --unset diff.tool\n-\tgit config --unset diff.guitool\n-\tgit config --unset difftool.test-tool.cmd\n-\tgit config --unset difftool.prompt\n-\tgit config --unset merge.tool\n-\tgit config --unset mergetool.test-tool.cmd\n-\tgit config --unset mergetool.prompt\n-\treturn 0\n+\ttest_config diff.tool test-tool &&\n+\ttest_config difftool.test-tool.cmd 'cat \"$LOCAL\"' &&\n+\ttest_config difftool.bogus-tool.cmd false\n }\n \n-restore_test_defaults()\n-{\n-\t# Restores the test defaults used by several tests\n-\tremove_config_vars\n-\tunset GIT_DIFF_TOOL\n-\tunset GIT_DIFFTOOL_PROMPT\n-\tunset GIT_DIFFTOOL_NO_PROMPT\n-\tgit config diff.tool test-tool &&\n-\tgit config difftool.test-tool.cmd 'cat $LOCAL'\n-\tgit config difftool.bogus-tool.cmd false\n-}\n-\n-prompt_given()\n+prompt_given ()\n {\n \tprompt=\"$1\"\n \ttest \"$prompt\" = \"Launch 'test-tool' [Y/n]: branch\"\n }\n \n-stdin_contains()\n+stdin_contains ()\n {\n \tgrep >/dev/null \"$1\"\n }\n \n-stdin_doesnot_contain()\n+stdin_doesnot_contain ()\n {\n \t! stdin_contains \"$1\"\n }\n@@ -65,249 +47,237 @@ test_expect_success PERL 'setup' '\n \n # Configure a custom difftool..cmd and use it\n test_expect_success PERL 'custom commands' '\n-\trestore_test_defaults &&\n-\tgit config difftool.test-tool.cmd \"cat \\$REMOTE\" &&\n+\tdifftool_test_setup &&\n+\ttest_config difftool.test-tool.cmd \"cat \\\"\\$REMOTE\\\"\" &&\n+\techo master >expect &&\n+\tgit difftool --no-prompt branch >actual &&\n+\ttest_cmp expect actual &&\n \n-\tdiff=$(git difftool --no-prompt branch) &&\n-\ttest \"$diff\" = \"master\" &&\n-\n-\trestore_test_defaults &&\n-\tdiff=$(git difftool --no-prompt branch) &&\n-\ttest \"$diff\" = \"branch\"\n+\ttest_config difftool.test-tool.cmd \"cat \\\"\\$LOCAL\\\"\" &&\n+\techo branch >expect &&\n+\tgit difftool --no-prompt branch >actual &&\n+\ttest_cmp expect actual\n '\n \n-# Ensures that a custom difftool..cmd overrides built-ins\n-test_expect_success PERL 'custom commands override built-ins' '\n-\trestore_test_defaults &&\n-\tgit config difftool.defaults.cmd \"cat \\$REMOTE\" &&\n-\n-\tdiff=$(git difftool --tool defaults --no-prompt branch) &&\n-\ttest \"$diff\" = \"master\" &&\n-\n-\tgit config --unset difftool.defaults.cmd\n+test_expect_success PERL 'custom tool commands override built-ins' '\n+\ttest_config difftool.defaults.cmd \"cat \\\"\\$REMOTE\\\"\" &&\n+\techo master >expect &&\n+\tgit difftool --tool defaults --no-prompt branch >actual &&\n+\ttest_cmp expect actual\n '\n \n-# Ensures that git-difftool ignores bogus --tool values\n test_expect_success PERL 'difftool ignores bad --tool values' '\n-\tdiff=$(git difftool --no-prompt --tool=bad-tool branch)\n-\ttest \"$?\" = 1 &&\n-\ttest \"$diff\" = \"\"\n+\t: >expect &&\n+\ttest_expect_code 1 \\\n+\t\tgit difftool --no-prompt --tool=bad-tool branch >actual &&\n+\ttest_cmp expect actual\n '\n \n test_expect_success PERL 'difftool forwards arguments to diff' '\n+\tdifftool_test_setup &&\n \t>for-diff &&\n \tgit add for-diff &&\n \techo changes>for-diff &&\n \tgit add for-diff &&\n-\tdiff=$(git difftool --cached --no-prompt -- for-diff) &&\n-\ttest \"$diff\" = \"\" &&\n+\t: >expect &&\n+\tgit difftool --cached --no-prompt -- for-diff >actual &&\n+\ttest_cmp expect actual &&\n \tgit reset -- for-diff &&\n \trm for-diff\n '\n \n test_expect_success PERL 'difftool honors --gui' '\n-\tgit config merge.tool bogus-tool &&\n-\tgit config diff.tool bogus-tool &&\n-\tgit config diff.guitool test-tool &&\n-\n-\tdiff=$(git difftool --no-prompt --gui branch) &&\n-\ttest \"$diff\" = \"branch\" &&\n+\tdifftool_test_setup &&\n+\ttest_config merge.tool bogus-tool &&\n+\ttest_config diff.tool bogus-tool &&\n+\ttest_config diff.guitool test-tool &&\n \n-\trestore_test_defaults\n+\techo branch >expect &&\n+\tgit difftool --no-prompt --gui branch >actual &&\n+\ttest_cmp expect actual\n '\n \n test_expect_success PERL 'difftool --gui last setting wins' '\n-\tgit config diff.guitool bogus-tool &&\n-\tgit difftool --no-prompt --gui --no-gui &&\n+\tdifftool_test_setup &&\n+\t: >expect &&\n+\tgit difftool --no-prompt --gui --no-gui >actual &&\n+\ttest_cmp expect actual &&\n \n-\tgit config merge.tool bogus-tool &&\n-\tgit config diff.tool bogus-tool &&\n-\tgit config diff.guitool test-tool &&\n-\tdiff=$(git difftool --no-prompt --no-gui --gui branch) &&\n-\ttest \"$diff\" = \"branch\" &&\n-\n-\trestore_test_defaults\n+\ttest_config merge.tool bogus-tool &&\n+\ttest_config diff.tool bogus-tool &&\n+\ttest_config diff.guitool test-tool &&\n+\techo branch >expect &&\n+\tgit difftool --no-prompt --no-gui --gui branch >actual &&\n+\ttest_cmp expect actual\n '\n \n test_expect_success PERL 'difftool --gui works without configured diff.guitool' '\n-\tgit config diff.tool test-tool &&\n-\n-\tdiff=$(git difftool --no-prompt --gui branch) &&\n-\ttest \"$diff\" = \"branch\" &&\n-\n-\trestore_test_defaults\n+\tdifftool_test_setup &&\n+\techo branch >expect &&\n+\tgit difftool --no-prompt --gui branch >actual &&\n+\ttest_cmp expect actual\n '\n \n # Specify the diff tool using $GIT_DIFF_TOOL\n test_expect_success PERL 'GIT_DIFF_TOOL variable' '\n-\ttest_might_fail git config --unset diff.tool &&\n-\tGIT_DIFF_TOOL=test-tool &&\n-\texport GIT_DIFF_TOOL &&\n-\n-\tdiff=$(git difftool --no-prompt branch) &&\n-\ttest \"$diff\" = \"branch\" &&\n-\n-\trestore_test_defaults\n+\tdifftool_test_setup &&\n+\tgit config --unset diff.tool &&\n+\techo branch >expect &&\n+\tGIT_DIFF_TOOL=test-tool git difftool --no-prompt branch >actual &&\n+\ttest_cmp expect actual\n '\n \n # Test the $GIT_*_TOOL variables and ensure\n # that $GIT_DIFF_TOOL always wins unless --tool is specified\n test_expect_success PERL 'GIT_DIFF_TOOL overrides' '\n-\tgit config diff.tool bogus-tool &&\n-\tgit config merge.tool bogus-tool &&\n-\n-\tGIT_DIFF_TOOL=test-tool &&\n-\texport GIT_DIFF_TOOL &&\n-\n-\tdiff=$(git difftool --no-prompt branch) &&\n-\ttest \"$diff\" = \"branch\" &&\n+\tdifftool_test_setup &&\n+\ttest_config diff.tool bogus-tool &&\n+\ttest_config merge.tool bogus-tool &&\n \n-\tGIT_DIFF_TOOL=bogus-tool &&\n-\texport GIT_DIFF_TOOL &&\n+\techo branch >expect &&\n+\tGIT_DIFF_TOOL=test-tool git difftool --no-prompt branch >actual &&\n+\ttest_cmp expect actual &&\n \n-\tdiff=$(git difftool --no-prompt --tool=test-tool branch) &&\n-\ttest \"$diff\" = \"branch\" &&\n-\n-\trestore_test_defaults\n+\ttest_config diff.tool bogus-tool &&\n+\ttest_config merge.tool bogus-tool &&\n+\tGIT_DIFF_TOOL=bogus-tool \\\n+\t\tgit difftool --no-prompt --tool=test-tool branch >actual &&\n+\ttest_cmp expect actual\n '\n \n # Test that we don't have to pass --no-prompt to difftool\n # when $GIT_DIFFTOOL_NO_PROMPT is true\n test_expect_success PERL 'GIT_DIFFTOOL_NO_PROMPT variable' '\n-\tGIT_DIFFTOOL_NO_PROMPT=true &&\n-\texport GIT_DIFFTOOL_NO_PROMPT &&\n-\n-\tdiff=$(git difftool branch) &&\n-\ttest \"$diff\" = \"branch\" &&\n-\n-\trestore_test_defaults\n+\tdifftool_test_setup &&\n+\techo branch >expect &&\n+\tGIT_DIFFTOOL_NO_PROMPT=true git difftool branch >actual &&\n+\ttest_cmp expect actual\n '\n \n # git-difftool supports the difftool.prompt variable.\n # Test that GIT_DIFFTOOL_PROMPT can override difftool.prompt = false\n test_expect_success PERL 'GIT_DIFFTOOL_PROMPT variable' '\n-\tgit config difftool.prompt false &&\n-\tGIT_DIFFTOOL_PROMPT=true &&\n-\texport GIT_DIFFTOOL_PROMPT &&\n-\n-\tprompt=$(echo | git difftool branch | tail -1) &&\n-\tprompt_given \"$prompt\" &&\n-\n-\trestore_test_defaults\n+\tdifftool_test_setup &&\n+\ttest_config difftool.prompt false &&\n+\techo >input &&\n+\tGIT_DIFFTOOL_PROMPT=true git difftool branch output &&\n+\tprompt=$(tail -1 expect &&\n+\tgit difftool branch >actual &&\n+\ttest_cmp expect actual\n '\n \n # Test that we don't have to pass --no-prompt when mergetool.prompt is false\n test_expect_success PERL 'difftool merge.prompt = false' '\n+\tdifftool_test_setup &&\n \ttest_might_fail git config --unset difftool.prompt &&\n-\tgit config mergetool.prompt false &&\n-\n-\tdiff=$(git difftool branch) &&\n-\ttest \"$diff\" = \"branch\" &&\n-\n-\trestore_test_defaults\n+\ttest_config mergetool.prompt false &&\n+\techo branch >expect &&\n+\tgit difftool branch >actual &&\n+\ttest_cmp expect actual\n '\n \n # Test that the -y flag can override difftool.prompt = true\n test_expect_success PERL 'difftool.prompt can overridden with -y' '\n-\tgit config difftool.prompt true &&\n-\n-\tdiff=$(git difftool -y branch) &&\n-\ttest \"$diff\" = \"branch\" &&\n-\n-\trestore_test_defaults\n+\tdifftool_test_setup &&\n+\ttest_config difftool.prompt true &&\n+\techo branch >expect &&\n+\tgit difftool -y branch >actual &&\n+\ttest_cmp expect actual\n '\n \n # Test that the --prompt flag can override difftool.prompt = false\n test_expect_success PERL 'difftool.prompt can overridden with --prompt' '\n-\tgit config difftool.prompt false &&\n-\n-\tprompt=$(echo | git difftool --prompt branch | tail -1) &&\n-\tprompt_given \"$prompt\" &&\n-\n-\trestore_test_defaults\n+\tdifftool_test_setup &&\n+\ttest_config difftool.prompt false &&\n+\techo >input &&\n+\tgit difftool --prompt branch output &&\n+\tprompt=$(tail -1 expect &&\n+\tgit difftool --prompt --no-prompt branch >actual &&\n+\ttest_cmp expect actual &&\n+\techo >input &&\n+\tgit difftool --no-prompt --prompt branch output &&\n+\tprompt=$(tail -1 expect &&\n+\tgit difftool --no-prompt branch >actual &&\n+\ttest_cmp expect actual &&\n \n \t# set merge.tool to something bogus, diff.tool to test-tool\n-\tgit config merge.tool bogus-tool &&\n-\tgit config diff.tool test-tool &&\n-\n-\tdiff=$(git difftool --no-prompt branch) &&\n-\ttest \"$diff\" = \"branch\" &&\n-\n-\trestore_test_defaults\n+\ttest_config merge.tool bogus-tool &&\n+\ttest_config diff.tool test-tool &&\n+\tgit difftool --no-prompt branch >actual &&\n+\ttest_cmp expect actual\n '\n \n test_expect_success PERL 'difftool..path' '\n-\tgit config difftool.tkdiff.path echo &&\n-\tdiff=$(git difftool --tool=tkdiff --no-prompt branch) &&\n-\tgit config --unset difftool.tkdiff.path &&\n-\tlines=$(echo \"$diff\" | grep file | wc -l) &&\n-\ttest \"$lines\" -eq 1 &&\n-\n-\trestore_test_defaults\n+\ttest_config difftool.tkdiff.path echo &&\n+\tgit difftool --tool=tkdiff --no-prompt branch >output &&\n+\tlines=$(grep file output | wc -l) &&\n+\ttest \"$lines\" -eq 1\n '\n \n test_expect_success PERL 'difftool --extcmd=cat' '\n-\tdiff=$(git difftool --no-prompt --extcmd=cat branch) &&\n-\ttest \"$diff\" = branch\"$LF\"master\n+\techo branch >expect &&\n+\techo master >>expect &&\n+\tgit difftool --no-prompt --extcmd=cat branch >actual &&\n+\ttest_cmp expect actual\n '\n \n test_expect_success PERL 'difftool --extcmd cat' '\n-\tdiff=$(git difftool --no-prompt --extcmd cat branch) &&\n-\ttest \"$diff\" = branch\"$LF\"master\n+\techo branch >expect &&\n+\techo master >>expect &&\n+\tgit difftool --no-prompt --extcmd=cat branch >actual &&\n+\ttest_cmp expect actual\n '\n \n test_expect_success PERL 'difftool -x cat' '\n-\tdiff=$(git difftool --no-prompt -x cat branch) &&\n-\ttest \"$diff\" = branch\"$LF\"master\n+\techo branch >expect &&\n+\techo master >>expect &&\n+\tgit difftool --no-prompt -x cat branch >actual &&\n+\ttest_cmp expect actual\n '\n \n test_expect_success PERL 'difftool --extcmd echo arg1' '\n-\tdiff=$(git difftool --no-prompt --extcmd sh\\ -c\\ \\\"echo\\ \\$1\\\" branch) &&\n-\ttest \"$diff\" = file\n+\techo file >expect &&\n+\tgit difftool --no-prompt \\\n+\t\t--extcmd sh\\ -c\\ \\\"echo\\ \\$1\\\" branch >actual &&\n+\ttest_cmp expect actual\n '\n \n test_expect_success PERL 'difftool --extcmd cat arg1' '\n-\tdiff=$(git difftool --no-prompt --extcmd sh\\ -c\\ \\\"cat\\ \\$1\\\" branch) &&\n-\ttest \"$diff\" = master\n+\techo master >expect &&\n+\tgit difftool --no-prompt \\\n+\t\t--extcmd sh\\ -c\\ \\\"cat\\ \\$1\\\" branch >actual &&\n+\ttest_cmp expect actual\n '\n \n test_expect_success PERL 'difftool --extcmd cat arg2' '\n-\tdiff=$(git difftool --no-prompt --extcmd sh\\ -c\\ \\\"cat\\ \\$2\\\" branch) &&\n-\ttest \"$diff\" = branch\n+\techo branch >expect &&\n+\tgit difftool --no-prompt \\\n+\t\t--extcmd sh\\ -c\\ \\\"cat\\ \\$2\\\" branch >actual &&\n+\ttest_cmp expect actual\n '\n \n # Create a second file on master and a different version on branch\n@@ -324,26 +294,26 @@ test_expect_success PERL 'setup with 2 files different' '\n '\n \n test_expect_success PERL 'say no to the first file' '\n-\tdiff=$( (echo n; echo) | git difftool -x cat branch ) &&\n-\n-\techo \"$diff\" | stdin_contains m2 &&\n-\techo \"$diff\" | stdin_contains br2 &&\n-\techo \"$diff\" | stdin_doesnot_contain master &&\n-\techo \"$diff\" | stdin_doesnot_contain branch\n+\t(echo n && echo) >input &&\n+\tgit difftool -x cat branch output &&\n+\tstdin_contains m2 input &&\n+\tgit difftool -x cat branch output &&\n+\tstdin_contains master output &&\n+\tstdin_contains tool output &&\n+\tstdin_contains sub output &&\n+\tstdin_contains sub output &&\n+\tstdin_contains sub output &&\n+\t\tstdin_contains sub \n---\nRebased against PATCH v3 3/4.\n\n t/t7800-difftool.sh | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh\nindex fb00273..21fbba9 100755\n--- a/t/t7800-difftool.sh\n+++ b/t/t7800-difftool.sh\n@@ -60,9 +60,9 @@ test_expect_success PERL 'custom commands' '\n '\n \n test_expect_success PERL 'custom tool commands override built-ins' '\n-\ttest_config difftool.defaults.cmd \"cat \\\"\\$REMOTE\\\"\" &&\n+\ttest_config difftool.vimdiff \"cat \\\"\\$REMOTE\\\"\" &&\n \techo master >expect &&\n-\tgit difftool --tool defaults --no-prompt branch >actual &&\n+\tgit difftool --tool vimdiff --no-prompt branch >actual &&\n \ttest_cmp expect actual\n '\n \n-- \n1.8.2.rc0.20.gf548dd7\n"},{"id":"209953","messageId":"7v7gm2xo48.fsf@alter.siamese.dyndns.org","threadId":"32954","inReplyTo":"1361419428-22410-4-git-send-email-davvid@gmail.com","subject":"Re: [PATCH v3 4/4] t7800: \"defaults\" is no longer a builtin tool name","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-02-21T04:55:03Z","receivedAt":"2013-02-21T04:55:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thanks; will replace and queue.\n"},{"id":"209954","messageId":"7vzjyyw9bi.fsf@alter.siamese.dyndns.org","threadId":"32954","inReplyTo":"1361419428-22410-4-git-send-email-davvid@gmail.com","subject":"Re: [PATCH v3 4/4] t7800: \"defaults\" is no longer a builtin tool name","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-02-21T05:00:01Z","receivedAt":"2013-02-21T05:00:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"David Aguilar writes:\n\n> diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh\n> index fb00273..21fbba9 100755\n> --- a/t/t7800-difftool.sh\n> +++ b/t/t7800-difftool.sh\n> @@ -60,9 +60,9 @@ test_expect_success PERL 'custom commands' '\n> '\n> \n> test_expect_success PERL 'custom tool commands override built-ins' '\n> -\ttest_config difftool.defaults.cmd \"cat \\\"\\$REMOTE\\\"\" &&\n> +\ttest_config difftool.vimdiff \"cat \\\"\\$REMOTE\\\"\" &&\n> \techo master >expect &&\n> -\tgit difftool --tool defaults --no-prompt branch >actual &&\n> +\tgit difftool --tool vimdiff --no-prompt branch >actual &&\n> \ttest_cmp expect actual\n> '\n\nEek.\n\n$ sh t7800-difftool.sh -i\nok 1 - setup\nok 2 - custom commands\nnot ok 3 - custom tool commands override built-ins\n#\n# test_config difftool.vimdiff \"cat \\\"\\$REMOTE\\\"\" &&\n# echo master >expect &&\n# git difftool --tool vimdiff --no-prompt branch >actual &&\n# test_cmp expect actual\n#\n\nRunning the same test with \"-v\" seems to get stuck with this\nforever:\n\nexpecting success:\n test_config difftool.vimdiff \"cat \\\"\\$REMOTE\\\"\" &&\n echo master >expect &&\n git difftool --tool vimdiff --no-prompt branch >actual &&\n test_cmp expect actual\n\nVim: Warning: Output is not to a terminal\nVim: Warning: Input is not from a terminal\n"},{"id":"210006","messageId":"CAJDDKr6QCQQYHvXQesVKn0hz_kvjJbEXQ9fsqojhGOCLRNGhrQ@mail.gmail.com","threadId":"32954","inReplyTo":"7vzjyyw9bi.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v3 4/4] t7800: \"defaults\" is no longer a builtin tool name","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2013-02-21T23:31:01Z","receivedAt":"2013-02-21T23:31:01Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"On Wed, Feb 20, 2013 at 9:00 PM, Junio C Hamano wrote:\n> David Aguilar writes:\n>\n>> diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh\n>> index fb00273..21fbba9 100755\n>> --- a/t/t7800-difftool.sh\n>> +++ b/t/t7800-difftool.sh\n>> @@ -60,9 +60,9 @@ test_expect_success PERL 'custom commands' '\n>> '\n>>\n>> test_expect_success PERL 'custom tool commands override built-ins' '\n>> - test_config difftool.defaults.cmd \"cat \\\"\\$REMOTE\\\"\" &&\n>> + test_config difftool.vimdiff \"cat \\\"\\$REMOTE\\\"\" &&\n>> echo master >expect &&\n>> - git difftool --tool defaults --no-prompt branch >actual &&\n>> + git difftool --tool vimdiff --no-prompt branch >actual &&\n>> test_cmp expect actual\n>> '\n>\n> Eek.\n>\n> $ sh t7800-difftool.sh -i\n> ok 1 - setup\n> ok 2 - custom commands\n> not ok 3 - custom tool commands override built-ins\n> #\n> # test_config difftool.vimdiff \"cat \\\"\\$REMOTE\\\"\" &&\n> # echo master >expect &&\n> # git difftool --tool vimdiff --no-prompt branch >actual &&\n> # test_cmp expect actual\n> #\n>\n> Running the same test with \"-v\" seems to get stuck with this\n> forever:\n>\n> expecting success:\n> test_config difftool.vimdiff \"cat \\\"\\$REMOTE\\\"\" &&\n> echo master >expect &&\n> git difftool --tool vimdiff --no-prompt branch >actual &&\n> test_cmp expect actual\n>\n> Vim: Warning: Output is not to a terminal\n> Vim: Warning: Input is not from a terminal\n>\n\nOh boy. I botched the rebase. I have a replacement. I thought I\nre-ran the tests! I'll resend it.\n-- \nDavid\n"},{"id":"211730","messageId":"514985FB.8080806@viscovery.net","threadId":"32954","inReplyTo":"1361419428-22410-3-git-send-email-davvid@gmail.com","subject":"Re: [PATCH v3 3/4] t7800: modernize tests","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2013-03-20T09:48:43Z","receivedAt":"2013-03-20T09:48:43Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 2/21/2013 5:03, schrieb David Aguilar:\n> test_expect_success PERL 'difftool -d' '\n> -\tdiff=$(git difftool -d --extcmd ls branch) &&\n> -\techo \"$diff\" | stdin_contains sub &&\n> -\techo \"$diff\" | stdin_contains file\n> +\tgit difftool -d --extcmd ls branch >output &&\n> +\tstdin_contains sub +\tstdin_contains file '\n\nThis test is broken on Windows. There is this code in git-difftool.perl\n\n\tfor my $file (@worktree) {\n...\n\t\t\tcopy(\"$b/$file\", \"$workdir/$file\") or\n\t\t\texit_cleanup($tmpdir, 1);\n...\n\t}\n\n@worktree is populated with all files in the worktree. At this point,\n\"output\" is among them. Then follows an attempt to copy a file over\n\"$workdir/$file\". I guess that is some link+remove magic going on behind\nthe scenes. At any rate, this fails on Windows with\n\"D:/Src/mingw-git/t/trash directory.t7800-difftool/../../git-difftool line\n408: Bad file number\", because files that are open cannot be written from\noutside (the file is open due to the redirection in the test snippet).\n\nWhat is going on here? Why can this ever succeed even on Unix?\n\nSame for some later tests.\n\nBTW, while debugging this, I found the use of the helper function\nstdin_contains() highly unhelpful; it just resolves to a 'grep' that on\ntop of all hides stdout. Please don't do that. Just use unadorned grep\nlike we do everywhere else.\n\n-- Hannes\n"},{"id":"211831","messageId":"CAJDDKr4O-OU57j0f7F2E2_4EvtCTE1gFP5uLv+fvD6VcpPiifg@mail.gmail.com","threadId":"32954","inReplyTo":"514985FB.8080806@viscovery.net","subject":"Re: [PATCH v3 3/4] t7800: modernize tests","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2013-03-20T22:59:19Z","receivedAt":"2013-03-20T22:59:19Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"On Wed, Mar 20, 2013 at 2:48 AM, Johannes Sixt wrote:\n> Am 2/21/2013 5:03, schrieb David Aguilar:\n>> test_expect_success PERL 'difftool -d' '\n>> - diff=$(git difftool -d --extcmd ls branch) &&\n>> - echo \"$diff\" | stdin_contains sub &&\n>> - echo \"$diff\" | stdin_contains file\n>> + git difftool -d --extcmd ls branch >output &&\n>> + stdin_contains sub > + stdin_contains file > '\n>\n> This test is broken on Windows. There is this code in git-difftool.perl\n>\n> for my $file (@worktree) {\n> ...\n> copy(\"$b/$file\", \"$workdir/$file\") or\n> exit_cleanup($tmpdir, 1);\n> ...\n> }\n>\n> @worktree is populated with all files in the worktree. At this point,\n> \"output\" is among them. Then follows an attempt to copy a file over\n> \"$workdir/$file\". I guess that is some link+remove magic going on behind\n> the scenes. At any rate, this fails on Windows with\n> \"D:/Src/mingw-git/t/trash directory.t7800-difftool/../../git-difftool line\n> 408: Bad file number\", because files that are open cannot be written from\n> outside (the file is open due to the redirection in the test snippet).\n>\n> What is going on here? Why can this ever succeed even on Unix?\n\nThanks for the report. Yes, these do pass on Unix.\n\nHmm I wonder what's going on here?\n\nI started digging in and the @worktree_files (aka @worktree above)\nis populated from the output of \"git diff --raw ...\".\n\nSeeing the \"output\" filename in \"diff --raw\" implies that one of the\ntests added \"output\" to the index somehow. I do not see that\nhappening anywhere, though, so I do not know how it would end up in\nthe @worktree array if it is not reported by \"diff --raw\".\n\n\nMy current understanding of how it could possibly be open twice:\n\n1. via the >output redirect\n2. via the copy() perl code which is fed by @worktree\n\nSo I'm confused. Why would we get different results on Windows?\n\nI just re-ran these tests from \"next\" to check my sanity and they\npassed on both Linux and OS X.\n\n> Same for some later tests.\n\nDitto.\n\n> BTW, while debugging this, I found the use of the helper function\n> stdin_contains() highly unhelpful; it just resolves to a 'grep' that on\n> top of all hides stdout. Please don't do that. Just use unadorned grep\n> like we do everywhere else.\n\nI'm not too opposed to that.\nThe one small advantage to the helper is that you can tweak the redirect\nin one central place, so it's not all for naught.\n\nSitaram, you added this back in:\n\nba959de1 git-difftool: allow skipping file by typing 'n' at prompt\n\nDo you have any thoughts?\n\nIt seems like removing the stdout redirect could be helpful for debugging,\nand if we did that then there's really no point in having the helper\n(aside from the indirection which can sometimes help during debugging).\nI don't really feel too strongly either way, but it did bother you\nwhile debugging the test script, so unadorned grep seems like the way to go.\n\nI'll wait and see if anybody else has any Windows-specific clues that\nwe can use to narrow down this problem.\n\nIn lieu of an immediate fix, are there any test prerequisite we can\nuse to skip these tests on windows? One or both of NOT_CYGWIN,NOT_MINGW?\n-- \nDavid\n"},{"id":"211842","messageId":"514AB9A5.3050601@viscovery.net","threadId":"32954","inReplyTo":"CAJDDKr4O-OU57j0f7F2E2_4EvtCTE1gFP5uLv+fvD6VcpPiifg@mail.gmail.com","subject":"Re: [PATCH v3 3/4] t7800: modernize tests","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2013-03-21T07:41:25Z","receivedAt":"2013-03-21T07:41:25Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 3/20/2013 23:59, schrieb David Aguilar:\n> I started digging in and the @worktree_files (aka @worktree above)\n> is populated from the output of \"git diff --raw ...\".\n> \n> Seeing the \"output\" filename in \"diff --raw\" implies that one of the\n> tests added \"output\" to the index somehow. I do not see that\n> happening anywhere, though, so I do not know how it would end up in\n> the @worktree array if it is not reported by \"diff --raw\".\n> \n> \n> My current understanding of how it could possibly be open twice:\n> \n> 1. via the >output redirect\n> 2. via the copy() perl code which is fed by @worktree\n> \n> So I'm confused. Why would we get different results on Windows?\n\nI tracked down the difference between Windows and Linux, and it is...\n\n\tfor my $file (@worktree) {\n\t\tnext if $symlinks && -l \"$b/$file\";\n\n... this line in sub dir_diff. On Linux, we take the short-cut, but on\nWindows we proceed through the rest of the loop, which ultimately finds a\ndifference here:\n\n\t\tmy $diff = compare(\"$b/$file\", \"$workdir/$file\");\n\nand attempts to copy a file here:\n\n\t\t\tcopy(\"$b/$file\", \"$workdir/$file\") or\n\nwhere one of the files is the locked \"output\" file.\n\nI don't know how essential symlinks are for the operation of git-difftool\nand whether something can be done about it. The immediate fix is\napparently to protect the tests with SYMLINKS.\n\n-- Hannes\n"},{"id":"211913","messageId":"514C04AA.6030305@viscovery.net","threadId":"32954","inReplyTo":"514AB9A5.3050601@viscovery.net","subject":"Re: [PATCH v3 3/4] t7800: modernize tests","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2013-03-22T07:13:46Z","receivedAt":"2013-03-22T07:13:46Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 3/21/2013 8:41, schrieb Johannes Sixt:\n> Am 3/20/2013 23:59, schrieb David Aguilar:\n>> I started digging in and the @worktree_files (aka @worktree above)\n>> is populated from the output of \"git diff --raw ...\".\n>>\n>> Seeing the \"output\" filename in \"diff --raw\" implies that one of the\n>> tests added \"output\" to the index somehow. I do not see that\n>> happening anywhere, though, so I do not know how it would end up in\n>> the @worktree array if it is not reported by \"diff --raw\".\n>>\n>>\n>> My current understanding of how it could possibly be open twice:\n>>\n>> 1. via the >output redirect\n>> 2. via the copy() perl code which is fed by @worktree\n>>\n>> So I'm confused. Why would we get different results on Windows?\n> \n> I tracked down the difference between Windows and Linux, and it is...\n> \n> \tfor my $file (@worktree) {\n> \t\tnext if $symlinks && -l \"$b/$file\";\n> \n> ... this line in sub dir_diff. On Linux, we take the short-cut, but on\n> Windows we proceed through the rest of the loop,\n\nAnd that is likely by design. From the docs:\n\n--symlinks\n--no-symlinks\n\n git difftool's default behavior is create symlinks to the working\n tree when run in --dir-diff mode.\n\n Specifying `--no-symlinks` instructs 'git difftool' to create\n copies instead. `--no-symlinks` is the default on Windows.\n\nAnd indeed, we have this initialization:\n\n\tmy %opts = (\n\t\t...\n\t\tsymlinks => $^O ne 'cygwin' &&\n\t\t\t\t$^O ne 'MSWin32' && $^O ne 'msys',\n\t\t...\n\t);\n\nCan the --dir-diff tests case pass on Cygwin when neither --symlinks nor\n--no-symlinks is passed?\n\nPerhaps the right solution is this:\n\ndiff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh\nindex c6d6b1c..19238f6 100755\n--- a/t/t7800-difftool.sh\n+++ b/t/t7800-difftool.sh\n@@ -328,14 +328,16 @@ test_expect_success PERL 'setup change in subdirectory' '\n \tgit commit -m \"modified both\"\n '\n \n-test_expect_success PERL 'difftool -d' '\n-\tgit difftool -d --extcmd ls branch >output &&\n+# passing --symlinks helps Cygwin, which defaults to --no-symlinks\n+\n+test_expect_success PERL,SYMLINKS 'difftool -d' '\n+\tgit difftool -d --symlinks --extcmd ls branch >output &&\n \tstdin_contains sub output &&\n+test_expect_success PERL,SYMLINKS 'difftool --dir-diff' '\n+\tgit difftool --dir-diff --symlinks --extcmd ls branch >output &&\n \tstdin_contains sub output &&\n+test_expect_success PERL,SYMLINKS 'difftool --dir-diff ignores --prompt' '\n+\tgit difftool --dir-diff --symlinks --prompt --extcmd ls branch >output &&\n \tstdin_contains sub output &&\n+\t\tgit difftool --dir-diff --symlinks --extcmd ls branch >output &&\n \t\tstdin_contains sub Am 3/21/2013 8:41, schrieb Johannes Sixt:\n> > Am 3/20/2013 23:59, schrieb David Aguilar:\n> >> I started digging in and the @worktree_files (aka @worktree above)\n> >> is populated from the output of \"git diff --raw ...\".\n> >>\n> >> Seeing the \"output\" filename in \"diff --raw\" implies that one of the\n> >> tests added \"output\" to the index somehow. I do not see that\n> >> happening anywhere, though, so I do not know how it would end up in\n> >> the @worktree array if it is not reported by \"diff --raw\".\n> >>\n> >>\n> >> My current understanding of how it could possibly be open twice:\n> >>\n> >> 1. via the >output redirect\n> >> 2. via the copy() perl code which is fed by @worktree\n> >>\n> >> So I'm confused. Why would we get different results on Windows?\n> > \n> > I tracked down the difference between Windows and Linux, and it is...\n> > \n> > \tfor my $file (@worktree) {\n> > \t\tnext if $symlinks && -l \"$b/$file\";\n> > \n> > ... this line in sub dir_diff. On Linux, we take the short-cut, but on\n> > Windows we proceed through the rest of the loop,\n> \n> And that is likely by design. From the docs:\n> \n> --symlinks\n> --no-symlinks\n> \n> git difftool's default behavior is create symlinks to the working\n> tree when run in --dir-diff mode.\n> \n> Specifying `--no-symlinks` instructs 'git difftool' to create\n> copies instead. `--no-symlinks` is the default on Windows.\n> \n> And indeed, we have this initialization:\n> \n> \tmy %opts = (\n> \t\t...\n> \t\tsymlinks => $^O ne 'cygwin' &&\n> \t\t\t\t$^O ne 'MSWin32' && $^O ne 'msys',\n> \t\t...\n> \t);\n> \n> Can the --dir-diff tests case pass on Cygwin when neither --symlinks nor\n> --no-symlinks is passed?\n> \n> Perhaps the right solution is this:\n\nWe already have tests that explicitly pass '--symlinks'. I wonder if it\nwould be better to change \"output\" to \".git/output\", which should avoid\nthe problem by moving the output file out of the working tree.\n\n> diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh\n> index c6d6b1c..19238f6 100755\n> --- a/t/t7800-difftool.sh\n> +++ b/t/t7800-difftool.sh\n> @@ -328,14 +328,16 @@ test_expect_success PERL 'setup change in subdirectory' '\n> \tgit commit -m \"modified both\"\n> '\n> \n> -test_expect_success PERL 'difftool -d' '\n> -\tgit difftool -d --extcmd ls branch >output &&\n> +# passing --symlinks helps Cygwin, which defaults to --no-symlinks\n> +\n> +test_expect_success PERL,SYMLINKS 'difftool -d' '\n> +\tgit difftool -d --symlinks --extcmd ls branch >output &&\n> \tstdin_contains sub \tstdin_contains file '\n> \n> -test_expect_success PERL 'difftool --dir-diff' '\n> -\tgit difftool --dir-diff --extcmd ls branch >output &&\n> +test_expect_success PERL,SYMLINKS 'difftool --dir-diff' '\n> +\tgit difftool --dir-diff --symlinks --extcmd ls branch >output &&\n> \tstdin_contains sub \tstdin_contains file '\n> @@ -362,16 +364,16 @@ test_expect_success PERL,SYMLINKS 'difftool --dir-diff --symlink without unstage\n> \ttest_cmp actual expect\n> '\n> \n> -test_expect_success PERL 'difftool --dir-diff ignores --prompt' '\n> -\tgit difftool --dir-diff --prompt --extcmd ls branch >output &&\n> +test_expect_success PERL,SYMLINKS 'difftool --dir-diff ignores --prompt' '\n> +\tgit difftool --dir-diff --symlinks --prompt --extcmd ls branch >output &&\n> \tstdin_contains sub \tstdin_contains file '\n> \n> -test_expect_success PERL 'difftool --dir-diff from subdirectory' '\n> +test_expect_success PERL,SYMLINKS 'difftool --dir-diff from subdirectory' '\n> \t(\n> \t\tcd sub &&\n> -\t\tgit difftool --dir-diff --extcmd ls branch >output &&\n> +\t\tgit difftool --dir-diff --symlinks --extcmd ls branch >output &&\n> \t\tstdin_contains sub \t\tstdin_contains file \t)\n> \n> (Only tested on MinGW, which skips the tests.) I leave it to you\n> to write --no-symlinks tests.\n> \n> -- Hannes\n"},{"id":"211929","messageId":"514C3D13.5010301@viscovery.net","threadId":"32954","inReplyTo":"20130322100030.GH2283@serenity.lan","subject":"Re: [PATCH v3 3/4] t7800: modernize tests","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2013-03-22T11:14:27Z","receivedAt":"2013-03-22T11:14:27Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 3/22/2013 11:00, schrieb John Keeping:\n> On Fri, Mar 22, 2013 at 08:13:46AM +0100, Johannes Sixt wrote:\n>> Am 3/21/2013 8:41, schrieb Johannes Sixt:\n>>> Am 3/20/2013 23:59, schrieb David Aguilar:\n>>>> I started digging in and the @worktree_files (aka @worktree above)\n>>>> is populated from the output of \"git diff --raw ...\".\n>>>>\n>>>> Seeing the \"output\" filename in \"diff --raw\" implies that one of the\n>>>> tests added \"output\" to the index somehow. I do not see that\n>>>> happening anywhere, though, so I do not know how it would end up in\n>>>> the @worktree array if it is not reported by \"diff --raw\".\n>>>>\n>>>>\n>>>> My current understanding of how it could possibly be open twice:\n>>>>\n>>>> 1. via the >output redirect\n>>>> 2. via the copy() perl code which is fed by @worktree\n>>>>\n>>>> So I'm confused. Why would we get different results on Windows?\n>>>\n>>> I tracked down the difference between Windows and Linux, and it is...\n>>>\n>>> \tfor my $file (@worktree) {\n>>> \t\tnext if $symlinks && -l \"$b/$file\";\n>>>\n>>> ... this line in sub dir_diff. On Linux, we take the short-cut, but on\n>>> Windows we proceed through the rest of the loop,\n>>\n>> And that is likely by design. From the docs:\n>>\n>> --symlinks\n>> --no-symlinks\n>>\n>> git difftool's default behavior is create symlinks to the working\n>> tree when run in --dir-diff mode.\n>>\n>> Specifying `--no-symlinks` instructs 'git difftool' to create\n>> copies instead. `--no-symlinks` is the default on Windows.\n>>\n>> And indeed, we have this initialization:\n>>\n>> \tmy %opts = (\n>> \t\t...\n>> \t\tsymlinks => $^O ne 'cygwin' &&\n>> \t\t\t\t$^O ne 'MSWin32' && $^O ne 'msys',\n>> \t\t...\n>> \t);\n>>\n>> Can the --dir-diff tests case pass on Cygwin when neither --symlinks nor\n>> --no-symlinks is passed?\n>>\n>> Perhaps the right solution is this:\n> \n> We already have tests that explicitly pass '--symlinks'. I wonder if it\n> would be better to change \"output\" to \".git/output\", which should avoid\n> the problem by moving the output file out of the working tree.\n\nThe point of using --symlinks is not to test the effect of the option, but\nto test the same thing on Unix and on Cygwin, because the latter uses\n--no-symlinks by default. Therefore, I think that this sketch is the right\nthing to do.\n\nBut the real problem seems to be that \"output\" should not be among the\nfiles treated in the cited pieces of code (unless I'm wrong, of course; I\nknow next to nothing about git-difftool). It should not matter where the\nfile lives. Just add --no-symlinks to the difftool invocation of test\n\"difftool -d\" and watch it fail on Linux, too.\n\n-- Hannes\n"},{"id":"211930","messageId":"20130322115352.GI2283@serenity.lan","threadId":"32954","inReplyTo":"514C3D13.5010301@viscovery.net","subject":"Re: [PATCH v3 3/4] t7800: modernize tests","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2013-03-22T11:53:52Z","receivedAt":"2013-03-22T11:53:52Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"On Fri, Mar 22, 2013 at 12:14:27PM +0100, Johannes Sixt wrote:\n> Am 3/22/2013 11:00, schrieb John Keeping:\n> > On Fri, Mar 22, 2013 at 08:13:46AM +0100, Johannes Sixt wrote:\n> >> Am 3/21/2013 8:41, schrieb Johannes Sixt:\n> >>> Am 3/20/2013 23:59, schrieb David Aguilar:\n> >>>> I started digging in and the @worktree_files (aka @worktree above)\n> >>>> is populated from the output of \"git diff --raw ...\".\n> >>>>\n> >>>> Seeing the \"output\" filename in \"diff --raw\" implies that one of the\n> >>>> tests added \"output\" to the index somehow. I do not see that\n> >>>> happening anywhere, though, so I do not know how it would end up in\n> >>>> the @worktree array if it is not reported by \"diff --raw\".\n> >>>>\n> >>>>\n> >>>> My current understanding of how it could possibly be open twice:\n> >>>>\n> >>>> 1. via the >output redirect\n> >>>> 2. via the copy() perl code which is fed by @worktree\n> >>>>\n> >>>> So I'm confused. Why would we get different results on Windows?\n> >>>\n> >>> I tracked down the difference between Windows and Linux, and it is...\n> >>>\n> >>> \tfor my $file (@worktree) {\n> >>> \t\tnext if $symlinks && -l \"$b/$file\";\n> >>>\n> >>> ... this line in sub dir_diff. On Linux, we take the short-cut, but on\n> >>> Windows we proceed through the rest of the loop,\n> >>\n> >> And that is likely by design. From the docs:\n> >>\n> >> --symlinks\n> >> --no-symlinks\n> >>\n> >> git difftool's default behavior is create symlinks to the working\n> >> tree when run in --dir-diff mode.\n> >>\n> >> Specifying `--no-symlinks` instructs 'git difftool' to create\n> >> copies instead. `--no-symlinks` is the default on Windows.\n> >>\n> >> And indeed, we have this initialization:\n> >>\n> >> \tmy %opts = (\n> >> \t\t...\n> >> \t\tsymlinks => $^O ne 'cygwin' &&\n> >> \t\t\t\t$^O ne 'MSWin32' && $^O ne 'msys',\n> >> \t\t...\n> >> \t);\n> >>\n> >> Can the --dir-diff tests case pass on Cygwin when neither --symlinks nor\n> >> --no-symlinks is passed?\n> >>\n> >> Perhaps the right solution is this:\n> > \n> > We already have tests that explicitly pass '--symlinks'. I wonder if it\n> > would be better to change \"output\" to \".git/output\", which should avoid\n> > the problem by moving the output file out of the working tree.\n> \n> The point of using --symlinks is not to test the effect of the option, but\n> to test the same thing on Unix and on Cygwin, because the latter uses\n> --no-symlinks by default. Therefore, I think that this sketch is the right\n> thing to do.\n\nSo shouldn't we be running all of the tests with both --symlinks and\n--no-symlinks (except those which explicitly check that these options do\nthe right thing)?\n\n> But the real problem seems to be that \"output\" should not be among the\n> files treated in the cited pieces of code (unless I'm wrong, of course; I\n> know next to nothing about git-difftool). It should not matter where the\n> file lives. Just add --no-symlinks to the difftool invocation of test\n> \"difftool -d\" and watch it fail on Linux, too.\n\nI fired up strace and it looks like difftool sees it as a working tree\nfile (i.e. the working tree content matches the RHS of the diff) in the\noutput of \"git diff --raw --no-abbrev -z branch\" and copies it over to\nthe temporary directories. Then when difftool completes it copies back\nthe working tree files from there.\n\nWhen the file is copied to the temporary directory, the diff command\nhasn't yet run so \"output\" is empty. When it's copied back it is after\nthe \"ls\" has run and so we overwrite the output of the command with the\noriginal empty file.\n\nSo sticking the output file under .git does solve this issue since it\nthen is not treated as a working tree file.\n\nWhether the current behaviour of difftool is entirely sensible is a\ndifferent question. I think we should at the very least by refusing to\noverwrite working tree files that have been modified since they were\ncopied to the temporary directory\n\nIf I ever end up on a system using --no-symlinks I can see myself being\nbitten by this by editing the original working tree file while\ninspecting the diff in a separate window. I suspect this is just as\ncommon a workflow as people editing the file in their diff tool and\nwanting the changes copied back to the working tree.\n\n\nJohn\n"},{"id":"211974","messageId":"cover.1363980749.git.john@keeping.me.uk","threadId":"32954","inReplyTo":"20130322115352.GI2283@serenity.lan","subject":"[PATCH 0/3] Improve difftool --dir-diff tests","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2013-03-22T19:36:30Z","receivedAt":"2013-03-22T19:36:30Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"How about doing this?\n\nThe first patch is a cleanup as suggested by Johannes[1], the second\nfixes the test failure on Windows and the third makes the test behaviour\nmore explicit and would have helped to detect this issue earlier.\n\n [1/3] t7800: don't hide grep output\n [2/3] t7800: fix tests when difftool uses --no-symlinks\n [3/3] t7800: run --dir-diff tests with and without symlinks\n\n t/t7800-difftool.sh | 71 +++++++++++++++++++++++++++--------------------------\n 1 file changed, 36 insertions(+), 35 deletions(-)\n\n-- \n1.8.2.324.ga64ebd9\n"},{"id":"211975","messageId":"4cbe25e4b1fd7a8d45067106d24f7d83ca5c8e5b.1363980749.git.john@keeping.me.uk","threadId":"32954","inReplyTo":"cover.1363980749.git.john@keeping.me.uk","subject":"[PATCH 1/3] t7800: don't hide grep output","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2013-03-22T19:36:31Z","receivedAt":"2013-03-22T19:36:31Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"Remove the stdin_contains and stdin_doesnt_contain helper functions\nwhich add nothing but hide the output of grep, hurting debugging.\n\nSuggested-by: Johannes Sixt \nSigned-off-by: John Keeping \n---\n t/t7800-difftool.sh | 44 +++++++++++++++++---------------------------\n 1 file changed, 17 insertions(+), 27 deletions(-)\n\ndiff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh\nindex 3aab6e1..e694972 100755\n--- a/t/t7800-difftool.sh\n+++ b/t/t7800-difftool.sh\n@@ -23,16 +23,6 @@ prompt_given ()\n \ttest \"$prompt\" = \"Launch 'test-tool' [Y/n]: branch\"\n }\n \n-stdin_contains ()\n-{\n-\tgrep >/dev/null \"$1\"\n-}\n-\n-stdin_doesnot_contain ()\n-{\n-\t! stdin_contains \"$1\"\n-}\n-\n # Create a file on master and change it on branch\n test_expect_success PERL 'setup' '\n \techo master >file &&\n@@ -296,24 +286,24 @@ test_expect_success PERL 'setup with 2 files different' '\n test_expect_success PERL 'say no to the first file' '\n \t(echo n && echo) >input &&\n \tgit difftool -x cat branch output &&\n-\tstdin_contains m2 input &&\n \tgit difftool -x cat branch output &&\n-\tstdin_contains master output &&\n-\tstdin_contains tool output &&\n-\tstdin_contains sub output &&\n-\tstdin_contains sub output &&\n-\tstdin_contains sub output &&\n-\t\tstdin_contains sub \n---\n t/t7800-difftool.sh | 24 ++++++++++++------------\n 1 file changed, 12 insertions(+), 12 deletions(-)\n\ndiff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh\nindex e694972..1eed439 100755\n--- a/t/t7800-difftool.sh\n+++ b/t/t7800-difftool.sh\n@@ -319,29 +319,29 @@ test_expect_success PERL 'setup change in subdirectory' '\n '\n \n test_expect_success PERL 'difftool -d' '\n-\tgit difftool -d --extcmd ls branch >output &&\n-\tgrep sub output &&\n-\tgrep file output\n+\tgit difftool -d --extcmd ls branch >.git/output &&\n+\tgrep sub .git/output &&\n+\tgrep file .git/output\n '\n \n test_expect_success PERL 'difftool --dir-diff' '\n-\tgit difftool --dir-diff --extcmd ls branch >output &&\n-\tgrep sub output &&\n-\tgrep file output\n+\tgit difftool --dir-diff --extcmd ls branch >.git/output &&\n+\tgrep sub .git/output &&\n+\tgrep file .git/output\n '\n \n test_expect_success PERL 'difftool --dir-diff ignores --prompt' '\n-\tgit difftool --dir-diff --prompt --extcmd ls branch >output &&\n-\tgrep sub output &&\n-\tgrep file output\n+\tgit difftool --dir-diff --prompt --extcmd ls branch >.git/output &&\n+\tgrep sub .git/output &&\n+\tgrep file .git/output\n '\n \n test_expect_success PERL 'difftool --dir-diff from subdirectory' '\n \t(\n \t\tcd sub &&\n-\t\tgit difftool --dir-diff --extcmd ls branch >output &&\n-\t\tgrep sub output &&\n-\t\tgrep file output\n+\t\tgit difftool --dir-diff --extcmd ls branch >../.git/output &&\n+\t\tgrep sub ../.git/output &&\n+\t\tgrep file ../.git/output\n \t)\n '\n \n-- \n1.8.2.324.ga64ebd9\n"},{"id":"211977","messageId":"9227365bf9cdfde61c1fdd374756187d00c84480.1363980749.git.john@keeping.me.uk","threadId":"32954","inReplyTo":"cover.1363980749.git.john@keeping.me.uk","subject":"[PATCH 3/3] t7800: run --dir-diff tests with and without symlinks","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2013-03-22T19:36:33Z","receivedAt":"2013-03-22T19:36:33Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"Currently the difftool --dir-diff tests may or may not use symlinks\ndepending on the operating system on which they are run. In one case\nthis has caused a test failure to be noticed only on Windows when the\ntest also fails on Linux when difftool is invoked with --no-symlinks.\n\nRewrite these tests so that they do not depend on the environment but\nrun explicitly with both --symlinks and --no-symlinks, protecting the\n--symlinks version with a SYMLINKS prerequisite.\n\nSigned-off-by: John Keeping \n---\n t/t7800-difftool.sh | 19 +++++++++++++++----\n 1 file changed, 15 insertions(+), 4 deletions(-)\n\ndiff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh\nindex 1eed439..4a70508 100755\n--- a/t/t7800-difftool.sh\n+++ b/t/t7800-difftool.sh\n@@ -318,25 +318,36 @@ test_expect_success PERL 'setup change in subdirectory' '\n \tgit commit -m \"modified both\"\n '\n \n-test_expect_success PERL 'difftool -d' '\n+run_dir_diff_test () {\n+\ttest_expect_success PERL \"$1 --no-symlinks\" \"\n+\t\tsymlinks=--no-symlinks\n+\t\t$2\n+\t\"\n+\ttest_expect_success PERL,SYMLINKS \"$1 --symlinks\" \"\n+\t\tsymlinks=--symlinks\n+\t\t$2\n+\t\"\n+}\n+\n+run_dir_diff_test 'difftool -d' '\n \tgit difftool -d --extcmd ls branch >.git/output &&\n \tgrep sub .git/output &&\n \tgrep file .git/output\n '\n \n-test_expect_success PERL 'difftool --dir-diff' '\n+run_dir_diff_test 'difftool --dir-diff' '\n \tgit difftool --dir-diff --extcmd ls branch >.git/output &&\n \tgrep sub .git/output &&\n \tgrep file .git/output\n '\n \n-test_expect_success PERL 'difftool --dir-diff ignores --prompt' '\n+run_dir_diff_test 'difftool --dir-diff ignores --prompt' '\n \tgit difftool --dir-diff --prompt --extcmd ls branch >.git/output &&\n \tgrep sub .git/output &&\n \tgrep file .git/output\n '\n \n-test_expect_success PERL 'difftool --dir-diff from subdirectory' '\n+run_dir_diff_test 'difftool --dir-diff from subdirectory' '\n \t(\n \t\tcd sub &&\n \t\tgit difftool --dir-diff --extcmd ls branch >../.git/output &&\n-- \n1.8.2.324.ga64ebd9\n"},{"id":"211981","messageId":"20130322210530.GJ2283@serenity.lan","threadId":"32954","inReplyTo":"9227365bf9cdfde61c1fdd374756187d00c84480.1363980749.git.john@keeping.me.uk","subject":"[PATCH 3/3 v2] t7800: run --dir-diff tests with and without symlinks","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2013-03-22T21:05:30Z","receivedAt":"2013-03-22T21:05:30Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"Currently the difftool --dir-diff tests may or may not use symlinks\ndepending on the operating system on which they are run. In one case\nthis has caused a test failure to be noticed only on Windows when the\ntest also fails on Linux when difftool is invoked with --no-symlinks.\n\nRewrite these tests so that they do not depend on the environment but\nrun explicitly with both --symlinks and --no-symlinks, protecting the\n--symlinks version with a SYMLINKS prerequisite.\n\nSigned-off-by: John Keeping \n---\nThe previous version of this was missing half the intended change :-(\nSorry for the noise.\n\n t/t7800-difftool.sh | 27 +++++++++++++++++++--------\n 1 file changed, 19 insertions(+), 8 deletions(-)\n\ndiff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh\nindex 1eed439..bba8a9d 100755\n--- a/t/t7800-difftool.sh\n+++ b/t/t7800-difftool.sh\n@@ -318,28 +318,39 @@ test_expect_success PERL 'setup change in subdirectory' '\n \tgit commit -m \"modified both\"\n '\n \n-test_expect_success PERL 'difftool -d' '\n-\tgit difftool -d --extcmd ls branch >.git/output &&\n+run_dir_diff_test () {\n+\ttest_expect_success PERL \"$1 --no-symlinks\" \"\n+\t\tsymlinks=--no-symlinks\n+\t\t$2\n+\t\"\n+\ttest_expect_success PERL,SYMLINKS \"$1 --symlinks\" \"\n+\t\tsymlinks=--symlinks\n+\t\t$2\n+\t\"\n+}\n+\n+run_dir_diff_test 'difftool -d' '\n+\tgit difftool -d $symlinks --extcmd ls branch >.git/output &&\n \tgrep sub .git/output &&\n \tgrep file .git/output\n '\n \n-test_expect_success PERL 'difftool --dir-diff' '\n-\tgit difftool --dir-diff --extcmd ls branch >.git/output &&\n+run_dir_diff_test 'difftool --dir-diff' '\n+\tgit difftool --dir-diff $symlinks --extcmd ls branch >.git/output &&\n \tgrep sub .git/output &&\n \tgrep file .git/output\n '\n \n-test_expect_success PERL 'difftool --dir-diff ignores --prompt' '\n-\tgit difftool --dir-diff --prompt --extcmd ls branch >.git/output &&\n+run_dir_diff_test 'difftool --dir-diff ignores --prompt' '\n+\tgit difftool --dir-diff $symlinks --prompt --extcmd ls branch >.git/output &&\n \tgrep sub .git/output &&\n \tgrep file .git/output\n '\n \n-test_expect_success PERL 'difftool --dir-diff from subdirectory' '\n+run_dir_diff_test 'difftool --dir-diff from subdirectory' '\n \t(\n \t\tcd sub &&\n-\t\tgit difftool --dir-diff --extcmd ls branch >../.git/output &&\n+\t\tgit difftool --dir-diff $symlinks --extcmd ls branch >../.git/output &&\n \t\tgrep sub ../.git/output &&\n \t\tgrep file ../.git/output\n \t)\n-- \n1.8.2.324.ga64ebd9\n"},{"id":"211995","messageId":"514CDACD.6030306@kdbg.org","threadId":"32954","inReplyTo":"5fc134f6c4a88232c78240539084e9d35db3a6cb.1363980749.git.john@keeping.me.uk","subject":"Re: [PATCH 2/3] t7800: fix tests when difftool uses --no-symlinks","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2013-03-22T22:27:25Z","receivedAt":"2013-03-22T22:27:25Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 22.03.2013 20:36, schrieb John Keeping:\n> When 'git difftool --dir-diff' is using --no-symlinks (either explicitly\n> or implicitly because it's running on Windows), any working tree files\n> that have been copied to the temporary directory are copied back after\n> the difftool completes. This includes untracked files in the working\n> tree.\n> \n> During the tests, this means that the following sequence occurs:\n> \n> 1) the shell opens \"output\" to redirect the difftool output\n> 2) difftool copies the empty \"output\" to the temporary directory\n\nBut this should not happen, should it?\n\n> 3) difftool runs \"ls\" which writes to \"output\"\n> 4) difftool copies the empty \"output\" file back over the output of the\n> command\n> 5) the output files doesn't contain the expected output, causing the\n> test to fail\n> \n> Avoid this by writing the output into .git/ which will not be copied or\n> overwritten.\n\nIsn't this just painting over the bug that \"output\" is incorrectly copied?\n\n> In the longer term, difftool probably needs to learn to warn the user\n> instead of overwrite any changes that have been made to the working tree\n> file.\n\nSure, but this is an independent issue.\n\n> diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh\n> index e694972..1eed439 100755\n> --- a/t/t7800-difftool.sh\n> +++ b/t/t7800-difftool.sh\n> @@ -319,29 +319,29 @@ test_expect_success PERL 'setup change in subdirectory' '\n> '\n> \n> test_expect_success PERL 'difftool -d' '\n> -\tgit difftool -d --extcmd ls branch >output &&\n> -\tgrep sub output &&\n> -\tgrep file output\n> +\tgit difftool -d --extcmd ls branch >.git/output &&\n> +\tgrep sub .git/output &&\n> +\tgrep file .git/output\n> '\n> ...\n"},{"id":"211996","messageId":"514CDBE8.80609@kdbg.org","threadId":"32954","inReplyTo":"4cbe25e4b1fd7a8d45067106d24f7d83ca5c8e5b.1363980749.git.john@keeping.me.uk","subject":"Re: [PATCH 1/3] t7800: don't hide grep output","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2013-03-22T22:32:08Z","receivedAt":"2013-03-22T22:32:08Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 22.03.2013 20:36, schrieb John Keeping:\n> Remove the stdin_contains and stdin_doesnt_contain helper functions\n> which add nothing but hide the output of grep, hurting debugging.\n\nThanks. Patch looks good.\n\n-- Hannes\n"},{"id":"211999","messageId":"7vd2ur5a1c.fsf@alter.siamese.dyndns.org","threadId":"32954","inReplyTo":"4cbe25e4b1fd7a8d45067106d24f7d83ca5c8e5b.1363980749.git.john@keeping.me.uk","subject":"Re: [PATCH 1/3] t7800: don't hide grep output","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-03-22T22:45:03Z","receivedAt":"2013-03-22T22:45:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Looks good from a cursory read. Thanks.\n"},{"id":"212000","messageId":"7v8v5f59n1.fsf@alter.siamese.dyndns.org","threadId":"32954","inReplyTo":"5fc134f6c4a88232c78240539084e9d35db3a6cb.1363980749.git.john@keeping.me.uk","subject":"Re: [PATCH 2/3] t7800: fix tests when difftool uses --no-symlinks","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-03-22T22:53:38Z","receivedAt":"2013-03-22T22:53:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"John Keeping writes:\n\n> When 'git difftool --dir-diff' is using --no-symlinks (either explicitly\n> or implicitly because it's running on Windows), any working tree files\n> that have been copied to the temporary directory are copied back after\n> the difftool completes. This includes untracked files in the working\n> tree.\n\nHmph. Why do we populate the temporary directory with a copy of an\nuntracked path in the first place? I thought the point of dir-diff\nwas to materialize only the relevant paths to two temporaries and\ncompare these temporaries with a tool that knows how to compare two\ndirectories?\n\nEven if you had path F in HEAD that you are no longer tracking in\nthe working tree, a normal\n\n\t$ git diff HEAD\n\nwould report the path F to have been deleted, so I would imagine\nthat the preimage side of the temporary directory should get a copy\nof HEAD:F at path F, while the postimage side of the temporary\ndirectory should not even have anything at path F, when dir-diff\nruns, no?\n\nIsn't that the real reason why the test fails? The path 'output' is\nnot being tracked at any revision or in the index that is involved\nin the test, is it?\n\n> During the tests, this means that the following sequence occurs:\n>\n> 1) the shell opens \"output\" to redirect the difftool output\n> 2) difftool copies the empty \"output\" to the temporary directory\n> 3) difftool runs \"ls\" which writes to \"output\"\n> 4) difftool copies the empty \"output\" file back over the output of the\n> command\n> 5) the output files doesn't contain the expected output, causing the\n> test to fail\n>\n> Avoid this by writing the output into .git/ which will not be copied or\n> overwritten.\n\nIt is a good idea to move these test output and expect test vectore\nfiles to a different place to make it easier to distinguish them\nfrom test input (e.g. \"sub\", \"file\", etc.) in general, but the\ndescription of the original problem sounds like it is just working\naround a bug to me. What am I missing?\n\n> In the longer term, difftool probably needs to learn to warn the user\n> instead of overwrite any changes that have been made to the working tree\n> file.\n>\n> Signed-off-by: John Keeping \n> ---\n> t/t7800-difftool.sh | 24 ++++++++++++------------\n> 1 file changed, 12 insertions(+), 12 deletions(-)\n>\n> diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh\n> index e694972..1eed439 100755\n> --- a/t/t7800-difftool.sh\n> +++ b/t/t7800-difftool.sh\n> @@ -319,29 +319,29 @@ test_expect_success PERL 'setup change in subdirectory' '\n> '\n> \n> test_expect_success PERL 'difftool -d' '\n> -\tgit difftool -d --extcmd ls branch >output &&\n> -\tgrep sub output &&\n> -\tgrep file output\n> +\tgit difftool -d --extcmd ls branch >.git/output &&\n> +\tgrep sub .git/output &&\n> +\tgrep file .git/output\n> '\n> \n> test_expect_success PERL 'difftool --dir-diff' '\n> -\tgit difftool --dir-diff --extcmd ls branch >output &&\n> -\tgrep sub output &&\n> -\tgrep file output\n> +\tgit difftool --dir-diff --extcmd ls branch >.git/output &&\n> +\tgrep sub .git/output &&\n> +\tgrep file .git/output\n> '\n> \n> test_expect_success PERL 'difftool --dir-diff ignores --prompt' '\n> -\tgit difftool --dir-diff --prompt --extcmd ls branch >output &&\n> -\tgrep sub output &&\n> -\tgrep file output\n> +\tgit difftool --dir-diff --prompt --extcmd ls branch >.git/output &&\n> +\tgrep sub .git/output &&\n> +\tgrep file .git/output\n> '\n> \n> test_expect_success PERL 'difftool --dir-diff from subdirectory' '\n> \t(\n> \t\tcd sub &&\n> -\t\tgit difftool --dir-diff --extcmd ls branch >output &&\n> -\t\tgrep sub output &&\n> -\t\tgrep file output\n> +\t\tgit difftool --dir-diff --extcmd ls branch >../.git/output &&\n> +\t\tgrep sub ../.git/output &&\n> +\t\tgrep file ../.git/output\n> \t)\n> '\n"},{"id":"212002","messageId":"20130322230516.GK2283@serenity.lan","threadId":"32954","inReplyTo":"7v8v5f59n1.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 2/3] t7800: fix tests when difftool uses --no-symlinks","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2013-03-22T23:05:16Z","receivedAt":"2013-03-22T23:05:16Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"On Fri, Mar 22, 2013 at 03:53:38PM -0700, Junio C Hamano wrote:\n> John Keeping writes:\n> \n> > When 'git difftool --dir-diff' is using --no-symlinks (either explicitly\n> > or implicitly because it's running on Windows), any working tree files\n> > that have been copied to the temporary directory are copied back after\n> > the difftool completes. This includes untracked files in the working\n> > tree.\n> \n> Hmph. Why do we populate the temporary directory with a copy of an\n> untracked path in the first place? I thought the point of dir-diff\n> was to materialize only the relevant paths to two temporaries and\n> compare these temporaries with a tool that knows how to compare two\n> directories?\n> \n> Even if you had path F in HEAD that you are no longer tracking in\n> the working tree, a normal\n> \n> \t$ git diff HEAD\n> \n> would report the path F to have been deleted, so I would imagine\n> that the preimage side of the temporary directory should get a copy\n> of HEAD:F at path F, while the postimage side of the temporary\n> directory should not even have anything at path F, when dir-diff\n> runs, no?\n> \n> Isn't that the real reason why the test fails? The path 'output' is\n> not being tracked at any revision or in the index that is involved\n> in the test, is it?\n\nActually it is, which is what I missed earlier.\n\nA couple of tests before this 'setup change in subdirectory' does 'git\nadd .' which is far more general than it needs. Perhaps this is a\nbetter change:\n\ndiff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh\nindex bba8a9d..561c993 100755\n--- a/t/t7800-difftool.sh\n+++ b/t/t7800-difftool.sh\n@@ -314,7 +314,7 @@ test_expect_success PERL 'setup change in subdirectory' '\n \tgit commit -m \"added sub/sub\" &&\n \techo test >>file &&\n \techo test >>sub/sub &&\n-\tgit add . &&\n+\tgit add file sub/sub &&\n \tgit commit -m \"modified both\"\n '\n \n\n> > During the tests, this means that the following sequence occurs:\n> >\n> > 1) the shell opens \"output\" to redirect the difftool output\n> > 2) difftool copies the empty \"output\" to the temporary directory\n> > 3) difftool runs \"ls\" which writes to \"output\"\n> > 4) difftool copies the empty \"output\" file back over the output of the\n> > command\n> > 5) the output files doesn't contain the expected output, causing the\n> > test to fail\n> >\n> > Avoid this by writing the output into .git/ which will not be copied or\n> > overwritten.\n> \n> It is a good idea to move these test output and expect test vectore\n> files to a different place to make it easier to distinguish them\n> from test input (e.g. \"sub\", \"file\", etc.) in general, but the\n> description of the original problem sounds like it is just working\n> around a bug to me. What am I missing?\n\nI think there is a bug, as described in the paragraph below, and this\ntest should be made independent of that. In light of the above I think\nwe can drop this patch and do this with that change instead.\n\n> > In the longer term, difftool probably needs to learn to warn the user\n> > instead of overwrite any changes that have been made to the working tree\n> > file.\n> >\n> > Signed-off-by: John Keeping \n> > ---\n> > t/t7800-difftool.sh | 24 ++++++++++++------------\n> > 1 file changed, 12 insertions(+), 12 deletions(-)\n> >\n> > diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh\n> > index e694972..1eed439 100755\n> > --- a/t/t7800-difftool.sh\n> > +++ b/t/t7800-difftool.sh\n> > @@ -319,29 +319,29 @@ test_expect_success PERL 'setup change in subdirectory' '\n> > '\n> > \n> > test_expect_success PERL 'difftool -d' '\n> > -\tgit difftool -d --extcmd ls branch >output &&\n> > -\tgrep sub output &&\n> > -\tgrep file output\n> > +\tgit difftool -d --extcmd ls branch >.git/output &&\n> > +\tgrep sub .git/output &&\n> > +\tgrep file .git/output\n> > '\n> > \n> > test_expect_success PERL 'difftool --dir-diff' '\n> > -\tgit difftool --dir-diff --extcmd ls branch >output &&\n> > -\tgrep sub output &&\n> > -\tgrep file output\n> > +\tgit difftool --dir-diff --extcmd ls branch >.git/output &&\n> > +\tgrep sub .git/output &&\n> > +\tgrep file .git/output\n> > '\n> > \n> > test_expect_success PERL 'difftool --dir-diff ignores --prompt' '\n> > -\tgit difftool --dir-diff --prompt --extcmd ls branch >output &&\n> > -\tgrep sub output &&\n> > -\tgrep file output\n> > +\tgit difftool --dir-diff --prompt --extcmd ls branch >.git/output &&\n> > +\tgrep sub .git/output &&\n> > +\tgrep file .git/output\n> > '\n> > \n> > test_expect_success PERL 'difftool --dir-diff from subdirectory' '\n> > \t(\n> > \t\tcd sub &&\n> > -\t\tgit difftool --dir-diff --extcmd ls branch >output &&\n> > -\t\tgrep sub output &&\n> > -\t\tgrep file output\n> > +\t\tgit difftool --dir-diff --extcmd ls branch >../.git/output &&\n> > +\t\tgrep sub ../.git/output &&\n> > +\t\tgrep file ../.git/output\n> > \t)\n> > '\n> --\n> To unsubscribe from this list: send the line \"unsubscribe git\" in\n> the body of a message to majordomo@vger.kernel.org\n> More majordomo info at http://vger.kernel.org/majordomo-info.html\n"},{"id":"212018","messageId":"CAJDDKr4658zFCZ8JpXUZUrS4URTb7zX0xGR4AeODDLOVjkzDGA@mail.gmail.com","threadId":"32954","inReplyTo":"20130322230516.GK2283@serenity.lan","subject":"Re: [PATCH 2/3] t7800: fix tests when difftool uses --no-symlinks","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2013-03-23T03:24:08Z","receivedAt":"2013-03-23T03:24:08Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"On Fri, Mar 22, 2013 at 4:05 PM, John Keeping wrote:\n> On Fri, Mar 22, 2013 at 03:53:38PM -0700, Junio C Hamano wrote:\n>> John Keeping writes:\n>>\n>> > When 'git difftool --dir-diff' is using --no-symlinks (either explicitly\n>> > or implicitly because it's running on Windows), any working tree files\n>> > that have been copied to the temporary directory are copied back after\n>> > the difftool completes. This includes untracked files in the working\n>> > tree.\n>>\n>> Hmph. Why do we populate the temporary directory with a copy of an\n>> untracked path in the first place? I thought the point of dir-diff\n>> was to materialize only the relevant paths to two temporaries and\n>> compare these temporaries with a tool that knows how to compare two\n>> directories?\n>>\n>> Even if you had path F in HEAD that you are no longer tracking in\n>> the working tree, a normal\n>>\n>> $ git diff HEAD\n>>\n>> would report the path F to have been deleted, so I would imagine\n>> that the preimage side of the temporary directory should get a copy\n>> of HEAD:F at path F, while the postimage side of the temporary\n>> directory should not even have anything at path F, when dir-diff\n>> runs, no?\n>>\n>> Isn't that the real reason why the test fails? The path 'output' is\n>> not being tracked at any revision or in the index that is involved\n>> in the test, is it?\n>\n> Actually it is, which is what I missed earlier.\n>\n> A couple of tests before this 'setup change in subdirectory' does 'git\n> add .' which is far more general than it needs. Perhaps this is a\n> better change:\n>\n> diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh\n> index bba8a9d..561c993 100755\n> --- a/t/t7800-difftool.sh\n> +++ b/t/t7800-difftool.sh\n> @@ -314,7 +314,7 @@ test_expect_success PERL 'setup change in subdirectory' '\n> git commit -m \"added sub/sub\" &&\n> echo test >>file &&\n> echo test >>sub/sub &&\n> - git add . &&\n> + git add file sub/sub &&\n> git commit -m \"modified both\"\n> '\n>\n>\n>> > During the tests, this means that the following sequence occurs:\n>> >\n>> > 1) the shell opens \"output\" to redirect the difftool output\n>> > 2) difftool copies the empty \"output\" to the temporary directory\n>> > 3) difftool runs \"ls\" which writes to \"output\"\n>> > 4) difftool copies the empty \"output\" file back over the output of the\n>> > command\n>> > 5) the output files doesn't contain the expected output, causing the\n>> > test to fail\n>> >\n>> > Avoid this by writing the output into .git/ which will not be copied or\n>> > overwritten.\n>>\n>> It is a good idea to move these test output and expect test vectore\n>> files to a different place to make it easier to distinguish them\n>> from test input (e.g. \"sub\", \"file\", etc.) in general, but the\n>> description of the original problem sounds like it is just working\n>> around a bug to me. What am I missing?\n>\n> I think there is a bug, as described in the paragraph below, and this\n> test should be made independent of that. In light of the above I think\n> we can drop this patch and do this with that change instead.\n\nI, too, was scratching my head when the Windows issue was first reported.\n\nThanks for clearing this up; fixing the blind \"add .\"\ncertainly seems like the way to go.\n-- \nDavid\n"},{"id":"212054","messageId":"cover.1364045138.git.john@keeping.me.uk","threadId":"32954","inReplyTo":"cover.1363980749.git.john@keeping.me.uk","subject":"[PATCH v2 0/3] difftool --dir-diff test improvements","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2013-03-23T13:31:38Z","receivedAt":"2013-03-23T13:31:38Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"This version fixes the actual cause of the test failure (not being\nspecific enough when adding files). This is done with a new version of\npatch 2.\n\nPatch 1 is unchanged and patch 3 only contains a minor change.\n\nThis is built on da/difftool-fixes. There may be a small textual\nconflict with jk/difftool-dir-diff-edit-fix in the context lines but I\ndon't believe there is any logical conflict.\n\nJohn Keeping (3):\n t7800: don't hide grep output\n t7800: fix tests when difftool uses --no-symlinks\n t7800: run --dir-diff tests with and without symlinks\n\n t/t7800-difftool.sh | 73 +++++++++++++++++++++++++++--------------------------\n 1 file changed, 37 insertions(+), 36 deletions(-)\n\n-- \n1.8.2.324.ga64ebd9\n"},{"id":"212055","messageId":"49f774ea5b09afb97945f7f1ba93b5c01e2c3dcf.1364045138.git.john@keeping.me.uk","threadId":"32954","inReplyTo":"cover.1364045138.git.john@keeping.me.uk","subject":"[PATCH v2 1/3] t7800: don't hide grep output","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2013-03-23T13:31:39Z","receivedAt":"2013-03-23T13:31:39Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"Remove the stdin_contains and stdin_doesnt_contain helper functions\nwhich add nothing but hide the output of grep, hurting debugging.\n\nSuggested-by: Johannes Sixt \nSigned-off-by: John Keeping \n---\n t/t7800-difftool.sh | 44 +++++++++++++++++---------------------------\n 1 file changed, 17 insertions(+), 27 deletions(-)\n\ndiff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh\nindex 3aab6e1..e694972 100755\n--- a/t/t7800-difftool.sh\n+++ b/t/t7800-difftool.sh\n@@ -23,16 +23,6 @@ prompt_given ()\n \ttest \"$prompt\" = \"Launch 'test-tool' [Y/n]: branch\"\n }\n \n-stdin_contains ()\n-{\n-\tgrep >/dev/null \"$1\"\n-}\n-\n-stdin_doesnot_contain ()\n-{\n-\t! stdin_contains \"$1\"\n-}\n-\n # Create a file on master and change it on branch\n test_expect_success PERL 'setup' '\n \techo master >file &&\n@@ -296,24 +286,24 @@ test_expect_success PERL 'setup with 2 files different' '\n test_expect_success PERL 'say no to the first file' '\n \t(echo n && echo) >input &&\n \tgit difftool -x cat branch output &&\n-\tstdin_contains m2 input &&\n \tgit difftool -x cat branch output &&\n-\tstdin_contains master output &&\n-\tstdin_contains tool output &&\n-\tstdin_contains sub output &&\n-\tstdin_contains sub output &&\n-\tstdin_contains sub output &&\n-\t\tstdin_contains sub \n\n---\nChanges since v1:\n- Fix the actual cause of the issue in the test instead of masking it by\n moving the output file under .git/\n\n t/t7800-difftool.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh\nindex e694972..a0b8042 100755\n--- a/t/t7800-difftool.sh\n+++ b/t/t7800-difftool.sh\n@@ -314,7 +314,7 @@ test_expect_success PERL 'setup change in subdirectory' '\n \tgit commit -m \"added sub/sub\" &&\n \techo test >>file &&\n \techo test >>sub/sub &&\n-\tgit add . &&\n+\tgit add file sub/sub &&\n \tgit commit -m \"modified both\"\n '\n \n-- \n1.8.2.324.ga64ebd9\n"},{"id":"212057","messageId":"cf71cc8757c7cb59e93d762fba922635c077376d.1364045138.git.john@keeping.me.uk","threadId":"32954","inReplyTo":"cover.1364045138.git.john@keeping.me.uk","subject":"[PATCH v2 3/3] t7800: run --dir-diff tests with and without symlinks","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2013-03-23T13:31:41Z","receivedAt":"2013-03-23T13:31:41Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"Currently the difftool --dir-diff tests may or may not use symlinks\ndepending on the operating system on which they are run. In one case\nthis has caused a test failure to be noticed only on Windows when the\ntest also fails on Linux when difftool is invoked with --no-symlinks.\n\nRewrite these tests so that they do not depend on the environment but\nrun explicitly with both --symlinks and --no-symlinks, protecting the\n--symlinks version with a SYMLINKS prerequisite.\n\nSigned-off-by: John Keeping \n\n---\nChanges since v1:\n- &&-chain from the symlinks=... line\n\n t/t7800-difftool.sh | 27 +++++++++++++++++++--------\n 1 file changed, 19 insertions(+), 8 deletions(-)\n\ndiff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh\nindex a0b8042..398e033 100755\n--- a/t/t7800-difftool.sh\n+++ b/t/t7800-difftool.sh\n@@ -318,28 +318,39 @@ test_expect_success PERL 'setup change in subdirectory' '\n \tgit commit -m \"modified both\"\n '\n \n-test_expect_success PERL 'difftool -d' '\n-\tgit difftool -d --extcmd ls branch >output &&\n+run_dir_diff_test () {\n+\ttest_expect_success PERL \"$1 --no-symlinks\" \"\n+\t\tsymlinks=--no-symlinks &&\n+\t\t$2\n+\t\"\n+\ttest_expect_success PERL,SYMLINKS \"$1 --symlinks\" \"\n+\t\tsymlinks=--symlinks &&\n+\t\t$2\n+\t\"\n+}\n+\n+run_dir_diff_test 'difftool -d' '\n+\tgit difftool -d $symlinks --extcmd ls branch >output &&\n \tgrep sub output &&\n \tgrep file output\n '\n \n-test_expect_success PERL 'difftool --dir-diff' '\n-\tgit difftool --dir-diff --extcmd ls branch >output &&\n+run_dir_diff_test 'difftool --dir-diff' '\n+\tgit difftool --dir-diff $symlinks --extcmd ls branch >output &&\n \tgrep sub output &&\n \tgrep file output\n '\n \n-test_expect_success PERL 'difftool --dir-diff ignores --prompt' '\n-\tgit difftool --dir-diff --prompt --extcmd ls branch >output &&\n+run_dir_diff_test 'difftool --dir-diff ignores --prompt' '\n+\tgit difftool --dir-diff $symlinks --prompt --extcmd ls branch >output &&\n \tgrep sub output &&\n \tgrep file output\n '\n \n-test_expect_success PERL 'difftool --dir-diff from subdirectory' '\n+run_dir_diff_test 'difftool --dir-diff from subdirectory' '\n \t(\n \t\tcd sub &&\n-\t\tgit difftool --dir-diff --extcmd ls branch >output &&\n+\t\tgit difftool --dir-diff $symlinks --extcmd ls branch >output &&\n \t\tgrep sub output &&\n \t\tgrep file output\n \t)\n-- \n1.8.2.324.ga64ebd9\n"},{"id":"212072","messageId":"7vd2up4bo7.fsf@alter.siamese.dyndns.org","threadId":"32954","inReplyTo":"e44349728c07d8ae22d4b73527b1d124b49cc4a9.1364045138.git.john@keeping.me.uk","subject":"Re: [PATCH v2 2/3] t7800: fix tests when difftool uses --no-symlinks","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-03-24T05:19:36Z","receivedAt":"2013-03-24T05:19:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"John Keeping writes:\n\n> When 'git difftool --dir-diff' is using --no-symlinks (either explicitly\n> or implicitly because it's running on Windows), any working tree files\n> that have been copied to the temporary directory are copied back after\n> the difftool completes.\n>\n> Because an earlier test uses \"git add .\", the \"output\" file used by\n> tests is tracked by Git and the following sequence occurs during some\n> tests:\n>\n> 1) the shell opens \"output\" to redirect the difftool output\n> 2) difftool copies the empty \"output\" to the temporary directory\n> 3) difftool runs \"ls\" which writes to \"output\"\n> 4) difftool copies the empty \"output\" file back over the output of the\n> command\n> 5) the output files doesn't contain the expected output, causing the\n> test to fail\n>\n> Instead of adding all changes, explicitly add only the files that the\n> test is using, allowing later tests to write their result files into the\n> working tree.\n\nGood.\n\n> In the longer term, difftool probably needs to learn to warn the user\n> instead of overwrite any changes that have been made to the working tree\n> file.\n\nQuestionable.\n\nAdmittedly I do not use difftool myself, and I have long assumed\nthat difftool users are using the tools to _view_ the changes, but\napparently some of the tools let the user muck with what is shown,\nand also apparently people seem to like the fact that they can make\nchanges. So I've led to believe the \"update in difftool, take the\nchange back to working tree, either by making symbolic links or\ncopying them back\" behaviour was a _feature_.\n\nIt is possible that this is not universally considerd as a feature,\nbut if that is the case, I think the right way to do this is to tell\nthe tools _not_ to let the user to modify contents they show in the\nfirst place, not letting the user modify and then warning after the\nfact.\n\n> Signed-off-by: John Keeping \n>\n> ---\n> Changes since v1:\n> - Fix the actual cause of the issue in the test instead of masking it by\n> moving the output file under .git/\n>\n> t/t7800-difftool.sh | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh\n> index e694972..a0b8042 100755\n> --- a/t/t7800-difftool.sh\n> +++ b/t/t7800-difftool.sh\n> @@ -314,7 +314,7 @@ test_expect_success PERL 'setup change in subdirectory' '\n> \tgit commit -m \"added sub/sub\" &&\n> \techo test >>file &&\n> \techo test >>sub/sub &&\n> -\tgit add . &&\n> +\tgit add file sub/sub &&\n> \tgit commit -m \"modified both\"\n> '\n"},{"id":"212076","messageId":"CAPig+cSjCkghxk50d-g89JUcRTi+mWWsNJ-MQsngdsOWrjuLgA@mail.gmail.com","threadId":"32954","inReplyTo":"e44349728c07d8ae22d4b73527b1d124b49cc4a9.1364045138.git.john@keeping.me.uk","subject":"Re: [PATCH v2 2/3] t7800: fix tests when difftool uses --no-symlinks","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2013-03-24T06:20:27Z","receivedAt":"2013-03-24T06:20:27Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sat, Mar 23, 2013 at 9:31 AM, John Keeping wrote:\n> 1) the shell opens \"output\" to redirect the difftool output\n> 2) difftool copies the empty \"output\" to the temporary directory\n> 3) difftool runs \"ls\" which writes to \"output\"\n> 4) difftool copies the empty \"output\" file back over the output of the\n> command\n> 5) the output files doesn't contain the expected output, causing the\n\ns/files doesn't/file doesn't/\n\n> test to fail\n>\n> Instead of adding all changes, explicitly add only the files that the\n> test is using, allowing later tests to write their result files into the\n> working tree.\n>\n> In the longer term, difftool probably needs to learn to warn the user\n> instead of overwrite any changes that have been made to the working tree\n\ns/overwrite/overwriting/\n\n> file.\n>\n> Signed-off-by: John Keeping \n"},{"id":"212090","messageId":"20130324123620.GA2286@serenity.lan","threadId":"32954","inReplyTo":"7vd2up4bo7.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2 2/3] t7800: fix tests when difftool uses --no-symlinks","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2013-03-24T12:36:20Z","receivedAt":"2013-03-24T12:36:20Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"On Sat, Mar 23, 2013 at 10:19:36PM -0700, Junio C Hamano wrote:\n> > In the longer term, difftool probably needs to learn to warn the user\n> > instead of overwrite any changes that have been made to the working tree\n> > file.\n> \n> Questionable.\n> \n> Admittedly I do not use difftool myself, and I have long assumed\n> that difftool users are using the tools to _view_ the changes, but\n> apparently some of the tools let the user muck with what is shown,\n> and also apparently people seem to like the fact that they can make\n> changes. So I've led to believe the \"update in difftool, take the\n> change back to working tree, either by making symbolic links or\n> copying them back\" behaviour was a _feature_.\n\nYes it is. I think my explanation wasn't clear enough here.\n\nWhat currently happens is that after the user's tool has finished\nrunning the working tree file and temporary file are compared and if\nthey are different then the temporary file is copied over the working\ntree file.\n\nThis is good if the user has edited the temporary file, but what if they\nedit they working tree file while using the tool to examine the\ndifferences? I think we need to at the very least look at the mtime of\nthe files and refuse to copy over the temporary file if that of the\nworking tree file is newer.\n\nObviously none of this matters if we can use symlinks, but in the\nnon-symlink case I think a user might find it surprising if the\n(unmodified) file used by their diff tool were suddenly copied over the\nworking tree wiping out the changes they have just made.\n\n> It is possible that this is not universally considerd as a feature,\n> but if that is the case, I think the right way to do this is to tell\n> the tools _not_ to let the user to modify contents they show in the\n> first place, not letting the user modify and then warning after the\n> fact.\n"},{"id":"212093","messageId":"CAJELnLF0UbHD_fZ1VMuUfrF79JQSeOABA-i3t-qX0KGo7na-hA@mail.gmail.com","threadId":"32954","inReplyTo":"7vd2up4bo7.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2 2/3] t7800: fix tests when difftool uses --no-symlinks","fromName":"Matt McClure","fromEmail":"matthewlmcclure@gmail.com","sentAt":"2013-03-24T13:24:02Z","receivedAt":"2013-03-24T13:24:02Z","isPatch":true,"sender":{"key":"matthewlmcclure@gmail.com","avatar":"https://gravatar.com/avatar/a8cde96b0594204c8d4c48baa51c983af37dedfa2b7a846e27547b8d63d46e7c?d=mp&s=160"},"body":"On Sun, Mar 24, 2013 at 1:19 AM, Junio C Hamano wrote:\n> Admittedly I do not use difftool myself, and I have long assumed\n> that difftool users are using the tools to _view_ the changes, but\n> apparently some of the tools let the user muck with what is shown,\n> and also apparently people seem to like the fact that they can make\n> changes. So I've led to believe the \"update in difftool, take the\n> change back to working tree, either by making symbolic links or\n> copying them back\" behaviour was a _feature_.\n\nDefinitely. Here's how I use it:\nhttp://matthewlmcclure.com/s/2013/03/14/use-emacs-dircmp-mode-as-a-git-difftool.html\n\n-- \nMatt McClure\nhttp://www.matthewlmcclure.com\nhttp://www.mapmyfitness.com/profile/matthewlmcclure\n"},{"id":"212094","messageId":"CAJELnLEhcY4Oc-EB=Mi7PKBQQF+EiVpW_dNH6G-abjZj0MAdNw@mail.gmail.com","threadId":"32954","inReplyTo":"20130324123620.GA2286@serenity.lan","subject":"Re: [PATCH v2 2/3] t7800: fix tests when difftool uses --no-symlinks","fromName":"Matt McClure","fromEmail":"matthewlmcclure@gmail.com","sentAt":"2013-03-24T13:31:45Z","receivedAt":"2013-03-24T13:31:45Z","isPatch":true,"sender":{"key":"matthewlmcclure@gmail.com","avatar":"https://gravatar.com/avatar/a8cde96b0594204c8d4c48baa51c983af37dedfa2b7a846e27547b8d63d46e7c?d=mp&s=160"},"body":"On Sun, Mar 24, 2013 at 8:36 AM, John Keeping wrote:\n> In the\n> non-symlink case I think a user might find it surprising if the\n> (unmodified) file used by their diff tool were suddenly copied over the\n> working tree wiping out the changes they have just made.\n\nThat's exactly what I was describing here:\nhttp://thread.gmane.org/gmane.comp.version-control.git/217979/focus=218006\n\n-- \nMatt McClure\nhttp://www.matthewlmcclure.com\nhttp://www.mapmyfitness.com/profile/matthewlmcclure\n"},{"id":"212097","messageId":"20130324151557.GB2286@serenity.lan","threadId":"32954","inReplyTo":"CAJELnLEhcY4Oc-EB=Mi7PKBQQF+EiVpW_dNH6G-abjZj0MAdNw@mail.gmail.com","subject":"Re: [PATCH v2 2/3] t7800: fix tests when difftool uses --no-symlinks","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2013-03-24T15:15:57Z","receivedAt":"2013-03-24T15:15:57Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"On Sun, Mar 24, 2013 at 09:31:45AM -0400, Matt McClure wrote:\n> On Sun, Mar 24, 2013 at 8:36 AM, John Keeping wrote:\n> > In the\n> > non-symlink case I think a user might find it surprising if the\n> > (unmodified) file used by their diff tool were suddenly copied over the\n> > working tree wiping out the changes they have just made.\n> \n> That's exactly what I was describing here:\n> http://thread.gmane.org/gmane.comp.version-control.git/217979/focus=218006\n\nAhh, I guess I didn't fully register the impact of that at the time and\nhad to rediscover the problem for myself ;-)\n\nHow about doing this (on top of jk/difftool-dir-diff-edit-fix)?\n\n-- >8 --\nSubject: [PATCH] difftool: don't overwrite modified files\n\nAfter running the user's diff tool, git-difftool will copy any files\nthat differ between the working tree and the temporary tree. This is\nuseful when the user edits the file in their diff tool but is wrong if\nthey edit the working tree file while examining the diff.\n\nInstead of copying unconditionally when the files differ, store the\ninitial hash of the working tree file and only copy the temporary file\nback if it was modified and the working tree file was not. If both\nfiles have been modified, print a warning and exit with an error.\n\nSigned-off-by: John Keeping \n---\n git-difftool.perl | 35 +++++++++++++++++++++--------------\n t/t7800-difftool.sh | 26 ++++++++++++++++++++++++++\n 2 files changed, 47 insertions(+), 14 deletions(-)\n\ndiff --git a/git-difftool.perl b/git-difftool.perl\nindex c433e86..be82b5a 100755\n--- a/git-difftool.perl\n+++ b/git-difftool.perl\n@@ -15,7 +15,6 @@ use strict;\n use warnings;\n use File::Basename qw(dirname);\n use File::Copy;\n-use File::Compare;\n use File::Find;\n use File::stat;\n use File::Path qw(mkpath rmtree);\n@@ -123,7 +122,7 @@ sub setup_dir_diff\n \tmy $rindex = '';\n \tmy %submodule;\n \tmy %symlink;\n-\tmy @working_tree = ();\n+\tmy %working_tree;\n \tmy @rawdiff = split('\\0', $diffrtn);\n \n \tmy $i = 0;\n@@ -175,7 +174,9 @@ EOF\n \n \t\tif ($rmode ne $null_mode) {\n \t\t\tif (use_wt_file($repo, $workdir, $dst_path, $rsha1, $symlinks)) {\n-\t\t\t\tpush(@working_tree, $dst_path);\n+\t\t\t\t$working_tree{$dst_path} =\n+\t\t\t\t\t$repo->command_oneline('hash-object',\n+\t\t\t\t\t\t\"$workdir/$dst_path\");\n \t\t\t} else {\n \t\t\t\t$rindex .= \"$rmode $rsha1\\t$dst_path\\0\";\n \t\t\t}\n@@ -227,7 +228,7 @@ EOF\n \t# not part of the index. Remove any trailing slash from $workdir\n \t# before starting to avoid double slashes in symlink targets.\n \t$workdir =~ s|/$||;\n-\tfor my $file (@working_tree) {\n+\tfor my $file (keys %working_tree) {\n \t\tmy $dir = dirname($file);\n \t\tunless (-d \"$rdir/$dir\") {\n \t\t\tmkpath(\"$rdir/$dir\") or\n@@ -278,7 +279,7 @@ EOF\n \t\texit_cleanup($tmpdir, 1) if not $ok;\n \t}\n \n-\treturn ($ldir, $rdir, $tmpdir, @working_tree);\n+\treturn ($ldir, $rdir, $tmpdir, %working_tree);\n }\n \n sub write_to_file\n@@ -376,7 +377,7 @@ sub dir_diff\n \tmy $error = 0;\n \tmy $repo = Git->repository();\n \tmy $workdir = find_worktree($repo);\n-\tmy ($a, $b, $tmpdir, @worktree) =\n+\tmy ($a, $b, $tmpdir, %worktree) =\n \t\tsetup_dir_diff($repo, $workdir, $symlinks);\n \n \tif (defined($extcmd)) {\n@@ -390,19 +391,25 @@ sub dir_diff\n \t# should be copied back to the working tree.\n \t# Do not copy back files when symlinks are used and the\n \t# external tool did not replace the original link with a file.\n-\tfor my $file (@worktree) {\n+\tfor my $file (keys %worktree) {\n \t\tnext if $symlinks && -l \"$b/$file\";\n \t\tnext if ! -f \"$b/$file\";\n \n-\t\tmy $diff = compare(\"$b/$file\", \"$workdir/$file\");\n-\t\tif ($diff == 0) {\n-\t\t\tnext;\n-\t\t} elsif ($diff == -1) {\n-\t\t\tmy $errmsg = \"warning: Could not compare \";\n-\t\t\t$errmsg += \"'$b/$file' with '$workdir/$file'\\n\";\n+\t\tmy $wt_hash = $repo->command_oneline('hash-object',\n+\t\t\t\"$workdir/$file\");\n+\t\tmy $tmp_hash = $repo->command_oneline('hash-object',\n+\t\t\t\"$b/$file\");\n+\t\tmy $wt_modified = $wt_hash ne $worktree{$file};\n+\t\tmy $tmp_modified = $tmp_hash ne $worktree{$file};\n+\n+\t\tif ($wt_modified and $tmp_modified) {\n+\t\t\tmy $errmsg = \"warning: Both files modified: \";\n+\t\t\t$errmsg .= \"'$workdir/$file' and '$b/$file'.\\n\";\n+\t\t\t$errmsg .= \"warning: Working tree file has been left.\\n\";\n+\t\t\t$errmsg .= \"warning:\\n\";\n \t\t\twarn $errmsg;\n \t\t\t$error = 1;\n-\t\t} elsif ($diff == 1) {\n+\t\t} elsif ($tmp_modified) {\n \t\t\tmy $mode = stat(\"$b/$file\")->mode;\n \t\t\tcopy(\"$b/$file\", \"$workdir/$file\") or\n \t\t\texit_cleanup($tmpdir, 1);\ndiff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh\nindex db3d3d6..be2042d 100755\n--- a/t/t7800-difftool.sh\n+++ b/t/t7800-difftool.sh\n@@ -407,4 +407,30 @@ test_expect_success PERL 'difftool --dir-diff from subdirectory' '\n \t)\n '\n \n+write_script modify-file <<\\EOF\n+echo \"new content\" >file\n+EOF\n+\n+test_expect_success PERL 'difftool --no-symlinks does not overwrite working tree file ' '\n+\techo \"orig content\" >file &&\n+\tgit difftool --dir-diff --no-symlinks --extcmd \"$(pwd)/modify-file\" branch &&\n+\techo \"new content\" >expect &&\n+\ttest_cmp expect file\n+'\n+\n+write_script modify-both-files <<\\EOF\n+echo \"wt content\" >file &&\n+echo \"tmp content\" >\"$2/file\" &&\n+echo \"$2\" >tmpdir\n+EOF\n+\n+test_expect_success PERL 'difftool --no-symlinks detects conflict ' '\n+\techo \"orig content\" >file &&\n+\ttest_must_fail git difftool --dir-diff --no-symlinks --extcmd \"$(pwd)/modify-both-files\" branch &&\n+\techo \"wt content\" >expect &&\n+\ttest_cmp expect file &&\n+\techo \"tmp content\" >expect &&\n+\ttest_cmp expect \"$(cat tmpdir)/file\"\n+'\n+\n test_done\n-- \n1.8.2.411.g65a544e\n"},{"id":"212136","messageId":"CAJDDKr7Uz44TQ8y2jpjhNadWUCD5Mo=GLdaLLh99eENARQSwcw@mail.gmail.com","threadId":"32954","inReplyTo":"20130324123620.GA2286@serenity.lan","subject":"Re: [PATCH v2 2/3] t7800: fix tests when difftool uses --no-symlinks","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2013-03-24T21:29:40Z","receivedAt":"2013-03-24T21:29:40Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"On Sun, Mar 24, 2013 at 5:36 AM, John Keeping wrote:\n> On Sat, Mar 23, 2013 at 10:19:36PM -0700, Junio C Hamano wrote:\n>> > In the longer term, difftool probably needs to learn to warn the user\n>> > instead of overwrite any changes that have been made to the working tree\n>> > file.\n>>\n>> Questionable.\n>>\n>> Admittedly I do not use difftool myself, and I have long assumed\n>> that difftool users are using the tools to _view_ the changes, but\n>> apparently some of the tools let the user muck with what is shown,\n>> and also apparently people seem to like the fact that they can make\n>> changes. So I've led to believe the \"update in difftool, take the\n>> change back to working tree, either by making symbolic links or\n>> copying them back\" behaviour was a _feature_.\n>\n> Yes it is. I think my explanation wasn't clear enough here.\n>\n> What currently happens is that after the user's tool has finished\n> running the working tree file and temporary file are compared and if\n> they are different then the temporary file is copied over the working\n> tree file.\n>\n> This is good if the user has edited the temporary file, but what if they\n> edit they working tree file while using the tool to examine the\n> differences? I think we need to at the very least look at the mtime of\n> the files and refuse to copy over the temporary file if that of the\n> working tree file is newer.\n>\n> Obviously none of this matters if we can use symlinks, but in the\n> non-symlink case I think a user might find it surprising if the\n> (unmodified) file used by their diff tool were suddenly copied over the\n> working tree wiping out the changes they have just made.\n\nThanks, this adds a little more safety to the operation, which is good.\nThe downside is that it's a performance hit since we end up running\nan additional hash-object on every worktree file.\nI would definitely choose safety/correctness in this situation.\n\nThis makes me wonder whether the modifiable mode should be made\nmore explicit, either in the documentation or via a flag.\n\nImagine if --dir-diff also honored --edit and --no-edit flags.\n\nRight now --edit is the default. If we had foreseen these various\nedge cases and unintended copy-backs then we may have initially\nchosen --no-edit as the default, but that's not really my point.\n\nWhat I'm thinking is that it might be good for the tool to\nlearn --edit/--no-edit so that the symlink/copy-back heuristic\ncan be documented alongside that option. Users can then know\nwhat to expect when using this mode. --no-edit would also be\nfaster since it can avoid all these extra steps.\n\nIt could also learn \"difftool.dirDiffEditable\" to control the\ndefault, which would eliminate the pain in needing to supply\nthe flag on every invocation.\n\nWhat do you think about officially supporting a read-only mode?\n-- \nDavid\n"},{"id":"212148","messageId":"514FFC3C.3010203@viscovery.net","threadId":"32954","inReplyTo":"cf71cc8757c7cb59e93d762fba922635c077376d.1364045138.git.john@keeping.me.uk","subject":"Re: [PATCH v2 3/3] t7800: run --dir-diff tests with and without symlinks","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2013-03-25T07:26:52Z","receivedAt":"2013-03-25T07:26:52Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"The series looks good, but I can't test it because it does not apply\nanywhere here.\n\nAm 3/23/2013 14:31, schrieb John Keeping:\n> Currently the difftool --dir-diff tests may or may not use symlinks\n> depending on the operating system on which they are run. In one case\n> this has caused a test failure to be noticed only on Windows when the\n> test also fails on Linux when difftool is invoked with --no-symlinks.\n> \n> Rewrite these tests so that they do not depend on the environment but\n> run explicitly with both --symlinks and --no-symlinks, protecting the\n> --symlinks version with a SYMLINKS prerequisite.\n\nAt first, I wondered what the point of having --symlinks and --no-symlinks\nwas when there is no discernable difference. But 1f229345 (difftool: Use\nsymlinks when diffing against the worktree) makes it pretty clear: It's an\noptimization, and --no-symlinks is only intended as an escape hatch.\n\n-- Hannes\n"},{"id":"212149","messageId":"514FFFC7.3090004@viscovery.net","threadId":"32954","inReplyTo":"20130324151557.GB2286@serenity.lan","subject":"Re: [PATCH v2 2/3] t7800: fix tests when difftool uses --no-symlinks","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2013-03-25T07:41:59Z","receivedAt":"2013-03-25T07:41:59Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 3/24/2013 16:15, schrieb John Keeping:\n> Subject: [PATCH] difftool: don't overwrite modified files\n> \n> After running the user's diff tool, git-difftool will copy any files\n> that differ between the working tree and the temporary tree. This is\n> useful when the user edits the file in their diff tool but is wrong if\n> they edit the working tree file while examining the diff.\n> \n> Instead of copying unconditionally when the files differ, store the\n> initial hash of the working tree file and only copy the temporary file\n> back if it was modified and the working tree file was not. If both\n> files have been modified, print a warning and exit with an error.\n> \n> Signed-off-by: John Keeping \n> ---\n> git-difftool.perl | 35 +++++++++++++++++++++--------------\n> t/t7800-difftool.sh | 26 ++++++++++++++++++++++++++\n> 2 files changed, 47 insertions(+), 14 deletions(-)\n> \n> diff --git a/git-difftool.perl b/git-difftool.perl\n> index c433e86..be82b5a 100755\n> --- a/git-difftool.perl\n> +++ b/git-difftool.perl\n> @@ -15,7 +15,6 @@ use strict;\n> use warnings;\n> use File::Basename qw(dirname);\n> use File::Copy;\n> -use File::Compare;\n> use File::Find;\n> use File::stat;\n> use File::Path qw(mkpath rmtree);\n> @@ -123,7 +122,7 @@ sub setup_dir_diff\n> \tmy $rindex = '';\n> \tmy %submodule;\n> \tmy %symlink;\n> -\tmy @working_tree = ();\n> +\tmy %working_tree;\n> \tmy @rawdiff = split('\\0', $diffrtn);\n> \n> \tmy $i = 0;\n> @@ -175,7 +174,9 @@ EOF\n> \n> \t\tif ($rmode ne $null_mode) {\n> \t\t\tif (use_wt_file($repo, $workdir, $dst_path, $rsha1, $symlinks)) {\n> -\t\t\t\tpush(@working_tree, $dst_path);\n> +\t\t\t\t$working_tree{$dst_path} =\n> +\t\t\t\t\t$repo->command_oneline('hash-object',\n> +\t\t\t\t\t\t\"$workdir/$dst_path\");\n> \t\t\t} else {\n> \t\t\t\t$rindex .= \"$rmode $rsha1\\t$dst_path\\0\";\n> \t\t\t}\n> @@ -227,7 +228,7 @@ EOF\n> \t# not part of the index. Remove any trailing slash from $workdir\n> \t# before starting to avoid double slashes in symlink targets.\n> \t$workdir =~ s|/$||;\n> -\tfor my $file (@working_tree) {\n> +\tfor my $file (keys %working_tree) {\n> \t\tmy $dir = dirname($file);\n> \t\tunless (-d \"$rdir/$dir\") {\n> \t\t\tmkpath(\"$rdir/$dir\") or\n> @@ -278,7 +279,7 @@ EOF\n> \t\texit_cleanup($tmpdir, 1) if not $ok;\n> \t}\n> \n> -\treturn ($ldir, $rdir, $tmpdir, @working_tree);\n> +\treturn ($ldir, $rdir, $tmpdir, %working_tree);\n> }\n> \n> sub write_to_file\n> @@ -376,7 +377,7 @@ sub dir_diff\n> \tmy $error = 0;\n> \tmy $repo = Git->repository();\n> \tmy $workdir = find_worktree($repo);\n> -\tmy ($a, $b, $tmpdir, @worktree) =\n> +\tmy ($a, $b, $tmpdir, %worktree) =\n> \t\tsetup_dir_diff($repo, $workdir, $symlinks);\n> \n> \tif (defined($extcmd)) {\n> @@ -390,19 +391,25 @@ sub dir_diff\n> \t# should be copied back to the working tree.\n> \t# Do not copy back files when symlinks are used and the\n> \t# external tool did not replace the original link with a file.\n> -\tfor my $file (@worktree) {\n> +\tfor my $file (keys %worktree) {\n> \t\tnext if $symlinks && -l \"$b/$file\";\n> \t\tnext if ! -f \"$b/$file\";\n> \n> -\t\tmy $diff = compare(\"$b/$file\", \"$workdir/$file\");\n> -\t\tif ($diff == 0) {\n> -\t\t\tnext;\n> -\t\t} elsif ($diff == -1) {\n> -\t\t\tmy $errmsg = \"warning: Could not compare \";\n> -\t\t\t$errmsg += \"'$b/$file' with '$workdir/$file'\\n\";\n> +\t\tmy $wt_hash = $repo->command_oneline('hash-object',\n> +\t\t\t\"$workdir/$file\");\n> +\t\tmy $tmp_hash = $repo->command_oneline('hash-object',\n> +\t\t\t\"$b/$file\");\n\nThis is gross. Can't we do much better here? Difftool already keeps a\nGIT_INDEX of the files in the temporary tree ($tmpdir/rindex). Running\ngit-diff-files should be sufficient to tell which ones where edited via\nthe users's diff-tool. Then you can restrict calling hash-object to only\nthose worktree files where an \"edit collision\" needs to be checked for.\n\nYou could also keep a parallel index that keeps the state of the same set\nof files in the worktree. Then another git-diff-files call could replace\nthe other half of hash-object calls.\n\n> +\t\tmy $wt_modified = $wt_hash ne $worktree{$file};\n> +\t\tmy $tmp_modified = $tmp_hash ne $worktree{$file};\n> +\n> +\t\tif ($wt_modified and $tmp_modified) {\n> +\t\t\tmy $errmsg = \"warning: Both files modified: \";\n> +\t\t\t$errmsg .= \"'$workdir/$file' and '$b/$file'.\\n\";\n> +\t\t\t$errmsg .= \"warning: Working tree file has been left.\\n\";\n> +\t\t\t$errmsg .= \"warning:\\n\";\n> \t\t\twarn $errmsg;\n> \t\t\t$error = 1;\n> -\t\t} elsif ($diff == 1) {\n> +\t\t} elsif ($tmp_modified) {\n> \t\t\tmy $mode = stat(\"$b/$file\")->mode;\n> \t\t\tcopy(\"$b/$file\", \"$workdir/$file\") or\n> \t\t\texit_cleanup($tmpdir, 1);\n\n-- Hannes\n"},{"id":"212157","messageId":"20130325103516.GC2286@serenity.lan","threadId":"32954","inReplyTo":"514FFC3C.3010203@viscovery.net","subject":"Re: [PATCH v2 3/3] t7800: run --dir-diff tests with and without symlinks","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2013-03-25T10:35:16Z","receivedAt":"2013-03-25T10:35:16Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"On Mon, Mar 25, 2013 at 08:26:52AM +0100, Johannes Sixt wrote:\n> The series looks good, but I can't test it because it does not apply\n> anywhere here.\n\nIt's built on top of da/difftool-fixes, is there some problem that stops\nit applying cleanly on top of that?\n\n> Am 3/23/2013 14:31, schrieb John Keeping:\n> > Currently the difftool --dir-diff tests may or may not use symlinks\n> > depending on the operating system on which they are run. In one case\n> > this has caused a test failure to be noticed only on Windows when the\n> > test also fails on Linux when difftool is invoked with --no-symlinks.\n> > \n> > Rewrite these tests so that they do not depend on the environment but\n> > run explicitly with both --symlinks and --no-symlinks, protecting the\n> > --symlinks version with a SYMLINKS prerequisite.\n> \n> At first, I wondered what the point of having --symlinks and --no-symlinks\n> was when there is no discernable difference. But 1f229345 (difftool: Use\n> symlinks when diffing against the worktree) makes it pretty clear: It's an\n> optimization, and --no-symlinks is only intended as an escape hatch.\n"},{"id":"212158","messageId":"20130325104219.GD2286@serenity.lan","threadId":"32954","inReplyTo":"514FFFC7.3090004@viscovery.net","subject":"Re: [PATCH v2 2/3] t7800: fix tests when difftool uses --no-symlinks","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2013-03-25T10:42:19Z","receivedAt":"2013-03-25T10:42:19Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"On Mon, Mar 25, 2013 at 08:41:59AM +0100, Johannes Sixt wrote:\n> Am 3/24/2013 16:15, schrieb John Keeping:\n> > Subject: [PATCH] difftool: don't overwrite modified files\n> > \n> > After running the user's diff tool, git-difftool will copy any files\n> > that differ between the working tree and the temporary tree. This is\n> > useful when the user edits the file in their diff tool but is wrong if\n> > they edit the working tree file while examining the diff.\n> > \n> > Instead of copying unconditionally when the files differ, store the\n> > initial hash of the working tree file and only copy the temporary file\n> > back if it was modified and the working tree file was not. If both\n> > files have been modified, print a warning and exit with an error.\n> > \n> > Signed-off-by: John Keeping \n> > ---\n> > git-difftool.perl | 35 +++++++++++++++++++++--------------\n> > t/t7800-difftool.sh | 26 ++++++++++++++++++++++++++\n> > 2 files changed, 47 insertions(+), 14 deletions(-)\n> > \n> > diff --git a/git-difftool.perl b/git-difftool.perl\n> > index c433e86..be82b5a 100755\n> > --- a/git-difftool.perl\n> > +++ b/git-difftool.perl\n> > @@ -15,7 +15,6 @@ use strict;\n> > use warnings;\n> > use File::Basename qw(dirname);\n> > use File::Copy;\n> > -use File::Compare;\n> > use File::Find;\n> > use File::stat;\n> > use File::Path qw(mkpath rmtree);\n> > @@ -123,7 +122,7 @@ sub setup_dir_diff\n> > \tmy $rindex = '';\n> > \tmy %submodule;\n> > \tmy %symlink;\n> > -\tmy @working_tree = ();\n> > +\tmy %working_tree;\n> > \tmy @rawdiff = split('\\0', $diffrtn);\n> > \n> > \tmy $i = 0;\n> > @@ -175,7 +174,9 @@ EOF\n> > \n> > \t\tif ($rmode ne $null_mode) {\n> > \t\t\tif (use_wt_file($repo, $workdir, $dst_path, $rsha1, $symlinks)) {\n> > -\t\t\t\tpush(@working_tree, $dst_path);\n> > +\t\t\t\t$working_tree{$dst_path} =\n> > +\t\t\t\t\t$repo->command_oneline('hash-object',\n> > +\t\t\t\t\t\t\"$workdir/$dst_path\");\n> > \t\t\t} else {\n> > \t\t\t\t$rindex .= \"$rmode $rsha1\\t$dst_path\\0\";\n> > \t\t\t}\n> > @@ -227,7 +228,7 @@ EOF\n> > \t# not part of the index. Remove any trailing slash from $workdir\n> > \t# before starting to avoid double slashes in symlink targets.\n> > \t$workdir =~ s|/$||;\n> > -\tfor my $file (@working_tree) {\n> > +\tfor my $file (keys %working_tree) {\n> > \t\tmy $dir = dirname($file);\n> > \t\tunless (-d \"$rdir/$dir\") {\n> > \t\t\tmkpath(\"$rdir/$dir\") or\n> > @@ -278,7 +279,7 @@ EOF\n> > \t\texit_cleanup($tmpdir, 1) if not $ok;\n> > \t}\n> > \n> > -\treturn ($ldir, $rdir, $tmpdir, @working_tree);\n> > +\treturn ($ldir, $rdir, $tmpdir, %working_tree);\n> > }\n> > \n> > sub write_to_file\n> > @@ -376,7 +377,7 @@ sub dir_diff\n> > \tmy $error = 0;\n> > \tmy $repo = Git->repository();\n> > \tmy $workdir = find_worktree($repo);\n> > -\tmy ($a, $b, $tmpdir, @worktree) =\n> > +\tmy ($a, $b, $tmpdir, %worktree) =\n> > \t\tsetup_dir_diff($repo, $workdir, $symlinks);\n> > \n> > \tif (defined($extcmd)) {\n> > @@ -390,19 +391,25 @@ sub dir_diff\n> > \t# should be copied back to the working tree.\n> > \t# Do not copy back files when symlinks are used and the\n> > \t# external tool did not replace the original link with a file.\n> > -\tfor my $file (@worktree) {\n> > +\tfor my $file (keys %worktree) {\n> > \t\tnext if $symlinks && -l \"$b/$file\";\n> > \t\tnext if ! -f \"$b/$file\";\n> > \n> > -\t\tmy $diff = compare(\"$b/$file\", \"$workdir/$file\");\n> > -\t\tif ($diff == 0) {\n> > -\t\t\tnext;\n> > -\t\t} elsif ($diff == -1) {\n> > -\t\t\tmy $errmsg = \"warning: Could not compare \";\n> > -\t\t\t$errmsg += \"'$b/$file' with '$workdir/$file'\\n\";\n> > +\t\tmy $wt_hash = $repo->command_oneline('hash-object',\n> > +\t\t\t\"$workdir/$file\");\n> > +\t\tmy $tmp_hash = $repo->command_oneline('hash-object',\n> > +\t\t\t\"$b/$file\");\n> \n> This is gross. Can't we do much better here? Difftool already keeps a\n> GIT_INDEX of the files in the temporary tree ($tmpdir/rindex). Running\n> git-diff-files should be sufficient to tell which ones where edited via\n> the users's diff-tool. Then you can restrict calling hash-object to only\n> those worktree files where an \"edit collision\" needs to be checked for.\n\nThat's only the case for files that are not copied from the working\ntree, so the temporary index doesn't contain the files that are of\ninterest here.\n\n> You could also keep a parallel index that keeps the state of the same set\n> of files in the worktree. Then another git-diff-files call could replace\n> the other half of hash-object calls.\n\nI like the idea of creating an index from the working tree files and\nusing it here. If we create a \"starting state\" index for these files,\nwe should be able to run git-diff-files against both the working tree\nand the temporary tree at this point and compare the output. I'll try\nthis approach this evening.\n\n> > +\t\tmy $wt_modified = $wt_hash ne $worktree{$file};\n> > +\t\tmy $tmp_modified = $tmp_hash ne $worktree{$file};\n> > +\n> > +\t\tif ($wt_modified and $tmp_modified) {\n> > +\t\t\tmy $errmsg = \"warning: Both files modified: \";\n> > +\t\t\t$errmsg .= \"'$workdir/$file' and '$b/$file'.\\n\";\n> > +\t\t\t$errmsg .= \"warning: Working tree file has been left.\\n\";\n> > +\t\t\t$errmsg .= \"warning:\\n\";\n> > \t\t\twarn $errmsg;\n> > \t\t\t$error = 1;\n> > -\t\t} elsif ($diff == 1) {\n> > +\t\t} elsif ($tmp_modified) {\n> > \t\t\tmy $mode = stat(\"$b/$file\")->mode;\n> > \t\t\tcopy(\"$b/$file\", \"$workdir/$file\") or\n> > \t\t\texit_cleanup($tmpdir, 1);\n"},{"id":"212160","messageId":"20130325105731.GE2286@serenity.lan","threadId":"32954","inReplyTo":"CAJDDKr7Uz44TQ8y2jpjhNadWUCD5Mo=GLdaLLh99eENARQSwcw@mail.gmail.com","subject":"Re: [PATCH v2 2/3] t7800: fix tests when difftool uses --no-symlinks","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2013-03-25T10:57:32Z","receivedAt":"2013-03-25T10:57:32Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"On Sun, Mar 24, 2013 at 02:29:40PM -0700, David Aguilar wrote:\n> This makes me wonder whether the modifiable mode should be made\n> more explicit, either in the documentation or via a flag.\n> \n> Imagine if --dir-diff also honored --edit and --no-edit flags.\n> \n> Right now --edit is the default. If we had foreseen these various\n> edge cases and unintended copy-backs then we may have initially\n> chosen --no-edit as the default, but that's not really my point.\n\nI view --symlinks as the default, which avoids most of this pain ;-)\nI guess we're talking about three different \"working tree files\" modes\nhere: symlink, copy-copyback and copy-readonly.\n\nI wonder if anyone uses --no-symlinks when they are not forced to by\ntheir operating system? What is the use case if they do?\n\n> What I'm thinking is that it might be good for the tool to\n> learn --edit/--no-edit so that the symlink/copy-back heuristic\n> can be documented alongside that option. Users can then know\n> what to expect when using this mode. --no-edit would also be\n> faster since it can avoid all these extra steps.\n> \n> It could also learn \"difftool.dirDiffEditable\" to control the\n> default, which would eliminate the pain in needing to supply\n> the flag on every invocation.\n> \n> What do you think about officially supporting a read-only mode?\n\nHow would that interoperate with symlink mode? Should --no-edit imply\n--no-symlinks or does the --[no-]edit option only have an effect if\n--no-symlinks is in effect?\n\nI don't think this is the first time this idea has been suggested, so\nthat's some indicator that it's a good idea. I'm not sure about\n--edit/--no-edit for this though. The behaviour isn't really similar to\nthe way that option works with git-commit, git-merge, etc. I don't have\na better suggestion at the moment though.\n\n\nJohn\n"},{"id":"212161","messageId":"51502E00.7070904@viscovery.net","threadId":"32954","inReplyTo":"20130325103516.GC2286@serenity.lan","subject":"Re: [PATCH v2 3/3] t7800: run --dir-diff tests with and without symlinks","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2013-03-25T10:59:12Z","receivedAt":"2013-03-25T10:59:12Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 3/25/2013 11:35, schrieb John Keeping:\n> On Mon, Mar 25, 2013 at 08:26:52AM +0100, Johannes Sixt wrote:\n>> The series looks good, but I can't test it because it does not apply\n>> anywhere here.\n> \n> It's built on top of da/difftool-fixes, is there some problem that stops\n> it applying cleanly on top of that?\n\nThanks. I had only tried trees that were \"contaminated\" by\njk/difftool-dir-diff-edit-fix, which is in conflict with da/difftool-fixes.\n\nt7800 passes on Windows with these patches.\n\n-- Hannes\n"},{"id":"212163","messageId":"20130325110220.GF2286@serenity.lan","threadId":"32954","inReplyTo":"51502E00.7070904@viscovery.net","subject":"Re: [PATCH v2 3/3] t7800: run --dir-diff tests with and without symlinks","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2013-03-25T11:02:20Z","receivedAt":"2013-03-25T11:02:20Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"On Mon, Mar 25, 2013 at 11:59:12AM +0100, Johannes Sixt wrote:\n> Am 3/25/2013 11:35, schrieb John Keeping:\n> > On Mon, Mar 25, 2013 at 08:26:52AM +0100, Johannes Sixt wrote:\n> >> The series looks good, but I can't test it because it does not apply\n> >> anywhere here.\n> > \n> > It's built on top of da/difftool-fixes, is there some problem that stops\n> > it applying cleanly on top of that?\n> \n> Thanks. I had only tried trees that were \"contaminated\" by\n> jk/difftool-dir-diff-edit-fix, which is in conflict with da/difftool-fixes.\n\nI think they merge OK, but I suspect git-am won't apply the patches\ncleanly on top of the result.\n\n> t7800 passes on Windows with these patches.\n\nThanks.\n"},{"id":"212166","messageId":"7v620f34xw.fsf@alter.siamese.dyndns.org","threadId":"32954","inReplyTo":"CAJDDKr7Uz44TQ8y2jpjhNadWUCD5Mo=GLdaLLh99eENARQSwcw@mail.gmail.com","subject":"Re: [PATCH v2 2/3] t7800: fix tests when difftool uses --no-symlinks","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-03-25T14:54:51Z","receivedAt":"2013-03-25T14:54:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"David Aguilar writes:\n\n> This makes me wonder whether the modifiable mode should be made\n> more explicit, either in the documentation or via a flag.\n>\n> Imagine if --dir-diff also honored --edit and --no-edit flags.\n>\n> Right now --edit is the default. If we had foreseen these various\n> edge cases and unintended copy-backs then we may have initially\n> chosen --no-edit as the default, but that's not really my point.\n>\n> What I'm thinking is that it might be good for the tool to\n> learn --edit/--no-edit so that the symlink/copy-back heuristic\n> can be documented alongside that option. Users can then know\n> what to expect when using this mode. --no-edit would also be\n> faster since it can avoid all these extra steps.\n>\n> It could also learn \"difftool.dirDiffEditable\" to control the\n> default, which would eliminate the pain in needing to supply\n> the flag on every invocation.\n>\n> What do you think about officially supporting a read-only mode?\n\nYeah, actually that was what I've been assuming the default (not\nsuggesting to change the default behaviour here).\n"},{"id":"212176","messageId":"7vvc8fzc8w.fsf@alter.siamese.dyndns.org","threadId":"32954","inReplyTo":"514FFFC7.3090004@viscovery.net","subject":"Re: [PATCH v2 2/3] t7800: fix tests when difftool uses --no-symlinks","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-03-25T16:15:59Z","receivedAt":"2013-03-25T16:15:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt writes:\n\n> Am 3/24/2013 16:15, schrieb John Keeping:\n> ...\n>> +\tfor my $file (keys %worktree) {\n>> \t\tnext if $symlinks && -l \"$b/$file\";\n>> \t\tnext if ! -f \"$b/$file\";\n>> \n>> -\t\tmy $diff = compare(\"$b/$file\", \"$workdir/$file\");\n>> -\t\tif ($diff == 0) {\n>> -\t\t\tnext;\n>> -\t\t} elsif ($diff == -1) {\n>> -\t\t\tmy $errmsg = \"warning: Could not compare \";\n>> -\t\t\t$errmsg += \"'$b/$file' with '$workdir/$file'\\n\";\n>> +\t\tmy $wt_hash = $repo->command_oneline('hash-object',\n>> +\t\t\t\"$workdir/$file\");\n>> +\t\tmy $tmp_hash = $repo->command_oneline('hash-object',\n>> +\t\t\t\"$b/$file\");\n>\n> This is gross. Can't we do much better here? Difftool already keeps a\n> GIT_INDEX of the files in the temporary tree ($tmpdir/rindex). Running\n> git-diff-files should be sufficient to tell which ones where edited via\n> the users's diff-tool. Then you can restrict calling hash-object to only\n> those worktree files where an \"edit collision\" needs to be checked for.\n\n;-).\n"},{"id":"212233","messageId":"20130325214430.GG2286@serenity.lan","threadId":"32954","inReplyTo":"20130325104219.GD2286@serenity.lan","subject":"[PATCH v2] difftool: don't overwrite modified files","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2013-03-25T21:44:30Z","receivedAt":"2013-03-25T21:44:30Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"After running the user's diff tool, git-difftool will copy any files\nthat differ between the working tree and the temporary tree. This is\nuseful when the user edits the file in their diff tool but is wrong if\nthey edit the working tree file while examining the diff.\n\nInstead of copying unconditionally when the files differ, create and\nindex from the working tree files and only copy the temporary file back\nif it was modified and the working tree file was not. If both files\nhave been modified, print a warning and exit with an error.\n\nNote that we cannot use an existing index in git-difftool since those\ncontain the modified files that need to be checked out but here we are\nlooking at those files which are copied from the working tree and not\nchecked out. These are precisely the files which are not in the\nexisting indices.\n\nSigned-off-by: John Keeping \n---\nOn Mon, Mar 25, 2013 at 10:42:19AM +0000, John Keeping wrote:\n> On Mon, Mar 25, 2013 at 08:41:59AM +0100, Johannes Sixt wrote:\n> > This is gross. Can't we do much better here? Difftool already keeps a\n> > GIT_INDEX of the files in the temporary tree ($tmpdir/rindex). Running\n> > git-diff-files should be sufficient to tell which ones where edited via\n> > the users's diff-tool. Then you can restrict calling hash-object to only\n> > those worktree files where an \"edit collision\" needs to be checked for.\n> \n> That's only the case for files that are not copied from the working\n> tree, so the temporary index doesn't contain the files that are of\n> interest here.\n> \n> > You could also keep a parallel index that keeps the state of the same set\n> > of files in the worktree. Then another git-diff-files call could replace\n> > the other half of hash-object calls.\n> \n> I like the idea of creating an index from the working tree files and\n> using it here. If we create a \"starting state\" index for these files,\n> we should be able to run git-diff-files against both the working tree\n> and the temporary tree at this point and compare the output.\n\nHere's an attempt at taking this approach, built on\njk/difftool-dir-diff-edit-fix.\n\n git-difftool.perl | 73 +++++++++++++++++++++++++++++++++++++++++++----------\n t/t7800-difftool.sh | 26 +++++++++++++++++++\n 2 files changed, 85 insertions(+), 14 deletions(-)\n\ndiff --git a/git-difftool.perl b/git-difftool.perl\nindex c433e86..d10f7d2 100755\n--- a/git-difftool.perl\n+++ b/git-difftool.perl\n@@ -13,9 +13,9 @@\n use 5.008;\n use strict;\n use warnings;\n+use Error qw(:try);\n use File::Basename qw(dirname);\n use File::Copy;\n-use File::Compare;\n use File::Find;\n use File::stat;\n use File::Path qw(mkpath rmtree);\n@@ -88,14 +88,45 @@ sub use_wt_file\n \tmy ($repo, $workdir, $file, $sha1, $symlinks) = @_;\n \tmy $null_sha1 = '0' x 40;\n \n-\tif ($sha1 eq $null_sha1) {\n-\t\treturn 1;\n-\t} elsif (not $symlinks) {\n+\tif ($sha1 ne $null_sha1 and not $symlinks) {\n \t\treturn 0;\n \t}\n \n \tmy $wt_sha1 = $repo->command_oneline('hash-object', \"$workdir/$file\");\n-\treturn $sha1 eq $wt_sha1;\n+\tmy $use = ($sha1 eq $null_sha1) || ($sha1 eq $wt_sha1);\n+\treturn ($use, $wt_sha1);\n+}\n+\n+sub changed_files\n+{\n+\tmy ($repo_path, $index, $worktree) = @_;\n+\t$ENV{GIT_INDEX_FILE} = $index;\n+\t$ENV{GIT_WORK_TREE} = $worktree;\n+\tmy $must_unset_git_dir = 0;\n+\tif (not defined($ENV{GIT_DIR})) {\n+\t\t$must_unset_git_dir = 1;\n+\t\t$ENV{GIT_DIR} = $repo_path;\n+\t}\n+\n+\tmy @refreshargs = qw/update-index --really-refresh -q --unmerged/;\n+\tmy @gitargs = qw/diff-files --name-only -z/;\n+\ttry {\n+\t\tGit::command_oneline(@refreshargs);\n+\t} catch Git::Error::Command with {};\n+\n+\tmy $line = Git::command_oneline(@gitargs);\n+\tmy @files;\n+\tif (defined $line) {\n+\t\t@files = split('\\0', $line);\n+\t} else {\n+\t\t@files = ();\n+\t}\n+\n+\tdelete($ENV{GIT_INDEX_FILE});\n+\tdelete($ENV{GIT_WORK_TREE});\n+\tdelete($ENV{GIT_DIR}) if ($must_unset_git_dir);\n+\n+\treturn map { $_ => 1 } @files;\n }\n \n sub setup_dir_diff\n@@ -121,6 +152,7 @@ sub setup_dir_diff\n \tmy $null_sha1 = '0' x 40;\n \tmy $lindex = '';\n \tmy $rindex = '';\n+\tmy $wtindex = '';\n \tmy %submodule;\n \tmy %symlink;\n \tmy @working_tree = ();\n@@ -174,8 +206,12 @@ EOF\n \t\t}\n \n \t\tif ($rmode ne $null_mode) {\n-\t\t\tif (use_wt_file($repo, $workdir, $dst_path, $rsha1, $symlinks)) {\n-\t\t\t\tpush(@working_tree, $dst_path);\n+\t\t\tmy ($use, $wt_sha1) = use_wt_file($repo, $workdir,\n+\t\t\t\t\t\t\t $dst_path, $rsha1,\n+\t\t\t\t\t\t\t $symlinks);\n+\t\t\tif ($use) {\n+\t\t\t\tpush @working_tree, $dst_path;\n+\t\t\t\t$wtindex .= \"$rmode $wt_sha1\\t$dst_path\\0\";\n \t\t\t} else {\n \t\t\t\t$rindex .= \"$rmode $rsha1\\t$dst_path\\0\";\n \t\t\t}\n@@ -218,6 +254,12 @@ EOF\n \t$rc = system('git', 'checkout-index', '--all', \"--prefix=$rdir/\");\n \texit_cleanup($tmpdir, $rc) if $rc != 0;\n \n+\t$ENV{GIT_INDEX_FILE} = \"$tmpdir/wtindex\";\n+\t($inpipe, $ctx) =\n+\t\t$repo->command_input_pipe(qw(update-index --info-only -z --index-info));\n+\tprint($inpipe $wtindex);\n+\t$repo->command_close_pipe($inpipe, $ctx);\n+\n \t# If $GIT_DIR was explicitly set just for the update/checkout\n \t# commands, then it should be unset before continuing.\n \tdelete($ENV{GIT_DIR}) if ($must_unset_git_dir);\n@@ -390,19 +432,22 @@ sub dir_diff\n \t# should be copied back to the working tree.\n \t# Do not copy back files when symlinks are used and the\n \t# external tool did not replace the original link with a file.\n+\tmy %wt_modified = changed_files($repo->repo_path(),\n+\t\t\"$tmpdir/wtindex\", \"$workdir\");\n+\tmy %tmp_modified = changed_files($repo->repo_path(),\n+\t\t\"$tmpdir/wtindex\", \"$b\");\n \tfor my $file (@worktree) {\n \t\tnext if $symlinks && -l \"$b/$file\";\n \t\tnext if ! -f \"$b/$file\";\n \n-\t\tmy $diff = compare(\"$b/$file\", \"$workdir/$file\");\n-\t\tif ($diff == 0) {\n-\t\t\tnext;\n-\t\t} elsif ($diff == -1) {\n-\t\t\tmy $errmsg = \"warning: Could not compare \";\n-\t\t\t$errmsg += \"'$b/$file' with '$workdir/$file'\\n\";\n+\t\tif (exists $wt_modified{$file} and exists $tmp_modified{$file}) {\n+\t\t\tmy $errmsg = \"warning: Both files modified: \";\n+\t\t\t$errmsg .= \"'$workdir/$file' and '$b/$file'.\\n\";\n+\t\t\t$errmsg .= \"warning: Working tree file has been left.\\n\";\n+\t\t\t$errmsg .= \"warning:\\n\";\n \t\t\twarn $errmsg;\n \t\t\t$error = 1;\n-\t\t} elsif ($diff == 1) {\n+\t\t} elsif ($tmp_modified{$file}) {\n \t\t\tmy $mode = stat(\"$b/$file\")->mode;\n \t\t\tcopy(\"$b/$file\", \"$workdir/$file\") or\n \t\t\texit_cleanup($tmpdir, 1);\ndiff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh\nindex db3d3d6..be2042d 100755\n--- a/t/t7800-difftool.sh\n+++ b/t/t7800-difftool.sh\n@@ -407,4 +407,30 @@ test_expect_success PERL 'difftool --dir-diff from subdirectory' '\n \t)\n '\n \n+write_script modify-file <<\\EOF\n+echo \"new content\" >file\n+EOF\n+\n+test_expect_success PERL 'difftool --no-symlinks does not overwrite working tree file ' '\n+\techo \"orig content\" >file &&\n+\tgit difftool --dir-diff --no-symlinks --extcmd \"$(pwd)/modify-file\" branch &&\n+\techo \"new content\" >expect &&\n+\ttest_cmp expect file\n+'\n+\n+write_script modify-both-files <<\\EOF\n+echo \"wt content\" >file &&\n+echo \"tmp content\" >\"$2/file\" &&\n+echo \"$2\" >tmpdir\n+EOF\n+\n+test_expect_success PERL 'difftool --no-symlinks detects conflict ' '\n+\techo \"orig content\" >file &&\n+\ttest_must_fail git difftool --dir-diff --no-symlinks --extcmd \"$(pwd)/modify-both-files\" branch &&\n+\techo \"wt content\" >expect &&\n+\ttest_cmp expect file &&\n+\techo \"tmp content\" >expect &&\n+\ttest_cmp expect \"$(cat tmpdir)/file\"\n+'\n+\n test_done\n-- \n1.8.2.411.g65a544e\n"},{"id":"212235","messageId":"7vobe7w3m9.fsf@alter.siamese.dyndns.org","threadId":"32954","inReplyTo":"51502E00.7070904@viscovery.net","subject":"Re: [PATCH v2 3/3] t7800: run --dir-diff tests with and without symlinks","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-03-25T21:50:38Z","receivedAt":"2013-03-25T21:50:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt writes:\n\n> Am 3/25/2013 11:35, schrieb John Keeping:\n>> On Mon, Mar 25, 2013 at 08:26:52AM +0100, Johannes Sixt wrote:\n>>> The series looks good, but I can't test it because it does not apply\n>>> anywhere here.\n>> \n>> It's built on top of da/difftool-fixes, is there some problem that stops\n>> it applying cleanly on top of that?\n>\n> Thanks. I had only tried trees that were \"contaminated\" by\n> jk/difftool-dir-diff-edit-fix, which is in conflict with da/difftool-fixes.\n\nYes, the conflict was unpleasant to deal with. John, I think what\nis queued on 'pu' is OK, but please double check after I push it out\nin a few hours.\n"},{"id":"212271","messageId":"51515E92.4030009@viscovery.net","threadId":"32954","inReplyTo":"20130325214430.GG2286@serenity.lan","subject":"Re: [PATCH v2] difftool: don't overwrite modified files","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2013-03-26T08:38:42Z","receivedAt":"2013-03-26T08:38:42Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 3/25/2013 22:44, schrieb John Keeping:\n> After running the user's diff tool, git-difftool will copy any files\n> that differ between the working tree and the temporary tree. This is\n> useful when the user edits the file in their diff tool but is wrong if\n> they edit the working tree file while examining the diff.\n> \n> Instead of copying unconditionally when the files differ, create and\n> index from the working tree files and only copy the temporary file back\n> if it was modified and the working tree file was not. If both files\n> have been modified, print a warning and exit with an error.\n> \n> Note that we cannot use an existing index in git-difftool since those\n> contain the modified files that need to be checked out but here we are\n> looking at those files which are copied from the working tree and not\n> checked out. These are precisely the files which are not in the\n> existing indices.\n> \n> Signed-off-by: John Keeping \n> ---\n> On Mon, Mar 25, 2013 at 10:42:19AM +0000, John Keeping wrote:\n>> On Mon, Mar 25, 2013 at 08:41:59AM +0100, Johannes Sixt wrote:\n>>> This is gross. Can't we do much better here? Difftool already keeps a\n>>> GIT_INDEX of the files in the temporary tree ($tmpdir/rindex). Running\n>>> git-diff-files should be sufficient to tell which ones where edited via\n>>> the users's diff-tool. Then you can restrict calling hash-object to only\n>>> those worktree files where an \"edit collision\" needs to be checked for.\n>>\n>> That's only the case for files that are not copied from the working\n>> tree, so the temporary index doesn't contain the files that are of\n>> interest here.\n>>\n>>> You could also keep a parallel index that keeps the state of the same set\n>>> of files in the worktree. Then another git-diff-files call could replace\n>>> the other half of hash-object calls.\n>>\n>> I like the idea of creating an index from the working tree files and\n>> using it here. If we create a \"starting state\" index for these files,\n>> we should be able to run git-diff-files against both the working tree\n>> and the temporary tree at this point and compare the output.\n> \n> Here's an attempt at taking this approach, built on\n> jk/difftool-dir-diff-edit-fix.\n> \n> git-difftool.perl | 73 +++++++++++++++++++++++++++++++++++++++++++----------\n> t/t7800-difftool.sh | 26 +++++++++++++++++++\n> 2 files changed, 85 insertions(+), 14 deletions(-)\n> \n> diff --git a/git-difftool.perl b/git-difftool.perl\n> index c433e86..d10f7d2 100755\n> --- a/git-difftool.perl\n> +++ b/git-difftool.perl\n> @@ -13,9 +13,9 @@\n> use 5.008;\n> use strict;\n> use warnings;\n> +use Error qw(:try);\n> use File::Basename qw(dirname);\n> use File::Copy;\n> -use File::Compare;\n> use File::Find;\n> use File::stat;\n> use File::Path qw(mkpath rmtree);\n> @@ -88,14 +88,45 @@ sub use_wt_file\n> \tmy ($repo, $workdir, $file, $sha1, $symlinks) = @_;\n> \tmy $null_sha1 = '0' x 40;\n> \n> -\tif ($sha1 eq $null_sha1) {\n> -\t\treturn 1;\n> -\t} elsif (not $symlinks) {\n> +\tif ($sha1 ne $null_sha1 and not $symlinks) {\n> \t\treturn 0;\n> \t}\n> \n> \tmy $wt_sha1 = $repo->command_oneline('hash-object', \"$workdir/$file\");\n> -\treturn $sha1 eq $wt_sha1;\n> +\tmy $use = ($sha1 eq $null_sha1) || ($sha1 eq $wt_sha1);\n> +\treturn ($use, $wt_sha1);\n> +}\n> +\n> +sub changed_files\n> +{\n> +\tmy ($repo_path, $index, $worktree) = @_;\n> +\t$ENV{GIT_INDEX_FILE} = $index;\n> +\t$ENV{GIT_WORK_TREE} = $worktree;\n> +\tmy $must_unset_git_dir = 0;\n> +\tif (not defined($ENV{GIT_DIR})) {\n> +\t\t$must_unset_git_dir = 1;\n> +\t\t$ENV{GIT_DIR} = $repo_path;\n> +\t}\n> +\n> +\tmy @refreshargs = qw/update-index --really-refresh -q --unmerged/;\n> +\tmy @gitargs = qw/diff-files --name-only -z/;\n> +\ttry {\n> +\t\tGit::command_oneline(@refreshargs);\n> +\t} catch Git::Error::Command with {};\n> +\n> +\tmy $line = Git::command_oneline(@gitargs);\n> +\tmy @files;\n> +\tif (defined $line) {\n> +\t\t@files = split('\\0', $line);\n> +\t} else {\n> +\t\t@files = ();\n> +\t}\n> +\n> +\tdelete($ENV{GIT_INDEX_FILE});\n> +\tdelete($ENV{GIT_WORK_TREE});\n> +\tdelete($ENV{GIT_DIR}) if ($must_unset_git_dir);\n> +\n> +\treturn map { $_ => 1 } @files;\n> }\n> \n> sub setup_dir_diff\n> @@ -121,6 +152,7 @@ sub setup_dir_diff\n> \tmy $null_sha1 = '0' x 40;\n> \tmy $lindex = '';\n> \tmy $rindex = '';\n> +\tmy $wtindex = '';\n> \tmy %submodule;\n> \tmy %symlink;\n> \tmy @working_tree = ();\n> @@ -174,8 +206,12 @@ EOF\n> \t\t}\n> \n> \t\tif ($rmode ne $null_mode) {\n> -\t\t\tif (use_wt_file($repo, $workdir, $dst_path, $rsha1, $symlinks)) {\n> -\t\t\t\tpush(@working_tree, $dst_path);\n> +\t\t\tmy ($use, $wt_sha1) = use_wt_file($repo, $workdir,\n> +\t\t\t\t\t\t\t $dst_path, $rsha1,\n> +\t\t\t\t\t\t\t $symlinks);\n> +\t\t\tif ($use) {\n> +\t\t\t\tpush @working_tree, $dst_path;\n> +\t\t\t\t$wtindex .= \"$rmode $wt_sha1\\t$dst_path\\0\";\n> \t\t\t} else {\n> \t\t\t\t$rindex .= \"$rmode $rsha1\\t$dst_path\\0\";\n> \t\t\t}\n> @@ -218,6 +254,12 @@ EOF\n> \t$rc = system('git', 'checkout-index', '--all', \"--prefix=$rdir/\");\n> \texit_cleanup($tmpdir, $rc) if $rc != 0;\n> \n> +\t$ENV{GIT_INDEX_FILE} = \"$tmpdir/wtindex\";\n> +\t($inpipe, $ctx) =\n> +\t\t$repo->command_input_pipe(qw(update-index --info-only -z --index-info));\n> +\tprint($inpipe $wtindex);\n> +\t$repo->command_close_pipe($inpipe, $ctx);\n> +\n> \t# If $GIT_DIR was explicitly set just for the update/checkout\n> \t# commands, then it should be unset before continuing.\n> \tdelete($ENV{GIT_DIR}) if ($must_unset_git_dir);\n> @@ -390,19 +432,22 @@ sub dir_diff\n> \t# should be copied back to the working tree.\n> \t# Do not copy back files when symlinks are used and the\n> \t# external tool did not replace the original link with a file.\n> +\tmy %wt_modified = changed_files($repo->repo_path(),\n> +\t\t\"$tmpdir/wtindex\", \"$workdir\");\n> +\tmy %tmp_modified = changed_files($repo->repo_path(),\n> +\t\t\"$tmpdir/wtindex\", \"$b\");\n> \tfor my $file (@worktree) {\n> \t\tnext if $symlinks && -l \"$b/$file\";\n> \t\tnext if ! -f \"$b/$file\";\n> \n> -\t\tmy $diff = compare(\"$b/$file\", \"$workdir/$file\");\n> -\t\tif ($diff == 0) {\n> -\t\t\tnext;\n> -\t\t} elsif ($diff == -1) {\n> -\t\t\tmy $errmsg = \"warning: Could not compare \";\n> -\t\t\t$errmsg += \"'$b/$file' with '$workdir/$file'\\n\";\n> +\t\tif (exists $wt_modified{$file} and exists $tmp_modified{$file}) {\n> +\t\t\tmy $errmsg = \"warning: Both files modified: \";\n> +\t\t\t$errmsg .= \"'$workdir/$file' and '$b/$file'.\\n\";\n> +\t\t\t$errmsg .= \"warning: Working tree file has been left.\\n\";\n> +\t\t\t$errmsg .= \"warning:\\n\";\n> \t\t\twarn $errmsg;\n> \t\t\t$error = 1;\n> -\t\t} elsif ($diff == 1) {\n> +\t\t} elsif ($tmp_modified{$file}) {\n> \t\t\tmy $mode = stat(\"$b/$file\")->mode;\n> \t\t\tcopy(\"$b/$file\", \"$workdir/$file\") or\n> \t\t\texit_cleanup($tmpdir, 1);\n\nI don't have a lot to say about the patch text, except that there is\nnothing obvious out of the ordinary, but please take this with a large\ngrain of salt, as I'm lacking context. (It's the first time these days\nthat I'm looking at difftool.)\n\nBTW, did you know that perl is mostly a write-only language? ;-)\n\n> diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh\n> index db3d3d6..be2042d 100755\n> --- a/t/t7800-difftool.sh\n> +++ b/t/t7800-difftool.sh\n> @@ -407,4 +407,30 @@ test_expect_success PERL 'difftool --dir-diff from subdirectory' '\n> \t)\n> '\n> \n> +write_script modify-file <<\\EOF\n> +echo \"new content\" >file\n> +EOF\n> +\n> +test_expect_success PERL 'difftool --no-symlinks does not overwrite working tree file ' '\n> +\techo \"orig content\" >file &&\n> +\tgit difftool --dir-diff --no-symlinks --extcmd \"$(pwd)/modify-file\" branch &&\n> +\techo \"new content\" >expect &&\n> +\ttest_cmp expect file\n> +'\n> +\n> +write_script modify-both-files <<\\EOF\n> +echo \"wt content\" >file &&\n> +echo \"tmp content\" >\"$2/file\" &&\n> +echo \"$2\" >tmpdir\n> +EOF\n> +\n> +test_expect_success PERL 'difftool --no-symlinks detects conflict ' '\n> +\techo \"orig content\" >file &&\n> +\ttest_must_fail git difftool --dir-diff --no-symlinks --extcmd \"$(pwd)/modify-both-files\" branch &&\n> +\techo \"wt content\" >expect &&\n> +\ttest_cmp expect file &&\n> +\techo \"tmp content\" >expect &&\n> +\ttest_cmp expect \"$(cat tmpdir)/file\"\n> +'\n\nThe new tests look good.\n\nOne question though: Do I understand correctly that the temporary\ndirectories are leaked in the case of an \"edit conflict\"? If so, is it\nworth a warning for the user to clean up the garbage?\n\n-- Hannes\n"},{"id":"212272","messageId":"51516095.7030602@viscovery.net","threadId":"32954","inReplyTo":"51515E92.4030009@viscovery.net","subject":"Re: [PATCH v2] difftool: don't overwrite modified files","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2013-03-26T08:47:17Z","receivedAt":"2013-03-26T08:47:17Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Forgot to mention: The patch passes t7800 on Windows.\n\nThanks,\n-- Hannes\n"},{"id":"212274","messageId":"20130326092232.GH2286@serenity.lan","threadId":"32954","inReplyTo":"7vobe7w3m9.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2 3/3] t7800: run --dir-diff tests with and without symlinks","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2013-03-26T09:22:32Z","receivedAt":"2013-03-26T09:22:32Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"On Mon, Mar 25, 2013 at 02:50:38PM -0700, Junio C Hamano wrote:\n> Johannes Sixt writes:\n> \n> > Am 3/25/2013 11:35, schrieb John Keeping:\n> >> On Mon, Mar 25, 2013 at 08:26:52AM +0100, Johannes Sixt wrote:\n> >>> The series looks good, but I can't test it because it does not apply\n> >>> anywhere here.\n> >> \n> >> It's built on top of da/difftool-fixes, is there some problem that stops\n> >> it applying cleanly on top of that?\n> >\n> > Thanks. I had only tried trees that were \"contaminated\" by\n> > jk/difftool-dir-diff-edit-fix, which is in conflict with da/difftool-fixes.\n> \n> Yes, the conflict was unpleasant to deal with. John, I think what\n> is queued on 'pu' is OK, but please double check after I push it out\n> in a few hours.\n\nThe result looks good to me.\n"},{"id":"212275","messageId":"20130326093141.GI2286@serenity.lan","threadId":"32954","inReplyTo":"51515E92.4030009@viscovery.net","subject":"Re: [PATCH v2] difftool: don't overwrite modified files","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2013-03-26T09:31:41Z","receivedAt":"2013-03-26T09:31:41Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"On Tue, Mar 26, 2013 at 09:38:42AM +0100, Johannes Sixt wrote:\n> Am 3/25/2013 22:44, schrieb John Keeping:\n> > After running the user's diff tool, git-difftool will copy any files\n> > that differ between the working tree and the temporary tree. This is\n> > useful when the user edits the file in their diff tool but is wrong if\n> > they edit the working tree file while examining the diff.\n> > \n> > Instead of copying unconditionally when the files differ, create and\n> > index from the working tree files and only copy the temporary file back\n> > if it was modified and the working tree file was not. If both files\n> > have been modified, print a warning and exit with an error.\n> > \n> > Note that we cannot use an existing index in git-difftool since those\n> > contain the modified files that need to be checked out but here we are\n> > looking at those files which are copied from the working tree and not\n> > checked out. These are precisely the files which are not in the\n> > existing indices.\n> > \n> > Signed-off-by: John Keeping \n> > ---\n> > On Mon, Mar 25, 2013 at 10:42:19AM +0000, John Keeping wrote:\n> >> On Mon, Mar 25, 2013 at 08:41:59AM +0100, Johannes Sixt wrote:\n> >>> This is gross. Can't we do much better here? Difftool already keeps a\n> >>> GIT_INDEX of the files in the temporary tree ($tmpdir/rindex). Running\n> >>> git-diff-files should be sufficient to tell which ones where edited via\n> >>> the users's diff-tool. Then you can restrict calling hash-object to only\n> >>> those worktree files where an \"edit collision\" needs to be checked for.\n> >>\n> >> That's only the case for files that are not copied from the working\n> >> tree, so the temporary index doesn't contain the files that are of\n> >> interest here.\n> >>\n> >>> You could also keep a parallel index that keeps the state of the same set\n> >>> of files in the worktree. Then another git-diff-files call could replace\n> >>> the other half of hash-object calls.\n> >>\n> >> I like the idea of creating an index from the working tree files and\n> >> using it here. If we create a \"starting state\" index for these files,\n> >> we should be able to run git-diff-files against both the working tree\n> >> and the temporary tree at this point and compare the output.\n> > \n> > Here's an attempt at taking this approach, built on\n> > jk/difftool-dir-diff-edit-fix.\n> > \n> > git-difftool.perl | 73 +++++++++++++++++++++++++++++++++++++++++++----------\n> > t/t7800-difftool.sh | 26 +++++++++++++++++++\n> > 2 files changed, 85 insertions(+), 14 deletions(-)\n> > \n> > diff --git a/git-difftool.perl b/git-difftool.perl\n> > index c433e86..d10f7d2 100755\n> > --- a/git-difftool.perl\n> > +++ b/git-difftool.perl\n> > @@ -13,9 +13,9 @@\n> > use 5.008;\n> > use strict;\n> > use warnings;\n> > +use Error qw(:try);\n> > use File::Basename qw(dirname);\n> > use File::Copy;\n> > -use File::Compare;\n> > use File::Find;\n> > use File::stat;\n> > use File::Path qw(mkpath rmtree);\n> > @@ -88,14 +88,45 @@ sub use_wt_file\n> > \tmy ($repo, $workdir, $file, $sha1, $symlinks) = @_;\n> > \tmy $null_sha1 = '0' x 40;\n> > \n> > -\tif ($sha1 eq $null_sha1) {\n> > -\t\treturn 1;\n> > -\t} elsif (not $symlinks) {\n> > +\tif ($sha1 ne $null_sha1 and not $symlinks) {\n> > \t\treturn 0;\n> > \t}\n> > \n> > \tmy $wt_sha1 = $repo->command_oneline('hash-object', \"$workdir/$file\");\n> > -\treturn $sha1 eq $wt_sha1;\n> > +\tmy $use = ($sha1 eq $null_sha1) || ($sha1 eq $wt_sha1);\n> > +\treturn ($use, $wt_sha1);\n> > +}\n> > +\n> > +sub changed_files\n> > +{\n> > +\tmy ($repo_path, $index, $worktree) = @_;\n> > +\t$ENV{GIT_INDEX_FILE} = $index;\n> > +\t$ENV{GIT_WORK_TREE} = $worktree;\n> > +\tmy $must_unset_git_dir = 0;\n> > +\tif (not defined($ENV{GIT_DIR})) {\n> > +\t\t$must_unset_git_dir = 1;\n> > +\t\t$ENV{GIT_DIR} = $repo_path;\n> > +\t}\n> > +\n> > +\tmy @refreshargs = qw/update-index --really-refresh -q --unmerged/;\n> > +\tmy @gitargs = qw/diff-files --name-only -z/;\n> > +\ttry {\n> > +\t\tGit::command_oneline(@refreshargs);\n> > +\t} catch Git::Error::Command with {};\n> > +\n> > +\tmy $line = Git::command_oneline(@gitargs);\n> > +\tmy @files;\n> > +\tif (defined $line) {\n> > +\t\t@files = split('\\0', $line);\n> > +\t} else {\n> > +\t\t@files = ();\n> > +\t}\n> > +\n> > +\tdelete($ENV{GIT_INDEX_FILE});\n> > +\tdelete($ENV{GIT_WORK_TREE});\n> > +\tdelete($ENV{GIT_DIR}) if ($must_unset_git_dir);\n> > +\n> > +\treturn map { $_ => 1 } @files;\n> > }\n> > \n> > sub setup_dir_diff\n> > @@ -121,6 +152,7 @@ sub setup_dir_diff\n> > \tmy $null_sha1 = '0' x 40;\n> > \tmy $lindex = '';\n> > \tmy $rindex = '';\n> > +\tmy $wtindex = '';\n> > \tmy %submodule;\n> > \tmy %symlink;\n> > \tmy @working_tree = ();\n> > @@ -174,8 +206,12 @@ EOF\n> > \t\t}\n> > \n> > \t\tif ($rmode ne $null_mode) {\n> > -\t\t\tif (use_wt_file($repo, $workdir, $dst_path, $rsha1, $symlinks)) {\n> > -\t\t\t\tpush(@working_tree, $dst_path);\n> > +\t\t\tmy ($use, $wt_sha1) = use_wt_file($repo, $workdir,\n> > +\t\t\t\t\t\t\t $dst_path, $rsha1,\n> > +\t\t\t\t\t\t\t $symlinks);\n> > +\t\t\tif ($use) {\n> > +\t\t\t\tpush @working_tree, $dst_path;\n> > +\t\t\t\t$wtindex .= \"$rmode $wt_sha1\\t$dst_path\\0\";\n> > \t\t\t} else {\n> > \t\t\t\t$rindex .= \"$rmode $rsha1\\t$dst_path\\0\";\n> > \t\t\t}\n> > @@ -218,6 +254,12 @@ EOF\n> > \t$rc = system('git', 'checkout-index', '--all', \"--prefix=$rdir/\");\n> > \texit_cleanup($tmpdir, $rc) if $rc != 0;\n> > \n> > +\t$ENV{GIT_INDEX_FILE} = \"$tmpdir/wtindex\";\n> > +\t($inpipe, $ctx) =\n> > +\t\t$repo->command_input_pipe(qw(update-index --info-only -z --index-info));\n> > +\tprint($inpipe $wtindex);\n> > +\t$repo->command_close_pipe($inpipe, $ctx);\n> > +\n> > \t# If $GIT_DIR was explicitly set just for the update/checkout\n> > \t# commands, then it should be unset before continuing.\n> > \tdelete($ENV{GIT_DIR}) if ($must_unset_git_dir);\n> > @@ -390,19 +432,22 @@ sub dir_diff\n> > \t# should be copied back to the working tree.\n> > \t# Do not copy back files when symlinks are used and the\n> > \t# external tool did not replace the original link with a file.\n> > +\tmy %wt_modified = changed_files($repo->repo_path(),\n> > +\t\t\"$tmpdir/wtindex\", \"$workdir\");\n> > +\tmy %tmp_modified = changed_files($repo->repo_path(),\n> > +\t\t\"$tmpdir/wtindex\", \"$b\");\n> > \tfor my $file (@worktree) {\n> > \t\tnext if $symlinks && -l \"$b/$file\";\n> > \t\tnext if ! -f \"$b/$file\";\n> > \n> > -\t\tmy $diff = compare(\"$b/$file\", \"$workdir/$file\");\n> > -\t\tif ($diff == 0) {\n> > -\t\t\tnext;\n> > -\t\t} elsif ($diff == -1) {\n> > -\t\t\tmy $errmsg = \"warning: Could not compare \";\n> > -\t\t\t$errmsg += \"'$b/$file' with '$workdir/$file'\\n\";\n> > +\t\tif (exists $wt_modified{$file} and exists $tmp_modified{$file}) {\n> > +\t\t\tmy $errmsg = \"warning: Both files modified: \";\n> > +\t\t\t$errmsg .= \"'$workdir/$file' and '$b/$file'.\\n\";\n> > +\t\t\t$errmsg .= \"warning: Working tree file has been left.\\n\";\n> > +\t\t\t$errmsg .= \"warning:\\n\";\n> > \t\t\twarn $errmsg;\n> > \t\t\t$error = 1;\n> > -\t\t} elsif ($diff == 1) {\n> > +\t\t} elsif ($tmp_modified{$file}) {\n> > \t\t\tmy $mode = stat(\"$b/$file\")->mode;\n> > \t\t\tcopy(\"$b/$file\", \"$workdir/$file\") or\n> > \t\t\texit_cleanup($tmpdir, 1);\n> \n> I don't have a lot to say about the patch text, except that there is\n> nothing obvious out of the ordinary, but please take this with a large\n> grain of salt, as I'm lacking context. (It's the first time these days\n> that I'm looking at difftool.)\n> \n> BTW, did you know that perl is mostly a write-only language? ;-)\n> \n> > diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh\n> > index db3d3d6..be2042d 100755\n> > --- a/t/t7800-difftool.sh\n> > +++ b/t/t7800-difftool.sh\n> > @@ -407,4 +407,30 @@ test_expect_success PERL 'difftool --dir-diff from subdirectory' '\n> > \t)\n> > '\n> > \n> > +write_script modify-file <<\\EOF\n> > +echo \"new content\" >file\n> > +EOF\n> > +\n> > +test_expect_success PERL 'difftool --no-symlinks does not overwrite working tree file ' '\n> > +\techo \"orig content\" >file &&\n> > +\tgit difftool --dir-diff --no-symlinks --extcmd \"$(pwd)/modify-file\" branch &&\n> > +\techo \"new content\" >expect &&\n> > +\ttest_cmp expect file\n> > +'\n> > +\n> > +write_script modify-both-files <<\\EOF\n> > +echo \"wt content\" >file &&\n> > +echo \"tmp content\" >\"$2/file\" &&\n> > +echo \"$2\" >tmpdir\n> > +EOF\n> > +\n> > +test_expect_success PERL 'difftool --no-symlinks detects conflict ' '\n> > +\techo \"orig content\" >file &&\n> > +\ttest_must_fail git difftool --dir-diff --no-symlinks --extcmd \"$(pwd)/modify-both-files\" branch &&\n> > +\techo \"wt content\" >expect &&\n> > +\ttest_cmp expect file &&\n> > +\techo \"tmp content\" >expect &&\n> > +\ttest_cmp expect \"$(cat tmpdir)/file\"\n> > +'\n> \n> The new tests look good.\n> \n> One question though: Do I understand correctly that the temporary\n> directories are leaked in the case of an \"edit conflict\"? If so, is it\n> worth a warning for the user to clean up the garbage?\n\nDo you mean for normal users or for those running the tests? In normal\nusage we do print a warning - it's in the existing code, triggered by\nsetting \"$error = 1\" - you can see that if you run the tests with \"-v\".\n\nThe last test does result in /tmp filling up with temporary directories\nthough, it would be good if the test could clean up after itself. The\nbest I can come up with is adding something like this immediately after\nrunning difftool but I'm not entirely happy with the \"..\" in the\nargument to rm:\n\n\ttest_when_finished rm -rf \"$(cat tmpdir)/..\"\n"},{"id":"212277","messageId":"5151702C.9060502@viscovery.net","threadId":"32954","inReplyTo":"20130326093141.GI2286@serenity.lan","subject":"Re: [PATCH v2] difftool: don't overwrite modified files","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2013-03-26T09:53:48Z","receivedAt":"2013-03-26T09:53:48Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 3/26/2013 10:31, schrieb John Keeping:\n> On Tue, Mar 26, 2013 at 09:38:42AM +0100, Johannes Sixt wrote:\n>> One question though: Do I understand correctly that the temporary\n>> directories are leaked in the case of an \"edit conflict\"? If so, is it\n>> worth a warning for the user to clean up the garbage?\n> \n> Do you mean for normal users or for those running the tests? In normal\n> usage we do print a warning - it's in the existing code, triggered by\n> setting \"$error = 1\" - you can see that if you run the tests with \"-v\".\n\nI meant for normal users. I see the error now. Thanks.\n\n> The last test does result in /tmp filling up with temporary directories\n> though, it would be good if the test could clean up after itself. The\n> best I can come up with is adding something like this immediately after\n> running difftool but I'm not entirely happy with the \"..\" in the\n> argument to rm:\n> \n> \ttest_when_finished rm -rf \"$(cat tmpdir)/..\"\n\nWrap the test in\n\n\t(\n\t\tTMPDIR=$TRASH_DIRECTORY &&\n\t\texport TMPDIR &&\n\t...\n\t)\n\nIt works for me.\n\n-- Hannes\n"},{"id":"212319","messageId":"20130326193419.GK2286@serenity.lan","threadId":"32954","inReplyTo":"5151702C.9060502@viscovery.net","subject":"Re: [PATCH v2] difftool: don't overwrite modified files","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2013-03-26T19:34:19Z","receivedAt":"2013-03-26T19:34:19Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"On Tue, Mar 26, 2013 at 10:53:48AM +0100, Johannes Sixt wrote:\n> Am 3/26/2013 10:31, schrieb John Keeping:\n> > On Tue, Mar 26, 2013 at 09:38:42AM +0100, Johannes Sixt wrote:\n> > The last test does result in /tmp filling up with temporary directories\n> > though, it would be good if the test could clean up after itself. The\n> > best I can come up with is adding something like this immediately after\n> > running difftool but I'm not entirely happy with the \"..\" in the\n> > argument to rm:\n> > \n> > \ttest_when_finished rm -rf \"$(cat tmpdir)/..\"\n> \n> Wrap the test in\n> \n> \t(\n> \t\tTMPDIR=$TRASH_DIRECTORY &&\n> \t\texport TMPDIR &&\n> \t...\n> \t)\n> \n> It works for me.\n\nNice. I've reviewed File::Spec and it looks like that TMPDIR takes\npriority on every operating system except VMS, and I don't think we care\nabout that.\n\nUnless Junio says otherwise, I'll hold off sending this until difftool\ncalms down a bit to avoid too many conflicted merges.\n"},{"id":"212333","messageId":"CAJELnLFxEa6v39ocVD+VLj9b86HhURsnFa+UKc+r3xYPLStSNw@mail.gmail.com","threadId":"32954","inReplyTo":"20130325214430.GG2286@serenity.lan","subject":"Re: [PATCH v2] difftool: don't overwrite modified files","fromName":"Matt McClure","fromEmail":"matthewlmcclure@gmail.com","sentAt":"2013-03-26T20:52:02Z","receivedAt":"2013-03-26T20:52:02Z","isPatch":true,"sender":{"key":"matthewlmcclure@gmail.com","avatar":"https://gravatar.com/avatar/a8cde96b0594204c8d4c48baa51c983af37dedfa2b7a846e27547b8d63d46e7c?d=mp&s=160"},"body":"On Mon, Mar 25, 2013 at 5:44 PM, John Keeping wrote:\n> Instead of copying unconditionally when the files differ, create and\n> index from the working tree files and only copy the temporary file back\n> if it was modified and the working tree file was not. If both files\n> have been modified, print a warning and exit with an error.\n\nWhen there's a conflict, does difftool save both conflicting files? Or\nonly the working tree copy? I think it should preserve both copies on\ndisk.\n\n-- \nMatt McClure\nhttp://www.matthewlmcclure.com\nhttp://www.mapmyfitness.com/profile/matthewlmcclure\n"},{"id":"212334","messageId":"20130326210141.GL2286@serenity.lan","threadId":"32954","inReplyTo":"CAJELnLFxEa6v39ocVD+VLj9b86HhURsnFa+UKc+r3xYPLStSNw@mail.gmail.com","subject":"Re: [PATCH v2] difftool: don't overwrite modified files","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2013-03-26T21:01:42Z","receivedAt":"2013-03-26T21:01:42Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"On Tue, Mar 26, 2013 at 04:52:02PM -0400, Matt McClure wrote:\n> On Mon, Mar 25, 2013 at 5:44 PM, John Keeping wrote:\n> > Instead of copying unconditionally when the files differ, create and\n> > index from the working tree files and only copy the temporary file back\n> > if it was modified and the working tree file was not. If both files\n> > have been modified, print a warning and exit with an error.\n> \n> When there's a conflict, does difftool save both conflicting files? Or\n> only the working tree copy? I think it should preserve both copies on\n> disk.\n\nIt preserves both copies - the \"clean the temporary directory\" step is\njust skipped.\n\nThis isn't ideal since the temporary copy will be under a temporary\ndirectory somewhere but is better than the current behaviour. It might\nbe nice to move the temporary file back with an extension so that the\nfiles are at least near each other but I don't think that's needed in\nthe first version of this change.\n"}]}