From: Patrick Steinhardt <ps@pks.im>
Date: Tue, 02 Sep 2025 08:50:48 GMT
Subject: Re: [PATCH v2 11/16] packfile: always add packfiles to MRU when adding a pack
Message-ID: <aLav6D4PhIHsUd36@pks.im>
In-Reply-To: <aK5ZmNxE4zhNt7Zg@nand.local>

On Tue, Aug 26, 2025 at 09:04:24PM -0400, Taylor Blau wrote:
> On Thu, Aug 21, 2025 at 09:39:09AM +0200, Patrick Steinhardt wrote:
> > When adding a packfile to it store we add it both to the list and map of
> > packfiles, but we don't append it to the most-recently-used list of
> > packs. We do know to add the packfile to the MRU list as soon as we
> > access any of its objects, but in between we're being inconistent. It
> > doesn't help that there are some subsystems that _do_ add the packfile
> > to the MRU after having added it, which only adds to the confusion.
> >
> > Refactor the code so that we unconditionally add packfiles to the MRU
> > when adding them to a packfile store.
> 
> I am a little confused why prepare_midx_pack() wants to add packs to the
> MRU cache so eagerly, and the commit which introduced that behavior
> (commit af96fe3392 (midx: add packs to packed_git linked list,
> 2019-04-29)) doesn't focus on that area in detail.
> 
> (Note that commit af96fe3392 *does* discuss a separate cache's behavior
> regarding the open file descriptor limit, but that LRU cache is a
> different one than the MRU cache we're discussing here.)
> 
> What I do wonder about is why af96fe3392 adds packs to the MRU cache in
> the first place. As far as I can tell, we never move MIDX'd packs to
> the front of the MRU cache at all. There are two spots that call
> list_move() on the MRU cache, which are:
> 
>  - packfile.c::find_pack_entry(), which enumerates MIDX'd
>    packs in a separate loop earlier on in the function, and ignores
>    packs in the MRU cache whose p->multi_pack_index bit is set.
> 
>  - builtin/pack-objects.c::want_object_in_pack_mtime(), which also
>    enumerates MIDX'd packs in a separate loop, though it does not
>    explicitly ignore packs in the MRU cache with the multi_pack_index
>    bit set.
> 
> In practice, though, I think these two are equivalent, since
> want_object_in_pack_mtime() will return before it gets to the MRU cache
> if it found the object in a MIDX'd pack.
> 
> So I don't think we need to be adding MIDX'd packs to the MRU cache in
> the first place.

I think the status quo is quite confusing. There are callers which
directly iterate through the list of packfiles in MRU order, and that
list is not guaranteed right now to even contain all packfiles that are
tracked in the packfile store. The list is complete when we only load
packfiles from disk, but if we ever manually add a packfile to the store
in-memory the list is not up-to-date anymore. I also couldn't find a
reason for that distinction.

Despite being confusing, there's another motivation here as discussed
with Peff in [1]: we can drop the distinction between the MRU list and
the "normal" list altogether. Ensuring that all packfiles are always
stored in the MRU is a prerequisite for that subsequent change. I
already got a patch series pending that does this refactoring.

That being said, I wouldn't mind moving this change into that subsequent
patch series, either. It doesn't really have a strong reason to exist
yet, but once we remove the distinction between the two packfile lists
we have a much stronger argument.

[1]: <20250820192008.GA1662788@coredump.intra.peff.net>

Patrick