David Aguilar <davvid@xxxxxxxxx> 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@xxxxxxxxx>
> Signed-off-by: David Aguilar <davvid@xxxxxxxxx>
> ---
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.
> 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
--
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