{"thread":{"id":"33323","subject":"[PATCH 0/5] difftool improvements","startedAt":"2013-03-29T11:28:31Z","lastAt":"2013-03-29T22:07:39Z","messageCount":9,"participants":["John Keeping","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":5},"messages":[{"id":"212609","messageId":"cover.1364555970.git.john@keeping.me.uk","threadId":"33323","inReplyTo":null,"subject":"[PATCH 0/5] difftool improvements","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2013-03-29T11:28:31Z","receivedAt":"2013-03-29T11:28:31Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"This is a consolidated series replacing jk/t7800-modernize.  The patches\nhere have been rebased on master.\n\nPatch 1 is new and moves the test added by commit 02c5631 (difftool\n--dir-diff: symlink all files matching the working tree) to the end of\nthe test file.  This means that once this is applied the second patch\nand the final three patches can become independent branches.\n\nPatch 2 is a fix for a long standing deficiency, but the potential for\nthis to happen has been increased by the commit mentioned above.  It has\nalready been through a couple of iterations [1].\n\nThe final three patches are the same as the current jk/t7800-modernize,\nwith one tweaked commit message.\n\n[1] http://article.gmane.org/gmane.comp.version-control.git/219100\n\nJohn Keeping (5):\n  t7800: move '--symlinks' specific test to the end\n  difftool: don't overwrite modified files\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 git-difftool.perl   |  73 +++++++++++++++++++++++++++++-------\n t/t7800-difftool.sh | 105 ++++++++++++++++++++++++++++++++++------------------\n 2 files changed, 127 insertions(+), 51 deletions(-)\n\n-- \n1.8.2.411.g65a544e\n"},{"id":"212610","messageId":"279dbdc3072772961ce28a90ffa0a7d8df017ffe.1364555970.git.john@keeping.me.uk","threadId":"33323","inReplyTo":"cover.1364555970.git.john@keeping.me.uk","subject":"[PATCH 1/5] t7800: move '--symlinks' specific test to the end","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2013-03-29T11:28:32Z","receivedAt":"2013-03-29T11:28:32Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"This will group the tests more logically when we introduce a helper to\nrun most --dir-diff tests with both --symlinks and --no-symlinks.\n\nSigned-off-by: John Keeping <john@keeping.me.uk>\n---\n t/t7800-difftool.sh | 30 +++++++++++++++---------------\n 1 file changed, 15 insertions(+), 15 deletions(-)\n\ndiff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh\nindex c6d6b1c..e6a16cd 100755\n--- a/t/t7800-difftool.sh\n+++ b/t/t7800-difftool.sh\n@@ -340,6 +340,21 @@ test_expect_success PERL 'difftool --dir-diff' '\n \tstdin_contains 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+\tstdin_contains sub <output &&\n+\tstdin_contains file <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\tstdin_contains sub <output &&\n+\t\tstdin_contains file <output\n+\t)\n+'\n+\n write_script .git/CHECK_SYMLINKS <<\\EOF\n for f in file file2 sub/sub\n do\n@@ -362,19 +377,4 @@ 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-\tstdin_contains sub <output &&\n-\tstdin_contains file <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\tstdin_contains sub <output &&\n-\t\tstdin_contains file <output\n-\t)\n-'\n-\n test_done\n-- \n1.8.2.411.g65a544e\n"},{"id":"212611","messageId":"8028ebdca0a219b44123a813d6bd8b95d076f2e9.1364555970.git.john@keeping.me.uk","threadId":"33323","inReplyTo":"cover.1364555970.git.john@keeping.me.uk","subject":"[PATCH 2/5] difftool: don't overwrite modified files","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2013-03-29T11:28:33Z","receivedAt":"2013-03-29T11:28:33Z","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 <john@keeping.me.uk>\n\n---\nChanges since v2:\n- Set TMPDIR to $TRASH_DIRECTORY in the test where difftool fails\n\n git-difftool.perl   | 73 +++++++++++++++++++++++++++++++++++++++++++----------\n t/t7800-difftool.sh | 30 ++++++++++++++++++++++\n 2 files changed, 89 insertions(+), 14 deletions(-)\n\ndiff --git a/git-difftool.perl b/git-difftool.perl\nindex 663640d..844f619 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 e6a16cd..017f55a 100755\n--- a/t/t7800-difftool.sh\n+++ b/t/t7800-difftool.sh\n@@ -377,4 +377,34 @@ test_expect_success PERL,SYMLINKS 'difftool --dir-diff --symlink without unstage\n \ttest_cmp actual expect\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+\t(\n+\t\tTMPDIR=$TRASH_DIRECTORY &&\n+\t\texport TMPDIR &&\n+\t\techo \"orig content\" >file &&\n+\t\ttest_must_fail git difftool --dir-diff --no-symlinks --extcmd \"$(pwd)/modify-both-files\" branch &&\n+\t\techo \"wt content\" >expect &&\n+\t\ttest_cmp expect file &&\n+\t\techo \"tmp content\" >expect &&\n+\t\ttest_cmp expect \"$(cat tmpdir)/file\"\n+\t)\n+'\n+\n test_done\n-- \n1.8.2.411.g65a544e\n"},{"id":"212612","messageId":"e6195818a21a6712098ca6042a4c05a66ecac98e.1364555970.git.john@keeping.me.uk","threadId":"33323","inReplyTo":"cover.1364555970.git.john@keeping.me.uk","subject":"[PATCH 3/5] t7800: don't hide grep output","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2013-03-29T11:28:34Z","receivedAt":"2013-03-29T11:28:34Z","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 <j.sixt@viscovery.net>\nSigned-off-by: John Keeping <john@keeping.me.uk>\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 017f55a..9fd09db 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 <input >output &&\n-\tstdin_contains m2 <output &&\n-\tstdin_contains br2 <output &&\n-\tstdin_doesnot_contain master <output &&\n-\tstdin_doesnot_contain branch <output\n+\tgrep m2 output &&\n+\tgrep br2 output &&\n+\t! grep master output &&\n+\t! grep branch output\n '\n \n test_expect_success PERL 'say no to the second file' '\n \t(echo && echo n) >input &&\n \tgit difftool -x cat branch <input >output &&\n-\tstdin_contains master <output &&\n-\tstdin_contains branch  <output &&\n-\tstdin_doesnot_contain m2 <output &&\n-\tstdin_doesnot_contain br2 <output\n+\tgrep master output &&\n+\tgrep branch output &&\n+\t! grep m2 output &&\n+\t! grep br2 output\n '\n \n test_expect_success PERL 'difftool --tool-help' '\n \tgit difftool --tool-help >output &&\n-\tstdin_contains tool <output\n+\tgrep tool output\n '\n \n test_expect_success PERL 'setup change in subdirectory' '\n@@ -330,28 +320,28 @@ test_expect_success PERL 'setup change in subdirectory' '\n \n test_expect_success PERL 'difftool -d' '\n \tgit difftool -d --extcmd ls branch >output &&\n-\tstdin_contains sub <output &&\n-\tstdin_contains file <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-\tstdin_contains sub <output &&\n-\tstdin_contains file <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-\tstdin_contains sub <output &&\n-\tstdin_contains file <output\n+\tgrep sub output &&\n+\tgrep file 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\tstdin_contains sub <output &&\n-\t\tstdin_contains file <output\n+\t\tgrep sub output &&\n+\t\tgrep file output\n \t)\n '\n \n-- \n1.8.2.411.g65a544e\n"},{"id":"212613","messageId":"7c54c9df46d8c0bcd0290c4c307001b36421fcb3.1364555970.git.john@keeping.me.uk","threadId":"33323","inReplyTo":"cover.1364555970.git.john@keeping.me.uk","subject":"[PATCH 4/5] t7800: fix tests when difftool uses --no-symlinks","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2013-03-29T11:28:35Z","receivedAt":"2013-03-29T11:28:35Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"When 'git difftool --dir-diff' is using --no-symlinks (either explicitly\nor implicitly because it's running on Windows), any working tree files\nthat have been copied to the temporary directory are copied back after\nthe difftool completes.\n\nBecause an earlier test uses \"git add .\", the \"output\" file used by\ntests is tracked by Git and the following sequence occurs during some\ntests:\n\n1) the shell opens \"output\" to redirect the difftool output\n2) difftool copies the empty \"output\" to the temporary directory\n3) difftool runs \"ls\" which writes to \"output\"\n4) difftool copies the empty \"output\" file back over the output of the\n   command\n5) the output file doesn't contain the expected output, causing the\n   test to fail\n\nInstead of adding all changes, explicitly add only the files that the\ntest is using, allowing later tests to write their result files into the\nworking tree.\n\nSigned-off-by: John Keeping <john@keeping.me.uk>\n\n---\nChanges since v2:\n- Fix grammar issues in commit message\n- Remove the final paragraph of the commit message since this is now\n  addressed by patch 2/5 in this series\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 9fd09db..df443a9 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.411.g65a544e\n"},{"id":"212614","messageId":"d5998552e411600b51f86155aa61b8491b3d6fcb.1364555970.git.john@keeping.me.uk","threadId":"33323","inReplyTo":"cover.1364555970.git.john@keeping.me.uk","subject":"[PATCH 5/5] t7800: run --dir-diff tests with and without symlinks","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2013-03-29T11:28:36Z","receivedAt":"2013-03-29T11:28:36Z","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 <john@keeping.me.uk>\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 df443a9..a6bd99e 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.411.g65a544e\n"},{"id":"212646","messageId":"7v38venejh.fsf@alter.siamese.dyndns.org","threadId":"33323","inReplyTo":"8028ebdca0a219b44123a813d6bd8b95d076f2e9.1364555970.git.john@keeping.me.uk","subject":"Re: [PATCH 2/5] difftool: don't overwrite modified files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-03-29T20:20:50Z","receivedAt":"2013-03-29T20:20:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"John Keeping <john@keeping.me.uk> writes:\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\nThanks.\n\nI should drop the t7800-modernize topic and queue this under a\nbetter name.  Perhaps \"difftool-no-overwrite-on-copyback\", or\nsomething.\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\n>\n> Signed-off-by: John Keeping <john@keeping.me.uk>\n>\n> ---\n> Changes since v2:\n> - Set TMPDIR to $TRASH_DIRECTORY in the test where difftool fails\n>\n>  git-difftool.perl   | 73 +++++++++++++++++++++++++++++++++++++++++++----------\n>  t/t7800-difftool.sh | 30 ++++++++++++++++++++++\n>  2 files changed, 89 insertions(+), 14 deletions(-)\n>\n> diff --git a/git-difftool.perl b/git-difftool.perl\n> index 663640d..844f619 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\nDo we need to run these two comparisons unconditionally?\n\nIt appears to me that in a sane and safe setting (i.e. $symlinks is\nset and the \"diff viewer\" touches the file through the symbolic\nlink) does not ever look at wt_modified/tmp_modified at all because\nthe first check in the loop will always trigger.\n\nI wonder if it makes sense to populate these two hashes lazily so\nthat the safe case does not have to pay the penalty at all.\n\n>  \tfor my $file (@worktree) {\n>  \t\tnext if $symlinks && -l \"$b/$file\";\n>  \t\tnext if ! -f \"$b/$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 ($tmp_modified{$file}) {\n\n\"exists\" if only for consistency?\n"},{"id":"212652","messageId":"20130329213802.GY2286@serenity.lan","threadId":"33323","inReplyTo":"7v38venejh.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 2/5] difftool: don't overwrite modified files","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2013-03-29T21:38:02Z","receivedAt":"2013-03-29T21:38:02Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"On Fri, Mar 29, 2013 at 01:20:50PM -0700, Junio C Hamano wrote:\n> John Keeping <john@keeping.me.uk> writes:\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> Thanks.\n> \n> I should drop the t7800-modernize topic and queue this under a\n> better name.  Perhaps \"difftool-no-overwrite-on-copyback\", or\n> something.\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> \n> >\n> > Signed-off-by: John Keeping <john@keeping.me.uk>\n> >\n> > ---\n> > Changes since v2:\n> > - Set TMPDIR to $TRASH_DIRECTORY in the test where difftool fails\n> >\n> >  git-difftool.perl   | 73 +++++++++++++++++++++++++++++++++++++++++++----------\n> >  t/t7800-difftool.sh | 30 ++++++++++++++++++++++\n> >  2 files changed, 89 insertions(+), 14 deletions(-)\n> >\n> > diff --git a/git-difftool.perl b/git-difftool.perl\n> > index 663640d..844f619 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> \n> Do we need to run these two comparisons unconditionally?\n> \n> It appears to me that in a sane and safe setting (i.e. $symlinks is\n> set and the \"diff viewer\" touches the file through the symbolic\n> link) does not ever look at wt_modified/tmp_modified at all because\n> the first check in the loop will always trigger.\n> \n> I wonder if it makes sense to populate these two hashes lazily so\n> that the safe case does not have to pay the penalty at all.\n\nSounds sensible.  I'll look into doing this.\n\n> >  \tfor my $file (@worktree) {\n> >  \t\tnext if $symlinks && -l \"$b/$file\";\n> >  \t\tnext if ! -f \"$b/$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 ($tmp_modified{$file}) {\n> \n> \"exists\" if only for consistency?\n\nNot just for consistency - this will generate an uninitialized value\nwarning if the tmp version of the file hasn't been modified.\n"},{"id":"212653","messageId":"20130329220738.GZ2286@serenity.lan","threadId":"33323","inReplyTo":"20130329213802.GY2286@serenity.lan","subject":"[PATCH 2/5 v2] difftool: don't overwrite modified files","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2013-03-29T22:07:39Z","receivedAt":"2013-03-29T22:07:39Z","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 <john@keeping.me.uk>\n\n---\nChanges since v1:\n- Lazily initialize \"*_modified\" hashes to avoid executing Git commands\n  in the common case where symlinks are in use and the diff tool writes\n  to the file through the symlink\n- Use exists consistently when checking \"*_modified\" hashes\n\n git-difftool.perl   | 85 ++++++++++++++++++++++++++++++++++++++++++++---------\n t/t7800-difftool.sh | 30 +++++++++++++++++++\n 2 files changed, 101 insertions(+), 14 deletions(-)\n\ndiff --git a/git-difftool.perl b/git-difftool.perl\nindex 663640d..6780292 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,34 @@ 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+\t#\n+\t# These hashes are loaded lazily since they aren't needed\n+\t# in the common case of --symlinks and the difftool updating\n+\t# files through the symlink.\n+\tmy %wt_modified;\n+\tmy %tmp_modified;\n+\tmy $indices_loaded = 0;\n+\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 (!$indices_loaded) {\n+\t\t\t%wt_modified = changed_files($repo->repo_path(),\n+\t\t\t\t\"$tmpdir/wtindex\", \"$workdir\");\n+\t\t\t%tmp_modified = changed_files($repo->repo_path(),\n+\t\t\t\t\"$tmpdir/wtindex\", \"$b\");\n+\t\t\t$indices_loaded = 1;\n+\t\t}\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 (exists $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 e6a16cd..017f55a 100755\n--- a/t/t7800-difftool.sh\n+++ b/t/t7800-difftool.sh\n@@ -377,4 +377,34 @@ test_expect_success PERL,SYMLINKS 'difftool --dir-diff --symlink without unstage\n \ttest_cmp actual expect\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+\t(\n+\t\tTMPDIR=$TRASH_DIRECTORY &&\n+\t\texport TMPDIR &&\n+\t\techo \"orig content\" >file &&\n+\t\ttest_must_fail git difftool --dir-diff --no-symlinks --extcmd \"$(pwd)/modify-both-files\" branch &&\n+\t\techo \"wt content\" >expect &&\n+\t\ttest_cmp expect file &&\n+\t\techo \"tmp content\" >expect &&\n+\t\ttest_cmp expect \"$(cat tmpdir)/file\"\n+\t)\n+'\n+\n test_done\n-- \n1.8.2.411.g65a544e\n"}]}