Re: [PATCH 2/5] difftool: don't overwrite modified files

[John Keeping <john@keeping.me.uk>]

On Fri, Mar 29, 2013 at 01:20:50PM -0700, Junio C Hamano wrote:
> John Keeping <john@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 <john@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.

Sounds sensible.  I'll look into doing this.

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

Not just for consistency - this will generate an uninitialized value
warning if the tmp version of the file hasn't been modified.