From: Barret Rhoden <brho@google.com>
Date: Tue, 04 Jun 2019 16:38:02 GMT
Subject: Re: Git Test Coverage Report (Thursday, May 30th)
Message-ID: <9ab619bb-9deb-4e57-a3ad-9e996425b783@google.com>
In-Reply-To: <80a23fb8-5ea8-dba3-ce7d-f6f5d4c02310@gmail.com>

On 6/3/19 2:40 PM, Derrick Stolee wrote:
> On 6/3/2019 2:11 PM, Barret Rhoden wrote:
>> Hi -
>>
>> On 5/30/19 2:24 PM, Derrick Stolee wrote:
>>>> 8934ac8c 1190)     ent->ignored == next->ignored &&
>>>> 8934ac8c 1191)     ent->unblamable == next->unblamable) {
>>> These lines are part of this diff:
>>>
>>> --- a/blame.c
>>> +++ b/blame.c
>>> @@ -479,7 +479,9 @@ void blame_coalesce(struct blame_scoreboard *sb)
>>>
>>>           for (ent = sb->ent; ent && (next = ent->next); ent = next) {
>>>                   if (ent->suspect == next->suspect &&
>>> -                   ent->s_lno + ent->num_lines == next->s_lno) {
>>> +                   ent->s_lno + ent->num_lines == next->s_lno &&
>>> +                   ent->ignored == next->ignored &&
>>> +                   ent->unblamable == next->unblamable) {
>>>                           ent->num_lines += next->num_lines;
>>>                           ent->next = next->next;
>>>                           blame_origin_decref(next->suspect);
>>>
>>> The fact that they are uncovered means that the && chain is short-circuited at
>>> "ent->s_lno + ent->num_lines == next->s_lno" before the new conditions can be
>>> checked. So, the block inside is never covered. It includes a call to
>>> blame_origin_decref() and free(), so it would be good to try and exercise this region.
>>
>> What is your setup for determining if a line is uncovered?  Are you running something like gcov for all of the tests in t/?
>>
>> I removed this change, and none of the other blame tests appeared to trigger this code block either, independently of this change.  (I put an assert(0) inside the block).
>>
>> However, two of our blame-ignore tests do get past the first two checks in the if clause, (the suspects are equal and the s_lno chunks are adjacent) and we do check the ignored/unblamable conditions.
>>
>> Specifically, if I undo this change and put an assert(0) in that block, two of our tests hit that code, and one of our tests fails if I don't do the check for ignored/unblamable.
> 
> The tests use gcov while running the tests in t/. Here is the build [1].
> 
> There are some i/o errors happening in the build, which I have not
> full diagnosed. It is entirely possible that you actually are covered,
> but there was an error collecting the coverage statistics. The simplest
> thing to do is to insert a die() statement and re-run the tests.

It looks like no existing tests cover that block in blame_coalesce(), 
regardless of my commit.  That's based on putting die() in there and 
running make in t/.  So at the worst, my patch isn't decreasing 
coverage.  That's a pretty low bar.  =)

I'll try to come up with a test, independent of my blame-ignore work, 
that can get in that block.

Thanks,

Barret