Re: [PATCH v6 06/13] bisect--helper: reimplement `bisect_next` and `bisect_auto_next` shell functions in C

>
> Miriam Rubio <mirucam@gmail.com> writes:
>
> > diff --git a/bisect.c b/bisect.c
> > index c6aba2b9f2..f0fca5c6f3 100644
> > --- a/bisect.c
> > +++ b/bisect.c
> > @@ -988,6 +988,12 @@ void read_bisect_terms(const char **read_bad, const char **read_good)
> >   * the bisection process finished successfully.
> >   * In this case the calling function or command should not turn a
> >   * BISECT_INTERNAL_SUCCESS_1ST_BAD_FOUND return code into an error or a non zero exit code.
> > + *
> > + * Checking BISECT_INTERNAL_SUCCESS_1ST_BAD_FOUND
> > + * in bisect_helper::bisect_next() and only transforming it to 0 at
> > + * the end of bisect_helper::cmd_bisect__helper() helps bypassing
> > + * all the code related to finding a commit to test.
> > + *
> >   * If no_checkout is non-zero, the bisection process does not
> >   * checkout the trial commit but instead simply updates BISECT_HEAD.
> >   */
>
> Not a problem introduced by this step, but the above description on
> no_checkout describes a parameter that no longer exists.
>
> The comments before a function is to guide the developers how to
> call the function correctly, so it should have been removed, moved
> to where no_checkout is used in the function, or moved to where
> BISECT_HEAD ref gets created, as necessary, but by mistake be5fe200
> (cmd_bisect__helper: defer parsing no-checkout flag, 2020-08-07),
> forgot to do any of the three.
>
>
> > +static enum bisect_error bisect_next(struct bisect_terms *terms, const char *prefix)
> > +{
> > +     int no_checkout;
> > +     enum bisect_error res;
> > +
> > +     bisect_autostart(terms);
> > +     if (bisect_next_check(terms, terms->term_good))
> > +             return BISECT_FAILED;
> > +
> > +     no_checkout = ref_exists("BISECT_HEAD");
> > +
> > +     /* Perform all bisection computation */
> > +     res = bisect_next_all(the_repository, prefix);
> > +
> > +     if (res == BISECT_INTERNAL_SUCCESS_1ST_BAD_FOUND) {
> > +             res = bisect_successful(terms);
> > +             return res ? res : BISECT_INTERNAL_SUCCESS_1ST_BAD_FOUND;
> > +     } else if (res == BISECT_ONLY_SKIPPED_LEFT) {
> > +             res = bisect_skipped_commits(terms);
> > +             return res ? res : BISECT_ONLY_SKIPPED_LEFT;
> > +     }
> > +     return res;
> > +}
> > +
>
> The no_checkout local variable is assigned but never used.  It is
> understandable if a variable that used to be used becomes unused
> when some part (i.e. the part that used to use the variable) of a
> function is factored out, but it is rather unusual how a brand new
> function has such an unused code and stay to be that way throughout
> a topic.  Makes a reviewer suspect that there may be a code missing,
> that has to use the variable to decide to do things differently, in
> this function.  It seems to break -Werror builds.