Re: [PATCH v6 2/2] bisect: add --reset-when-found to leave when done

> > @@ -682,7 +761,8 @@ static int bisect_successful(struct bisect_terms *terms)
> >       return res;
> >  }
> >
> > -static enum bisect_error bisect_next(struct bisect_terms *terms, const char *prefix)
> > +static enum bisect_error bisect_next(struct bisect_terms *terms,
> > +                                  const char *prefix)
> >  {
> >       enum bisect_error res;
> >
> > @@ -705,7 +785,8 @@ static enum bisect_error bisect_next(struct bisect_terms *terms, const char *pre
> >       return res;
> >  }
> >
> > -static enum bisect_error bisect_auto_next(struct bisect_terms *terms, const char *prefix)
> > +static enum bisect_error bisect_auto_next(struct bisect_terms *terms,
> > +                                       const char *prefix)
> >  {
> >       if (bisect_next_check(terms, NULL)) {
> >               bisect_print_status(terms);
>
> The above two hunks are pure style clean-ups.  When having others to
> review a 500+ line patch, you would want to omit them or move them
> to a separate preliminary clean-up step, to avoid distracting them.

I'll remove.

> >  static int cmd_bisect__terms(int argc, const char **argv, const char *prefix UNUSED,
> > @@ -1489,7 +1614,8 @@ int cmd_bisect(int argc,
> >                   !one_of(argv[0], terms.term_good, terms.term_bad, NULL))
> >                       usage_msg_optf(_("unknown command: '%s'"), git_bisect_usage,
> >                                      options, argv[0]);
> > -             res = bisect_state(&terms, argc, argv);
> > +             else
> > +                     res = bisect_state(&terms, argc, argv);
> >               free_terms(&terms);
> >       } else {
> >               argc--;
>
> What is this change about?  We used to see if the given terms
> (bad/good) are sensible and otherwise barfed with usage_msg_optf()
> that never returns, so we did without "else".  With "else" you are
> making it more explicit.  The value of such a change is debatable.
> Some would say that, just like 'if ... die()', it is already
> explicit enough that 'if ... usage()' never returns and does not
> require an "else".  Some would say new readers may not know die()
> and usage() do not return, so "else" makes it more explicit.  My
> stance is that we should not optimize our code for total newbies
> [*], so I may have a mild preference for the original over the
> updated version, but it is minor.  In other words, I would not mind
> if an author wrote this either way in new code.
>
> However.
>
> If an author is adding a new feature, I would recommend against
> making such a change that would only force reviewers to read more
> and think more about the change.  Do not waste reviewers' attention,
> which is a precious resource, to something much less relevant for
> the goal of your topic.

Good points.

> > @@ -1497,5 +1623,15 @@ int cmd_bisect(int argc,
> >               res = fn(argc, argv, prefix, repo);
> >       }
> >
> > +     if (res == BISECT_INTERNAL_SUCCESS_1ST_BAD_FOUND) {
> > +             enum reset_when_found_mode mode;
> > +
> > +             if (read_reset_when_found(&mode))
> > +                     res = BISECT_FAILED;
> > +             else if (mode != RESET_WHEN_FOUND_NONE &&
> > +                      bisect_reset_when_found(mode))
> > +                     res = BISECT_FAILED;
> > +     }
>
> Are there "dead end" states, other than '1st-bad-found', in which we
> can no longer make any progress?  One thing that comes to mind is
> "you said this one is good, but that contradicts what you said about
> its ancestor that you said is bad".  I wonder if we want to do
> anything special here, just as this part of the code handles the
> '1st-bad-found' state, for such "dead end" states.

Maybe, but I'm not sure this topic is the right place to dig into this?


Harald