Re: difftool -d symlinks, under what conditions

On Tue, Nov 27, 2012 at 7:41 AM, Matt McClure [off-list ref] wrote:

On Tuesday, November 27, 2012, David Aguilar wrote:

quoted

It seems that there is an edge case here that we are not
accounting for: unmodified worktree paths, when checked out
into the temporary directory, can be edited by the tool when
comparing against older commits.  These edits will be lost.

Yes. That is exactly my desired use case. I want to make edits while I'm reviewing the diff.

I took a crack at implementing the change to make difftool -d use
symlinks more aggressively. I've tested it lightly, and it works for
the limited cases I've tried. This is my first foray into the Git
source code, so it's entirely possible that there are unintended side
effects and regressions if other features depend on the same code path
and make different assumptions.

https://github.com/matthewlmcclure/git/compare/difftool-directory-symlink-work-tree

Your thoughts on the change?

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

On Tue, Mar 12, 2013 at 02:12:29PM -0400, Matt McClure wrote:

On Tue, Nov 27, 2012 at 7:41 AM, Matt McClure [off-list ref] wrote:
Your thoughts on the change?

Please include the patch in your message so that interested parties can
comment on it here, especially since the compare view on GitHub seems to
mangle the tabs.

For others' reference the patch is:

-- >8 --
From: Matt McClure <redacted>
Subject: [PATCH] difftool: Make directory diff symlink work tree

difftool -d formerly knew how to symlink to the work tree when the work
tree contains uncommitted changes. In practice, prior to this change, it
would not symlink to the work tree in case there were no uncommitted
changes, even when the user invoked difftool with the form:

    git difftool -d [--options] <commit> [--] [<path>...]
        This form is to view the changes you have in your working tree
        relative to the named <commit>. You can use HEAD to compare it
        with the latest commit, or a branch name to compare with the tip
        of a different branch.

Instead, prior to this change, difftool would use the file's HEAD blob
sha1 to find its content rather than the work tree content. This change
teaches `git diff --raw` to emit the null SHA1 for consumption by
difftool -d, so that difftool -d will use a symlink rather than a copy
of the file.

Before:

    $ git diff --raw HEAD^ -- diff-lib.c
    :100644 100644 f35de0f... ead9399... M  diff-lib.c

After:

    $ ./git diff --raw HEAD^ -- diff-lib.c
    :100644 100644 f35de0f... 0000000... M  diff-lib.c
---
 diff-lib.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/diff-lib.c b/diff-lib.c
index f35de0f..ead9399 100644
--- a/diff-lib.c
+++ b/diff-lib.c

@@ -319,6 +319,10 @@ static int show_modified(struct rev_info *revs,
 		return -1;
 	}
 
+	if (!cached && hashcmp(old->sha1, new->sha1)) {
+		sha1 = null_sha1;
+	}
+
 	if (revs->combine_merges && !cached &&
 	    (hashcmp(sha1, old->sha1) || hashcmp(old->sha1, new->sha1))) {
 		struct combine_diff_path *p;

-- 
1.8.2.rc2.4.g7799588

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

On Tue, Mar 12, 2013 at 02:12:29PM -0400, Matt McClure wrote:

quoted

On Tue, Nov 27, 2012 at 7:41 AM, Matt McClure [off-list ref] wrote:
Your thoughts on the change?

Please include the patch in your message so that interested parties can
comment on it here, especially since the compare view on GitHub seems to
mangle the tabs.

For others' reference the patch is:

-- >8 --
From: Matt McClure <redacted>
Subject: [PATCH] difftool: Make directory diff symlink work tree

difftool -d formerly knew how to symlink to the work tree when the work
tree contains uncommitted changes. In practice, prior to this change, it
would not symlink to the work tree in case there were no uncommitted
changes, even when the user invoked difftool with the form:

    git difftool -d [--options] <commit> [--] [<path>...]
        This form is to view the changes you have in your working tree
        relative to the named <commit>. You can use HEAD to compare it
        with the latest commit, or a branch name to compare with the tip
        of a different branch.

Instead, prior to this change, difftool would use the file's HEAD blob
sha1 to find its content rather than the work tree content. This change
teaches `git diff --raw` to emit the null SHA1 for consumption by
difftool -d, so that difftool -d will use a symlink rather than a copy
of the file.

Before:

    $ git diff --raw HEAD^ -- diff-lib.c
    :100644 100644 f35de0f... ead9399... M  diff-lib.c

After:

    $ ./git diff --raw HEAD^ -- diff-lib.c
    :100644 100644 f35de0f... 0000000... M  diff-lib.c

Interesting approach.  While this does get the intended behavior
for difftool, I'm afraid this would be a grave regression for
existing "git diff --raw" users who cannot have such behavior.

I don't think we could do this without adding an additional flag
to trigger this change in behavior (e.g. --null-sha1-for-....?)
so that existing users are unaffected by the change.

It feels like forcing the null SHA-1 is heavy-handed, but I
haven't thought it through enough.

While this may be a quick way to get this behavior,
I wonder if there is a better way.

Does anybody else have any comments/suggestions on how to
better accomplish this?

-- 
David

difftool -d formerly knew how to symlink to the work tree when the work
tree contains uncommitted changes. In practice, prior to this change, it
would not symlink to the work tree in case there were no uncommitted
changes, even when the user invoked difftool with the form:

    git difftool -d [--options] <commit> [--] [<path>...]
        This form is to view the changes you have in your working tree
        relative to the named <commit>. You can use HEAD to compare it
        with the latest commit, or a branch name to compare with the tip
        of a different branch.

Instead, prior to this change, difftool would use the file's HEAD blob
sha1 to find its content rather than the work tree content. This change
teaches `git diff --raw` to emit the null SHA1 for consumption by
difftool -d, so that difftool -d will use a symlink rather than a copy
of the file.

Before:

    $ git diff --raw HEAD^ -- diff-lib.c
    :100644 100644 f35de0f... ead9399... M  diff-lib.c

After:

    $ ./git diff --raw HEAD^ -- diff-lib.c
    :100644 100644 f35de0f... 0000000... M  diff-lib.c

When I tried this I got the expected behaviour even without this patch.

It turns out that an uncommitted, but *staged* change emits the SHA1 of
the blob rather than the null SHA1.  Do you get the desired behaviour if
you "git reset" before using difftool?

If so I think you want some new mode of operation for difftool instead
of this patch which will also affect unrelated commands.

On Tue, Mar 12, 2013 at 12:23:52PM -0700, David Aguilar wrote:

I don't think we could do this without adding an additional flag
to trigger this change in behavior (e.g. --null-sha1-for-....?)
so that existing users are unaffected by the change.

It feels like forcing the null SHA-1 is heavy-handed, but I
haven't thought it through enough.

While this may be a quick way to get this behavior,
I wonder if there is a better way.

Does anybody else have any comments/suggestions on how to
better accomplish this?

How about something like "--symlink-all" where the everything in the
right-hand tree is symlink'd?

Something like this perhaps:

-- >8 --

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

@@ -85,7 +85,7 @@ sub exit_cleanup
 
 sub setup_dir_diff
 {
-	my ($repo, $workdir, $symlinks) = @_;
+	my ($repo, $workdir, $symlinks, $symlink_all) = @_;
 
 	# Run the diff; exit immediately if no diff found
 	# 'Repository' and 'WorkingCopy' must be explicitly set to insure that

@@ -159,10 +159,10 @@ EOF
 		}
 
 		if ($rmode ne $null_mode) {
-			if ($rsha1 ne $null_sha1) {
-				$rindex .= "$rmode $rsha1\t$dst_path\0";
-			} else {
+			if ($symlink_all or $rsha1 eq $null_sha1) {
 				push(@working_tree, $dst_path);
+			} else {
+				$rindex .= "$rmode $rsha1\t$dst_path\0";
 			}
 		}
 	}

@@ -299,6 +299,7 @@ sub main
 		prompt => undef,
 		symlinks => $^O ne 'cygwin' &&
 				$^O ne 'MSWin32' && $^O ne 'msys',
+		symlink_all => undef,
 		tool_help => undef,
 	);
 	GetOptions('g|gui!' => \$opts{gui},

@@ -308,6 +309,7 @@ sub main
 		'y' => sub { $opts{prompt} = 0; },
 		'symlinks' => \$opts{symlinks},
 		'no-symlinks' => sub { $opts{symlinks} = 0; },
+		'symlink-all' => \$opts{symlink_all},
 		't|tool:s' => \$opts{difftool_cmd},
 		'tool-help' => \$opts{tool_help},
 		'x|extcmd:s' => \$opts{extcmd});

@@ -346,7 +348,7 @@ sub main
 	# will invoke a separate instance of 'git-difftool--helper' for
 	# each file that changed.
 	if (defined($opts{dirdiff})) {
-		dir_diff($opts{extcmd}, $opts{symlinks});
+		dir_diff($opts{extcmd}, $opts{symlinks}, $opts{symlink_all});
 	} else {
 		file_diff($opts{prompt});
 	}

@@ -354,13 +356,13 @@ sub main
 
 sub dir_diff
 {
-	my ($extcmd, $symlinks) = @_;
+	my ($extcmd, $symlinks, $symlink_all) = @_;
 	my $rc;
 	my $error = 0;
 	my $repo = Git->repository();
 	my $workdir = find_worktree($repo);
 	my ($a, $b, $tmpdir, @worktree) =
-		setup_dir_diff($repo, $workdir, $symlinks);
+		setup_dir_diff($repo, $workdir, $symlinks, $symlink_all);
 
 	if (defined($extcmd)) {
 		$rc = system($extcmd, $a, $b);

On Mar 12, 2013, at 1:25 PM, John Keeping [off-list ref] wrote:

When I tried this I got the expected behaviour even without this patch.

    git diff --raw commit

emits the null SHA1 if the working tree file's stat differs from the
blob corresponding to commit. Is that the case you observed?

On Tue, Mar 12, 2013 at 05:12:28PM -0600, Matt McClure wrote:

On Mar 12, 2013, at 1:25 PM, John Keeping [off-list ref] wrote:

quoted

When I tried this I got the expected behaviour even without this patch.

    git diff --raw commit

emits the null SHA1 if the working tree file's stat differs from the
blob corresponding to commit. Is that the case you observed?

Yes, although it's slightly more subtle than that - the null SHA1 only
occurs if the working tree file has unstaged changes; if you add the
changes to the index then the null SHA1 is no longer used since the blob
is now available in Git's object store.


John

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

When I tried this I got the expected behaviour even without this patch.

It turns out that an uncommitted, but *staged* change emits the SHA1 of
the blob rather than the null SHA1.  Do you get the desired behaviour if
you "git reset" before using difftool?

I tried this:

$ git diff --raw HEAD^
:100644 100644 f35de0f... ead9399... M  diff-lib.c

$ git reset HEAD^
Unstaged changes after reset:
M diff-lib.c

$ git diff --raw
:100644 100644 f35de0f... 0000000... M  diff-lib.c

$ git difftool -d

and the last command did indeed create symlinks into my working tree
rather than file copies.

So... it seems that using git-reset is at least a workaround to get
the symlink behavior I want as a user, though the dance I have to do
is a little more awkward than `git difftool -d HEAD^` would be.

If so I think you want some new mode of operation for difftool instead
of this patch which will also affect unrelated commands.

Are you suggesting that difftool do the reset work above given a new
option or by default?

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

On Tue, Mar 12, 2013 at 08:26:21PM -0400, Matt McClure wrote:

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

quoted

If so I think you want some new mode of operation for difftool instead
of this patch which will also affect unrelated commands.

Are you suggesting that difftool do the reset work above given a new
option or by default?

I was suggesting something like the "--symlink-all" option discussed in
the parallel thread, but it looks like we now have a better solution
than that.


John