{"thread":{"id":"64368","subject":"[PATCH] bisect: fix handling of `help` and invalid subcommands","startedAt":"2025-10-22T08:36:15Z","lastAt":"2025-10-23T15:14:17Z","messageCount":6,"participants":["Ruoyu Zhong via GitGitGadget","Ben Knoble","Junio C Hamano","Ruoyu Zhong"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"529398","messageId":"pull.2078.git.git.1761122173126.gitgitgadget@gmail.com","threadId":"64368","inReplyTo":null,"subject":"[PATCH] bisect: fix handling of `help` and invalid subcommands","fromName":"Ruoyu Zhong via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-10-22T08:36:13Z","receivedAt":"2025-10-22T08:36:15Z","isPatch":true,"sender":{"key":"zhongruoyu@outlook.com","avatar":"https://avatars.githubusercontent.com/u/25842576?v=4"},"body":"From: Ruoyu Zhong <zhongruoyu@outlook.com>\n\nAs documented in git-bisect(1), `git bisect help` should display usage\ninformation. However, since the migration of `git bisect` to a full\nbuiltin command in 73fce29427 (Turn `git bisect` into a full built-in,\n2022-11-10), this behavior was broken. Running `git bisect help` would,\ninstead of showing usage, either fail silently if already in a bisect\nsession, or otherwise trigger an interactive autostart prompt asking \"Do\nyou want me to do it for you [Y/n]?\".\n\nSimilarly, since df63421be9 (bisect--helper: handle states directly,\n2022-11-10), running invalid subcommands like `git bisect foobar` also\nled to the same behavior.\n\nThis occurred because `help` and other unrecognized subcommands were\nbeing unconditionally passed to `bisect_state`, which then called\n`bisect_autostart`, triggering the interactive prompt.\n\nFix this by:\n1. Adding explicit handling for the `help` subcommand to show usage;\n2. Validating that unrecognized commands are actually valid state\n   commands before calling `bisect_state`;\n3. Showing an error with usage for truly invalid commands.\n\nThis ensures that `git bisect help` displays the usage as documented,\nand invalid commands fail cleanly without entering interactive mode.\nAlternate terms are still handled correctly through\n`check_and_set_terms`.\n\nSigned-off-by: Ruoyu Zhong <zhongruoyu@outlook.com>\n---\n    bisect: fix handling of help and invalid subcommands\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2078%2FZhongRuoyu%2Fgit-bisect-subcommands-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2078/ZhongRuoyu/git-bisect-subcommands-v1\nPull-Request: https://github.com/git/git/pull/2078\n\n builtin/bisect.c | 6 +++++-\n 1 file changed, 5 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/bisect.c b/builtin/bisect.c\nindex 8b8d870cd1..993caf545d 100644\n--- a/builtin/bisect.c\n+++ b/builtin/bisect.c\n@@ -1453,9 +1453,13 @@ int cmd_bisect(int argc,\n \t\tif (!argc)\n \t\t\tusage_msg_opt(_(\"need a command\"), git_bisect_usage, options);\n \n+\t\tif (!strcmp(argv[0], \"help\"))\n+\t\t\tusage_with_options(git_bisect_usage, options);\n+\n \t\tset_terms(&terms, \"bad\", \"good\");\n \t\tget_terms(&terms);\n-\t\tif (check_and_set_terms(&terms, argv[0]))\n+\t\tif (check_and_set_terms(&terms, argv[0]) ||\n+\t\t    !one_of(argv[0], terms.term_good, terms.term_bad, NULL))\n \t\t\tusage_msg_optf(_(\"unknown command: '%s'\"), git_bisect_usage,\n \t\t\t\t       options, argv[0]);\n \t\tres = bisect_state(&terms, argc, argv);\n\nbase-commit: 81f86aacc4eb74cdb9c2c8082d36d2070c666045\n-- \ngitgitgadget\n"},{"id":"529434","messageId":"3DA38465-7636-4EEF-B074-53E4628F5355@gmail.com","threadId":"64368","inReplyTo":"pull.2078.git.git.1761122173126.gitgitgadget@gmail.com","subject":"Re: [PATCH] bisect: fix handling of `help` and invalid subcommands","fromName":"Ben Knoble","fromEmail":"ben.knoble@gmail.com","sentAt":"2025-10-22T17:52:44Z","receivedAt":"2025-10-22T17:52:56Z","isPatch":true,"sender":{"key":"ben.knoble@gmail.com","avatar":"https://avatars.githubusercontent.com/u/22802209?v=4"},"body":"\n> Le 22 oct. 2025 à 04:37, Ruoyu Zhong via GitGitGadget <gitgitgadget@gmail.com> a écrit :\n> \n> From: Ruoyu Zhong <zhongruoyu@outlook.com>\n> \n> As documented in git-bisect(1), `git bisect help` should display usage\n> information. However, since the migration of `git bisect` to a full\n> builtin command in 73fce29427 (Turn `git bisect` into a full built-in,\n> 2022-11-10), this behavior was broken. Running `git bisect help` would,\n> instead of showing usage, either fail silently if already in a bisect\n> session, or otherwise trigger an interactive autostart prompt asking \"Do\n> you want me to do it for you [Y/n]?\".\n\nGood catch!\n\nFWIW, in this project we describe the buggy behavior in the present tense (« is broken », « Running git bisect shows », etc.)\n\n> \n> Similarly, since df63421be9 (bisect--helper: handle states directly,\n> 2022-11-10), running invalid subcommands like `git bisect foobar` also\n> led to the same behavior.\n> \n> This occurred because `help` and other unrecognized subcommands were\n> being unconditionally passed to `bisect_state`, which then called\n> `bisect_autostart`, triggering the interactive prompt.\n> \n> Fix this by:\n> 1. Adding explicit handling for the `help` subcommand to show usage;\n> 2. Validating that unrecognized commands are actually valid state\n>   commands before calling `bisect_state`;\n> 3. Showing an error with usage for truly invalid commands.\n> \n> This ensures that `git bisect help` displays the usage as documented,\n> and invalid commands fail cleanly without entering interactive mode.\n> Alternate terms are still handled correctly through\n> `check_and_set_terms`.\n> \n> Signed-off-by: Ruoyu Zhong <zhongruoyu@outlook.com>\n> ---\n>    bisect: fix handling of help and invalid subcommands\n> \n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2078%2FZhongRuoyu%2Fgit-bisect-subcommands-v1\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2078/ZhongRuoyu/git-bisect-subcommands-v1\n> Pull-Request: https://github.com/git/git/pull/2078\n> \n> builtin/bisect.c | 6 +++++-\n> 1 file changed, 5 insertions(+), 1 deletion(-)\n> \n> diff --git a/builtin/bisect.c b/builtin/bisect.c\n> index 8b8d870cd1..993caf545d 100644\n> --- a/builtin/bisect.c\n> +++ b/builtin/bisect.c\n> @@ -1453,9 +1453,13 @@ int cmd_bisect(int argc,\n>        if (!argc)\n>            usage_msg_opt(_(\"need a command\"), git_bisect_usage, options);\n> \n> +        if (!strcmp(argv[0], \"help\"))\n> +            usage_with_options(git_bisect_usage, options);\n> +\n\nFrom 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.\n\nIt 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.\n\n>        set_terms(&terms, \"bad\", \"good\");\n>        get_terms(&terms);\n> -        if (check_and_set_terms(&terms, argv[0]))\n> +        if (check_and_set_terms(&terms, argv[0]) ||\n> +            !one_of(argv[0], terms.term_good, terms.term_bad, NULL))\n>            usage_msg_optf(_(\"unknown command: '%s'\"), git_bisect_usage,\n>                       options, argv[0]);\n>        res = bisect_state(&terms, argc, argv);\n> \n> base-commit: 81f86aacc4eb74cdb9c2c8082d36d2070c666045\n> --\n> gitgitgadget\n\nI think this part is OK, since we only intend to check this when using the « git bisect <term> » form. \n"},{"id":"529435","messageId":"xmqqwm4myetn.fsf@gitster.g","threadId":"64368","inReplyTo":"pull.2078.git.git.1761122173126.gitgitgadget@gmail.com","subject":"Re: [PATCH] bisect: fix handling of `help` and invalid subcommands","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-10-22T18:24:52Z","receivedAt":"2025-10-22T18:24:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Ruoyu Zhong via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> From: Ruoyu Zhong <zhongruoyu@outlook.com>\n>\n> As documented in git-bisect(1), `git bisect help` should display usage\n> information. However, since the migration of `git bisect` to a full\n> builtin command in 73fce29427 (Turn `git bisect` into a full built-in,\n> 2022-11-10), this behavior was broken. Running `git bisect help` would,\n> instead of showing usage, either fail silently if already in a bisect\n> session, or otherwise trigger an interactive autostart prompt asking \"Do\n> you want me to do it for you [Y/n]?\".\n>\n> Similarly, since df63421be9 (bisect--helper: handle states directly,\n> 2022-11-10), running invalid subcommands like `git bisect foobar` also\n> led to the same behavior.\n>\n> This occurred because `help` and other unrecognized subcommands were\n> being unconditionally passed to `bisect_state`, which then called\n> `bisect_autostart`, triggering the interactive prompt.\n\nVery good observations.\n\n> diff --git a/builtin/bisect.c b/builtin/bisect.c\n> index 8b8d870cd1..993caf545d 100644\n> --- a/builtin/bisect.c\n> +++ b/builtin/bisect.c\n> @@ -1453,9 +1453,13 @@ int cmd_bisect(int argc,\n>  \t\tif (!argc)\n>  \t\t\tusage_msg_opt(_(\"need a command\"), git_bisect_usage, options);\n>  \n> +\t\tif (!strcmp(argv[0], \"help\"))\n> +\t\t\tusage_with_options(git_bisect_usage, options);\n\nI briefly wondered why\n\n    $ git grep 'strcmp.*\"help\"'\n\ngives a single hit in parse-options.c but that is simply because\n\"git bisect\" is an oddball.  Everybody, including \"git bisect\"\nitself, takes \"git <cmd> --help\", but in addition, \"git bisect\" also\ntakes \"git bisect help\".  So this addition is unfortunate but cannot\nbe helped (no pun intended).  In an ideal world, as we support the\nbog standard \"--help\", if we could remove \"help\" as a subcommand,\nthen the other part of this patch would take care of \"bisect help\"\nwithout this part by saying \"unknown command\" ;-) but we cannot\nretroactively do so now.\n\nI also wonder if it would be cleaner to add \"help\" as a genuine\nsubcommand to the options[] table just like all the other\nsubcommands.  The above would not be needed if we did so.  But I do\nnot see huge upside for doing so (i.e., a single strcmp() with a\ncall like we see above, vs. a new helper function to make the same\ncall, to usage_with_options()), so what is written in this patch is\nperfectly fine.\n\n>  \t\tset_terms(&terms, \"bad\", \"good\");\n>  \t\tget_terms(&terms);\n> -\t\tif (check_and_set_terms(&terms, argv[0]))\n> +\t\tif (check_and_set_terms(&terms, argv[0]) ||\n> +\t\t    !one_of(argv[0], terms.term_good, terms.term_bad, NULL))\n>  \t\t\tusage_msg_optf(_(\"unknown command: '%s'\"), git_bisect_usage,\n>  \t\t\t\t       options, argv[0]);\n\nThis change is a bit hard to reason about, so let me think aloud.\n\nIf we were saying \"git bisect olde\" after somehow changing bad/good\nto newe/olde, then we do not want to say \"unknown command\", and the\nway it avoids that is to see if the given command is one of the\nterms check_and_set_terms() have updated.  \n\nIn other words, if argv[0] caused check_and_set_terms() to return\nnon-zero, we do not have to do \"unknown command\", because the helper\nwould have issued a warning already.  When it returns 0, it may be\nbecause it saw a valid command \"skip\", etc. that cannot be a custom\ngood/bad/new/old (in which case our one_of() would be false and we\nend up saying \"unknown command\"---have I just spot a bug???), or we\nhaven't set custom terms and argv[0] is one of bad/good/new/old (in\nwhich case our one_of() would be true and we avoid saying \"unknown\ncommand\").\n\nAnd it turns out that my finding about the 'skip' etc. is not a bug,\nas these valid commands that cannot be good/bad/new/old aliases are\nalready caught by parse_options() call and we know argv[0] is not\nsuch a valid subcommand.\n\nSo after all this looks good.\n\nThanks.  Will queue.\n"},{"id":"529443","messageId":"DE6BAECC-9A60-490C-AEB4-0958177A1588@outlook.com","threadId":"64368","inReplyTo":"3DA38465-7636-4EEF-B074-53E4628F5355@gmail.com","subject":"Re: [PATCH] bisect: fix handling of `help` and invalid subcommands","fromName":"Ruoyu Zhong","fromEmail":"zhongruoyu@outlook.com","sentAt":"2025-10-22T20:15:42Z","receivedAt":"2025-10-22T20:15:44Z","isPatch":true,"sender":{"key":"zhongruoyu@outlook.com","avatar":"https://avatars.githubusercontent.com/u/25842576?v=4"},"body":"Hi Ben,\n\n\nThanks for the review!\n\n> On Oct 23, 2025, at 1:52 AM, Ben Knoble <ben.knoble@gmail.com> wrote:\n> \n> Good catch!\n> \n> FWIW, in this project we describe the buggy behavior in the present tense (« is broken », « Running git bisect shows », etc.)\n\nThanks! Will keep this in mind.\n\n>> diff --git a/builtin/bisect.c b/builtin/bisect.c\n>> index 8b8d870cd1..993caf545d 100644\n>> --- a/builtin/bisect.c\n>> +++ b/builtin/bisect.c\n>> @@ -1453,9 +1453,13 @@ int cmd_bisect(int argc,\n>>       if (!argc)\n>>           usage_msg_opt(_(\"need a command\"), git_bisect_usage, options);\n>> \n>> +        if (!strcmp(argv[0], \"help\"))\n>> +            usage_with_options(git_bisect_usage, options);\n>> +\n> \n> 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.\n\nI intended to keep it simple so I did not make it an OPT_SUBCOMMAND at the first\nplace. Given that Junio is okay with it, I'm going to keep it as is for now.\nStill happy to turn this into an OPT_SUBCOMMAND if you would like.\n\n> 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.\n\n\nThanks for pointing that out! Yes, I think so too. Will do in a separate patch,\nif you agree, in order not to digress too much.\n\n\nRegards,\nRuoyu\n\n"},{"id":"529445","messageId":"E1128C6E-C711-4742-B6C9-C3CD487CE4E6@outlook.com","threadId":"64368","inReplyTo":"xmqqwm4myetn.fsf@gitster.g","subject":"Re: [PATCH] bisect: fix handling of `help` and invalid subcommands","fromName":"Ruoyu Zhong","fromEmail":"zhongruoyu@outlook.com","sentAt":"2025-10-22T20:20:30Z","receivedAt":"2025-10-22T20:20:32Z","isPatch":true,"sender":{"key":"zhongruoyu@outlook.com","avatar":"https://avatars.githubusercontent.com/u/25842576?v=4"},"body":"Hi Junio,\n\n\nI appreciate your detailed and thoughtful analysis and I resonate with your\npoints. Specifically:\n\n> I also wonder if it would be cleaner to add \"help\" as a genuine\n> subcommand to the options[] table just like all the other\n> subcommands.  The above would not be needed if we did so.  But I do\n> not see huge upside for doing so (i.e., a single strcmp() with a\n> call like we see above, vs. a new helper function to make the same\n> call, to usage_with_options()), so what is written in this patch is\n> perfectly fine.\n\nAgreed. Will keep this as is, as I also mentioned in my response to Ben.\n\n>> set_terms(&terms, \"bad\", \"good\");\n>> get_terms(&terms);\n>> - if (check_and_set_terms(&terms, argv[0]))\n>> + if (check_and_set_terms(&terms, argv[0]) ||\n>> +    !one_of(argv[0], terms.term_good, terms.term_bad, NULL))\n>> usage_msg_optf(_(\"unknown command: '%s'\"), git_bisect_usage,\n>>       options, argv[0]);\n> \n> This change is a bit hard to reason about, so let me think aloud.\n> \n> If we were saying \"git bisect olde\" after somehow changing bad/good\n> to newe/olde, then we do not want to say \"unknown command\", and the\n> way it avoids that is to see if the given command is one of the\n> terms check_and_set_terms() have updated.  \n> \n> In other words, if argv[0] caused check_and_set_terms() to return\n> non-zero, we do not have to do \"unknown command\", because the helper\n> would have issued a warning already.  When it returns 0, it may be\n> because it saw a valid command \"skip\", etc. that cannot be a custom\n> good/bad/new/old (in which case our one_of() would be false and we\n> end up saying \"unknown command\"---have I just spot a bug???), or we\n> haven't set custom terms and argv[0] is one of bad/good/new/old (in\n> which case our one_of() would be true and we avoid saying \"unknown\n> command\").\n> \n> And it turns out that my finding about the 'skip' etc. is not a bug,\n> as these valid commands that cannot be good/bad/new/old aliases are\n> already caught by parse_options() call and we know argv[0] is not\n> such a valid subcommand.\n> \n> So after all this looks good.\n\nThank you for walking through this. This is very helpful and also matches my own\nthought process when I first looked at it. The key is that this part of the code\nis only reached if fn is still NULL after parse_options, i.e., argv[0] is not\none of the known subcommands.\n\n\nRegards,\nRuoyu\n\n"},{"id":"529507","messageId":"FA7FE583-1EDB-40D4-A839-3AC78A9666DF@gmail.com","threadId":"64368","inReplyTo":"DE6BAECC-9A60-490C-AEB4-0958177A1588@outlook.com","subject":"Re: [PATCH] bisect: fix handling of `help` and invalid subcommands","fromName":"Ben Knoble","fromEmail":"ben.knoble@gmail.com","sentAt":"2025-10-23T15:14:05Z","receivedAt":"2025-10-23T15:14:17Z","isPatch":true,"sender":{"key":"ben.knoble@gmail.com","avatar":"https://avatars.githubusercontent.com/u/22802209?v=4"},"body":"\n> Le 22 oct. 2025 à 16:15, Ruoyu Zhong <zhongruoyu@outlook.com> a écrit :\n> \n> Hi Ben,\n> \n> \n> Thanks for the review!\n> \n>> On Oct 23, 2025, at 1:52 AM, Ben Knoble <ben.knoble@gmail.com> wrote:\n>> Good catch!\n>> FWIW, in this project we describe the buggy behavior in the present tense (« is broken », « Running git bisect shows », etc.)\n> \n> Thanks! Will keep this in mind.\n> \n>>> diff --git a/builtin/bisect.c b/builtin/bisect.c\n>>> index 8b8d870cd1..993caf545d 100644\n>>> --- a/builtin/bisect.c\n>>> +++ b/builtin/bisect.c\n>>> @@ -1453,9 +1453,13 @@ int cmd_bisect(int argc,\n>>>     if (!argc)\n>>>         usage_msg_opt(_(\"need a command\"), git_bisect_usage, options);\n>>> +        if (!strcmp(argv[0], \"help\"))\n>>> +            usage_with_options(git_bisect_usage, options);\n>>> +\n>> 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.\n> \n> I intended to keep it simple so I did not make it an OPT_SUBCOMMAND at the first\n> place. Given that Junio is okay with it, I'm going to keep it as is for now.\n> Still happy to turn this into an OPT_SUBCOMMAND if you would like.\n> \n>> 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.\n> \n> \n> Thanks for pointing that out! Yes, I think so too. Will do in a separate patch,\n> if you agree, in order not to digress too much.\n\nSeparate is fine, although it might have been nice as part of a series with this one. Since Junio has already marked it to queue, no reason to revisit all the above for this patch. Thanks !\n"}]}