Hi Elijah,
On Tue, 8 Feb 2022, Elijah Newren wrote:
> On Fri, Jan 28, 2022 at 3:52 PM Johannes Schindelin via GitGitGadget
> <gitgitgadget@xxxxxxxxx> wrote:
> >
> > From: Johannes Schindelin <johannes.schindelin@xxxxxx>
> >
> > 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@xxxxxx>
> > ---
> > 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...)
Good point! That code will go away within the same patch series, of
course, but it is good to keep this in an intermediate commit.
Thanks,
Dscho
