Re: [PATCH 06/29] bisect: libify `exit_if_skipped_commits` to `error_if_skipped*` and its dependents

>
> Hi Dscho,
>
> On Mon, Jan 20, 2020 at 10:57 PM Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
> >
> > On Mon, 20 Jan 2020, Miriam Rubio wrote:
>
> > >       printf("There are only 'skip'ped commits left to test.\n"
> > >              "The first %s commit could be any of:\n", term_bad);
> > > @@ -676,7 +676,13 @@ static void exit_if_skipped_commits(struct commit_list *tried,
> > >       if (bad)
> > >               printf("%s\n", oid_to_hex(bad));
> > >       printf(_("We cannot bisect more!\n"));
> > > -     exit(2);
> > > +
> > > +     /*
> > > +      * We don't want to clean the bisection state
> > > +      * as we need to get back to where we started
> > > +      * by using `git bisect reset`.
> > > +      */
> > > +     return -2;
> >
> > This comment is a good indicator that the constant `-2` here is a "magic"
> > number and it would most likely make sense to turn the return type from an
> > `int` into an `enum` instead.
>
> Many functions use `return error(...)` and error codes from these
> functions and from exit_if_skipped_commits() are going to get mixed.
> So I am not sure that using an enum for only some of the error codes
> will make things clearer.
>
> > >  static int is_expected_rev(const struct object_id *oid)
> > > @@ -949,7 +955,7 @@ int bisect_next_all(struct repository *r, const char *prefix, int no_checkout)
> > >  {
> > >       struct rev_info revs;
> > >       struct commit_list *tried;
> > > -     int reaches = 0, all = 0, nr, steps;
> > > +     int reaches = 0, all = 0, nr, steps, res;
> > >       struct object_id *bisect_rev;
> > >       char *steps_msg;
> > >
> > > @@ -972,8 +978,9 @@ int bisect_next_all(struct repository *r, const char *prefix, int no_checkout)
> > >                * We should exit here only if the "bad"
> > >                * commit is also a "skip" commit.
> > >                */
> > > -             exit_if_skipped_commits(tried, NULL);
> > > -
> > > +             res = error_if_skipped_commits(tried, NULL);
> > > +             if (res)
> > > +                     exit(-res);
> >
> > So we still `exit()` in `libgit.a`? I hoped for a more thorough
> > libification.
>
> The exit() calls are removed in later patches.
>
> > Besides, the `if (res)` probably wants to be an `if (res < 0)`, right?
>
> Yeah, I agree.
>