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

[Eric Sunshine <sunshine@sunshineco.com>]

On Fri, Jun 10, 2016 at 9:39 AM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
> On Fri, Jun 10, 2016 at 3:03 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>> 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.
>
> Sure I will squash and will explain it in the commit message.

Explain what in the commit message? If anything, I'd expect the commit
message to shrink since you won't need to explain anymore that this
function is split out.

>>> +       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.
>
> I wanted to imitate the code. But I guess it would actually be better
> if I drop this file_exists().

There is a bit of a lesson to be learned by this example. While it's
true that the C conversion should retain the behavior of the original
shell code, that does not mean blindly mirroring the implementation
line for line is a good idea. A couple things to take into
consideration:

There are idiomatic ways of doing things in each language. What is
idiomatic in shell is not necessarily so in C. The C conversion should
employ C idioms and flow in a way which is natural for C code.

Consider what the original shell code is doing at a higher level than
merely by reading it line-by-line. In the case in question, the code
is:

    test -f "$GIT_DIR/BISECT_EXPECTED_REV" &&
    test "$1" = $(cat "$GIT_DIR/BISECT_EXPECTED_REV")

While it's true that it's asking "does the file exist and is its value
the same as $1", the 'test -f' avoids a "file not found" error from
the $(cat ...) invocation. Since the return value of
strbuf_read_file() effectively encapsulates the "does the file exist"
check, a separate check isn't really needed.

>>> +       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?
>
> Definitely not. Thanks for pointing it out. :) It went off my head
> that strbuf_read_file returns the bytes it reads. Also the code
> comment regarding strbuf_read_file does not mention it which probably
> misguided me. I should also send a fixing patch so that someone else
> does not fall into this like I did.

Out of curiosity, did the test suite pass with this patch applied?
This is such an egregious bug that it's hard to imagine the tests
passing, but if they did, then that may be a good indication that
coverage is too sparse and ought to be improved.