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

On Mon, Mar 19, 2012 at 2:00 PM, Junio C Hamano [off-list ref] wrote:

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.

 * 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