From: Junio C Hamano Date: Tue, 20 Mar 2012 05:52:44 GMT Subject: Re: [PATCH 0/9 v2] difftool: teach command to perform directory diffs Message-ID: <7v4ntj95ub.fsf@alter.siamese.dyndns.org> In-Reply-To: David Aguilar writes: > On Mon, Mar 19, 2012 at 2:00 PM, Junio C Hamano 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. Yeah. Does tonight's 'pu' pass for you? > I do not have Windows to test with, but this supposedly works since > Git.pm does not use git.exe either. Yeah, that matches my guess. >>  * 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. Well, it does not seem to dot-source git-sh-setup so it probably stays where it was launched, so in that case there is nothing that needs to be done, including SUBDIRECTORY_OK which nobody would look at IIRC. > 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]. Yeah, that makes sense.