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

[Christian Couder <christian.couder@gmail.com>]

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.

Thanks for your review,
Christian.