Re: [PATCH v2 2/3] t7800: fix tests when difftool uses --no-symlinks

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

Am 3/24/2013 16:15, schrieb John Keeping:
> Subject: [PATCH] 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, store the
> initial hash of the working tree file 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.
> 
> Signed-off-by: John Keeping <john@keeping.me.uk>
> ---
>  git-difftool.perl   | 35 +++++++++++++++++++++--------------
>  t/t7800-difftool.sh | 26 ++++++++++++++++++++++++++
>  2 files changed, 47 insertions(+), 14 deletions(-)
> 
> diff --git a/git-difftool.perl b/git-difftool.perl
> index c433e86..be82b5a 100755
> --- a/git-difftool.perl
> +++ b/git-difftool.perl
> @@ -15,7 +15,6 @@ use strict;
>  use warnings;
>  use File::Basename qw(dirname);
>  use File::Copy;
> -use File::Compare;
>  use File::Find;
>  use File::stat;
>  use File::Path qw(mkpath rmtree);
> @@ -123,7 +122,7 @@ sub setup_dir_diff
>  	my $rindex = '';
>  	my %submodule;
>  	my %symlink;
> -	my @working_tree = ();
> +	my %working_tree;
>  	my @rawdiff = split('\0', $diffrtn);
>  
>  	my $i = 0;
> @@ -175,7 +174,9 @@ EOF
>  
>  		if ($rmode ne $null_mode) {
>  			if (use_wt_file($repo, $workdir, $dst_path, $rsha1, $symlinks)) {
> -				push(@working_tree, $dst_path);
> +				$working_tree{$dst_path} =
> +					$repo->command_oneline('hash-object',
> +						"$workdir/$dst_path");
>  			} else {
>  				$rindex .= "$rmode $rsha1\t$dst_path\0";
>  			}
> @@ -227,7 +228,7 @@ EOF
>  	# not part of the index. Remove any trailing slash from $workdir
>  	# before starting to avoid double slashes in symlink targets.
>  	$workdir =~ s|/$||;
> -	for my $file (@working_tree) {
> +	for my $file (keys %working_tree) {
>  		my $dir = dirname($file);
>  		unless (-d "$rdir/$dir") {
>  			mkpath("$rdir/$dir") or
> @@ -278,7 +279,7 @@ EOF
>  		exit_cleanup($tmpdir, 1) if not $ok;
>  	}
>  
> -	return ($ldir, $rdir, $tmpdir, @working_tree);
> +	return ($ldir, $rdir, $tmpdir, %working_tree);
>  }
>  
>  sub write_to_file
> @@ -376,7 +377,7 @@ sub dir_diff
>  	my $error = 0;
>  	my $repo = Git->repository();
>  	my $workdir = find_worktree($repo);
> -	my ($a, $b, $tmpdir, @worktree) =
> +	my ($a, $b, $tmpdir, %worktree) =
>  		setup_dir_diff($repo, $workdir, $symlinks);
>  
>  	if (defined($extcmd)) {
> @@ -390,19 +391,25 @@ 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.
> -	for my $file (@worktree) {
> +	for my $file (keys %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";
> +		my $wt_hash = $repo->command_oneline('hash-object',
> +			"$workdir/$file");
> +		my $tmp_hash = $repo->command_oneline('hash-object',
> +			"$b/$file");

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.

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.

> +		my $wt_modified = $wt_hash ne $worktree{$file};
> +		my $tmp_modified = $tmp_hash ne $worktree{$file};
> +
> +		if ($wt_modified and $tmp_modified) {
> +			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) {
>  			my $mode = stat("$b/$file")->mode;
>  			copy("$b/$file", "$workdir/$file") or
>  			exit_cleanup($tmpdir, 1);

-- Hannes