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

From: Linus Torvalds <torvalds@osdl.org>
Date: Dec 13, 2005, 01:34 UTC
Message-ID: <Pine.LNX.4.64.0512121720150.15597@g5.osdl.org>
In-Reply-To: <7vlkypdcsb.fsf@assigned-by-dhcp.cox.net>

On Mon, 12 Dec 2005, Junio C Hamano wrote:

Show 6 quoted lines

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