bisect: fix handling of `help` and invalid subcommands

From: Ruoyu Zhong <[email protected]>

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.

Fix this by:
1. Adding explicit handling for the help subcommand to show usage;
2. Validating that unrecognized commands are actually valid state commands before calling bisect_state;
3. Showing an error with usage for truly invalid commands.

This ensures that git bisect help displays the usage as documented, and invalid commands fail cleanly without entering interactive mode. Alternate terms are still handled correctly through check_and_set_terms.

Signed-off-by Ruoyu Zhong <[email protected]>

---
bisect: fix handling of help and invalid subcommands

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2078%2FZhongRuoyu%2Fgit-bisect-subcommands-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2078/ZhongRuoyu/git-bisect-subcommands-v1
Pull-Request: https://github.com/git/git/pull/2078

builtin/bisect.c | 6 +++++-

1 file changed, 5 insertions(+), 1 deletion(-)

builtin/bisect.c +0 -0

index 8b8d870cd1..993caf545d 100644

builtin/bisect.c +2 -1

       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]);
       res = bisect_state(&terms, argc, argv);

base-commit: 81f86aacc4eb74cdb9c2c8082d36d2070c666045 --
gitgitgadget

Good catch!

FWIW, in this project we describe the buggy behavior in the present tense (« is broken », « Running git bisect shows », etc.)

Quoted 8 lines

builtin/bisect.c +0 -0

index 8b8d870cd1..993caf545d 100644
@@ -1453,9 +1453,13 @@ int cmd_bisect(int argc,
      if (!argc)
          usage_msg_opt(_("need a command"), git_bisect_usage, options);
 

builtin/bisect.c int cmd_bisect(int argc, · line 1458

Quoted 3 lines

builtin/bisect.c +3 -0

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

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.

Hi Ben,

Thanks for the review!

Quoted 3 lines · Ben Knoble

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.

Quoted 11 lines

builtin/bisect.c int cmd_bisect(int argc, · line 1457

Quoted 10 lines

builtin/bisect.c +3 -0

index 8b8d870cd1..993caf545d 100644
@@ -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.

Quoted 1 line

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.

From: Ruoyu Zhong <[email protected]>

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.

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.

Quoted 7 lines

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.