Re: [PATCH] bisect: fix handling of `help` and invalid subcommands

From: Ruoyu Zhong <zhongruoyu@outlook.com>
Date: Oct 22, 2025, 20:15 UTC
Message-ID: <DE6BAECC-9A60-490C-AEB4-0958177A1588@outlook.com>
In-Reply-To: <3DA38465-7636-4EEF-B074-53E4628F5355@gmail.com>

Hi Ben,

Thanks for the review!

Show 5 quoted lines

> On Oct 23, 2025, at 1:52 AM, Ben Knoble <ben.knoble@gmail.com> wrote:
> 
> Good catch!
> 
> FWIW, in this project we describe the buggy behavior in the present tense (« is broken », « Running git bisect shows », etc.)

Thanks! Will keep this in mind.

Show 13 quoted lines

>> diff --git a/builtin/bisect.c b/builtin/bisect.c
>> index 8b8d870cd1..993caf545d 100644
>> --- a/builtin/bisect.c
>> +++ b/builtin/bisect.c
>> @@ -1453,9 +1453,13 @@ int cmd_bisect(int argc,
>>       if (!argc)
>>           usage_msg_opt(_("need a command"), git_bisect_usage, options);
>> 
>> +        if (!strcmp(argv[0], "help"))
>> +            usage_with_options(git_bisect_usage, options);
>> +
> 
> From an extremely quick look at the code, this might be better handled with a new OPT_SUBCOMMAND, though that might mean making the options array statically scoped to this file rather than the function.

I intended to keep it simple so I did not make it an OPT_SUBCOMMAND at the first place. Given that Junio is okay with it, I'm going to keep it as is for now. Still happy to turn this into an OPT_SUBCOMMAND if you would like.

> It would also be nice to update the usage to match the manual while we’re here, which presumably in turn affects the test between command usage and manuals.

Thanks for pointing that out! Yes, I think so too. Will do in a separate patch, if you agree, in order not to digress too much.

Regards, Ruoyu