[v2] parse-options.c: use "enum parse_opt_result" for parse_nodash_opt()

Commit Message

Ævar Arnfjörð Bjarmason Nov. 10, 2021, 1:27 a.m. UTC

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 <[e-mail]>
---

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 <[e-mail]>
     
      ## parse-options.c ##

 parse-options.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

Comments

Junio C Hamano Nov. 11, 2021, 2:01 a.m. UTC | #1

Ævar Arnfjörð Bjarmason  <[e-mail]> 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.

Patch

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)