Re: difftool -d symlinks, under what conditions

Matt McClure [off-list ref] writes:

An alternative approach would be to reuse git-diff's option parsing
and make it tell git-difftool when git-diff sees the working tree
case. At this point, I haven't seen an obvious place in the source
where git-diff makes that choice, but if someone could point me in the
right direction, I think I'd actually prefer that approach. What do
you think?

I do not think you want to go there.  That wouldn't solve the third
case in my previous message, no?

On Mar 12, 2013, at 4:16 PM, Junio C Hamano [off-list ref] wrote:

Matt McClure [off-list ref] writes:

quoted

An alternative approach would be to reuse git-diff's option parsing

I do not think you want to go there.  That wouldn't solve the third
case in my previous message, no?

I think I don't fully understand your third bullet.

* If you are comparing two trees, and especially if your RHS is not
  HEAD, you will send everything to a temporary without
  symlinks. Any edit made by the user will be lost.

I think you're suggesting to use a symlink any time the content of any
given RHS revision is the same as the working tree.

I imagine that might confuse me as a user. It would create
circumstances where some files are symlinked and others aren't for
reasons that won't be straightforward.

I imagine solving that case, I might instead implement a copy back to
the working tree with conflict detection/resolution. Some earlier
iterations of the directory diff feature used copy back without
conflict detection and created situations where I clobbered my own
changes by finishing a directory diff after making edits concurrently.

On Tue, Mar 12, 2013 at 04:48:16PM -0600, Matt McClure wrote:

On Mar 12, 2013, at 4:16 PM, Junio C Hamano [off-list ref] wrote:

quoted

Matt McClure [off-list ref] writes:

* If you are comparing two trees, and especially if your RHS is not
  HEAD, you will send everything to a temporary without
  symlinks. Any edit made by the user will be lost.

I think you're suggesting to use a symlink any time the content of any
given RHS revision is the same as the working tree.

I imagine that might confuse me as a user. It would create
circumstances where some files are symlinked and others aren't for
reasons that won't be straightforward.

I imagine solving that case, I might instead implement a copy back to
the working tree with conflict detection/resolution. Some earlier
iterations of the directory diff feature used copy back without
conflict detection and created situations where I clobbered my own
changes by finishing a directory diff after making edits concurrently.

The code to copy back working tree files is already there, it just
triggers using the same logic as the creation of symlinks in the first
place and doesn't attempt any conflict detection.  I suspect that any
more comprehensive solution will need to restrict the use of "git
difftool -d" whenever the index contains unmerged entries or when there
are both staged and unstaged changes, since the merge resolution will
cause these states to be lost.

The implementation of Junio's suggestion is relatively straightforward
(this is untested, although t7800 passes, and can probably be improved
by someone better versed in Perl).  Does this work for your original
scenario?

-- >8 --

diff --git a/git-difftool.perl b/git-difftool.perl
index 0a90de4..5f093ae 100755
--- a/git-difftool.perl
+++ b/git-difftool.perl

@@ -83,6 +83,21 @@ sub exit_cleanup
 	exit($status | ($status >> 8));
 }
 
+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) {
+		return 0;
+	}
+
+	my $wt_sha1 = $repo->command_oneline('hash-object', "$workdir/$file");
+	return $sha1 eq $wt_sha1;
+}
+
 sub setup_dir_diff
 {
 	my ($repo, $workdir, $symlinks) = @_;

@@ -159,10 +174,10 @@ EOF
 		}
 
 		if ($rmode ne $null_mode) {
-			if ($rsha1 ne $null_sha1) {
-				$rindex .= "$rmode $rsha1\t$dst_path\0";
-			} else {
+			if (use_wt_file($repo, $workdir, $dst_path, $rsha1, $symlinks)) {
 				push(@working_tree, $dst_path);
+			} else {
+				$rindex .= "$rmode $rsha1\t$dst_path\0";
 			}
 		}
 	}

-- 
1.8.2.rc2.4.g7799588

On Tue, Mar 12, 2013 at 8:17 PM, John Keeping [off-list ref] wrote:

Does this work for your original scenario?

Yes. Thanks!

-- 
Matt McClure
http://www.matthewlmcclure.com
http://www.mapmyfitness.com/profile/matthewlmcclure

On Tue, Mar 12, 2013 at 5:17 PM, John Keeping [off-list ref] wrote:

On Tue, Mar 12, 2013 at 04:48:16PM -0600, Matt McClure wrote:

quoted

On Mar 12, 2013, at 4:16 PM, Junio C Hamano [off-list ref] wrote:

quoted

Matt McClure [off-list ref] writes:

* If you are comparing two trees, and especially if your RHS is not
  HEAD, you will send everything to a temporary without
  symlinks. Any edit made by the user will be lost.

I think you're suggesting to use a symlink any time the content of any
given RHS revision is the same as the working tree.

I imagine that might confuse me as a user. It would create
circumstances where some files are symlinked and others aren't for
reasons that won't be straightforward.

I imagine solving that case, I might instead implement a copy back to
the working tree with conflict detection/resolution. Some earlier
iterations of the directory diff feature used copy back without
conflict detection and created situations where I clobbered my own
changes by finishing a directory diff after making edits concurrently.

The code to copy back working tree files is already there, it just
triggers using the same logic as the creation of symlinks in the first
place and doesn't attempt any conflict detection.  I suspect that any
more comprehensive solution will need to restrict the use of "git
difftool -d" whenever the index contains unmerged entries or when there
are both staged and unstaged changes, since the merge resolution will
cause these states to be lost.

The implementation of Junio's suggestion is relatively straightforward
(this is untested, although t7800 passes, and can probably be improved
by someone better versed in Perl).  Does this work for your original
scenario?

This is a nice straightforward approach.

As Junio mentioned, a good next step would be this patch
in combination with making the truly temporary files
created by dir-diff readonly.

Will that need a win32 platform check?
Does anyone want to take this and whip it into a proper patch?

-- 
David