Re: fact-import: failed to apply delta

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 10, 2009, 22:47 UTC
Message-ID: <7vfxillxiu.fsf@gitster.siamese.dyndns.org>
In-Reply-To: <7vprhqkjrr.fsf@gitster.siamese.dyndns.org>

Junio C Hamano <gitster@pobox.com> writes:

Show 28 quoted lines

> "Shawn O. Pearce" <spearce@spearce.org> writes:
>
>> Daniel Barkalow <barkalow@iabervon.org> wrote:
>>> On Tue, 10 Feb 2009, Shawn O. Pearce wrote:
>>> > 
>>> > We should dump the cached_objects table in sha1_file.c during
>>> > a checkpoint in fast-import.
>>> 
>>> No, that one's keyed by sha1, and doesn't get collisions; it's the 
>>> delta_base_cache that's the issue; it's keyed by struct packed_git * and 
>>> offset.
>>
>> Uh, yea, I realize that after I sent the message.  Does this patch
>> fix it for you?
>>
>> --8<--
>> Clear the delta base cache during fast-import checkpoint
>>
>> Otherwise we may reuse the same memory address for a totally
>> different "struct packed_git", and a previously cached object from
>> the prior occupant might be returned when trying to unpack an object
>> from the new pack.
>
> Can this be made more automatic?
>
> For example if you do this every time a new pack is installed to
> sha1_file(), like in add_packed_git() perhaps, wouldn't that be much less
> error prone?

On second thought, I think fast-import is the only program that plays funny games of feeding a packed_git that is *not* part of the real list of packed_git installed in the system to unpack_entry(), so probably your patch is a better idea.

We really need a better comment in the codepath from gfi_unpack_entry() to unpack_entry() that there is a very unusual thing going on.

By the way, strictly speaking, you need to release the delta_base_cache entries that is based on pack_data and nothing else, no?