Re: [PATCH v3 6/6] commit-graph: show usage on "commit-graph [write|verify] garbage"

On Tue, Jul 20, 2021 at 07:47:39PM +0200, SZEDER Gábor wrote:

On Tue, Jul 20, 2021 at 01:39:45PM +0200, Ævar Arnfjörð Bjarmason wrote:

quoted

Change the parse_options() invocation in the commit-graph code to make
sense. We're calling it twice, once for common options parsing, and
then for the sub-commands.

But we never checked if we had something leftover in argc in "write"
or "verify", as a result we'd silently accept garbage in these
subcommands. Let's not do that.

Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
 builtin/commit-graph.c  | 10 ++++++++--
 t/t5318-commit-graph.sh |  5 +++++
 2 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c
index bf34aa43f22..88cbcb5a64f 100644
--- a/builtin/commit-graph.c
+++ b/builtin/commit-graph.c

@@ -104,7 +104,10 @@ static int graph_verify(int argc, const char **argv)
 	opts.progress = isatty(2);
 	argc = parse_options(argc, argv, NULL,
 			     options,
-			     builtin_commit_graph_verify_usage, 0);
+			     builtin_commit_graph_verify_usage,
+			     PARSE_OPT_KEEP_UNKNOWN);
+	if (argc)
+		usage_with_options(builtin_commit_graph_verify_usage, options);

Checking 'argc' alone is sufficient to catch unsupported parameters.

Using PARSE_OPT_KEEP_UNKNOWN is not only unnecessary, but arguably
wrong here

And it's wrong in 'multi-pack-index' and 'env--helper' as well.

because 'git commit-graph write --foo' won't print "error:
unknown option `foo'", and we don't want to pass the remaining
unrecognized options to a different command, like e.g. 'git difftool',
or another parse_options(), like e.g. 'git archive'.