unpack_entry (was: [BUG] commit walk machinery is dangerous !)

From: Shawn O. Pearce <spearce@spearce.org>
Date: Jul 15, 2008, 05:10 UTC
Message-ID: <20080715051056.GI2432@spearce.org>
In-Reply-To: <7vod50dote.fsf@gitster.siamese.dyndns.org>

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

> This is unrelated to the issue at hand, but I also notice that there are
> few callsites outside sha1_file.c that bypasses cache_or_unpack_entry()
> and call unpack_entry() directly.  I wonder if they should be using the
> cached version, making unpack_entry() static...

There are two such callsites that I found:

$ git grep -n --cached unpack_entry fast-import.c:1201: return unpack_entry(p, oe->offset, &type, sizep); pack-check.c:114: data = unpack_entry(p, entries[i].offset, &type,

Now the one in fast-import.c could be using the cached version. This is simply fast-import trying to reuse sha1_file.c for reading a previously written object to the pack.

Stuffing the item into the cache is perhaps pointless here as gfi does its own sort of caching and only goes through this code path when that caching has tossed the least-recently-used data and its suddenly needed again.

So yea, it could be using the caching form, but it would maybe be doing more work (and using more memory) than it really needs.

In pack-check.c we are looping through the objects in the order they appear in the index so we can unpack them and verify each object's SHA-1 signature. Please note this is perhaps the slowest way to enumerate through the pack and is why you can clone a repository over git:// faster than you can run `fsck --full`.

If you really want to verify every single object's SHA-1, run the damn pack through index-pack and compare the new index to the old index (hint they should be identical, bit-for-bit).

So again, in this case the caching may only cause us to do more work (and use more memory) than we need as we are slamming the delta base cache with all sorts of unrelated base objects. It probably has a low hit ratio anyway during this loop. :-|

-- 
Shawn.