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

["René Scharfe" <rene.scharfe@lsrfire.ath.cx>]

Am 03.06.2013 02:04, schrieb Felipe Contreras:
> On Sun, Jun 2, 2013 at 6:47 PM, René Scharfe
> <rene.scharfe@lsrfire.ath.cx> wrote:
>> Am 03.06.2013 01:23, schrieb Felipe Contreras:
>>
>>> I didn't say we should do 'if (ce) free(ce);' instead of 'free(ce);' I
>>> said we should do 'if (cd && ce != o->df_conflict_entry)' instead of
>>> 'if (ce != o->df_conflict_entry)'.
>>
>>
>> I did assume you meant the latter.
>>
>>
>>> There's no reason not to.
>>
>>
>> Only the minor ones already mentioned: More text,
>
> Five characters.
>
>> one more branch in object
>> code,
>
> Which might actually be more optimal.

The difference in absolute numbers will most certainly be within the 
noise for this one case.

>> no benefit except for some hypothetical future case that's caught by
>> the test suite anyway -- or by code review.
>
> That's not the benefit, the benefit is that the code is clearer.

I don't see that, and I don't like adding a check that I don't expect to 
be ever needed.  Users are safe because the test suite is going to catch 
a missing check.

In general, I think those who introduce dependencies should add the 
necessary checks.  They have to consider the invariants anyway, no 
matter how many checks you add beforehand for their convenience.

>> I wonder if we already reached the point where we spent more time discussing
>> this change than the time needed by the envisioned developer to find and fix
>> the NULL check that suddenly became necessary. :)
>
> Maybe, but if what you want is to avoid the discussion, you could just
> add the extra five characters and be done with it.

Or you could submit a patch on top that adds the check.  I'd send it out 
if you'd supply a commit message.  My review comment would be "mostly 
harmless, but I don't like it very much because it's not needed now and 
probably won't ever".

But I'm more interested in a different way forward: Would it make sense 
to push the allocations (and frees) into the merge functions?  Currently 
they duplicate one of the cache entries.  Would the merge functions 
become too ugly or too big if they'd have to build them themselves, 
avoiding duplication?  Would it be significantly faster?

René