* [PATCH v2 01/11] parse-options.h: move PARSE_OPT_SHELL_EVAL between enums
2021-10-01 14:29 ` [PATCH v2 00/11] fix bug, use existing enums Ævar Arnfjörð Bjarmason
@ 2021-10-01 14:29 ` Ævar Arnfjörð Bjarmason
2021-10-01 14:29 ` [PATCH v2 02/11] parse-options.[ch]: consistently use "enum parse_opt_flags" Ævar Arnfjörð Bjarmason
` (11 subsequent siblings)
12 siblings, 0 replies; 61+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-10-01 14:29 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Thomas Rast, René Scharfe,
Ævar Arnfjörð Bjarmason
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(-)
diff --git a/parse-options.h b/parse-options.h
index 39d90882548..3a3176ae65c 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -33,6 +33,7 @@ enum parse_opt_flags {
PARSE_OPT_KEEP_UNKNOWN = 1 << 3,
PARSE_OPT_NO_INTERNAL_HELP = 1 << 4,
PARSE_OPT_ONE_SHOT = 1 << 5,
+ PARSE_OPT_SHELL_EVAL = 1 << 6,
};
enum parse_opt_option_flags {
@@ -44,7 +45,6 @@ enum parse_opt_option_flags {
PARSE_OPT_NODASH = 1 << 5,
PARSE_OPT_LITERAL_ARGHELP = 1 << 6,
PARSE_OPT_FROM_ALIAS = 1 << 7,
- PARSE_OPT_SHELL_EVAL = 1 << 8,
PARSE_OPT_NOCOMPLETE = 1 << 9,
PARSE_OPT_COMP_ARG = 1 << 10,
PARSE_OPT_CMDMODE = 1 << 11,
--
2.33.0.1374.gc8f4fa74caf
^ permalink raw reply related [flat|nested] 61+ messages in thread
* [PATCH v2 02/11] parse-options.[ch]: consistently use "enum parse_opt_flags"
2021-10-01 14:29 ` [PATCH v2 00/11] fix bug, use existing enums Ævar Arnfjörð Bjarmason
2021-10-01 14:29 ` [PATCH v2 01/11] parse-options.h: move PARSE_OPT_SHELL_EVAL between enums Ævar Arnfjörð Bjarmason
@ 2021-10-01 14:29 ` Ævar Arnfjörð Bjarmason
2021-10-01 21:45 ` Junio C Hamano
2021-10-01 14:29 ` [PATCH v2 03/11] parse-options.[ch]: consistently use "enum parse_opt_result" Ævar Arnfjörð Bjarmason
` (10 subsequent siblings)
12 siblings, 1 reply; 61+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-10-01 14:29 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Thomas Rast, René Scharfe,
Ævar Arnfjörð Bjarmason
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(-)
diff --git a/parse-options.c b/parse-options.c
index 55c5821b08d..9c8ba963400 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;
diff --git a/parse-options.h b/parse-options.h
index 3a3176ae65c..fb5aafd4f7b 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -213,7 +213,8 @@ struct option {
*/
int parse_options(int argc, const char **argv, const char *prefix,
const struct option *options,
- const char * const usagestr[], int flags);
+ const char * const usagestr[],
+ enum parse_opt_flags flags);
NORETURN void usage_with_options(const char * const *usagestr,
const struct option *options);
@@ -270,7 +271,8 @@ struct parse_opt_ctx_t {
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);
int parse_options_step(struct parse_opt_ctx_t *ctx,
const struct option *options,
--
2.33.0.1374.gc8f4fa74caf
^ permalink raw reply related [flat|nested] 61+ messages in thread
* Re: [PATCH v2 02/11] parse-options.[ch]: consistently use "enum parse_opt_flags"
2021-10-01 14:29 ` [PATCH v2 02/11] parse-options.[ch]: consistently use "enum parse_opt_flags" Ævar Arnfjörð Bjarmason
@ 2021-10-01 21:45 ` Junio C Hamano
0 siblings, 0 replies; 61+ messages in thread
From: Junio C Hamano @ 2021-10-01 21:45 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: git, Jeff King, Thomas Rast, René Scharfe
Ævar Arnfjörð Bjarmason <avarab@gmail.com> 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)
^ permalink raw reply [flat|nested] 61+ messages in thread
* [PATCH v2 03/11] parse-options.[ch]: consistently use "enum parse_opt_result"
2021-10-01 14:29 ` [PATCH v2 00/11] fix bug, use existing enums Ævar Arnfjörð Bjarmason
2021-10-01 14:29 ` [PATCH v2 01/11] parse-options.h: move PARSE_OPT_SHELL_EVAL between enums Ævar Arnfjörð Bjarmason
2021-10-01 14:29 ` [PATCH v2 02/11] parse-options.[ch]: consistently use "enum parse_opt_flags" Ævar Arnfjörð Bjarmason
@ 2021-10-01 14:29 ` Ævar Arnfjörð Bjarmason
2021-10-01 14:29 ` [PATCH v2 04/11] parse-options.c: use exhaustive "case" arms for " Ævar Arnfjörð Bjarmason
` (9 subsequent siblings)
12 siblings, 0 replies; 61+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-10-01 14:29 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Thomas Rast, René Scharfe,
Ævar Arnfjörð Bjarmason
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(-)
diff --git a/builtin/blame.c b/builtin/blame.c
index 641523ff9af..9273fb222d5 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 3e7ab1ca821..e7f7af5de3f 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 9c8ba963400..f718242096c 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 fb5aafd4f7b..d931300f4d6 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);
--
2.33.0.1374.gc8f4fa74caf
^ permalink raw reply related [flat|nested] 61+ messages in thread
* [PATCH v2 04/11] parse-options.c: use exhaustive "case" arms for "enum parse_opt_result"
2021-10-01 14:29 ` [PATCH v2 00/11] fix bug, use existing enums Ævar Arnfjörð Bjarmason
` (2 preceding siblings ...)
2021-10-01 14:29 ` [PATCH v2 03/11] parse-options.[ch]: consistently use "enum parse_opt_result" Ævar Arnfjörð Bjarmason
@ 2021-10-01 14:29 ` Ævar Arnfjörð Bjarmason
2021-10-01 14:29 ` [PATCH v2 05/11] parse-options.c: use exhaustive "case" arms for "enum parse_opt_type" Ævar Arnfjörð Bjarmason
` (8 subsequent siblings)
12 siblings, 0 replies; 61+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-10-01 14:29 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Thomas Rast, René Scharfe,
Ævar Arnfjörð Bjarmason
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(-)
diff --git a/parse-options.c b/parse-options.c
index f718242096c..e33700d6e71 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)) {
--
2.33.0.1374.gc8f4fa74caf
^ permalink raw reply related [flat|nested] 61+ messages in thread
* [PATCH v2 05/11] parse-options.c: use exhaustive "case" arms for "enum parse_opt_type"
2021-10-01 14:29 ` [PATCH v2 00/11] fix bug, use existing enums Ævar Arnfjörð Bjarmason
` (3 preceding siblings ...)
2021-10-01 14:29 ` [PATCH v2 04/11] parse-options.c: use exhaustive "case" arms for " Ævar Arnfjörð Bjarmason
@ 2021-10-01 14:29 ` Ævar Arnfjörð Bjarmason
2021-10-01 14:29 ` [PATCH v2 06/11] parse-options.h: make the "flags" in "struct option" an enum Ævar Arnfjörð Bjarmason
` (7 subsequent siblings)
12 siblings, 0 replies; 61+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-10-01 14:29 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Thomas Rast, René Scharfe,
Ævar Arnfjörð Bjarmason
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(-)
diff --git a/parse-options.c b/parse-options.c
index e33700d6e71..dedd40efec5 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -219,9 +219,14 @@ static enum parse_opt_result get_value(struct parse_opt_ctx_t *p,
optname(opt, flags));
return 0;
- default:
+ /* special types */
+ case OPTION_END:
+ case OPTION_GROUP:
+ case OPTION_NUMBER:
+ case OPTION_ALIAS:
BUG("opt->type %d should not happen", opt->type);
}
+ return -1; /* gcc 10.2.1-6's -Werror=return-type */
}
static enum parse_opt_result parse_short_opt(struct parse_opt_ctx_t *p,
@@ -443,6 +448,9 @@ 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:
@@ -468,8 +476,14 @@ 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"
"That case is not supported yet.");
- default:
- ; /* ok. (usually accepts an argument) */
+
+ case OPTION_BITOP:
+ case OPTION_FILENAME:
+ case OPTION_GROUP:
+ case OPTION_INTEGER:
+ case OPTION_MAGNITUDE:
+ case OPTION_STRING:
+ break;
}
if (opts->argh &&
strcspn(opts->argh, " _") != strlen(opts->argh))
@@ -532,6 +546,9 @@ 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:
@@ -543,7 +560,14 @@ 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_GROUP:
+ case OPTION_NUMBER:
+ case OPTION_ALIAS:
+ /* options with no arguments */
+ case OPTION_BITOP:
+ /* options with arguments (usually) */
+ case OPTION_LOWLEVEL_CALLBACK:
break;
}
if (!has_unset_form)
@@ -578,6 +602,8 @@ 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:
@@ -593,7 +619,19 @@ static int show_gitcomp(const struct option *opts, int show_all)
break;
suffix = "=";
break;
- default:
+ /* special types */
+ case OPTION_NUMBER:
+ case OPTION_ALIAS:
+
+ /* options with no arguments */
+ case OPTION_BIT:
+ case OPTION_NEGBIT:
+ case OPTION_BITOP:
+ case OPTION_COUNTUP:
+ case OPTION_SET_INT:
+
+ /* options with arguments (usually) */
+ case OPTION_LOWLEVEL_CALLBACK:
break;
}
if (opts->flags & PARSE_OPT_COMP_ARG)
diff --git a/parse-options.h b/parse-options.h
index d931300f4d6..a1c7c86ad30 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -264,7 +264,7 @@ 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;
--
2.33.0.1374.gc8f4fa74caf
^ permalink raw reply related [flat|nested] 61+ messages in thread
* [PATCH v2 06/11] parse-options.h: make the "flags" in "struct option" an enum
2021-10-01 14:29 ` [PATCH v2 00/11] fix bug, use existing enums Ævar Arnfjörð Bjarmason
` (4 preceding siblings ...)
2021-10-01 14:29 ` [PATCH v2 05/11] parse-options.c: use exhaustive "case" arms for "enum parse_opt_type" Ævar Arnfjörð Bjarmason
@ 2021-10-01 14:29 ` Ævar Arnfjörð Bjarmason
2021-10-01 14:29 ` [PATCH v2 07/11] parse-options.c: move optname() earlier in the file Ævar Arnfjörð Bjarmason
` (6 subsequent siblings)
12 siblings, 0 replies; 61+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-10-01 14:29 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Thomas Rast, René Scharfe,
Ævar Arnfjörð Bjarmason
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(-)
diff --git a/parse-options.h b/parse-options.h
index a1c7c86ad30..74b66ba6e93 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -134,7 +134,7 @@ struct option {
const char *argh;
const char *help;
- int flags;
+ enum parse_opt_option_flags flags;
parse_opt_cb *callback;
intptr_t defval;
parse_opt_ll_cb *ll_callback;
--
2.33.0.1374.gc8f4fa74caf
^ permalink raw reply related [flat|nested] 61+ messages in thread
* [PATCH v2 07/11] parse-options.c: move optname() earlier in the file
2021-10-01 14:29 ` [PATCH v2 00/11] fix bug, use existing enums Ævar Arnfjörð Bjarmason
` (5 preceding siblings ...)
2021-10-01 14:29 ` [PATCH v2 06/11] parse-options.h: make the "flags" in "struct option" an enum Ævar Arnfjörð Bjarmason
@ 2021-10-01 14:29 ` Ævar Arnfjörð Bjarmason
2021-10-01 14:29 ` [PATCH v2 08/11] commit-graph: stop using optname() Ævar Arnfjörð Bjarmason
` (5 subsequent siblings)
12 siblings, 0 replies; 61+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-10-01 14:29 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Thomas Rast, René Scharfe,
Ævar Arnfjörð Bjarmason
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(-)
diff --git a/parse-options.c b/parse-options.c
index dedd40efec5..2cf6f4d01c1 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)
@@ -1044,18 +1059,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;
-}
--
2.33.0.1374.gc8f4fa74caf
^ permalink raw reply related [flat|nested] 61+ messages in thread
* [PATCH v2 08/11] commit-graph: stop using optname()
2021-10-01 14:29 ` [PATCH v2 00/11] fix bug, use existing enums Ævar Arnfjörð Bjarmason
` (6 preceding siblings ...)
2021-10-01 14:29 ` [PATCH v2 07/11] parse-options.c: move optname() earlier in the file Ævar Arnfjörð Bjarmason
@ 2021-10-01 14:29 ` Ævar Arnfjörð Bjarmason
2021-10-01 17:12 ` René Scharfe
2021-10-01 14:29 ` [PATCH v2 09/11] parse-options.[ch]: make opt{bug,name}() "static" Ævar Arnfjörð Bjarmason
` (4 subsequent siblings)
12 siblings, 1 reply; 61+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-10-01 14:29 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Thomas Rast, René Scharfe,
Ævar Arnfjörð Bjarmason
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(-)
diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c
index 0386f5c7755..36552db89fe 100644
--- a/builtin/commit-graph.c
+++ b/builtin/commit-graph.c
@@ -172,8 +172,7 @@ static int write_option_max_new_filters(const struct option *opt,
const char *s;
*to = strtol(arg, (char **)&s, 10);
if (*s)
- return error(_("%s expects a numerical value"),
- optname(opt, opt->flags));
+ return error(_("option `max-new-filters' expects a numerical value"));
}
return 0;
}
--
2.33.0.1374.gc8f4fa74caf
^ permalink raw reply related [flat|nested] 61+ messages in thread
* Re: [PATCH v2 08/11] commit-graph: stop using optname()
2021-10-01 14:29 ` [PATCH v2 08/11] commit-graph: stop using optname() Ævar Arnfjörð Bjarmason
@ 2021-10-01 17:12 ` René Scharfe
0 siblings, 0 replies; 61+ messages in thread
From: René Scharfe @ 2021-10-01 17:12 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason, git
Cc: Junio C Hamano, Jeff King, Thomas Rast
Am 01.10.21 um 16:29 schrieb Ævar Arnfjörð Bjarmason:
> 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(-)
>
> diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c
> index 0386f5c7755..36552db89fe 100644
> --- a/builtin/commit-graph.c
> +++ b/builtin/commit-graph.c
> @@ -172,8 +172,7 @@ static int write_option_max_new_filters(const struct option *opt,
> const char *s;
> *to = strtol(arg, (char **)&s, 10);
> if (*s)
> - return error(_("%s expects a numerical value"),
> - optname(opt, opt->flags));
> + return error(_("option `max-new-filters' expects a numerical value"));
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.
> }
> return 0;
> }
>
^ permalink raw reply [flat|nested] 61+ messages in thread
* [PATCH v2 09/11] parse-options.[ch]: make opt{bug,name}() "static"
2021-10-01 14:29 ` [PATCH v2 00/11] fix bug, use existing enums Ævar Arnfjörð Bjarmason
` (7 preceding siblings ...)
2021-10-01 14:29 ` [PATCH v2 08/11] commit-graph: stop using optname() Ævar Arnfjörð Bjarmason
@ 2021-10-01 14:29 ` Ævar Arnfjörð Bjarmason
2021-10-01 14:29 ` [PATCH v2 10/11] parse-options tests: test optname() output Ævar Arnfjörð Bjarmason
` (3 subsequent siblings)
12 siblings, 0 replies; 61+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-10-01 14:29 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Thomas Rast, René Scharfe,
Ævar Arnfjörð Bjarmason
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(-)
diff --git a/parse-options.c b/parse-options.c
index 2cf6f4d01c1..0239c6bd418 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 74b66ba6e93..dd79c9c566f 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"
--
2.33.0.1374.gc8f4fa74caf
^ permalink raw reply related [flat|nested] 61+ messages in thread
* [PATCH v2 10/11] parse-options tests: test optname() output
2021-10-01 14:29 ` [PATCH v2 00/11] fix bug, use existing enums Ævar Arnfjörð Bjarmason
` (8 preceding siblings ...)
2021-10-01 14:29 ` [PATCH v2 09/11] parse-options.[ch]: make opt{bug,name}() "static" Ævar Arnfjörð Bjarmason
@ 2021-10-01 14:29 ` Ævar Arnfjörð Bjarmason
2021-10-01 14:29 ` [PATCH v2 11/11] parse-options: change OPT_{SHORT,UNSET} to an enum Ævar Arnfjörð Bjarmason
` (2 subsequent siblings)
12 siblings, 0 replies; 61+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-10-01 14:29 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Thomas Rast, René Scharfe,
Ævar Arnfjörð Bjarmason
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(-)
diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh
index da310ed29b1..d6f391a497b 100755
--- a/t/t0040-parse-options.sh
+++ b/t/t0040-parse-options.sh
@@ -168,9 +168,45 @@ test_expect_success 'long options' '
'
test_expect_success 'missing required value' '
- test_expect_code 129 test-tool parse-options -s &&
- test_expect_code 129 test-tool parse-options --string &&
- test_expect_code 129 test-tool parse-options --file
+ cat >expect <<-\EOF &&
+ error: switch `s'\'' requires a value
+ EOF
+ test_expect_code 129 test-tool parse-options -s 2>actual &&
+ test_cmp expect actual &&
+
+ cat >expect <<-\EOF &&
+ error: option `string'\'' requires a value
+ EOF
+ test_expect_code 129 test-tool parse-options --string 2>actual &&
+ test_cmp expect actual &&
+
+ cat >expect <<-\EOF &&
+ error: option `file'\'' requires a value
+ EOF
+ test_expect_code 129 test-tool parse-options --file 2>actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'superfluous value provided: boolean' '
+ cat >expect <<-\EOF &&
+ error: option `yes'\'' takes no value
+ EOF
+ test_expect_code 129 test-tool parse-options --yes=hi 2>actual &&
+ test_cmp expect actual &&
+
+ cat >expect <<-\EOF &&
+ error: option `no-yes'\'' takes no value
+ EOF
+ test_expect_code 129 test-tool parse-options --no-yes=hi 2>actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'superfluous value provided: cmdmode' '
+ cat >expect <<-\EOF &&
+ error: option `mode1'\'' takes no value
+ EOF
+ test_expect_code 129 test-tool parse-options --mode1=hi 2>actual &&
+ test_cmp expect actual
'
cat >expect <<\EOF
--
2.33.0.1374.gc8f4fa74caf
^ permalink raw reply related [flat|nested] 61+ messages in thread
* [PATCH v2 11/11] parse-options: change OPT_{SHORT,UNSET} to an enum
2021-10-01 14:29 ` [PATCH v2 00/11] fix bug, use existing enums Ævar Arnfjörð Bjarmason
` (9 preceding siblings ...)
2021-10-01 14:29 ` [PATCH v2 10/11] parse-options tests: test optname() output Ævar Arnfjörð Bjarmason
@ 2021-10-01 14:29 ` Ævar Arnfjörð Bjarmason
2021-10-01 21:52 ` [PATCH v2 00/11] fix bug, use existing enums Junio C Hamano
2021-10-08 19:07 ` [PATCH v3 00/10] fix bug, use more enums Ævar Arnfjörð Bjarmason
12 siblings, 0 replies; 61+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-10-01 14:29 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Thomas Rast, René Scharfe,
Ævar Arnfjörð Bjarmason
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(-)
diff --git a/parse-options.c b/parse-options.c
index 0239c6bd418..61c294b7895 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;
@@ -318,11 +323,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;
--
2.33.0.1374.gc8f4fa74caf
^ permalink raw reply related [flat|nested] 61+ messages in thread
* Re: [PATCH v2 00/11] fix bug, use existing enums
2021-10-01 14:29 ` [PATCH v2 00/11] fix bug, use existing enums Ævar Arnfjörð Bjarmason
` (10 preceding siblings ...)
2021-10-01 14:29 ` [PATCH v2 11/11] parse-options: change OPT_{SHORT,UNSET} to an enum Ævar Arnfjörð Bjarmason
@ 2021-10-01 21:52 ` Junio C Hamano
2021-10-01 21:53 ` Junio C Hamano
2021-10-08 19:07 ` [PATCH v3 00/10] fix bug, use more enums Ævar Arnfjörð Bjarmason
12 siblings, 1 reply; 61+ messages in thread
From: Junio C Hamano @ 2021-10-01 21:52 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: git, Jeff King, Thomas Rast, René Scharfe
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
> 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.
Other than [3] and [4] that I am not yet convinced is a good idea,
this round looks good to me.
^ permalink raw reply [flat|nested] 61+ messages in thread
* Re: [PATCH v2 00/11] fix bug, use existing enums
2021-10-01 21:52 ` [PATCH v2 00/11] fix bug, use existing enums Junio C Hamano
@ 2021-10-01 21:53 ` Junio C Hamano
0 siblings, 0 replies; 61+ messages in thread
From: Junio C Hamano @ 2021-10-01 21:53 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: git, Jeff King, Thomas Rast, René Scharfe
Junio C Hamano <gitster@pobox.com> writes:
> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
>> 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.
>
> Other than [3] and [4] that I am not yet convinced is a good idea,
> this round looks good to me.
Not 3&4, but 4&5.
^ permalink raw reply [flat|nested] 61+ messages in thread
* [PATCH v3 00/10] fix bug, use more enums
2021-10-01 14:29 ` [PATCH v2 00/11] fix bug, use existing enums Ævar Arnfjörð Bjarmason
` (11 preceding siblings ...)
2021-10-01 21:52 ` [PATCH v2 00/11] fix bug, use existing enums Junio C Hamano
@ 2021-10-08 19:07 ` Ævar Arnfjörð Bjarmason
2021-10-08 19:07 ` [PATCH v3 01/10] parse-options.h: move PARSE_OPT_SHELL_EVAL between enums Ævar Arnfjörð Bjarmason
` (9 more replies)
12 siblings, 10 replies; 61+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-10-08 19:07 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Thomas Rast, René Scharfe,
Ævar Arnfjörð Bjarmason
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
--
2.33.0.1446.g6af949f83bd
^ permalink raw reply [flat|nested] 61+ messages in thread
* [PATCH v3 01/10] parse-options.h: move PARSE_OPT_SHELL_EVAL between enums
2021-10-08 19:07 ` [PATCH v3 00/10] fix bug, use more enums Ævar Arnfjörð Bjarmason
@ 2021-10-08 19:07 ` Ævar Arnfjörð Bjarmason
2021-10-08 19:07 ` [PATCH v3 02/10] parse-options.[ch]: consistently use "enum parse_opt_flags" Ævar Arnfjörð Bjarmason
` (8 subsequent siblings)
9 siblings, 0 replies; 61+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-10-08 19:07 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Thomas Rast, René Scharfe,
Ævar Arnfjörð Bjarmason
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(-)
diff --git a/parse-options.h b/parse-options.h
index 39d90882548..3a3176ae65c 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -33,6 +33,7 @@ enum parse_opt_flags {
PARSE_OPT_KEEP_UNKNOWN = 1 << 3,
PARSE_OPT_NO_INTERNAL_HELP = 1 << 4,
PARSE_OPT_ONE_SHOT = 1 << 5,
+ PARSE_OPT_SHELL_EVAL = 1 << 6,
};
enum parse_opt_option_flags {
@@ -44,7 +45,6 @@ enum parse_opt_option_flags {
PARSE_OPT_NODASH = 1 << 5,
PARSE_OPT_LITERAL_ARGHELP = 1 << 6,
PARSE_OPT_FROM_ALIAS = 1 << 7,
- PARSE_OPT_SHELL_EVAL = 1 << 8,
PARSE_OPT_NOCOMPLETE = 1 << 9,
PARSE_OPT_COMP_ARG = 1 << 10,
PARSE_OPT_CMDMODE = 1 << 11,
--
2.33.0.1446.g6af949f83bd
^ permalink raw reply related [flat|nested] 61+ messages in thread
* [PATCH v3 02/10] parse-options.[ch]: consistently use "enum parse_opt_flags"
2021-10-08 19:07 ` [PATCH v3 00/10] fix bug, use more enums Ævar Arnfjörð Bjarmason
2021-10-08 19:07 ` [PATCH v3 01/10] parse-options.h: move PARSE_OPT_SHELL_EVAL between enums Ævar Arnfjörð Bjarmason
@ 2021-10-08 19:07 ` Ævar Arnfjörð Bjarmason
2021-10-08 19:07 ` [PATCH v3 03/10] parse-options.[ch]: consistently use "enum parse_opt_result" Ævar Arnfjörð Bjarmason
` (7 subsequent siblings)
9 siblings, 0 replies; 61+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-10-08 19:07 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Thomas Rast, René Scharfe,
Ævar Arnfjörð Bjarmason
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(-)
diff --git a/parse-options.c b/parse-options.c
index 55c5821b08d..9c8ba963400 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;
diff --git a/parse-options.h b/parse-options.h
index 3a3176ae65c..2e8798d8744 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -213,7 +213,8 @@ struct option {
*/
int parse_options(int argc, const char **argv, const char *prefix,
const struct option *options,
- const char * const usagestr[], int flags);
+ const char * const usagestr[],
+ enum parse_opt_flags flags);
NORETURN void usage_with_options(const char * const *usagestr,
const struct option *options);
@@ -262,7 +263,7 @@ 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;
@@ -270,7 +271,8 @@ struct parse_opt_ctx_t {
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);
int parse_options_step(struct parse_opt_ctx_t *ctx,
const struct option *options,
--
2.33.0.1446.g6af949f83bd
^ permalink raw reply related [flat|nested] 61+ messages in thread
* [PATCH v3 03/10] parse-options.[ch]: consistently use "enum parse_opt_result"
2021-10-08 19:07 ` [PATCH v3 00/10] fix bug, use more enums Ævar Arnfjörð Bjarmason
2021-10-08 19:07 ` [PATCH v3 01/10] parse-options.h: move PARSE_OPT_SHELL_EVAL between enums Ævar Arnfjörð Bjarmason
2021-10-08 19:07 ` [PATCH v3 02/10] parse-options.[ch]: consistently use "enum parse_opt_flags" Ævar Arnfjörð Bjarmason
@ 2021-10-08 19:07 ` Ævar Arnfjörð Bjarmason
2021-11-06 19:11 ` SZEDER Gábor
2021-10-08 19:07 ` [PATCH v3 04/10] parse-options.c: use exhaustive "case" arms for "enum parse_opt_result" Ævar Arnfjörð Bjarmason
` (6 subsequent siblings)
9 siblings, 1 reply; 61+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-10-08 19:07 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Thomas Rast, René Scharfe,
Ævar Arnfjörð Bjarmason
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(-)
diff --git a/builtin/blame.c b/builtin/blame.c
index 641523ff9af..9273fb222d5 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 3e7ab1ca821..e7f7af5de3f 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 9c8ba963400..f718242096c 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 2e8798d8744..a1c7c86ad30 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);
--
2.33.0.1446.g6af949f83bd
^ permalink raw reply related [flat|nested] 61+ messages in thread
* Re: [PATCH v3 03/10] parse-options.[ch]: consistently use "enum parse_opt_result"
2021-10-08 19:07 ` [PATCH v3 03/10] parse-options.[ch]: consistently use "enum parse_opt_result" Ævar Arnfjörð Bjarmason
@ 2021-11-06 19:11 ` SZEDER Gábor
2021-11-06 21:31 ` Ævar Arnfjörð Bjarmason
0 siblings, 1 reply; 61+ messages in thread
From: SZEDER Gábor @ 2021-11-06 19:11 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: git, Junio C Hamano, Jeff King, Thomas Rast, René Scharfe
On Fri, Oct 08, 2021 at 09:07:39PM +0200, Ævar Arnfjörð Bjarmason wrote:
> 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>
> ---
> diff --git a/parse-options.c b/parse-options.c
> index 9c8ba963400..f718242096c 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,
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.
^ permalink raw reply [flat|nested] 61+ messages in thread
* Re: [PATCH v3 03/10] parse-options.[ch]: consistently use "enum parse_opt_result"
2021-11-06 19:11 ` SZEDER Gábor
@ 2021-11-06 21:31 ` Ævar Arnfjörð Bjarmason
2021-11-09 11:04 ` [PATCH 0/2] parse-options.[ch]: enum fixup & enum nit Ævar Arnfjörð Bjarmason
0 siblings, 1 reply; 61+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-11-06 21:31 UTC (permalink / raw)
To: SZEDER Gábor
Cc: git, Junio C Hamano, Jeff King, Thomas Rast, René Scharfe
On Sat, Nov 06 2021, SZEDER Gábor wrote:
> On Fri, Oct 08, 2021 at 09:07:39PM +0200, Ævar Arnfjörð Bjarmason wrote:
>> 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>
>> ---
>
>> diff --git a/parse-options.c b/parse-options.c
>> index 9c8ba963400..f718242096c 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,
>
> 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.
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!
^ permalink raw reply [flat|nested] 61+ messages in thread
* [PATCH 0/2] parse-options.[ch]: enum fixup & enum nit
2021-11-06 21:31 ` Ævar Arnfjörð Bjarmason
@ 2021-11-09 11:04 ` Ævar Arnfjörð Bjarmason
2021-11-09 11:04 ` [PATCH 1/2] parse-options.[ch]: revert use of "enum" for parse_options() Ævar Arnfjörð Bjarmason
2021-11-09 11:04 ` [PATCH 2/2] parse-options.c: use "enum parse_opt_result" for parse_nodash_opt() Ævar Arnfjörð Bjarmason
0 siblings, 2 replies; 61+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-11-09 11:04 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, SZEDER Gábor, Thomas Rast,
René Scharfe, Ævar Arnfjörð Bjarmason
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(-)
--
2.34.0.rc1.741.gab7bfd97031
^ permalink raw reply [flat|nested] 61+ messages in thread
* [PATCH 1/2] parse-options.[ch]: revert use of "enum" for parse_options()
2021-11-09 11:04 ` [PATCH 0/2] parse-options.[ch]: enum fixup & enum nit Ævar Arnfjörð Bjarmason
@ 2021-11-09 11:04 ` Ævar Arnfjörð Bjarmason
2021-11-09 17:45 ` Junio C Hamano
2021-11-09 11:04 ` [PATCH 2/2] parse-options.c: use "enum parse_opt_result" for parse_nodash_opt() Ævar Arnfjörð Bjarmason
1 sibling, 1 reply; 61+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-11-09 11:04 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, SZEDER Gábor, Thomas Rast,
René Scharfe, Ævar Arnfjörð Bjarmason
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(-)
diff --git a/parse-options.c b/parse-options.c
index 9a0484c8831..fc5b43ff0b2 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -860,11 +860,11 @@ int parse_options_end(struct parse_opt_ctx_t *ctx)
return ctx->cpidx + ctx->argc;
}
-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)
+int 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;
diff --git a/parse-options.h b/parse-options.h
index bdea052c399..275fb440818 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -213,11 +213,10 @@ struct option {
* untouched and parse_options() returns the number of options
* processed.
*/
-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);
+int 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);
--
2.34.0.rc1.741.gab7bfd97031
^ permalink raw reply related [flat|nested] 61+ messages in thread
* Re: [PATCH 1/2] parse-options.[ch]: revert use of "enum" for parse_options()
2021-11-09 11:04 ` [PATCH 1/2] parse-options.[ch]: revert use of "enum" for parse_options() Ævar Arnfjörð Bjarmason
@ 2021-11-09 17:45 ` Junio C Hamano
0 siblings, 0 replies; 61+ messages in thread
From: Junio C Hamano @ 2021-11-09 17:45 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: git, Jeff King, SZEDER Gábor, Thomas Rast, René Scharfe
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
> 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(-)
Yes, this makes total sense. We went overboard during the series.
> diff --git a/parse-options.c b/parse-options.c
> index 9a0484c8831..fc5b43ff0b2 100644
> --- a/parse-options.c
> +++ b/parse-options.c
> @@ -860,11 +860,11 @@ int parse_options_end(struct parse_opt_ctx_t *ctx)
> return ctx->cpidx + ctx->argc;
> }
>
> -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)
> +int 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;
> diff --git a/parse-options.h b/parse-options.h
> index bdea052c399..275fb440818 100644
> --- a/parse-options.h
> +++ b/parse-options.h
> @@ -213,11 +213,10 @@ struct option {
> * untouched and parse_options() returns the number of options
> * processed.
> */
> -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);
> +int 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);
^ permalink raw reply [flat|nested] 61+ messages in thread
* [PATCH 2/2] parse-options.c: use "enum parse_opt_result" for parse_nodash_opt()
2021-11-09 11:04 ` [PATCH 0/2] parse-options.[ch]: enum fixup & enum nit Ævar Arnfjörð Bjarmason
2021-11-09 11:04 ` [PATCH 1/2] parse-options.[ch]: revert use of "enum" for parse_options() Ævar Arnfjörð Bjarmason
@ 2021-11-09 11:04 ` Ævar Arnfjörð Bjarmason
2021-11-09 17:58 ` Junio C Hamano
1 sibling, 1 reply; 61+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-11-09 11:04 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, SZEDER Gábor, Thomas Rast,
René Scharfe, Ævar Arnfjörð Bjarmason
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(-)
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
^ permalink raw reply related [flat|nested] 61+ messages in thread
* Re: [PATCH 2/2] parse-options.c: use "enum parse_opt_result" for parse_nodash_opt()
2021-11-09 11:04 ` [PATCH 2/2] parse-options.c: use "enum parse_opt_result" for parse_nodash_opt() Ævar Arnfjörð Bjarmason
@ 2021-11-09 17:58 ` Junio C Hamano
2021-11-09 23:18 ` Ævar Arnfjörð Bjarmason
0 siblings, 1 reply; 61+ messages in thread
From: Junio C Hamano @ 2021-11-09 17:58 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: git, Jeff King, SZEDER Gábor, Thomas Rast, René Scharfe
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
> 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(-)
>
> 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).
^ permalink raw reply [flat|nested] 61+ messages in thread
* Re: [PATCH 2/2] parse-options.c: use "enum parse_opt_result" for parse_nodash_opt()
2021-11-09 17:58 ` Junio C Hamano
@ 2021-11-09 23:18 ` Ævar Arnfjörð Bjarmason
2021-11-09 23:37 ` Junio C Hamano
2021-11-10 1:27 ` [PATCH v2] " Ævar Arnfjörð Bjarmason
0 siblings, 2 replies; 61+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-11-09 23:18 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Jeff King, SZEDER Gábor, Thomas Rast, René Scharfe
On Tue, Nov 09 2021, Junio C Hamano wrote:
> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
>> 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(-)
>>
>> 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.
^ permalink raw reply [flat|nested] 61+ messages in thread
* Re: [PATCH 2/2] parse-options.c: use "enum parse_opt_result" for parse_nodash_opt()
2021-11-09 23:18 ` Ævar Arnfjörð Bjarmason
@ 2021-11-09 23:37 ` Junio C Hamano
2021-11-10 1:27 ` [PATCH v2] " Ævar Arnfjörð Bjarmason
1 sibling, 0 replies; 61+ messages in thread
From: Junio C Hamano @ 2021-11-09 23:37 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: git, Jeff King, SZEDER Gábor, Thomas Rast, René Scharfe
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>>> - 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.
OK. That sounds like a good explanation for the change, to be
recorded in the proposed log message.
Thanks.
^ permalink raw reply [flat|nested] 61+ messages in thread
* [PATCH v2] parse-options.c: use "enum parse_opt_result" for parse_nodash_opt()
2021-11-09 23:18 ` Ævar Arnfjörð Bjarmason
2021-11-09 23:37 ` Junio C Hamano
@ 2021-11-10 1:27 ` Ævar Arnfjörð Bjarmason
2021-11-11 2:01 ` Junio C Hamano
1 sibling, 1 reply; 61+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-11-10 1:27 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, SZEDER Gábor, Thomas Rast,
René Scharfe, Ævar Arnfjörð Bjarmason
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(-)
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
^ permalink raw reply related [flat|nested] 61+ messages in thread
* Re: [PATCH v2] parse-options.c: use "enum parse_opt_result" for parse_nodash_opt()
2021-11-10 1:27 ` [PATCH v2] " Ævar Arnfjörð Bjarmason
@ 2021-11-11 2:01 ` Junio C Hamano
0 siblings, 0 replies; 61+ messages in thread
From: Junio C Hamano @ 2021-11-11 2:01 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: git, Jeff King, SZEDER Gábor, Thomas Rast, René Scharfe
Ævar Arnfjörð Bjarmason <avarab@gmail.com> 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.
^ permalink raw reply [flat|nested] 61+ messages in thread
* [PATCH v3 04/10] parse-options.c: use exhaustive "case" arms for "enum parse_opt_result"
2021-10-08 19:07 ` [PATCH v3 00/10] fix bug, use more enums Ævar Arnfjörð Bjarmason
` (2 preceding siblings ...)
2021-10-08 19:07 ` [PATCH v3 03/10] parse-options.[ch]: consistently use "enum parse_opt_result" Ævar Arnfjörð Bjarmason
@ 2021-10-08 19:07 ` Ævar Arnfjörð Bjarmason
2021-10-08 19:07 ` [PATCH v3 05/10] parse-options.h: make the "flags" in "struct option" an enum Ævar Arnfjörð Bjarmason
` (5 subsequent siblings)
9 siblings, 0 replies; 61+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-10-08 19:07 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Thomas Rast, René Scharfe,
Ævar Arnfjörð Bjarmason
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(-)
diff --git a/parse-options.c b/parse-options.c
index f718242096c..e33700d6e71 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)) {
--
2.33.0.1446.g6af949f83bd
^ permalink raw reply related [flat|nested] 61+ messages in thread
* [PATCH v3 05/10] parse-options.h: make the "flags" in "struct option" an enum
2021-10-08 19:07 ` [PATCH v3 00/10] fix bug, use more enums Ævar Arnfjörð Bjarmason
` (3 preceding siblings ...)
2021-10-08 19:07 ` [PATCH v3 04/10] parse-options.c: use exhaustive "case" arms for "enum parse_opt_result" Ævar Arnfjörð Bjarmason
@ 2021-10-08 19:07 ` Ævar Arnfjörð Bjarmason
2021-10-08 19:07 ` [PATCH v3 06/10] parse-options.c: move optname() earlier in the file Ævar Arnfjörð Bjarmason
` (4 subsequent siblings)
9 siblings, 0 replies; 61+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-10-08 19:07 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Thomas Rast, René Scharfe,
Ævar Arnfjörð Bjarmason
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(-)
diff --git a/parse-options.h b/parse-options.h
index a1c7c86ad30..74b66ba6e93 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -134,7 +134,7 @@ struct option {
const char *argh;
const char *help;
- int flags;
+ enum parse_opt_option_flags flags;
parse_opt_cb *callback;
intptr_t defval;
parse_opt_ll_cb *ll_callback;
--
2.33.0.1446.g6af949f83bd
^ permalink raw reply related [flat|nested] 61+ messages in thread
* [PATCH v3 06/10] parse-options.c: move optname() earlier in the file
2021-10-08 19:07 ` [PATCH v3 00/10] fix bug, use more enums Ævar Arnfjörð Bjarmason
` (4 preceding siblings ...)
2021-10-08 19:07 ` [PATCH v3 05/10] parse-options.h: make the "flags" in "struct option" an enum Ævar Arnfjörð Bjarmason
@ 2021-10-08 19:07 ` Ævar Arnfjörð Bjarmason
2021-10-08 19:07 ` [PATCH v3 07/10] commit-graph: stop using optname() Ævar Arnfjörð Bjarmason
` (3 subsequent siblings)
9 siblings, 0 replies; 61+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-10-08 19:07 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Thomas Rast, René Scharfe,
Ævar Arnfjörð Bjarmason
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(-)
diff --git a/parse-options.c b/parse-options.c
index e33700d6e71..9e2da8383d7 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;
-}
--
2.33.0.1446.g6af949f83bd
^ permalink raw reply related [flat|nested] 61+ messages in thread
* [PATCH v3 07/10] commit-graph: stop using optname()
2021-10-08 19:07 ` [PATCH v3 00/10] fix bug, use more enums Ævar Arnfjörð Bjarmason
` (5 preceding siblings ...)
2021-10-08 19:07 ` [PATCH v3 06/10] parse-options.c: move optname() earlier in the file Ævar Arnfjörð Bjarmason
@ 2021-10-08 19:07 ` Ævar Arnfjörð Bjarmason
2021-10-08 19:07 ` [PATCH v3 08/10] parse-options.[ch]: make opt{bug,name}() "static" Ævar Arnfjörð Bjarmason
` (2 subsequent siblings)
9 siblings, 0 replies; 61+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-10-08 19:07 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Thomas Rast, René Scharfe,
Ævar Arnfjörð Bjarmason
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(-)
diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c
index 3c3de3a156f..ab8e5cd59af 100644
--- a/builtin/commit-graph.c
+++ b/builtin/commit-graph.c
@@ -172,8 +172,8 @@ static int write_option_max_new_filters(const struct option *opt,
const char *s;
*to = strtol(arg, (char **)&s, 10);
if (*s)
- return error(_("%s expects a numerical value"),
- optname(opt, opt->flags));
+ return error(_("option `%s' expects a numerical value"),
+ "max-new-filters");
}
return 0;
}
--
2.33.0.1446.g6af949f83bd
^ permalink raw reply related [flat|nested] 61+ messages in thread
* [PATCH v3 08/10] parse-options.[ch]: make opt{bug,name}() "static"
2021-10-08 19:07 ` [PATCH v3 00/10] fix bug, use more enums Ævar Arnfjörð Bjarmason
` (6 preceding siblings ...)
2021-10-08 19:07 ` [PATCH v3 07/10] commit-graph: stop using optname() Ævar Arnfjörð Bjarmason
@ 2021-10-08 19:07 ` Ævar Arnfjörð Bjarmason
2021-10-08 19:07 ` [PATCH v3 09/10] parse-options tests: test optname() output Ævar Arnfjörð Bjarmason
2021-10-08 19:07 ` [PATCH v3 10/10] parse-options: change OPT_{SHORT,UNSET} to an enum Ævar Arnfjörð Bjarmason
9 siblings, 0 replies; 61+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-10-08 19:07 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Thomas Rast, René Scharfe,
Ævar Arnfjörð Bjarmason
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(-)
diff --git a/parse-options.c b/parse-options.c
index 9e2da8383d7..64bd4c32854 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 74b66ba6e93..dd79c9c566f 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"
--
2.33.0.1446.g6af949f83bd
^ permalink raw reply related [flat|nested] 61+ messages in thread
* [PATCH v3 09/10] parse-options tests: test optname() output
2021-10-08 19:07 ` [PATCH v3 00/10] fix bug, use more enums Ævar Arnfjörð Bjarmason
` (7 preceding siblings ...)
2021-10-08 19:07 ` [PATCH v3 08/10] parse-options.[ch]: make opt{bug,name}() "static" Ævar Arnfjörð Bjarmason
@ 2021-10-08 19:07 ` Ævar Arnfjörð Bjarmason
2021-10-08 19:07 ` [PATCH v3 10/10] parse-options: change OPT_{SHORT,UNSET} to an enum Ævar Arnfjörð Bjarmason
9 siblings, 0 replies; 61+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-10-08 19:07 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Thomas Rast, René Scharfe,
Ævar Arnfjörð Bjarmason
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(-)
diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh
index da310ed29b1..d6f391a497b 100755
--- a/t/t0040-parse-options.sh
+++ b/t/t0040-parse-options.sh
@@ -168,9 +168,45 @@ test_expect_success 'long options' '
'
test_expect_success 'missing required value' '
- test_expect_code 129 test-tool parse-options -s &&
- test_expect_code 129 test-tool parse-options --string &&
- test_expect_code 129 test-tool parse-options --file
+ cat >expect <<-\EOF &&
+ error: switch `s'\'' requires a value
+ EOF
+ test_expect_code 129 test-tool parse-options -s 2>actual &&
+ test_cmp expect actual &&
+
+ cat >expect <<-\EOF &&
+ error: option `string'\'' requires a value
+ EOF
+ test_expect_code 129 test-tool parse-options --string 2>actual &&
+ test_cmp expect actual &&
+
+ cat >expect <<-\EOF &&
+ error: option `file'\'' requires a value
+ EOF
+ test_expect_code 129 test-tool parse-options --file 2>actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'superfluous value provided: boolean' '
+ cat >expect <<-\EOF &&
+ error: option `yes'\'' takes no value
+ EOF
+ test_expect_code 129 test-tool parse-options --yes=hi 2>actual &&
+ test_cmp expect actual &&
+
+ cat >expect <<-\EOF &&
+ error: option `no-yes'\'' takes no value
+ EOF
+ test_expect_code 129 test-tool parse-options --no-yes=hi 2>actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'superfluous value provided: cmdmode' '
+ cat >expect <<-\EOF &&
+ error: option `mode1'\'' takes no value
+ EOF
+ test_expect_code 129 test-tool parse-options --mode1=hi 2>actual &&
+ test_cmp expect actual
'
cat >expect <<\EOF
--
2.33.0.1446.g6af949f83bd
^ permalink raw reply related [flat|nested] 61+ messages in thread
* [PATCH v3 10/10] parse-options: change OPT_{SHORT,UNSET} to an enum
2021-10-08 19:07 ` [PATCH v3 00/10] fix bug, use more enums Ævar Arnfjörð Bjarmason
` (8 preceding siblings ...)
2021-10-08 19:07 ` [PATCH v3 09/10] parse-options tests: test optname() output Ævar Arnfjörð Bjarmason
@ 2021-10-08 19:07 ` Ævar Arnfjörð Bjarmason
9 siblings, 0 replies; 61+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-10-08 19:07 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Thomas Rast, René Scharfe,
Ævar Arnfjörð Bjarmason
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(-)
diff --git a/parse-options.c b/parse-options.c
index 64bd4c32854..2a2c0ee24f2 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;
--
2.33.0.1446.g6af949f83bd
^ permalink raw reply related [flat|nested] 61+ messages in thread