threads / discuss / 41752

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

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

## tl;dr

10 messages between Mar 19, 2016 and Mar 24, 2016.

replies: 9people: 4as markdown or json

Chirayu Desai· Mar 19, 2016, 16:49 UTC · lore
Hi, I want to work on this as my GSoC micro project.
Show 10 quoted lines
> 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.

Show 5 quoted lines
> 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?

Thanks, Chirayu Desai

Pranit Bauva· Mar 19, 2016, 17:04 UTC · re: Chirayu Desai · lore
On Sat, Mar 19, 2016 at 10:19 PM, Chirayu Desai <chirayudesai1@gmail.com> wrote:
Show 31 quoted lines
> 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 .

Chirayu Desai· Mar 19, 2016, 17:51 UTC · re: Pranit Bauva · lore

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 .
Jeff King· Mar 19, 2016, 17:57 UTC · re: Chirayu Desai · lore
On Sat, Mar 19, 2016 at 10:19:02PM +0530, Chirayu Desai wrote:
Show 13 quoted lines
> > 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.
Show 6 quoted lines
> > 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".

-Peff
Chirayu Desai· Mar 19, 2016, 18:08 UTC · re: Jeff King · lore
On Sat, Mar 19, 2016 at 11:27 PM, Jeff King <peff@peff.net> wrote:
Show 28 quoted lines
> 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.

Gotcha, will need to figure out where exactly would those names be resolved, can do after following the code path a bit more, can do.

Show 10 quoted lines
>
>> > 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".
I'll try to go through that as well.
>
> -Peff
-Chirayu
Jeff King· Mar 19, 2016, 18:12 UTC · re: Chirayu Desai · lore
On Sat, Mar 19, 2016 at 11:38:09PM +0530, Chirayu Desai wrote:
Show 5 quoted lines
> > 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.
> Gotcha, will need to figure out where exactly would those names be
> resolved, can do after following the code path a bit more, can do.

Without looking too closely, I suspect you could do it as the first step in filter_refs(). Or alternatively, add a function to "finalize" the filter state before making queries of it.

-Peff
Chirayu Desai· Mar 20, 2016, 06:49 UTC · re: Jeff King · lore

I went for 3, and have sent a patch for that here - [PATCH/GSoC] parse-options: Add a new nousage opt However, it currently has one bug Running 'git tag --contains qq' twice will first show an error, then print qq, meaning that the first command creates the tag qq. Running 'git tag -l --contains qq' works fine. My first question is if 'git tag --contains' (without '-l') supposed to work? If not, then I would fix that bug, otherwise fix the bug my code introduced, and add tests for it.

-Chirayu
On Sat, Mar 19, 2016 at 11:42 PM, Jeff King <peff@peff.net> wrote:
Show 13 quoted lines
> On Sat, Mar 19, 2016 at 11:38:09PM +0530, Chirayu Desai wrote:
>
>> > 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.
>> Gotcha, will need to figure out where exactly would those names be
>> resolved, can do after following the code path a bit more, can do.
>
> Without looking too closely, I suspect you could do it as the first step
> in filter_refs(). Or alternatively, add a function to "finalize" the
> filter state before making queries of it.
>
> -Peff
Jeff King· Mar 23, 2016, 22:41 UTC · re: Chirayu Desai · lore
On Sun, Mar 20, 2016 at 12:19:46PM +0530, Chirayu Desai wrote:
Show 9 quoted lines
> I went for 3, and have sent a patch for that here - [PATCH/GSoC]
> parse-options: Add a new nousage opt
> However, it currently has one bug
> Running 'git tag --contains qq' twice will first show an error, then
> print qq, meaning that the first command creates the tag qq.
> Running 'git tag -l --contains qq' works fine.
> My first question is if 'git tag --contains' (without '-l') supposed to work?
> If not, then I would fix that bug, otherwise fix the bug my code
> introduced, and add tests for it.

Yes, "--contains" should imply "-l", and we should complain if there is an attempt to create a tag.

This seems to work with the tip of "master":
  $ git tag --contains v2.8.0-rc3
  v2.8.0-rc3
  v2.8.0-rc4
  $ git tag --contains qq
  error: malformed object name qq
  [...and then the usage...]
  $ git tag --contains HEAD qq
  fatal: --contains option is only allowed with -l.
  $ git rev-parse --verify qq
  fatal: Needed a single revision
but with your patch:
  $ git tag --contains qq
  error: malformed object name qq
  $ git rev-parse --verify qq
  e9cacb7f8231dd6616671f9bcdd0945043483064

So presumably we're not aborting the program when the options fail to parse, and it continues to process the "qq" as a tag to be created.

-Peff
Chirayu Desai· Mar 24, 2016, 17:22 UTC · re: Jeff King · lore
On Thu, Mar 24, 2016 at 4:11 AM, Jeff King <peff@peff.net> wrote:
Show 14 quoted lines
> On Sun, Mar 20, 2016 at 12:19:46PM +0530, Chirayu Desai wrote:
>
>> I went for 3, and have sent a patch for that here - [PATCH/GSoC]
>> parse-options: Add a new nousage opt
>> However, it currently has one bug
>> Running 'git tag --contains qq' twice will first show an error, then
>> print qq, meaning that the first command creates the tag qq.
>> Running 'git tag -l --contains qq' works fine.
>> My first question is if 'git tag --contains' (without '-l') supposed to work?
>> If not, then I would fix that bug, otherwise fix the bug my code
>> introduced, and add tests for it.
>
> Yes, "--contains" should imply "-l", and we should complain if there is
> an attempt to create a tag.
Right, makes sense.
Show 27 quoted lines
>
> This seems to work with the tip of "master":
>
>   $ git tag --contains v2.8.0-rc3
>   v2.8.0-rc3
>   v2.8.0-rc4
>
>   $ git tag --contains qq
>   error: malformed object name qq
>   [...and then the usage...]
>
>   $ git tag --contains HEAD qq
>   fatal: --contains option is only allowed with -l.
>
>   $ git rev-parse --verify qq
>   fatal: Needed a single revision
>
> but with your patch:
>
>   $ git tag --contains qq
>   error: malformed object name qq
>
>   $ git rev-parse --verify qq
>   e9cacb7f8231dd6616671f9bcdd0945043483064
>
> So presumably we're not aborting the program when the options fail to
> parse, and it continues to process the "qq" as a tag to be created.

Yep, it was because I was returning PARSE_OPT_DONE on case -3 in parse-options.c::parse_options_step Making it return -1 fixed that.

>
> -Peff
Thank you for the help, detailed explanations and code reviews.
Junio C Hamano· Mar 20, 2016, 22:25 UTC · re: Jeff King · lore
Jeff King <peff@peff.net> writes:
Show 38 quoted lines
> 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.

← back to recent threads