Re: [PATCH v2 5/6] bisect--helper: `is_expected_rev` & `check_expected_revs` shell function in C

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Jun 15, 2016, 21:14 UTC
Message-ID: <CAPig+cRNcg496Ty2SJ1ojm1n3OpHzX0obMAD2bY3AzoyrKdA9w@mail.gmail.com>
In-Reply-To: <20160615140026.10519-6-pranit.bauva@gmail.com>

On Wed, Jun 15, 2016 at 10:00 AM, Pranit Bauva <pranit.bauva@gmail.com> wrote:

Show 23 quoted lines

> Reimplement `is_expected_rev` & `check_expected_revs` shell function in
> C and add a `--check-expected-revs` subcommand to `git bisect--helper` to
> call it from git-bisect.sh .
> [...]
> Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
> ---
> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
> @@ -162,13 +162,44 @@ static int bisect_reset(const char *commit)
> +static int is_expected_rev(const char *expected_hex)
> +{
> +       struct strbuf actual_hex = STRBUF_INIT;
> +       int res;
> +
> +       if (strbuf_read_file(&actual_hex, git_path_bisect_expected_rev(), 0) < 0) {
> +               strbuf_release(&actual_hex);
> +               return 0;
> +       }
> +
> +       strbuf_trim(&actual_hex);
> +       res = !strcmp(actual_hex.buf, expected_hex);
> +       strbuf_release(&actual_hex);
> +       return res;
> +}

Not worth a re-roll, but this could be re-structured to avoid having to remember to release the strbuf at all exits:

    struct strbuf actual_hex = ...;
    int res = 0;

    if (strbuf_read_file(...) >= 0) {
        strbuf_trim(...);
        res = !strcmp(...);
    }
    strbuf_release(...);
    return res;

Alternately:

    if (strbuf_read_file(...) < 0)
        goto done;

    strbuf_trim(...);
    res = !strcmp(...);

done:
    strbuf_release(...);
    return res;

which is a bit less compact.

Show 13 quoted lines

> +static int check_expected_revs(const char **revs, int rev_nr)
> +{
> +       int i;
> +
> +       for (i = 0; i < rev_nr; i++) {
> +               if (!is_expected_rev(revs[i])) {
> +                       remove_path(git_path_bisect_ancestors_ok());
> +                       remove_path(git_path_bisect_expected_rev());
> +                       return 0;
> +               }
> +       }
> +       return 0;
> +}

Hmm, all execution paths return 0, so it feels a bit pointless to have this function return a value at all.

You could also use a 'break' inside the loop rather than 'return' since the return value is the same inside or outside the loop and nothing else happens after the loop.