From 3f9ab7ccdea91b8312a14d39ce752b4d6685d067 Mon Sep 17 00:00:00 2001 From: Ævar Arnfjörð Bjarmason Date: Fri, 8 Oct 2021 21:07:38 +0200 Subject: parse-options.[ch]: consistently use "enum parse_opt_flags" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Signed-off-by: Junio C Hamano --- parse-options.c | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) (limited to 'parse-options.c') diff --git a/parse-options.c b/parse-options.c index 55c5821b08..9c8ba96340 100644 --- a/parse-options.c +++ b/parse-options.c @@ -481,7 +481,8 @@ static void parse_options_check(const struct option *opts) 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; @@ -506,7 +507,8 @@ static void parse_options_start_1(struct parse_opt_ctx_t *ctx, void parse_options_start(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) { memset(ctx, 0, sizeof(*ctx)); parse_options_start_1(ctx, argc, argv, prefix, options, flags); @@ -838,8 +840,9 @@ int parse_options_end(struct parse_opt_ctx_t *ctx) } int parse_options(int argc, const char **argv, const char *prefix, - const struct option *options, const char * const usagestr[], - int flags) + const struct option *options, + const char * const usagestr[], + enum parse_opt_flags flags) { struct parse_opt_ctx_t ctx; struct option *real_options; -- cgit v1.3-5-g9baa From 352e761388b5fa41bf40e7c04edf3cb07d888d94 Mon Sep 17 00:00:00 2001 From: Ævar Arnfjörð Bjarmason Date: Fri, 8 Oct 2021 21:07:39 +0200 Subject: parse-options.[ch]: consistently use "enum parse_opt_result" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Signed-off-by: Junio C Hamano --- builtin/blame.c | 3 +++ builtin/shortlog.c | 3 +++ parse-options.c | 31 +++++++++++++++++-------------- parse-options.h | 15 ++++++++------- 4 files changed, 31 insertions(+), 21 deletions(-) (limited to 'parse-options.c') diff --git a/builtin/blame.c b/builtin/blame.c index 641523ff9a..9273fb222d 100644 --- a/builtin/blame.c +++ b/builtin/blame.c @@ -917,6 +917,9 @@ int cmd_blame(int argc, const char **argv, const char *prefix) PARSE_OPT_KEEP_DASHDASH | PARSE_OPT_KEEP_ARGV0); for (;;) { switch (parse_options_step(&ctx, options, blame_opt_usage)) { + case PARSE_OPT_NON_OPTION: + case PARSE_OPT_UNKNOWN: + break; case PARSE_OPT_HELP: case PARSE_OPT_ERROR: exit(129); diff --git a/builtin/shortlog.c b/builtin/shortlog.c index 3e7ab1ca82..e7f7af5de3 100644 --- a/builtin/shortlog.c +++ b/builtin/shortlog.c @@ -374,6 +374,9 @@ int cmd_shortlog(int argc, const char **argv, const char *prefix) for (;;) { switch (parse_options_step(&ctx, options, shortlog_usage)) { + case PARSE_OPT_NON_OPTION: + case PARSE_OPT_UNKNOWN: + break; case PARSE_OPT_HELP: case PARSE_OPT_ERROR: exit(129); diff --git a/parse-options.c b/parse-options.c index 9c8ba96340..f718242096 100644 --- a/parse-options.c +++ b/parse-options.c @@ -699,13 +699,14 @@ 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[]) { int internal_help = !(ctx->flags & PARSE_OPT_NO_INTERNAL_HELP); @@ -839,10 +840,11 @@ int parse_options_end(struct parse_opt_ctx_t *ctx) return ctx->cpidx + ctx->argc; } -int parse_options(int argc, const char **argv, const char *prefix, - const struct option *options, - const char * const usagestr[], - enum parse_opt_flags flags) +enum parse_opt_result parse_options(int argc, const char **argv, + const char *prefix, + const struct option *options, + const char * const usagestr[], + enum parse_opt_flags flags) { struct parse_opt_ctx_t ctx; struct option *real_options; @@ -900,9 +902,10 @@ 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; diff --git a/parse-options.h b/parse-options.h index 2e8798d874..a1c7c86ad3 100644 --- a/parse-options.h +++ b/parse-options.h @@ -211,10 +211,11 @@ struct option { * untouched and parse_options() returns the number of options * processed. */ -int parse_options(int argc, const char **argv, const char *prefix, - const struct option *options, - const char * const usagestr[], - enum parse_opt_flags flags); +enum parse_opt_result parse_options(int argc, const char **argv, + const char *prefix, + const struct option *options, + const char * const usagestr[], + enum parse_opt_flags flags); NORETURN void usage_with_options(const char * const *usagestr, const struct option *options); @@ -274,9 +275,9 @@ void parse_options_start(struct parse_opt_ctx_t *ctx, const struct option *options, enum parse_opt_flags flags); -int parse_options_step(struct parse_opt_ctx_t *ctx, - const struct option *options, - const char * const usagestr[]); +enum parse_opt_result parse_options_step(struct parse_opt_ctx_t *ctx, + const struct option *options, + const char * const usagestr[]); int parse_options_end(struct parse_opt_ctx_t *ctx); -- cgit v1.3-5-g9baa From 1b887353d75f62c957e04c7d6ff706142c761a4c Mon Sep 17 00:00:00 2001 From: Ævar Arnfjörð Bjarmason Date: Fri, 8 Oct 2021 21:07:40 +0200 Subject: parse-options.c: use exhaustive "case" arms for "enum parse_opt_result" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Signed-off-by: Junio C Hamano --- parse-options.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'parse-options.c') diff --git a/parse-options.c b/parse-options.c index f718242096..e33700d6e7 100644 --- a/parse-options.c +++ b/parse-options.c @@ -866,7 +866,7 @@ enum parse_opt_result parse_options(int argc, const char **argv, 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)) { -- cgit v1.3-5-g9baa From 3c2047a711a47fbaf0b20d8f9a551c764a6c3f34 Mon Sep 17 00:00:00 2001 From: Ævar Arnfjörð Bjarmason Date: Fri, 8 Oct 2021 21:07:42 +0200 Subject: parse-options.c: move optname() earlier in the file MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Signed-off-by: Junio C Hamano --- parse-options.c | 30 +++++++++++++++--------------- 1 file changed, 15 insertions(+), 15 deletions(-) (limited to 'parse-options.c') diff --git a/parse-options.c b/parse-options.c index e33700d6e7..9e2da8383d 100644 --- a/parse-options.c +++ b/parse-options.c @@ -22,6 +22,21 @@ int optbug(const struct option *opt, const char *reason) return error("BUG: switch '%c' %s", opt->short_name, reason); } +const char *optname(const struct option *opt, int flags) +{ + static struct strbuf sb = STRBUF_INIT; + + strbuf_reset(&sb); + if (flags & OPT_SHORT) + strbuf_addf(&sb, "switch `%c'", opt->short_name); + else if (flags & OPT_UNSET) + strbuf_addf(&sb, "option `no-%s'", opt->long_name); + else + strbuf_addf(&sb, "option `%s'", opt->long_name); + + return sb.buf; +} + static enum parse_opt_result get_arg(struct parse_opt_ctx_t *p, const struct option *opt, int flags, const char **arg) @@ -1006,18 +1021,3 @@ void NORETURN usage_msg_opt(const char *msg, fprintf(stderr, "fatal: %s\n\n", msg); usage_with_options(usagestr, options); } - -const char *optname(const struct option *opt, int flags) -{ - static struct strbuf sb = STRBUF_INIT; - - strbuf_reset(&sb); - if (flags & OPT_SHORT) - strbuf_addf(&sb, "switch `%c'", opt->short_name); - else if (flags & OPT_UNSET) - strbuf_addf(&sb, "option `no-%s'", opt->long_name); - else - strbuf_addf(&sb, "option `%s'", opt->long_name); - - return sb.buf; -} -- cgit v1.3-5-g9baa From 28794ec72e284ba398b562bc117e091853ef0332 Mon Sep 17 00:00:00 2001 From: Ævar Arnfjörð Bjarmason Date: Fri, 8 Oct 2021 21:07:44 +0200 Subject: parse-options.[ch]: make opt{bug,name}() "static" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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=', 2020-09-18). Signed-off-by: Ævar Arnfjörð Bjarmason Signed-off-by: Junio C Hamano --- parse-options.c | 4 ++-- parse-options.h | 3 --- 2 files changed, 2 insertions(+), 5 deletions(-) (limited to 'parse-options.c') diff --git a/parse-options.c b/parse-options.c index 9e2da8383d..64bd4c3285 100644 --- a/parse-options.c +++ b/parse-options.c @@ -11,7 +11,7 @@ static int disallow_abbreviated_options; #define OPT_SHORT 1 #define OPT_UNSET 2 -int optbug(const struct option *opt, const char *reason) +static int optbug(const struct option *opt, const char *reason) { if (opt->long_name) { if (opt->short_name) @@ -22,7 +22,7 @@ int optbug(const struct option *opt, const char *reason) return error("BUG: switch '%c' %s", opt->short_name, reason); } -const char *optname(const struct option *opt, int flags) +static const char *optname(const struct option *opt, int flags) { static struct strbuf sb = STRBUF_INIT; diff --git a/parse-options.h b/parse-options.h index 74b66ba6e9..dd79c9c566 100644 --- a/parse-options.h +++ b/parse-options.h @@ -224,9 +224,6 @@ NORETURN void usage_msg_opt(const char *msg, const char * const *usagestr, const struct option *options); -int optbug(const struct option *opt, const char *reason); -const char *optname(const struct option *opt, int flags); - /* * Use these assertions for callbacks that expect to be called with NONEG and * NOARG respectively, and do not otherwise handle the "unset" and "arg" -- cgit v1.3-5-g9baa From d342834529495508fa1f23e223d4917cbdcfb54d Mon Sep 17 00:00:00 2001 From: Ævar Arnfjörð Bjarmason Date: Fri, 8 Oct 2021 21:07:46 +0200 Subject: parse-options: change OPT_{SHORT,UNSET} to an enum MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Signed-off-by: Junio C Hamano --- parse-options.c | 23 ++++++++++++++--------- 1 file changed, 14 insertions(+), 9 deletions(-) (limited to 'parse-options.c') diff --git a/parse-options.c b/parse-options.c index 64bd4c3285..2a2c0ee24f 100644 --- a/parse-options.c +++ b/parse-options.c @@ -8,8 +8,11 @@ static int disallow_abbreviated_options; -#define OPT_SHORT 1 -#define OPT_UNSET 2 +enum opt_parsed { + OPT_LONG = 0, + OPT_SHORT = 1<<0, + OPT_UNSET = 1<<1, +}; static int optbug(const struct option *opt, const char *reason) { @@ -22,7 +25,7 @@ static int optbug(const struct option *opt, const char *reason) return error("BUG: switch '%c' %s", opt->short_name, reason); } -static const char *optname(const struct option *opt, int flags) +static const char *optname(const struct option *opt, enum opt_parsed flags) { static struct strbuf sb = STRBUF_INIT; @@ -31,15 +34,17 @@ static const char *optname(const struct option *opt, int flags) strbuf_addf(&sb, "switch `%c'", opt->short_name); else if (flags & OPT_UNSET) strbuf_addf(&sb, "option `no-%s'", opt->long_name); - else + else if (flags == OPT_LONG) strbuf_addf(&sb, "option `%s'", opt->long_name); + else + BUG("optname() got unknown flags %d", flags); return sb.buf; } static enum parse_opt_result get_arg(struct parse_opt_ctx_t *p, const struct option *opt, - int flags, const char **arg) + enum opt_parsed flags, const char **arg) { if (p->opt) { *arg = p->opt; @@ -65,7 +70,7 @@ static void fix_filename(const char *prefix, const char **file) static enum parse_opt_result opt_command_mode_error( const struct option *opt, const struct option *all_opts, - int flags) + enum opt_parsed flags) { const struct option *that; struct strbuf that_name = STRBUF_INIT; @@ -97,7 +102,7 @@ static enum parse_opt_result opt_command_mode_error( static enum parse_opt_result get_value(struct parse_opt_ctx_t *p, const struct option *opt, const struct option *all_opts, - int flags) + enum opt_parsed flags) { const char *s, *arg; const int unset = flags & OPT_UNSET; @@ -313,11 +318,11 @@ static enum parse_opt_result parse_long_opt( const struct option *all_opts = options; const char *arg_end = strchrnul(arg, '='); const struct option *abbrev_option = NULL, *ambiguous_option = NULL; - int abbrev_flags = 0, ambiguous_flags = 0; + enum opt_parsed abbrev_flags = OPT_LONG, ambiguous_flags = OPT_LONG; for (; options->type != OPTION_END; options++) { const char *rest, *long_name = options->long_name; - int flags = 0, opt_flags = 0; + enum opt_parsed flags = OPT_LONG, opt_flags = OPT_LONG; if (!long_name) continue; -- cgit v1.3-5-g9baa