Hi,
El mar., 21 ene. 2020 a las 7:40, Christian Couder
(<christian.couder@xxxxxxxxx>) escribió:
>
> 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.
>
Noted!
Thank you Johannes and Christian.
> Thanks for your review,
> Christian.
