Re: [PATCH 1/2] bisect--helper: `is_expected_rev` shell function in C
On Fri, Jun 10, 2016 at 9:39 AM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
Show 10 quoted lines
> 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.
Show 9 quoted lines
>>> + 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.
Show 25 quoted lines
>>> + 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.
Pranit Bauva