Re: [PATCH v2 11/16] packfile: always add packfiles to MRU when adding a pack

> 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.