Re: [PATCH 0/9 v2] difftool: teach command to perform directory diffs

From: David Aguilar <davvid@gmail.com>
Date: Mar 20, 2012, 02:52 UTC
Message-ID: <CAJDDKr74cLSfj9JCeQQcm8U_26Re0u0nueoQ3nvj0O4FWBsqxg@mail.gmail.com>
In-Reply-To: <7vhaxk9ui6.fsf@alter.siamese.dyndns.org>

On Mon, Mar 19, 2012 at 2:00 PM, Junio C Hamano <gitster@pobox.com> wrote:

Show 11 quoted lines

> Thanks.
>
> I do not know a difftool user, and I wasn't paying close attention to the
> discussion but I recall these points raised and I do not recall the
> resolutions:
>
>  * In [1/9], use of pass_through would mean 'git difftool' must be fed the
>   options for difftool and then options meant for underlying diff
>   machinery.  Is this limitation still there?  If so, isn't this a
>   regression?  Shouldn't it at least be advertised to the users a lot
>   stronger in documentation?

Tim asserted that this is not the case. The options should pass through. Hopefully there aren't any behavior changes between perl versions and this option.

I sent a patch adding a test case to cover this scenario. I would prefer that we avoid a regression. If there's a regression then we can do without Getopt::Long, IMO.

I believe users should be oblivious as to which options are for difftool and which are for diff. E.g. `git difftool --cached -t xxdiff` -- the user does not care that --cached is for diff and -t is for difftool.

>  * In [4/9], you remove the .exe extension when running git, which was
>   there since the beginning of difftool 5c38ea3 (contrib: add 'git
>   difftool' for launching common merge tools, 2009-01-16).  I think it is
>   safe but are Windows folks OK with this?

I do not have Windows to test with, but this supposedly works since Git.pm does not use git.exe either. The git.exe stuff was originally there because difftool originally did exec('git.exe', ...). It was later changed to use system() and it's possible that we could have dropped the .exe extension at that time.

But, like I said, I don't have any Windows machines with which to verify this behavior, and highly appreciate feedback from Windows folks.

Show 5 quoted lines

>  * In [6/9], the exit code in the failure case seem to be modified from
>   that of the underlying "git diff" to whatever croak gives us.  Is this
>   a regression, or nobody cares error status from difftool?  I personally
>   suspect it is the latter, but just double-checking if you have
>   considered it.

This doesn't seem like too big of an issue. Had we documented the old exit codes then it would be a bigger concern. I would personally prefer to preserve the exit code from diff itself, but if that complicates it too much then the new behavior is ok.

>  * In [7/9], difftool--helper declares SUBDIRECTORY_OK, but there doesn't
>   seem to be any inclusion of git-sh-setup in this script, and the patch
>   does not have any effort to prepend $prefix to paths relative to $cwd.
>   What good does the variable do here?

I'll defer to Tim on this one. This seems like an oversight. It seems like something should be done to handle it.

I recall that we made $GIT_PREFIX available to aliases and builtins not too long ago. That may help here. See 1f5d271f5e8f7b1e2a5b296ff43ca4087eb08244.

Also.. I think we need some tests to cover the new behavior. A test to cover the subdirectory behavior would be especially helpful given the note about [7/9].

-- 
David