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

2 messages, 2 authors, 2016-06-15 · open the first message on its own page

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

From: Junio C Hamano <hidden>
Date: 2016-06-15 23:07:04

David Aguilar [off-list ref] writes:

difftool's dir-diff should never reuse a symlink, regardless of
what it points to.  Tighten use_wt_file() so that it rejects all
symlinks.

Helped-by: Junio C Hamano [off-list ref]
Signed-off-by: David Aguilar <redacted>
---

Sorry.  I do recall saying "it is wrong to feed the contents of a
file that a symlink points at to hash-object" but other than that,
I completely lost track.

What purpose does this function play in its callchain?  What does
its caller wants it to compute?  Is use of the entity in the working
tree completely optional?  Would the caller happily produce correct
result even if we changed this function to unconditionally return
($use=0, $wt_sha1='0'x40) regardless of the result of lstat(2) on
"$workdir/$file"?

The conclusion of the thought process that starts from "it is wrong
to feed the contents of a file that a symlink points at to
hash-object" may not be "so let's return $use=0 for all symlinks",
which is this patch. Depending on what its caller wants it to
compute, the right conclusion may be "we need to call hash-object
correctly by first running readlink and then feeding the result to
it".

And if the answer is "the caller wants us to compute the hash for a
symbolic link and say $use=1", then we would instead need to do
an equivalent of

	wt_sha1=$(readlink "$workdir/$file" | hash-object --stdin)

I cannot quite tell which from the patch and explanation.

Perhaps an additional test or two would help illustrate what issues
are being addressed better?

Thanks.

quoted hunk

 git-difftool.perl | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/git-difftool.perl b/git-difftool.perl
index 1abe647..873db57 100755
--- a/git-difftool.perl
+++ b/git-difftool.perl

@@ -70,13 +70,13 @@ sub use_wt_file
 	my ($repo, $workdir, $file, $sha1) = @_;
 	my $null_sha1 = '0' x 40;
 
-	if (! -f "$workdir/$file") {
-		return (0, $null_sha1);
+	my $workfile = "$workdir/$file";
+	if (-f $workfile && ! -l $workfile) {
+		my $wt_sha1 = $repo->command_oneline('hash-object', $workfile);
+		my $use = ($sha1 eq $null_sha1) || ($sha1 eq $wt_sha1);
+		return ($use, $wt_sha1);
 	}
-
-	my $wt_sha1 = $repo->command_oneline('hash-object', "$workdir/$file");
-	my $use = ($sha1 eq $null_sha1) || ($sha1 eq $wt_sha1);
-	return ($use, $wt_sha1);
+	return (0, $null_sha1);
 }
 
 sub changed_files

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

From: David Aguilar <hidden>
Date: 2016-06-15 23:07:06

On Tue, Oct 27, 2015 at 03:24:49PM -0700, Junio C Hamano wrote:

David Aguilar [off-list ref] writes:

quoted

difftool's dir-diff should never reuse a symlink, regardless of
what it points to.  Tighten use_wt_file() so that it rejects all
symlinks.

Helped-by: Junio C Hamano [off-list ref]
Signed-off-by: David Aguilar <redacted>
---

Sorry.  I do recall saying "it is wrong to feed the contents of a
file that a symlink points at to hash-object" but other than that,
I completely lost track.

What purpose does this function play in its callchain?  What does
its caller wants it to compute?  Is use of the entity in the working
tree completely optional?  Would the caller happily produce correct
result even if we changed this function to unconditionally return
($use=0, $wt_sha1='0'x40) regardless of the result of lstat(2) on
"$workdir/$file"?

The conclusion of the thought process that starts from "it is wrong
to feed the contents of a file that a symlink points at to
hash-object" may not be "so let's return $use=0 for all symlinks",
which is this patch. Depending on what its caller wants it to
compute, the right conclusion may be "we need to call hash-object
correctly by first running readlink and then feeding the result to
it".

And if the answer is "the caller wants us to compute the hash for a
symbolic link and say $use=1", then we would instead need to do
an equivalent of

	wt_sha1=$(readlink "$workdir/$file" | hash-object --stdin)

I cannot quite tell which from the patch and explanation.

Perhaps an additional test or two would help illustrate what issues
are being addressed better?

Thanks.

Right.  At first I thought I could revise the commit message to
make it clearer that we simply want to skip all symlinks, since
it never makes sense to reuse a worktree symlinks, but looking
at the tests and implementation makes me realize that it's not
that simple.

This is going to take a bit more time to get right.  John, I was
hoping you'd be able to take a look -- I'm playing catch-up too.
When it was first reported I let it sit for a while in hopes
that the original author would pickup the issue, but months
passed and I figured I'd take a stab at helping the user out.

Anyways, it'll take me a bit more time to understand the code
and work out a sensible solution.  My gut feeling is that we
should adjust the dir-diff feature so that it ignores all
symlinks.  That seems like a simple answer since we're deciding
to skip that chunk of complexity.

John, do you have any thoughts on how we can best handle this?
-- 
David

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help