Re: [PATCH v2 7/7] unpack-trees: free cache_entry array members for merges

From: René Scharfe <rene.scharfe@lsrfire.ath.cx>
Date: Jun 2, 2013, 20:26 UTC
Message-ID: <51ABAA84.8090301@lsrfire.ath.cx>
In-Reply-To: <CAMP44s0Af90Sfi47_mxEGRFHAsAhaoO6T1jjbj7SptucNO-k0w@mail.gmail.com>

Am 02.06.2013 19:59, schrieb Felipe Contreras:

Show 25 quoted lines

> On Sun, Jun 2, 2013 at 12:54 PM, René Scharfe
> <rene.scharfe@lsrfire.ath.cx> wrote:
>> Am 02.06.2013 19:25, schrieb Felipe Contreras:
>>>
>>> On Sun, Jun 2, 2013 at 10:46 AM, René Scharfe
>>> <rene.scharfe@lsrfire.ath.cx> wrote:
>>>>
>>>> +               for (i = 0; i < n; i++) {
>>>> +                       struct cache_entry *ce = src[i + o->merge];
>>>> +                       if (ce != o->df_conflict_entry)
>>>
>>>
>>> It's possible that ce is NULL, but you didn't add that check because
>>> free(NULL) still works? Or because ce cannot be NULL?
>>>
>>> If it's the former, I think it's clearer if we check that ce is not
>>> NULL either way.
>>
>>
>> It is NULL if one tree misses an entry (e.g. a new or removed file). free
>> handles NULL and we generally avoid duplicating its NULL-check.
>
> Yeah, but I can see somebody adding code inside that 'if' clause to
> print the cache entry, and see a crash only to wonder what's going on.
> And to save what? 5 characters?

The person adding code that depends on ce not being NULL needs to add that check as well. Let's not worry too much about future changes that may or (more likely IMHO) may not be done. The test suite covers this case multiple times, so such a mistake doesn't have a realistic chance to hit master.

René