Hi Dscho,
On Mon, Jan 20, 2020 at 10:57 PM Johannes Schindelin
<Johannes.Schindelin@xxxxxx> 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.
