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

Hi,

El mar., 21 ene. 2020 a las 7:40, Christian Couder
([off-list ref]) escribió:

Hi Dscho,

On Mon, Jan 20, 2020 at 10:57 PM Johannes Schindelin
[off-list ref] wrote:

quoted

On Mon, 20 Jan 2020, Miriam Rubio wrote:

quoted

quoted

      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.

quoted

quoted

 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.

quoted

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.