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

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 28, 2014, 19:42 UTC
Message-ID: <xmqq1tynuiqh.fsf@gitster.dls.corp.google.com>
In-Reply-To: <531061D1.2060206@alum.mit.edu>

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

Show 26 quoted lines

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