Re: Delitifier broken (Re: diff-core segfault)
- From
Junio C Hamano <junkio@cox.net>
- Date
- Dec 13, 2005, 01:08 UTC
- Message-ID
- <7vlkypdcsb.fsf@assigned-by-dhcp.cox.net>
- In-Reply-To
- <Pine.LNX.4.64.0512121529200.15597@g5.osdl.org>
Linus Torvalds <torvalds@osdl.org> writes:
Show 5 quoted lines
> 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;