difftool: don't overwrite modified files

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

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

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

Signed-off-by: John Keeping <john@keeping.me.uk>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/git-difftool.perl b/git-difftool.perl
index 663640d33cb99a98135b38b95947416b4a8d49b9..67802922ccc41fa2993c1bce4ea1a3d2899c7a40 100755 (executable)
--- a/git-difftool.perl
+++ b/git-difftool.perl
@@ -13,9 +13,9 @@
 use 5.008;
 use strict;
 use warnings;
+use Error qw(:try);
 use File::Basename qw(dirname);
 use File::Copy;
-use File::Compare;
 use File::Find;
 use File::stat;
 use File::Path qw(mkpath rmtree);
@@ -88,14 +88,45 @@ sub use_wt_file
        my ($repo, $workdir, $file, $sha1, $symlinks) = @_;
        my $null_sha1 = '0' x 40;
 
-       if ($sha1 eq $null_sha1) {
-               return 1;
-       } elsif (not $symlinks) {
+       if ($sha1 ne $null_sha1 and not $symlinks) {
                return 0;
        }
 
        my $wt_sha1 = $repo->command_oneline('hash-object', "$workdir/$file");
-       return $sha1 eq $wt_sha1;
+       my $use = ($sha1 eq $null_sha1) || ($sha1 eq $wt_sha1);
+       return ($use, $wt_sha1);
+}
+
+sub changed_files
+{
+       my ($repo_path, $index, $worktree) = @_;
+       $ENV{GIT_INDEX_FILE} = $index;
+       $ENV{GIT_WORK_TREE} = $worktree;
+       my $must_unset_git_dir = 0;
+       if (not defined($ENV{GIT_DIR})) {
+               $must_unset_git_dir = 1;
+               $ENV{GIT_DIR} = $repo_path;
+       }
+
+       my @refreshargs = qw/update-index --really-refresh -q --unmerged/;
+       my @gitargs = qw/diff-files --name-only -z/;
+       try {
+               Git::command_oneline(@refreshargs);
+       } catch Git::Error::Command with {};
+
+       my $line = Git::command_oneline(@gitargs);
+       my @files;
+       if (defined $line) {
+               @files = split('\0', $line);
+       } else {
+               @files = ();
+       }
+
+       delete($ENV{GIT_INDEX_FILE});
+       delete($ENV{GIT_WORK_TREE});
+       delete($ENV{GIT_DIR}) if ($must_unset_git_dir);
+
+       return map { $_ => 1 } @files;
 }
 
 sub setup_dir_diff
@@ -121,6 +152,7 @@ sub setup_dir_diff
        my $null_sha1 = '0' x 40;
        my $lindex = '';
        my $rindex = '';
+       my $wtindex = '';
        my %submodule;
        my %symlink;
        my @working_tree = ();
@@ -174,8 +206,12 @@ sub setup_dir_diff
                }
 
                if ($rmode ne $null_mode) {
-                       if (use_wt_file($repo, $workdir, $dst_path, $rsha1, $symlinks)) {
-                               push(@working_tree, $dst_path);
+                       my ($use, $wt_sha1) = use_wt_file($repo, $workdir,
+                                                         $dst_path, $rsha1,
+                                                         $symlinks);
+                       if ($use) {
+                               push @working_tree, $dst_path;
+                               $wtindex .= "$rmode $wt_sha1\t$dst_path\0";
                        } else {
                                $rindex .= "$rmode $rsha1\t$dst_path\0";
                        }
@@ -218,6 +254,12 @@ sub setup_dir_diff
        $rc = system('git', 'checkout-index', '--all', "--prefix=$rdir/");
        exit_cleanup($tmpdir, $rc) if $rc != 0;
 
+       $ENV{GIT_INDEX_FILE} = "$tmpdir/wtindex";
+       ($inpipe, $ctx) =
+               $repo->command_input_pipe(qw(update-index --info-only -z --index-info));
+       print($inpipe $wtindex);
+       $repo->command_close_pipe($inpipe, $ctx);
+
        # If $GIT_DIR was explicitly set just for the update/checkout
        # commands, then it should be unset before continuing.
        delete($ENV{GIT_DIR}) if ($must_unset_git_dir);
@@ -390,19 +432,34 @@ sub dir_diff
        # should be copied back to the working tree.
        # Do not copy back files when symlinks are used and the
        # external tool did not replace the original link with a file.
+       #
+       # These hashes are loaded lazily since they aren't needed
+       # in the common case of --symlinks and the difftool updating
+       # files through the symlink.
+       my %wt_modified;
+       my %tmp_modified;
+       my $indices_loaded = 0;
+
        for my $file (@worktree) {
                next if $symlinks && -l "$b/$file";
                next if ! -f "$b/$file";
 
-               my $diff = compare("$b/$file", "$workdir/$file");
-               if ($diff == 0) {
-                       next;
-               } elsif ($diff == -1) {
-                       my $errmsg = "warning: Could not compare ";
-                       $errmsg += "'$b/$file' with '$workdir/$file'\n";
+               if (!$indices_loaded) {
+                       %wt_modified = changed_files($repo->repo_path(),
+                               "$tmpdir/wtindex", "$workdir");
+                       %tmp_modified = changed_files($repo->repo_path(),
+                               "$tmpdir/wtindex", "$b");
+                       $indices_loaded = 1;
+               }
+
+               if (exists $wt_modified{$file} and exists $tmp_modified{$file}) {
+                       my $errmsg = "warning: Both files modified: ";
+                       $errmsg .= "'$workdir/$file' and '$b/$file'.\n";
+                       $errmsg .= "warning: Working tree file has been left.\n";
+                       $errmsg .= "warning:\n";
                        warn $errmsg;
                        $error = 1;
-               } elsif ($diff == 1) {
+               } elsif (exists $tmp_modified{$file}) {
                        my $mode = stat("$b/$file")->mode;
                        copy("$b/$file", "$workdir/$file") or
                        exit_cleanup($tmpdir, 1);

diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
index e6a16cda79aba4794386b5f11fff61a429c9a3a2..017f55adf4d88c3e550812388d29b7a9a4d9db3d 100755 (executable)
--- a/t/t7800-difftool.sh
+++ b/t/t7800-difftool.sh
@@ -377,4 +377,34 @@ test_expect_success PERL,SYMLINKS 'difftool --dir-diff --symlink without unstage
        test_cmp actual expect
 '
 
+write_script modify-file <<\EOF
+echo "new content" >file
+EOF
+
+test_expect_success PERL 'difftool --no-symlinks does not overwrite working tree file ' '
+       echo "orig content" >file &&
+       git difftool --dir-diff --no-symlinks --extcmd "$(pwd)/modify-file" branch &&
+       echo "new content" >expect &&
+       test_cmp expect file
+'
+
+write_script modify-both-files <<\EOF
+echo "wt content" >file &&
+echo "tmp content" >"$2/file" &&
+echo "$2" >tmpdir
+EOF
+
+test_expect_success PERL 'difftool --no-symlinks detects conflict ' '
+       (
+               TMPDIR=$TRASH_DIRECTORY &&
+               export TMPDIR &&
+               echo "orig content" >file &&
+               test_must_fail git difftool --dir-diff --no-symlinks --extcmd "$(pwd)/modify-both-files" branch &&
+               echo "wt content" >expect &&
+               test_cmp expect file &&
+               echo "tmp content" >expect &&
+               test_cmp expect "$(cat tmpdir)/file"
+       )
+'
+
 test_done