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

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi Junio,

On Wed, 9 Feb 2022, Junio C Hamano wrote:

> "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
> writes:
>
> > @@ -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;
> > +	}
> > +
>
> I do agree with "if we want to reuse this function without changing
> it much, --bisect-state must become an optional and the default mode
> of operation" as a goal, but I do not quite get this change.  From
> the rephased description of the goal, what I would expect is more
> like
>
> 	if (!cmdmode && possibly-other-conditions-like-argc-check)
> 		cmdmode = BISECT_STATE;
>
> and nothing else.

That would not work, though: we must ensure that the `argv[0]` is a
bad/good term (hence the need to call `set_terms()` and `get_terms()` to
retrieve potentially overridden terms and then the call to
`check_and_set_terms()` to verify that it was one of those terms).

> Between the case where --bisect-state was and was not given explicitly,
> check-and-set-terms is or is not called.  I
> can see that checking would be _nice_ when we try to decide if the
> first token makes sense as good/bad and the user wanted to do the
> "state" thing impolicitly, but if it is worth checking in implicit
> case, shouldn't we be checking when the --bisect-state is explicitly
> given as well?

I tried to keep the `if (!cmdmode) usage...` in place, and that's probably
what caused your confusion. I replaced it with:

-- snip --
@@ -1210,10 +1210,7 @@ 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)
-               usage_with_options(git_bisect_helper_usage, options);
-
-       switch (cmdmode) {
+       switch (cmdmode ? cmdmode : BISECT_STATE) {
        case BISECT_START:
                set_terms(&terms, "bad", "good");
                res = bisect_start(&terms, argv, argc);
@@ -1221,6 +1218,11 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
        case BISECT_STATE:
                set_terms(&terms, "bad", "good");
                get_terms(&terms);
+               if (!cmdmode &&
+                   (!argc || check_and_set_terms(&terms, argv[0]))) {
+                       char *msg = xstrfmt(_("unknown command: '%s'"), argv[0]);
+                       usage_msg_opt(msg, git_bisect_helper_usage, options);
+               }
                res = bisect_state(&terms, argv, argc);
                break;
        case BISECT_TERMS:
-- snap --

I am not the best judge whether this is more obvious because I am so
familiar with this code, but I hope that it is at least more concise.

> And the actual execution of the BISECT_STATE command is even more
> puzzling, below.
>
> >  	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;
>
> This case arm has been moved but because there is no fall-through in
> this switch statement, the movement is a no-op.  But the code has
> also changed with this patch.  We used to do set_terms/get_terms
> unconditionally, but we do not even do so when terms_good (but not
> terms_bad) is already set.
>
> Is this an unrelated bugfix of some kind?  This does not look
> related to "making --bisect-state optional and implicit default" at
> all.  At least the proposed log message does not explain why.

I had hoped that this part of the commit message would be clear enough:

	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.

Since my hope did not turn into reality, I split out that move into its
own commit (making the patch series even longer, but hopefully at least
making it more readable, too).

Ciao,
Dscho