From: Ruoyu Zhong <zhongruoyu@outlook.com>
Date: Wed, 22 Oct 2025 20:20:30 GMT
Subject: Re: [PATCH] bisect: fix handling of `help` and invalid subcommands
Message-ID: <E1128C6E-C711-4742-B6C9-C3CD487CE4E6@outlook.com>
In-Reply-To: <xmqqwm4myetn.fsf@gitster.g>

Hi Junio,


I appreciate your detailed and thoughtful analysis and I resonate with your
points. Specifically:

> I also wonder if it would be cleaner to add "help" as a genuine
> subcommand to the options[] table just like all the other
> subcommands.  The above would not be needed if we did so.  But I do
> not see huge upside for doing so (i.e., a single strcmp() with a
> call like we see above, vs. a new helper function to make the same
> call, to usage_with_options()), so what is written in this patch is
> perfectly fine.

Agreed. Will keep this as is, as I also mentioned in my response to Ben.

>> set_terms(&terms, "bad", "good");
>> get_terms(&terms);
>> - if (check_and_set_terms(&terms, argv[0]))
>> + if (check_and_set_terms(&terms, argv[0]) ||
>> +    !one_of(argv[0], terms.term_good, terms.term_bad, NULL))
>> usage_msg_optf(_("unknown command: '%s'"), git_bisect_usage,
>>       options, argv[0]);
> 
> This change is a bit hard to reason about, so let me think aloud.
> 
> If we were saying "git bisect olde" after somehow changing bad/good
> to newe/olde, then we do not want to say "unknown command", and the
> way it avoids that is to see if the given command is one of the
> terms check_and_set_terms() have updated.  
> 
> In other words, if argv[0] caused check_and_set_terms() to return
> non-zero, we do not have to do "unknown command", because the helper
> would have issued a warning already.  When it returns 0, it may be
> because it saw a valid command "skip", etc. that cannot be a custom
> good/bad/new/old (in which case our one_of() would be false and we
> end up saying "unknown command"---have I just spot a bug???), or we
> haven't set custom terms and argv[0] is one of bad/good/new/old (in
> which case our one_of() would be true and we avoid saying "unknown
> command").
> 
> And it turns out that my finding about the 'skip' etc. is not a bug,
> as these valid commands that cannot be good/bad/new/old aliases are
> already caught by parse_options() call and we know argv[0] is not
> such a valid subcommand.
> 
> So after all this looks good.

Thank you for walking through this. This is very helpful and also matches my own
thought process when I first looked at it. The key is that this part of the code
is only reached if fn is still NULL after parse_options, i.e., argv[0] is not
one of the known subcommands.


Regards,
Ruoyu