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

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

Thanks.

I should drop the t7800-modernize topic and queue this under a
better name.  Perhaps "difftool-no-overwrite-on-copyback", or
something.

> 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 <j...@keeping.me.uk>
>
> ---
> Changes since v2:
> - Set TMPDIR to $TRASH_DIRECTORY in the test where difftool fails
>
>  git-difftool.perl   | 73 
> +++++++++++++++++++++++++++++++++++++++++++----------
>  t/t7800-difftool.sh | 30 ++++++++++++++++++++++
>  2 files changed, 89 insertions(+), 14 deletions(-)
>
> diff --git a/git-difftool.perl b/git-difftool.perl
> index 663640d..844f619 100755
> --- a/git-difftool.perl
> +++ b/git-difftool.perl
> @@ -13,9 +13,9 @@
>  use 5.008;
>  use strict;
>  use warnings;
> +use Error qw(:try);
>  use File::Basename qw(dirname);
>  use File::Copy;
> -use File::Compare;
>  use File::Find;
>  use File::stat;
>  use File::Path qw(mkpath rmtree);
> @@ -88,14 +88,45 @@ sub use_wt_file
>       my ($repo, $workdir, $file, $sha1, $symlinks) = @_;
>       my $null_sha1 = '0' x 40;
>  
> -     if ($sha1 eq $null_sha1) {
> -             return 1;
> -     } elsif (not $symlinks) {
> +     if ($sha1 ne $null_sha1 and not $symlinks) {
>               return 0;
>       }
>  
>       my $wt_sha1 = $repo->command_oneline('hash-object', "$workdir/$file");
> -     return $sha1 eq $wt_sha1;
> +     my $use = ($sha1 eq $null_sha1) || ($sha1 eq $wt_sha1);
> +     return ($use, $wt_sha1);
> +}
> +
> +sub changed_files
> +{
> +     my ($repo_path, $index, $worktree) = @_;
> +     $ENV{GIT_INDEX_FILE} = $index;
> +     $ENV{GIT_WORK_TREE} = $worktree;
> +     my $must_unset_git_dir = 0;
> +     if (not defined($ENV{GIT_DIR})) {
> +             $must_unset_git_dir = 1;
> +             $ENV{GIT_DIR} = $repo_path;
> +     }
> +
> +     my @refreshargs = qw/update-index --really-refresh -q --unmerged/;
> +     my @gitargs = qw/diff-files --name-only -z/;
> +     try {
> +             Git::command_oneline(@refreshargs);
> +     } catch Git::Error::Command with {};
> +
> +     my $line = Git::command_oneline(@gitargs);
> +     my @files;
> +     if (defined $line) {
> +             @files = split('\0', $line);
> +     } else {
> +             @files = ();
> +     }
> +
> +     delete($ENV{GIT_INDEX_FILE});
> +     delete($ENV{GIT_WORK_TREE});
> +     delete($ENV{GIT_DIR}) if ($must_unset_git_dir);
> +
> +     return map { $_ => 1 } @files;
>  }
>  
>  sub setup_dir_diff
> @@ -121,6 +152,7 @@ sub setup_dir_diff
>       my $null_sha1 = '0' x 40;
>       my $lindex = '';
>       my $rindex = '';
> +     my $wtindex = '';
>       my %submodule;
>       my %symlink;
>       my @working_tree = ();
> @@ -174,8 +206,12 @@ EOF
>               }
>  
>               if ($rmode ne $null_mode) {
> -                     if (use_wt_file($repo, $workdir, $dst_path, $rsha1, 
> $symlinks)) {
> -                             push(@working_tree, $dst_path);
> +                     my ($use, $wt_sha1) = use_wt_file($repo, $workdir,
> +                                                       $dst_path, $rsha1,
> +                                                       $symlinks);
> +                     if ($use) {
> +                             push @working_tree, $dst_path;
> +                             $wtindex .= "$rmode $wt_sha1\t$dst_path\0";
>                       } else {
>                               $rindex .= "$rmode $rsha1\t$dst_path\0";
>                       }
> @@ -218,6 +254,12 @@ EOF
>       $rc = system('git', 'checkout-index', '--all', "--prefix=$rdir/");
>       exit_cleanup($tmpdir, $rc) if $rc != 0;
>  
> +     $ENV{GIT_INDEX_FILE} = "$tmpdir/wtindex";
> +     ($inpipe, $ctx) =
> +             $repo->command_input_pipe(qw(update-index --info-only -z 
> --index-info));
> +     print($inpipe $wtindex);
> +     $repo->command_close_pipe($inpipe, $ctx);
> +
>       # If $GIT_DIR was explicitly set just for the update/checkout
>       # commands, then it should be unset before continuing.
>       delete($ENV{GIT_DIR}) if ($must_unset_git_dir);
> @@ -390,19 +432,22 @@ sub dir_diff
>       # should be copied back to the working tree.
>       # Do not copy back files when symlinks are used and the
>       # external tool did not replace the original link with a file.
> +     my %wt_modified = changed_files($repo->repo_path(),
> +             "$tmpdir/wtindex", "$workdir");
> +     my %tmp_modified = changed_files($repo->repo_path(),
> +             "$tmpdir/wtindex", "$b");

Do we need to run these two comparisons unconditionally?

It appears to me that in a sane and safe setting (i.e. $symlinks is
set and the "diff viewer" touches the file through the symbolic
link) does not ever look at wt_modified/tmp_modified at all because
the first check in the loop will always trigger.

I wonder if it makes sense to populate these two hashes lazily so
that the safe case does not have to pay the penalty at all.

>       for my $file (@worktree) {
>               next if $symlinks && -l "$b/$file";
>               next if ! -f "$b/$file";
>  
> +             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 ($tmp_modified{$file}) {

"exists" if only for consistency?
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to