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

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Mon, Feb 15 2021, Taylor Blau wrote:

> On Mon, Feb 15, 2021 at 10:54:31PM +0100, Ævar Arnfjörð Bjarmason wrote:
>>
>> 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.
>
> Yeah, that's a good catch.
>
>> 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/
>
> 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);
     }
    
:)

> diff --git a/builtin/multi-pack-index.c b/builtin/multi-pack-index.c
> index 5ab80bc722..ce4f1a0bcb 100644
> --- a/builtin/multi-pack-index.c
> +++ b/builtin/multi-pack-index.c
> @@ -165,5 +165,6 @@ 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_with_options(builtin_multi_pack_index_usage,
> +                                  builtin_multi_pack_index_options);
>  }
>
> Is it OK if I use your Signed-off-by on both of those two new patches?

Yes please, should have included it to begin with.