Re: [PATCH v2] difftool: don't overwrite modified files

[Johannes Sixt <j.sixt@viscovery.net>]

Am 3/25/2013 22:44, schrieb John Keeping:
> 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>
> ---
> On Mon, Mar 25, 2013 at 10:42:19AM +0000, John Keeping wrote:
>> On Mon, Mar 25, 2013 at 08:41:59AM +0100, Johannes Sixt wrote:
>>> This is gross. Can't we do much better here? Difftool already keeps a
>>> GIT_INDEX of the files in the temporary tree ($tmpdir/rindex). Running
>>> git-diff-files should be sufficient to tell which ones where edited via
>>> the users's diff-tool. Then you can restrict calling hash-object to only
>>> those worktree files where an "edit collision" needs to be checked for.
>>
>> That's only the case for files that are not copied from the working
>> tree, so the temporary index doesn't contain the files that are of
>> interest here.
>>
>>> You could also keep a parallel index that keeps the state of the same set
>>> of files in the worktree. Then another git-diff-files call could replace
>>> the other half of hash-object calls.
>>
>> I like the idea of creating an index from the working tree files and
>> using it here.  If we create a "starting state" index for these files,
>> we should be able to run git-diff-files against both the working tree
>> and the temporary tree at this point and compare the output.
> 
> Here's an attempt at taking this approach, built on
> jk/difftool-dir-diff-edit-fix.
> 
>  git-difftool.perl   | 73 +++++++++++++++++++++++++++++++++++++++++++----------
>  t/t7800-difftool.sh | 26 +++++++++++++++++++
>  2 files changed, 85 insertions(+), 14 deletions(-)
> 
> diff --git a/git-difftool.perl b/git-difftool.perl
> index c433e86..d10f7d2 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");
>  	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 (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 ($tmp_modified{$file}) {
>  			my $mode = stat("$b/$file")->mode;
>  			copy("$b/$file", "$workdir/$file") or
>  			exit_cleanup($tmpdir, 1);

I don't have a lot to say about the patch text, except that there is
nothing obvious out of the ordinary, but please take this with a large
grain of salt, as I'm lacking context. (It's the first time these days
that I'm looking at difftool.)

BTW, did you know that perl is mostly a write-only language? ;-)

> diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
> index db3d3d6..be2042d 100755
> --- a/t/t7800-difftool.sh
> +++ b/t/t7800-difftool.sh
> @@ -407,4 +407,30 @@ test_expect_success PERL 'difftool --dir-diff from subdirectory' '
>  	)
>  '
>  
> +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 ' '
> +	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"
> +'

The new tests look good.

One question though: Do I understand correctly that the temporary
directories are leaked in the case of an "edit conflict"? If so, is it
worth a warning for the user to clean up the garbage?

-- Hannes