{"thread":{"id":"56604","subject":"[PATCH 00/10] fix bug, use existing enums","startedAt":"2021-09-28T13:14:38Z","lastAt":"2021-11-11T02:01:45Z","messageCount":61,"participants":["Ævar Arnfjörð Bjarmason","Junio C Hamano","Taylor Blau","René Scharfe","SZEDER Gábor"],"isPatch":true,"patchVersion":1,"patchTotal":10},"messages":[{"id":"437292","messageId":"cover-00.10-00000000000-20210928T130905Z-avarab@gmail.com","threadId":"56604","inReplyTo":null,"subject":"[PATCH 00/10] fix bug, use existing enums","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-28T13:14:21Z","receivedAt":"2021-09-28T13:14:38Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"I have some feature changes planned for parse-options.[ch], including\nones that allow us to delete some boilerplate (and sometimes buggy)\ncode in built-ins by having the API do heavier lifting.\n\nIn adding anything to the API I've found it hard to deal with it using\nits own enums inconsistently, sometimes it's an \"int\", sometimes it's\nthe \"enum\", and having the \"default\" cases makes it hard to assert\nthat you've added things to all the right places.\n\n2-6,7-10/10 is that rather straightforward conversion. 1,7/10 also\nhave fixes to existing bugs that happened due to mixing up the enum\nfields in one way or the other.\n\nÆvar Arnfjörð Bjarmason (10):\n parse-options.h: move PARSE_OPT_SHELL_EVAL between enums\n parse-options.[ch]: consistently use \"enum parse_opt_flags\"\n parse-options.[ch]: consistently use \"enum parse_opt_result\"\n parse-options.c: use exhaustive \"case\" arms for \"enum parse_opt_type\"\n parse-options.h: make the \"flags\" in \"struct option\" an enum\n parse-options.c: move optname() earlier in the file\n commit-graph: stop using optname()\n parse-options.[ch]: make opt{bug,name}() \"static\"\n parse-options tests: test optname() output\n parse-options: change OPT_{SHORT,UNSET} to an enum\n\n builtin/blame.c | 3 +\n builtin/commit-graph.c | 3 +-\n builtin/shortlog.c | 3 +\n parse-options.c | 116 ++++++++++++++++++++++++++-------------\n parse-options.h | 26 ++++-----\n t/t0040-parse-options.sh | 42 +++++++++++++-\n 6 files changed, 138 insertions(+), 55 deletions(-)\n\n-- \n2.33.0.1340.ge9f77250f2b\n\n"},{"id":"437293","messageId":"patch-01.10-19d1573a4e0-20210928T130905Z-avarab@gmail.com","threadId":"56604","inReplyTo":"cover-00.10-00000000000-20210928T130905Z-avarab@gmail.com","subject":"[PATCH 01/10] parse-options.h: move PARSE_OPT_SHELL_EVAL between enums","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-28T13:14:22Z","receivedAt":"2021-09-28T13:14:41Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Fix a bad landmine of a bug which has been with us ever since\nPARSE_OPT_SHELL_EVAL was added in 47e9cd28f8a (parseopt: wrap\nrev-parse --parseopt usage for eval consumption, 2010-06-12).\n\nIt's an argument to parse_options() and should therefore be in \"enum\nparse_opt_flags\", but it was added to the per-option \"enum\nparse_opt_option_flags\" by mistake.\n\nTherefore as soon as we'd have an enum member in the former that\nreached its value of \"1 << 8\" we'd run into a seemingly bizarre bug\nwhere that new option would turn on the unrelated PARSE_OPT_SHELL_EVAL\nin \"git rev-parse --parseopt\" by proxy.\n\nI manually checked that no other enum members suffered from such\noverlap, by setting the values to non-overlapping values, and making\nthe relevant codepaths BUG() out if the given value was above/below\nthe expected (excluding flags=0 in the case of \"enum\nparse_opt_flags\").\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n parse-options.h | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/parse-options.h b/parse-options.h\nindex 39d90882548..3a3176ae65c 100644\n--- a/parse-options.h\n+++ b/parse-options.h\n@@ -33,6 +33,7 @@ enum parse_opt_flags {\n \tPARSE_OPT_KEEP_UNKNOWN = 1 << 3,\n \tPARSE_OPT_NO_INTERNAL_HELP = 1 << 4,\n \tPARSE_OPT_ONE_SHOT = 1 << 5,\n+\tPARSE_OPT_SHELL_EVAL = 1 << 6,\n };\n \n enum parse_opt_option_flags {\n@@ -44,7 +45,6 @@ enum parse_opt_option_flags {\n \tPARSE_OPT_NODASH = 1 << 5,\n \tPARSE_OPT_LITERAL_ARGHELP = 1 << 6,\n \tPARSE_OPT_FROM_ALIAS = 1 << 7,\n-\tPARSE_OPT_SHELL_EVAL = 1 << 8,\n \tPARSE_OPT_NOCOMPLETE = 1 << 9,\n \tPARSE_OPT_COMP_ARG = 1 << 10,\n \tPARSE_OPT_CMDMODE = 1 << 11,\n-- \n2.33.0.1340.ge9f77250f2b\n\n"},{"id":"437294","messageId":"patch-02.10-289bb437eb5-20210928T130905Z-avarab@gmail.com","threadId":"56604","inReplyTo":"cover-00.10-00000000000-20210928T130905Z-avarab@gmail.com","subject":"[PATCH 02/10] parse-options.[ch]: consistently use \"enum parse_opt_flags\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-28T13:14:23Z","receivedAt":"2021-09-28T13:14:42Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Use the \"enum parse_opt_flags\" instead of an \"int flags\" as arguments\nto the various functions in parse-options.c. This will help to catch\ncases where we're not handling cases in switch statements, and\ngenerally make it obvious which \"flags\" we're referring to in this\ncase.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n parse-options.c | 13 ++++++++-----\n parse-options.h | 6 ++++--\n 2 files changed, 12 insertions(+), 7 deletions(-)\n\ndiff --git a/parse-options.c b/parse-options.c\nindex 55c5821b08d..9dce8b7f8a8 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -481,7 +481,8 @@ static void parse_options_check(const struct option *opts)\n \n static void parse_options_start_1(struct parse_opt_ctx_t *ctx,\n \t\t\t\t int argc, const char **argv, const char *prefix,\n-\t\t\t\t const struct option *options, int flags)\n+\t\t\t\t const struct option *options,\n+\t\t\t\t enum parse_opt_flags flags)\n {\n \tctx->argc = argc;\n \tctx->argv = argv;\n@@ -506,7 +507,8 @@ static void parse_options_start_1(struct parse_opt_ctx_t *ctx,\n \n void parse_options_start(struct parse_opt_ctx_t *ctx,\n \t\t\t int argc, const char **argv, const char *prefix,\n-\t\t\t const struct option *options, int flags)\n+\t\t\t const struct option *options,\n+\t\t\t enum parse_opt_flags flags)\n {\n \tmemset(ctx, 0, sizeof(*ctx));\n \tparse_options_start_1(ctx, argc, argv, prefix, options, flags);\n@@ -838,8 +840,9 @@ int parse_options_end(struct parse_opt_ctx_t *ctx)\n }\n \n int parse_options(int argc, const char **argv, const char *prefix,\n-\t\t const struct option *options, const char * const usagestr[],\n-\t\t int flags)\n+\t\t const struct option *options,\n+\t\t const char * const usagestr[],\n+\t\t enum parse_opt_flags flags)\n {\n \tstruct parse_opt_ctx_t ctx;\n \tstruct option *real_options;\n@@ -861,7 +864,7 @@ int parse_options(int argc, const char **argv, const char *prefix,\n \tcase PARSE_OPT_NON_OPTION:\n \tcase PARSE_OPT_DONE:\n \t\tbreak;\n-\tdefault: /* PARSE_OPT_UNKNOWN */\n+\tcase PARSE_OPT_UNKNOWN:\n \t\tif (ctx.argv[0][1] == '-') {\n \t\t\terror(_(\"unknown option `%s'\"), ctx.argv[0] + 2);\n \t\t} else if (isascii(*ctx.opt)) {\ndiff --git a/parse-options.h b/parse-options.h\nindex 3a3176ae65c..fb5aafd4f7b 100644\n--- a/parse-options.h\n+++ b/parse-options.h\n@@ -213,7 +213,8 @@ struct option {\n */\n int parse_options(int argc, const char **argv, const char *prefix,\n \t\t const struct option *options,\n-\t\t const char * const usagestr[], int flags);\n+\t\t const char * const usagestr[],\n+\t\t enum parse_opt_flags flags);\n \n NORETURN void usage_with_options(const char * const *usagestr,\n \t\t\t\t const struct option *options);\n@@ -270,7 +271,8 @@ struct parse_opt_ctx_t {\n \n void parse_options_start(struct parse_opt_ctx_t *ctx,\n \t\t\t int argc, const char **argv, const char *prefix,\n-\t\t\t const struct option *options, int flags);\n+\t\t\t const struct option *options,\n+\t\t\t enum parse_opt_flags flags);\n \n int parse_options_step(struct parse_opt_ctx_t *ctx,\n \t\t const struct option *options,\n-- \n2.33.0.1340.ge9f77250f2b\n\n"},{"id":"437295","messageId":"patch-03.10-fbcbaa47329-20210928T130905Z-avarab@gmail.com","threadId":"56604","inReplyTo":"cover-00.10-00000000000-20210928T130905Z-avarab@gmail.com","subject":"[PATCH 03/10] parse-options.[ch]: consistently use \"enum parse_opt_result\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-28T13:14:24Z","receivedAt":"2021-09-28T13:14:44Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Use the \"enum parse_opt_result\" instead of an \"int flags\" as the\nreturn value of the applicable functions in parse-options.c.\n\nThis will help catch future bugs, such as the missing \"case\" arms in\nthe two existing users of the API in \"blame.c\" and \"shortlog.c\". A\nthird caller in 309be813c9b (update-index: migrate to parse-options\nAPI, 2010-12-01) was already checking for these.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n builtin/blame.c | 3 +++\n builtin/shortlog.c | 3 +++\n parse-options.c | 15 ++++++++-------\n parse-options.h | 15 ++++++++-------\n 4 files changed, 22 insertions(+), 14 deletions(-)\n\ndiff --git a/builtin/blame.c b/builtin/blame.c\nindex 641523ff9af..9273fb222d5 100644\n--- a/builtin/blame.c\n+++ b/builtin/blame.c\n@@ -917,6 +917,9 @@ int cmd_blame(int argc, const char **argv, const char *prefix)\n \t\t\t PARSE_OPT_KEEP_DASHDASH | PARSE_OPT_KEEP_ARGV0);\n \tfor (;;) {\n \t\tswitch (parse_options_step(&ctx, options, blame_opt_usage)) {\n+\t\tcase PARSE_OPT_NON_OPTION:\n+\t\tcase PARSE_OPT_UNKNOWN:\n+\t\t\tbreak;\n \t\tcase PARSE_OPT_HELP:\n \t\tcase PARSE_OPT_ERROR:\n \t\t\texit(129);\ndiff --git a/builtin/shortlog.c b/builtin/shortlog.c\nindex 3e7ab1ca821..e7f7af5de3f 100644\n--- a/builtin/shortlog.c\n+++ b/builtin/shortlog.c\n@@ -374,6 +374,9 @@ int cmd_shortlog(int argc, const char **argv, const char *prefix)\n \n \tfor (;;) {\n \t\tswitch (parse_options_step(&ctx, options, shortlog_usage)) {\n+\t\tcase PARSE_OPT_NON_OPTION:\n+\t\tcase PARSE_OPT_UNKNOWN:\n+\t\t\tbreak;\n \t\tcase PARSE_OPT_HELP:\n \t\tcase PARSE_OPT_ERROR:\n \t\t\texit(129);\ndiff --git a/parse-options.c b/parse-options.c\nindex 9dce8b7f8a8..799cd884f2b 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -703,9 +703,9 @@ static int usage_with_options_internal(struct parse_opt_ctx_t *,\n \t\t\t\t const char * const *,\n \t\t\t\t const struct option *, int, int);\n \n-int parse_options_step(struct parse_opt_ctx_t *ctx,\n-\t\t const struct option *options,\n-\t\t const char * const usagestr[])\n+enum parse_opt_result parse_options_step(struct parse_opt_ctx_t *ctx,\n+\t\t\t\t\t const struct option *options,\n+\t\t\t\t\t const char * const usagestr[])\n {\n \tint internal_help = !(ctx->flags & PARSE_OPT_NO_INTERNAL_HELP);\n \n@@ -839,10 +839,11 @@ int parse_options_end(struct parse_opt_ctx_t *ctx)\n \treturn ctx->cpidx + ctx->argc;\n }\n \n-int parse_options(int argc, const char **argv, const char *prefix,\n-\t\t const struct option *options,\n-\t\t const char * const usagestr[],\n-\t\t enum parse_opt_flags flags)\n+enum parse_opt_result parse_options(int argc, const char **argv,\n+\t\t\t\t const char *prefix,\n+\t\t\t\t const struct option *options,\n+\t\t\t\t const char * const usagestr[],\n+\t\t\t\t enum parse_opt_flags flags)\n {\n \tstruct parse_opt_ctx_t ctx;\n \tstruct option *real_options;\ndiff --git a/parse-options.h b/parse-options.h\nindex fb5aafd4f7b..d931300f4d6 100644\n--- a/parse-options.h\n+++ b/parse-options.h\n@@ -211,10 +211,11 @@ struct option {\n * untouched and parse_options() returns the number of options\n * processed.\n */\n-int parse_options(int argc, const char **argv, const char *prefix,\n-\t\t const struct option *options,\n-\t\t const char * const usagestr[],\n-\t\t enum parse_opt_flags flags);\n+enum parse_opt_result parse_options(int argc, const char **argv,\n+\t\t\t\t const char *prefix,\n+\t\t\t\t const struct option *options,\n+\t\t\t\t const char * const usagestr[],\n+\t\t\t\t enum parse_opt_flags flags);\n \n NORETURN void usage_with_options(const char * const *usagestr,\n \t\t\t\t const struct option *options);\n@@ -274,9 +275,9 @@ void parse_options_start(struct parse_opt_ctx_t *ctx,\n \t\t\t const struct option *options,\n \t\t\t enum parse_opt_flags flags);\n \n-int parse_options_step(struct parse_opt_ctx_t *ctx,\n-\t\t const struct option *options,\n-\t\t const char * const usagestr[]);\n+enum parse_opt_result parse_options_step(struct parse_opt_ctx_t *ctx,\n+\t\t\t\t\t const struct option *options,\n+\t\t\t\t\t const char * const usagestr[]);\n \n int parse_options_end(struct parse_opt_ctx_t *ctx);\n \n-- \n2.33.0.1340.ge9f77250f2b\n\n"},{"id":"437296","messageId":"patch-04.10-624a19000e1-20210928T130905Z-avarab@gmail.com","threadId":"56604","inReplyTo":"cover-00.10-00000000000-20210928T130905Z-avarab@gmail.com","subject":"[PATCH 04/10] parse-options.c: use exhaustive \"case\" arms for \"enum parse_opt_type\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-28T13:14:25Z","receivedAt":"2021-09-28T13:14:45Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Change code in get_value(), parse_options_check() etc. to do away with\nthe \"default\" case in favor of exhaustively checking the relevant\nfields.\n\nThe added \"return -1\" is needed for the GCC version commented on\ninline, my local clang 11.0.1-2 does not require it. Let's add it for\nnow to appease GCC.\n\nThe added \"special types\" etc. comments correspond to the relevant\ncomments and ordering on the \"enum parse_opt_type\". Let's try to keep\nthe same order and commentary as there where possible for\nclarity. This doesn't reach that end-state, and due to the different\nhandling of options it's probably not worth it to get there, but let's\nmatch its ordering where it's easy to do so.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n parse-options.c | 43 ++++++++++++++++++++++++++++++++++++++-----\n parse-options.h | 2 +-\n 2 files changed, 39 insertions(+), 6 deletions(-)\n\ndiff --git a/parse-options.c b/parse-options.c\nindex 799cd884f2b..733cbfa8821 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -219,9 +219,14 @@ static enum parse_opt_result get_value(struct parse_opt_ctx_t *p,\n \t\t\t\t optname(opt, flags));\n \t\treturn 0;\n \n-\tdefault:\n+\t/* special types */\n+\tcase OPTION_END:\n+\tcase OPTION_GROUP:\n+\tcase OPTION_NUMBER:\n+\tcase OPTION_ALIAS:\n \t\tBUG(\"opt->type %d should not happen\", opt->type);\n \t}\n+\treturn -1; /* gcc 10.2.1-6's -Werror=return-type */\n }\n \n static enum parse_opt_result parse_short_opt(struct parse_opt_ctx_t *p,\n@@ -468,8 +473,15 @@ static void parse_options_check(const struct option *opts)\n \t\t\tBUG(\"OPT_ALIAS() should not remain at this point. \"\n \t\t\t \"Are you using parse_options_step() directly?\\n\"\n \t\t\t \"That case is not supported yet.\");\n-\t\tdefault:\n-\t\t\t; /* ok. (usually accepts an argument) */\n+\n+\t\tcase OPTION_BITOP:\n+\t\tcase OPTION_END:\n+\t\tcase OPTION_FILENAME:\n+\t\tcase OPTION_GROUP:\n+\t\tcase OPTION_INTEGER:\n+\t\tcase OPTION_MAGNITUDE:\n+\t\tcase OPTION_STRING:\n+\t\t\tbreak;\n \t\t}\n \t\tif (opts->argh &&\n \t\t strcspn(opts->argh, \" _\") != strlen(opts->argh))\n@@ -543,7 +555,15 @@ static void show_negated_gitcomp(const struct option *opts, int show_all,\n \t\tcase OPTION_SET_INT:\n \t\t\thas_unset_form = 1;\n \t\t\tbreak;\n-\t\tdefault:\n+\t\t/* special types */\n+\t\tcase OPTION_END:\n+\t\tcase OPTION_GROUP:\n+\t\tcase OPTION_NUMBER:\n+\t\tcase OPTION_ALIAS:\n+\t\t/* options with no arguments */\n+\t\tcase OPTION_BITOP:\n+\t\t/* options with arguments (usually) */\n+\t\tcase OPTION_LOWLEVEL_CALLBACK:\n \t\t\tbreak;\n \t\t}\n \t\tif (!has_unset_form)\n@@ -593,7 +613,20 @@ static int show_gitcomp(const struct option *opts, int show_all)\n \t\t\t\tbreak;\n \t\t\tsuffix = \"=\";\n \t\t\tbreak;\n-\t\tdefault:\n+\t\t/* special types */\n+\t\tcase OPTION_END:\n+\t\tcase OPTION_NUMBER:\n+\t\tcase OPTION_ALIAS:\n+\n+\t\t/* options with no arguments */\n+\t\tcase OPTION_BIT:\n+\t\tcase OPTION_NEGBIT:\n+\t\tcase OPTION_BITOP:\n+\t\tcase OPTION_COUNTUP:\n+\t\tcase OPTION_SET_INT:\n+\n+\t\t/* options with arguments (usually) */\n+\t\tcase OPTION_LOWLEVEL_CALLBACK:\n \t\t\tbreak;\n \t\t}\n \t\tif (opts->flags & PARSE_OPT_COMP_ARG)\ndiff --git a/parse-options.h b/parse-options.h\nindex d931300f4d6..a1c7c86ad30 100644\n--- a/parse-options.h\n+++ b/parse-options.h\n@@ -264,7 +264,7 @@ struct parse_opt_ctx_t {\n \tconst char **out;\n \tint argc, cpidx, total;\n \tconst char *opt;\n-\tint flags;\n+\tenum parse_opt_flags flags;\n \tconst char *prefix;\n \tconst char **alias_groups; /* must be in groups of 3 elements! */\n \tstruct option *updated_options;\n-- \n2.33.0.1340.ge9f77250f2b\n\n"},{"id":"437297","messageId":"patch-05.10-697e432c012-20210928T130905Z-avarab@gmail.com","threadId":"56604","inReplyTo":"cover-00.10-00000000000-20210928T130905Z-avarab@gmail.com","subject":"[PATCH 05/10] parse-options.h: make the \"flags\" in \"struct option\" an enum","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-28T13:14:26Z","receivedAt":"2021-09-28T13:14:46Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Change the \"flags\" members of \"struct option\" to refer to their\ncorresponding \"enum\" defined earlier in the file.\n\nThe benefit of changing this to an enum isn't as great as with some\n\"enum parse_opt_type\" as we'll always check this as a bitfield, so we\ncan't rely on the compiler checking \"case\" arms for us. But let's do\nit for consistency with the rest of the file.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n parse-options.h | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/parse-options.h b/parse-options.h\nindex a1c7c86ad30..74b66ba6e93 100644\n--- a/parse-options.h\n+++ b/parse-options.h\n@@ -134,7 +134,7 @@ struct option {\n \tconst char *argh;\n \tconst char *help;\n \n-\tint flags;\n+\tenum parse_opt_option_flags flags;\n \tparse_opt_cb *callback;\n \tintptr_t defval;\n \tparse_opt_ll_cb *ll_callback;\n-- \n2.33.0.1340.ge9f77250f2b\n\n"},{"id":"437298","messageId":"patch-07.10-b0b313795c7-20210928T130905Z-avarab@gmail.com","threadId":"56604","inReplyTo":"cover-00.10-00000000000-20210928T130905Z-avarab@gmail.com","subject":"[PATCH 07/10] commit-graph: stop using optname()","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-28T13:14:28Z","receivedAt":"2021-09-28T13:14:48Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Stop using optname() in builtin/commit-graph.c to emit an error with\nthe --max-new-filters option. This changes code added in 809e0327f57\n(builtin/commit-graph.c: introduce '--max-new-filters=',\n2020-09-18).\n\nSee 9440b831ad5 (parse-options: replace opterror() with optname(),\n2018-11-10) for why using optname() like this is considered bad,\ni.e. it's assembling human-readable output piecemeal, and the \"option\n`X'\" at the start can't be translated.\n\nIt didn't matter in this case, but this code was also buggy in its use\nof \"opt->flags\" to optname(), that function expects flags, but not\n*those* flags.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n builtin/commit-graph.c | 3 +--\n 1 file changed, 1 insertion(+), 2 deletions(-)\n\ndiff --git a/builtin/commit-graph.c b/builtin/commit-graph.c\nindex 0386f5c7755..36552db89fe 100644\n--- a/builtin/commit-graph.c\n+++ b/builtin/commit-graph.c\n@@ -172,8 +172,7 @@ static int write_option_max_new_filters(const struct option *opt,\n \t\tconst char *s;\n \t\t*to = strtol(arg, (char **)&s, 10);\n \t\tif (*s)\n-\t\t\treturn error(_(\"%s expects a numerical value\"),\n-\t\t\t\t optname(opt, opt->flags));\n+\t\t\treturn error(_(\"option `max-new-filters' expects a numerical value\"));\n \t}\n \treturn 0;\n }\n-- \n2.33.0.1340.ge9f77250f2b\n\n"},{"id":"437299","messageId":"patch-06.10-c065f7d7362-20210928T130905Z-avarab@gmail.com","threadId":"56604","inReplyTo":"cover-00.10-00000000000-20210928T130905Z-avarab@gmail.com","subject":"[PATCH 06/10] parse-options.c: move optname() earlier in the file","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-28T13:14:27Z","receivedAt":"2021-09-28T13:14:50Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"In preparation for making \"optname\" a static function move it above\nits first user in parse-options.c.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n parse-options.c | 30 +++++++++++++++---------------\n 1 file changed, 15 insertions(+), 15 deletions(-)\n\ndiff --git a/parse-options.c b/parse-options.c\nindex 733cbfa8821..238a283db5d 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -22,6 +22,21 @@ int optbug(const struct option *opt, const char *reason)\n \treturn error(\"BUG: switch '%c' %s\", opt->short_name, reason);\n }\n \n+const char *optname(const struct option *opt, int flags)\n+{\n+\tstatic struct strbuf sb = STRBUF_INIT;\n+\n+\tstrbuf_reset(&sb);\n+\tif (flags & OPT_SHORT)\n+\t\tstrbuf_addf(&sb, \"switch `%c'\", opt->short_name);\n+\telse if (flags & OPT_UNSET)\n+\t\tstrbuf_addf(&sb, \"option `no-%s'\", opt->long_name);\n+\telse\n+\t\tstrbuf_addf(&sb, \"option `%s'\", opt->long_name);\n+\n+\treturn sb.buf;\n+}\n+\n static enum parse_opt_result get_arg(struct parse_opt_ctx_t *p,\n \t\t\t\t const struct option *opt,\n \t\t\t\t int flags, const char **arg)\n@@ -1037,18 +1052,3 @@ void NORETURN usage_msg_opt(const char *msg,\n \tfprintf(stderr, \"fatal: %s\\n\\n\", msg);\n \tusage_with_options(usagestr, options);\n }\n-\n-const char *optname(const struct option *opt, int flags)\n-{\n-\tstatic struct strbuf sb = STRBUF_INIT;\n-\n-\tstrbuf_reset(&sb);\n-\tif (flags & OPT_SHORT)\n-\t\tstrbuf_addf(&sb, \"switch `%c'\", opt->short_name);\n-\telse if (flags & OPT_UNSET)\n-\t\tstrbuf_addf(&sb, \"option `no-%s'\", opt->long_name);\n-\telse\n-\t\tstrbuf_addf(&sb, \"option `%s'\", opt->long_name);\n-\n-\treturn sb.buf;\n-}\n-- \n2.33.0.1340.ge9f77250f2b\n\n"},{"id":"437300","messageId":"patch-08.10-07ba0e6f995-20210928T130905Z-avarab@gmail.com","threadId":"56604","inReplyTo":"cover-00.10-00000000000-20210928T130905Z-avarab@gmail.com","subject":"[PATCH 08/10] parse-options.[ch]: make opt{bug,name}() \"static\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-28T13:14:29Z","receivedAt":"2021-09-28T13:14:51Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Change these two functions to \"static\", the last user of \"optname()\"\noutside of parse-options.c itself went away in the preceding commit,\nfor the reasons noted in 9440b831ad5 (parse-options: replace\nopterror() with optname(), 2018-11-10) we shouldn't be adding any more\nusers of it.\n\nThe \"optbug()\" function was never used outside of parse-options.c, but\nwas made non-static in 1f275b7c4ca (parse-options: export opterr,\noptbug, 2011-08-11). I think the only external user of optname() was\nthe commit-graph.c caller added in 09e0327f57 (builtin/commit-graph.c:\nintroduce '--max-new-filters=', 2020-09-18).\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n parse-options.c | 4 ++--\n parse-options.h | 3 ---\n 2 files changed, 2 insertions(+), 5 deletions(-)\n\ndiff --git a/parse-options.c b/parse-options.c\nindex 238a283db5d..57e95846e83 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -11,7 +11,7 @@ static int disallow_abbreviated_options;\n #define OPT_SHORT 1\n #define OPT_UNSET 2\n \n-int optbug(const struct option *opt, const char *reason)\n+static int optbug(const struct option *opt, const char *reason)\n {\n \tif (opt->long_name) {\n \t\tif (opt->short_name)\n@@ -22,7 +22,7 @@ int optbug(const struct option *opt, const char *reason)\n \treturn error(\"BUG: switch '%c' %s\", opt->short_name, reason);\n }\n \n-const char *optname(const struct option *opt, int flags)\n+static const char *optname(const struct option *opt, int flags)\n {\n \tstatic struct strbuf sb = STRBUF_INIT;\n \ndiff --git a/parse-options.h b/parse-options.h\nindex 74b66ba6e93..dd79c9c566f 100644\n--- a/parse-options.h\n+++ b/parse-options.h\n@@ -224,9 +224,6 @@ NORETURN void usage_msg_opt(const char *msg,\n \t\t\t const char * const *usagestr,\n \t\t\t const struct option *options);\n \n-int optbug(const struct option *opt, const char *reason);\n-const char *optname(const struct option *opt, int flags);\n-\n /*\n * Use these assertions for callbacks that expect to be called with NONEG and\n * NOARG respectively, and do not otherwise handle the \"unset\" and \"arg\"\n-- \n2.33.0.1340.ge9f77250f2b\n\n"},{"id":"437301","messageId":"patch-09.10-ce508fddc8f-20210928T130905Z-avarab@gmail.com","threadId":"56604","inReplyTo":"cover-00.10-00000000000-20210928T130905Z-avarab@gmail.com","subject":"[PATCH 09/10] parse-options tests: test optname() output","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-28T13:14:30Z","receivedAt":"2021-09-28T13:14:53Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"There were no tests for checking the specific output that we'll\ngenerate in optname(), let's add some. That output was added back in\n4a59fd13122 (Add a simple option parser., 2007-10-15).\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n t/t0040-parse-options.sh | 42 +++++++++++++++++++++++++++++++++++++---\n 1 file changed, 39 insertions(+), 3 deletions(-)\n\ndiff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh\nindex da310ed29b1..d6f391a497b 100755\n--- a/t/t0040-parse-options.sh\n+++ b/t/t0040-parse-options.sh\n@@ -168,9 +168,45 @@ test_expect_success 'long options' '\n '\n \n test_expect_success 'missing required value' '\n-\ttest_expect_code 129 test-tool parse-options -s &&\n-\ttest_expect_code 129 test-tool parse-options --string &&\n-\ttest_expect_code 129 test-tool parse-options --file\n+\tcat >expect <<-\\EOF &&\n+\terror: switch `s'\\'' requires a value\n+\tEOF\n+\ttest_expect_code 129 test-tool parse-options -s 2>actual &&\n+\ttest_cmp expect actual &&\n+\n+\tcat >expect <<-\\EOF &&\n+\terror: option `string'\\'' requires a value\n+\tEOF\n+\ttest_expect_code 129 test-tool parse-options --string 2>actual &&\n+\ttest_cmp expect actual &&\n+\n+\tcat >expect <<-\\EOF &&\n+\terror: option `file'\\'' requires a value\n+\tEOF\n+\ttest_expect_code 129 test-tool parse-options --file 2>actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'superfluous value provided: boolean' '\n+\tcat >expect <<-\\EOF &&\n+\terror: option `yes'\\'' takes no value\n+\tEOF\n+\ttest_expect_code 129 test-tool parse-options --yes=hi 2>actual &&\n+\ttest_cmp expect actual &&\n+\n+\tcat >expect <<-\\EOF &&\n+\terror: option `no-yes'\\'' takes no value\n+\tEOF\n+\ttest_expect_code 129 test-tool parse-options --no-yes=hi 2>actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'superfluous value provided: cmdmode' '\n+\tcat >expect <<-\\EOF &&\n+\terror: option `mode1'\\'' takes no value\n+\tEOF\n+\ttest_expect_code 129 test-tool parse-options --mode1=hi 2>actual &&\n+\ttest_cmp expect actual\n '\n \n cat >expect <<\\EOF\n-- \n2.33.0.1340.ge9f77250f2b\n\n"},{"id":"437302","messageId":"patch-10.10-a28ab961125-20210928T130905Z-avarab@gmail.com","threadId":"56604","inReplyTo":"cover-00.10-00000000000-20210928T130905Z-avarab@gmail.com","subject":"[PATCH 10/10] parse-options: change OPT_{SHORT,UNSET} to an enum","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-28T13:14:31Z","receivedAt":"2021-09-28T13:14:54Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Change the comparisons against OPT_SHORT and OPT_UNSET to an enum\nwhich keeps track of how a given option got parsed. The case of \"0\"\nwas an implicit OPT_LONG, so let's add an explicit label for it.\n\nDue to the xor in 0f1930c5875 (parse-options: allow positivation of\noptions starting, with no-, 2012-02-25) the code already relied on\nthis being set back to 0. To avoid refactoring the logic involved in\nthat let's just start the enum at \"0\" instead of the usual \"1<<0\" (1),\nbut BUG() out if we don't have one of our expected flags.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n parse-options.c | 23 ++++++++++++++---------\n 1 file changed, 14 insertions(+), 9 deletions(-)\n\ndiff --git a/parse-options.c b/parse-options.c\nindex 57e95846e83..1eb3b51f753 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -8,8 +8,11 @@\n \n static int disallow_abbreviated_options;\n \n-#define OPT_SHORT 1\n-#define OPT_UNSET 2\n+enum opt_parsed {\n+\tOPT_LONG = 0,\n+\tOPT_SHORT = 1<<0,\n+\tOPT_UNSET = 1<<1,\n+};\n \n static int optbug(const struct option *opt, const char *reason)\n {\n@@ -22,7 +25,7 @@ static int optbug(const struct option *opt, const char *reason)\n \treturn error(\"BUG: switch '%c' %s\", opt->short_name, reason);\n }\n \n-static const char *optname(const struct option *opt, int flags)\n+static const char *optname(const struct option *opt, enum opt_parsed flags)\n {\n \tstatic struct strbuf sb = STRBUF_INIT;\n \n@@ -31,15 +34,17 @@ static const char *optname(const struct option *opt, int flags)\n \t\tstrbuf_addf(&sb, \"switch `%c'\", opt->short_name);\n \telse if (flags & OPT_UNSET)\n \t\tstrbuf_addf(&sb, \"option `no-%s'\", opt->long_name);\n-\telse\n+\telse if (flags == OPT_LONG)\n \t\tstrbuf_addf(&sb, \"option `%s'\", opt->long_name);\n+\telse\n+\t\tBUG(\"optname() got unknown flags %d\", flags);\n \n \treturn sb.buf;\n }\n \n static enum parse_opt_result get_arg(struct parse_opt_ctx_t *p,\n \t\t\t\t const struct option *opt,\n-\t\t\t\t int flags, const char **arg)\n+\t\t\t\t enum opt_parsed flags, const char **arg)\n {\n \tif (p->opt) {\n \t\t*arg = p->opt;\n@@ -65,7 +70,7 @@ static void fix_filename(const char *prefix, const char **file)\n static enum parse_opt_result opt_command_mode_error(\n \tconst struct option *opt,\n \tconst struct option *all_opts,\n-\tint flags)\n+\tenum opt_parsed flags)\n {\n \tconst struct option *that;\n \tstruct strbuf that_name = STRBUF_INIT;\n@@ -97,7 +102,7 @@ static enum parse_opt_result opt_command_mode_error(\n static enum parse_opt_result get_value(struct parse_opt_ctx_t *p,\n \t\t\t\t const struct option *opt,\n \t\t\t\t const struct option *all_opts,\n-\t\t\t\t int flags)\n+\t\t\t\t enum opt_parsed flags)\n {\n \tconst char *s, *arg;\n \tconst int unset = flags & OPT_UNSET;\n@@ -318,11 +323,11 @@ static enum parse_opt_result parse_long_opt(\n \tconst struct option *all_opts = options;\n \tconst char *arg_end = strchrnul(arg, '=');\n \tconst struct option *abbrev_option = NULL, *ambiguous_option = NULL;\n-\tint abbrev_flags = 0, ambiguous_flags = 0;\n+\tenum opt_parsed abbrev_flags = OPT_LONG, ambiguous_flags = OPT_LONG;\n \n \tfor (; options->type != OPTION_END; options++) {\n \t\tconst char *rest, *long_name = options->long_name;\n-\t\tint flags = 0, opt_flags = 0;\n+\t\tenum opt_parsed flags = OPT_LONG, opt_flags = OPT_LONG;\n \n \t\tif (!long_name)\n \t\t\tcontinue;\n-- \n2.33.0.1340.ge9f77250f2b\n\n"},{"id":"437397","messageId":"xmqqilykgs2k.fsf@gitster.g","threadId":"56604","inReplyTo":"patch-02.10-289bb437eb5-20210928T130905Z-avarab@gmail.com","subject":"Re: [PATCH 02/10] parse-options.[ch]: consistently use \"enum parse_opt_flags\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-09-29T00:10:11Z","receivedAt":"2021-09-29T00:10:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> Use the \"enum parse_opt_flags\" instead of an \"int flags\" as arguments\n> to the various functions in parse-options.c. This will help to catch\n> cases where we're not handling cases in switch statements, and\n\nI am not sure about most part this change, and the claim the second\nsentence makes is certainly dubious. Let's look at the first hunk:\n\n> diff --git a/parse-options.c b/parse-options.c\n> index 55c5821b08d..9dce8b7f8a8 100644\n> --- a/parse-options.c\n> +++ b/parse-options.c\n> @@ -481,7 +481,8 @@ static void parse_options_check(const struct option *opts)\n\n static void parse_options_start_1(struct parse_opt_ctx_t *ctx,\n \t\t\t\t int argc, const char **argv, const char *prefix,\n-\t\t\t\t const struct option *options, int flags)\n+\t\t\t\t const struct option *options,\n+\t\t\t\t enum parse_opt_flags flags)\n {\n \tctx->argc = argc;\n \tctx->argv = argv;\n \tif (!(flags & PARSE_OPT_ONE_SHOT)) {\n \t\tctx->argc--;\n \t\tctx->argv++;\n \t}\n \tctx->total = ctx->argc;\n \tctx->out = argv;\n \tctx->prefix = prefix;\n \tctx->cpidx = ((flags & PARSE_OPT_KEEP_ARGV0) != 0);\n \tctx->flags = flags;\n \tif ((flags & PARSE_OPT_KEEP_UNKNOWN) &&\n \t (flags & PARSE_OPT_STOP_AT_NON_OPTION) &&\n \t !(flags & PARSE_OPT_ONE_SHOT))\n \t\tBUG(\"STOP_AT_NON_OPTION and KEEP_UNKNOWN don't go together\");\n \tif ((flags & PARSE_OPT_ONE_SHOT) &&\n \t (flags & PARSE_OPT_KEEP_ARGV0))\n \t\tBUG(\"Can't keep argv0 if you don't have it\");\n \tparse_options_check(options);\n }\n \nThe \"flags\" parameter does not take a value that is an \"enum\" in the\nusual \"enumeration\" sense at all. Even though parse_opt_flags\ndefines 7 distinct \"enum\" values, each enumerated value is a small\nunsigned integer with only single bit set, the caller can throw a\nvalue that is not among these 7 by OR'ing them together. We would\nnot sensibly do\n\n\tswitch (flags) {\n\tcase PARSE_OPT_KEEP_UNKNOWN:\n\t\t...\n\nIn general, I am not all that enthusiastic for such a(n) (ab)use of\n\"enum\" for bit patterns, much less than \"enumerate all possible\nvalues to make sure compilers would help us catch missing logic\".\n\nThe \"parse_opt_result\" enum is the \"right\" kind of enumeration that\nI can stand behind fully. The hunk related to that enum in this\npatch is quite reasonable and takes advantage of the fact that the\nenum is meant to be the enumeration of all possible values.\n\nCompared to it, parse_opt_flags and parse_opt_option_flags, not\nreally.\n\nIf we wanted to really clean up the latter two, perhaps we should\ndefine the bit (which *can* be made to a proper \"here are the all\npossible values\" enumeration), like this:\n\n enum parse_opt_flags_bit {\n\tPARSE_OPT_KEEP_DASHDASH_BIT = 0,\n PARSE_OPT_STOP_AT_NON_OPTION_BIT,\n\tPARSE_OPT_KEEP_ARGV0_BIT,\n\t...\n\tPARSE_OPT_SHELL_EVAL_BIT,\n };\n\nand then update the users to use (1U << PARSE_OPT_$FLAG$_BIT), or\ndrop the pretense that it is a good idea to use enum for bit pattern\nand use the CPP macro, i.e.\n\n #define PARSE_OPT_KEEP_DASHDASH (1U<<0)\n #define PARSE_OPT_STOP_AT_NON_OPTION (1U<<1)\n ...\n #define PARSE_OPT_SHELL_EVAL (1U<<6)\n\nto make it clear that we do not mean these are \"enumeration of\npossible values\".\n\nThanks.\n"},{"id":"437399","messageId":"xmqqee98gryf.fsf@gitster.g","threadId":"56604","inReplyTo":"patch-03.10-fbcbaa47329-20210928T130905Z-avarab@gmail.com","subject":"Re: [PATCH 03/10] parse-options.[ch]: consistently use \"enum parse_opt_result\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-09-29T00:12:40Z","receivedAt":"2021-09-29T00:12:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> Use the \"enum parse_opt_result\" instead of an \"int flags\" as the\n> return value of the applicable functions in parse-options.c.\n>\n> This will help catch future bugs, such as the missing \"case\" arms in\n> the two existing users of the API in \"blame.c\" and \"shortlog.c\". A\n> third caller in 309be813c9b (update-index: migrate to parse-options\n> API, 2010-12-01) was already checking for these.\n\nAs I said, this one I am happy about. The previous step had a\nchange related to this enum mixed in it (by mistake, I think),\nwhich belongs here instead.\n"},{"id":"437400","messageId":"xmqq5yukgrlm.fsf@gitster.g","threadId":"56604","inReplyTo":"patch-04.10-624a19000e1-20210928T130905Z-avarab@gmail.com","subject":"Re: [PATCH 04/10] parse-options.c: use exhaustive \"case\" arms for \"enum parse_opt_type\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-09-29T00:20:21Z","receivedAt":"2021-09-29T00:20:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> Change code in get_value(), parse_options_check() etc. to do away with\n> the \"default\" case in favor of exhaustively checking the relevant\n> fields.\n\nI am not sure if this is a good idea in the bigger picture.\n\nIf we know that unlisted cases should not happen, having \"default:\nBUG()\" without explicitly listing them is just as expressive as the\nresult of this patch with much shorter code.\n\nWhen a new enum member is added without adding corresponding\nprocessing of the new value, either way it will be caught as a\nBUG(). Removing \"default: BUG()\" does allow you to catch such an\nerror at compilation time, and keeping it may prevent you from doing\nso, but in practice, you'd be adding test coverage for the new case,\nwhich means that, even if your compiler is not cooperating, your\ntest suite addition will hit \"default: BUG();\" in such a case.\n\nSo ...\n\n> -\tdefault:\n> +\t/* special types */\n> +\tcase OPTION_END:\n> +\tcase OPTION_GROUP:\n> +\tcase OPTION_NUMBER:\n> +\tcase OPTION_ALIAS:\n> \t\tBUG(\"opt->type %d should not happen\", opt->type);\n> \t}\n> +\treturn -1; /* gcc 10.2.1-6's -Werror=return-type */\n> }\n"},{"id":"437401","messageId":"xmqq1r58grie.fsf@gitster.g","threadId":"56604","inReplyTo":"patch-05.10-697e432c012-20210928T130905Z-avarab@gmail.com","subject":"Re: [PATCH 05/10] parse-options.h: make the \"flags\" in \"struct option\" an enum","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-09-29T00:22:17Z","receivedAt":"2021-09-29T00:22:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> Change the \"flags\" members of \"struct option\" to refer to their\n> corresponding \"enum\" defined earlier in the file.\n>\n> The benefit of changing this to an enum isn't as great as with some\n> \"enum parse_opt_type\" as we'll always check this as a bitfield, so we\n> can't rely on the compiler checking \"case\" arms for us. But let's do\n> it for consistency with the rest of the file.\n\nFor the same reason as my comment on [02/10], this is more or less\n\"Meh\" for me. If we wanted to make any change here, I'd probably\nsuggest making it \"unsigned\" to clarify that this is a OR'ed bit\npatterns.\n\n> Signed-off-by: Ævar Arnfjörð Bjarmason \n> ---\n> parse-options.h | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/parse-options.h b/parse-options.h\n> index a1c7c86ad30..74b66ba6e93 100644\n> --- a/parse-options.h\n> +++ b/parse-options.h\n> @@ -134,7 +134,7 @@ struct option {\n> \tconst char *argh;\n> \tconst char *help;\n> \n> -\tint flags;\n> +\tenum parse_opt_option_flags flags;\n> \tparse_opt_cb *callback;\n> \tintptr_t defval;\n> \tparse_opt_ll_cb *ll_callback;\n"},{"id":"437407","messageId":"xmqqwnn0fbm5.fsf@gitster.g","threadId":"56604","inReplyTo":"patch-10.10-a28ab961125-20210928T130905Z-avarab@gmail.com","subject":"Re: [PATCH 10/10] parse-options: change OPT_{SHORT,UNSET} to an enum","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-09-29T00:50:58Z","receivedAt":"2021-09-29T00:51:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> Change the comparisons against OPT_SHORT and OPT_UNSET to an enum\n> which keeps track of how a given option got parsed. The case of \"0\"\n> was an implicit OPT_LONG, so let's add an explicit label for it.\n\nI think this is going backwards. If we want to change anything in\nthis area, you may\n\n (1) spell the values not as 1 and 2 but as bit-shifts (1U<<0),\n (1U<<1), and\n\n (2) turn \"flags\" into \"unsigned\".\n\nbut I do not think of a reason why it would be a good move to use\nenum for \"these are bits\", for the same reason as my comments on\n[2/10].\n\nThanks.\n"},{"id":"437444","messageId":"87tui3vk8y.fsf@evledraar.gmail.com","threadId":"56604","inReplyTo":"xmqq5yukgrlm.fsf@gitster.g","subject":"Re: [PATCH 04/10] parse-options.c: use exhaustive \"case\" arms for \"enum parse_opt_type\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-29T08:48:52Z","receivedAt":"2021-09-29T08:50:37Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Sep 28 2021, Junio C Hamano wrote:\n\n> Ævar Arnfjörð Bjarmason writes:\n>\n>> Change code in get_value(), parse_options_check() etc. to do away with\n>> the \"default\" case in favor of exhaustively checking the relevant\n>> fields.\n>\n> I am not sure if this is a good idea in the bigger picture.\n>\n> If we know that unlisted cases should not happen, having \"default:\n> BUG()\" without explicitly listing them is just as expressive as the\n> result of this patch with much shorter code.\n>\n> When a new enum member is added without adding corresponding\n> processing of the new value, either way it will be caught as a\n> BUG(). Removing \"default: BUG()\" does allow you to catch such an\n> error at compilation time, and keeping it may prevent you from doing\n> so, but in practice, you'd be adding test coverage for the new case,\n> which means that, even if your compiler is not cooperating, your\n> test suite addition will hit \"default: BUG();\" in such a case.\n\nYes we'll catch them since these are loops over all options. But that\nassumes that:\n\n1) You have a test that's stressing the relevant entry point\n\n2) That you run all your tests, e.g. one of these entry points is the\n \"git completion\" one\n\nI think listing the remaining enum arms is a small price to pay for\nhaving that all moved to compile-time.\n\n>> -\tdefault:\n>> +\t/* special types */\n>> +\tcase OPTION_END:\n>> +\tcase OPTION_GROUP:\n>> +\tcase OPTION_NUMBER:\n>> +\tcase OPTION_ALIAS:\n>> \t\tBUG(\"opt->type %d should not happen\", opt->type);\n>> \t}\n>> +\treturn -1; /* gcc 10.2.1-6's -Werror=return-type */\n>> }\n\n"},{"id":"437445","messageId":"87mtnvvj3c.fsf@evledraar.gmail.com","threadId":"56604","inReplyTo":"xmqqilykgs2k.fsf@gitster.g","subject":"Re: [PATCH 02/10] parse-options.[ch]: consistently use \"enum parse_opt_flags\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-29T08:53:18Z","receivedAt":"2021-09-29T09:15:07Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Sep 28 2021, Junio C Hamano wrote:\n\n> Ævar Arnfjörð Bjarmason writes:\n>\n>> Use the \"enum parse_opt_flags\" instead of an \"int flags\" as arguments\n>> to the various functions in parse-options.c. This will help to catch\n>> cases where we're not handling cases in switch statements, and\n>\n> I am not sure about most part this change, and the claim the second\n> sentence makes is certainly dubious. Let's look at the first hunk:\n>\n>> diff --git a/parse-options.c b/parse-options.c\n>> index 55c5821b08d..9dce8b7f8a8 100644\n>> --- a/parse-options.c\n>> +++ b/parse-options.c\n>> @@ -481,7 +481,8 @@ static void parse_options_check(const struct option *opts)\n>\n> static void parse_options_start_1(struct parse_opt_ctx_t *ctx,\n> \t\t\t\t int argc, const char **argv, const char *prefix,\n> -\t\t\t\t const struct option *options, int flags)\n> +\t\t\t\t const struct option *options,\n> +\t\t\t\t enum parse_opt_flags flags)\n> {\n> \tctx->argc = argc;\n> \tctx->argv = argv;\n> \tif (!(flags & PARSE_OPT_ONE_SHOT)) {\n> \t\tctx->argc--;\n> \t\tctx->argv++;\n> \t}\n> \tctx->total = ctx->argc;\n> \tctx->out = argv;\n> \tctx->prefix = prefix;\n> \tctx->cpidx = ((flags & PARSE_OPT_KEEP_ARGV0) != 0);\n> \tctx->flags = flags;\n> \tif ((flags & PARSE_OPT_KEEP_UNKNOWN) &&\n> \t (flags & PARSE_OPT_STOP_AT_NON_OPTION) &&\n> \t !(flags & PARSE_OPT_ONE_SHOT))\n> \t\tBUG(\"STOP_AT_NON_OPTION and KEEP_UNKNOWN don't go together\");\n> \tif ((flags & PARSE_OPT_ONE_SHOT) &&\n> \t (flags & PARSE_OPT_KEEP_ARGV0))\n> \t\tBUG(\"Can't keep argv0 if you don't have it\");\n> \tparse_options_check(options);\n> }\n> \n> The \"flags\" parameter does not take a value that is an \"enum\" in the\n> usual \"enumeration\" sense at all. Even though parse_opt_flags\n> defines 7 distinct \"enum\" values, each enumerated value is a small\n> unsigned integer with only single bit set, the caller can throw a\n> value that is not among these 7 by OR'ing them together. We would\n> not sensibly do\n>\n> \tswitch (flags) {\n> \tcase PARSE_OPT_KEEP_UNKNOWN:\n> \t\t...\n>\n> In general, I am not all that enthusiastic for such a(n) (ab)use of\n> \"enum\" for bit patterns, much less than \"enumerate all possible\n> values to make sure compilers would help us catch missing logic\".\n\nI agree that it's not as nice as enums where the fields are mutually\nexclusive, since those can be checked via \"case\" arms, and this is\n\"unchecked\" bitfields.\n\nSo e.g. the bug I fixed in 01/10 would not be found by a compiler I have\naccess to (and I don't think one currently exists).\n\nBut I think this is perfectly good use of enums, we use this\nenums-as-bitfields pattern in various other places,\ne.g. builtin/rebase.c's \"flags\", the \"commit_graph_write_flags\",\n\"expire_reflog_flags\" & \"todo_item_flags\", just to name a few from some\nquick grepping.\n\nOne advantage is that integrates nicely with some wider C\ntooling. E.g. before this series, starting \"git stash show\" under gdb\nand inspecting flags:\n\n (gdb) p flags\n $1 = 9\n\nAnd after:\n\n (gdb) p flags\n $1 = (PARSE_OPT_KEEP_DASHDASH | PARSE_OPT_KEEP_UNKNOWN)\n\nSo the type information and bitfield-ness are retained.\n\nAlthough you might argue that it leads you into a trap, as adding:\n\n flags |= PARSE_OPT_LASTARG_DEFAULT;\n\nWill result in:\n\n (gdb) p flags\n $2 = (PARSE_OPT_KEEP_DASHDASH | PARSE_OPT_KEEP_UNKNOWN | PARSE_OPT_NO_INTERNAL_HELP)\n\nI.e. it decodes the enum based on the int value & its known labels, and\nit just so happens that PARSE_OPT_LASTARG_DEFAULT has the same value as\nPARSE_OPT_NO_INTERNAL_HELP in an unrelated enum.\n\n> The \"parse_opt_result\" enum is the \"right\" kind of enumeration that\n> I can stand behind fully. The hunk related to that enum in this\n> patch is quite reasonable and takes advantage of the fact that the\n> enum is meant to be the enumeration of all possible values.\n>\n> Compared to it, parse_opt_flags and parse_opt_option_flags, not\n> really.\n>\n> If we wanted to really clean up the latter two, perhaps we should\n> define the bit (which *can* be made to a proper \"here are the all\n> possible values\" enumeration), like this:\n>\n> enum parse_opt_flags_bit {\n> \tPARSE_OPT_KEEP_DASHDASH_BIT = 0,\n> PARSE_OPT_STOP_AT_NON_OPTION_BIT,\n> \tPARSE_OPT_KEEP_ARGV0_BIT,\n> \t...\n> \tPARSE_OPT_SHELL_EVAL_BIT,\n> };\n>\n> and then update the users to use (1U << PARSE_OPT_$FLAG$_BIT), or\n> drop the pretense that it is a good idea to use enum for bit pattern\n> and use the CPP macro, i.e.\n>\n> #define PARSE_OPT_KEEP_DASHDASH (1U<<0)\n> #define PARSE_OPT_STOP_AT_NON_OPTION (1U<<1)\n> ...\n> #define PARSE_OPT_SHELL_EVAL (1U<<6)\n>\n> to make it clear that we do not mean these are \"enumeration of\n> possible values\".\n\nI'm not sure what the former suggestion here buys us, but the latter\nwill drop the type information as noted above, i.e. you'll get a:\n\n (gdb) p flags\n $1 = 9\n"},{"id":"437460","messageId":"xmqqbl4bfmgb.fsf@gitster.g","threadId":"56604","inReplyTo":"87mtnvvj3c.fsf@evledraar.gmail.com","subject":"Re: [PATCH 02/10] parse-options.[ch]: consistently use \"enum parse_opt_flags\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-09-29T15:09:08Z","receivedAt":"2021-09-29T15:09:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> On Tue, Sep 28 2021, Junio C Hamano wrote:\n>\n>> Ævar Arnfjörð Bjarmason writes:\n>>\n>>> Use the \"enum parse_opt_flags\" instead of an \"int flags\" as arguments\n>>> to the various functions in parse-options.c. This will help to catch\n>>> cases where we're not handling cases in switch statements, and\n\n ...\n\n> But I think this is perfectly good use of enums, we use this\n> enums-as-bitfields pattern in various other places,\n> e.g. builtin/rebase.c's \"flags\", the \"commit_graph_write_flags\",\n> \"expire_reflog_flags\" & \"todo_item_flags\", just to name a few from some\n> quick grepping.\n\nMany codepaths already misusing is not an excuse to add another ;-)\n\nBut ...\n\n> One advantage is that integrates nicely with some wider C\n> tooling. E.g. before this series, starting \"git stash show\" under gdb\n> and inspecting flags:\n>\n> (gdb) p flags\n> $1 = 9\n>\n> And after:\n>\n> (gdb) p flags\n> $1 = (PARSE_OPT_KEEP_DASHDASH | PARSE_OPT_KEEP_UNKNOWN)\n\n... this is a pleasant surprise---the last time I checked, debuggers\nwere that clever to notice that the distinct values are names for\nindividual bits. I can buy this as an argument for using enums for\nnames for individual bits. For this to work, obviously, variable\nand struct members need to be given the appropriate type.\n\nSo I agree with the change in 2/10.\n\nExcept that one place there was a change related to a different enum\nthat is a true enumeration in this step. It belongs to 3/10, I\nthink. Also, the sales pitch for this step in the proposed commit\nlog message needs rewriting---this will \"not\" help to catch cases\nwhere we're not handling cases in switch statements; if you are\nselling it because you think it will help debuggers and other\ntooling, let's describe it as such.\n\nEven though I think debuggers are overrated ;-)\n\nThanks.\n"},{"id":"437461","messageId":"xmqq7dezfm7m.fsf@gitster.g","threadId":"56604","inReplyTo":"87tui3vk8y.fsf@evledraar.gmail.com","subject":"Re: [PATCH 04/10] parse-options.c: use exhaustive \"case\" arms for \"enum parse_opt_type\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-09-29T15:14:21Z","receivedAt":"2021-09-29T15:14:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> I think listing the remaining enum arms is a small price to pay for\n> having that all moved to compile-time.\n\nI can sympathize with that point of view, since I used to think the\nsame way, but I am not yet convinced. An example like this from\nyour postimage, which doubles the size of a switch statement with\nempty case arms, demonstrates that it is not a \"small\" price.\n\nAdmittedly, the original switch statement is particularly bad,\nthough ;-)\n\n \t\tswitch (opts->type) {\n \t\tcase OPTION_STRING:\n \t\tcase OPTION_FILENAME:\n \t\tcase OPTION_INTEGER:\n \t\tcase OPTION_MAGNITUDE:\n \t\tcase OPTION_CALLBACK:\n \t\tcase OPTION_BIT:\n \t\tcase OPTION_NEGBIT:\n \t\tcase OPTION_COUNTUP:\n \t\tcase OPTION_SET_INT:\n \t\t\thas_unset_form = 1;\n \t\t\tbreak;\n-\t\tdefault:\n+\t\t/* special types */\n+\t\tcase OPTION_END:\n+\t\tcase OPTION_GROUP:\n+\t\tcase OPTION_NUMBER:\n+\t\tcase OPTION_ALIAS:\n+\t\t/* options with no arguments */\n+\t\tcase OPTION_BITOP:\n+\t\t/* options with arguments (usually) */\n+\t\tcase OPTION_LOWLEVEL_CALLBACK:\n \t\t\tbreak;\n \t\t}\n"},{"id":"437464","messageId":"xmqqy27fe5fr.fsf@gitster.g","threadId":"56604","inReplyTo":"patch-02.10-289bb437eb5-20210928T130905Z-avarab@gmail.com","subject":"Re: [PATCH 02/10] parse-options.[ch]: consistently use \"enum parse_opt_flags\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-09-29T16:02:00Z","receivedAt":"2021-09-29T16:02:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> Use the \"enum parse_opt_flags\" instead of an \"int flags\" as arguments\n> ...\n\n> @@ -861,7 +864,7 @@ int parse_options(int argc, const char **argv, const char *prefix,\n> \tcase PARSE_OPT_NON_OPTION:\n> \tcase PARSE_OPT_DONE:\n> \t\tbreak;\n> -\tdefault: /* PARSE_OPT_UNKNOWN */\n> +\tcase PARSE_OPT_UNKNOWN:\n> \t\tif (ctx.argv[0][1] == '-') {\n> \t\t\terror(_(\"unknown option `%s'\"), ctx.argv[0] + 2);\n> \t\t} else if (isascii(*ctx.opt)) {\n\nThis part does not belong to this step or 03/10 (parse_opt_result).\nIt belongs to 04/10 (drop default from switching on enum).\n"},{"id":"437470","messageId":"YVSiOMaKNoDZ3SlO@nand.local","threadId":"56604","inReplyTo":"patch-07.10-b0b313795c7-20210928T130905Z-avarab@gmail.com","subject":"Re: [PATCH 07/10] commit-graph: stop using optname()","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-09-29T17:28:24Z","receivedAt":"2021-09-29T17:28:29Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Sep 28, 2021 at 03:14:28PM +0200, Ævar Arnfjörð Bjarmason wrote:\n> Stop using optname() in builtin/commit-graph.c to emit an error with\n> the --max-new-filters option. This changes code added in 809e0327f57\n> (builtin/commit-graph.c: introduce '--max-new-filters=',\n> 2020-09-18).\n>\n> See 9440b831ad5 (parse-options: replace opterror() with optname(),\n> 2018-11-10) for why using optname() like this is considered bad,\n> i.e. it's assembling human-readable output piecemeal, and the \"option\n> `X'\" at the start can't be translated.\n\nIn fact, using optname there (which blames to me) was a mistake for an\neven simpler reason: there is no abbreviated form of\n`--max-new-filters`, and we know that by the time this error is emitted\nthat we got the positive form instead of `--no-max-new-filters`.\n\nSo we could have just written the option's name verbatim, and given\ntranslators something easier to work with.\n\n> It didn't matter in this case, but this code was also buggy in its use\n> of \"opt->flags\" to optname(), that function expects flags, but not\n> *those* flags.\n>\n> Signed-off-by: Ævar Arnfjörð Bjarmason \n> ---\n> builtin/commit-graph.c | 3 +--\n> 1 file changed, 1 insertion(+), 2 deletions(-)\n>\n> diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c\n> index 0386f5c7755..36552db89fe 100644\n> --- a/builtin/commit-graph.c\n> +++ b/builtin/commit-graph.c\n> @@ -172,8 +172,7 @@ static int write_option_max_new_filters(const struct option *opt,\n> \t\tconst char *s;\n> \t\t*to = strtol(arg, (char **)&s, 10);\n> \t\tif (*s)\n> -\t\t\treturn error(_(\"%s expects a numerical value\"),\n> -\t\t\t\t optname(opt, opt->flags));\n> +\t\t\treturn error(_(\"option `max-new-filters' expects a numerical value\"));\n\nMakes sense. The `'-style quoting is still weird to me. It is consistent\nwith some of the conversions in 9440b831ad5, but most importantly with\nhow parse-options.c:get_value() behaves (because it calls optname\nunderneath).\n\n(This has nothing to do with your patch, but I thought the custom\nwrite_option_max_new_filters callback was weird when I wrote it. It's\nworking around trying to make the negated form set a value to `-1`\ninstead of `0`. But it's an annoying hack, because we have to call\nstrtol() ourselves when we're not negated. *sigh*).\n\nLooks good to me.\n\nThanks,\nTaylor\n"},{"id":"437686","messageId":"87tui0ri8j.fsf@evledraar.gmail.com","threadId":"56604","inReplyTo":"YVSiOMaKNoDZ3SlO@nand.local","subject":"Re: [PATCH 07/10] commit-graph: stop using optname()","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-10-01T13:16:38Z","receivedAt":"2021-10-01T13:24:02Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Sep 29 2021, Taylor Blau wrote:\n\n> On Tue, Sep 28, 2021 at 03:14:28PM +0200, Ævar Arnfjörð Bjarmason wrote:\n>> Stop using optname() in builtin/commit-graph.c to emit an error with\n>> the --max-new-filters option. This changes code added in 809e0327f57\n>> (builtin/commit-graph.c: introduce '--max-new-filters=',\n>> 2020-09-18).\n>>\n>> See 9440b831ad5 (parse-options: replace opterror() with optname(),\n>> 2018-11-10) for why using optname() like this is considered bad,\n>> i.e. it's assembling human-readable output piecemeal, and the \"option\n>> `X'\" at the start can't be translated.\n>\n> In fact, using optname there (which blames to me) was a mistake for an\n> even simpler reason: there is no abbreviated form of\n> `--max-new-filters`, and we know that by the time this error is emitted\n> that we got the positive form instead of `--no-max-new-filters`.\n>\n> So we could have just written the option's name verbatim, and given\n> translators something easier to work with.\n\nAs an aside: Did you intend for this to work:\n\n git commit-graph write --max-new-filters=123 --no-max-new-filters\n\nIt's in the docstring, but then you're using OPT_CALLBACK_F(), but just\nto set a flag of \"0\", so a OPT_CALLBACK() would do, along with a\nPARSE_OPT_NONEG.\n\nI'm about to re-roll this, but won't change that, but I think it\nprobably makes sense as a follow-on cleanuup.\n\nI think you'd probably want a BUG_ON_OPT_NEG() instead for the \"unset\"\nhandling here. This seems like another case of mixing the state of\nparse_options() with that of flags for the underlying API that we\ndiscussed elsewhere either for this command or multi-pack-index. But\nmore on that below...\n\nAlso the usage if --no-* is wanted should not be:\n\n [--no-max-new-filters] [--max-new-filters ]\n\nBut is currently:\n\n [--[no-]max-new-filters ]\n\nWhich says the --no-* will take the , but it won't.\n\n>> It didn't matter in this case, but this code was also buggy in its use\n>> of \"opt->flags\" to optname(), that function expects flags, but not\n>> *those* flags.\n>>\n>> Signed-off-by: Ævar Arnfjörð Bjarmason \n>> ---\n>> builtin/commit-graph.c | 3 +--\n>> 1 file changed, 1 insertion(+), 2 deletions(-)\n>>\n>> diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c\n>> index 0386f5c7755..36552db89fe 100644\n>> --- a/builtin/commit-graph.c\n>> +++ b/builtin/commit-graph.c\n>> @@ -172,8 +172,7 @@ static int write_option_max_new_filters(const struct option *opt,\n>> \t\tconst char *s;\n>> \t\t*to = strtol(arg, (char **)&s, 10);\n>> \t\tif (*s)\n>> -\t\t\treturn error(_(\"%s expects a numerical value\"),\n>> -\t\t\t\t optname(opt, opt->flags));\n>> +\t\t\treturn error(_(\"option `max-new-filters' expects a numerical value\"));\n>\n> Makes sense. The `'-style quoting is still weird to me. It is consistent\n> with some of the conversions in 9440b831ad5, but most importantly with\n> how parse-options.c:get_value() behaves (because it calls optname\n> underneath).\n\nYeah I just reproduced the existing output here.\n\n> (This has nothing to do with your patch, but I thought the custom\n> write_option_max_new_filters callback was weird when I wrote it. It's\n> working around trying to make the negated form set a value to `-1`\n> instead of `0`. But it's an annoying hack, because we have to call\n> strtol() ourselves when we're not negated. *sigh*).\n\n...on the \"more on that below\", looks like you intended that -1 handling\nin some way, but I don't really see why yet, other than the \"mixing the\nstate\" I noted above.\n"},{"id":"437688","messageId":"cover-v2-00.11-00000000000-20211001T142631Z-avarab@gmail.com","threadId":"56604","inReplyTo":"cover-00.10-00000000000-20210928T130905Z-avarab@gmail.com","subject":"[PATCH v2 00/11] fix bug, use existing enums","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-10-01T14:29:06Z","receivedAt":"2021-10-01T14:29:23Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"I have some feature changes planned for parse-options.[ch], including\nones that allow us to delete some boilerplate (and sometimes buggy)\ncode in built-ins by having the API do heavier lifting.\n\nIn adding anything to the API I've found it hard to deal with it using\nits own enums inconsistently, sometimes it's an \"int\", sometimes it's\nthe \"enum\", and having the \"default\" cases makes it hard to assert\nthat you've added things to all the right places.\n\nThis re-roll of the v1[1] should hopefully address all the feedback on\nthat version, particularly the motivation for the enum-as-bitfield\nlabeling, as expanded on in 02/11.\n\n1. http://lore.kernel.org/git/cover-00.10-00000000000-20210928T130905Z-avarab@gmail.com\n\nÆvar Arnfjörð Bjarmason (11):\n parse-options.h: move PARSE_OPT_SHELL_EVAL between enums\n parse-options.[ch]: consistently use \"enum parse_opt_flags\"\n parse-options.[ch]: consistently use \"enum parse_opt_result\"\n parse-options.c: use exhaustive \"case\" arms for \"enum\n parse_opt_result\"\n parse-options.c: use exhaustive \"case\" arms for \"enum parse_opt_type\"\n parse-options.h: make the \"flags\" in \"struct option\" an enum\n parse-options.c: move optname() earlier in the file\n commit-graph: stop using optname()\n parse-options.[ch]: make opt{bug,name}() \"static\"\n parse-options tests: test optname() output\n parse-options: change OPT_{SHORT,UNSET} to an enum\n\n builtin/blame.c | 3 +\n builtin/commit-graph.c | 3 +-\n builtin/shortlog.c | 3 +\n parse-options.c | 135 ++++++++++++++++++++++++++-------------\n parse-options.h | 26 ++++----\n t/t0040-parse-options.sh | 42 +++++++++++-\n 6 files changed, 151 insertions(+), 61 deletions(-)\n\nRange-diff against v1:\n 1: 19d1573a4e0 = 1: 553931702df parse-options.h: move PARSE_OPT_SHELL_EVAL between enums\n 2: 289bb437eb5 ! 2: 99f5251c557 parse-options.[ch]: consistently use \"enum parse_opt_flags\"\n @@ Commit message\n parse-options.[ch]: consistently use \"enum parse_opt_flags\"\n \n Use the \"enum parse_opt_flags\" instead of an \"int flags\" as arguments\n - to the various functions in parse-options.c. This will help to catch\n - cases where we're not handling cases in switch statements, and\n - generally make it obvious which \"flags\" we're referring to in this\n - case.\n + to the various functions in parse-options.c.\n +\n + In C enums aren't first-class types, and the \"enum\n + parse_opt_option_flag\" uses a enum-as-bitfield pattern. So unlike\n + exhaustively enumerated \"case\" arms we're not going to get validation\n + that we used the \"right\" enum labels.\n +\n + I.e. this won't catch the sort of bug that was fixed with\n + \"PARSE_OPT_SHELL_EVAL\" in the preceding commit.\n +\n + But there's still a benefit to doing this when it comes to the wider C\n + ecosystem. E.g. the GNU debugger (gdb) will helpfully detect and print\n + out meaningful enum labels in this case. Here's the output before and\n + after when breaking in \"parse_options()\" after invoking \"git stash\n + show\":\n +\n + Before:\n +\n + (gdb) p flags\n + $1 = 9\n +\n + After:\n +\n + (gdb) p flags\n + $1 = (PARSE_OPT_KEEP_DASHDASH | PARSE_OPT_KEEP_UNKNOWN)\n +\n + Of course as noted in[1] there's a limit to this smartness,\n + i.e. manually setting it with unrelated enum labels won't be\n + caught. There are some third-party extensions to do more exhaustive\n + checking[2], perhaps we'll be able to make use of them sooner than\n + later.\n +\n + We've also got prior art using this pattern in the codebase. See\n + e.g. \"enum bloom_filter_computed\" added in 312cff52074 (bloom: split\n + 'get_bloom_filter()' in two, 2020-09-16) and the \"permitted\" enum\n + added in ce910287e72 (add -p: fix checking of user input, 2020-08-17).\n +\n + 1. https://lore.kernel.org/git/87mtnvvj3c.fsf@evledraar.gmail.com/\n + 2. https://github.com/sinelaw/elfs-clang-plugins/blob/master/enums_conversion/README.md\n \n Signed-off-by: Ævar Arnfjörð Bjarmason \n \n @@ parse-options.c: int parse_options_end(struct parse_opt_ctx_t *ctx)\n {\n \tstruct parse_opt_ctx_t ctx;\n \tstruct option *real_options;\n -@@ parse-options.c: int parse_options(int argc, const char **argv, const char *prefix,\n - \tcase PARSE_OPT_NON_OPTION:\n - \tcase PARSE_OPT_DONE:\n - \t\tbreak;\n --\tdefault: /* PARSE_OPT_UNKNOWN */\n -+\tcase PARSE_OPT_UNKNOWN:\n - \t\tif (ctx.argv[0][1] == '-') {\n - \t\t\terror(_(\"unknown option `%s'\"), ctx.argv[0] + 2);\n - \t\t} else if (isascii(*ctx.opt)) {\n \n ## parse-options.h ##\n @@ parse-options.h: struct option {\n 3: fbcbaa47329 ! 3: be198e42df9 parse-options.[ch]: consistently use \"enum parse_opt_result\"\n @@ Commit message\n third caller in 309be813c9b (update-index: migrate to parse-options\n API, 2010-12-01) was already checking for these.\n \n + As can be seen when trying to sort through the deluge of warnings\n + produced when compiling this with CC=g++ (mostly unrelated to this\n + change) we're not consistently using \"enum parse_opt_result\" even now,\n + i.e. we'll return error() and \"return 0;\". See f41179f16ba\n + (parse-options: avoid magic return codes, 2019-01-27) for a commit\n + which started changing some of that.\n +\n + I'm not doing any more of that exhaustive migration here, and it's\n + probably not worthwhile past the point of being able to check \"enum\n + parse_opt_result\" in switch().\n +\n Signed-off-by: Ævar Arnfjörð Bjarmason \n \n ## builtin/blame.c ##\n @@ builtin/shortlog.c: int cmd_shortlog(int argc, const char **argv, const char *pr\n \t\t\texit(129);\n \n ## parse-options.c ##\n -@@ parse-options.c: static int usage_with_options_internal(struct parse_opt_ctx_t *,\n - \t\t\t\t const char * const *,\n - \t\t\t\t const struct option *, int, int);\n +@@ parse-options.c: static void free_preprocessed_options(struct option *options)\n + \tfree(options);\n + }\n \n +-static int usage_with_options_internal(struct parse_opt_ctx_t *,\n +-\t\t\t\t const char * const *,\n +-\t\t\t\t const struct option *, int, int);\n +-\n -int parse_options_step(struct parse_opt_ctx_t *ctx,\n -\t\t const struct option *options,\n -\t\t const char * const usagestr[])\n ++static enum parse_opt_result usage_with_options_internal(struct parse_opt_ctx_t *,\n ++\t\t\t\t\t\t\t const char * const *,\n ++\t\t\t\t\t\t\t const struct option *,\n ++\t\t\t\t\t\t\t int, int);\n ++\n +enum parse_opt_result parse_options_step(struct parse_opt_ctx_t *ctx,\n +\t\t\t\t\t const struct option *options,\n +\t\t\t\t\t const char * const usagestr[])\n @@ parse-options.c: int parse_options_end(struct parse_opt_ctx_t *ctx)\n {\n \tstruct parse_opt_ctx_t ctx;\n \tstruct option *real_options;\n +@@ parse-options.c: static int usage_argh(const struct option *opts, FILE *outfile)\n + #define USAGE_OPTS_WIDTH 24\n + #define USAGE_GAP 2\n + \n +-static int usage_with_options_internal(struct parse_opt_ctx_t *ctx,\n +-\t\t\t\t const char * const *usagestr,\n +-\t\t\t\t const struct option *opts, int full, int err)\n ++static enum parse_opt_result usage_with_options_internal(struct parse_opt_ctx_t *ctx,\n ++\t\t\t\t\t\t\t const char * const *usagestr,\n ++\t\t\t\t\t\t\t const struct option *opts,\n ++\t\t\t\t\t\t\t int full, int err)\n + {\n + \tFILE *outfile = err ? stderr : stdout;\n + \tint need_newline;\n \n ## parse-options.h ##\n @@ parse-options.h: struct option {\n -: ----------- > 4: a253db7a60d parse-options.c: use exhaustive \"case\" arms for \"enum parse_opt_result\"\n 4: 624a19000e1 ! 5: 467828780d0 parse-options.c: use exhaustive \"case\" arms for \"enum parse_opt_type\"\n @@ Commit message\n handling of options it's probably not worth it to get there, but let's\n match its ordering where it's easy to do so.\n \n + There was a discussion about whether this was worth the added\n + verbosity, as argued in[1] I think it's worth it for getting\n + compile-time checking when adding new option types. We *should* have\n + tests for some of these, but e.g. in the show_gitcomp() case one might\n + run through the whole test suite and only hit a missing case at the\n + end on the completion tests.\n +\n + This technically changes the handling of OPTION_END, but it's\n + obviously the right thing to do. We're calling this code from within a\n + loop that uses OPTION_END as a break condition, so it was never caught\n + by the \"default\" case.\n +\n + So let's make encountering OPTION_END a BUG(), just like it already is\n + in the get_value() handling added in 4a59fd13122 (Add a simple option\n + parser., 2007-10-15).\n +\n + 1. https://lore.kernel.org/git/87tui3vk8y.fsf@evledraar.gmail.com/\n +\n Signed-off-by: Ævar Arnfjörð Bjarmason \n \n ## parse-options.c ##\n @@ parse-options.c: static enum parse_opt_result get_value(struct parse_opt_ctx_t *\n }\n \n static enum parse_opt_result parse_short_opt(struct parse_opt_ctx_t *p,\n +@@ parse-options.c: static void parse_options_check(const struct option *opts)\n + \t\t\terr |= optbug(opts, \"uses feature \"\n + \t\t\t\t\t\"not supported for dashless options\");\n + \t\tswitch (opts->type) {\n ++\t\tcase OPTION_END:\n ++\t\t\tBUG(\"unreachable\");\n ++\n + \t\tcase OPTION_COUNTUP:\n + \t\tcase OPTION_BIT:\n + \t\tcase OPTION_NEGBIT:\n @@ parse-options.c: static void parse_options_check(const struct option *opts)\n \t\t\tBUG(\"OPT_ALIAS() should not remain at this point. \"\n \t\t\t \"Are you using parse_options_step() directly?\\n\"\n @@ parse-options.c: static void parse_options_check(const struct option *opts)\n -\t\t\t; /* ok. (usually accepts an argument) */\n +\n +\t\tcase OPTION_BITOP:\n -+\t\tcase OPTION_END:\n +\t\tcase OPTION_FILENAME:\n +\t\tcase OPTION_GROUP:\n +\t\tcase OPTION_INTEGER:\n @@ parse-options.c: static void parse_options_check(const struct option *opts)\n \t\t}\n \t\tif (opts->argh &&\n \t\t strcspn(opts->argh, \" _\") != strlen(opts->argh))\n +@@ parse-options.c: static void show_negated_gitcomp(const struct option *opts, int show_all,\n + \t\t\tcontinue;\n + \n + \t\tswitch (opts->type) {\n ++\t\tcase OPTION_END:\n ++\t\t\tBUG(\"unreachable\");\n ++\n + \t\tcase OPTION_STRING:\n + \t\tcase OPTION_FILENAME:\n + \t\tcase OPTION_INTEGER:\n @@ parse-options.c: static void show_negated_gitcomp(const struct option *opts, int show_all,\n \t\tcase OPTION_SET_INT:\n \t\t\thas_unset_form = 1;\n \t\t\tbreak;\n -\t\tdefault:\n +\t\t/* special types */\n -+\t\tcase OPTION_END:\n +\t\tcase OPTION_GROUP:\n +\t\tcase OPTION_NUMBER:\n +\t\tcase OPTION_ALIAS:\n @@ parse-options.c: static void show_negated_gitcomp(const struct option *opts, int\n \t\t\tbreak;\n \t\t}\n \t\tif (!has_unset_form)\n +@@ parse-options.c: static int show_gitcomp(const struct option *opts, int show_all)\n + \t\t\tcontinue;\n + \n + \t\tswitch (opts->type) {\n ++\t\tcase OPTION_END:\n ++\t\t\tBUG(\"unreachable\");\n + \t\tcase OPTION_GROUP:\n + \t\t\tcontinue;\n + \t\tcase OPTION_STRING:\n @@ parse-options.c: static int show_gitcomp(const struct option *opts, int show_all)\n \t\t\t\tbreak;\n \t\t\tsuffix = \"=\";\n \t\t\tbreak;\n -\t\tdefault:\n +\t\t/* special types */\n -+\t\tcase OPTION_END:\n +\t\tcase OPTION_NUMBER:\n +\t\tcase OPTION_ALIAS:\n +\n 5: 697e432c012 = 6: 34669327550 parse-options.h: make the \"flags\" in \"struct option\" an enum\n 6: c065f7d7362 = 7: e82a4e477d5 parse-options.c: move optname() earlier in the file\n 7: b0b313795c7 = 8: 58683b3d89d commit-graph: stop using optname()\n 8: 07ba0e6f995 = 9: 8cbee660174 parse-options.[ch]: make opt{bug,name}() \"static\"\n 9: ce508fddc8f = 10: f727f683eb1 parse-options tests: test optname() output\n10: a28ab961125 = 11: 4fbc07fc7fd parse-options: change OPT_{SHORT,UNSET} to an enum\n-- \n2.33.0.1374.gc8f4fa74caf\n\n"},{"id":"437689","messageId":"patch-v2-01.11-553931702df-20211001T142631Z-avarab@gmail.com","threadId":"56604","inReplyTo":"cover-v2-00.11-00000000000-20211001T142631Z-avarab@gmail.com","subject":"[PATCH v2 01/11] parse-options.h: move PARSE_OPT_SHELL_EVAL between enums","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-10-01T14:29:07Z","receivedAt":"2021-10-01T14:29:24Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Fix a bad landmine of a bug which has been with us ever since\nPARSE_OPT_SHELL_EVAL was added in 47e9cd28f8a (parseopt: wrap\nrev-parse --parseopt usage for eval consumption, 2010-06-12).\n\nIt's an argument to parse_options() and should therefore be in \"enum\nparse_opt_flags\", but it was added to the per-option \"enum\nparse_opt_option_flags\" by mistake.\n\nTherefore as soon as we'd have an enum member in the former that\nreached its value of \"1 << 8\" we'd run into a seemingly bizarre bug\nwhere that new option would turn on the unrelated PARSE_OPT_SHELL_EVAL\nin \"git rev-parse --parseopt\" by proxy.\n\nI manually checked that no other enum members suffered from such\noverlap, by setting the values to non-overlapping values, and making\nthe relevant codepaths BUG() out if the given value was above/below\nthe expected (excluding flags=0 in the case of \"enum\nparse_opt_flags\").\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n parse-options.h | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/parse-options.h b/parse-options.h\nindex 39d90882548..3a3176ae65c 100644\n--- a/parse-options.h\n+++ b/parse-options.h\n@@ -33,6 +33,7 @@ enum parse_opt_flags {\n \tPARSE_OPT_KEEP_UNKNOWN = 1 << 3,\n \tPARSE_OPT_NO_INTERNAL_HELP = 1 << 4,\n \tPARSE_OPT_ONE_SHOT = 1 << 5,\n+\tPARSE_OPT_SHELL_EVAL = 1 << 6,\n };\n \n enum parse_opt_option_flags {\n@@ -44,7 +45,6 @@ enum parse_opt_option_flags {\n \tPARSE_OPT_NODASH = 1 << 5,\n \tPARSE_OPT_LITERAL_ARGHELP = 1 << 6,\n \tPARSE_OPT_FROM_ALIAS = 1 << 7,\n-\tPARSE_OPT_SHELL_EVAL = 1 << 8,\n \tPARSE_OPT_NOCOMPLETE = 1 << 9,\n \tPARSE_OPT_COMP_ARG = 1 << 10,\n \tPARSE_OPT_CMDMODE = 1 << 11,\n-- \n2.33.0.1374.gc8f4fa74caf\n\n"},{"id":"437690","messageId":"patch-v2-02.11-99f5251c557-20211001T142631Z-avarab@gmail.com","threadId":"56604","inReplyTo":"cover-v2-00.11-00000000000-20211001T142631Z-avarab@gmail.com","subject":"[PATCH v2 02/11] parse-options.[ch]: consistently use \"enum parse_opt_flags\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-10-01T14:29:08Z","receivedAt":"2021-10-01T14:29:27Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Use the \"enum parse_opt_flags\" instead of an \"int flags\" as arguments\nto the various functions in parse-options.c.\n\nIn C enums aren't first-class types, and the \"enum\nparse_opt_option_flag\" uses a enum-as-bitfield pattern. So unlike\nexhaustively enumerated \"case\" arms we're not going to get validation\nthat we used the \"right\" enum labels.\n\nI.e. this won't catch the sort of bug that was fixed with\n\"PARSE_OPT_SHELL_EVAL\" in the preceding commit.\n\nBut there's still a benefit to doing this when it comes to the wider C\necosystem. E.g. the GNU debugger (gdb) will helpfully detect and print\nout meaningful enum labels in this case. Here's the output before and\nafter when breaking in \"parse_options()\" after invoking \"git stash\nshow\":\n\nBefore:\n\n (gdb) p flags\n $1 = 9\n\nAfter:\n\n (gdb) p flags\n $1 = (PARSE_OPT_KEEP_DASHDASH | PARSE_OPT_KEEP_UNKNOWN)\n\nOf course as noted in[1] there's a limit to this smartness,\ni.e. manually setting it with unrelated enum labels won't be\ncaught. There are some third-party extensions to do more exhaustive\nchecking[2], perhaps we'll be able to make use of them sooner than\nlater.\n\nWe've also got prior art using this pattern in the codebase. See\ne.g. \"enum bloom_filter_computed\" added in 312cff52074 (bloom: split\n'get_bloom_filter()' in two, 2020-09-16) and the \"permitted\" enum\nadded in ce910287e72 (add -p: fix checking of user input, 2020-08-17).\n\n1. https://lore.kernel.org/git/87mtnvvj3c.fsf@evledraar.gmail.com/\n2. https://github.com/sinelaw/elfs-clang-plugins/blob/master/enums_conversion/README.md\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n parse-options.c | 11 +++++++----\n parse-options.h | 6 ++++--\n 2 files changed, 11 insertions(+), 6 deletions(-)\n\ndiff --git a/parse-options.c b/parse-options.c\nindex 55c5821b08d..9c8ba963400 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -481,7 +481,8 @@ static void parse_options_check(const struct option *opts)\n \n static void parse_options_start_1(struct parse_opt_ctx_t *ctx,\n \t\t\t\t int argc, const char **argv, const char *prefix,\n-\t\t\t\t const struct option *options, int flags)\n+\t\t\t\t const struct option *options,\n+\t\t\t\t enum parse_opt_flags flags)\n {\n \tctx->argc = argc;\n \tctx->argv = argv;\n@@ -506,7 +507,8 @@ static void parse_options_start_1(struct parse_opt_ctx_t *ctx,\n \n void parse_options_start(struct parse_opt_ctx_t *ctx,\n \t\t\t int argc, const char **argv, const char *prefix,\n-\t\t\t const struct option *options, int flags)\n+\t\t\t const struct option *options,\n+\t\t\t enum parse_opt_flags flags)\n {\n \tmemset(ctx, 0, sizeof(*ctx));\n \tparse_options_start_1(ctx, argc, argv, prefix, options, flags);\n@@ -838,8 +840,9 @@ int parse_options_end(struct parse_opt_ctx_t *ctx)\n }\n \n int parse_options(int argc, const char **argv, const char *prefix,\n-\t\t const struct option *options, const char * const usagestr[],\n-\t\t int flags)\n+\t\t const struct option *options,\n+\t\t const char * const usagestr[],\n+\t\t enum parse_opt_flags flags)\n {\n \tstruct parse_opt_ctx_t ctx;\n \tstruct option *real_options;\ndiff --git a/parse-options.h b/parse-options.h\nindex 3a3176ae65c..fb5aafd4f7b 100644\n--- a/parse-options.h\n+++ b/parse-options.h\n@@ -213,7 +213,8 @@ struct option {\n */\n int parse_options(int argc, const char **argv, const char *prefix,\n \t\t const struct option *options,\n-\t\t const char * const usagestr[], int flags);\n+\t\t const char * const usagestr[],\n+\t\t enum parse_opt_flags flags);\n \n NORETURN void usage_with_options(const char * const *usagestr,\n \t\t\t\t const struct option *options);\n@@ -270,7 +271,8 @@ struct parse_opt_ctx_t {\n \n void parse_options_start(struct parse_opt_ctx_t *ctx,\n \t\t\t int argc, const char **argv, const char *prefix,\n-\t\t\t const struct option *options, int flags);\n+\t\t\t const struct option *options,\n+\t\t\t enum parse_opt_flags flags);\n \n int parse_options_step(struct parse_opt_ctx_t *ctx,\n \t\t const struct option *options,\n-- \n2.33.0.1374.gc8f4fa74caf\n\n"},{"id":"437691","messageId":"patch-v2-03.11-be198e42df9-20211001T142631Z-avarab@gmail.com","threadId":"56604","inReplyTo":"cover-v2-00.11-00000000000-20211001T142631Z-avarab@gmail.com","subject":"[PATCH v2 03/11] parse-options.[ch]: consistently use \"enum parse_opt_result\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-10-01T14:29:09Z","receivedAt":"2021-10-01T14:29:29Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Use the \"enum parse_opt_result\" instead of an \"int flags\" as the\nreturn value of the applicable functions in parse-options.c.\n\nThis will help catch future bugs, such as the missing \"case\" arms in\nthe two existing users of the API in \"blame.c\" and \"shortlog.c\". A\nthird caller in 309be813c9b (update-index: migrate to parse-options\nAPI, 2010-12-01) was already checking for these.\n\nAs can be seen when trying to sort through the deluge of warnings\nproduced when compiling this with CC=g++ (mostly unrelated to this\nchange) we're not consistently using \"enum parse_opt_result\" even now,\ni.e. we'll return error() and \"return 0;\". See f41179f16ba\n(parse-options: avoid magic return codes, 2019-01-27) for a commit\nwhich started changing some of that.\n\nI'm not doing any more of that exhaustive migration here, and it's\nprobably not worthwhile past the point of being able to check \"enum\nparse_opt_result\" in switch().\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n builtin/blame.c | 3 +++\n builtin/shortlog.c | 3 +++\n parse-options.c | 31 +++++++++++++++++--------------\n parse-options.h | 15 ++++++++-------\n 4 files changed, 31 insertions(+), 21 deletions(-)\n\ndiff --git a/builtin/blame.c b/builtin/blame.c\nindex 641523ff9af..9273fb222d5 100644\n--- a/builtin/blame.c\n+++ b/builtin/blame.c\n@@ -917,6 +917,9 @@ int cmd_blame(int argc, const char **argv, const char *prefix)\n \t\t\t PARSE_OPT_KEEP_DASHDASH | PARSE_OPT_KEEP_ARGV0);\n \tfor (;;) {\n \t\tswitch (parse_options_step(&ctx, options, blame_opt_usage)) {\n+\t\tcase PARSE_OPT_NON_OPTION:\n+\t\tcase PARSE_OPT_UNKNOWN:\n+\t\t\tbreak;\n \t\tcase PARSE_OPT_HELP:\n \t\tcase PARSE_OPT_ERROR:\n \t\t\texit(129);\ndiff --git a/builtin/shortlog.c b/builtin/shortlog.c\nindex 3e7ab1ca821..e7f7af5de3f 100644\n--- a/builtin/shortlog.c\n+++ b/builtin/shortlog.c\n@@ -374,6 +374,9 @@ int cmd_shortlog(int argc, const char **argv, const char *prefix)\n \n \tfor (;;) {\n \t\tswitch (parse_options_step(&ctx, options, shortlog_usage)) {\n+\t\tcase PARSE_OPT_NON_OPTION:\n+\t\tcase PARSE_OPT_UNKNOWN:\n+\t\t\tbreak;\n \t\tcase PARSE_OPT_HELP:\n \t\tcase PARSE_OPT_ERROR:\n \t\t\texit(129);\ndiff --git a/parse-options.c b/parse-options.c\nindex 9c8ba963400..f718242096c 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -699,13 +699,14 @@ static void free_preprocessed_options(struct option *options)\n \tfree(options);\n }\n \n-static int usage_with_options_internal(struct parse_opt_ctx_t *,\n-\t\t\t\t const char * const *,\n-\t\t\t\t const struct option *, int, int);\n-\n-int parse_options_step(struct parse_opt_ctx_t *ctx,\n-\t\t const struct option *options,\n-\t\t const char * const usagestr[])\n+static enum parse_opt_result usage_with_options_internal(struct parse_opt_ctx_t *,\n+\t\t\t\t\t\t\t const char * const *,\n+\t\t\t\t\t\t\t const struct option *,\n+\t\t\t\t\t\t\t int, int);\n+\n+enum parse_opt_result parse_options_step(struct parse_opt_ctx_t *ctx,\n+\t\t\t\t\t const struct option *options,\n+\t\t\t\t\t const char * const usagestr[])\n {\n \tint internal_help = !(ctx->flags & PARSE_OPT_NO_INTERNAL_HELP);\n \n@@ -839,10 +840,11 @@ int parse_options_end(struct parse_opt_ctx_t *ctx)\n \treturn ctx->cpidx + ctx->argc;\n }\n \n-int parse_options(int argc, const char **argv, const char *prefix,\n-\t\t const struct option *options,\n-\t\t const char * const usagestr[],\n-\t\t enum parse_opt_flags flags)\n+enum parse_opt_result parse_options(int argc, const char **argv,\n+\t\t\t\t const char *prefix,\n+\t\t\t\t const struct option *options,\n+\t\t\t\t const char * const usagestr[],\n+\t\t\t\t enum parse_opt_flags flags)\n {\n \tstruct parse_opt_ctx_t ctx;\n \tstruct option *real_options;\n@@ -900,9 +902,10 @@ static int usage_argh(const struct option *opts, FILE *outfile)\n #define USAGE_OPTS_WIDTH 24\n #define USAGE_GAP 2\n \n-static int usage_with_options_internal(struct parse_opt_ctx_t *ctx,\n-\t\t\t\t const char * const *usagestr,\n-\t\t\t\t const struct option *opts, int full, int err)\n+static enum parse_opt_result usage_with_options_internal(struct parse_opt_ctx_t *ctx,\n+\t\t\t\t\t\t\t const char * const *usagestr,\n+\t\t\t\t\t\t\t const struct option *opts,\n+\t\t\t\t\t\t\t int full, int err)\n {\n \tFILE *outfile = err ? stderr : stdout;\n \tint need_newline;\ndiff --git a/parse-options.h b/parse-options.h\nindex fb5aafd4f7b..d931300f4d6 100644\n--- a/parse-options.h\n+++ b/parse-options.h\n@@ -211,10 +211,11 @@ struct option {\n * untouched and parse_options() returns the number of options\n * processed.\n */\n-int parse_options(int argc, const char **argv, const char *prefix,\n-\t\t const struct option *options,\n-\t\t const char * const usagestr[],\n-\t\t enum parse_opt_flags flags);\n+enum parse_opt_result parse_options(int argc, const char **argv,\n+\t\t\t\t const char *prefix,\n+\t\t\t\t const struct option *options,\n+\t\t\t\t const char * const usagestr[],\n+\t\t\t\t enum parse_opt_flags flags);\n \n NORETURN void usage_with_options(const char * const *usagestr,\n \t\t\t\t const struct option *options);\n@@ -274,9 +275,9 @@ void parse_options_start(struct parse_opt_ctx_t *ctx,\n \t\t\t const struct option *options,\n \t\t\t enum parse_opt_flags flags);\n \n-int parse_options_step(struct parse_opt_ctx_t *ctx,\n-\t\t const struct option *options,\n-\t\t const char * const usagestr[]);\n+enum parse_opt_result parse_options_step(struct parse_opt_ctx_t *ctx,\n+\t\t\t\t\t const struct option *options,\n+\t\t\t\t\t const char * const usagestr[]);\n \n int parse_options_end(struct parse_opt_ctx_t *ctx);\n \n-- \n2.33.0.1374.gc8f4fa74caf\n\n"},{"id":"437692","messageId":"patch-v2-04.11-a253db7a60d-20211001T142631Z-avarab@gmail.com","threadId":"56604","inReplyTo":"cover-v2-00.11-00000000000-20211001T142631Z-avarab@gmail.com","subject":"[PATCH v2 04/11] parse-options.c: use exhaustive \"case\" arms for \"enum parse_opt_result\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-10-01T14:29:10Z","receivedAt":"2021-10-01T14:29:30Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Change the \"default\" case in parse_options() that handles the return\nvalue of parse_options_step() to simply have a \"case\" arm for\nPARSE_OPT_UNKNOWN, instead of leaving it to a comment. This means the\ncompiler can warn us about any missing case arms.\n\nThis adjusts code added in ff43ec3e2d2 (parse-opt: create\nparse_options_step., 2008-06-23), given its age it may pre-date the\nexistence (or widespread use) of this coding style, which we've since\nadopted more widely.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n parse-options.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/parse-options.c b/parse-options.c\nindex f718242096c..e33700d6e71 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -866,7 +866,7 @@ enum parse_opt_result parse_options(int argc, const char **argv,\n \tcase PARSE_OPT_NON_OPTION:\n \tcase PARSE_OPT_DONE:\n \t\tbreak;\n-\tdefault: /* PARSE_OPT_UNKNOWN */\n+\tcase PARSE_OPT_UNKNOWN:\n \t\tif (ctx.argv[0][1] == '-') {\n \t\t\terror(_(\"unknown option `%s'\"), ctx.argv[0] + 2);\n \t\t} else if (isascii(*ctx.opt)) {\n-- \n2.33.0.1374.gc8f4fa74caf\n\n"},{"id":"437693","messageId":"patch-v2-05.11-467828780d0-20211001T142631Z-avarab@gmail.com","threadId":"56604","inReplyTo":"cover-v2-00.11-00000000000-20211001T142631Z-avarab@gmail.com","subject":"[PATCH v2 05/11] parse-options.c: use exhaustive \"case\" arms for \"enum parse_opt_type\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-10-01T14:29:11Z","receivedAt":"2021-10-01T14:29:31Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Change code in get_value(), parse_options_check() etc. to do away with\nthe \"default\" case in favor of exhaustively checking the relevant\nfields.\n\nThe added \"return -1\" is needed for the GCC version commented on\ninline, my local clang 11.0.1-2 does not require it. Let's add it for\nnow to appease GCC.\n\nThe added \"special types\" etc. comments correspond to the relevant\ncomments and ordering on the \"enum parse_opt_type\". Let's try to keep\nthe same order and commentary as there where possible for\nclarity. This doesn't reach that end-state, and due to the different\nhandling of options it's probably not worth it to get there, but let's\nmatch its ordering where it's easy to do so.\n\nThere was a discussion about whether this was worth the added\nverbosity, as argued in[1] I think it's worth it for getting\ncompile-time checking when adding new option types. We *should* have\ntests for some of these, but e.g. in the show_gitcomp() case one might\nrun through the whole test suite and only hit a missing case at the\nend on the completion tests.\n\nThis technically changes the handling of OPTION_END, but it's\nobviously the right thing to do. We're calling this code from within a\nloop that uses OPTION_END as a break condition, so it was never caught\nby the \"default\" case.\n\nSo let's make encountering OPTION_END a BUG(), just like it already is\nin the get_value() handling added in 4a59fd13122 (Add a simple option\nparser., 2007-10-15).\n\n1. https://lore.kernel.org/git/87tui3vk8y.fsf@evledraar.gmail.com/\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n parse-options.c | 48 +++++++++++++++++++++++++++++++++++++++++++-----\n parse-options.h | 2 +-\n 2 files changed, 44 insertions(+), 6 deletions(-)\n\ndiff --git a/parse-options.c b/parse-options.c\nindex e33700d6e71..dedd40efec5 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -219,9 +219,14 @@ static enum parse_opt_result get_value(struct parse_opt_ctx_t *p,\n \t\t\t\t optname(opt, flags));\n \t\treturn 0;\n \n-\tdefault:\n+\t/* special types */\n+\tcase OPTION_END:\n+\tcase OPTION_GROUP:\n+\tcase OPTION_NUMBER:\n+\tcase OPTION_ALIAS:\n \t\tBUG(\"opt->type %d should not happen\", opt->type);\n \t}\n+\treturn -1; /* gcc 10.2.1-6's -Werror=return-type */\n }\n \n static enum parse_opt_result parse_short_opt(struct parse_opt_ctx_t *p,\n@@ -443,6 +448,9 @@ static void parse_options_check(const struct option *opts)\n \t\t\terr |= optbug(opts, \"uses feature \"\n \t\t\t\t\t\"not supported for dashless options\");\n \t\tswitch (opts->type) {\n+\t\tcase OPTION_END:\n+\t\t\tBUG(\"unreachable\");\n+\n \t\tcase OPTION_COUNTUP:\n \t\tcase OPTION_BIT:\n \t\tcase OPTION_NEGBIT:\n@@ -468,8 +476,14 @@ static void parse_options_check(const struct option *opts)\n \t\t\tBUG(\"OPT_ALIAS() should not remain at this point. \"\n \t\t\t \"Are you using parse_options_step() directly?\\n\"\n \t\t\t \"That case is not supported yet.\");\n-\t\tdefault:\n-\t\t\t; /* ok. (usually accepts an argument) */\n+\n+\t\tcase OPTION_BITOP:\n+\t\tcase OPTION_FILENAME:\n+\t\tcase OPTION_GROUP:\n+\t\tcase OPTION_INTEGER:\n+\t\tcase OPTION_MAGNITUDE:\n+\t\tcase OPTION_STRING:\n+\t\t\tbreak;\n \t\t}\n \t\tif (opts->argh &&\n \t\t strcspn(opts->argh, \" _\") != strlen(opts->argh))\n@@ -532,6 +546,9 @@ static void show_negated_gitcomp(const struct option *opts, int show_all,\n \t\t\tcontinue;\n \n \t\tswitch (opts->type) {\n+\t\tcase OPTION_END:\n+\t\t\tBUG(\"unreachable\");\n+\n \t\tcase OPTION_STRING:\n \t\tcase OPTION_FILENAME:\n \t\tcase OPTION_INTEGER:\n@@ -543,7 +560,14 @@ static void show_negated_gitcomp(const struct option *opts, int show_all,\n \t\tcase OPTION_SET_INT:\n \t\t\thas_unset_form = 1;\n \t\t\tbreak;\n-\t\tdefault:\n+\t\t/* special types */\n+\t\tcase OPTION_GROUP:\n+\t\tcase OPTION_NUMBER:\n+\t\tcase OPTION_ALIAS:\n+\t\t/* options with no arguments */\n+\t\tcase OPTION_BITOP:\n+\t\t/* options with arguments (usually) */\n+\t\tcase OPTION_LOWLEVEL_CALLBACK:\n \t\t\tbreak;\n \t\t}\n \t\tif (!has_unset_form)\n@@ -578,6 +602,8 @@ static int show_gitcomp(const struct option *opts, int show_all)\n \t\t\tcontinue;\n \n \t\tswitch (opts->type) {\n+\t\tcase OPTION_END:\n+\t\t\tBUG(\"unreachable\");\n \t\tcase OPTION_GROUP:\n \t\t\tcontinue;\n \t\tcase OPTION_STRING:\n@@ -593,7 +619,19 @@ static int show_gitcomp(const struct option *opts, int show_all)\n \t\t\t\tbreak;\n \t\t\tsuffix = \"=\";\n \t\t\tbreak;\n-\t\tdefault:\n+\t\t/* special types */\n+\t\tcase OPTION_NUMBER:\n+\t\tcase OPTION_ALIAS:\n+\n+\t\t/* options with no arguments */\n+\t\tcase OPTION_BIT:\n+\t\tcase OPTION_NEGBIT:\n+\t\tcase OPTION_BITOP:\n+\t\tcase OPTION_COUNTUP:\n+\t\tcase OPTION_SET_INT:\n+\n+\t\t/* options with arguments (usually) */\n+\t\tcase OPTION_LOWLEVEL_CALLBACK:\n \t\t\tbreak;\n \t\t}\n \t\tif (opts->flags & PARSE_OPT_COMP_ARG)\ndiff --git a/parse-options.h b/parse-options.h\nindex d931300f4d6..a1c7c86ad30 100644\n--- a/parse-options.h\n+++ b/parse-options.h\n@@ -264,7 +264,7 @@ struct parse_opt_ctx_t {\n \tconst char **out;\n \tint argc, cpidx, total;\n \tconst char *opt;\n-\tint flags;\n+\tenum parse_opt_flags flags;\n \tconst char *prefix;\n \tconst char **alias_groups; /* must be in groups of 3 elements! */\n \tstruct option *updated_options;\n-- \n2.33.0.1374.gc8f4fa74caf\n\n"},{"id":"437694","messageId":"patch-v2-06.11-34669327550-20211001T142631Z-avarab@gmail.com","threadId":"56604","inReplyTo":"cover-v2-00.11-00000000000-20211001T142631Z-avarab@gmail.com","subject":"[PATCH v2 06/11] parse-options.h: make the \"flags\" in \"struct option\" an enum","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-10-01T14:29:12Z","receivedAt":"2021-10-01T14:29:32Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Change the \"flags\" members of \"struct option\" to refer to their\ncorresponding \"enum\" defined earlier in the file.\n\nThe benefit of changing this to an enum isn't as great as with some\n\"enum parse_opt_type\" as we'll always check this as a bitfield, so we\ncan't rely on the compiler checking \"case\" arms for us. But let's do\nit for consistency with the rest of the file.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n parse-options.h | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/parse-options.h b/parse-options.h\nindex a1c7c86ad30..74b66ba6e93 100644\n--- a/parse-options.h\n+++ b/parse-options.h\n@@ -134,7 +134,7 @@ struct option {\n \tconst char *argh;\n \tconst char *help;\n \n-\tint flags;\n+\tenum parse_opt_option_flags flags;\n \tparse_opt_cb *callback;\n \tintptr_t defval;\n \tparse_opt_ll_cb *ll_callback;\n-- \n2.33.0.1374.gc8f4fa74caf\n\n"},{"id":"437695","messageId":"patch-v2-07.11-e82a4e477d5-20211001T142631Z-avarab@gmail.com","threadId":"56604","inReplyTo":"cover-v2-00.11-00000000000-20211001T142631Z-avarab@gmail.com","subject":"[PATCH v2 07/11] parse-options.c: move optname() earlier in the file","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-10-01T14:29:13Z","receivedAt":"2021-10-01T14:29:34Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"In preparation for making \"optname\" a static function move it above\nits first user in parse-options.c.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n parse-options.c | 30 +++++++++++++++---------------\n 1 file changed, 15 insertions(+), 15 deletions(-)\n\ndiff --git a/parse-options.c b/parse-options.c\nindex dedd40efec5..2cf6f4d01c1 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -22,6 +22,21 @@ int optbug(const struct option *opt, const char *reason)\n \treturn error(\"BUG: switch '%c' %s\", opt->short_name, reason);\n }\n \n+const char *optname(const struct option *opt, int flags)\n+{\n+\tstatic struct strbuf sb = STRBUF_INIT;\n+\n+\tstrbuf_reset(&sb);\n+\tif (flags & OPT_SHORT)\n+\t\tstrbuf_addf(&sb, \"switch `%c'\", opt->short_name);\n+\telse if (flags & OPT_UNSET)\n+\t\tstrbuf_addf(&sb, \"option `no-%s'\", opt->long_name);\n+\telse\n+\t\tstrbuf_addf(&sb, \"option `%s'\", opt->long_name);\n+\n+\treturn sb.buf;\n+}\n+\n static enum parse_opt_result get_arg(struct parse_opt_ctx_t *p,\n \t\t\t\t const struct option *opt,\n \t\t\t\t int flags, const char **arg)\n@@ -1044,18 +1059,3 @@ void NORETURN usage_msg_opt(const char *msg,\n \tfprintf(stderr, \"fatal: %s\\n\\n\", msg);\n \tusage_with_options(usagestr, options);\n }\n-\n-const char *optname(const struct option *opt, int flags)\n-{\n-\tstatic struct strbuf sb = STRBUF_INIT;\n-\n-\tstrbuf_reset(&sb);\n-\tif (flags & OPT_SHORT)\n-\t\tstrbuf_addf(&sb, \"switch `%c'\", opt->short_name);\n-\telse if (flags & OPT_UNSET)\n-\t\tstrbuf_addf(&sb, \"option `no-%s'\", opt->long_name);\n-\telse\n-\t\tstrbuf_addf(&sb, \"option `%s'\", opt->long_name);\n-\n-\treturn sb.buf;\n-}\n-- \n2.33.0.1374.gc8f4fa74caf\n\n"},{"id":"437696","messageId":"patch-v2-08.11-58683b3d89d-20211001T142631Z-avarab@gmail.com","threadId":"56604","inReplyTo":"cover-v2-00.11-00000000000-20211001T142631Z-avarab@gmail.com","subject":"[PATCH v2 08/11] commit-graph: stop using optname()","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-10-01T14:29:14Z","receivedAt":"2021-10-01T14:29:35Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Stop using optname() in builtin/commit-graph.c to emit an error with\nthe --max-new-filters option. This changes code added in 809e0327f57\n(builtin/commit-graph.c: introduce '--max-new-filters=',\n2020-09-18).\n\nSee 9440b831ad5 (parse-options: replace opterror() with optname(),\n2018-11-10) for why using optname() like this is considered bad,\ni.e. it's assembling human-readable output piecemeal, and the \"option\n`X'\" at the start can't be translated.\n\nIt didn't matter in this case, but this code was also buggy in its use\nof \"opt->flags\" to optname(), that function expects flags, but not\n*those* flags.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n builtin/commit-graph.c | 3 +--\n 1 file changed, 1 insertion(+), 2 deletions(-)\n\ndiff --git a/builtin/commit-graph.c b/builtin/commit-graph.c\nindex 0386f5c7755..36552db89fe 100644\n--- a/builtin/commit-graph.c\n+++ b/builtin/commit-graph.c\n@@ -172,8 +172,7 @@ static int write_option_max_new_filters(const struct option *opt,\n \t\tconst char *s;\n \t\t*to = strtol(arg, (char **)&s, 10);\n \t\tif (*s)\n-\t\t\treturn error(_(\"%s expects a numerical value\"),\n-\t\t\t\t optname(opt, opt->flags));\n+\t\t\treturn error(_(\"option `max-new-filters' expects a numerical value\"));\n \t}\n \treturn 0;\n }\n-- \n2.33.0.1374.gc8f4fa74caf\n\n"},{"id":"437697","messageId":"patch-v2-09.11-8cbee660174-20211001T142631Z-avarab@gmail.com","threadId":"56604","inReplyTo":"cover-v2-00.11-00000000000-20211001T142631Z-avarab@gmail.com","subject":"[PATCH v2 09/11] parse-options.[ch]: make opt{bug,name}() \"static\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-10-01T14:29:15Z","receivedAt":"2021-10-01T14:29:35Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Change these two functions to \"static\", the last user of \"optname()\"\noutside of parse-options.c itself went away in the preceding commit,\nfor the reasons noted in 9440b831ad5 (parse-options: replace\nopterror() with optname(), 2018-11-10) we shouldn't be adding any more\nusers of it.\n\nThe \"optbug()\" function was never used outside of parse-options.c, but\nwas made non-static in 1f275b7c4ca (parse-options: export opterr,\noptbug, 2011-08-11). I think the only external user of optname() was\nthe commit-graph.c caller added in 09e0327f57 (builtin/commit-graph.c:\nintroduce '--max-new-filters=', 2020-09-18).\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n parse-options.c | 4 ++--\n parse-options.h | 3 ---\n 2 files changed, 2 insertions(+), 5 deletions(-)\n\ndiff --git a/parse-options.c b/parse-options.c\nindex 2cf6f4d01c1..0239c6bd418 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -11,7 +11,7 @@ static int disallow_abbreviated_options;\n #define OPT_SHORT 1\n #define OPT_UNSET 2\n \n-int optbug(const struct option *opt, const char *reason)\n+static int optbug(const struct option *opt, const char *reason)\n {\n \tif (opt->long_name) {\n \t\tif (opt->short_name)\n@@ -22,7 +22,7 @@ int optbug(const struct option *opt, const char *reason)\n \treturn error(\"BUG: switch '%c' %s\", opt->short_name, reason);\n }\n \n-const char *optname(const struct option *opt, int flags)\n+static const char *optname(const struct option *opt, int flags)\n {\n \tstatic struct strbuf sb = STRBUF_INIT;\n \ndiff --git a/parse-options.h b/parse-options.h\nindex 74b66ba6e93..dd79c9c566f 100644\n--- a/parse-options.h\n+++ b/parse-options.h\n@@ -224,9 +224,6 @@ NORETURN void usage_msg_opt(const char *msg,\n \t\t\t const char * const *usagestr,\n \t\t\t const struct option *options);\n \n-int optbug(const struct option *opt, const char *reason);\n-const char *optname(const struct option *opt, int flags);\n-\n /*\n * Use these assertions for callbacks that expect to be called with NONEG and\n * NOARG respectively, and do not otherwise handle the \"unset\" and \"arg\"\n-- \n2.33.0.1374.gc8f4fa74caf\n\n"},{"id":"437698","messageId":"patch-v2-10.11-f727f683eb1-20211001T142631Z-avarab@gmail.com","threadId":"56604","inReplyTo":"cover-v2-00.11-00000000000-20211001T142631Z-avarab@gmail.com","subject":"[PATCH v2 10/11] parse-options tests: test optname() output","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-10-01T14:29:16Z","receivedAt":"2021-10-01T14:29:47Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"There were no tests for checking the specific output that we'll\ngenerate in optname(), let's add some. That output was added back in\n4a59fd13122 (Add a simple option parser., 2007-10-15).\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n t/t0040-parse-options.sh | 42 +++++++++++++++++++++++++++++++++++++---\n 1 file changed, 39 insertions(+), 3 deletions(-)\n\ndiff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh\nindex da310ed29b1..d6f391a497b 100755\n--- a/t/t0040-parse-options.sh\n+++ b/t/t0040-parse-options.sh\n@@ -168,9 +168,45 @@ test_expect_success 'long options' '\n '\n \n test_expect_success 'missing required value' '\n-\ttest_expect_code 129 test-tool parse-options -s &&\n-\ttest_expect_code 129 test-tool parse-options --string &&\n-\ttest_expect_code 129 test-tool parse-options --file\n+\tcat >expect <<-\\EOF &&\n+\terror: switch `s'\\'' requires a value\n+\tEOF\n+\ttest_expect_code 129 test-tool parse-options -s 2>actual &&\n+\ttest_cmp expect actual &&\n+\n+\tcat >expect <<-\\EOF &&\n+\terror: option `string'\\'' requires a value\n+\tEOF\n+\ttest_expect_code 129 test-tool parse-options --string 2>actual &&\n+\ttest_cmp expect actual &&\n+\n+\tcat >expect <<-\\EOF &&\n+\terror: option `file'\\'' requires a value\n+\tEOF\n+\ttest_expect_code 129 test-tool parse-options --file 2>actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'superfluous value provided: boolean' '\n+\tcat >expect <<-\\EOF &&\n+\terror: option `yes'\\'' takes no value\n+\tEOF\n+\ttest_expect_code 129 test-tool parse-options --yes=hi 2>actual &&\n+\ttest_cmp expect actual &&\n+\n+\tcat >expect <<-\\EOF &&\n+\terror: option `no-yes'\\'' takes no value\n+\tEOF\n+\ttest_expect_code 129 test-tool parse-options --no-yes=hi 2>actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'superfluous value provided: cmdmode' '\n+\tcat >expect <<-\\EOF &&\n+\terror: option `mode1'\\'' takes no value\n+\tEOF\n+\ttest_expect_code 129 test-tool parse-options --mode1=hi 2>actual &&\n+\ttest_cmp expect actual\n '\n \n cat >expect <<\\EOF\n-- \n2.33.0.1374.gc8f4fa74caf\n\n"},{"id":"437699","messageId":"patch-v2-11.11-4fbc07fc7fd-20211001T142631Z-avarab@gmail.com","threadId":"56604","inReplyTo":"cover-v2-00.11-00000000000-20211001T142631Z-avarab@gmail.com","subject":"[PATCH v2 11/11] parse-options: change OPT_{SHORT,UNSET} to an enum","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-10-01T14:29:17Z","receivedAt":"2021-10-01T14:29:52Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Change the comparisons against OPT_SHORT and OPT_UNSET to an enum\nwhich keeps track of how a given option got parsed. The case of \"0\"\nwas an implicit OPT_LONG, so let's add an explicit label for it.\n\nDue to the xor in 0f1930c5875 (parse-options: allow positivation of\noptions starting, with no-, 2012-02-25) the code already relied on\nthis being set back to 0. To avoid refactoring the logic involved in\nthat let's just start the enum at \"0\" instead of the usual \"1<<0\" (1),\nbut BUG() out if we don't have one of our expected flags.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n parse-options.c | 23 ++++++++++++++---------\n 1 file changed, 14 insertions(+), 9 deletions(-)\n\ndiff --git a/parse-options.c b/parse-options.c\nindex 0239c6bd418..61c294b7895 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -8,8 +8,11 @@\n \n static int disallow_abbreviated_options;\n \n-#define OPT_SHORT 1\n-#define OPT_UNSET 2\n+enum opt_parsed {\n+\tOPT_LONG = 0,\n+\tOPT_SHORT = 1<<0,\n+\tOPT_UNSET = 1<<1,\n+};\n \n static int optbug(const struct option *opt, const char *reason)\n {\n@@ -22,7 +25,7 @@ static int optbug(const struct option *opt, const char *reason)\n \treturn error(\"BUG: switch '%c' %s\", opt->short_name, reason);\n }\n \n-static const char *optname(const struct option *opt, int flags)\n+static const char *optname(const struct option *opt, enum opt_parsed flags)\n {\n \tstatic struct strbuf sb = STRBUF_INIT;\n \n@@ -31,15 +34,17 @@ static const char *optname(const struct option *opt, int flags)\n \t\tstrbuf_addf(&sb, \"switch `%c'\", opt->short_name);\n \telse if (flags & OPT_UNSET)\n \t\tstrbuf_addf(&sb, \"option `no-%s'\", opt->long_name);\n-\telse\n+\telse if (flags == OPT_LONG)\n \t\tstrbuf_addf(&sb, \"option `%s'\", opt->long_name);\n+\telse\n+\t\tBUG(\"optname() got unknown flags %d\", flags);\n \n \treturn sb.buf;\n }\n \n static enum parse_opt_result get_arg(struct parse_opt_ctx_t *p,\n \t\t\t\t const struct option *opt,\n-\t\t\t\t int flags, const char **arg)\n+\t\t\t\t enum opt_parsed flags, const char **arg)\n {\n \tif (p->opt) {\n \t\t*arg = p->opt;\n@@ -65,7 +70,7 @@ static void fix_filename(const char *prefix, const char **file)\n static enum parse_opt_result opt_command_mode_error(\n \tconst struct option *opt,\n \tconst struct option *all_opts,\n-\tint flags)\n+\tenum opt_parsed flags)\n {\n \tconst struct option *that;\n \tstruct strbuf that_name = STRBUF_INIT;\n@@ -97,7 +102,7 @@ static enum parse_opt_result opt_command_mode_error(\n static enum parse_opt_result get_value(struct parse_opt_ctx_t *p,\n \t\t\t\t const struct option *opt,\n \t\t\t\t const struct option *all_opts,\n-\t\t\t\t int flags)\n+\t\t\t\t enum opt_parsed flags)\n {\n \tconst char *s, *arg;\n \tconst int unset = flags & OPT_UNSET;\n@@ -318,11 +323,11 @@ static enum parse_opt_result parse_long_opt(\n \tconst struct option *all_opts = options;\n \tconst char *arg_end = strchrnul(arg, '=');\n \tconst struct option *abbrev_option = NULL, *ambiguous_option = NULL;\n-\tint abbrev_flags = 0, ambiguous_flags = 0;\n+\tenum opt_parsed abbrev_flags = OPT_LONG, ambiguous_flags = OPT_LONG;\n \n \tfor (; options->type != OPTION_END; options++) {\n \t\tconst char *rest, *long_name = options->long_name;\n-\t\tint flags = 0, opt_flags = 0;\n+\t\tenum opt_parsed flags = OPT_LONG, opt_flags = OPT_LONG;\n \n \t\tif (!long_name)\n \t\t\tcontinue;\n-- \n2.33.0.1374.gc8f4fa74caf\n\n"},{"id":"437713","messageId":"fffcb0ac-5952-c8c5-2c8f-c546ef5e3b1f@web.de","threadId":"56604","inReplyTo":"patch-v2-08.11-58683b3d89d-20211001T142631Z-avarab@gmail.com","subject":"Re: [PATCH v2 08/11] commit-graph: stop using optname()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2021-10-01T17:12:08Z","receivedAt":"2021-10-01T17:12:26Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 01.10.21 um 16:29 schrieb Ævar Arnfjörð Bjarmason:\n> Stop using optname() in builtin/commit-graph.c to emit an error with\n> the --max-new-filters option. This changes code added in 809e0327f57\n> (builtin/commit-graph.c: introduce '--max-new-filters=',\n> 2020-09-18).\n>\n> See 9440b831ad5 (parse-options: replace opterror() with optname(),\n> 2018-11-10) for why using optname() like this is considered bad,\n> i.e. it's assembling human-readable output piecemeal, and the \"option\n> `X'\" at the start can't be translated.\n>\n> It didn't matter in this case, but this code was also buggy in its use\n> of \"opt->flags\" to optname(), that function expects flags, but not\n> *those* flags.\n>\n> Signed-off-by: Ævar Arnfjörð Bjarmason \n> ---\n> builtin/commit-graph.c | 3 +--\n> 1 file changed, 1 insertion(+), 2 deletions(-)\n>\n> diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c\n> index 0386f5c7755..36552db89fe 100644\n> --- a/builtin/commit-graph.c\n> +++ b/builtin/commit-graph.c\n> @@ -172,8 +172,7 @@ static int write_option_max_new_filters(const struct option *opt,\n> \t\tconst char *s;\n> \t\t*to = strtol(arg, (char **)&s, 10);\n> \t\tif (*s)\n> -\t\t\treturn error(_(\"%s expects a numerical value\"),\n> -\t\t\t\t optname(opt, opt->flags));\n> +\t\t\treturn error(_(\"option `max-new-filters' expects a numerical value\"));\n\nThe option name itself is untranslatable. parse_opt_abbrev_cb() has code\nlike this:\n\n\t\t\treturn error(_(\"option `%s' expects a numerical value\"),\n\t\t\t\t opt->long_name);\n\nThis would work here as well. Advantage: One less string to translate.\n\n> \t}\n> \treturn 0;\n> }\n>\n"},{"id":"437735","messageId":"xmqq7dew4dxu.fsf@gitster.g","threadId":"56604","inReplyTo":"patch-v2-02.11-99f5251c557-20211001T142631Z-avarab@gmail.com","subject":"Re: [PATCH v2 02/11] parse-options.[ch]: consistently use \"enum parse_opt_flags\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-10-01T21:45:17Z","receivedAt":"2021-10-01T21:45:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> Use the \"enum parse_opt_flags\" instead of an \"int flags\" as arguments\n> to the various functions in parse-options.c.\n\nOK.\n\n> In C enums aren't first-class types, and the \"enum\n> parse_opt_option_flag\" uses a enum-as-bitfield pattern. So unlike\n> exhaustively enumerated \"case\" arms we're not going to get validation\n> that we used the \"right\" enum labels.\n>\n> I.e. this won't catch the sort of bug that was fixed with\n> \"PARSE_OPT_SHELL_EVAL\" in the preceding commit.\n\nDrop the two paragraphs above. You do not sell a patch by saying\nwhat benefit it does *not* give us. We do buy a patch by what\nbenefit it does give us, which you describe well below.\n\n> But there's still a benefit to doing this when it comes to the wider C\n> ecosystem. E.g. the GNU debugger (gdb) will helpfully detect and print\n> out meaningful enum labels in this case. Here's the output before and\n> after when breaking in \"parse_options()\" after invoking \"git stash\n> show\":\n>\n> Before:\n>\n> (gdb) p flags\n> $1 = 9\n>\n> After:\n>\n> (gdb) p flags\n> $1 = (PARSE_OPT_KEEP_DASHDASH | PARSE_OPT_KEEP_UNKNOWN)\n"},{"id":"437736","messageId":"xmqqzgrs2z17.fsf@gitster.g","threadId":"56604","inReplyTo":"cover-v2-00.11-00000000000-20211001T142631Z-avarab@gmail.com","subject":"Re: [PATCH v2 00/11] fix bug, use existing enums","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-10-01T21:52:36Z","receivedAt":"2021-10-01T21:52:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> This re-roll of the v1[1] should hopefully address all the feedback on\n> that version, particularly the motivation for the enum-as-bitfield\n> labeling, as expanded on in 02/11.\n\nOther than [3] and [4] that I am not yet convinced is a good idea,\nthis round looks good to me.\n\n\n"},{"id":"437737","messageId":"xmqqv92g2z0i.fsf@gitster.g","threadId":"56604","inReplyTo":"xmqqzgrs2z17.fsf@gitster.g","subject":"Re: [PATCH v2 00/11] fix bug, use existing enums","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-10-01T21:53:01Z","receivedAt":"2021-10-01T21:53:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Ævar Arnfjörð Bjarmason writes:\n>\n>> This re-roll of the v1[1] should hopefully address all the feedback on\n>> that version, particularly the motivation for the enum-as-bitfield\n>> labeling, as expanded on in 02/11.\n>\n> Other than [3] and [4] that I am not yet convinced is a good idea,\n> this round looks good to me.\n\nNot 3&4, but 4&5.\n"},{"id":"438310","messageId":"cover-v3-00.10-00000000000-20211008T190536Z-avarab@gmail.com","threadId":"56604","inReplyTo":"cover-v2-00.11-00000000000-20211001T142631Z-avarab@gmail.com","subject":"[PATCH v3 00/10] fix bug, use more enums","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-10-08T19:07:36Z","receivedAt":"2021-10-08T19:07:53Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"A re-roll of\nhttps://lore.kernel.org/git/cover-v2-00.11-00000000000-20211001T142631Z-avarab@gmail.com/;\nI dropped the addition of exhaustive enum arms for parse_opt_type.\n\nThere's other tweaks here pointed out by Junio & René, thanks both!\nThe \"enum parse_opt_flags flags\" change in the range-diff below was\nthere before, but it was (incorrectly, to begin with) in the\nnow-dropped patch.\n\nÆvar Arnfjörð Bjarmason (10):\n parse-options.h: move PARSE_OPT_SHELL_EVAL between enums\n parse-options.[ch]: consistently use \"enum parse_opt_flags\"\n parse-options.[ch]: consistently use \"enum parse_opt_result\"\n parse-options.c: use exhaustive \"case\" arms for \"enum\n parse_opt_result\"\n parse-options.h: make the \"flags\" in \"struct option\" an enum\n parse-options.c: move optname() earlier in the file\n commit-graph: stop using optname()\n parse-options.[ch]: make opt{bug,name}() \"static\"\n parse-options tests: test optname() output\n parse-options: change OPT_{SHORT,UNSET} to an enum\n\n builtin/blame.c | 3 ++\n builtin/commit-graph.c | 4 +-\n builtin/shortlog.c | 3 ++\n parse-options.c | 87 ++++++++++++++++++++++------------------\n parse-options.h | 26 ++++++------\n t/t0040-parse-options.sh | 42 +++++++++++++++++--\n 6 files changed, 109 insertions(+), 56 deletions(-)\n\nRange-diff against v2:\n 1: 553931702df = 1: 59195845497 parse-options.h: move PARSE_OPT_SHELL_EVAL between enums\n 2: 99f5251c557 ! 2: d4aaaa645de parse-options.[ch]: consistently use \"enum parse_opt_flags\"\n @@ Commit message\n Use the \"enum parse_opt_flags\" instead of an \"int flags\" as arguments\n to the various functions in parse-options.c.\n \n - In C enums aren't first-class types, and the \"enum\n - parse_opt_option_flag\" uses a enum-as-bitfield pattern. So unlike\n - exhaustively enumerated \"case\" arms we're not going to get validation\n - that we used the \"right\" enum labels.\n -\n - I.e. this won't catch the sort of bug that was fixed with\n - \"PARSE_OPT_SHELL_EVAL\" in the preceding commit.\n -\n - But there's still a benefit to doing this when it comes to the wider C\n - ecosystem. E.g. the GNU debugger (gdb) will helpfully detect and print\n - out meaningful enum labels in this case. Here's the output before and\n - after when breaking in \"parse_options()\" after invoking \"git stash\n - show\":\n + Even though this is an enum bitfield there's there's a benefit to\n + doing this when it comes to the wider C ecosystem. E.g. the GNU\n + debugger (gdb) will helpfully detect and print out meaningful enum\n + labels in this case. Here's the output before and after when breaking\n + in \"parse_options()\" after invoking \"git stash show\":\n \n Before:\n \n @@ parse-options.h: struct option {\n NORETURN void usage_with_options(const char * const *usagestr,\n \t\t\t\t const struct option *options);\n @@ parse-options.h: struct parse_opt_ctx_t {\n + \tconst char **out;\n + \tint argc, cpidx, total;\n + \tconst char *opt;\n +-\tint flags;\n ++\tenum parse_opt_flags flags;\n + \tconst char *prefix;\n + \tconst char **alias_groups; /* must be in groups of 3 elements! */\n + \tstruct option *updated_options;\n +@@ parse-options.h: struct parse_opt_ctx_t {\n \n void parse_options_start(struct parse_opt_ctx_t *ctx,\n \t\t\t int argc, const char **argv, const char *prefix,\n 3: be198e42df9 = 3: 828e8b96574 parse-options.[ch]: consistently use \"enum parse_opt_result\"\n 4: a253db7a60d = 4: bebf3448c49 parse-options.c: use exhaustive \"case\" arms for \"enum parse_opt_result\"\n 5: 467828780d0 < -: ----------- parse-options.c: use exhaustive \"case\" arms for \"enum parse_opt_type\"\n 6: 34669327550 = 5: 23e62d4139f parse-options.h: make the \"flags\" in \"struct option\" an enum\n 7: e82a4e477d5 = 6: 7afdb22885d parse-options.c: move optname() earlier in the file\n 8: 58683b3d89d ! 7: 67a9b38f66c commit-graph: stop using optname()\n @@ Commit message\n of \"opt->flags\" to optname(), that function expects flags, but not\n *those* flags.\n \n + Let's pass \"max-new-filters\" to the new error because the option name\n + isn't translatable, and because we can re-use a translation added in\n + f7e68a08780 (parse-options: check empty value in OPT_INTEGER and\n + OPT_ABBREV, 2019-05-29).\n +\n Signed-off-by: Ævar Arnfjörð Bjarmason \n \n ## builtin/commit-graph.c ##\n @@ builtin/commit-graph.c: static int write_option_max_new_filters(const struct opt\n \t\tif (*s)\n -\t\t\treturn error(_(\"%s expects a numerical value\"),\n -\t\t\t\t optname(opt, opt->flags));\n -+\t\t\treturn error(_(\"option `max-new-filters' expects a numerical value\"));\n ++\t\t\treturn error(_(\"option `%s' expects a numerical value\"),\n ++\t\t\t\t \"max-new-filters\");\n \t}\n \treturn 0;\n }\n 9: 8cbee660174 = 8: 520cc5a8dc0 parse-options.[ch]: make opt{bug,name}() \"static\"\n10: f727f683eb1 = 9: a82987a03c7 parse-options tests: test optname() output\n11: 4fbc07fc7fd = 10: e05627d3634 parse-options: change OPT_{SHORT,UNSET} to an enum\n-- \n2.33.0.1446.g6af949f83bd\n\n"},{"id":"438311","messageId":"patch-v3-01.10-59195845497-20211008T190536Z-avarab@gmail.com","threadId":"56604","inReplyTo":"cover-v3-00.10-00000000000-20211008T190536Z-avarab@gmail.com","subject":"[PATCH v3 01/10] parse-options.h: move PARSE_OPT_SHELL_EVAL between enums","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-10-08T19:07:37Z","receivedAt":"2021-10-08T19:07:55Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Fix a bad landmine of a bug which has been with us ever since\nPARSE_OPT_SHELL_EVAL was added in 47e9cd28f8a (parseopt: wrap\nrev-parse --parseopt usage for eval consumption, 2010-06-12).\n\nIt's an argument to parse_options() and should therefore be in \"enum\nparse_opt_flags\", but it was added to the per-option \"enum\nparse_opt_option_flags\" by mistake.\n\nTherefore as soon as we'd have an enum member in the former that\nreached its value of \"1 << 8\" we'd run into a seemingly bizarre bug\nwhere that new option would turn on the unrelated PARSE_OPT_SHELL_EVAL\nin \"git rev-parse --parseopt\" by proxy.\n\nI manually checked that no other enum members suffered from such\noverlap, by setting the values to non-overlapping values, and making\nthe relevant codepaths BUG() out if the given value was above/below\nthe expected (excluding flags=0 in the case of \"enum\nparse_opt_flags\").\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n parse-options.h | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/parse-options.h b/parse-options.h\nindex 39d90882548..3a3176ae65c 100644\n--- a/parse-options.h\n+++ b/parse-options.h\n@@ -33,6 +33,7 @@ enum parse_opt_flags {\n \tPARSE_OPT_KEEP_UNKNOWN = 1 << 3,\n \tPARSE_OPT_NO_INTERNAL_HELP = 1 << 4,\n \tPARSE_OPT_ONE_SHOT = 1 << 5,\n+\tPARSE_OPT_SHELL_EVAL = 1 << 6,\n };\n \n enum parse_opt_option_flags {\n@@ -44,7 +45,6 @@ enum parse_opt_option_flags {\n \tPARSE_OPT_NODASH = 1 << 5,\n \tPARSE_OPT_LITERAL_ARGHELP = 1 << 6,\n \tPARSE_OPT_FROM_ALIAS = 1 << 7,\n-\tPARSE_OPT_SHELL_EVAL = 1 << 8,\n \tPARSE_OPT_NOCOMPLETE = 1 << 9,\n \tPARSE_OPT_COMP_ARG = 1 << 10,\n \tPARSE_OPT_CMDMODE = 1 << 11,\n-- \n2.33.0.1446.g6af949f83bd\n\n"},{"id":"438312","messageId":"patch-v3-03.10-828e8b96574-20211008T190536Z-avarab@gmail.com","threadId":"56604","inReplyTo":"cover-v3-00.10-00000000000-20211008T190536Z-avarab@gmail.com","subject":"[PATCH v3 03/10] parse-options.[ch]: consistently use \"enum parse_opt_result\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-10-08T19:07:39Z","receivedAt":"2021-10-08T19:08:07Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Use the \"enum parse_opt_result\" instead of an \"int flags\" as the\nreturn value of the applicable functions in parse-options.c.\n\nThis will help catch future bugs, such as the missing \"case\" arms in\nthe two existing users of the API in \"blame.c\" and \"shortlog.c\". A\nthird caller in 309be813c9b (update-index: migrate to parse-options\nAPI, 2010-12-01) was already checking for these.\n\nAs can be seen when trying to sort through the deluge of warnings\nproduced when compiling this with CC=g++ (mostly unrelated to this\nchange) we're not consistently using \"enum parse_opt_result\" even now,\ni.e. we'll return error() and \"return 0;\". See f41179f16ba\n(parse-options: avoid magic return codes, 2019-01-27) for a commit\nwhich started changing some of that.\n\nI'm not doing any more of that exhaustive migration here, and it's\nprobably not worthwhile past the point of being able to check \"enum\nparse_opt_result\" in switch().\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n builtin/blame.c | 3 +++\n builtin/shortlog.c | 3 +++\n parse-options.c | 31 +++++++++++++++++--------------\n parse-options.h | 15 ++++++++-------\n 4 files changed, 31 insertions(+), 21 deletions(-)\n\ndiff --git a/builtin/blame.c b/builtin/blame.c\nindex 641523ff9af..9273fb222d5 100644\n--- a/builtin/blame.c\n+++ b/builtin/blame.c\n@@ -917,6 +917,9 @@ int cmd_blame(int argc, const char **argv, const char *prefix)\n \t\t\t PARSE_OPT_KEEP_DASHDASH | PARSE_OPT_KEEP_ARGV0);\n \tfor (;;) {\n \t\tswitch (parse_options_step(&ctx, options, blame_opt_usage)) {\n+\t\tcase PARSE_OPT_NON_OPTION:\n+\t\tcase PARSE_OPT_UNKNOWN:\n+\t\t\tbreak;\n \t\tcase PARSE_OPT_HELP:\n \t\tcase PARSE_OPT_ERROR:\n \t\t\texit(129);\ndiff --git a/builtin/shortlog.c b/builtin/shortlog.c\nindex 3e7ab1ca821..e7f7af5de3f 100644\n--- a/builtin/shortlog.c\n+++ b/builtin/shortlog.c\n@@ -374,6 +374,9 @@ int cmd_shortlog(int argc, const char **argv, const char *prefix)\n \n \tfor (;;) {\n \t\tswitch (parse_options_step(&ctx, options, shortlog_usage)) {\n+\t\tcase PARSE_OPT_NON_OPTION:\n+\t\tcase PARSE_OPT_UNKNOWN:\n+\t\t\tbreak;\n \t\tcase PARSE_OPT_HELP:\n \t\tcase PARSE_OPT_ERROR:\n \t\t\texit(129);\ndiff --git a/parse-options.c b/parse-options.c\nindex 9c8ba963400..f718242096c 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -699,13 +699,14 @@ static void free_preprocessed_options(struct option *options)\n \tfree(options);\n }\n \n-static int usage_with_options_internal(struct parse_opt_ctx_t *,\n-\t\t\t\t const char * const *,\n-\t\t\t\t const struct option *, int, int);\n-\n-int parse_options_step(struct parse_opt_ctx_t *ctx,\n-\t\t const struct option *options,\n-\t\t const char * const usagestr[])\n+static enum parse_opt_result usage_with_options_internal(struct parse_opt_ctx_t *,\n+\t\t\t\t\t\t\t const char * const *,\n+\t\t\t\t\t\t\t const struct option *,\n+\t\t\t\t\t\t\t int, int);\n+\n+enum parse_opt_result parse_options_step(struct parse_opt_ctx_t *ctx,\n+\t\t\t\t\t const struct option *options,\n+\t\t\t\t\t const char * const usagestr[])\n {\n \tint internal_help = !(ctx->flags & PARSE_OPT_NO_INTERNAL_HELP);\n \n@@ -839,10 +840,11 @@ int parse_options_end(struct parse_opt_ctx_t *ctx)\n \treturn ctx->cpidx + ctx->argc;\n }\n \n-int parse_options(int argc, const char **argv, const char *prefix,\n-\t\t const struct option *options,\n-\t\t const char * const usagestr[],\n-\t\t enum parse_opt_flags flags)\n+enum parse_opt_result parse_options(int argc, const char **argv,\n+\t\t\t\t const char *prefix,\n+\t\t\t\t const struct option *options,\n+\t\t\t\t const char * const usagestr[],\n+\t\t\t\t enum parse_opt_flags flags)\n {\n \tstruct parse_opt_ctx_t ctx;\n \tstruct option *real_options;\n@@ -900,9 +902,10 @@ static int usage_argh(const struct option *opts, FILE *outfile)\n #define USAGE_OPTS_WIDTH 24\n #define USAGE_GAP 2\n \n-static int usage_with_options_internal(struct parse_opt_ctx_t *ctx,\n-\t\t\t\t const char * const *usagestr,\n-\t\t\t\t const struct option *opts, int full, int err)\n+static enum parse_opt_result usage_with_options_internal(struct parse_opt_ctx_t *ctx,\n+\t\t\t\t\t\t\t const char * const *usagestr,\n+\t\t\t\t\t\t\t const struct option *opts,\n+\t\t\t\t\t\t\t int full, int err)\n {\n \tFILE *outfile = err ? stderr : stdout;\n \tint need_newline;\ndiff --git a/parse-options.h b/parse-options.h\nindex 2e8798d8744..a1c7c86ad30 100644\n--- a/parse-options.h\n+++ b/parse-options.h\n@@ -211,10 +211,11 @@ struct option {\n * untouched and parse_options() returns the number of options\n * processed.\n */\n-int parse_options(int argc, const char **argv, const char *prefix,\n-\t\t const struct option *options,\n-\t\t const char * const usagestr[],\n-\t\t enum parse_opt_flags flags);\n+enum parse_opt_result parse_options(int argc, const char **argv,\n+\t\t\t\t const char *prefix,\n+\t\t\t\t const struct option *options,\n+\t\t\t\t const char * const usagestr[],\n+\t\t\t\t enum parse_opt_flags flags);\n \n NORETURN void usage_with_options(const char * const *usagestr,\n \t\t\t\t const struct option *options);\n@@ -274,9 +275,9 @@ void parse_options_start(struct parse_opt_ctx_t *ctx,\n \t\t\t const struct option *options,\n \t\t\t enum parse_opt_flags flags);\n \n-int parse_options_step(struct parse_opt_ctx_t *ctx,\n-\t\t const struct option *options,\n-\t\t const char * const usagestr[]);\n+enum parse_opt_result parse_options_step(struct parse_opt_ctx_t *ctx,\n+\t\t\t\t\t const struct option *options,\n+\t\t\t\t\t const char * const usagestr[]);\n \n int parse_options_end(struct parse_opt_ctx_t *ctx);\n \n-- \n2.33.0.1446.g6af949f83bd\n\n"},{"id":"438313","messageId":"patch-v3-02.10-d4aaaa645de-20211008T190536Z-avarab@gmail.com","threadId":"56604","inReplyTo":"cover-v3-00.10-00000000000-20211008T190536Z-avarab@gmail.com","subject":"[PATCH v3 02/10] parse-options.[ch]: consistently use \"enum parse_opt_flags\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-10-08T19:07:38Z","receivedAt":"2021-10-08T19:08:09Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Use the \"enum parse_opt_flags\" instead of an \"int flags\" as arguments\nto the various functions in parse-options.c.\n\nEven though this is an enum bitfield there's there's a benefit to\ndoing this when it comes to the wider C ecosystem. E.g. the GNU\ndebugger (gdb) will helpfully detect and print out meaningful enum\nlabels in this case. Here's the output before and after when breaking\nin \"parse_options()\" after invoking \"git stash show\":\n\nBefore:\n\n (gdb) p flags\n $1 = 9\n\nAfter:\n\n (gdb) p flags\n $1 = (PARSE_OPT_KEEP_DASHDASH | PARSE_OPT_KEEP_UNKNOWN)\n\nOf course as noted in[1] there's a limit to this smartness,\ni.e. manually setting it with unrelated enum labels won't be\ncaught. There are some third-party extensions to do more exhaustive\nchecking[2], perhaps we'll be able to make use of them sooner than\nlater.\n\nWe've also got prior art using this pattern in the codebase. See\ne.g. \"enum bloom_filter_computed\" added in 312cff52074 (bloom: split\n'get_bloom_filter()' in two, 2020-09-16) and the \"permitted\" enum\nadded in ce910287e72 (add -p: fix checking of user input, 2020-08-17).\n\n1. https://lore.kernel.org/git/87mtnvvj3c.fsf@evledraar.gmail.com/\n2. https://github.com/sinelaw/elfs-clang-plugins/blob/master/enums_conversion/README.md\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n parse-options.c | 11 +++++++----\n parse-options.h | 8 +++++---\n 2 files changed, 12 insertions(+), 7 deletions(-)\n\ndiff --git a/parse-options.c b/parse-options.c\nindex 55c5821b08d..9c8ba963400 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -481,7 +481,8 @@ static void parse_options_check(const struct option *opts)\n \n static void parse_options_start_1(struct parse_opt_ctx_t *ctx,\n \t\t\t\t int argc, const char **argv, const char *prefix,\n-\t\t\t\t const struct option *options, int flags)\n+\t\t\t\t const struct option *options,\n+\t\t\t\t enum parse_opt_flags flags)\n {\n \tctx->argc = argc;\n \tctx->argv = argv;\n@@ -506,7 +507,8 @@ static void parse_options_start_1(struct parse_opt_ctx_t *ctx,\n \n void parse_options_start(struct parse_opt_ctx_t *ctx,\n \t\t\t int argc, const char **argv, const char *prefix,\n-\t\t\t const struct option *options, int flags)\n+\t\t\t const struct option *options,\n+\t\t\t enum parse_opt_flags flags)\n {\n \tmemset(ctx, 0, sizeof(*ctx));\n \tparse_options_start_1(ctx, argc, argv, prefix, options, flags);\n@@ -838,8 +840,9 @@ int parse_options_end(struct parse_opt_ctx_t *ctx)\n }\n \n int parse_options(int argc, const char **argv, const char *prefix,\n-\t\t const struct option *options, const char * const usagestr[],\n-\t\t int flags)\n+\t\t const struct option *options,\n+\t\t const char * const usagestr[],\n+\t\t enum parse_opt_flags flags)\n {\n \tstruct parse_opt_ctx_t ctx;\n \tstruct option *real_options;\ndiff --git a/parse-options.h b/parse-options.h\nindex 3a3176ae65c..2e8798d8744 100644\n--- a/parse-options.h\n+++ b/parse-options.h\n@@ -213,7 +213,8 @@ struct option {\n */\n int parse_options(int argc, const char **argv, const char *prefix,\n \t\t const struct option *options,\n-\t\t const char * const usagestr[], int flags);\n+\t\t const char * const usagestr[],\n+\t\t enum parse_opt_flags flags);\n \n NORETURN void usage_with_options(const char * const *usagestr,\n \t\t\t\t const struct option *options);\n@@ -262,7 +263,7 @@ struct parse_opt_ctx_t {\n \tconst char **out;\n \tint argc, cpidx, total;\n \tconst char *opt;\n-\tint flags;\n+\tenum parse_opt_flags flags;\n \tconst char *prefix;\n \tconst char **alias_groups; /* must be in groups of 3 elements! */\n \tstruct option *updated_options;\n@@ -270,7 +271,8 @@ struct parse_opt_ctx_t {\n \n void parse_options_start(struct parse_opt_ctx_t *ctx,\n \t\t\t int argc, const char **argv, const char *prefix,\n-\t\t\t const struct option *options, int flags);\n+\t\t\t const struct option *options,\n+\t\t\t enum parse_opt_flags flags);\n \n int parse_options_step(struct parse_opt_ctx_t *ctx,\n \t\t const struct option *options,\n-- \n2.33.0.1446.g6af949f83bd\n\n"},{"id":"438314","messageId":"patch-v3-05.10-23e62d4139f-20211008T190536Z-avarab@gmail.com","threadId":"56604","inReplyTo":"cover-v3-00.10-00000000000-20211008T190536Z-avarab@gmail.com","subject":"[PATCH v3 05/10] parse-options.h: make the \"flags\" in \"struct option\" an enum","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-10-08T19:07:41Z","receivedAt":"2021-10-08T19:08:12Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Change the \"flags\" members of \"struct option\" to refer to their\ncorresponding \"enum\" defined earlier in the file.\n\nThe benefit of changing this to an enum isn't as great as with some\n\"enum parse_opt_type\" as we'll always check this as a bitfield, so we\ncan't rely on the compiler checking \"case\" arms for us. But let's do\nit for consistency with the rest of the file.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n parse-options.h | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/parse-options.h b/parse-options.h\nindex a1c7c86ad30..74b66ba6e93 100644\n--- a/parse-options.h\n+++ b/parse-options.h\n@@ -134,7 +134,7 @@ struct option {\n \tconst char *argh;\n \tconst char *help;\n \n-\tint flags;\n+\tenum parse_opt_option_flags flags;\n \tparse_opt_cb *callback;\n \tintptr_t defval;\n \tparse_opt_ll_cb *ll_callback;\n-- \n2.33.0.1446.g6af949f83bd\n\n"},{"id":"438315","messageId":"patch-v3-04.10-bebf3448c49-20211008T190536Z-avarab@gmail.com","threadId":"56604","inReplyTo":"cover-v3-00.10-00000000000-20211008T190536Z-avarab@gmail.com","subject":"[PATCH v3 04/10] parse-options.c: use exhaustive \"case\" arms for \"enum parse_opt_result\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-10-08T19:07:40Z","receivedAt":"2021-10-08T19:08:30Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Change the \"default\" case in parse_options() that handles the return\nvalue of parse_options_step() to simply have a \"case\" arm for\nPARSE_OPT_UNKNOWN, instead of leaving it to a comment. This means the\ncompiler can warn us about any missing case arms.\n\nThis adjusts code added in ff43ec3e2d2 (parse-opt: create\nparse_options_step., 2008-06-23), given its age it may pre-date the\nexistence (or widespread use) of this coding style, which we've since\nadopted more widely.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n parse-options.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/parse-options.c b/parse-options.c\nindex f718242096c..e33700d6e71 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -866,7 +866,7 @@ enum parse_opt_result parse_options(int argc, const char **argv,\n \tcase PARSE_OPT_NON_OPTION:\n \tcase PARSE_OPT_DONE:\n \t\tbreak;\n-\tdefault: /* PARSE_OPT_UNKNOWN */\n+\tcase PARSE_OPT_UNKNOWN:\n \t\tif (ctx.argv[0][1] == '-') {\n \t\t\terror(_(\"unknown option `%s'\"), ctx.argv[0] + 2);\n \t\t} else if (isascii(*ctx.opt)) {\n-- \n2.33.0.1446.g6af949f83bd\n\n"},{"id":"438316","messageId":"patch-v3-06.10-7afdb22885d-20211008T190536Z-avarab@gmail.com","threadId":"56604","inReplyTo":"cover-v3-00.10-00000000000-20211008T190536Z-avarab@gmail.com","subject":"[PATCH v3 06/10] parse-options.c: move optname() earlier in the file","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-10-08T19:07:42Z","receivedAt":"2021-10-08T19:08:34Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"In preparation for making \"optname\" a static function move it above\nits first user in parse-options.c.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n parse-options.c | 30 +++++++++++++++---------------\n 1 file changed, 15 insertions(+), 15 deletions(-)\n\ndiff --git a/parse-options.c b/parse-options.c\nindex e33700d6e71..9e2da8383d7 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -22,6 +22,21 @@ int optbug(const struct option *opt, const char *reason)\n \treturn error(\"BUG: switch '%c' %s\", opt->short_name, reason);\n }\n \n+const char *optname(const struct option *opt, int flags)\n+{\n+\tstatic struct strbuf sb = STRBUF_INIT;\n+\n+\tstrbuf_reset(&sb);\n+\tif (flags & OPT_SHORT)\n+\t\tstrbuf_addf(&sb, \"switch `%c'\", opt->short_name);\n+\telse if (flags & OPT_UNSET)\n+\t\tstrbuf_addf(&sb, \"option `no-%s'\", opt->long_name);\n+\telse\n+\t\tstrbuf_addf(&sb, \"option `%s'\", opt->long_name);\n+\n+\treturn sb.buf;\n+}\n+\n static enum parse_opt_result get_arg(struct parse_opt_ctx_t *p,\n \t\t\t\t const struct option *opt,\n \t\t\t\t int flags, const char **arg)\n@@ -1006,18 +1021,3 @@ void NORETURN usage_msg_opt(const char *msg,\n \tfprintf(stderr, \"fatal: %s\\n\\n\", msg);\n \tusage_with_options(usagestr, options);\n }\n-\n-const char *optname(const struct option *opt, int flags)\n-{\n-\tstatic struct strbuf sb = STRBUF_INIT;\n-\n-\tstrbuf_reset(&sb);\n-\tif (flags & OPT_SHORT)\n-\t\tstrbuf_addf(&sb, \"switch `%c'\", opt->short_name);\n-\telse if (flags & OPT_UNSET)\n-\t\tstrbuf_addf(&sb, \"option `no-%s'\", opt->long_name);\n-\telse\n-\t\tstrbuf_addf(&sb, \"option `%s'\", opt->long_name);\n-\n-\treturn sb.buf;\n-}\n-- \n2.33.0.1446.g6af949f83bd\n\n"},{"id":"438317","messageId":"patch-v3-07.10-67a9b38f66c-20211008T190536Z-avarab@gmail.com","threadId":"56604","inReplyTo":"cover-v3-00.10-00000000000-20211008T190536Z-avarab@gmail.com","subject":"[PATCH v3 07/10] commit-graph: stop using optname()","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-10-08T19:07:43Z","receivedAt":"2021-10-08T19:08:37Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Stop using optname() in builtin/commit-graph.c to emit an error with\nthe --max-new-filters option. This changes code added in 809e0327f57\n(builtin/commit-graph.c: introduce '--max-new-filters=',\n2020-09-18).\n\nSee 9440b831ad5 (parse-options: replace opterror() with optname(),\n2018-11-10) for why using optname() like this is considered bad,\ni.e. it's assembling human-readable output piecemeal, and the \"option\n`X'\" at the start can't be translated.\n\nIt didn't matter in this case, but this code was also buggy in its use\nof \"opt->flags\" to optname(), that function expects flags, but not\n*those* flags.\n\nLet's pass \"max-new-filters\" to the new error because the option name\nisn't translatable, and because we can re-use a translation added in\nf7e68a08780 (parse-options: check empty value in OPT_INTEGER and\nOPT_ABBREV, 2019-05-29).\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n builtin/commit-graph.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/commit-graph.c b/builtin/commit-graph.c\nindex 3c3de3a156f..ab8e5cd59af 100644\n--- a/builtin/commit-graph.c\n+++ b/builtin/commit-graph.c\n@@ -172,8 +172,8 @@ static int write_option_max_new_filters(const struct option *opt,\n \t\tconst char *s;\n \t\t*to = strtol(arg, (char **)&s, 10);\n \t\tif (*s)\n-\t\t\treturn error(_(\"%s expects a numerical value\"),\n-\t\t\t\t optname(opt, opt->flags));\n+\t\t\treturn error(_(\"option `%s' expects a numerical value\"),\n+\t\t\t\t \"max-new-filters\");\n \t}\n \treturn 0;\n }\n-- \n2.33.0.1446.g6af949f83bd\n\n"},{"id":"438318","messageId":"patch-v3-08.10-520cc5a8dc0-20211008T190536Z-avarab@gmail.com","threadId":"56604","inReplyTo":"cover-v3-00.10-00000000000-20211008T190536Z-avarab@gmail.com","subject":"[PATCH v3 08/10] parse-options.[ch]: make opt{bug,name}() \"static\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-10-08T19:07:44Z","receivedAt":"2021-10-08T19:08:40Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Change these two functions to \"static\", the last user of \"optname()\"\noutside of parse-options.c itself went away in the preceding commit,\nfor the reasons noted in 9440b831ad5 (parse-options: replace\nopterror() with optname(), 2018-11-10) we shouldn't be adding any more\nusers of it.\n\nThe \"optbug()\" function was never used outside of parse-options.c, but\nwas made non-static in 1f275b7c4ca (parse-options: export opterr,\noptbug, 2011-08-11). I think the only external user of optname() was\nthe commit-graph.c caller added in 09e0327f57 (builtin/commit-graph.c:\nintroduce '--max-new-filters=', 2020-09-18).\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n parse-options.c | 4 ++--\n parse-options.h | 3 ---\n 2 files changed, 2 insertions(+), 5 deletions(-)\n\ndiff --git a/parse-options.c b/parse-options.c\nindex 9e2da8383d7..64bd4c32854 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -11,7 +11,7 @@ static int disallow_abbreviated_options;\n #define OPT_SHORT 1\n #define OPT_UNSET 2\n \n-int optbug(const struct option *opt, const char *reason)\n+static int optbug(const struct option *opt, const char *reason)\n {\n \tif (opt->long_name) {\n \t\tif (opt->short_name)\n@@ -22,7 +22,7 @@ int optbug(const struct option *opt, const char *reason)\n \treturn error(\"BUG: switch '%c' %s\", opt->short_name, reason);\n }\n \n-const char *optname(const struct option *opt, int flags)\n+static const char *optname(const struct option *opt, int flags)\n {\n \tstatic struct strbuf sb = STRBUF_INIT;\n \ndiff --git a/parse-options.h b/parse-options.h\nindex 74b66ba6e93..dd79c9c566f 100644\n--- a/parse-options.h\n+++ b/parse-options.h\n@@ -224,9 +224,6 @@ NORETURN void usage_msg_opt(const char *msg,\n \t\t\t const char * const *usagestr,\n \t\t\t const struct option *options);\n \n-int optbug(const struct option *opt, const char *reason);\n-const char *optname(const struct option *opt, int flags);\n-\n /*\n * Use these assertions for callbacks that expect to be called with NONEG and\n * NOARG respectively, and do not otherwise handle the \"unset\" and \"arg\"\n-- \n2.33.0.1446.g6af949f83bd\n\n"},{"id":"438319","messageId":"patch-v3-09.10-a82987a03c7-20211008T190536Z-avarab@gmail.com","threadId":"56604","inReplyTo":"cover-v3-00.10-00000000000-20211008T190536Z-avarab@gmail.com","subject":"[PATCH v3 09/10] parse-options tests: test optname() output","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-10-08T19:07:45Z","receivedAt":"2021-10-08T19:08:43Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"There were no tests for checking the specific output that we'll\ngenerate in optname(), let's add some. That output was added back in\n4a59fd13122 (Add a simple option parser., 2007-10-15).\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n t/t0040-parse-options.sh | 42 +++++++++++++++++++++++++++++++++++++---\n 1 file changed, 39 insertions(+), 3 deletions(-)\n\ndiff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh\nindex da310ed29b1..d6f391a497b 100755\n--- a/t/t0040-parse-options.sh\n+++ b/t/t0040-parse-options.sh\n@@ -168,9 +168,45 @@ test_expect_success 'long options' '\n '\n \n test_expect_success 'missing required value' '\n-\ttest_expect_code 129 test-tool parse-options -s &&\n-\ttest_expect_code 129 test-tool parse-options --string &&\n-\ttest_expect_code 129 test-tool parse-options --file\n+\tcat >expect <<-\\EOF &&\n+\terror: switch `s'\\'' requires a value\n+\tEOF\n+\ttest_expect_code 129 test-tool parse-options -s 2>actual &&\n+\ttest_cmp expect actual &&\n+\n+\tcat >expect <<-\\EOF &&\n+\terror: option `string'\\'' requires a value\n+\tEOF\n+\ttest_expect_code 129 test-tool parse-options --string 2>actual &&\n+\ttest_cmp expect actual &&\n+\n+\tcat >expect <<-\\EOF &&\n+\terror: option `file'\\'' requires a value\n+\tEOF\n+\ttest_expect_code 129 test-tool parse-options --file 2>actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'superfluous value provided: boolean' '\n+\tcat >expect <<-\\EOF &&\n+\terror: option `yes'\\'' takes no value\n+\tEOF\n+\ttest_expect_code 129 test-tool parse-options --yes=hi 2>actual &&\n+\ttest_cmp expect actual &&\n+\n+\tcat >expect <<-\\EOF &&\n+\terror: option `no-yes'\\'' takes no value\n+\tEOF\n+\ttest_expect_code 129 test-tool parse-options --no-yes=hi 2>actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'superfluous value provided: cmdmode' '\n+\tcat >expect <<-\\EOF &&\n+\terror: option `mode1'\\'' takes no value\n+\tEOF\n+\ttest_expect_code 129 test-tool parse-options --mode1=hi 2>actual &&\n+\ttest_cmp expect actual\n '\n \n cat >expect <<\\EOF\n-- \n2.33.0.1446.g6af949f83bd\n\n"},{"id":"438320","messageId":"patch-v3-10.10-e05627d3634-20211008T190536Z-avarab@gmail.com","threadId":"56604","inReplyTo":"cover-v3-00.10-00000000000-20211008T190536Z-avarab@gmail.com","subject":"[PATCH v3 10/10] parse-options: change OPT_{SHORT,UNSET} to an enum","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-10-08T19:07:46Z","receivedAt":"2021-10-08T19:08:46Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Change the comparisons against OPT_SHORT and OPT_UNSET to an enum\nwhich keeps track of how a given option got parsed. The case of \"0\"\nwas an implicit OPT_LONG, so let's add an explicit label for it.\n\nDue to the xor in 0f1930c5875 (parse-options: allow positivation of\noptions starting, with no-, 2012-02-25) the code already relied on\nthis being set back to 0. To avoid refactoring the logic involved in\nthat let's just start the enum at \"0\" instead of the usual \"1<<0\" (1),\nbut BUG() out if we don't have one of our expected flags.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n parse-options.c | 23 ++++++++++++++---------\n 1 file changed, 14 insertions(+), 9 deletions(-)\n\ndiff --git a/parse-options.c b/parse-options.c\nindex 64bd4c32854..2a2c0ee24f2 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -8,8 +8,11 @@\n \n static int disallow_abbreviated_options;\n \n-#define OPT_SHORT 1\n-#define OPT_UNSET 2\n+enum opt_parsed {\n+\tOPT_LONG = 0,\n+\tOPT_SHORT = 1<<0,\n+\tOPT_UNSET = 1<<1,\n+};\n \n static int optbug(const struct option *opt, const char *reason)\n {\n@@ -22,7 +25,7 @@ static int optbug(const struct option *opt, const char *reason)\n \treturn error(\"BUG: switch '%c' %s\", opt->short_name, reason);\n }\n \n-static const char *optname(const struct option *opt, int flags)\n+static const char *optname(const struct option *opt, enum opt_parsed flags)\n {\n \tstatic struct strbuf sb = STRBUF_INIT;\n \n@@ -31,15 +34,17 @@ static const char *optname(const struct option *opt, int flags)\n \t\tstrbuf_addf(&sb, \"switch `%c'\", opt->short_name);\n \telse if (flags & OPT_UNSET)\n \t\tstrbuf_addf(&sb, \"option `no-%s'\", opt->long_name);\n-\telse\n+\telse if (flags == OPT_LONG)\n \t\tstrbuf_addf(&sb, \"option `%s'\", opt->long_name);\n+\telse\n+\t\tBUG(\"optname() got unknown flags %d\", flags);\n \n \treturn sb.buf;\n }\n \n static enum parse_opt_result get_arg(struct parse_opt_ctx_t *p,\n \t\t\t\t const struct option *opt,\n-\t\t\t\t int flags, const char **arg)\n+\t\t\t\t enum opt_parsed flags, const char **arg)\n {\n \tif (p->opt) {\n \t\t*arg = p->opt;\n@@ -65,7 +70,7 @@ static void fix_filename(const char *prefix, const char **file)\n static enum parse_opt_result opt_command_mode_error(\n \tconst struct option *opt,\n \tconst struct option *all_opts,\n-\tint flags)\n+\tenum opt_parsed flags)\n {\n \tconst struct option *that;\n \tstruct strbuf that_name = STRBUF_INIT;\n@@ -97,7 +102,7 @@ static enum parse_opt_result opt_command_mode_error(\n static enum parse_opt_result get_value(struct parse_opt_ctx_t *p,\n \t\t\t\t const struct option *opt,\n \t\t\t\t const struct option *all_opts,\n-\t\t\t\t int flags)\n+\t\t\t\t enum opt_parsed flags)\n {\n \tconst char *s, *arg;\n \tconst int unset = flags & OPT_UNSET;\n@@ -313,11 +318,11 @@ static enum parse_opt_result parse_long_opt(\n \tconst struct option *all_opts = options;\n \tconst char *arg_end = strchrnul(arg, '=');\n \tconst struct option *abbrev_option = NULL, *ambiguous_option = NULL;\n-\tint abbrev_flags = 0, ambiguous_flags = 0;\n+\tenum opt_parsed abbrev_flags = OPT_LONG, ambiguous_flags = OPT_LONG;\n \n \tfor (; options->type != OPTION_END; options++) {\n \t\tconst char *rest, *long_name = options->long_name;\n-\t\tint flags = 0, opt_flags = 0;\n+\t\tenum opt_parsed flags = OPT_LONG, opt_flags = OPT_LONG;\n \n \t\tif (!long_name)\n \t\t\tcontinue;\n-- \n2.33.0.1446.g6af949f83bd\n\n"},{"id":"440588","messageId":"20211106191103.GA5811@szeder.dev","threadId":"56604","inReplyTo":"patch-v3-03.10-828e8b96574-20211008T190536Z-avarab@gmail.com","subject":"Re: [PATCH v3 03/10] parse-options.[ch]: consistently use \"enum parse_opt_result\"","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2021-11-06T19:11:03Z","receivedAt":"2021-11-06T19:11:16Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Fri, Oct 08, 2021 at 09:07:39PM +0200, Ævar Arnfjörð Bjarmason wrote:\n> Use the \"enum parse_opt_result\" instead of an \"int flags\" as the\n> return value of the applicable functions in parse-options.c.\n> \n> This will help catch future bugs, such as the missing \"case\" arms in\n> the two existing users of the API in \"blame.c\" and \"shortlog.c\". A\n> third caller in 309be813c9b (update-index: migrate to parse-options\n> API, 2010-12-01) was already checking for these.\n> \n> As can be seen when trying to sort through the deluge of warnings\n> produced when compiling this with CC=g++ (mostly unrelated to this\n> change) we're not consistently using \"enum parse_opt_result\" even now,\n> i.e. we'll return error() and \"return 0;\". See f41179f16ba\n> (parse-options: avoid magic return codes, 2019-01-27) for a commit\n> which started changing some of that.\n> \n> I'm not doing any more of that exhaustive migration here, and it's\n> probably not worthwhile past the point of being able to check \"enum\n> parse_opt_result\" in switch().\n> \n> Signed-off-by: Ævar Arnfjörð Bjarmason \n> ---\n\n> diff --git a/parse-options.c b/parse-options.c\n> index 9c8ba963400..f718242096c 100644\n> --- a/parse-options.c\n> +++ b/parse-options.c\n> @@ -699,13 +699,14 @@ static void free_preprocessed_options(struct option *options)\n> \tfree(options);\n> }\n> \n> -static int usage_with_options_internal(struct parse_opt_ctx_t *,\n> -\t\t\t\t const char * const *,\n> -\t\t\t\t const struct option *, int, int);\n> -\n> -int parse_options_step(struct parse_opt_ctx_t *ctx,\n> -\t\t const struct option *options,\n> -\t\t const char * const usagestr[])\n> +static enum parse_opt_result usage_with_options_internal(struct parse_opt_ctx_t *,\n> +\t\t\t\t\t\t\t const char * const *,\n> +\t\t\t\t\t\t\t const struct option *,\n> +\t\t\t\t\t\t\t int, int);\n> +\n> +enum parse_opt_result parse_options_step(struct parse_opt_ctx_t *ctx,\n> +\t\t\t\t\t const struct option *options,\n> +\t\t\t\t\t const char * const usagestr[])\n> {\n> \tint internal_help = !(ctx->flags & PARSE_OPT_NO_INTERNAL_HELP);\n> \n> @@ -839,10 +840,11 @@ int parse_options_end(struct parse_opt_ctx_t *ctx)\n> \treturn ctx->cpidx + ctx->argc;\n> }\n> \n> -int parse_options(int argc, const char **argv, const char *prefix,\n> -\t\t const struct option *options,\n> -\t\t const char * const usagestr[],\n> -\t\t enum parse_opt_flags flags)\n> +enum parse_opt_result parse_options(int argc, const char **argv,\n\nThe return type of this function should have been left unchanged,\nbecause it contains only one return statement:\n\n return parse_options_end(&ctx);\n\nand parse_options_end() does return an int.\n\n"},{"id":"440617","messageId":"211106.86lf21ezqx.gmgdl@evledraar.gmail.com","threadId":"56604","inReplyTo":"20211106191103.GA5811@szeder.dev","subject":"Re: [PATCH v3 03/10] parse-options.[ch]: consistently use \"enum parse_opt_result\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-11-06T21:31:33Z","receivedAt":"2021-11-06T21:33:01Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Sat, Nov 06 2021, SZEDER Gábor wrote:\n\n> On Fri, Oct 08, 2021 at 09:07:39PM +0200, Ævar Arnfjörð Bjarmason wrote:\n>> Use the \"enum parse_opt_result\" instead of an \"int flags\" as the\n>> return value of the applicable functions in parse-options.c.\n>> \n>> This will help catch future bugs, such as the missing \"case\" arms in\n>> the two existing users of the API in \"blame.c\" and \"shortlog.c\". A\n>> third caller in 309be813c9b (update-index: migrate to parse-options\n>> API, 2010-12-01) was already checking for these.\n>> \n>> As can be seen when trying to sort through the deluge of warnings\n>> produced when compiling this with CC=g++ (mostly unrelated to this\n>> change) we're not consistently using \"enum parse_opt_result\" even now,\n>> i.e. we'll return error() and \"return 0;\". See f41179f16ba\n>> (parse-options: avoid magic return codes, 2019-01-27) for a commit\n>> which started changing some of that.\n>> \n>> I'm not doing any more of that exhaustive migration here, and it's\n>> probably not worthwhile past the point of being able to check \"enum\n>> parse_opt_result\" in switch().\n>> \n>> Signed-off-by: Ævar Arnfjörð Bjarmason \n>> ---\n>\n>> diff --git a/parse-options.c b/parse-options.c\n>> index 9c8ba963400..f718242096c 100644\n>> --- a/parse-options.c\n>> +++ b/parse-options.c\n>> @@ -699,13 +699,14 @@ static void free_preprocessed_options(struct option *options)\n>> \tfree(options);\n>> }\n>> \n>> -static int usage_with_options_internal(struct parse_opt_ctx_t *,\n>> -\t\t\t\t const char * const *,\n>> -\t\t\t\t const struct option *, int, int);\n>> -\n>> -int parse_options_step(struct parse_opt_ctx_t *ctx,\n>> -\t\t const struct option *options,\n>> -\t\t const char * const usagestr[])\n>> +static enum parse_opt_result usage_with_options_internal(struct parse_opt_ctx_t *,\n>> +\t\t\t\t\t\t\t const char * const *,\n>> +\t\t\t\t\t\t\t const struct option *,\n>> +\t\t\t\t\t\t\t int, int);\n>> +\n>> +enum parse_opt_result parse_options_step(struct parse_opt_ctx_t *ctx,\n>> +\t\t\t\t\t const struct option *options,\n>> +\t\t\t\t\t const char * const usagestr[])\n>> {\n>> \tint internal_help = !(ctx->flags & PARSE_OPT_NO_INTERNAL_HELP);\n>> \n>> @@ -839,10 +840,11 @@ int parse_options_end(struct parse_opt_ctx_t *ctx)\n>> \treturn ctx->cpidx + ctx->argc;\n>> }\n>> \n>> -int parse_options(int argc, const char **argv, const char *prefix,\n>> -\t\t const struct option *options,\n>> -\t\t const char * const usagestr[],\n>> -\t\t enum parse_opt_flags flags)\n>> +enum parse_opt_result parse_options(int argc, const char **argv,\n>\n> The return type of this function should have been left unchanged,\n> because it contains only one return statement:\n>\n> return parse_options_end(&ctx);\n>\n> and parse_options_end() does return an int.\n\nIndeed. I'll submit a fix for that sooner than later. I think I got that\nand *_step() mixed up, parse_options() just returns \"here's what we've\ngot left in argc\".\n\nI think due to C's happy-go-lucky enum-as-int semantics I'll probably\nleave it until post-2.34, i.e. I don't think it can/will break anything\nat runtime, but should be fixed for sure.\n\nThanks for spotting!\n"},{"id":"440735","messageId":"cover-0.2-00000000000-20211109T110113Z-avarab@gmail.com","threadId":"56604","inReplyTo":"211106.86lf21ezqx.gmgdl@evledraar.gmail.com","subject":"[PATCH 0/2] parse-options.[ch]: enum fixup & enum nit","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-11-09T11:04:42Z","receivedAt":"2021-11-09T11:04:51Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"This tiny series fixes an issue new in v2.34.0-rc* where a recent\npatch of mine incorrectly changed a return type to an enum that wasn't\napplicable to that function. That fix is in 1/2. Thanks to SZEDER for\nspotting it!\n\nJunio: As noted in [1] I don't think this needs to be integrated\nbefore the release, but if you wanted to pick it up anyway I think\nit's safe. Maybe someone's compiler will complain about the pre-image.\n\n2/2 then marks a static function I missed with an enum, which it was\nalready always returning except in an error case.\n\n1. https://lore.kernel.org/git/211106.86lf21ezqx.gmgdl@evledraar.gmail.com/\n\nÆvar Arnfjörð Bjarmason (2):\n parse-options.[ch]: revert use of \"enum\" for parse_options()\n parse-options.c: use \"enum parse_opt_result\" for parse_nodash_opt()\n\n parse-options.c | 17 +++++++++--------\n parse-options.h | 9 ++++-----\n 2 files changed, 13 insertions(+), 13 deletions(-)\n\n-- \n2.34.0.rc1.741.gab7bfd97031\n\n"},{"id":"440736","messageId":"patch-1.2-057a9f81b47-20211109T110113Z-avarab@gmail.com","threadId":"56604","inReplyTo":"cover-0.2-00000000000-20211109T110113Z-avarab@gmail.com","subject":"[PATCH 1/2] parse-options.[ch]: revert use of \"enum\" for parse_options()","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-11-09T11:04:43Z","receivedAt":"2021-11-09T11:04:52Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Revert the parse_options() prototype change in my recent\n352e761388b (parse-options.[ch]: consistently use \"enum\nparse_opt_result\", 2021-10-08) was incorrect. The parse_options()\nfunction returns the number of argc elements that haven't been\nprocessed, not \"enum parse_opt_result\".\n\nReported-by: SZEDER Gábor \nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n parse-options.c | 10 +++++-----\n parse-options.h | 9 ++++-----\n 2 files changed, 9 insertions(+), 10 deletions(-)\n\ndiff --git a/parse-options.c b/parse-options.c\nindex 9a0484c8831..fc5b43ff0b2 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -860,11 +860,11 @@ int parse_options_end(struct parse_opt_ctx_t *ctx)\n \treturn ctx->cpidx + ctx->argc;\n }\n \n-enum parse_opt_result parse_options(int argc, const char **argv,\n-\t\t\t\t const char *prefix,\n-\t\t\t\t const struct option *options,\n-\t\t\t\t const char * const usagestr[],\n-\t\t\t\t enum parse_opt_flags flags)\n+int parse_options(int argc, const char **argv,\n+\t\t const char *prefix,\n+\t\t const struct option *options,\n+\t\t const char * const usagestr[],\n+\t\t enum parse_opt_flags flags)\n {\n \tstruct parse_opt_ctx_t ctx;\n \tstruct option *real_options;\ndiff --git a/parse-options.h b/parse-options.h\nindex bdea052c399..275fb440818 100644\n--- a/parse-options.h\n+++ b/parse-options.h\n@@ -213,11 +213,10 @@ struct option {\n * untouched and parse_options() returns the number of options\n * processed.\n */\n-enum parse_opt_result parse_options(int argc, const char **argv,\n-\t\t\t\t const char *prefix,\n-\t\t\t\t const struct option *options,\n-\t\t\t\t const char * const usagestr[],\n-\t\t\t\t enum parse_opt_flags flags);\n+int parse_options(int argc, const char **argv, const char *prefix,\n+\t\t const struct option *options,\n+\t\t const char * const usagestr[],\n+\t\t enum parse_opt_flags flags);\n \n NORETURN void usage_with_options(const char * const *usagestr,\n \t\t\t\t const struct option *options);\n-- \n2.34.0.rc1.741.gab7bfd97031\n\n"},{"id":"440737","messageId":"patch-2.2-aa6224b10f8-20211109T110113Z-avarab@gmail.com","threadId":"56604","inReplyTo":"cover-0.2-00000000000-20211109T110113Z-avarab@gmail.com","subject":"[PATCH 2/2] parse-options.c: use \"enum parse_opt_result\" for parse_nodash_opt()","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-11-09T11:04:44Z","receivedAt":"2021-11-09T11:04:54Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Change the parse_nodash_opt() function to use \"enum\nparse_opt_result\". In 352e761388b (parse-options.[ch]: consistently\nuse \"enum parse_opt_result\", 2021-10-08) its only caller\nparse_options_step() started using that return type, and the\nget_value() which will be called and return from it uses the same\nenum.\n\nLet's do the same here so that this function always returns an \"enum\nparse_opt_result\" value.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n parse-options.c | 7 ++++---\n 1 file changed, 4 insertions(+), 3 deletions(-)\n\ndiff --git a/parse-options.c b/parse-options.c\nindex fc5b43ff0b2..629e7963497 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -404,8 +404,9 @@ static enum parse_opt_result parse_long_opt(\n \treturn PARSE_OPT_UNKNOWN;\n }\n \n-static int parse_nodash_opt(struct parse_opt_ctx_t *p, const char *arg,\n-\t\t\t const struct option *options)\n+static enum parse_opt_result parse_nodash_opt(struct parse_opt_ctx_t *p,\n+\t\t\t\t\t const char *arg,\n+\t\t\t\t\t const struct option *options)\n {\n \tconst struct option *all_opts = options;\n \n@@ -415,7 +416,7 @@ static int parse_nodash_opt(struct parse_opt_ctx_t *p, const char *arg,\n \t\tif (options->short_name == arg[0] && arg[1] == '\\0')\n \t\t\treturn get_value(p, options, all_opts, OPT_SHORT);\n \t}\n-\treturn -2;\n+\treturn PARSE_OPT_ERROR;\n }\n \n static void check_typos(const char *arg, const struct option *options)\n-- \n2.34.0.rc1.741.gab7bfd97031\n\n"},{"id":"440755","messageId":"xmqqsfw52pg4.fsf@gitster.g","threadId":"56604","inReplyTo":"patch-1.2-057a9f81b47-20211109T110113Z-avarab@gmail.com","subject":"Re: [PATCH 1/2] parse-options.[ch]: revert use of \"enum\" for parse_options()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-11-09T17:45:15Z","receivedAt":"2021-11-09T17:45:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> Revert the parse_options() prototype change in my recent\n> 352e761388b (parse-options.[ch]: consistently use \"enum\n> parse_opt_result\", 2021-10-08) was incorrect. The parse_options()\n> function returns the number of argc elements that haven't been\n> processed, not \"enum parse_opt_result\".\n>\n> Reported-by: SZEDER Gábor \n> Signed-off-by: Ævar Arnfjörð Bjarmason \n> ---\n> parse-options.c | 10 +++++-----\n> parse-options.h | 9 ++++-----\n> 2 files changed, 9 insertions(+), 10 deletions(-)\n\nYes, this makes total sense. We went overboard during the series.\n\n> diff --git a/parse-options.c b/parse-options.c\n> index 9a0484c8831..fc5b43ff0b2 100644\n> --- a/parse-options.c\n> +++ b/parse-options.c\n> @@ -860,11 +860,11 @@ int parse_options_end(struct parse_opt_ctx_t *ctx)\n> \treturn ctx->cpidx + ctx->argc;\n> }\n> \n> -enum parse_opt_result parse_options(int argc, const char **argv,\n> -\t\t\t\t const char *prefix,\n> -\t\t\t\t const struct option *options,\n> -\t\t\t\t const char * const usagestr[],\n> -\t\t\t\t enum parse_opt_flags flags)\n> +int parse_options(int argc, const char **argv,\n> +\t\t const char *prefix,\n> +\t\t const struct option *options,\n> +\t\t const char * const usagestr[],\n> +\t\t enum parse_opt_flags flags)\n> {\n> \tstruct parse_opt_ctx_t ctx;\n> \tstruct option *real_options;\n> diff --git a/parse-options.h b/parse-options.h\n> index bdea052c399..275fb440818 100644\n> --- a/parse-options.h\n> +++ b/parse-options.h\n> @@ -213,11 +213,10 @@ struct option {\n> * untouched and parse_options() returns the number of options\n> * processed.\n> */\n> -enum parse_opt_result parse_options(int argc, const char **argv,\n> -\t\t\t\t const char *prefix,\n> -\t\t\t\t const struct option *options,\n> -\t\t\t\t const char * const usagestr[],\n> -\t\t\t\t enum parse_opt_flags flags);\n> +int parse_options(int argc, const char **argv, const char *prefix,\n> +\t\t const struct option *options,\n> +\t\t const char * const usagestr[],\n> +\t\t enum parse_opt_flags flags);\n> \n> NORETURN void usage_with_options(const char * const *usagestr,\n> \t\t\t\t const struct option *options);\n"},{"id":"440756","messageId":"xmqqo86t2oud.fsf@gitster.g","threadId":"56604","inReplyTo":"patch-2.2-aa6224b10f8-20211109T110113Z-avarab@gmail.com","subject":"Re: [PATCH 2/2] parse-options.c: use \"enum parse_opt_result\" for parse_nodash_opt()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-11-09T17:58:18Z","receivedAt":"2021-11-09T17:58:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> Change the parse_nodash_opt() function to use \"enum\n> parse_opt_result\". In 352e761388b (parse-options.[ch]: consistently\n> use \"enum parse_opt_result\", 2021-10-08) its only caller\n> parse_options_step() started using that return type, and the\n> get_value() which will be called and return from it uses the same\n> enum.\n>\n> Let's do the same here so that this function always returns an \"enum\n> parse_opt_result\" value.\n>\n> Signed-off-by: Ævar Arnfjörð Bjarmason \n> ---\n> parse-options.c | 7 ++++---\n> 1 file changed, 4 insertions(+), 3 deletions(-)\n>\n> diff --git a/parse-options.c b/parse-options.c\n> index fc5b43ff0b2..629e7963497 100644\n> --- a/parse-options.c\n> +++ b/parse-options.c\n> @@ -404,8 +404,9 @@ static enum parse_opt_result parse_long_opt(\n> \treturn PARSE_OPT_UNKNOWN;\n> }\n> \n> -static int parse_nodash_opt(struct parse_opt_ctx_t *p, const char *arg,\n> -\t\t\t const struct option *options)\n> +static enum parse_opt_result parse_nodash_opt(struct parse_opt_ctx_t *p,\n> +\t\t\t\t\t const char *arg,\n> +\t\t\t\t\t const struct option *options)\n> {\n> \tconst struct option *all_opts = options;\n> \n> @@ -415,7 +416,7 @@ static int parse_nodash_opt(struct parse_opt_ctx_t *p, const char *arg,\n> \t\tif (options->short_name == arg[0] && arg[1] == '\\0')\n> \t\t\treturn get_value(p, options, all_opts, OPT_SHORT);\n> \t}\n> -\treturn -2;\n> +\treturn PARSE_OPT_ERROR;\n> }\n\nThe current caller only checks to skip a token that yields 0 (aka\nPARSE_OPT_DONE) and does not distinguish between other values, so\nthis won't change the behaviour of the current code, but it is \nnot clear if returning -1 (aka PARSE_OPT_ERROR) is better than -2\n(aka PARSE_OPT_HELP).\n\n"},{"id":"440782","messageId":"211110.867ddgkjcp.gmgdl@evledraar.gmail.com","threadId":"56604","inReplyTo":"xmqqo86t2oud.fsf@gitster.g","subject":"Re: [PATCH 2/2] parse-options.c: use \"enum parse_opt_result\" for parse_nodash_opt()","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-11-09T23:18:24Z","receivedAt":"2021-11-09T23:19:38Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Nov 09 2021, Junio C Hamano wrote:\n\n> Ævar Arnfjörð Bjarmason writes:\n>\n>> Change the parse_nodash_opt() function to use \"enum\n>> parse_opt_result\". In 352e761388b (parse-options.[ch]: consistently\n>> use \"enum parse_opt_result\", 2021-10-08) its only caller\n>> parse_options_step() started using that return type, and the\n>> get_value() which will be called and return from it uses the same\n>> enum.\n>>\n>> Let's do the same here so that this function always returns an \"enum\n>> parse_opt_result\" value.\n>>\n>> Signed-off-by: Ævar Arnfjörð Bjarmason \n>> ---\n>> parse-options.c | 7 ++++---\n>> 1 file changed, 4 insertions(+), 3 deletions(-)\n>>\n>> diff --git a/parse-options.c b/parse-options.c\n>> index fc5b43ff0b2..629e7963497 100644\n>> --- a/parse-options.c\n>> +++ b/parse-options.c\n>> @@ -404,8 +404,9 @@ static enum parse_opt_result parse_long_opt(\n>> \treturn PARSE_OPT_UNKNOWN;\n>> }\n>> \n>> -static int parse_nodash_opt(struct parse_opt_ctx_t *p, const char *arg,\n>> -\t\t\t const struct option *options)\n>> +static enum parse_opt_result parse_nodash_opt(struct parse_opt_ctx_t *p,\n>> +\t\t\t\t\t const char *arg,\n>> +\t\t\t\t\t const struct option *options)\n>> {\n>> \tconst struct option *all_opts = options;\n>> \n>> @@ -415,7 +416,7 @@ static int parse_nodash_opt(struct parse_opt_ctx_t *p, const char *arg,\n>> \t\tif (options->short_name == arg[0] && arg[1] == '\\0')\n>> \t\t\treturn get_value(p, options, all_opts, OPT_SHORT);\n>> \t}\n>> -\treturn -2;\n>> +\treturn PARSE_OPT_ERROR;\n>> }\n>\n> The current caller only checks to skip a token that yields 0 (aka\n> PARSE_OPT_DONE) and does not distinguish between other values, so\n> this won't change the behaviour of the current code, but it is \n> not clear if returning -1 (aka PARSE_OPT_ERROR) is better than -2\n> (aka PARSE_OPT_HELP).\n\nI think PARSE_OPT_ERROR is probably better.\n\nIt looks like the -2 return value might have been somewhat blindly\ncopy/pasted between 07fe54db3cd (parse-opt: do not print errors on\nunknown options, return -2 intead., 2008-06-23) and 51a9949eda7\n(parseopt: add PARSE_OPT_NODASH, 2009-05-07).\n\nI.e. we use the full enum values for the code in the former, but in the\nlatter we're just looking for \"not zero\", so error/-1 seemed like a\nbetter fit.\n\n"},{"id":"440787","messageId":"xmqqa6iczysb.fsf@gitster.g","threadId":"56604","inReplyTo":"211110.867ddgkjcp.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH 2/2] parse-options.c: use \"enum parse_opt_result\" for parse_nodash_opt()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-11-09T23:37:08Z","receivedAt":"2021-11-09T23:37:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n>>> -\treturn -2;\n>>> +\treturn PARSE_OPT_ERROR;\n>>> }\n>>\n>> The current caller only checks to skip a token that yields 0 (aka\n>> PARSE_OPT_DONE) and does not distinguish between other values, so\n>> this won't change the behaviour of the current code, but it is \n>> not clear if returning -1 (aka PARSE_OPT_ERROR) is better than -2\n>> (aka PARSE_OPT_HELP).\n>\n> I think PARSE_OPT_ERROR is probably better.\n>\n> It looks like the -2 return value might have been somewhat blindly\n> copy/pasted between 07fe54db3cd (parse-opt: do not print errors on\n> unknown options, return -2 intead., 2008-06-23) and 51a9949eda7\n> (parseopt: add PARSE_OPT_NODASH, 2009-05-07).\n>\n> I.e. we use the full enum values for the code in the former, but in the\n> latter we're just looking for \"not zero\", so error/-1 seemed like a\n> better fit.\n\nOK. That sounds like a good explanation for the change, to be\nrecorded in the proposed log message.\n\nThanks.\n"},{"id":"440793","messageId":"patch-v2-1.1-376f76bb44e-20211110T012523Z-avarab@gmail.com","threadId":"56604","inReplyTo":"211110.867ddgkjcp.gmgdl@evledraar.gmail.com","subject":"[PATCH v2] parse-options.c: use \"enum parse_opt_result\" for parse_nodash_opt()","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-11-10T01:27:04Z","receivedAt":"2021-11-10T01:27:10Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Change the parse_nodash_opt() function to use \"enum\nparse_opt_result\".In 352e761388b (parse-options.[ch]: consistently\nuse \"enum parse_opt_result\", 2021-10-08) its only caller\nparse_options_step() started using that return type, and the\nget_value() which will be called and return from it uses the same\nenum.\n\nLet's do the same here so that this function always returns an \"enum\nparse_opt_result\" value.\n\nWe could go for either PARSE_OPT_HELP (-2) or PARSE_OPT_ERROR (-1)\nhere. The reason we ended up with \"-2\" is that in code added in\n07fe54db3cd (parse-opt: do not print errors on unknown options, return\n\"-2\" instead., 2008-06-23) we used that value in a meaningful way.\n\nThen in 51a9949eda7 (parseopt: add PARSE_OPT_NODASH, 2009-05-07) the\nuse of \"-2\" was seemingly copy/pasted from parse_long_opt(), which was\nthe function immediately above the parse_nodash_opt() function added\nin that commit.\n\nSince we only care about whether the return value here is non-zero\nlet's use the more generic PARSE_OPT_ERROR.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n\nNow with an updated commit message per the v1 discussion. I peeled off\nthe 1/2 patch as it's already on \"master\" as 06a199f38b5\n(parse-options.[ch]: revert use of \"enum\" for parse_options(),\n2021-11-09).\n\nRange-diff against v1:\n1: 057a9f81b47 < -: ----------- parse-options.[ch]: revert use of \"enum\" for parse_options()\n2: aa6224b10f8 ! 1: 376f76bb44e parse-options.c: use \"enum parse_opt_result\" for parse_nodash_opt()\n @@ Commit message\n parse-options.c: use \"enum parse_opt_result\" for parse_nodash_opt()\n \n Change the parse_nodash_opt() function to use \"enum\n - parse_opt_result\". In 352e761388b (parse-options.[ch]: consistently\n + parse_opt_result\".In 352e761388b (parse-options.[ch]: consistently\n use \"enum parse_opt_result\", 2021-10-08) its only caller\n parse_options_step() started using that return type, and the\n get_value() which will be called and return from it uses the same\n @@ Commit message\n Let's do the same here so that this function always returns an \"enum\n parse_opt_result\" value.\n \n + We could go for either PARSE_OPT_HELP (-2) or PARSE_OPT_ERROR (-1)\n + here. The reason we ended up with \"-2\" is that in code added in\n + 07fe54db3cd (parse-opt: do not print errors on unknown options, return\n + \"-2\" instead., 2008-06-23) we used that value in a meaningful way.\n +\n + Then in 51a9949eda7 (parseopt: add PARSE_OPT_NODASH, 2009-05-07) the\n + use of \"-2\" was seemingly copy/pasted from parse_long_opt(), which was\n + the function immediately above the parse_nodash_opt() function added\n + in that commit.\n +\n + Since we only care about whether the return value here is non-zero\n + let's use the more generic PARSE_OPT_ERROR.\n +\n Signed-off-by: Ævar Arnfjörð Bjarmason \n \n ## parse-options.c ##\n\n parse-options.c | 7 ++++---\n 1 file changed, 4 insertions(+), 3 deletions(-)\n\ndiff --git a/parse-options.c b/parse-options.c\nindex fc5b43ff0b2..629e7963497 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -404,8 +404,9 @@ static enum parse_opt_result parse_long_opt(\n \treturn PARSE_OPT_UNKNOWN;\n }\n \n-static int parse_nodash_opt(struct parse_opt_ctx_t *p, const char *arg,\n-\t\t\t const struct option *options)\n+static enum parse_opt_result parse_nodash_opt(struct parse_opt_ctx_t *p,\n+\t\t\t\t\t const char *arg,\n+\t\t\t\t\t const struct option *options)\n {\n \tconst struct option *all_opts = options;\n \n@@ -415,7 +416,7 @@ static int parse_nodash_opt(struct parse_opt_ctx_t *p, const char *arg,\n \t\tif (options->short_name == arg[0] && arg[1] == '\\0')\n \t\t\treturn get_value(p, options, all_opts, OPT_SHORT);\n \t}\n-\treturn -2;\n+\treturn PARSE_OPT_ERROR;\n }\n \n static void check_typos(const char *arg, const struct option *options)\n-- \n2.34.0.rc1.741.gab7bfd97031\n\n"},{"id":"440896","messageId":"xmqqilwzv4ak.fsf@gitster.g","threadId":"56604","inReplyTo":"patch-v2-1.1-376f76bb44e-20211110T012523Z-avarab@gmail.com","subject":"Re: [PATCH v2] parse-options.c: use \"enum parse_opt_result\" for parse_nodash_opt()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-11-11T02:01:39Z","receivedAt":"2021-11-11T02:01:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> Change the parse_nodash_opt() function to use \"enum\n> parse_opt_result\".In 352e761388b (parse-options.[ch]: consistently\n\ns/.In/. In/; no need to resend for this.\n\n> use \"enum parse_opt_result\", 2021-10-08) its only caller\n> parse_options_step() started using that return type, and the\n> get_value() which will be called and return from it uses the same\n> enum.\n> ...\n> Since we only care about whether the return value here is non-zero\n> let's use the more generic PARSE_OPT_ERROR.\n\nI do not see a reason why anybody may think that it is a sensible\nthing for \"this returns a value from an enum, not int, so make it\nso\" patch, which is not supposed to change the sematics, to do,\nthough. It is even so since we know the current caller does not\ncare. The need of the next caller may tell us what the reasonable\nreturn value should be, but we do not know well enough to justify\nchanging the value.\n\n\n\n"}]}