On Wed, Jun 8, 2016 at 11:24 AM, Pranit Bauva <pranit.bauva@xxxxxxxxx> 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@xxxxxxxxx>
> ---
> 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?
> +}
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html