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

On Mon, Feb 15 2021, Taylor Blau wrote:

On Mon, Feb 15, 2021 at 10:54:31PM +0100, Ævar Arnfjörð Bjarmason wrote:

quoted

On Mon, Feb 15 2021, Taylor Blau wrote:

quoted

 	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.

Yeah, that's a good catch.

quoted

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?

I split this into two patches: one to move the trace2_cmd_mode() calls
around, and another to replace the final 'die()' with the usage text.

Thanks for picking it up.

Like I said in my review of your patches to the commit-graph builtin
here:

    https://lore.kernel.org/git/YCrDGhIq7kU57p1s@nand.local/ (local)

I don't find the 'if (argc && ...)' style more readable, so the second
patch looks like this instead:

*Nod* FWIW (and this is getting way to nit-y) I don't disagree with you
about the "argc &&" being not very readable,

I just lean more on the side of getting rid of duplicate branches,
you'll still need the if (!argc) usage(...) case above without that
pattern, or some replacement for it.

But we can have our cake (not re-check argc all the time) and eat it too
(not copy/paste usage_with_options()). Isn't it beautiful?
    
    diff --git a/builtin/multi-pack-index.c b/builtin/multi-pack-index.c
    index caf0248a98..7ff50439f8 100644
    --- a/builtin/multi-pack-index.c
    +++ b/builtin/multi-pack-index.c
    @@ -144,12 +144,8 @@ 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 (!argc)
    +		goto usage;
     	if (!strcmp(argv[0], "repack"))
     		return cmd_multi_pack_index_repack(argc, argv);
     	else if (!strcmp(argv[0], "write"))
    @@ -159,5 +155,7 @@ int cmd_multi_pack_index(int argc, const char **argv,
     	else if (!strcmp(argv[0], "expire"))
     		return cmd_multi_pack_index_expire(argc, argv);
     	else
    -		die(_("unrecognized subcommand: %s"), argv[0]);
    +usage:
    +		usage_with_options(builtin_multi_pack_index_usage,
    +				   builtin_multi_pack_index_options);
     }
    
:)

Yes please, should have included it to begin with.