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

From: Chirayu Desai <chirayudesai1@gmail.com>
Date: Mar 19, 2016, 17:51 UTC
Message-ID: <CAJj6+1Fgj7VyVSSi2Qy=yEtEjuQWb7A8GDX-mY7h3V_mnRY2bw@mail.gmail.com>
In-Reply-To: <CAFZEwPP1GwH6a1kLTCn6ETov6YeK-t9PFJ_-wWP2P6v7CObiGQ@mail.gmail.com>

The original discussion [1] (the e-mail to which Jeff replied) doesn't contain much either, but I'll try to explain it.

$ git tag --contains q error: malformed object name q usage: git tag ... <entire usage text, printed on git tag -help / -h>

The same happens with 'git branch --contains', and 'git for-each-ref --contains`, as they use the same underlying code. Just showing the error message would be enough in this case.

The issue here is that the callback returns the "error: malformed object name %s" print, which parse_options sees as some sort of error and tries to help the user by printing the usage. [2]

One simple solution would be to change "return error("malformed object name %s", arg);" to a die("message"); return;, however it might not be the best option (though I'm not seeing any other user of this function, the only place it is being used is for OPT_CONTAINS and OPT_WITH

> 3. teach parse-options to accept some specific non-zero return code that means "return an error, but don't show the usage"

Would be a good general purpose alternative.

I need to look at this a bit more, and also get some hints / clarity from Jeff regarding 2. if possible.

Thanks, Chirayu Desai

[1] http://article.gmane.org/gmane.comp.version-control.git/284323 [2] https://github.com/git/git/blob/047057bb4159533b3323003f89160588c9e61fbd/parse-options-cb.c#L81

On Sat, Mar 19, 2016 at 10:34 PM, Pranit Bauva <pranit.bauva@gmail.com> wrote:

Show 38 quoted lines

> On Sat, Mar 19, 2016 at 10:19 PM, Chirayu Desai <chirayudesai1@gmail.com> wrote:
>> Hi, I want to work on this as my GSoC micro project.
>>
>>> On Mon, Jan 18, 2016 at 10:24:31PM +0100, Toralf Förster wrote:
>>> > very first line is "error: malformed object name <id>" which tells all, or ?
>>> 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.
>>> 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.
>>> 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.
>>> I think any one of those would be a good project for somebody looking to
>>> get their feet wet in working on git. I think (2) is the cleanest.
>>> -Peff
>>
>> What would be the best way to proceed with this?
>
> The extract that you posted isn't very clear.
> I guess posting a link with the previous discussion would be quite
> helpful as some people don't have the previous emails in the inbox.
> The archives can be found at
> http://dir.gmane.org/gmane.comp.version-control.git .