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

[Junio C Hamano <gitster@pobox.com>]

"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.  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?

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.

Thanks.

>  	default:
>  		BUG("unknown subcommand %d", cmdmode);
>  	}