Re: [PATCH 0/9 v2] difftool: teach command to perform directory diffs
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Mar 20, 2012, 05:52 UTC
- Message-ID
- <7v4ntj95ub.fsf@alter.siamese.dyndns.org>
- In-Reply-To
- <CAJDDKr74cLSfj9JCeQQcm8U_26Re0u0nueoQ3nvj0O4FWBsqxg@mail.gmail.com>
David Aguilar <davvid@gmail.com> writes:
Show 20 quoted lines
> On Mon, Mar 19, 2012 at 2:00 PM, Junio C Hamano <gitster@pobox.com> 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.
Show 7 quoted lines
>> * 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.