Re: [PATCH] difftool: avoid symlinks when reusing worktree files

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Junio C Hamano <gitster@xxxxxxxxx> writes:

> So I think it is fine to return $use=0 for any symbolic link from
> use_wt_file.  Anything you do there will be replaced by the loop
> over %symlink that appears later in the caller.  The caller discards
> $wt_sha1 when $use=0 is returned, so the second return value does
> not matter.

So let me try to update your patch with the result of the study of
the codeflow.

-- >8 --
From: David Aguilar <davvid@xxxxxxxxx>
Subject: difftool: ignore symbolic links in use_wt_file

The caller is preparing a narrowed-down copy of the working tree and
this function is asked if the path should be included in that copy.
If we say yes, the path from the working tree will be either symlinked
or copied into the narrowed-down copy.

For any path that is a symbolic link, the caller later fixes up the
narrowed-down copy by unlinking the path and replacing it with a
regular file it writes out that mimics the way how "git diff"
compares symbolic links.

Let's answer "no, you do not want to copy/symlink the working tree
file" for all symbolic links from this function, as we know the
result will not be used because it will be overwritten anyway.

Incidentally, this also stops the function from feeding a symbolic
link in the working tree to hash-object, which is a wrong thing to
do to begin with. The link may be pointing at a directory, or worse
may be dangling (both would be noticed as an error).  Even if the
link points at a regular file, hashing the contents of a file that
is pointed at by the link is not correct (Git hashes the contents of
the link itself, not the pointee).

Signed-off-by: David Aguilar <davvid@xxxxxxxxx>
Signed-off-by: Junio C Hamano <gitster@xxxxxxxxx>
---
 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..488d14b 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 (-l "$workdir/$file" || ! -e _) {
 		return (0, $null_sha1);
 	}
 
diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
index ea35a02..a771cf7 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]