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

From: Junio C Hamano <junkio@cox.net>
Date: Dec 12, 2005, 21:54 UTC
Message-ID: <7vek4igevq.fsf@assigned-by-dhcp.cox.net>
In-Reply-To: <Pine.LNX.4.64.0512121620380.26663@localhost.localdomain>

Nicolas Pitre <nico@cam.org> writes:

Show 6 quoted lines

>> 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.