"Johannes Schindelin via GitGitGadget" <gitgitgadget@xxxxxxxxx>
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);
> }
