Hi Junio,
On Wed, 9 Feb 2022, Junio C Hamano wrote:
> "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.
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
