Re: [PATCH v2 05/16] blame: accept multiple -L ranges

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Aug 6, 2013, 22:44 UTC
Message-ID: <CAPig+cTHbV-ijfXNBn45v2opOH7Jh0A22QaxCeKhRBPA39xAwQ@mail.gmail.com>
In-Reply-To: <7v1u66fqrz.fsf@alter.siamese.dyndns.org>

On Tue, Aug 6, 2013 at 5:33 PM, Junio C Hamano <gitster@pobox.com> wrote:

Show 19 quoted lines

>> +     for (range_i = ranges.nr; range_i > 0; --range_i) {
>> +             const struct range *r = &ranges.ranges[range_i - 1];
>> +             long bottom = r->start;
>> +             long top = r->end;
>> +             struct blame_entry *next = ent;
>> +             ent = xcalloc(1, sizeof(*ent));
>> +             ent->lno = bottom;
>> +             ent->num_lines = top - bottom;
>> +             ent->suspect = o;
>> +             ent->s_lno = bottom;
>> +             ent->next = next;
>> +             if (next)
>> +                     next->prev = ent;
>> +             origin_incref(o);
>> +     }
>> +     origin_decref(o);
>
> Hmph, I do not see where the need for this decref is coming from.
> Did we incref once too many somewhere?

Each constructed blame_entry must own a reference to the suspect. o->refcnt should equal the number of blame_entries. At construction, a 'struct origin' has refcnt 1. In the original code, which supported only a single initial range (blame_entry), we had:

  o = get-initial-suspect();  # refcnt already 1
  ent->suspect = o;  # refcnt still 1
  sb.ent = ent;
  assign_blame(&sb);

So, o->refcnt equals the number of blame_entries (1) when assign_blame() is called.

The new for-loop calls origin_incref() on each iteration since each blame_entry needs to own a reference to the suspect. Assume that we have two disjoint -L ranges:

  o = get-initial-suspect();  # refcnt already 1
  foreach range:
    ent = new blame_entry;
    ent->suspect = o;
    origin_incref(o);  # refcnt++
  end
  # for 2 ranges, refcnt incremented twice, so value is 3
  origin_decref(o);  # refcnt = 2
  sb.ent = ent;
  assign_blame(&sb);

Thus, as with the original code, o->refcnt equals the number of blame_entries (2) when assign_blame() is called.

The same holds for the boundary case when the file is empty and there is no range. o->refcnt starts at 1, the loop is never entered so no blame_entries are created, and o->refcnt gets decremented to 0, which again matches the number of blame_entries (0).