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

[David Aguilar <davvid@gmail.com>]

On Thu, Oct 29, 2015 at 11:19:01AM -0700, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> 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@gmail.com>
> 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@gmail.com>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---

This is a very nicely worded commit message.  Thanks for the
thorough explanation.


>  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);
>  	}

The "-e _" shorthand caught my eye ~ I didn't know perl could do that!
Nice.

Underline is barely mentioned in perlvar, but it's obvious what
(I think) it means, and since Perl is DWIM, it must be right. ;-)
-- 
David