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

[Junio C Hamano <gitster@pobox.com>]

"Ruoyu Zhong via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Ruoyu Zhong <zhongruoyu@outlook.com>
>
> As documented in git-bisect(1), `git bisect help` should display usage
> information. However, since the migration of `git bisect` to a full
> builtin command in 73fce29427 (Turn `git bisect` into a full built-in,
> 2022-11-10), this behavior was broken. Running `git bisect help` would,
> instead of showing usage, either fail silently if already in a bisect
> session, or otherwise trigger an interactive autostart prompt asking "Do
> you want me to do it for you [Y/n]?".
>
> Similarly, since df63421be9 (bisect--helper: handle states directly,
> 2022-11-10), running invalid subcommands like `git bisect foobar` also
> led to the same behavior.
>
> This occurred because `help` and other unrecognized subcommands were
> being unconditionally passed to `bisect_state`, which then called
> `bisect_autostart`, triggering the interactive prompt.

Very good observations.

> 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);

I briefly wondered why

    $ git grep 'strcmp.*"help"'

gives a single hit in parse-options.c but that is simply because
"git bisect" is an oddball.  Everybody, including "git bisect"
itself, takes "git <cmd> --help", but in addition, "git bisect" also
takes "git bisect help".  So this addition is unfortunate but cannot
be helped (no pun intended).  In an ideal world, as we support the
bog standard "--help", if we could remove "help" as a subcommand,
then the other part of this patch would take care of "bisect help"
without this part by saying "unknown command" ;-) but we cannot
retroactively do so now.

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.

>  		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.

Thanks.  Will queue.