patch, 10 partsfix bug, use existing enums

I have some feature changes planned for parse-options.[ch], including ones that allow us to delete some boilerplate (and sometimes buggy) code in built-ins by having the API do heavier lifting.

In adding anything to the API I've found it hard to deal with it using its own enums inconsistently, sometimes it's an "int", sometimes it's the "enum", and having the "default" cases makes it hard to assert that you've added things to all the right places.

2-6,7-10/10 is that rather straightforward conversion. 1,7/10 also have fixes to existing bugs that happened due to mixing up the enum fields in one way or the other.

Ævar Arnfjörð Bjarmason (10):
  parse-options.h: move PARSE_OPT_SHELL_EVAL between enums
  parse-options.[ch]: consistently use "enum parse_opt_flags"
  parse-options.[ch]: consistently use "enum parse_opt_result"
  parse-options.c: use exhaustive "case" arms for "enum parse_opt_type"
  parse-options.h: make the "flags" in "struct option" an enum
  parse-options.c: move optname() earlier in the file
  commit-graph: stop using optname()
  parse-options.[ch]: make opt{bug,name}() "static"
  parse-options tests: test optname() output
  parse-options: change OPT_{SHORT,UNSET} to an enum

 builtin/blame.c          |   3 +
 builtin/commit-graph.c   |   3 +-
 builtin/shortlog.c       |   3 +
 parse-options.c          | 116 ++++++++++++++++++++++++++-------------
 parse-options.h          |  26 ++++-----
 t/t0040-parse-options.sh |  42 +++++++++++++-
 6 files changed, 138 insertions(+), 55 deletions(-)

Fix a bad landmine of a bug which has been with us ever since PARSE_OPT_SHELL_EVAL was added in 47e9cd28f8a (parseopt: wrap rev-parse --parseopt usage for eval consumption, 2010-06-12).

It's an argument to parse_options() and should therefore be in "enum parse_opt_flags", but it was added to the per-option "enum parse_opt_option_flags" by mistake.

Therefore as soon as we'd have an enum member in the former that reached its value of "1 << 8" we'd run into a seemingly bizarre bug where that new option would turn on the unrelated PARSE_OPT_SHELL_EVAL in "git rev-parse --parseopt" by proxy.

I manually checked that no other enum members suffered from such overlap, by setting the values to non-overlapping values, and making the relevant codepaths BUG() out if the given value was above/below the expected (excluding flags=0 in the case of "enum parse_opt_flags").

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 parse-options.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Use the "enum parse_opt_flags" instead of an "int flags" as arguments to the various functions in parse-options.c. This will help to catch cases where we're not handling cases in switch statements, and generally make it obvious which "flags" we're referring to in this case.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 parse-options.c | 13 ++++++++-----
 parse-options.h |  6 ++++--
 2 files changed, 12 insertions(+), 7 deletions(-)

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

I am not sure about most part this change, and the claim the second sentence makes is certainly dubious. Let's look at the first hunk:

 static void parse_options_start_1(struct parse_opt_ctx_t *ctx,
 				  int argc, const char **argv, const char *prefix,
-				  const struct option *options, int flags)
+				  const struct option *options,
+				  enum parse_opt_flags flags)
 {
 	ctx->argc = argc;
 	ctx->argv = argv;
 	if (!(flags & PARSE_OPT_ONE_SHOT)) {
 		ctx->argc--;
 		ctx->argv++;
 	}
 	ctx->total = ctx->argc;
 	ctx->out   = argv;
 	ctx->prefix = prefix;
 	ctx->cpidx = ((flags & PARSE_OPT_KEEP_ARGV0) != 0);
 	ctx->flags = flags;
 	if ((flags & PARSE_OPT_KEEP_UNKNOWN) &&
 	    (flags & PARSE_OPT_STOP_AT_NON_OPTION) &&
 	    !(flags & PARSE_OPT_ONE_SHOT))
 		BUG("STOP_AT_NON_OPTION and KEEP_UNKNOWN don't go together");
 	if ((flags & PARSE_OPT_ONE_SHOT) &&
 	    (flags & PARSE_OPT_KEEP_ARGV0))
 		BUG("Can't keep argv0 if you don't have it");
 	parse_options_check(options);
 }
 
The "flags" parameter does not take a value that is an "enum" in the
usual "enumeration" sense at all.  Even though parse_opt_flags
defines 7 distinct "enum" values, each enumerated value is a small
unsigned integer with only single bit set, the caller can throw a
value that is not among these 7 by OR'ing them together.  We would
not sensibly do

	switch (flags) {
	case PARSE_OPT_KEEP_UNKNOWN:
		...

In general, I am not all that enthusiastic for such a(n) (ab)use of "enum" for bit patterns, much less than "enumerate all possible values to make sure compilers would help us catch missing logic".

The "parse_opt_result" enum is the "right" kind of enumeration that I can stand behind fully. The hunk related to that enum in this patch is quite reasonable and takes advantage of the fact that the enum is meant to be the enumeration of all possible values.

Compared to it, parse_opt_flags and parse_opt_option_flags, not really.

If we wanted to really clean up the latter two, perhaps we should define the bit (which *can* be made to a proper "here are the all possible values" enumeration), like this:

    enum parse_opt_flags_bit {
	PARSE_OPT_KEEP_DASHDASH_BIT = 0,
        PARSE_OPT_STOP_AT_NON_OPTION_BIT,
	PARSE_OPT_KEEP_ARGV0_BIT,
	...
	PARSE_OPT_SHELL_EVAL_BIT,
    };

and then update the users to use (1U << PARSE_OPT_$FLAG$_BIT), or drop the pretense that it is a good idea to use enum for bit pattern and use the CPP macro, i.e.

    #define PARSE_OPT_KEEP_DASHDASH (1U<<0)
    #define PARSE_OPT_STOP_AT_NON_OPTION (1U<<1)
    ...
    #define PARSE_OPT_SHELL_EVAL (1U<<6)

to make it clear that we do not mean these are "enumeration of possible values".

Thanks.

On Tue, Sep 28 2021, Junio C Hamano wrote:

I agree that it's not as nice as enums where the fields are mutually exclusive, since those can be checked via "case" arms, and this is "unchecked" bitfields.

So e.g. the bug I fixed in 01/10 would not be found by a compiler I have access to (and I don't think one currently exists).

But I think this is perfectly good use of enums, we use this enums-as-bitfields pattern in various other places, e.g. builtin/rebase.c's "flags", the "commit_graph_write_flags", "expire_reflog_flags" & "todo_item_flags", just to name a few from some quick grepping.

One advantage is that integrates nicely with some wider C tooling. E.g. before this series, starting "git stash show" under gdb and inspecting flags:

    (gdb) p flags
    $1 = 9

And after:

    (gdb) p flags
    $1 = (PARSE_OPT_KEEP_DASHDASH | PARSE_OPT_KEEP_UNKNOWN)

So the type information and bitfield-ness are retained.

Although you might argue that it leads you into a trap, as adding:

    flags |= PARSE_OPT_LASTARG_DEFAULT;

Will result in:

    (gdb) p flags
    $2 = (PARSE_OPT_KEEP_DASHDASH | PARSE_OPT_KEEP_UNKNOWN | PARSE_OPT_NO_INTERNAL_HELP)

I.e. it decodes the enum based on the int value & its known labels, and it just so happens that PARSE_OPT_LASTARG_DEFAULT has the same value as PARSE_OPT_NO_INTERNAL_HELP in an unrelated enum.

I'm not sure what the former suggestion here buys us, but the latter will drop the type information as noted above, i.e. you'll get a:

    (gdb) p flags
    $1 = 9

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

 ...

Many codepaths already misusing is not an excuse to add another ;-)

But ...

... this is a pleasant surprise---the last time I checked, debuggers were that clever to notice that the distinct values are names for individual bits. I can buy this as an argument for using enums for names for individual bits. For this to work, obviously, variable and struct members need to be given the appropriate type.

So I agree with the change in 2/10.

Except that one place there was a change related to a different enum that is a true enumeration in this step. It belongs to 3/10, I think. Also, the sales pitch for this step in the proposed commit log message needs rewriting---this will "not" help to catch cases where we're not handling cases in switch statements; if you are selling it because you think it will help debuggers and other tooling, let's describe it as such.

Even though I think debuggers are overrated ;-)

Thanks.

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

This part does not belong to this step or 03/10 (parse_opt_result). It belongs to 04/10 (drop default from switching on enum).

Use the "enum parse_opt_result" instead of an "int flags" as the return value of the applicable functions in parse-options.c.

This will help catch future bugs, such as the missing "case" arms in the two existing users of the API in "blame.c" and "shortlog.c". A third caller in 309be813c9b (update-index: migrate to parse-options API, 2010-12-01) was already checking for these.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/blame.c    |  3 +++
 builtin/shortlog.c |  3 +++
 parse-options.c    | 15 ++++++++-------
 parse-options.h    | 15 ++++++++-------
 4 files changed, 22 insertions(+), 14 deletions(-)

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

As I said, this one I am happy about. The previous step had a change related to this enum mixed in it (by mistake, I think), which belongs here instead.

Change code in get_value(), parse_options_check() etc. to do away with the "default" case in favor of exhaustively checking the relevant fields.

The added "return -1" is needed for the GCC version commented on inline, my local clang 11.0.1-2 does not require it. Let's add it for now to appease GCC.

The added "special types" etc. comments correspond to the relevant comments and ordering on the "enum parse_opt_type". Let's try to keep the same order and commentary as there where possible for clarity. This doesn't reach that end-state, and due to the different handling of options it's probably not worth it to get there, but let's match its ordering where it's easy to do so.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 parse-options.c | 43 ++++++++++++++++++++++++++++++++++++++-----
 parse-options.h |  2 +-
 2 files changed, 39 insertions(+), 6 deletions(-)

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

I am not sure if this is a good idea in the bigger picture.

If we know that unlisted cases should not happen, having "default: BUG()" without explicitly listing them is just as expressive as the result of this patch with much shorter code.

When a new enum member is added without adding corresponding processing of the new value, either way it will be caught as a BUG(). Removing "default: BUG()" does allow you to catch such an error at compilation time, and keeping it may prevent you from doing so, but in practice, you'd be adding test coverage for the new case, which means that, even if your compiler is not cooperating, your test suite addition will hit "default: BUG();" in such a case.

So ...

On Tue, Sep 28 2021, Junio C Hamano wrote:

Yes we'll catch them since these are loops over all options. But that assumes that:

1) You have a test that's stressing the relevant entry point

2) That you run all your tests, e.g. one of these entry points is the
   "git completion" one

I think listing the remaining enum arms is a small price to pay for having that all moved to compile-time.

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

I can sympathize with that point of view, since I used to think the same way, but I am not yet convinced. An example like this from your postimage, which doubles the size of a switch statement with empty case arms, demonstrates that it is not a "small" price.

Admittedly, the original switch statement is particularly bad, though ;-)

 		switch (opts->type) {
 		case OPTION_STRING:
 		case OPTION_FILENAME:
 		case OPTION_INTEGER:
 		case OPTION_MAGNITUDE:
 		case OPTION_CALLBACK:
 		case OPTION_BIT:
 		case OPTION_NEGBIT:
 		case OPTION_COUNTUP:
 		case OPTION_SET_INT:
 			has_unset_form = 1;
 			break;
-		default:
+		/* special types */
+		case OPTION_END:
+		case OPTION_GROUP:
+		case OPTION_NUMBER:
+		case OPTION_ALIAS:
+		/* options with no arguments */
+		case OPTION_BITOP:
+		/* options with arguments (usually) */
+		case OPTION_LOWLEVEL_CALLBACK:
 			break;
 		}

Change the "flags" members of "struct option" to refer to their corresponding "enum" defined earlier in the file.

The benefit of changing this to an enum isn't as great as with some "enum parse_opt_type" as we'll always check this as a bitfield, so we can't rely on the compiler checking "case" arms for us. But let's do it for consistency with the rest of the file.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 parse-options.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

For the same reason as my comment on [02/10], this is more or less "Meh" for me. If we wanted to make any change here, I'd probably suggest making it "unsigned" to clarify that this is a OR'ed bit patterns.

Stop using optname() in builtin/commit-graph.c to emit an error with the --max-new-filters option. This changes code added in 809e0327f57 (builtin/commit-graph.c: introduce '--max-new-filters=<n>', 2020-09-18).

See 9440b831ad5 (parse-options: replace opterror() with optname(), 2018-11-10) for why using optname() like this is considered bad, i.e. it's assembling human-readable output piecemeal, and the "option `X'" at the start can't be translated.

It didn't matter in this case, but this code was also buggy in its use of "opt->flags" to optname(), that function expects flags, but not *those* flags.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/commit-graph.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

On Tue, Sep 28, 2021 at 03:14:28PM +0200, Ævar Arnfjörð Bjarmason wrote:

In fact, using optname there (which blames to me) was a mistake for an even simpler reason: there is no abbreviated form of `--max-new-filters`, and we know that by the time this error is emitted that we got the positive form instead of `--no-max-new-filters`.

So we could have just written the option's name verbatim, and given translators something easier to work with.

Makes sense. The `'-style quoting is still weird to me. It is consistent with some of the conversions in 9440b831ad5, but most importantly with how parse-options.c:get_value() behaves (because it calls optname underneath).

(This has nothing to do with your patch, but I thought the custom write_option_max_new_filters callback was weird when I wrote it. It's working around trying to make the negated form set a value to `-1` instead of `0`. But it's an annoying hack, because we have to call strtol() ourselves when we're not negated. *sigh*).

Looks good to me.

Thanks, Taylor

On Wed, Sep 29 2021, Taylor Blau wrote:

As an aside: Did you intend for this to work:

    git commit-graph write --max-new-filters=123 --no-max-new-filters

It's in the docstring, but then you're using OPT_CALLBACK_F(), but just to set a flag of "0", so a OPT_CALLBACK() would do, along with a PARSE_OPT_NONEG.

I'm about to re-roll this, but won't change that, but I think it probably makes sense as a follow-on cleanuup.

I think you'd probably want a BUG_ON_OPT_NEG() instead for the "unset" handling here. This seems like another case of mixing the state of parse_options() with that of flags for the underlying API that we discussed elsewhere either for this command or multi-pack-index. But more on that below...

Also the usage if --no-* is wanted should not be:

    [--no-max-new-filters] [--max-new-filters <n>]

But is currently:

    [--[no-]max-new-filters <n>]

Which says the --no-* will take the <n>, but it won't.

Yeah I just reproduced the existing output here.

...on the "more on that below", looks like you intended that -1 handling in some way, but I don't really see why yet, other than the "mixing the state" I noted above.

In preparation for making "optname" a static function move it above its first user in parse-options.c.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 parse-options.c | 30 +++++++++++++++---------------
 1 file changed, 15 insertions(+), 15 deletions(-)

Change these two functions to "static", the last user of "optname()" outside of parse-options.c itself went away in the preceding commit, for the reasons noted in 9440b831ad5 (parse-options: replace opterror() with optname(), 2018-11-10) we shouldn't be adding any more users of it.

The "optbug()" function was never used outside of parse-options.c, but was made non-static in 1f275b7c4ca (parse-options: export opterr, optbug, 2011-08-11). I think the only external user of optname() was the commit-graph.c caller added in 09e0327f57 (builtin/commit-graph.c: introduce '--max-new-filters=<n>', 2020-09-18).

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 parse-options.c | 4 ++--
 parse-options.h | 3 ---
 2 files changed, 2 insertions(+), 5 deletions(-)

There were no tests for checking the specific output that we'll generate in optname(), let's add some. That output was added back in 4a59fd13122 (Add a simple option parser., 2007-10-15).

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t0040-parse-options.sh | 42 +++++++++++++++++++++++++++++++++++++---
 1 file changed, 39 insertions(+), 3 deletions(-)

Change the comparisons against OPT_SHORT and OPT_UNSET to an enum which keeps track of how a given option got parsed. The case of "0" was an implicit OPT_LONG, so let's add an explicit label for it.

Due to the xor in 0f1930c5875 (parse-options: allow positivation of options starting, with no-, 2012-02-25) the code already relied on this being set back to 0. To avoid refactoring the logic involved in that let's just start the enum at "0" instead of the usual "1<<0" (1), but BUG() out if we don't have one of our expected flags.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 parse-options.c | 23 ++++++++++++++---------
 1 file changed, 14 insertions(+), 9 deletions(-)

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

I think this is going backwards. If we want to change anything in this area, you may

 (1) spell the values not as 1 and 2 but as bit-shifts (1U<<0),
     (1U<<1), and

 (2) turn "flags" into "unsigned".

but I do not think of a reason why it would be a good move to use enum for "these are bits", for the same reason as my comments on [2/10].

Thanks.

I have some feature changes planned for parse-options.[ch], including ones that allow us to delete some boilerplate (and sometimes buggy) code in built-ins by having the API do heavier lifting.

In adding anything to the API I've found it hard to deal with it using its own enums inconsistently, sometimes it's an "int", sometimes it's the "enum", and having the "default" cases makes it hard to assert that you've added things to all the right places.

This re-roll of the v1[1] should hopefully address all the feedback on that version, particularly the motivation for the enum-as-bitfield labeling, as expanded on in 02/11.

1. http://lore.kernel.org/git/cover-00.10-00000000000-20210928T130905Z-avarab@gmail.com

Ævar Arnfjörð Bjarmason (11):
  parse-options.h: move PARSE_OPT_SHELL_EVAL between enums
  parse-options.[ch]: consistently use "enum parse_opt_flags"
  parse-options.[ch]: consistently use "enum parse_opt_result"
  parse-options.c: use exhaustive "case" arms for "enum
    parse_opt_result"
  parse-options.c: use exhaustive "case" arms for "enum parse_opt_type"
  parse-options.h: make the "flags" in "struct option" an enum
  parse-options.c: move optname() earlier in the file
  commit-graph: stop using optname()
  parse-options.[ch]: make opt{bug,name}() "static"
  parse-options tests: test optname() output
  parse-options: change OPT_{SHORT,UNSET} to an enum

 builtin/blame.c          |   3 +
 builtin/commit-graph.c   |   3 +-
 builtin/shortlog.c       |   3 +
 parse-options.c          | 135 ++++++++++++++++++++++++++-------------
 parse-options.h          |  26 ++++----
 t/t0040-parse-options.sh |  42 +++++++++++-
 6 files changed, 151 insertions(+), 61 deletions(-)

Range-diff against v1:
 1:  19d1573a4e0 =  1:  553931702df parse-options.h: move PARSE_OPT_SHELL_EVAL between enums
 2:  289bb437eb5 !  2:  99f5251c557 parse-options.[ch]: consistently use "enum parse_opt_flags"
    @@ Commit message
         parse-options.[ch]: consistently use "enum parse_opt_flags"
     
         Use the "enum parse_opt_flags" instead of an "int flags" as arguments
    -    to the various functions in parse-options.c. This will help to catch
    -    cases where we're not handling cases in switch statements, and
    -    generally make it obvious which "flags" we're referring to in this
    -    case.
    +    to the various functions in parse-options.c.
    +
    +    In C enums aren't first-class types, and the "enum
    +    parse_opt_option_flag" uses a enum-as-bitfield pattern. So unlike
    +    exhaustively enumerated "case" arms we're not going to get validation
    +    that we used the "right" enum labels.
    +
    +    I.e. this won't catch the sort of bug that was fixed with
    +    "PARSE_OPT_SHELL_EVAL" in the preceding commit.
    +
    +    But there's still a benefit to doing this when it comes to the wider C
    +    ecosystem. E.g. the GNU debugger (gdb) will helpfully detect and print
    +    out meaningful enum labels in this case. Here's the output before and
    +    after when breaking in "parse_options()" after invoking "git stash
    +    show":
    +
    +    Before:
    +
    +        (gdb) p flags
    +        $1 = 9
    +
    +    After:
    +
    +        (gdb) p flags
    +        $1 = (PARSE_OPT_KEEP_DASHDASH | PARSE_OPT_KEEP_UNKNOWN)
    +
    +    Of course as noted in[1] there's a limit to this smartness,
    +    i.e. manually setting it with unrelated enum labels won't be
    +    caught. There are some third-party extensions to do more exhaustive
    +    checking[2], perhaps we'll be able to make use of them sooner than
    +    later.
    +
    +    We've also got prior art using this pattern in the codebase. See
    +    e.g. "enum bloom_filter_computed" added in 312cff52074 (bloom: split
    +    'get_bloom_filter()' in two, 2020-09-16) and the "permitted" enum
    +    added in ce910287e72 (add -p: fix checking of user input, 2020-08-17).
    +
    +    1. https://lore.kernel.org/git/87mtnvvj3c.fsf@evledraar.gmail.com/
    +    2. https://github.com/sinelaw/elfs-clang-plugins/blob/master/enums_conversion/README.md
     
         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
    @@ parse-options.c: int parse_options_end(struct parse_opt_ctx_t *ctx)
      {
      	struct parse_opt_ctx_t ctx;
      	struct option *real_options;
    -@@ parse-options.c: int parse_options(int argc, const char **argv, const char *prefix,
    - 	case PARSE_OPT_NON_OPTION:
    - 	case PARSE_OPT_DONE:
    - 		break;
    --	default: /* PARSE_OPT_UNKNOWN */
    -+	case PARSE_OPT_UNKNOWN:
    - 		if (ctx.argv[0][1] == '-') {
    - 			error(_("unknown option `%s'"), ctx.argv[0] + 2);
    - 		} else if (isascii(*ctx.opt)) {
     
      ## parse-options.h ##
     @@ parse-options.h: struct option {
 3:  fbcbaa47329 !  3:  be198e42df9 parse-options.[ch]: consistently use "enum parse_opt_result"
    @@ Commit message
         third caller in 309be813c9b (update-index: migrate to parse-options
         API, 2010-12-01) was already checking for these.
     
    +    As can be seen when trying to sort through the deluge of warnings
    +    produced when compiling this with CC=g++ (mostly unrelated to this
    +    change) we're not consistently using "enum parse_opt_result" even now,
    +    i.e. we'll return error() and "return 0;". See f41179f16ba
    +    (parse-options: avoid magic return codes, 2019-01-27) for a commit
    +    which started changing some of that.
    +
    +    I'm not doing any more of that exhaustive migration here, and it's
    +    probably not worthwhile past the point of being able to check "enum
    +    parse_opt_result" in switch().
    +
         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
      ## builtin/blame.c ##
    @@ builtin/shortlog.c: int cmd_shortlog(int argc, const char **argv, const char *pr
      			exit(129);
     
      ## parse-options.c ##
    -@@ parse-options.c: static int usage_with_options_internal(struct parse_opt_ctx_t *,
    - 				       const char * const *,
    - 				       const struct option *, int, int);
    +@@ parse-options.c: static void free_preprocessed_options(struct option *options)
    + 	free(options);
    + }
      
    +-static int usage_with_options_internal(struct parse_opt_ctx_t *,
    +-				       const char * const *,
    +-				       const struct option *, int, int);
    +-
     -int parse_options_step(struct parse_opt_ctx_t *ctx,
     -		       const struct option *options,
     -		       const char * const usagestr[])
    ++static enum parse_opt_result usage_with_options_internal(struct parse_opt_ctx_t *,
    ++							 const char * const *,
    ++							 const struct option *,
    ++							 int, int);
    ++
     +enum parse_opt_result parse_options_step(struct parse_opt_ctx_t *ctx,
     +					 const struct option *options,
     +					 const char * const usagestr[])
    @@ parse-options.c: int parse_options_end(struct parse_opt_ctx_t *ctx)
      {
      	struct parse_opt_ctx_t ctx;
      	struct option *real_options;
    +@@ parse-options.c: static int usage_argh(const struct option *opts, FILE *outfile)
    + #define USAGE_OPTS_WIDTH 24
    + #define USAGE_GAP         2
    + 
    +-static int usage_with_options_internal(struct parse_opt_ctx_t *ctx,
    +-				       const char * const *usagestr,
    +-				       const struct option *opts, int full, int err)
    ++static enum parse_opt_result usage_with_options_internal(struct parse_opt_ctx_t *ctx,
    ++							 const char * const *usagestr,
    ++							 const struct option *opts,
    ++							 int full, int err)
    + {
    + 	FILE *outfile = err ? stderr : stdout;
    + 	int need_newline;
     
      ## parse-options.h ##
     @@ parse-options.h: struct option {
 -:  ----------- >  4:  a253db7a60d parse-options.c: use exhaustive "case" arms for "enum parse_opt_result"
 4:  624a19000e1 !  5:  467828780d0 parse-options.c: use exhaustive "case" arms for "enum parse_opt_type"
    @@ Commit message
         handling of options it's probably not worth it to get there, but let's
         match its ordering where it's easy to do so.
     
    +    There was a discussion about whether this was worth the added
    +    verbosity, as argued in[1] I think it's worth it for getting
    +    compile-time checking when adding new option types. We *should* have
    +    tests for some of these, but e.g. in the show_gitcomp() case one might
    +    run through the whole test suite and only hit a missing case at the
    +    end on the completion tests.
    +
    +    This technically changes the handling of OPTION_END, but it's
    +    obviously the right thing to do. We're calling this code from within a
    +    loop that uses OPTION_END as a break condition, so it was never caught
    +    by the "default" case.
    +
    +    So let's make encountering OPTION_END a BUG(), just like it already is
    +    in the get_value() handling added in 4a59fd13122 (Add a simple option
    +    parser., 2007-10-15).
    +
    +    1. https://lore.kernel.org/git/87tui3vk8y.fsf@evledraar.gmail.com/
    +
         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
      ## parse-options.c ##
    @@ parse-options.c: static enum parse_opt_result get_value(struct parse_opt_ctx_t *
      }
      
      static enum parse_opt_result parse_short_opt(struct parse_opt_ctx_t *p,
    +@@ parse-options.c: static void parse_options_check(const struct option *opts)
    + 			err |= optbug(opts, "uses feature "
    + 					"not supported for dashless options");
    + 		switch (opts->type) {
    ++		case OPTION_END:
    ++			BUG("unreachable");
    ++
    + 		case OPTION_COUNTUP:
    + 		case OPTION_BIT:
    + 		case OPTION_NEGBIT:
     @@ parse-options.c: static void parse_options_check(const struct option *opts)
      			BUG("OPT_ALIAS() should not remain at this point. "
      			    "Are you using parse_options_step() directly?\n"
    @@ parse-options.c: static void parse_options_check(const struct option *opts)
     -			; /* ok. (usually accepts an argument) */
     +
     +		case OPTION_BITOP:
    -+		case OPTION_END:
     +		case OPTION_FILENAME:
     +		case OPTION_GROUP:
     +		case OPTION_INTEGER:
    @@ parse-options.c: static void parse_options_check(const struct option *opts)
      		}
      		if (opts->argh &&
      		    strcspn(opts->argh, " _") != strlen(opts->argh))
    +@@ parse-options.c: static void show_negated_gitcomp(const struct option *opts, int show_all,
    + 			continue;
    + 
    + 		switch (opts->type) {
    ++		case OPTION_END:
    ++			BUG("unreachable");
    ++
    + 		case OPTION_STRING:
    + 		case OPTION_FILENAME:
    + 		case OPTION_INTEGER:
     @@ parse-options.c: static void show_negated_gitcomp(const struct option *opts, int show_all,
      		case OPTION_SET_INT:
      			has_unset_form = 1;
      			break;
     -		default:
     +		/* special types */
    -+		case OPTION_END:
     +		case OPTION_GROUP:
     +		case OPTION_NUMBER:
     +		case OPTION_ALIAS:
    @@ parse-options.c: static void show_negated_gitcomp(const struct option *opts, int
      			break;
      		}
      		if (!has_unset_form)
    +@@ parse-options.c: static int show_gitcomp(const struct option *opts, int show_all)
    + 			continue;
    + 
    + 		switch (opts->type) {
    ++		case OPTION_END:
    ++			BUG("unreachable");
    + 		case OPTION_GROUP:
    + 			continue;
    + 		case OPTION_STRING:
     @@ parse-options.c: static int show_gitcomp(const struct option *opts, int show_all)
      				break;
      			suffix = "=";
      			break;
     -		default:
     +		/* special types */
    -+		case OPTION_END:
     +		case OPTION_NUMBER:
     +		case OPTION_ALIAS:
     +
 5:  697e432c012 =  6:  34669327550 parse-options.h: make the "flags" in "struct option" an enum
 6:  c065f7d7362 =  7:  e82a4e477d5 parse-options.c: move optname() earlier in the file
 7:  b0b313795c7 =  8:  58683b3d89d commit-graph: stop using optname()
 8:  07ba0e6f995 =  9:  8cbee660174 parse-options.[ch]: make opt{bug,name}() "static"
 9:  ce508fddc8f = 10:  f727f683eb1 parse-options tests: test optname() output
10:  a28ab961125 = 11:  4fbc07fc7fd parse-options: change OPT_{SHORT,UNSET} to an enum

Fix a bad landmine of a bug which has been with us ever since PARSE_OPT_SHELL_EVAL was added in 47e9cd28f8a (parseopt: wrap rev-parse --parseopt usage for eval consumption, 2010-06-12).

It's an argument to parse_options() and should therefore be in "enum parse_opt_flags", but it was added to the per-option "enum parse_opt_option_flags" by mistake.

Therefore as soon as we'd have an enum member in the former that reached its value of "1 << 8" we'd run into a seemingly bizarre bug where that new option would turn on the unrelated PARSE_OPT_SHELL_EVAL in "git rev-parse --parseopt" by proxy.

I manually checked that no other enum members suffered from such overlap, by setting the values to non-overlapping values, and making the relevant codepaths BUG() out if the given value was above/below the expected (excluding flags=0 in the case of "enum parse_opt_flags").

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 parse-options.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Use the "enum parse_opt_flags" instead of an "int flags" as arguments to the various functions in parse-options.c.

In C enums aren't first-class types, and the "enum parse_opt_option_flag" uses a enum-as-bitfield pattern. So unlike exhaustively enumerated "case" arms we're not going to get validation that we used the "right" enum labels.

I.e. this won't catch the sort of bug that was fixed with "PARSE_OPT_SHELL_EVAL" in the preceding commit.

But there's still a benefit to doing this when it comes to the wider C ecosystem. E.g. the GNU debugger (gdb) will helpfully detect and print out meaningful enum labels in this case. Here's the output before and after when breaking in "parse_options()" after invoking "git stash show":

Before:

    (gdb) p flags
    $1 = 9

After:

    (gdb) p flags
    $1 = (PARSE_OPT_KEEP_DASHDASH | PARSE_OPT_KEEP_UNKNOWN)

Of course as noted in[1] there's a limit to this smartness, i.e. manually setting it with unrelated enum labels won't be caught. There are some third-party extensions to do more exhaustive checking[2], perhaps we'll be able to make use of them sooner than later.

We've also got prior art using this pattern in the codebase. See e.g. "enum bloom_filter_computed" added in 312cff52074 (bloom: split 'get_bloom_filter()' in two, 2020-09-16) and the "permitted" enum added in ce910287e72 (add -p: fix checking of user input, 2020-08-17).

1. https://lore.kernel.org/git/87mtnvvj3c.fsf@evledraar.gmail.com/
2. https://github.com/sinelaw/elfs-clang-plugins/blob/master/enums_conversion/README.md

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 parse-options.c | 11 +++++++----
 parse-options.h |  6 ++++--
 2 files changed, 11 insertions(+), 6 deletions(-)

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

OK.

Drop the two paragraphs above. You do not sell a patch by saying what benefit it does *not* give us. We do buy a patch by what benefit it does give us, which you describe well below.

Use the "enum parse_opt_result" instead of an "int flags" as the return value of the applicable functions in parse-options.c.

This will help catch future bugs, such as the missing "case" arms in the two existing users of the API in "blame.c" and "shortlog.c". A third caller in 309be813c9b (update-index: migrate to parse-options API, 2010-12-01) was already checking for these.

As can be seen when trying to sort through the deluge of warnings produced when compiling this with CC=g++ (mostly unrelated to this change) we're not consistently using "enum parse_opt_result" even now, i.e. we'll return error() and "return 0;". See f41179f16ba (parse-options: avoid magic return codes, 2019-01-27) for a commit which started changing some of that.

I'm not doing any more of that exhaustive migration here, and it's probably not worthwhile past the point of being able to check "enum parse_opt_result" in switch().

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/blame.c    |  3 +++
 builtin/shortlog.c |  3 +++
 parse-options.c    | 31 +++++++++++++++++--------------
 parse-options.h    | 15 ++++++++-------
 4 files changed, 31 insertions(+), 21 deletions(-)

Change the "default" case in parse_options() that handles the return value of parse_options_step() to simply have a "case" arm for PARSE_OPT_UNKNOWN, instead of leaving it to a comment. This means the compiler can warn us about any missing case arms.

This adjusts code added in ff43ec3e2d2 (parse-opt: create parse_options_step., 2008-06-23), given its age it may pre-date the existence (or widespread use) of this coding style, which we've since adopted more widely.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 parse-options.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Change code in get_value(), parse_options_check() etc. to do away with the "default" case in favor of exhaustively checking the relevant fields.

The added "return -1" is needed for the GCC version commented on inline, my local clang 11.0.1-2 does not require it. Let's add it for now to appease GCC.

The added "special types" etc. comments correspond to the relevant comments and ordering on the "enum parse_opt_type". Let's try to keep the same order and commentary as there where possible for clarity. This doesn't reach that end-state, and due to the different handling of options it's probably not worth it to get there, but let's match its ordering where it's easy to do so.

There was a discussion about whether this was worth the added verbosity, as argued in[1] I think it's worth it for getting compile-time checking when adding new option types. We *should* have tests for some of these, but e.g. in the show_gitcomp() case one might run through the whole test suite and only hit a missing case at the end on the completion tests.

This technically changes the handling of OPTION_END, but it's obviously the right thing to do. We're calling this code from within a loop that uses OPTION_END as a break condition, so it was never caught by the "default" case.

So let's make encountering OPTION_END a BUG(), just like it already is in the get_value() handling added in 4a59fd13122 (Add a simple option parser., 2007-10-15).

1. https://lore.kernel.org/git/87tui3vk8y.fsf@evledraar.gmail.com/

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 parse-options.c | 48 +++++++++++++++++++++++++++++++++++++++++++-----
 parse-options.h |  2 +-
 2 files changed, 44 insertions(+), 6 deletions(-)

Change the "flags" members of "struct option" to refer to their corresponding "enum" defined earlier in the file.

The benefit of changing this to an enum isn't as great as with some "enum parse_opt_type" as we'll always check this as a bitfield, so we can't rely on the compiler checking "case" arms for us. But let's do it for consistency with the rest of the file.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 parse-options.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

In preparation for making "optname" a static function move it above its first user in parse-options.c.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 parse-options.c | 30 +++++++++++++++---------------
 1 file changed, 15 insertions(+), 15 deletions(-)

Stop using optname() in builtin/commit-graph.c to emit an error with the --max-new-filters option. This changes code added in 809e0327f57 (builtin/commit-graph.c: introduce '--max-new-filters=<n>', 2020-09-18).

See 9440b831ad5 (parse-options: replace opterror() with optname(), 2018-11-10) for why using optname() like this is considered bad, i.e. it's assembling human-readable output piecemeal, and the "option `X'" at the start can't be translated.

It didn't matter in this case, but this code was also buggy in its use of "opt->flags" to optname(), that function expects flags, but not *those* flags.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/commit-graph.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

Am 01.10.21 um 16:29 schrieb Ævar Arnfjörð Bjarmason:

The option name itself is untranslatable. parse_opt_abbrev_cb() has code like this:

			return error(_("option `%s' expects a numerical value"),
				     opt->long_name);

This would work here as well.  Advantage: One less string to translate.

Change these two functions to "static", the last user of "optname()" outside of parse-options.c itself went away in the preceding commit, for the reasons noted in 9440b831ad5 (parse-options: replace opterror() with optname(), 2018-11-10) we shouldn't be adding any more users of it.

The "optbug()" function was never used outside of parse-options.c, but was made non-static in 1f275b7c4ca (parse-options: export opterr, optbug, 2011-08-11). I think the only external user of optname() was the commit-graph.c caller added in 09e0327f57 (builtin/commit-graph.c: introduce '--max-new-filters=<n>', 2020-09-18).

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 parse-options.c | 4 ++--
 parse-options.h | 3 ---
 2 files changed, 2 insertions(+), 5 deletions(-)

There were no tests for checking the specific output that we'll generate in optname(), let's add some. That output was added back in 4a59fd13122 (Add a simple option parser., 2007-10-15).

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t0040-parse-options.sh | 42 +++++++++++++++++++++++++++++++++++++---
 1 file changed, 39 insertions(+), 3 deletions(-)

Change the comparisons against OPT_SHORT and OPT_UNSET to an enum which keeps track of how a given option got parsed. The case of "0" was an implicit OPT_LONG, so let's add an explicit label for it.

Due to the xor in 0f1930c5875 (parse-options: allow positivation of options starting, with no-, 2012-02-25) the code already relied on this being set back to 0. To avoid refactoring the logic involved in that let's just start the enum at "0" instead of the usual "1<<0" (1), but BUG() out if we don't have one of our expected flags.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 parse-options.c | 23 ++++++++++++++---------
 1 file changed, 14 insertions(+), 9 deletions(-)

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

Other than [3] and [4] that I am not yet convinced is a good idea, this round looks good to me.

Junio C Hamano <gitster@pobox.com> writes:

Not 3&4, but 4&5.

A re-roll of https://lore.kernel.org/git/cover-v2-00.11-00000000000-20211001T142631Z-avarab@gmail.com/; I dropped the addition of exhaustive enum arms for parse_opt_type.

There's other tweaks here pointed out by Junio & René, thanks both! The "enum parse_opt_flags flags" change in the range-diff below was there before, but it was (incorrectly, to begin with) in the now-dropped patch.

Ævar Arnfjörð Bjarmason (10):
  parse-options.h: move PARSE_OPT_SHELL_EVAL between enums
  parse-options.[ch]: consistently use "enum parse_opt_flags"
  parse-options.[ch]: consistently use "enum parse_opt_result"
  parse-options.c: use exhaustive "case" arms for "enum
    parse_opt_result"
  parse-options.h: make the "flags" in "struct option" an enum
  parse-options.c: move optname() earlier in the file
  commit-graph: stop using optname()
  parse-options.[ch]: make opt{bug,name}() "static"
  parse-options tests: test optname() output
  parse-options: change OPT_{SHORT,UNSET} to an enum

 builtin/blame.c          |  3 ++
 builtin/commit-graph.c   |  4 +-
 builtin/shortlog.c       |  3 ++
 parse-options.c          | 87 ++++++++++++++++++++++------------------
 parse-options.h          | 26 ++++++------
 t/t0040-parse-options.sh | 42 +++++++++++++++++--
 6 files changed, 109 insertions(+), 56 deletions(-)

Range-diff against v2:
 1:  553931702df =  1:  59195845497 parse-options.h: move PARSE_OPT_SHELL_EVAL between enums
 2:  99f5251c557 !  2:  d4aaaa645de parse-options.[ch]: consistently use "enum parse_opt_flags"
    @@ Commit message
         Use the "enum parse_opt_flags" instead of an "int flags" as arguments
         to the various functions in parse-options.c.
     
    -    In C enums aren't first-class types, and the "enum
    -    parse_opt_option_flag" uses a enum-as-bitfield pattern. So unlike
    -    exhaustively enumerated "case" arms we're not going to get validation
    -    that we used the "right" enum labels.
    -
    -    I.e. this won't catch the sort of bug that was fixed with
    -    "PARSE_OPT_SHELL_EVAL" in the preceding commit.
    -
    -    But there's still a benefit to doing this when it comes to the wider C
    -    ecosystem. E.g. the GNU debugger (gdb) will helpfully detect and print
    -    out meaningful enum labels in this case. Here's the output before and
    -    after when breaking in "parse_options()" after invoking "git stash
    -    show":
    +    Even though this is an enum bitfield there's there's a benefit to
    +    doing this when it comes to the wider C ecosystem. E.g. the GNU
    +    debugger (gdb) will helpfully detect and print out meaningful enum
    +    labels in this case. Here's the output before and after when breaking
    +    in "parse_options()" after invoking "git stash show":
     
         Before:
     
    @@ parse-options.h: struct option {
      NORETURN void usage_with_options(const char * const *usagestr,
      				 const struct option *options);
     @@ parse-options.h: struct parse_opt_ctx_t {
    + 	const char **out;
    + 	int argc, cpidx, total;
    + 	const char *opt;
    +-	int flags;
    ++	enum parse_opt_flags flags;
    + 	const char *prefix;
    + 	const char **alias_groups; /* must be in groups of 3 elements! */
    + 	struct option *updated_options;
    +@@ parse-options.h: struct parse_opt_ctx_t {
      
      void parse_options_start(struct parse_opt_ctx_t *ctx,
      			 int argc, const char **argv, const char *prefix,
 3:  be198e42df9 =  3:  828e8b96574 parse-options.[ch]: consistently use "enum parse_opt_result"
 4:  a253db7a60d =  4:  bebf3448c49 parse-options.c: use exhaustive "case" arms for "enum parse_opt_result"
 5:  467828780d0 <  -:  ----------- parse-options.c: use exhaustive "case" arms for "enum parse_opt_type"
 6:  34669327550 =  5:  23e62d4139f parse-options.h: make the "flags" in "struct option" an enum
 7:  e82a4e477d5 =  6:  7afdb22885d parse-options.c: move optname() earlier in the file
 8:  58683b3d89d !  7:  67a9b38f66c commit-graph: stop using optname()
    @@ Commit message
         of "opt->flags" to optname(), that function expects flags, but not
         *those* flags.
     
    +    Let's pass "max-new-filters" to the new error because the option name
    +    isn't translatable, and because we can re-use a translation added in
    +    f7e68a08780 (parse-options: check empty value in OPT_INTEGER and
    +    OPT_ABBREV, 2019-05-29).
    +
         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
      ## builtin/commit-graph.c ##
    @@ builtin/commit-graph.c: static int write_option_max_new_filters(const struct opt
      		if (*s)
     -			return error(_("%s expects a numerical value"),
     -				     optname(opt, opt->flags));
    -+			return error(_("option `max-new-filters' expects a numerical value"));
    ++			return error(_("option `%s' expects a numerical value"),
    ++				     "max-new-filters");
      	}
      	return 0;
      }
 9:  8cbee660174 =  8:  520cc5a8dc0 parse-options.[ch]: make opt{bug,name}() "static"
10:  f727f683eb1 =  9:  a82987a03c7 parse-options tests: test optname() output
11:  4fbc07fc7fd = 10:  e05627d3634 parse-options: change OPT_{SHORT,UNSET} to an enum

Fix a bad landmine of a bug which has been with us ever since PARSE_OPT_SHELL_EVAL was added in 47e9cd28f8a (parseopt: wrap rev-parse --parseopt usage for eval consumption, 2010-06-12).

It's an argument to parse_options() and should therefore be in "enum parse_opt_flags", but it was added to the per-option "enum parse_opt_option_flags" by mistake.

Therefore as soon as we'd have an enum member in the former that reached its value of "1 << 8" we'd run into a seemingly bizarre bug where that new option would turn on the unrelated PARSE_OPT_SHELL_EVAL in "git rev-parse --parseopt" by proxy.

I manually checked that no other enum members suffered from such overlap, by setting the values to non-overlapping values, and making the relevant codepaths BUG() out if the given value was above/below the expected (excluding flags=0 in the case of "enum parse_opt_flags").

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 parse-options.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Use the "enum parse_opt_result" instead of an "int flags" as the return value of the applicable functions in parse-options.c.

This will help catch future bugs, such as the missing "case" arms in the two existing users of the API in "blame.c" and "shortlog.c". A third caller in 309be813c9b (update-index: migrate to parse-options API, 2010-12-01) was already checking for these.

As can be seen when trying to sort through the deluge of warnings produced when compiling this with CC=g++ (mostly unrelated to this change) we're not consistently using "enum parse_opt_result" even now, i.e. we'll return error() and "return 0;". See f41179f16ba (parse-options: avoid magic return codes, 2019-01-27) for a commit which started changing some of that.

I'm not doing any more of that exhaustive migration here, and it's probably not worthwhile past the point of being able to check "enum parse_opt_result" in switch().

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/blame.c    |  3 +++
 builtin/shortlog.c |  3 +++
 parse-options.c    | 31 +++++++++++++++++--------------
 parse-options.h    | 15 ++++++++-------
 4 files changed, 31 insertions(+), 21 deletions(-)

On Fri, Oct 08, 2021 at 09:07:39PM +0200, Ævar Arnfjörð Bjarmason wrote:

The return type of this function should have been left unchanged, because it contains only one return statement:

          return parse_options_end(&ctx);

and parse_options_end() does return an int.

On Sat, Nov 06 2021, SZEDER Gábor wrote:

Indeed. I'll submit a fix for that sooner than later. I think I got that and *_step() mixed up, parse_options() just returns "here's what we've got left in argc".

I think due to C's happy-go-lucky enum-as-int semantics I'll probably leave it until post-2.34, i.e. I don't think it can/will break anything at runtime, but should be fixed for sure.

Thanks for spotting!

This tiny series fixes an issue new in v2.34.0-rc* where a recent patch of mine incorrectly changed a return type to an enum that wasn't applicable to that function. That fix is in 1/2. Thanks to SZEDER for spotting it!

Junio: As noted in [1] I don't think this needs to be integrated
before the release, but if you wanted to pick it up anyway I think
it's safe. Maybe someone's compiler will complain about the pre-image.

2/2 then marks a static function I missed with an enum, which it was already always returning except in an error case.

1. https://lore.kernel.org/git/211106.86lf21ezqx.gmgdl@evledraar.gmail.com/

Ævar Arnfjörð Bjarmason (2):
  parse-options.[ch]: revert use of "enum" for parse_options()
  parse-options.c: use "enum parse_opt_result" for parse_nodash_opt()

 parse-options.c | 17 +++++++++--------
 parse-options.h |  9 ++++-----
 2 files changed, 13 insertions(+), 13 deletions(-)

Revert the parse_options() prototype change in my recent 352e761388b (parse-options.[ch]: consistently use "enum parse_opt_result", 2021-10-08) was incorrect. The parse_options() function returns the number of argc elements that haven't been processed, not "enum parse_opt_result".

Reported-by: SZEDER Gábor <szeder.dev@gmail.com>
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 parse-options.c | 10 +++++-----
 parse-options.h |  9 ++++-----
 2 files changed, 9 insertions(+), 10 deletions(-)

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

Yes, this makes total sense.  We went overboard during the series.

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

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> 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:

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.

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

OK. That sounds like a good explanation for the change, to be recorded in the proposed log message.

Thanks.

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 <avarab@gmail.com>
---

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 <avarab@gmail.com>
     
      ## parse-options.c ##

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

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

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

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.

Use the "enum parse_opt_flags" instead of an "int flags" as arguments to the various functions in parse-options.c.

Even though this is an enum bitfield there's there's a benefit to doing this when it comes to the wider C ecosystem. E.g. the GNU debugger (gdb) will helpfully detect and print out meaningful enum labels in this case. Here's the output before and after when breaking in "parse_options()" after invoking "git stash show":

Before:

    (gdb) p flags
    $1 = 9

After:

    (gdb) p flags
    $1 = (PARSE_OPT_KEEP_DASHDASH | PARSE_OPT_KEEP_UNKNOWN)

Of course as noted in[1] there's a limit to this smartness, i.e. manually setting it with unrelated enum labels won't be caught. There are some third-party extensions to do more exhaustive checking[2], perhaps we'll be able to make use of them sooner than later.

We've also got prior art using this pattern in the codebase. See e.g. "enum bloom_filter_computed" added in 312cff52074 (bloom: split 'get_bloom_filter()' in two, 2020-09-16) and the "permitted" enum added in ce910287e72 (add -p: fix checking of user input, 2020-08-17).

1. https://lore.kernel.org/git/87mtnvvj3c.fsf@evledraar.gmail.com/
2. https://github.com/sinelaw/elfs-clang-plugins/blob/master/enums_conversion/README.md

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 parse-options.c | 11 +++++++----
 parse-options.h |  8 +++++---
 2 files changed, 12 insertions(+), 7 deletions(-)

Change the "flags" members of "struct option" to refer to their corresponding "enum" defined earlier in the file.

The benefit of changing this to an enum isn't as great as with some "enum parse_opt_type" as we'll always check this as a bitfield, so we can't rely on the compiler checking "case" arms for us. But let's do it for consistency with the rest of the file.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 parse-options.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Change the "default" case in parse_options() that handles the return value of parse_options_step() to simply have a "case" arm for PARSE_OPT_UNKNOWN, instead of leaving it to a comment. This means the compiler can warn us about any missing case arms.

This adjusts code added in ff43ec3e2d2 (parse-opt: create parse_options_step., 2008-06-23), given its age it may pre-date the existence (or widespread use) of this coding style, which we've since adopted more widely.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 parse-options.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

In preparation for making "optname" a static function move it above its first user in parse-options.c.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 parse-options.c | 30 +++++++++++++++---------------
 1 file changed, 15 insertions(+), 15 deletions(-)

Stop using optname() in builtin/commit-graph.c to emit an error with the --max-new-filters option. This changes code added in 809e0327f57 (builtin/commit-graph.c: introduce '--max-new-filters=<n>', 2020-09-18).

See 9440b831ad5 (parse-options: replace opterror() with optname(), 2018-11-10) for why using optname() like this is considered bad, i.e. it's assembling human-readable output piecemeal, and the "option `X'" at the start can't be translated.

It didn't matter in this case, but this code was also buggy in its use of "opt->flags" to optname(), that function expects flags, but not *those* flags.

Let's pass "max-new-filters" to the new error because the option name isn't translatable, and because we can re-use a translation added in f7e68a08780 (parse-options: check empty value in OPT_INTEGER and OPT_ABBREV, 2019-05-29).

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/commit-graph.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Change these two functions to "static", the last user of "optname()" outside of parse-options.c itself went away in the preceding commit, for the reasons noted in 9440b831ad5 (parse-options: replace opterror() with optname(), 2018-11-10) we shouldn't be adding any more users of it.

The "optbug()" function was never used outside of parse-options.c, but was made non-static in 1f275b7c4ca (parse-options: export opterr, optbug, 2011-08-11). I think the only external user of optname() was the commit-graph.c caller added in 09e0327f57 (builtin/commit-graph.c: introduce '--max-new-filters=<n>', 2020-09-18).

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 parse-options.c | 4 ++--
 parse-options.h | 3 ---
 2 files changed, 2 insertions(+), 5 deletions(-)

There were no tests for checking the specific output that we'll generate in optname(), let's add some. That output was added back in 4a59fd13122 (Add a simple option parser., 2007-10-15).

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t0040-parse-options.sh | 42 +++++++++++++++++++++++++++++++++++++---
 1 file changed, 39 insertions(+), 3 deletions(-)

Change the comparisons against OPT_SHORT and OPT_UNSET to an enum which keeps track of how a given option got parsed. The case of "0" was an implicit OPT_LONG, so let's add an explicit label for it.

Due to the xor in 0f1930c5875 (parse-options: allow positivation of options starting, with no-, 2012-02-25) the code already relied on this being set back to 0. To avoid refactoring the logic involved in that let's just start the enum at "0" instead of the usual "1<<0" (1), but BUG() out if we don't have one of our expected flags.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 parse-options.c | 23 ++++++++++++++---------
 1 file changed, 14 insertions(+), 9 deletions(-)