Re: [PATCH 06/11] bisect--helper: make `--bisect-state` optional

[Elijah Newren <newren@gmail.com>]

On Fri, Jan 28, 2022 at 3:52 PM Johannes Schindelin via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> In preparation for making `git bisect` a real built-in, let's prepare
> the `bisect--helper` built-in to handle `git bisect--helper good` and
> `git bisect--helper bad`, i.e. do not require the `--bisect-state`
> option to be passed explicitly.
>
> To prepare for converting `bisect--helper` to a full built-in
> implementation of `git bisect` (which must not require the
> `--bisect-state` option to be specified at all), let's move the handling
> toward the end of the `switch (cmdmode)` block.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  builtin/bisect--helper.c | 23 ++++++++++++++++-------
>  1 file changed, 16 insertions(+), 7 deletions(-)
>
> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
> index cc5a9ca41b9..219fa99cd0b 100644
> --- a/builtin/bisect--helper.c
> +++ b/builtin/bisect--helper.c
> @@ -26,8 +26,8 @@ static const char * const git_bisect_helper_usage[] = {
>         N_("git bisect--helper --bisect-start [--term-{new,bad}=<term> --term-{old,good}=<term>]"
>                                             " [--no-checkout] [--first-parent] [<bad> [<good>...]] [--] [<paths>...]"),
>         N_("git bisect--helper --bisect-next"),
> -       N_("git bisect--helper --bisect-state (bad|new) [<rev>]"),
> -       N_("git bisect--helper --bisect-state (good|old) [<rev>...]"),
> +       N_("git bisect--helper [--bisect-state] (bad|new) [<rev>]"),
> +       N_("git bisect--helper [--bisect-state] (good|old) [<rev>...]"),
>         N_("git bisect--helper --bisect-replay <filename>"),
>         N_("git bisect--helper --bisect-skip [(<rev>|<range>)...]"),
>         N_("git bisect--helper --bisect-visualize"),
> @@ -1210,6 +1210,13 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
>                              git_bisect_helper_usage,
>                              PARSE_OPT_KEEP_DASHDASH | PARSE_OPT_KEEP_UNKNOWN);
>
> +       if (!cmdmode && argc > 0) {
> +               set_terms(&terms, "bad", "good");
> +               get_terms(&terms);
> +               if (!check_and_set_terms(&terms, argv[0]))
> +                       cmdmode = BISECT_STATE;
> +       }
> +
>         if (!cmdmode)
>                 usage_with_options(git_bisect_helper_usage, options);
>
> @@ -1218,11 +1225,6 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
>                 set_terms(&terms, "bad", "good");
>                 res = bisect_start(&terms, argv, argc);
>                 break;
> -       case BISECT_STATE:
> -               set_terms(&terms, "bad", "good");
> -               get_terms(&terms);
> -               res = bisect_state(&terms, argv, argc);
> -               break;
>         case BISECT_TERMS:
>                 if (argc > 1)
>                         return error(_("--bisect-terms requires 0 or 1 argument"));
> @@ -1265,6 +1267,13 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
>                 get_terms(&terms);
>                 res = bisect_run(&terms, argv, argc);
>                 break;
> +       case BISECT_STATE:
> +               if (!terms.term_good) {
> +                       set_terms(&terms, "bad", "good");
> +                       get_terms(&terms);
> +               }
> +               res = bisect_state(&terms, argv, argc);
> +               break;
>         default:
>                 BUG("unknown subcommand %d", cmdmode);
>         }
> --
> gitgitgadget

Does it make sense to also include something like this:

diff --git a/git-bisect.sh b/git-bisect.sh
index 405cf76f2a..fbf56649d7 100755
--- a/git-bisect.sh
+++ b/git-bisect.sh
@@ -60,7 +60,7 @@ case "$#" in
        start)
                git bisect--helper --bisect-start "$@" ;;
        bad|good|new|old|"$TERM_BAD"|"$TERM_GOOD")
-               git bisect--helper --bisect-state "$cmd" "$@" ;;
+               git bisect--helper "$cmd" "$@" ;;
        skip)
                git bisect--helper --bisect-skip "$@" || exit;;
        next)

to prove that you've made it optional?  (Well, assuming one has run
the tests with that change, but I did...)