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

[Eric Sunshine <sunshine@sunshineco.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.

> 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?

> +}