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

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