From: Tim Henigan Date: Tue, 20 Mar 2012 13:07:41 GMT Subject: Re: [PATCH 0/9 v2] difftool: teach command to perform directory diffs Message-ID: In-Reply-To: <7vhaxk9ui6.fsf@alter.siamese.dyndns.org> 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 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.