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

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

Linus Torvalds <torvalds@osdl.org> writes:

> Do what pack-objects.c does: just call "diff_delta()" and check the result 
> for NULL. If the result is NULL, then you have to do some special code, 
> because that means that it's a full create or a full delete (or it's an 
> unchanged empty file). Regardless, it really _is_ a special case, and it 
> would be silly to generate a delta for it.

When the result is NULL, it could be delta against empty, or
other failure in diff_delta() (could be it exceeded max_size,
could be it could not allocate memory, could be we introduced
some other failure modes later...).

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.  More importantly, for the updated
delta code that allows empty input to work, the codepaths the
various existing callers that check with NULL must not be
assuming non-NULL return means non empty input -- otherwise my
change would subtly break things -- and I do not have enough
energy to verify that right now.

Since we do not break files smaller than MINIMUM_BREAK_SIZE,
this becomes a non-issue with the attached patch.  I do not know
why I did not check both sides when I did it the first time; I
do not know why I was too stupid to notice that the earlier test
in the if() was far more expensive than the later one, either ;-).

-- >8 --
diff --git a/diffcore-break.c b/diffcore-break.c
index e6a468e..9b27456 100644
--- a/diffcore-break.c
+++ b/diffcore-break.c
@@ -55,12 +55,6 @@ static int should_break(struct diff_file
 			     * is the default.
 			     */
 
-	if (!S_ISREG(src->mode) || !S_ISREG(dst->mode))
-		return 0; /* leave symlink rename alone */
-
-	if (diff_populate_filespec(src, 0) || diff_populate_filespec(dst, 0))
-		return 0; /* error but caught downstream */
-
 	base_size = ((src->size < dst->size) ? src->size : dst->size);
 
 	delta = diff_delta(src->data, src->size,
@@ -169,9 +163,15 @@ void diffcore_break(int break_score)
 		if (DIFF_FILE_VALID(p->one) && DIFF_FILE_VALID(p->two) &&
 		    !S_ISDIR(p->one->mode) && !S_ISDIR(p->two->mode) &&
 		    !strcmp(p->one->path, p->two->path)) {
-			if (should_break(p->one, p->two,
-					 break_score, &score) &&
-			    MINIMUM_BREAK_SIZE <= p->one->size) {
+			
+			if (S_ISREG(p->one->mode) &&
+			    S_ISREG(p->two->mode) &&
+			    !diff_populate_filespec(p->one, 0) &&
+			    MINIMUM_BREAK_SIZE <= p->one->size &&
+			    !diff_populate_filespec(p->two, 0) &&
+			    MINIMUM_BREAK_SIZE <= p->two->size &&
+			    should_break(p->one, p->two,
+					 break_score, &score)) {
 				/* Split this into delete and create */
 				struct diff_filespec *null_one, *null_two;
 				struct diff_filepair *dp;