{"thread":{"id":"58124","subject":"[PATCH] multi-pack-index: simplify handling of unknown --options","startedAt":"2022-07-08T20:28:57Z","lastAt":"2022-07-10T21:55:02Z","messageCount":5,"participants":["SZEDER Gábor","Junio C Hamano","Ævar Arnfjörð Bjarmason"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"458661","messageId":"20220708202847.662319-1-szeder.dev@gmail.com","threadId":"58124","inReplyTo":null,"subject":"[PATCH] multi-pack-index: simplify handling of unknown --options","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2022-07-08T20:28:47Z","receivedAt":"2022-07-08T20:28:57Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"Although parse_options() can handle unknown --options just fine, none\nof 'git multi-pack-index's subcommands rely on it, but do it on their\nown: they invoke parse_options() with the PARSE_OPT_KEEP_UNKNOWN flag,\nthen check whether there are any unparsed arguments left, and print\nusage and quit if necessary.\n\nLet parse_options() handle unknown options instead, which, besides\nsimpler code, has the additional benefit that it prints not only the\nusage but an \"error: unknown option `foo'\" message as well.\n\nSigned-off-by: SZEDER Gábor <szeder.dev@gmail.com>\n---\n builtin/multi-pack-index.c | 20 ++++----------------\n 1 file changed, 4 insertions(+), 16 deletions(-)\n\ndiff --git a/builtin/multi-pack-index.c b/builtin/multi-pack-index.c\nindex 5edbb7fe86..97a87ad8cb 100644\n--- a/builtin/multi-pack-index.c\n+++ b/builtin/multi-pack-index.c\n@@ -134,10 +134,7 @@ static int cmd_multi_pack_index_write(int argc, const char **argv)\n \t\topts.flags |= MIDX_PROGRESS;\n \targc = parse_options(argc, argv, NULL,\n \t\t\t     options, builtin_multi_pack_index_write_usage,\n-\t\t\t     PARSE_OPT_KEEP_UNKNOWN);\n-\tif (argc)\n-\t\tusage_with_options(builtin_multi_pack_index_write_usage,\n-\t\t\t\t   options);\n+\t\t\t     0);\n \n \tFREE_AND_NULL(options);\n \n@@ -176,10 +173,7 @@ static int cmd_multi_pack_index_verify(int argc, const char **argv)\n \t\topts.flags |= MIDX_PROGRESS;\n \targc = parse_options(argc, argv, NULL,\n \t\t\t     options, builtin_multi_pack_index_verify_usage,\n-\t\t\t     PARSE_OPT_KEEP_UNKNOWN);\n-\tif (argc)\n-\t\tusage_with_options(builtin_multi_pack_index_verify_usage,\n-\t\t\t\t   options);\n+\t\t\t     0);\n \n \tFREE_AND_NULL(options);\n \n@@ -202,10 +196,7 @@ static int cmd_multi_pack_index_expire(int argc, const char **argv)\n \t\topts.flags |= MIDX_PROGRESS;\n \targc = parse_options(argc, argv, NULL,\n \t\t\t     options, builtin_multi_pack_index_expire_usage,\n-\t\t\t     PARSE_OPT_KEEP_UNKNOWN);\n-\tif (argc)\n-\t\tusage_with_options(builtin_multi_pack_index_expire_usage,\n-\t\t\t\t   options);\n+\t\t\t     0);\n \n \tFREE_AND_NULL(options);\n \n@@ -232,10 +223,7 @@ static int cmd_multi_pack_index_repack(int argc, const char **argv)\n \targc = parse_options(argc, argv, NULL,\n \t\t\t     options,\n \t\t\t     builtin_multi_pack_index_repack_usage,\n-\t\t\t     PARSE_OPT_KEEP_UNKNOWN);\n-\tif (argc)\n-\t\tusage_with_options(builtin_multi_pack_index_repack_usage,\n-\t\t\t\t   options);\n+\t\t\t     0);\n \n \tFREE_AND_NULL(options);\n \n-- \n2.37.0.340.g5e8d960d32\n\n"},{"id":"458664","messageId":"xmqqh73ruxc8.fsf@gitster.g","threadId":"58124","inReplyTo":"20220708202847.662319-1-szeder.dev@gmail.com","subject":"Re: [PATCH] multi-pack-index: simplify handling of unknown --options","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-07-08T21:08:07Z","receivedAt":"2022-07-08T21:08:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"SZEDER Gábor <szeder.dev@gmail.com> writes:\n\n> Although parse_options() can handle unknown --options just fine, none\n> of 'git multi-pack-index's subcommands rely on it, but do it on their\n> own: they invoke parse_options() with the PARSE_OPT_KEEP_UNKNOWN flag,\n> then check whether there are any unparsed arguments left, and print\n> usage and quit if necessary.\n\nThe existing code check if there are any unparsed arguments or\noptions.  \n\nOmitting PARSE_OPT_KEEP_UNKNOWN allows parse_options() to deal with\nunknown options by complaining, but it happily leaves non-options on\nthe command line and reports how many of them there are.\n\nDoesn't this patch make\n\n\t$ git multi-pack-index write what-is-this-extra-arg-doing-here\n\nsilently ignore the extra argument instead of barfing on it?\n\n> Let parse_options() handle unknown options instead, which, besides\n> simpler code, has the additional benefit that it prints not only the\n> usage but an \"error: unknown option `foo'\" message as well.\n\nYes, I agree that getting rid of KEEP_UNKNOWN is a very good idea\nfor this reason.  But I suspect that we still need the \"did we get\nan extra argument we do not know what to do with?\" check.\n\nThanks.\n\n> Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>\n> ---\n>  builtin/multi-pack-index.c | 20 ++++----------------\n>  1 file changed, 4 insertions(+), 16 deletions(-)\n>\n> diff --git a/builtin/multi-pack-index.c b/builtin/multi-pack-index.c\n> index 5edbb7fe86..97a87ad8cb 100644\n> --- a/builtin/multi-pack-index.c\n> +++ b/builtin/multi-pack-index.c\n> @@ -134,10 +134,7 @@ static int cmd_multi_pack_index_write(int argc, const char **argv)\n>  \t\topts.flags |= MIDX_PROGRESS;\n>  \targc = parse_options(argc, argv, NULL,\n>  \t\t\t     options, builtin_multi_pack_index_write_usage,\n> -\t\t\t     PARSE_OPT_KEEP_UNKNOWN);\n> -\tif (argc)\n> -\t\tusage_with_options(builtin_multi_pack_index_write_usage,\n> -\t\t\t\t   options);\n> +\t\t\t     0);\n>  \n>  \tFREE_AND_NULL(options);\n>  \n> @@ -176,10 +173,7 @@ static int cmd_multi_pack_index_verify(int argc, const char **argv)\n>  \t\topts.flags |= MIDX_PROGRESS;\n>  \targc = parse_options(argc, argv, NULL,\n>  \t\t\t     options, builtin_multi_pack_index_verify_usage,\n> -\t\t\t     PARSE_OPT_KEEP_UNKNOWN);\n> -\tif (argc)\n> -\t\tusage_with_options(builtin_multi_pack_index_verify_usage,\n> -\t\t\t\t   options);\n> +\t\t\t     0);\n>  \n>  \tFREE_AND_NULL(options);\n>  \n> @@ -202,10 +196,7 @@ static int cmd_multi_pack_index_expire(int argc, const char **argv)\n>  \t\topts.flags |= MIDX_PROGRESS;\n>  \targc = parse_options(argc, argv, NULL,\n>  \t\t\t     options, builtin_multi_pack_index_expire_usage,\n> -\t\t\t     PARSE_OPT_KEEP_UNKNOWN);\n> -\tif (argc)\n> -\t\tusage_with_options(builtin_multi_pack_index_expire_usage,\n> -\t\t\t\t   options);\n> +\t\t\t     0);\n>  \n>  \tFREE_AND_NULL(options);\n>  \n> @@ -232,10 +223,7 @@ static int cmd_multi_pack_index_repack(int argc, const char **argv)\n>  \targc = parse_options(argc, argv, NULL,\n>  \t\t\t     options,\n>  \t\t\t     builtin_multi_pack_index_repack_usage,\n> -\t\t\t     PARSE_OPT_KEEP_UNKNOWN);\n> -\tif (argc)\n> -\t\tusage_with_options(builtin_multi_pack_index_repack_usage,\n> -\t\t\t\t   options);\n> +\t\t\t     0);\n>  \n>  \tFREE_AND_NULL(options);\n"},{"id":"458715","messageId":"20220710151645.GA2038@szeder.dev","threadId":"58124","inReplyTo":"xmqqh73ruxc8.fsf@gitster.g","subject":"Re: [PATCH] multi-pack-index: simplify handling of unknown --options","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2022-07-10T15:16:45Z","receivedAt":"2022-07-10T15:16:52Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Fri, Jul 08, 2022 at 02:08:07PM -0700, Junio C Hamano wrote:\n> SZEDER Gábor <szeder.dev@gmail.com> writes:\n> \n> > Although parse_options() can handle unknown --options just fine, none\n> > of 'git multi-pack-index's subcommands rely on it, but do it on their\n> > own: they invoke parse_options() with the PARSE_OPT_KEEP_UNKNOWN flag,\n> > then check whether there are any unparsed arguments left, and print\n> > usage and quit if necessary.\n> \n> The existing code check if there are any unparsed arguments or\n> options.  \n> \n> Omitting PARSE_OPT_KEEP_UNKNOWN allows parse_options() to deal with\n> unknown options by complaining, but it happily leaves non-options on\n> the command line and reports how many of them there are.\n> \n> Doesn't this patch make\n> \n> \t$ git multi-pack-index write what-is-this-extra-arg-doing-here\n> \n> silently ignore the extra argument instead of barfing on it?\n> \n> > Let parse_options() handle unknown options instead, which, besides\n> > simpler code, has the additional benefit that it prints not only the\n> > usage but an \"error: unknown option `foo'\" message as well.\n> \n> Yes, I agree that getting rid of KEEP_UNKNOWN is a very good idea\n> for this reason.  But I suspect that we still need the \"did we get\n> an extra argument we do not know what to do with?\" check.\n\nUh, indeed.  I got too trigger-happy with deleting lines.\nUpdated patch below.\n\n  ---  >8  ---\n\nSubject: multi-pack-index: simplify handling of unknown --options\n\nAlthough parse_options() can handle unknown --options just fine, none\nof 'git multi-pack-index's subcommands rely on it, but do it on their\nown: they invoke parse_options() with the PARSE_OPT_KEEP_UNKNOWN flag,\nthen check whether there are any unparsed arguments left, and print\nusage and quit if necessary.\n\nDrop that PARSE_OPT_KEEP_UNKNOWN flag to let parse_options() handle\nunknown options instead, which has the additional benefit that it\nprints not only the usage but an \"error: unknown option `foo'\" message\nas well.\n\nDo leave the unparsed arguments check to catch any unexpected\nnon-option arguments, though, e.g. 'git multi-pack-index write foo'.\n\nSigned-off-by: SZEDER Gábor <szeder.dev@gmail.com>\n---\n builtin/multi-pack-index.c | 8 ++++----\n 1 file changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/multi-pack-index.c b/builtin/multi-pack-index.c\nindex 5edbb7fe86..8f24d59a75 100644\n--- a/builtin/multi-pack-index.c\n+++ b/builtin/multi-pack-index.c\n@@ -134,7 +134,7 @@ static int cmd_multi_pack_index_write(int argc, const char **argv)\n \t\topts.flags |= MIDX_PROGRESS;\n \targc = parse_options(argc, argv, NULL,\n \t\t\t     options, builtin_multi_pack_index_write_usage,\n-\t\t\t     PARSE_OPT_KEEP_UNKNOWN);\n+\t\t\t     0);\n \tif (argc)\n \t\tusage_with_options(builtin_multi_pack_index_write_usage,\n \t\t\t\t   options);\n@@ -176,7 +176,7 @@ static int cmd_multi_pack_index_verify(int argc, const char **argv)\n \t\topts.flags |= MIDX_PROGRESS;\n \targc = parse_options(argc, argv, NULL,\n \t\t\t     options, builtin_multi_pack_index_verify_usage,\n-\t\t\t     PARSE_OPT_KEEP_UNKNOWN);\n+\t\t\t     0);\n \tif (argc)\n \t\tusage_with_options(builtin_multi_pack_index_verify_usage,\n \t\t\t\t   options);\n@@ -202,7 +202,7 @@ static int cmd_multi_pack_index_expire(int argc, const char **argv)\n \t\topts.flags |= MIDX_PROGRESS;\n \targc = parse_options(argc, argv, NULL,\n \t\t\t     options, builtin_multi_pack_index_expire_usage,\n-\t\t\t     PARSE_OPT_KEEP_UNKNOWN);\n+\t\t\t     0);\n \tif (argc)\n \t\tusage_with_options(builtin_multi_pack_index_expire_usage,\n \t\t\t\t   options);\n@@ -232,7 +232,7 @@ static int cmd_multi_pack_index_repack(int argc, const char **argv)\n \targc = parse_options(argc, argv, NULL,\n \t\t\t     options,\n \t\t\t     builtin_multi_pack_index_repack_usage,\n-\t\t\t     PARSE_OPT_KEEP_UNKNOWN);\n+\t\t\t     0);\n \tif (argc)\n \t\tusage_with_options(builtin_multi_pack_index_repack_usage,\n \t\t\t\t   options);\n-- \n2.37.0.340.g5e8d960d32\n\n"},{"id":"458718","messageId":"220710.86edyt7x0d.gmgdl@evledraar.gmail.com","threadId":"58124","inReplyTo":"20220710151645.GA2038@szeder.dev","subject":"Re: [PATCH] multi-pack-index: simplify handling of unknown --options","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-07-10T16:26:13Z","receivedAt":"2022-07-10T16:28:39Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Sun, Jul 10 2022, SZEDER Gábor wrote:\n\n> On Fri, Jul 08, 2022 at 02:08:07PM -0700, Junio C Hamano wrote:\n>> SZEDER Gábor <szeder.dev@gmail.com> writes:\n>> \n>> > Although parse_options() can handle unknown --options just fine, none\n>> > of 'git multi-pack-index's subcommands rely on it, but do it on their\n>> > own: they invoke parse_options() with the PARSE_OPT_KEEP_UNKNOWN flag,\n>> > then check whether there are any unparsed arguments left, and print\n>> > usage and quit if necessary.\n>> \n>> The existing code check if there are any unparsed arguments or\n>> options.  \n>> \n>> Omitting PARSE_OPT_KEEP_UNKNOWN allows parse_options() to deal with\n>> unknown options by complaining, but it happily leaves non-options on\n>> the command line and reports how many of them there are.\n>> \n>> Doesn't this patch make\n>> \n>> \t$ git multi-pack-index write what-is-this-extra-arg-doing-here\n>> \n>> silently ignore the extra argument instead of barfing on it?\n>> \n>> > Let parse_options() handle unknown options instead, which, besides\n>> > simpler code, has the additional benefit that it prints not only the\n>> > usage but an \"error: unknown option `foo'\" message as well.\n>> \n>> Yes, I agree that getting rid of KEEP_UNKNOWN is a very good idea\n>> for this reason.  But I suspect that we still need the \"did we get\n>> an extra argument we do not know what to do with?\" check.\n>\n> Uh, indeed.  I got too trigger-happy with deleting lines.\n> Updated patch below.\n>\n>   ---  >8  ---\n>\n> Subject: multi-pack-index: simplify handling of unknown --options\n>\n> Although parse_options() can handle unknown --options just fine, none\n> of 'git multi-pack-index's subcommands rely on it, but do it on their\n> own: they invoke parse_options() with the PARSE_OPT_KEEP_UNKNOWN flag,\n> then check whether there are any unparsed arguments left, and print\n> usage and quit if necessary.\n>\n> Drop that PARSE_OPT_KEEP_UNKNOWN flag to let parse_options() handle\n> unknown options instead, which has the additional benefit that it\n> prints not only the usage but an \"error: unknown option `foo'\" message\n> as well.\n>\n> Do leave the unparsed arguments check to catch any unexpected\n> non-option arguments, though, e.g. 'git multi-pack-index write foo'.\n>\n> Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>\n> ---\n>  builtin/multi-pack-index.c | 8 ++++----\n>  1 file changed, 4 insertions(+), 4 deletions(-)\n>\n> diff --git a/builtin/multi-pack-index.c b/builtin/multi-pack-index.c\n> index 5edbb7fe86..8f24d59a75 100644\n> --- a/builtin/multi-pack-index.c\n> +++ b/builtin/multi-pack-index.c\n> @@ -134,7 +134,7 @@ static int cmd_multi_pack_index_write(int argc, const char **argv)\n>  \t\topts.flags |= MIDX_PROGRESS;\n>  \targc = parse_options(argc, argv, NULL,\n>  \t\t\t     options, builtin_multi_pack_index_write_usage,\n> -\t\t\t     PARSE_OPT_KEEP_UNKNOWN);\n> +\t\t\t     0);\n>  \tif (argc)\n>  \t\tusage_with_options(builtin_multi_pack_index_write_usage,\n>  \t\t\t\t   options);\n> @@ -176,7 +176,7 @@ static int cmd_multi_pack_index_verify(int argc, const char **argv)\n>  \t\topts.flags |= MIDX_PROGRESS;\n>  \targc = parse_options(argc, argv, NULL,\n>  \t\t\t     options, builtin_multi_pack_index_verify_usage,\n> -\t\t\t     PARSE_OPT_KEEP_UNKNOWN);\n> +\t\t\t     0);\n>  \tif (argc)\n>  \t\tusage_with_options(builtin_multi_pack_index_verify_usage,\n>  \t\t\t\t   options);\n> @@ -202,7 +202,7 @@ static int cmd_multi_pack_index_expire(int argc, const char **argv)\n>  \t\topts.flags |= MIDX_PROGRESS;\n>  \targc = parse_options(argc, argv, NULL,\n>  \t\t\t     options, builtin_multi_pack_index_expire_usage,\n> -\t\t\t     PARSE_OPT_KEEP_UNKNOWN);\n> +\t\t\t     0);\n>  \tif (argc)\n>  \t\tusage_with_options(builtin_multi_pack_index_expire_usage,\n>  \t\t\t\t   options);\n> @@ -232,7 +232,7 @@ static int cmd_multi_pack_index_repack(int argc, const char **argv)\n>  \targc = parse_options(argc, argv, NULL,\n>  \t\t\t     options,\n>  \t\t\t     builtin_multi_pack_index_repack_usage,\n> -\t\t\t     PARSE_OPT_KEEP_UNKNOWN);\n> +\t\t\t     0);\n>  \tif (argc)\n>  \t\tusage_with_options(builtin_multi_pack_index_repack_usage,\n>  \t\t\t\t   options);\n\nLooks good, FWIW I've had this in my local tree for a long time (been\nmeaning to submit it), which manages to delete the \"argc\" handling for\nthis & others.\n\nBut I think just doing this is good for now (multi-pack-index isn't in\nthe diff below, but I did it in another commit):\nhttps://github.com/avar/git/commit/17b838ddd5a):\n\ndiff --git a/builtin/checkout--worker.c b/builtin/checkout--worker.c\nindex ede7dc32a43..e866057ebcc 100644\n--- a/builtin/checkout--worker.c\n+++ b/builtin/checkout--worker.c\n@@ -126,9 +126,8 @@ int cmd_checkout__worker(int argc, const char **argv, const char *prefix)\n \n \tgit_config(git_default_config, NULL);\n \targc = parse_options(argc, argv, prefix, checkout_worker_options,\n-\t\t\t     checkout_worker_usage, 0);\n-\tif (argc > 0)\n-\t\tusage_with_options(checkout_worker_usage, checkout_worker_options);\n+\t\t\t     checkout_worker_usage,\n+\t\t\t     PARSE_OPT_ERROR_AT_NON_OPTION);\n \n \tif (state.base_dir)\n \t\tstate.base_dir_len = strlen(state.base_dir);\ndiff --git a/builtin/column.c b/builtin/column.c\nindex 158fdf53d9f..a22b4532301 100644\n--- a/builtin/column.c\n+++ b/builtin/column.c\n@@ -43,9 +43,8 @@ int cmd_column(int argc, const char **argv, const char *prefix)\n \n \tmemset(&copts, 0, sizeof(copts));\n \tcopts.padding = 1;\n-\targc = parse_options(argc, argv, prefix, options, builtin_column_usage, 0);\n-\tif (argc)\n-\t\tusage_with_options(builtin_column_usage, options);\n+\tparse_options(argc, argv, prefix, options, builtin_column_usage,\n+\t\t      PARSE_OPT_ERROR_AT_NON_OPTION);\n \tif (real_command || command) {\n \t\tif (!real_command || !command || strcmp(real_command, command))\n \t\t\tdie(_(\"--command must be the first argument\"));\ndiff --git a/builtin/commit-graph.c b/builtin/commit-graph.c\nindex 51c4040ea6c..b92774c7fd3 100644\n--- a/builtin/commit-graph.c\n+++ b/builtin/commit-graph.c\n@@ -80,11 +80,9 @@ static int graph_verify(int argc, const char **argv)\n \ttrace2_cmd_mode(\"verify\");\n \n \topts.progress = isatty(2);\n-\targc = parse_options(argc, argv, NULL,\n-\t\t\t     options,\n-\t\t\t     builtin_commit_graph_verify_usage, 0);\n-\tif (argc)\n-\t\tusage_with_options(builtin_commit_graph_verify_usage, options);\n+\tparse_options(argc, argv, NULL, options,\n+\t\t      builtin_commit_graph_verify_usage,\n+\t\t      PARSE_OPT_ERROR_AT_NON_OPTION);\n \n \tif (!opts.obj_dir)\n \t\topts.obj_dir = get_object_directory();\n@@ -241,11 +239,9 @@ static int graph_write(int argc, const char **argv)\n \n \tgit_config(git_commit_graph_write_config, &opts);\n \n-\targc = parse_options(argc, argv, NULL,\n-\t\t\t     options,\n-\t\t\t     builtin_commit_graph_write_usage, 0);\n-\tif (argc)\n-\t\tusage_with_options(builtin_commit_graph_write_usage, options);\n+\tparse_options(argc, argv, NULL, options,\n+\t\t      builtin_commit_graph_write_usage,\n+\t\t      PARSE_OPT_ERROR_AT_NON_OPTION);\n \n \tif (opts.reachable + opts.stdin_packs + opts.stdin_commits > 1)\n \t\tdie(_(\"use at most one of --reachable, --stdin-commits, or --stdin-packs\"));\ndiff --git a/builtin/count-objects.c b/builtin/count-objects.c\nindex 07b94195962..4a7452f4b9f 100644\n--- a/builtin/count-objects.c\n+++ b/builtin/count-objects.c\n@@ -103,10 +103,8 @@ int cmd_count_objects(int argc, const char **argv, const char *prefix)\n \n \tgit_config(git_default_config, NULL);\n \n-\targc = parse_options(argc, argv, prefix, opts, count_objects_usage, 0);\n-\t/* we do not take arguments other than flags for now */\n-\tif (argc)\n-\t\tusage_with_options(count_objects_usage, opts);\n+\tparse_options(argc, argv, prefix, opts, count_objects_usage,\n+\t\t      PARSE_OPT_ERROR_AT_NON_OPTION);\n \tif (verbose) {\n \t\treport_garbage = real_report_garbage;\n \t\treport_linked_checkout_garbage();\ndiff --git a/builtin/fmt-merge-msg.c b/builtin/fmt-merge-msg.c\nindex f1a8290cba7..df6b210127b 100644\n--- a/builtin/fmt-merge-msg.c\n+++ b/builtin/fmt-merge-msg.c\n@@ -37,9 +37,7 @@ int cmd_fmt_merge_msg(int argc, const char **argv, const char *prefix)\n \n \tgit_config(fmt_merge_msg_config, NULL);\n \targc = parse_options(argc, argv, prefix, options, fmt_merge_msg_usage,\n-\t\t\t     0);\n-\tif (argc > 0)\n-\t\tusage_with_options(fmt_merge_msg_usage, options);\n+\t\t\t     PARSE_OPT_ERROR_AT_NON_OPTION);\n \tif (shortlog_len < 0)\n \t\tshortlog_len = (merge_log_config > 0) ? merge_log_config : 0;\n \ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex 021e9256ae2..d3201d8781f 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -587,9 +587,7 @@ int cmd_gc(int argc, const char **argv, const char *prefix)\n \t\tpack_refs = !is_bare_repository();\n \n \targc = parse_options(argc, argv, prefix, builtin_gc_options,\n-\t\t\t     builtin_gc_usage, 0);\n-\tif (argc > 0)\n-\t\tusage_with_options(builtin_gc_usage, builtin_gc_options);\n+\t\t\t     builtin_gc_usage, PARSE_OPT_ERROR_AT_NON_OPTION);\n \n \tif (prune_expire && parse_expiry_date(prune_expire, &dummy))\n \t\tdie(_(\"failed to parse prune expiry value %s\"), prune_expire);\n@@ -2491,10 +2489,9 @@ static int maintenance_start(int argc, const char **argv, const char *prefix)\n \t\tOPT_END()\n \t};\n \n-\targc = parse_options(argc, argv, prefix, options,\n-\t\t\t     builtin_maintenance_start_usage, 0);\n-\tif (argc)\n-\t\tusage_with_options(builtin_maintenance_start_usage, options);\n+\tparse_options(argc, argv, prefix, options,\n+\t\t      builtin_maintenance_start_usage,\n+\t\t      PARSE_OPT_ERROR_AT_NON_OPTION);\n \n \topts.scheduler = resolve_scheduler(opts.scheduler);\n \tvalidate_scheduler(opts.scheduler);\ndiff --git a/builtin/notes.c b/builtin/notes.c\nindex a3d0d15a227..35329411f50 100644\n--- a/builtin/notes.c\n+++ b/builtin/notes.c\n@@ -958,13 +958,8 @@ static int prune(int argc, const char **argv, const char *prefix)\n \t\tOPT_END()\n \t};\n \n-\targc = parse_options(argc, argv, prefix, options, git_notes_prune_usage,\n-\t\t\t     0);\n-\n-\tif (argc) {\n-\t\terror(_(\"too many arguments\"));\n-\t\tusage_with_options(git_notes_prune_usage, options);\n-\t}\n+\tparse_options(argc, argv, prefix, options, git_notes_prune_usage,\n+\t\t      PARSE_OPT_ERROR_AT_NON_OPTION);\n \n \tt = init_notes_check(\"prune\", NOTES_INIT_WRITABLE);\n \ndiff --git a/builtin/stripspace.c b/builtin/stripspace.c\nindex 1e34cf2bebd..39708db8b1b 100644\n--- a/builtin/stripspace.c\n+++ b/builtin/stripspace.c\n@@ -42,9 +42,8 @@ int cmd_stripspace(int argc, const char **argv, const char *prefix)\n \t\tOPT_END()\n \t};\n \n-\targc = parse_options(argc, argv, prefix, options, stripspace_usage, 0);\n-\tif (argc)\n-\t\tusage_with_options(stripspace_usage, options);\n+\tparse_options(argc, argv, prefix, options, stripspace_usage,\n+\t\t      PARSE_OPT_ERROR_AT_NON_OPTION);\n \n \tif (mode == STRIP_COMMENTS || mode == COMMENT_LINES) {\n \t\tsetup_git_directory_gently(&nongit);\ndiff --git a/builtin/worktree.c b/builtin/worktree.c\nindex cd62eef240e..29803379006 100644\n--- a/builtin/worktree.c\n+++ b/builtin/worktree.c\n@@ -153,9 +153,8 @@ static int prune(int ac, const char **av, const char *prefix)\n \t};\n \n \texpire = TIME_MAX;\n-\tac = parse_options(ac, av, prefix, options, worktree_usage, 0);\n-\tif (ac)\n-\t\tusage_with_options(worktree_usage, options);\n+\tparse_options(ac, av, prefix, options, worktree_usage,\n+\t\t      PARSE_OPT_ERROR_AT_NON_OPTION);\n \tprune_worktrees();\n \treturn 0;\n }\n@@ -772,10 +771,9 @@ static int list(int ac, const char **av, const char *prefix)\n \t};\n \n \texpire = TIME_MAX;\n-\tac = parse_options(ac, av, prefix, options, worktree_usage, 0);\n-\tif (ac)\n-\t\tusage_with_options(worktree_usage, options);\n-\telse if (verbose && porcelain)\n+\tparse_options(ac, av, prefix, options, worktree_usage,\n+\t\t      PARSE_OPT_ERROR_AT_NON_OPTION);\n+\tif (verbose && porcelain)\n \t\tdie(_(\"options '%s' and '%s' cannot be used together\"), \"--verbose\", \"--porcelain\");\n \telse if (!line_terminator && !porcelain)\n \t\tdie(_(\"the option '%s' requires '%s'\"), \"-z\", \"--porcelain\");\ndiff --git a/imap-send.c b/imap-send.c\nindex a50af56b827..f7c4efa3759 100644\n--- a/imap-send.c\n+++ b/imap-send.c\n@@ -1526,10 +1526,8 @@ int cmd_main(int argc, const char **argv)\n \tsetup_git_directory_gently(&nongit_ok);\n \tgit_config(git_imap_config, NULL);\n \n-\targc = parse_options(argc, (const char **)argv, \"\", imap_send_options, imap_send_usage, 0);\n-\n-\tif (argc)\n-\t\tusage_with_options(imap_send_usage, imap_send_options);\n+\tparse_options(argc, (const char **)argv, \"\", imap_send_options,\n+\t\t      imap_send_usage, PARSE_OPT_ERROR_AT_NON_OPTION);\n \n #ifndef USE_CURL_FOR_IMAP_SEND\n \tif (use_curl) {\ndiff --git a/parse-options.c b/parse-options.c\nindex 60477156720..368875bb32d 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -631,6 +631,8 @@ void parse_options_start(struct parse_opt_ctx_t *ctx,\n \tctx->out   = argv;\n \tctx->prefix = prefix;\n \tctx->cpidx = ((flags & PARSE_OPT_KEEP_ARGV0) != 0);\n+\tif (flags & PARSE_OPT_ERROR_AT_NON_OPTION)\n+\t\tflags |= PARSE_OPT_STOP_AT_NON_OPTION;\n \tctx->flags = flags;\n \tparse_options_alter_flags(ctx);\n \tparse_options_check_flags(options, ctx->flags);\n@@ -894,6 +896,11 @@ int parse_options(int argc, const char **argv,\n \tcase PARSE_OPT_COMPLETE:\n \t\texit(0);\n \tcase PARSE_OPT_NON_OPTION:\n+\t\tif (flags & PARSE_OPT_ERROR_AT_NON_OPTION) {\n+\t\t\terror(_(\"unknown non-option: `%s'\"), ctx.argv[0]);\n+\t\t\tusage_with_options(usagestr, options);\n+\t\t}\n+\t\tbreak;\n \tcase PARSE_OPT_DONE:\n \t\tbreak;\n \tcase PARSE_OPT_UNKNOWN:\ndiff --git a/parse-options.h b/parse-options.h\nindex 460afb93256..013e539d6cc 100644\n--- a/parse-options.h\n+++ b/parse-options.h\n@@ -34,6 +34,7 @@ enum parse_opt_flags {\n \tPARSE_OPT_SHELL_EVAL = 1 << 6,\n \tPARSE_OPT_REV_PARSE_PARSEOPT = 1 << 7,\n \tPARSE_OPT_INTERNAL_HELP_ON_ONE_ARG = 1 << 8,\n+\tPARSE_OPT_ERROR_AT_NON_OPTION = 1 << 9,\n };\n \n /**\ndiff --git a/t/helper/test-getcwd.c b/t/helper/test-getcwd.c\nindex d680038a780..b96bdc72c1e 100644\n--- a/t/helper/test-getcwd.c\n+++ b/t/helper/test-getcwd.c\n@@ -14,9 +14,8 @@ int cmd__getcwd(int argc, const char **argv)\n \t};\n \tchar *cwd;\n \n-\targc = parse_options(argc, argv, \"test-tools\", options, getcwd_usage, 0);\n-\tif (argc > 0)\n-\t\tusage_with_options(getcwd_usage, options);\n+\targc = parse_options(argc, argv, \"test-tools\", options, getcwd_usage,\n+\t\t\t     PARSE_OPT_ERROR_AT_NON_OPTION);\n \n \tcwd = xgetcwd();\n \tputs(cwd);\n"},{"id":"458733","messageId":"xmqq1qusr5u9.fsf@gitster.g","threadId":"58124","inReplyTo":"20220710151645.GA2038@szeder.dev","subject":"Re: [PATCH] multi-pack-index: simplify handling of unknown --options","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-07-10T21:54:54Z","receivedAt":"2022-07-10T21:55:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"SZEDER Gábor <szeder.dev@gmail.com> writes:\n\n>> > Let parse_options() handle unknown options instead, which, besides\n>> > simpler code, has the additional benefit that it prints not only the\n>> > usage but an \"error: unknown option `foo'\" message as well.\n>> \n>> Yes, I agree that getting rid of KEEP_UNKNOWN is a very good idea\n>> for this reason.  But I suspect that we still need the \"did we get\n>> an extra argument we do not know what to do with?\" check.\n>\n> Uh, indeed.  I got too trigger-happy with deleting lines.\n> Updated patch below.\n\nOK.  I suspect that a test would have caught the breakage in the\noriginal.  Would it make sense to add one now?\n\nThanks, will queue.\n"}]}