Subject: Re: [PATCH v2 02/11] parse-options.[ch]: consistently use "enum parse_opt_flags"

From: Junio C Hamano <[email-masked]>
Date: Fri, 01 Oct 2021 14:45:17 -0700

Ævar Arnfjörð Bjarmason  <[email-masked]> writes:

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

OK.

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

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.

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