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

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 27, 2015, 22:24 UTC
Message-ID: <xmqq1tcgne4u.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <1445981088-6285-1-git-send-email-davvid@gmail.com>

David Aguilar <davvid@gmail.com> writes:

Show 7 quoted lines

> 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.

Show 27 quoted lines

>  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