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

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

[Toralf Förster]

 very first line is "error: malformed object name <id>" which tells all, or ?

-- 
Toralf, pgp: C4EACDDE 0076E94E

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

[Jeff King]

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)

  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.

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

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

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

[Chirayu Desai]

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?

Thanks,
Chirayu Desai

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

[Pranit Bauva]

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 .

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

[Chirayu Desai]

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:
> 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 .

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

[Jeff King]

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".

-Peff

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

[Chirayu Desai]

On Sat, Mar 19, 2016 at 11:27 PM, Jeff King <peff@peff.net> wrote:
> 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.
>
>> > 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

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

[Jeff King]

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

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

[Chirayu Desai]

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:
> 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

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

[Jeff King]

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.

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

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

[Chirayu Desai]

On Thu, Mar 24, 2016 at 4:11 AM, Jeff King <peff@peff.net> wrote:
> 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.
>
> 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.

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

[Junio C Hamano]

Jeff King <peff@peff.net> 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.