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

Ævar Arnfjörð Bjarmason  [off-list ref] writes:

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

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

Ævar Arnfjörð Bjarmason  [off-list ref] writes:

quoted

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

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.

We could go for either PARSE_OPT_HELP (-2) or PARSE_OPT_ERROR (-1)
here. The reason we ended up with "-2" is that in code added in
07fe54db3cd (parse-opt: do not print errors on unknown options, return
"-2" instead., 2008-06-23) we used that value in a meaningful way.

Then in 51a9949eda7 (parseopt: add PARSE_OPT_NODASH, 2009-05-07) the
use of "-2" was seemingly copy/pasted from parse_long_opt(), which was
the function immediately above the parse_nodash_opt() function added
in that commit.

Since we only care about whether the return value here is non-zero
let's use the more generic PARSE_OPT_ERROR.

Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---

Now with an updated commit message per the v1 discussion. I peeled off
the 1/2 patch as it's already on "master" as 06a199f38b5
(parse-options.[ch]: revert use of "enum" for parse_options(),
2021-11-09).

Range-diff against v1:
1:  057a9f81b47 < -:  ----------- parse-options.[ch]: revert use of "enum" for parse_options()
2:  aa6224b10f8 ! 1:  376f76bb44e parse-options.c: use "enum parse_opt_result" for parse_nodash_opt()
    @@ Commit message
         parse-options.c: use "enum parse_opt_result" for parse_nodash_opt()
     
         Change the parse_nodash_opt() function to use "enum
    -    parse_opt_result". In 352e761388b (parse-options.[ch]: consistently
    +    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
    @@ Commit message
         Let's do the same here so that this function always returns an "enum
         parse_opt_result" value.
     
    +    We could go for either PARSE_OPT_HELP (-2) or PARSE_OPT_ERROR (-1)
    +    here. The reason we ended up with "-2" is that in code added in
    +    07fe54db3cd (parse-opt: do not print errors on unknown options, return
    +    "-2" instead., 2008-06-23) we used that value in a meaningful way.
    +
    +    Then in 51a9949eda7 (parseopt: add PARSE_OPT_NODASH, 2009-05-07) the
    +    use of "-2" was seemingly copy/pasted from parse_long_opt(), which was
    +    the function immediately above the parse_nodash_opt() function added
    +    in that commit.
    +
    +    Since we only care about whether the return value here is non-zero
    +    let's use the more generic PARSE_OPT_ERROR.
    +
         Signed-off-by: Ævar Arnfjörð Bjarmason [off-list ref]
     
      ## parse-options.c ##

 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;
 }
 
 static void check_typos(const char *arg, const struct option *options)

-- 
2.34.0.rc1.741.gab7bfd97031

Ævar Arnfjörð Bjarmason  [off-list ref] writes:

Change the parse_nodash_opt() function to use "enum
parse_opt_result".In 352e761388b (parse-options.[ch]: consistently

s/.In/. In/; no need to resend for this.

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.
...
Since we only care about whether the return value here is non-zero
let's use the more generic PARSE_OPT_ERROR.

I do not see a reason why anybody may think that it is a sensible
thing for "this returns a value from an enum, not int, so make it
so" patch, which is not supposed to change the sematics, to do,
though.  It is even so since we know the current caller does not
care.  The need of the next caller may tell us what the reasonable
return value should be, but we do not know well enough to justify
changing the value.