Re: [PATCH v14 08/27] bisect--helper: `is_expected_rev` & `check_expected_revs` shell function in C

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 24, 2016, 22:13 UTC
Message-ID: <xmqqlgzlolwa.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <01020156b73fe69f-6b734e3c-806c-431d-aa52-c96b3783f248-000000@eu-west-1.amazonses.com>

Pranit Bauva <pranit.bauva@gmail.com> writes:

Show 7 quoted lines

> +static int is_expected_rev(const char *expected_hex)
> +{
> +	struct strbuf actual_hex = STRBUF_INIT;
> +	int res = 0;
> +	if (strbuf_read_file(&actual_hex, git_path_bisect_expected_rev(), 0) >= 0) {
> +		strbuf_trim(&actual_hex);
> +		res = !strcmp(actual_hex.buf, expected_hex);

If it is known to have 40-hex:

 (1) accepting ">= 0" seems way too lenient.  You only expect a
     41-byte file (or 42 if somebody would write CRLF, but I do not
     think anybody other than yourself is expected to write into
     this file, and you do not write CRLF yourself);

 (2) strbuf_trim() is overly loose.  You only want to trim the
     terimnating LF and it is an error to have other trailing
     whitespaces.

I think the latter is not a new problem and it is OK to leave it as-is; limiting (1) to >= 40 may still be a good change, though, because it makes the intention of the code clearer.