git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Christian Couder <christian.couder@gmail.com>
Date
Jan 21, 2020, 06:40 UTC
Message-ID
<CAP8UFD1T3_1t0Qf3jP1GttNHvaLEWoaeAVeZid6AMH_MAKPjSw@mail.gmail.com>
In-Reply-To
<nycvar.QRO.7.76.6.2001202253450.46@tvgsbejvaqbjf.bet>
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:
Show 18 quoted lines
> >       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.

Show 22 quoted lines
> >  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.

Previous: Johannes SchindelinNext: Miriam R.
Message 31 of 54 in “Finish converting git bisect to C part 1”
  1. Miriam RubioJan 20, 2020
  2. 03/29 bisect: use the standard 'if (!var)' way to check for 0Miriam Rubio, Jan 20, 2020
  3. 01/29 bisect--helper: convert `vocab_*` char pointers to char arraysMiriam Rubio, Jan 20, 2020
  4. 02/29 bisect--helper: change `retval` to `res`Miriam Rubio, Jan 20, 2020
  5. 07/29 bisect: libify `bisect_checkout`Miriam Rubio, Jan 20, 2020
  6. 08/29 bisect: libify `check_merge_bases` and its dependentsMiriam Rubio, Jan 20, 2020
  7. Johannes SchindelinJan 20, 2020
  8. Miriam R.Jan 21, 2020
  9. 04/29 run-command: make `exists_in_PATH()` non-staticMiriam Rubio, Jan 20, 2020
  10. 09/29 bisect: libify `check_good_are_ancestors_of_bad` and its dependentsMiriam Rubio, Jan 20, 2020
  11. Johannes SchindelinJan 20, 2020
  12. Christian CouderJan 21, 2020
  13. Miriam R.Jan 21, 2020
  14. 05/29 bisect--helper: introduce new `decide_next()` functionMiriam Rubio, Jan 20, 2020
  15. 10/29 bisect: libify `handle_bad_merge_base` and its dependentsMiriam Rubio, Jan 20, 2020
  16. Johannes SchindelinJan 20, 2020
  17. Christian CouderJan 21, 2020
  18. Miriam R.Jan 21, 2020
  19. 11/29 bisect: libify `bisect_next_all`Miriam Rubio, Jan 20, 2020
  20. Johannes SchindelinJan 20, 2020
  21. Christian CouderJan 21, 2020
  22. Johannes SchindelinJan 30, 2020
  23. 12/29 bisect--helper: reimplement `bisect_next` and `bisect_auto_next` shell functions in CMiriam Rubio, Jan 20, 2020
  24. Johannes SchindelinJan 30, 2020
  25. Miriam R.Jan 31, 2020
  26. Christian CouderFeb 17, 2020
  27. Johannes SchindelinFeb 17, 2020
  28. 13/29 bisect--helper: finish porting `bisect_start()` to CMiriam Rubio, Jan 20, 2020
  29. 06/29 bisect: libify `exit_if_skipped_commits` to `error_if_skipped*` and its dependentsMiriam Rubio, Jan 20, 2020
  30. Johannes SchindelinJan 20, 2020
  31. Christian CouderJan 21, 2020
  32. Miriam R.Jan 21, 2020
  33. 14/29 bisect--helper: retire `--bisect-clean-state` subcommandMiriam Rubio, Jan 20, 2020
  34. 15/29 bisect--helper: retire `--next-all` subcommandMiriam Rubio, Jan 20, 2020
  35. 17/29 bisect--helper: reimplement `bisect_state` & `bisect_head` shell functions in CMiriam Rubio, Jan 20, 2020
  36. 16/29 bisect--helper: reimplement `bisect_autostart` shell function in CMiriam Rubio, Jan 20, 2020
  37. 19/29 bisect--helper: retire `--write-terms` subcommandMiriam Rubio, Jan 20, 2020
  38. 18/29 bisect--helper: retire `--check-expected-revs` subcommandMiriam Rubio, Jan 20, 2020
  39. 20/29 bisect--helper: reimplement `bisect_log` shell function in CMiriam Rubio, Jan 20, 2020
  40. 21/29 bisect--helper: reimplement `bisect_replay` shell function in CMiriam Rubio, Jan 20, 2020
  41. 22/29 bisect--helper: retire `--bisect-write` subcommandMiriam Rubio, Jan 20, 2020
  42. 24/29 bisect--helper: retire `--bisect-autostart` subcommandMiriam Rubio, Jan 20, 2020
  43. 25/29 bisect--helper: retire `--bisect-auto-next` subcommandMiriam Rubio, Jan 20, 2020
  44. 23/29 bisect--helper: use `res` instead of return in BISECT_RESET case optionMiriam Rubio, Jan 20, 2020
  45. 26/29 bisect--helper: reimplement `bisect_skip` shell function in CMiriam Rubio, Jan 20, 2020
  46. 27/29 bisect--helper: retire `--check-and-set-terms` subcommandMiriam Rubio, Jan 20, 2020
  47. 28/29 bisect--helper: reimplement `bisect_visualize()`shell function in CMiriam Rubio, Jan 20, 2020
  48. 29/29 bisect--helper: reimplement `bisect_run` shell function in CMiriam Rubio, Jan 20, 2020
  49. Johannes SchindelinJan 20, 2020
  50. Christian CouderJan 20, 2020
  51. Johannes SchindelinJan 30, 2020
  52. Junio C HamanoJan 30, 2020
  53. Miriam R.Jan 21, 2020
  54. Johannes SchindelinJan 30, 2020

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.