From: Derrick Stolee <stolee@gmail.com>
Date: Tue, 01 Sep 2026 17:12:44 GMT
Subject: Re: [PATCH v3 4/4] packfile: recover when a multi-pack-index names a removed pack
Message-ID: <2729941e-c682-42dd-ac82-9d59c9c9668e@gmail.com>
In-Reply-To: <CABPp-BEK8f4Dh=3z-Q768iBV-d-wdpXGSKhsfFacGwHEFabZKA@mail.gmail.com>

On 9/1/2026 12:47 PM, Elijah Newren wrote:
> On Tue, Sep 1, 2026 at 8:26 AM Derrick Stolee <stolee@gmail.com> wrote:
>>
>> On 8/29/2026 3:00 AM, Elijah Newren via GitGitGadget wrote:
>>> From: Elijah Newren <newren@gmail.com>
>>
>> I'm late in reviewing this patch, so forgive me responding inline as
>> I discover how it works.
>>
>> tl;dr: Good patch. LGTM.
> 
> Thanks for taking a look; I wanted to point out two minor clarifications...
> 
>>> +     /*
>>> +      * Recovery for a concurrent-repack race: a stale MIDX may still name a
>>> +      * vanished owning pack even though the object survives in another pack
>>> +      * the same MIDX covers.  The regular fallback above skips MIDX-covered
>>> +      * packs, and repreparing the on-disk pack set does not reload the
>>> +      * borrowed, cached MIDX, so scan its packs directly for the survivor.
>>> +      *
>>> +      * Do this only on the second read, by which point repreparing packs has
>>> +      * already had a chance to find an object merely relocated into a new,
>>> +      * uncovered pack; only a genuine hidden duplicate reaches here.
>>> +      */
>>
>> This comment does a lot of important context-setting to show
>> that we are in a very narrow case: the stale MIDX has multiple
>> packs that contain the requested object, but the "newer" one
>> was deleted without creating a new packfile, so we need to
>> look at each contained pack for the object from its pack-index.
> 
> Actually, a new packfile is typically created, it just doesn't have
> the object in question -- and doesn't need to, because a pre-existing
> (also midx-covered) pack already has it.

Thanks. That helps me understand why this can occur regularly
enough to be triggered in the wild.

>>> +     if (midx_result == MIDX_FILL_OWNER_UNAVAILABLE &&
>>> +         (flags & OBJECT_INFO_SECOND_READ)) {
>>> +             struct multi_pack_index *m = store->midx;
>>> +             uint32_t i;
>>> +
>>> +             for (i = 0; i < m->num_packs + m->num_packs_in_base; i++) {
>>> +                     struct packed_git *p;
>>> +
>>> +                     if (prepare_midx_pack(m, i))
>>> +                             continue;
>>> +                     p = nth_midxed_pack(m, i);
>>> +                     if (p && packfile_fill_entry(p, oid, e, bad_pack))
>>> +                             return 1;
>>> +             }
>>> +     }
>>> +
>>
>> This is hopefully a very rare case, but it's good to have
>> this "fall back to O(num packs)" situation.
> 
> It's actually a fall back to O(num_packs_in_the_midx); on developer
> laptops that's probably about the same as O(num_packs), but on busy
> servers constantly receiving pushes, the total number of packs often
> dwarfs the number of packs in the midx.

Thanks. You're absolutely right that I was not specific enough and
in server situations this loop will be very short.

Thanks,
-Stolee