git difftool --dir-diff error in the presence of symlinks to directories

git difftool --dir-diff error in the presence of symlinks to directories

[Ismail Badawi]

Reproduce like this (using git 2.4.3):

git init
mkdir foo
touch foo/bar
git add .
git commit -m "Initial commit."
ln -s foo link
git add .
git commit -m "Add link to foo."
git difftool -d HEAD^ HEAD

That last command outputs:

fatal: Unable to hash /Users/isbadawi/test/link
hash-object /Users/isbadawi/test/link: command returned error: 128

Briefly looking at the 'git difftool' source it looks like it uses the
output of 'git diff --raw' and calls 'hash-object' on any object whose
mode is nonzero, including symlinks.

I'm not sure what the right thing to do here is -- just thought I'd
report this failure.

Thanks,
Ismail

Re: git difftool --dir-diff error in the presence of symlinks to directories

[David Aguilar]

On Wed, Jun 17, 2015 at 06:39:27PM -0400, Ismail Badawi wrote:
> Reproduce like this (using git 2.4.3):
> 
> git init
> mkdir foo
> touch foo/bar
> git add .
> git commit -m "Initial commit."
> ln -s foo link
> git add .
> git commit -m "Add link to foo."
> git difftool -d HEAD^ HEAD
> 
> That last command outputs:
> 
> fatal: Unable to hash /Users/isbadawi/test/link
> hash-object /Users/isbadawi/test/link: command returned error: 128
> 
> Briefly looking at the 'git difftool' source it looks like it uses the
> output of 'git diff --raw' and calls 'hash-object' on any object whose
> mode is nonzero, including symlinks.
> 
> I'm not sure what the right thing to do here is -- just thought I'd
> report this failure.
> 
> Thanks,
> Ismail

Thanks for the bug report, Ismail.

This looks like an unhandled edge case that difftool's dir-diff
mode doesn't take into account.  I've cc'd Tim and John since
they're both familiar with this part of the code.

We should take your bug recipe above and add it to difftool's
test suite when fixing the bug.  I've included a diff below that
adds the above snippet to t7800-difftool.sh.

As far as how to handle this edge case -- one option is that we
could detect that the link points at a directory and ignore it.

I think what's happening is that we're hashing the file so that
we can later detect whether it's changed.  The fix might be as
simple as skipping links to directories in that code path, but I
haven't looked too closely yet.  I can take a look in a couple
of weeks if no one beats me to it by then.

--- >8 --- >8 ---
diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
index ea35a02..f67fc65 100755
--- a/t/t7800-difftool.sh
+++ b/t/t7800-difftool.sh
@@ -504,4 +504,24 @@ test_expect_success PERL 'difftool properly honors gitlink and core.worktree' '
 	)
 '
 
+test_expect_success PERL,SYMLINKS 'difftool --dir-diff symlinked directories' '
+	test_config diff.tool checktrees &&
+	test_config difftool.checktrees.cmd "echo good" &&
+	git init dirlinks &&
+	(
+		cd dirlinks &&
+		mkdir foo &&
+		: >foo/bar &&
+		git add foo/bar &&
+		test_commit one &&
+		ln -s foo link
+		git add link
+		test_commit two &&
+		echo good >expect &&
+		git difftool --dir-diff HEAD~ >actual &&
+		test_commit expect actual
+
+	)
+'
+
 test_done
-- 
David