Re: Delitifier broken (Re: diff-core segfault)

[Junio C Hamano <junkio@cox.net>]

Nicolas Pitre <nico@cam.org> writes:

>> This is not just "diff".  Our deltify code is half-broken, and
>> in the worst case this can corrupt our packs if an empty blob is
>> involved.
>
> I would say involving an empty blob with deltas _is_ the bug in the 
> first place.  Please don't let that happen.

Not all use of delta is to produce a pack.  An empty->empty
delta is a valid two byte \0\0 sequence, and I do not see any
reason to forbid it.  Although using such delta to represent
anything in a pack does *not* make any sense as you say, it
makes other callers simpler if they do not have to check if
from_len and to_len are empty before calling the delta code.
They care about from_len=0 (or to_len=0) case to produce similar
results as from_len=1 (or to_len=1) case and do not care at all
about the produced delta being a useful one for compressed
storage purposes.

> Especially with pack files, an empty blob can be represented with a 
> _single_ byte.  A delta must always be against something else and simply 
> storing the reference for the object the delta is against will always 
> use at least 20 bytes even for empty ones.

True, and the pack code is actually safe.  It punts on NULL
return, so my initial worry about packs turns out to be
unneeded.

> If my opinion is still of any weight I'd strongly vote for the former.  

I ended up doing both ;-).  The call site of diffcore-break was
certainly careless and broken (fixed); I've run git-grep to
check all callers to diff_delta() and the only one that did not
check the return value with NULL was the one that started with
thread.