Re: [Outreachy][PATCH v2 00/11] Finish converting git bisect to C part 1

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi Miriam,

On Tue, 28 Jan 2020, Miriam Rubio wrote:

> These patches correspond to a first part of patch series
> of Outreachy project "Finish converting `git bisect` from shell to C"
> started by the interns Pranit Bauva and Tanushree Tumane
> (https://public-inbox.org/git/pull.117.git.gitgitgadget@gmail.com) and
> continued by Miriam Rubio.
>
> This first part are formed of preparatory/clean-up patches and all
> `bisect.c` libification work.
>
> These patch series emails were generated from:
> https://gitlab.com/mirucam/git/commits/git-bisect-work-part1
>
> It has to be noted that in this version 2 nothing has been done about a
> reviewer suggestion of using enums for error codes, because there was no
> consensus about using them by the reviewers.

It is a pity, as "magic" constants tend to remain "magic" (and eventually
"tragic" when they have not been addressed properly).

However, this does not need to hold up your patch series.

I noted a couple of rather minor things; It looks pretty good to me
already, though, and I thank you very much for splitting this patch series
off, so that only the libification remains. It made for a pleasant read.

Thanks,
Dscho

>
> --- Changes since v1 Finish converting git bisect to C part 1 patch series ---
>
> General changes
> ---------------
>
> * Previous patch series version has been split in smaller groups
> in order to facilitate revision and integration.
> * Rebase on master branch: c7a6207591 (Sync with maint, 2020-01-27).
> * Improve commit messages.
>
> Specific changes
> ----------------
>
> [6/11] bisect: libify `exit_if_skipped_commits` to `error_if_skipped*`
> and its dependents
>
> * Remove redundant sentences in commit message.
> * Use `if (res < 0)` instead of `if (res)`.
>
> [8/11] bisect: libify `check_merge_bases` and its dependents
>
> * Remove redundant sentence in commit message.
>
> --
>
> [9/11] bisect: libify `check_good_are_ancestors_of_bad` and its
> dependents
>
> * Remove redundant sentences in commit message.
> * Return in `if (!current_bad_oid)` condition.
>
> --
>
> [10/11] bisect: libify `handle_bad_merge_base` and its dependents
>
> * Remove redundant sentence in commit message.
>
> --
>
> [11/11] bisect: libify `bisect_next_all`
>
> * Remove redundant sentence in commit message.
> * Add return codes explanations in `bisect.h`.
>
> --
>
> Miriam Rubio (2):
>   bisect--helper: convert `vocab_*` char pointers to char arrays
>   bisect: use the standard 'if (!var)' way to check for 0
>
> Pranit Bauva (7):
>   run-command: make `exists_in_PATH()` non-static
>   bisect: libify `exit_if_skipped_commits` to `error_if_skipped*` and
>     its dependents
>   bisect: libify `bisect_checkout`
>   bisect: libify `check_merge_bases` and its dependents
>   bisect: libify `check_good_are_ancestors_of_bad` and its dependents
>   bisect: libify `handle_bad_merge_base` and its dependents
>   bisect: libify `bisect_next_all`
>
> Tanushree Tumane (2):
>   bisect--helper: change `retval` to `res`
>   bisect--helper: introduce new `decide_next()` function
>
>  bisect.c                 | 136 +++++++++++++++++++++++++++------------
>  bisect.h                 |  23 +++++++
>  builtin/bisect--helper.c | 118 +++++++++++++++++----------------
>  run-command.c            |   2 +-
>  run-command.h            |  11 ++++
>  5 files changed, 193 insertions(+), 97 deletions(-)
>
> --
> 2.21.1 (Apple Git-122.3)
>
>