Re: [PATCH 1/9] difftool: parse options using Getopt::Long

From: David Aguilar <davvid@gmail.com>
Date: Mar 18, 2012, 03:29 UTC
Message-ID: <CAJDDKr5bvNK2Pd+-wbTNgZdui91-zs_qST3PWKnHVmp-vGYVqg@mail.gmail.com>
In-Reply-To: <CAFouetgpz__1q7F_a3LQoeypCVBXOyTPWmL0e9=rw7vV65d7_g@mail.gmail.com>

On Sat, Mar 17, 2012 at 6:54 AM, Tim Henigan <tim.henigan@gmail.com> wrote:

Show 44 quoted lines

> On Fri, Mar 16, 2012 at 11:21 PM, David Aguilar <davvid@gmail.com> wrote:
>> On Fri, Mar 16, 2012 at 6:57 PM, Tim Henigan <tim.henigan@gmail.com> 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