Re: [PATCH 2/2] parse-options.c: use "enum parse_opt_result" for parse_nodash_opt()

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Tue, Nov 09 2021, Junio C Hamano wrote:

> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
>
>> Change the parse_nodash_opt() function to use "enum
>> parse_opt_result". In 352e761388b (parse-options.[ch]: consistently
>> use "enum parse_opt_result", 2021-10-08) its only caller
>> parse_options_step() started using that return type, and the
>> get_value() which will be called and return from it uses the same
>> enum.
>>
>> Let's do the same here so that this function always returns an "enum
>> parse_opt_result" value.
>>
>> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>> ---
>>  parse-options.c | 7 ++++---
>>  1 file changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/parse-options.c b/parse-options.c
>> index fc5b43ff0b2..629e7963497 100644
>> --- a/parse-options.c
>> +++ b/parse-options.c
>> @@ -404,8 +404,9 @@ static enum parse_opt_result parse_long_opt(
>>  	return PARSE_OPT_UNKNOWN;
>>  }
>>  
>> -static int parse_nodash_opt(struct parse_opt_ctx_t *p, const char *arg,
>> -			    const struct option *options)
>> +static enum parse_opt_result parse_nodash_opt(struct parse_opt_ctx_t *p,
>> +					      const char *arg,
>> +					      const struct option *options)
>>  {
>>  	const struct option *all_opts = options;
>>  
>> @@ -415,7 +416,7 @@ static int parse_nodash_opt(struct parse_opt_ctx_t *p, const char *arg,
>>  		if (options->short_name == arg[0] && arg[1] == '\0')
>>  			return get_value(p, options, all_opts, OPT_SHORT);
>>  	}
>> -	return -2;
>> +	return PARSE_OPT_ERROR;
>>  }
>
> The current caller only checks to skip a token that yields 0 (aka
> PARSE_OPT_DONE) and does not distinguish between other values, so
> this won't change the behaviour of the current code, but it is 
> not clear if returning -1 (aka PARSE_OPT_ERROR) is better than -2
> (aka PARSE_OPT_HELP).

I think PARSE_OPT_ERROR is probably better.

It looks like the -2 return value might have been somewhat blindly
copy/pasted between 07fe54db3cd (parse-opt: do not print errors on
unknown options, return -2 intead., 2008-06-23) and 51a9949eda7
(parseopt: add PARSE_OPT_NODASH, 2009-05-07).

I.e. we use the full enum values for the code in the former, but in the
latter we're just looking for "not zero", so error/-1 seemed like a
better fit.