From: David Aguilar Date: Sun, 18 Mar 2012 03:29:45 GMT Subject: Re: [PATCH 1/9] difftool: parse options using Getopt::Long Message-ID: In-Reply-To: On Sat, Mar 17, 2012 at 6:54 AM, Tim Henigan wrote: > On Fri, Mar 16, 2012 at 11:21 PM, David Aguilar wrote: >> On Fri, Mar 16, 2012 at 6:57 PM, Tim Henigan wrote: >> >> I've also wanted to do the same in the past.  The one thing holding me >> back was this note from the perldocs: >> >> "If pass_through is also enabled, options processing will terminate at >> the first unrecognized option, or non-option, whichever comes first." >> >> http://search.cpan.org/~jv/Getopt-Long-2.38/lib/Getopt/Long.pm > > That sentence is listed under the documentation of the 'permute' > option.  The documentation of 'pass_through' states that: > > "Options that are unknown, ambiguous or supplied with an invalid > option value are passed through in @ARGV instead of being flagged as > errors. This makes it possible to write wrapper scripts that process > only part of the user supplied command line arguments, and pass the > remaining options to some other program. > > If require_order is enabled, options processing will terminate at the > first unrecognized option, or non-option, whichever comes first. > However, if permute is enabled instead, results can become confusing." > > So early termination would only be a problem if 'pass_through' was > enabled at the same time as 'require_order' or 'permute'.  To verify, > I confirmed that 'git difftool --cached --diff-dir' works as expected. > > >> Is this indeed the case?  I am a little ashamed that the difftool >> tests do not cover this area.  That would be a valuable first step >> towards exploring this approach, IMO. > > I will review the the test cases.  If this goes forward, I still need > to add test cases to confirm the new '--dir-diff' option. > > >> BTW, I hate @ARGV parsing loops just as much as anyone!  I was not >> ignorant of Getopt::Long, and no, I was not re-inventing the wheel for >> no reason.  The reason it was done that way was so that we can forward >> everything we don't know about to git-diff. > > I understand and was not implying anything different.  I simply > thought this would be a positive change. Hehehe, well apparently I was ignorant about the permute_order and pass_through interaction afterall ;-) Thanks for the careful reading of the docs. I should have done that sooner. I highly appreciate your rework of this stuff -- this cleanup is quite nice. ++Getopt::Long. I sent a patch testing the "forward options to diff" behavior sometime yesterday(?) so we should be good on that side. Testing the diff-diff stuff will be good. Thanks a lot for this, it's a sweet feature and I know the users I help will get a lot of mileage out of it. -- David