Subject: Re: "git tag --contains <id>" is too chatty, if <id> is invalid

From: Junio C Hamano <[email-masked]>
Date: Sun, 20 Mar 2016 15:25:04 -0700

Jeff King <[email-masked]> writes:

> On Sat, Mar 19, 2016 at 10:19:02PM +0530, Chirayu Desai wrote:
>
>> > Yeah, I agree that showing the "-h" help is a bit much.
>> > This is a side effect of looking up in the commit in the parse-options
>> > callback. It has to signal an error to the option parser, and then the
>> > option parser always shows the help on an error.
>> > I think we'd need to do one of:
>> > 1. call die() in the option-parsing callback (this is probably a bad
>> > precedent, as the callbacks might be reused from a place that wants
>> > to behave differently)
>> I assume you mean parse-options-cb.c:parse_opt_commits() by the callback.
>> I see that it is currently used only by commands which have a "--with"
>> or "--contains" option,
>> and all of them behave the same way, printing the full usage, so a one
>> line change in that function would fix it for all of those.
>
> Yes, that is the right callback.
>
>> > 2. have the callback just store the argument string, and then resolve
>> > the commit later (and die or whatever if it doesn't exist). This
>> > pushes more work onto the caller, but in this case it's all done by
>> > the ref-filter code, so it could presumably happen during another
>> > part of the ref-filter setup.
>> I'm not quire sure how exactly to do that.
>
> You'd teach parse_opt_commits() to store the string _name_ of the
> argument (e.g., using a string_list rather than a commit_list), and then
> later resolve those names into commits.
>
>> > 3. teach parse-options to accept some specific non-zero return code
>> > that means "return an error, but don't show the usage"
>> This sounds good, but also the most intrusive of 3.
>
> Yeah. Reading the options again, I kind of like this one. The only trick
> is that you would need to make sure no other callbacks are returning the
> value you choose for the "don't show the usage" flag. That is probably
> not too bad, though. There aren't that many callbacks, and they are not
> likely to be using values besides "-1" and "0".

My knee-jerk preference among the three is 2., but I think I'll know
when I see a patch ;-)

Thanks for helping the candidates.