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

I have successfully tested on:
  - Linux with Git v1.7.10-rc0 and 1
  - Windows 7 with msysgit v1.7.9

I installed cygwin Git on Windows 7, but found that 'git difftool'
would not work, even before my patches were applied.  It appears that
I need to learn more about cygwin before I can use it as a test
platform.


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

 * 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?

Both the documentation and testing has shown that 'pass_through' works
as I expected.  That is, options that difftool cares about are
extracted from @ARGV while all others are passed on to the underlying
'git diff' command.  The order of the arguments/options does not
matter.  We "use 5.008;" at the top of the script, so I believe this
change is correct and safe.  That being said, I have a limited number
of computers/configurations to test on.

 * 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?

My testing on Windows with msysgit has shown that this change is safe.
 Also, even though I had other problems when testing on cygwin (noted
at the top of the email), the 'difftool' script was able to
successfully execute 'git diff --raw -z' without appending '.exe'.

 * 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.

In my testing, if "Git::command_noisy(('diff', @ARGV))" fails, the
exit code from 'git diff' is passed back to the terminal.  So not only
is the 'git diff' exit code echoed to the terminal, but '$?' also
contains the correct exit code.

 * 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?

This was an oversight.  It was copied over from a previous
implementation that still used ". git-sh-setup".

So the only outstanding changes that I am planning are:
  1) Remove SUBDIRECTORY_OK
  2) Add tests for the new '--dir-diff' option

Would it be better to resend the complete patches series with these
changes or just add new patches to the end?

Please let me know if I missed anything else.