From: Linus Torvalds Date: Tue, 13 Dec 2005 01:34:28 GMT Subject: Re: Delitifier broken (Re: diff-core segfault) Message-ID: In-Reply-To: <7vlkypdcsb.fsf@assigned-by-dhcp.cox.net> On Mon, 12 Dec 2005, Junio C Hamano wrote: > > I'll revert the changes anyway, but not because I necessarily > agree with you two. I am not 100% confident that the core of > the diff_delta code would work fine with empty input (it seems > to from my limited test), and I do not want to break things > unnecessarily at this point. Well, I checked the pack-objects.c side, and your patch to diff_delta() should not hurt at least there. We already check the size and would have broken out long before if either side was zero-sized. But that's kind of part of the point - any user of diff_delta() is likely to have checked the size anyway for other reasons. There's just very seldom any valid reason to generate a delta against an empty file, there's no interesting information that diff_delta() can really give us. Basically, the binary diffs that diff-delta returns are interesting for just two things: - efficient packing, in the pack-objects.c style. As mentioned, pack-objects.c needs to check the size heuristics before doing diff_delta() _anyway_, for performance reasons as well as simply because the secondary use of diff_delta() is to estimate how big the delta is, and it's always pointless to generate a delta that is guaranteed to be bigger than the file (which is always the case with either side being an empty file - the size difference will inevitably be bigger than the size of the resulting file). - difference size estimation (ie for rename/copy detection) This boils down to the same case as the secondary use of pack-objects, ie delta size estimation. Again, if either side is empty, we _know_ that the delta generation is pointless, because the delta is always going to be bigger than the end result, and thus it can't be sensible for rename/copy detection. So in one sense I actually agree with your patch: it makes the deltifier code more generic and actually simplifies the diff_delta() code a bit by avoiding one special case, and in that sense it's a good change. So the reason I disagree with it is that doing the delta is always going to be unnecessary work. And regardless of how we're ever going to use the delta, we _know_ that it's unnecessary work. So I think your diffcore-break.c patch is much more appropriate: it also fixes the bug, but it fixes it by virtue of realizing that the delta cannot matter and thus should never even be computed. Now, your diff_setup() change may actually be worth it because of the simplification, but on the other hand, you can also consider the NULL return as being nice because it's effectively a way of saying "the delta is meaningless, why did you even ask me?" Linus