Hey Junio,
On Thu, Aug 25, 2016 at 3:43 AM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
> Pranit Bauva <pranit.bauva@xxxxxxxxx> writes:
>
>> +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.
I can do the first change. Since this wasn't present in the shell
code, I will also mention it in the commit message.
Regards,
Pranit Bauva
--
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