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

[David Aguilar <davvid@gmail.com>]

On Tue, Oct 27, 2015 at 03:24:49PM -0700, Junio C Hamano wrote:
> David Aguilar <davvid@gmail.com> 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 <gitster@pobox.com>
> > Signed-off-by: David Aguilar <davvid@gmail.com>
> > ---
> 
> 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