On Sun, Jul 10 2022, SZEDER Gábor wrote: > On Fri, Jul 08, 2022 at 02:08:07PM -0700, Junio C Hamano wrote: >> SZEDER Gábor <szeder.dev@xxxxxxxxx> writes: >> >> > Although parse_options() can handle unknown --options just fine, none >> > of 'git multi-pack-index's subcommands rely on it, but do it on their >> > own: they invoke parse_options() with the PARSE_OPT_KEEP_UNKNOWN flag, >> > then check whether there are any unparsed arguments left, and print >> > usage and quit if necessary. >> >> The existing code check if there are any unparsed arguments or >> options. >> >> Omitting PARSE_OPT_KEEP_UNKNOWN allows parse_options() to deal with >> unknown options by complaining, but it happily leaves non-options on >> the command line and reports how many of them there are. >> >> Doesn't this patch make >> >> $ git multi-pack-index write what-is-this-extra-arg-doing-here >> >> silently ignore the extra argument instead of barfing on it? >> >> > Let parse_options() handle unknown options instead, which, besides >> > simpler code, has the additional benefit that it prints not only the >> > usage but an "error: unknown option `foo'" message as well. >> >> Yes, I agree that getting rid of KEEP_UNKNOWN is a very good idea >> for this reason. But I suspect that we still need the "did we get >> an extra argument we do not know what to do with?" check. > > Uh, indeed. I got too trigger-happy with deleting lines. > Updated patch below. > > --- >8 --- > > Subject: multi-pack-index: simplify handling of unknown --options > > Although parse_options() can handle unknown --options just fine, none > of 'git multi-pack-index's subcommands rely on it, but do it on their > own: they invoke parse_options() with the PARSE_OPT_KEEP_UNKNOWN flag, > then check whether there are any unparsed arguments left, and print > usage and quit if necessary. > > Drop that PARSE_OPT_KEEP_UNKNOWN flag to let parse_options() handle > unknown options instead, which has the additional benefit that it > prints not only the usage but an "error: unknown option `foo'" message > as well. > > Do leave the unparsed arguments check to catch any unexpected > non-option arguments, though, e.g. 'git multi-pack-index write foo'. > > Signed-off-by: SZEDER Gábor <szeder.dev@xxxxxxxxx> > --- > builtin/multi-pack-index.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/builtin/multi-pack-index.c b/builtin/multi-pack-index.c > index 5edbb7fe86..8f24d59a75 100644 > --- a/builtin/multi-pack-index.c > +++ b/builtin/multi-pack-index.c > @@ -134,7 +134,7 @@ static int cmd_multi_pack_index_write(int argc, const char **argv) > opts.flags |= MIDX_PROGRESS; > argc = parse_options(argc, argv, NULL, > options, builtin_multi_pack_index_write_usage, > - PARSE_OPT_KEEP_UNKNOWN); > + 0); > if (argc) > usage_with_options(builtin_multi_pack_index_write_usage, > options); > @@ -176,7 +176,7 @@ static int cmd_multi_pack_index_verify(int argc, const char **argv) > opts.flags |= MIDX_PROGRESS; > argc = parse_options(argc, argv, NULL, > options, builtin_multi_pack_index_verify_usage, > - PARSE_OPT_KEEP_UNKNOWN); > + 0); > if (argc) > usage_with_options(builtin_multi_pack_index_verify_usage, > options); > @@ -202,7 +202,7 @@ static int cmd_multi_pack_index_expire(int argc, const char **argv) > opts.flags |= MIDX_PROGRESS; > argc = parse_options(argc, argv, NULL, > options, builtin_multi_pack_index_expire_usage, > - PARSE_OPT_KEEP_UNKNOWN); > + 0); > if (argc) > usage_with_options(builtin_multi_pack_index_expire_usage, > options); > @@ -232,7 +232,7 @@ static int cmd_multi_pack_index_repack(int argc, const char **argv) > argc = parse_options(argc, argv, NULL, > options, > builtin_multi_pack_index_repack_usage, > - PARSE_OPT_KEEP_UNKNOWN); > + 0); > if (argc) > usage_with_options(builtin_multi_pack_index_repack_usage, > options); Looks good, FWIW I've had this in my local tree for a long time (been meaning to submit it), which manages to delete the "argc" handling for this & others. But I think just doing this is good for now (multi-pack-index isn't in the diff below, but I did it in another commit): https://github.com/avar/git/commit/17b838ddd5a): diff --git a/builtin/checkout--worker.c b/builtin/checkout--worker.c index ede7dc32a43..e866057ebcc 100644 --- a/builtin/checkout--worker.c +++ b/builtin/checkout--worker.c @@ -126,9 +126,8 @@ int cmd_checkout__worker(int argc, const char **argv, const char *prefix) git_config(git_default_config, NULL); argc = parse_options(argc, argv, prefix, checkout_worker_options, - checkout_worker_usage, 0); - if (argc > 0) - usage_with_options(checkout_worker_usage, checkout_worker_options); + checkout_worker_usage, + PARSE_OPT_ERROR_AT_NON_OPTION); if (state.base_dir) state.base_dir_len = strlen(state.base_dir); diff --git a/builtin/column.c b/builtin/column.c index 158fdf53d9f..a22b4532301 100644 --- a/builtin/column.c +++ b/builtin/column.c @@ -43,9 +43,8 @@ int cmd_column(int argc, const char **argv, const char *prefix) memset(&copts, 0, sizeof(copts)); copts.padding = 1; - argc = parse_options(argc, argv, prefix, options, builtin_column_usage, 0); - if (argc) - usage_with_options(builtin_column_usage, options); + parse_options(argc, argv, prefix, options, builtin_column_usage, + PARSE_OPT_ERROR_AT_NON_OPTION); if (real_command || command) { if (!real_command || !command || strcmp(real_command, command)) die(_("--command must be the first argument")); diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c index 51c4040ea6c..b92774c7fd3 100644 --- a/builtin/commit-graph.c +++ b/builtin/commit-graph.c @@ -80,11 +80,9 @@ static int graph_verify(int argc, const char **argv) trace2_cmd_mode("verify"); opts.progress = isatty(2); - argc = parse_options(argc, argv, NULL, - options, - builtin_commit_graph_verify_usage, 0); - if (argc) - usage_with_options(builtin_commit_graph_verify_usage, options); + parse_options(argc, argv, NULL, options, + builtin_commit_graph_verify_usage, + PARSE_OPT_ERROR_AT_NON_OPTION); if (!opts.obj_dir) opts.obj_dir = get_object_directory(); @@ -241,11 +239,9 @@ static int graph_write(int argc, const char **argv) git_config(git_commit_graph_write_config, &opts); - argc = parse_options(argc, argv, NULL, - options, - builtin_commit_graph_write_usage, 0); - if (argc) - usage_with_options(builtin_commit_graph_write_usage, options); + parse_options(argc, argv, NULL, options, + builtin_commit_graph_write_usage, + PARSE_OPT_ERROR_AT_NON_OPTION); if (opts.reachable + opts.stdin_packs + opts.stdin_commits > 1) die(_("use at most one of --reachable, --stdin-commits, or --stdin-packs")); diff --git a/builtin/count-objects.c b/builtin/count-objects.c index 07b94195962..4a7452f4b9f 100644 --- a/builtin/count-objects.c +++ b/builtin/count-objects.c @@ -103,10 +103,8 @@ int cmd_count_objects(int argc, const char **argv, const char *prefix) git_config(git_default_config, NULL); - argc = parse_options(argc, argv, prefix, opts, count_objects_usage, 0); - /* we do not take arguments other than flags for now */ - if (argc) - usage_with_options(count_objects_usage, opts); + parse_options(argc, argv, prefix, opts, count_objects_usage, + PARSE_OPT_ERROR_AT_NON_OPTION); if (verbose) { report_garbage = real_report_garbage; report_linked_checkout_garbage(); diff --git a/builtin/fmt-merge-msg.c b/builtin/fmt-merge-msg.c index f1a8290cba7..df6b210127b 100644 --- a/builtin/fmt-merge-msg.c +++ b/builtin/fmt-merge-msg.c @@ -37,9 +37,7 @@ int cmd_fmt_merge_msg(int argc, const char **argv, const char *prefix) git_config(fmt_merge_msg_config, NULL); argc = parse_options(argc, argv, prefix, options, fmt_merge_msg_usage, - 0); - if (argc > 0) - usage_with_options(fmt_merge_msg_usage, options); + PARSE_OPT_ERROR_AT_NON_OPTION); if (shortlog_len < 0) shortlog_len = (merge_log_config > 0) ? merge_log_config : 0; diff --git a/builtin/gc.c b/builtin/gc.c index 021e9256ae2..d3201d8781f 100644 --- a/builtin/gc.c +++ b/builtin/gc.c @@ -587,9 +587,7 @@ int cmd_gc(int argc, const char **argv, const char *prefix) pack_refs = !is_bare_repository(); argc = parse_options(argc, argv, prefix, builtin_gc_options, - builtin_gc_usage, 0); - if (argc > 0) - usage_with_options(builtin_gc_usage, builtin_gc_options); + builtin_gc_usage, PARSE_OPT_ERROR_AT_NON_OPTION); if (prune_expire && parse_expiry_date(prune_expire, &dummy)) die(_("failed to parse prune expiry value %s"), prune_expire); @@ -2491,10 +2489,9 @@ static int maintenance_start(int argc, const char **argv, const char *prefix) OPT_END() }; - argc = parse_options(argc, argv, prefix, options, - builtin_maintenance_start_usage, 0); - if (argc) - usage_with_options(builtin_maintenance_start_usage, options); + parse_options(argc, argv, prefix, options, + builtin_maintenance_start_usage, + PARSE_OPT_ERROR_AT_NON_OPTION); opts.scheduler = resolve_scheduler(opts.scheduler); validate_scheduler(opts.scheduler); diff --git a/builtin/notes.c b/builtin/notes.c index a3d0d15a227..35329411f50 100644 --- a/builtin/notes.c +++ b/builtin/notes.c @@ -958,13 +958,8 @@ static int prune(int argc, const char **argv, const char *prefix) OPT_END() }; - argc = parse_options(argc, argv, prefix, options, git_notes_prune_usage, - 0); - - if (argc) { - error(_("too many arguments")); - usage_with_options(git_notes_prune_usage, options); - } + parse_options(argc, argv, prefix, options, git_notes_prune_usage, + PARSE_OPT_ERROR_AT_NON_OPTION); t = init_notes_check("prune", NOTES_INIT_WRITABLE); diff --git a/builtin/stripspace.c b/builtin/stripspace.c index 1e34cf2bebd..39708db8b1b 100644 --- a/builtin/stripspace.c +++ b/builtin/stripspace.c @@ -42,9 +42,8 @@ int cmd_stripspace(int argc, const char **argv, const char *prefix) OPT_END() }; - argc = parse_options(argc, argv, prefix, options, stripspace_usage, 0); - if (argc) - usage_with_options(stripspace_usage, options); + parse_options(argc, argv, prefix, options, stripspace_usage, + PARSE_OPT_ERROR_AT_NON_OPTION); if (mode == STRIP_COMMENTS || mode == COMMENT_LINES) { setup_git_directory_gently(&nongit); diff --git a/builtin/worktree.c b/builtin/worktree.c index cd62eef240e..29803379006 100644 --- a/builtin/worktree.c +++ b/builtin/worktree.c @@ -153,9 +153,8 @@ static int prune(int ac, const char **av, const char *prefix) }; expire = TIME_MAX; - ac = parse_options(ac, av, prefix, options, worktree_usage, 0); - if (ac) - usage_with_options(worktree_usage, options); + parse_options(ac, av, prefix, options, worktree_usage, + PARSE_OPT_ERROR_AT_NON_OPTION); prune_worktrees(); return 0; } @@ -772,10 +771,9 @@ static int list(int ac, const char **av, const char *prefix) }; expire = TIME_MAX; - ac = parse_options(ac, av, prefix, options, worktree_usage, 0); - if (ac) - usage_with_options(worktree_usage, options); - else if (verbose && porcelain) + parse_options(ac, av, prefix, options, worktree_usage, + PARSE_OPT_ERROR_AT_NON_OPTION); + if (verbose && porcelain) die(_("options '%s' and '%s' cannot be used together"), "--verbose", "--porcelain"); else if (!line_terminator && !porcelain) die(_("the option '%s' requires '%s'"), "-z", "--porcelain"); diff --git a/imap-send.c b/imap-send.c index a50af56b827..f7c4efa3759 100644 --- a/imap-send.c +++ b/imap-send.c @@ -1526,10 +1526,8 @@ int cmd_main(int argc, const char **argv) setup_git_directory_gently(&nongit_ok); git_config(git_imap_config, NULL); - argc = parse_options(argc, (const char **)argv, "", imap_send_options, imap_send_usage, 0); - - if (argc) - usage_with_options(imap_send_usage, imap_send_options); + parse_options(argc, (const char **)argv, "", imap_send_options, + imap_send_usage, PARSE_OPT_ERROR_AT_NON_OPTION); #ifndef USE_CURL_FOR_IMAP_SEND if (use_curl) { diff --git a/parse-options.c b/parse-options.c index 60477156720..368875bb32d 100644 --- a/parse-options.c +++ b/parse-options.c @@ -631,6 +631,8 @@ void parse_options_start(struct parse_opt_ctx_t *ctx, ctx->out = argv; ctx->prefix = prefix; ctx->cpidx = ((flags & PARSE_OPT_KEEP_ARGV0) != 0); + if (flags & PARSE_OPT_ERROR_AT_NON_OPTION) + flags |= PARSE_OPT_STOP_AT_NON_OPTION; ctx->flags = flags; parse_options_alter_flags(ctx); parse_options_check_flags(options, ctx->flags); @@ -894,6 +896,11 @@ int parse_options(int argc, const char **argv, case PARSE_OPT_COMPLETE: exit(0); case PARSE_OPT_NON_OPTION: + if (flags & PARSE_OPT_ERROR_AT_NON_OPTION) { + error(_("unknown non-option: `%s'"), ctx.argv[0]); + usage_with_options(usagestr, options); + } + break; case PARSE_OPT_DONE: break; case PARSE_OPT_UNKNOWN: diff --git a/parse-options.h b/parse-options.h index 460afb93256..013e539d6cc 100644 --- a/parse-options.h +++ b/parse-options.h @@ -34,6 +34,7 @@ enum parse_opt_flags { PARSE_OPT_SHELL_EVAL = 1 << 6, PARSE_OPT_REV_PARSE_PARSEOPT = 1 << 7, PARSE_OPT_INTERNAL_HELP_ON_ONE_ARG = 1 << 8, + PARSE_OPT_ERROR_AT_NON_OPTION = 1 << 9, }; /** diff --git a/t/helper/test-getcwd.c b/t/helper/test-getcwd.c index d680038a780..b96bdc72c1e 100644 --- a/t/helper/test-getcwd.c +++ b/t/helper/test-getcwd.c @@ -14,9 +14,8 @@ int cmd__getcwd(int argc, const char **argv) }; char *cwd; - argc = parse_options(argc, argv, "test-tools", options, getcwd_usage, 0); - if (argc > 0) - usage_with_options(getcwd_usage, options); + argc = parse_options(argc, argv, "test-tools", options, getcwd_usage, + PARSE_OPT_ERROR_AT_NON_OPTION); cwd = xgetcwd(); puts(cwd);
