Pranit Bauva <pranit.bauva@xxxxxxxxx> writes:
> 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.
There is no need for that, as that was present in the original. The
original compared the string with something that is known to be
40-hex, so anything shorter than 40 wouldn't have matched.
It is merely a trivial and obvious optimization to do that in C
rewrite.