[PATCH] parse-options.c:parse_options_check() change OPTION_NUMBER to OPTION_CMDMODE

[PATCH] parse-options.c:parse_options_check() change OPTION_NUMBER to OPTION_CMDMODE

[Sun He]

Signed-off-by: Sun He <sunheehnus@gmail.com>
---
 parse-options.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/parse-options.c b/parse-options.c
index 7b8d3fa..59a52b0 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -371,7 +371,7 @@ static void parse_options_check(const struct option *opts)
 		case OPTION_NEGBIT:
 		case OPTION_SET_INT:
 		case OPTION_SET_PTR:
-		case OPTION_NUMBER:
+		case OPTION_CMDMODE:
 			if ((opts->flags & PARSE_OPT_OPTARG) ||
 			    !(opts->flags & PARSE_OPT_NOARG))
 				err |= optbug(opts, "should not accept an argument");
-- 
1.9.0.138.g2de3478.dirty

Hi,
When I was reading code yesterday, I came across this protential bug.
parse-options.h says that OPTION_CMDMODE is an option with no arguments and OPTION_NUMBER is special type option.

According to the information the program says (Should not accept an argument), I think you should take this patch into consideration.
Thanks.

He Sun

Re: [PATCH] parse-options.c:parse_options_check() change OPTION_NUMBER to OPTION_CMDMODE

[Michael Haggerty]

On 02/28/2014 10:07 AM, Sun He wrote:
> Signed-off-by: Sun He <sunheehnus@gmail.com>
> ---
>  parse-options.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/parse-options.c b/parse-options.c
> index 7b8d3fa..59a52b0 100644
> --- a/parse-options.c
> +++ b/parse-options.c
> @@ -371,7 +371,7 @@ static void parse_options_check(const struct option *opts)
>  		case OPTION_NEGBIT:
>  		case OPTION_SET_INT:
>  		case OPTION_SET_PTR:
> -		case OPTION_NUMBER:
> +		case OPTION_CMDMODE:
>  			if ((opts->flags & PARSE_OPT_OPTARG) ||
>  			    !(opts->flags & PARSE_OPT_NOARG))
>  				err |= optbug(opts, "should not accept an argument");
> 
> -- 
> 1.9.0.138.g2de3478.dirty
> 
> Hi,
> When I was reading code yesterday, I came across this protential bug.
> parse-options.h says that OPTION_CMDMODE is an option with no arguments and OPTION_NUMBER is special type option.
> 
> According to the information the program says (Should not accept an argument), I think you should take this patch into consideration.
> Thanks.
> 
> He Sun

Please resubmit this change in the proper format (as described by Eric
Sunshine WRT another one of your patches).  Also please remember to
distinguish between information that should go in the commit log
message, which will be stored permanently to the repository and help
future developers understand your change, vs. side notes meant only for
the mailing list.  For example, for the log message, stuff like:

    OPTION_CMDMODE should be used when ... So change the mode to
    OPTION_CMDMODE so that ...

vs. for the mailing list discussion:

    When I was reading code yesterday ...

The former goes above the "---" line and the latter goes directly below it.

BTW, none of my comments should be taken to indicate whether the commit
itself makes sense or not.  I haven't checked that far.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/

Re: [PATCH] parse-options.c:parse_options_check() change OPTION_NUMBER to OPTION_CMDMODE

[罗丹岳]

On Fri, Feb 28, 2014 at 6:15 PM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
>
> On 02/28/2014 10:07 AM, Sun He wrote:
> > Signed-off-by: Sun He <sunheehnus@gmail.com>
> > ---
> >  parse-options.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/parse-options.c b/parse-options.c
> > index 7b8d3fa..59a52b0 100644
> > --- a/parse-options.c
> > +++ b/parse-options.c
> > @@ -371,7 +371,7 @@ static void parse_options_check(const struct option *opts)
> >               case OPTION_NEGBIT:
> >               case OPTION_SET_INT:
> >               case OPTION_SET_PTR:
> > -             case OPTION_NUMBER:
> > +             case OPTION_CMDMODE:
> >                       if ((opts->flags & PARSE_OPT_OPTARG) ||
> >                           !(opts->flags & PARSE_OPT_NOARG))
> >                               err |= optbug(opts, "should not accept an argument");
> >
> > --
> > 1.9.0.138.g2de3478.dirty
> >
> > Hi,
> > When I was reading code yesterday, I came across this protential bug.
> > parse-options.h says that OPTION_CMDMODE is an option with no arguments and OPTION_NUMBER is special type option.
> >
> > According to the information the program says (Should not accept an argument), I think you should take this patch into consideration.
> > Thanks.
> >
> > He Sun
>
> Please resubmit this change in the proper format (as described by Eric
> Sunshine WRT another one of your patches).  Also please remember to
> distinguish between information that should go in the commit log
> message, which will be stored permanently to the repository and help
> future developers understand your change, vs. side notes meant only for
> the mailing list.  For example, for the log message, stuff like:
>
>     OPTION_CMDMODE should be used when ... So change the mode to
>     OPTION_CMDMODE so that ...
>
> vs. for the mailing list discussion:
>
>     When I was reading code yesterday ...
>
> The former goes above the "---" line and the latter goes directly below it.
>
> BTW, none of my comments should be taken to indicate whether the commit
> itself makes sense or not.  I haven't checked that far.
>
> Michael
>
> --
> Michael Haggerty
> mhagger@alum.mit.edu
> http://softwareswirl.blogspot.com/
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


>.<

Re: [PATCH] parse-options.c:parse_options_check() change OPTION_NUMBER to OPTION_CMDMODE

[Junio C Hamano]

Michael Haggerty <mhagger@alum.mit.edu> writes:

> On 02/28/2014 10:07 AM, Sun He wrote:
>> Signed-off-by: Sun He <sunheehnus@gmail.com>
>> ---
>>  parse-options.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/parse-options.c b/parse-options.c
>> index 7b8d3fa..59a52b0 100644
>> --- a/parse-options.c
>> +++ b/parse-options.c
>> @@ -371,7 +371,7 @@ static void parse_options_check(const struct option *opts)
>>  		case OPTION_NEGBIT:
>>  		case OPTION_SET_INT:
>>  		case OPTION_SET_PTR:
>> -		case OPTION_NUMBER:
>> +		case OPTION_CMDMODE:
>>  			if ((opts->flags & PARSE_OPT_OPTARG) ||
>>  			    !(opts->flags & PARSE_OPT_NOARG))
>>  				err |= optbug(opts, "should not accept an argument");
>> 
>> -- 
>> 1.9.0.138.g2de3478.dirty
>> 
>> Hi,
>> When I was reading code yesterday, I came across this protential bug.
>> parse-options.h says that OPTION_CMDMODE is an option with no arguments and OPTION_NUMBER is special type option.

Please, no overlong line in the text part that makes things
unnecessarily hard to read.

I agree with all the comments in the message I am responding to.

> BTW, none of my comments should be taken to indicate whether the commit
> itself makes sense or not.  I haven't checked that far.

While I think it is sensible to make sure CMDMODE is not marked to
allow arguments, I do not understand why "special type" would imply
"it is allowed to be marked to take an argument".  Why is it a
good change to remove "case OPTION_NUMBER:" here?

Re: [PATCH] parse-options.c:parse_options_check() change OPTION_NUMBER to OPTION_CMDMODE

[He Sun]

2014-03-01 3:42 GMT+08:00 Junio C Hamano <gitster@pobox.com>:
> Michael Haggerty <mhagger@alum.mit.edu> writes:
>
>> On 02/28/2014 10:07 AM, Sun He wrote:
>>> Signed-off-by: Sun He <sunheehnus@gmail.com>
>>> ---
>>>  parse-options.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/parse-options.c b/parse-options.c
>>> index 7b8d3fa..59a52b0 100644
>>> --- a/parse-options.c
>>> +++ b/parse-options.c
>>> @@ -371,7 +371,7 @@ static void parse_options_check(const struct option *opts)
>>>              case OPTION_NEGBIT:
>>>              case OPTION_SET_INT:
>>>              case OPTION_SET_PTR:
>>> -            case OPTION_NUMBER:
>>> +            case OPTION_CMDMODE:
>>>                      if ((opts->flags & PARSE_OPT_OPTARG) ||
>>>                          !(opts->flags & PARSE_OPT_NOARG))
>>>                              err |= optbug(opts, "should not accept an argument");
>>>
>>> --
>>> 1.9.0.138.g2de3478.dirty
>>>
>>> Hi,
>>> When I was reading code yesterday, I came across this protential bug.
>>> parse-options.h says that OPTION_CMDMODE is an option with no arguments and OPTION_NUMBER is special type option.
>
> Please, no overlong line in the text part that makes things
> unnecessarily hard to read.
>
> I agree with all the comments in the message I am responding to.
>

OK, got it. Thanks.

>> BTW, none of my comments should be taken to indicate whether the commit
>> itself makes sense or not.  I haven't checked that far.
>
> While I think it is sensible to make sure CMDMODE is not marked to
> allow arguments, I do not understand why "special type" would imply
> "it is allowed to be marked to take an argument".  Why is it a
> good change to remove "case OPTION_NUMBER:" here?

The comments in parse-options.h (Line 10-16) says that CMDMODE is not
marked to allow arguments. And I am not sure if removing OPTION_NUMBER
is a proper way.