Re: [PATCH 1/2] bisect--helper: `is_expected_rev` shell function in C

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Jun 9, 2016, 21:33 UTC
Message-ID: <CAPig+cRC_y9MBSsrLEs0OxL4=FMfU-=ACwhESUiECrWrDNvaCw@mail.gmail.com>
In-Reply-To: <20160608152415.7770-1-pranit.bauva@gmail.com>

On Wed, Jun 8, 2016 at 11:24 AM, Pranit Bauva <pranit.bauva@gmail.com> wrote:

> Reimplement `is_expected_rev` shell function in C. This will further be
> called from `check_expected_revs` function. This is a quite small
> function thus subcommand facility is redundant.

This patch should be squashed into patch 2/2, as it is otherwise pointless without that patch, and merely adds dead code.

Show 10 quoted lines

> Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
> ---
> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
> @@ -160,6 +160,20 @@ int bisect_reset(const char *commit)
> +static int is_expected_rev(const char *expected_hex)
> +{
> +       struct strbuf actual_hex = STRBUF_INIT;
> +
> +       if (!file_exists(git_path_bisect_expected_rev()))
> +               return 0;

Invoking file_exists() seems unnecessarily redundant when you can discern effectively the same by checking the return value of strbuf_read_file() below. I'd drop the file_exists() check altogether.

> +       if (!strbuf_read_file(&actual_hex, git_path_bisect_expected_rev(), 0))
> +               return 0;

What exactly is this trying to do? Considering that strbuf_read_file() returns -1 upon error, otherwise the number of bytes read, if I'm reading this correctly, is_expected_rev() returns false if strbuf_read_file() encounters an error (which is fine) but also when it successfully reads the file and its content length is non-zero (which is very odd).

> +       strbuf_trim(&actual_hex);
> +       return !strcmp(actual_hex.buf, expected_hex);

Thus, it only ever gets to this point if the file exists but is empty, which is very unlikely to match 'expected_hex'. I could understand it if you checked the result of strbuf_read_file() with <0 or even <=0, but the current code doesn't make sense to me.

Am I misunderstanding?

> +}