Subject: Re: [PATCH v2 4/4] builtin/multi-pack-index.c: split sub-commands

From: Ævar Arnfjörð Bjarmason <[email-masked]>
Date: Mon, 15 Feb 2021 22:54:31 +0100

On Mon, Feb 15 2021, Taylor Blau wrote:

>  	trace2_cmd_mode(argv[0]);

Not a new issue, but curious that in the commit-graph.c code we'll first
validate, but here write garbage into the trace2_cmd_mode() before
potentially dying.

>  
>  	if (!strcmp(argv[0], "repack"))
> -		return midx_repack(the_repository, opts.object_dir,
> -			(size_t)opts.batch_size, opts.flags);
> -	if (opts.batch_size)
> -		die(_("--batch-size option is only for 'repack' subcommand"));
> -
> -	if (!strcmp(argv[0], "write"))
> -		return write_midx_file(opts.object_dir, opts.flags);
> -	if (!strcmp(argv[0], "verify"))
> -		return verify_midx_file(the_repository, opts.object_dir, opts.flags);
> -	if (!strcmp(argv[0], "expire"))
> -		return expire_midx_packs(the_repository, opts.object_dir, opts.flags);
> -
> -	die(_("unrecognized subcommand: %s"), argv[0]);
> +		return cmd_multi_pack_index_repack(argc, argv);
> +	else if (!strcmp(argv[0], "write"))
> +		return cmd_multi_pack_index_write(argc, argv);
> +	else if (!strcmp(argv[0], "verify"))
> +		return cmd_multi_pack_index_verify(argc, argv);
> +	else if (!strcmp(argv[0], "expire"))
> +		return cmd_multi_pack_index_expire(argc, argv);
> +	else
> +		die(_("unrecognized subcommand: %s"), argv[0]);

I realize this is the existing behavior, but let's just make this die()
be the usage_with_options() we emit above in this case?

So maybe this on top?

diff --git a/builtin/multi-pack-index.c b/builtin/multi-pack-index.c
index caf0248a98..6f9223d538 100644
--- a/builtin/multi-pack-index.c
+++ b/builtin/multi-pack-index.c
@@ -65,6 +65,8 @@ static int cmd_multi_pack_index_write(int argc, const char **argv)
 {
 	struct option *options = common_opts;
 
+	trace2_cmd_mode(argv[0]);
+
 	argc = parse_options(argc, argv, NULL,
 			     options, builtin_multi_pack_index_write_usage,
 			     PARSE_OPT_KEEP_UNKNOWN);
@@ -79,6 +81,8 @@ static int cmd_multi_pack_index_verify(int argc, const char **argv)
 {
 	struct option *options = common_opts;
 
+	trace2_cmd_mode(argv[0]);
+
 	argc = parse_options(argc, argv, NULL,
 			     options, builtin_multi_pack_index_verify_usage,
 			     PARSE_OPT_KEEP_UNKNOWN);
@@ -93,6 +97,8 @@ static int cmd_multi_pack_index_expire(int argc, const char **argv)
 {
 	struct option *options = common_opts;
 
+	trace2_cmd_mode(argv[0]);
+
 	argc = parse_options(argc, argv, NULL,
 			     options, builtin_multi_pack_index_expire_usage,
 			     PARSE_OPT_KEEP_UNKNOWN);
@@ -112,6 +118,8 @@ static int cmd_multi_pack_index_repack(int argc, const char **argv)
 		OPT_END(),
 	};
 
+	trace2_cmd_mode(argv[0]);
+
 	options = parse_options_dup(builtin_multi_pack_index_repack_options);
 	options = add_common_options(options);
 
@@ -144,20 +152,15 @@ int cmd_multi_pack_index(int argc, const char **argv,
 	if (!opts.object_dir)
 		opts.object_dir = get_object_directory();
 
-	if (argc == 0)
-		usage_with_options(builtin_multi_pack_index_usage,
-				   builtin_multi_pack_index_options);
-
-	trace2_cmd_mode(argv[0]);
-
-	if (!strcmp(argv[0], "repack"))
+	if (argc && !strcmp(argv[0], "repack"))
 		return cmd_multi_pack_index_repack(argc, argv);
-	else if (!strcmp(argv[0], "write"))
+	else if (argc && !strcmp(argv[0], "write"))
 		return cmd_multi_pack_index_write(argc, argv);
-	else if (!strcmp(argv[0], "verify"))
+	else if (argc && !strcmp(argv[0], "verify"))
 		return cmd_multi_pack_index_verify(argc, argv);
-	else if (!strcmp(argv[0], "expire"))
+	else if (argc && !strcmp(argv[0], "expire"))
 		return cmd_multi_pack_index_expire(argc, argv);
 	else
-		die(_("unrecognized subcommand: %s"), argv[0]);
+		usage_with_options(builtin_multi_pack_index_usage,
+				   builtin_multi_pack_index_options);
 }