[PATCH] difftool: gracefully handle symlinks to directories

[PATCH] difftool: gracefully handle symlinks to directories

[David Aguilar]

difftool's dir-diff feature was blindly feeding worktree paths
to hash-object without checking whether the path was indeed a
file, causing the feature to fail when repositories contain
symlinks to directories.

Ensure that only files are ever given to hash-object.
Add a test to demonstrate the breakage.

Reported-by: Ismail Badawi <ismail@badawi.io>
Signed-off-by: David Aguilar <davvid@gmail.com>
---
 git-difftool.perl   |  4 +---
 t/t7800-difftool.sh | 19 +++++++++++++++++++
 2 files changed, 20 insertions(+), 3 deletions(-)

diff --git a/git-difftool.perl b/git-difftool.perl
index 7df7c8a..1abe647 100755
--- a/git-difftool.perl
+++ b/git-difftool.perl
@@ -70,9 +70,7 @@ sub use_wt_file
 	my ($repo, $workdir, $file, $sha1) = @_;
 	my $null_sha1 = '0' x 40;
 
-	if (! -e "$workdir/$file") {
-		# If the file doesn't exist in the working tree, we cannot
-		# use it.
+	if (! -f "$workdir/$file") {
 		return (0, $null_sha1);
 	}
 
diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
index 48c6e2b..ec8bc8c 100755
--- a/t/t7800-difftool.sh
+++ b/t/t7800-difftool.sh
@@ -504,4 +504,23 @@ test_expect_success PERL 'difftool properly honors gitlink and core.worktree' '
 	)
 '
 
+test_expect_success PERL,SYMLINKS 'difftool --dir-diff symlinked directories' '
+	git init dirlinks &&
+	(
+		cd dirlinks &&
+		git config diff.tool checktrees &&
+		git config difftool.checktrees.cmd "echo good" &&
+		mkdir foo &&
+		: >foo/bar &&
+		git add foo/bar &&
+		test_commit symlink-one &&
+		ln -s foo link &&
+		git add link &&
+		test_commit symlink-two &&
+		echo good >expect &&
+		git difftool --tool=checktrees --dir-diff HEAD~ >actual &&
+		test_cmp expect actual
+	)
+'
+
 test_done
-- 
2.6.2.281.gac28444

Re: [PATCH] difftool: gracefully handle symlinks to directories

[Junio C Hamano]

David Aguilar <davvid@gmail.com> writes:

> difftool's dir-diff feature was blindly feeding worktree paths
> to hash-object without checking whether the path was indeed a
> file, causing the feature to fail when repositories contain
> symlinks to directories.

Wait.  Anything that considers symlinks "to directories" any special
smells like a misdesign here.  Why is it safe to substitute a
symbolic link that happens to point at a file with the file it
points at?

Because the way you would hash a symblic link is not by hashing the
file it points at, but by hashing the result of readlink(2) of it,
we must not reuse the working tree files for any symbolic link,
regardless of its target, I would think.

After all, a symbolic link may even be dangling and not pointing at
anything.

>
> Ensure that only files are ever given to hash-object.
> Add a test to demonstrate the breakage.
>
> Reported-by: Ismail Badawi <ismail@badawi.io>
> Signed-off-by: David Aguilar <davvid@gmail.com>
> ---
>  git-difftool.perl   |  4 +---
>  t/t7800-difftool.sh | 19 +++++++++++++++++++
>  2 files changed, 20 insertions(+), 3 deletions(-)
>
> diff --git a/git-difftool.perl b/git-difftool.perl
> index 7df7c8a..1abe647 100755
> --- a/git-difftool.perl
> +++ b/git-difftool.perl
> @@ -70,9 +70,7 @@ sub use_wt_file
>  	my ($repo, $workdir, $file, $sha1) = @_;
>  	my $null_sha1 = '0' x 40;
>  
> -	if (! -e "$workdir/$file") {
> -		# If the file doesn't exist in the working tree, we cannot
> -		# use it.
> +	if (! -f "$workdir/$file") {
>  		return (0, $null_sha1);
>  	}
>  
> diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
> index 48c6e2b..ec8bc8c 100755
> --- a/t/t7800-difftool.sh
> +++ b/t/t7800-difftool.sh
> @@ -504,4 +504,23 @@ test_expect_success PERL 'difftool properly honors gitlink and core.worktree' '
>  	)
>  '
>  
> +test_expect_success PERL,SYMLINKS 'difftool --dir-diff symlinked directories' '
> +	git init dirlinks &&
> +	(
> +		cd dirlinks &&
> +		git config diff.tool checktrees &&
> +		git config difftool.checktrees.cmd "echo good" &&
> +		mkdir foo &&
> +		: >foo/bar &&
> +		git add foo/bar &&
> +		test_commit symlink-one &&
> +		ln -s foo link &&
> +		git add link &&
> +		test_commit symlink-two &&
> +		echo good >expect &&
> +		git difftool --tool=checktrees --dir-diff HEAD~ >actual &&
> +		test_cmp expect actual
> +	)
> +'
> +
>  test_done

Re: [PATCH] difftool: gracefully handle symlinks to directories

[David Aguilar]

On Thu, Oct 22, 2015 at 11:23:54AM -0700, Junio C Hamano wrote:
> David Aguilar <davvid@gmail.com> writes:
> 
> > difftool's dir-diff feature was blindly feeding worktree paths
> > to hash-object without checking whether the path was indeed a
> > file, causing the feature to fail when repositories contain
> > symlinks to directories.
> 
> Wait.  Anything that considers symlinks "to directories" any special
> smells like a misdesign here.  Why is it safe to substitute a
> symbolic link that happens to point at a file with the file it
> points at?
> 
> Because the way you would hash a symblic link is not by hashing the
> file it points at, but by hashing the result of readlink(2) of it,
> we must not reuse the working tree files for any symbolic link,
> regardless of its target, I would think.
> 
> After all, a symbolic link may even be dangling and not pointing at
> anything.

Ah, right.  I think the simplest thing to do is to tighten
use_wt_file() so that it always rejects symlinks.  That seems
like a safe way to go for now without needing to invent a new
paradigm for how to handle symlinks in the dir-diff code.

I just sent a follow-up patch that does just that.  Let me know
if you'd like a replacement patch that combines the two patches
instead.

Thanks for the review,
-- 
David