From: Patrick Steinhardt <ps@pks.im>
Date: Thu, 30 Oct 2025 08:59:12 GMT
Subject: Re: [PATCH 5/8] builtin/pack-objects: simplify logic to find kept or nonlocal objects
Message-ID: <aQMo4PqG_U6JIkOt@pks.im>
In-Reply-To: <875xbxrc4q.fsf@iotcl.com>

On Wed, Oct 29, 2025 at 03:55:17PM +0100, Toon Claes wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> 
> > The function `has_sha1_pack_kept_or_nonlocal()` takes an object ID and
> > then searches through packed objects to figure out whether the object
> > exists in a kept or non-local pack. As a performance optimization we
> > remember the packfile that contains a given object ID so that the next
> > call to the function first checks that same packfile again.
> >
> > The way this is written is rather hard to follow though, as the caching
> > mechanism is intertwined with the loop that iterates through the packs.
> > Consequently, we need to do some gymnastics to re-start the iteration if
> > the cached pack does not contain the objects.
> 
> Okay, this took me while, but yes this function was really hard to
> understand. Thanks for simplifying.
> 
> Naive question, what's the point of keeping a "last_found"? We have one
> global "last_found" for the last time this function was called, and we
> have no control which OIDs get passed to this function. Why look into
> "last_found" first?

I guess it's just a micro-optimization. I'm sure it exists for a reason,
but honestly I didn't feel like opening that can of worms. The caching
just made me scratch my head in subsequent refactorings, so I cared more
about making it maintainable than questioning its existence.

Patrick