* Re: [PATCH] multi-pack-index: simplify handling of unknown --options
2022-07-10 15:16 ` SZEDER Gábor
@ 2022-07-10 16:26 ` Ævar Arnfjörð Bjarmason
2022-07-10 21:54 ` Junio C Hamano
1 sibling, 0 replies; 5+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-07-10 16:26 UTC (permalink / raw)
To: SZEDER Gábor; +Cc: Junio C Hamano, git
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@gmail.com> 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@gmail.com>
> ---
> 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);
^ permalink raw reply related [flat|nested] 5+ messages in thread