Re: [PATCH] difftool: avoid symlinks when reusing worktree files
- From
David Aguilar <davvid@gmail.com>
- Date
- Oct 30, 2015, 07:28 UTC
- Message-ID
- <20151030072857.GA15031@gmail.com>
- In-Reply-To
- <xmqq4mh9sfl6.fsf@gitster.mtv.corp.google.com>
On Thu, Oct 29, 2015 at 11:19:01AM -0700, Junio C Hamano wrote:
Show 40 quoted lines
> 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.
Show 18 quoted lines
> 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