Re: [PATCH 8/9 v13] difftool: teach difftool to handle directory diffs

> On Sun, Apr 15, 2012 at 9:01 PM, David Aguilar <davvid@gmail.com> wrote:
>> On Sun, Apr 15, 2012 at 3:20 PM, David Aguilar <davvid@gmail.com> wrote:
>>> On Fri, Apr 13, 2012 at 9:36 AM, Tim Henigan <tim.henigan@gmail.com> wrote:
>>>
>>> I started testing this patch.  I started on the commit before what's
>>> in pu and then applied this patch:
>>>
>>> $ git checkout e9653615fafcbac6109da99fac4fa66b0b432048
>>> $ git am difftool.patch
>>>
>>> The basics work and I know folks will be really happy when this
>>> feature lands.  Folks have personally asked me for this feature in the
>>> past.  I dig it.  I'd also like to help pursue using symlinks sometime
>>> in the future if that sounds like a reasonable thing to you, but the
>>> stabilizing the existing implementation is more important right now.
>
> I appreciate everyone's patience as this feature continues to develop.
>  It has not gone as smoothly as I hoped ;)
>
>
>>> I ran into some issues when trying it against a few random commits.  I
>>> went pretty far back in git's history to see what would happen.
>>>
>>> $ git difftool --dir-diff e5b06629de847663aaf0f7daae8de81338da3901 | tail
>>> Use of uninitialized value $rmode in string eq at
>>> /home/david/src/git/git-difftool line 96.
>
> I ran the same test using both my local branch [1] and the tip of
> Junio's th/difftool-diffall branch [2].   In both cases, I see the
> "RelNotes: no such file" error (more on that below), but I do not see
> uninitialized value errors.
>
> The section of code that reads the mode, SHA1 and path info from the
> diff output looks like this:
>
>    my @rawdiff = split('\0', $diffrtn);
>
>    for (my $i=0; $i<$#rawdiff; $i+=2) {
>        my ($lmode, $rmode, $lsha1, $rsha1, $status) = split(' ',
> substr($rawdiff[$i], 1));
>        ...
>
> If the "$lmode, ..." variables are not defined, then the output of
> 'git diff --raw' is malformed in some way (perhaps some error?).  At
> the very end of your output, I saw this as well:
>
>>> fatal: malformed index info /t9800-git-p4-basic.sh      :100755 100755
>>> a25f18d36a196a4b85f6cac15a6a081744fe8fa1
>>> d41470541650590355bf0de1a1b556b3502492b5 M
>>> update-index -z --index-info: command returned error: 128
>
> Would it be possible for you to try either Junio's or my branch to
> insure something did not go wrong with your locally applied patch?
> Also, which platform are you testing on?

>> xxdiff still gives an error message about "$tmp/left/RelNotes: No such
>> file or directory" with --no-renames so we may want to touch some
>> dummy files to make the tools happy.
>
> I get the same error.  I looked into it and found that "RelNotes" is a
> symbolic link.  If we look at the standard diff of the example you
> gave, we see the following:
>
>    $ git diff e5b0662 -- RelNotes
>    diff --git a/RelNotes b/RelNotes
>    index 7d92769..2c2a169 120000
>    --- a/RelNotes
>    +++ b/RelNotes
>    @@ -1 +1 @@
>    -Documentation/RelNotes/1.7.8.txt
>    \ No newline at end of file
>    +Documentation/RelNotes/1.7.10.txt
>    \ No newline at end of file
>
> When 'git-difftool' executes this command, it sees that the "RelNotes"
> file changed, but it is not smart enough to copy the link target.  So
> the "/tmp/git.diffall.XXXXX/left" directory has "RelNotes" in it, but
> not the target of the link "Documentation/RelNotes/1.7.8.txt".
>
> In xxdiff, this manifests as a "file not found" error.  In meld, it is
> shown as a "Dangling symlink".
>
> I will look into ways to deal with this, probably adding special logic
> to deal with file modes of "120000".