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

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 9, 2022, 19:45 UTC
Message-ID: <xmqqh7977rj8.fsf@gitster.g>
In-Reply-To: <eddbdde222a01113d8facdcb17d6ec85676edbe7.1643328752.git.gitgitgadget@gmail.com>

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com> writes:

Show 11 quoted lines

> @@ -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.

Show 26 quoted lines

>  	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);
>  	}