From: Elijah Newren <newren@gmail.com>
Date: Fri, 28 Aug 2026 07:29:49 GMT
Subject: Re: [PATCH v2 4/4] packfile: recover when a multi-pack-index names a removed pack
Message-ID: <CABPp-BFhPONjNuVZQfgwKuYdgbm5Fjjttz5q5wSYX6j1Zdwdww@mail.gmail.com>
In-Reply-To: <20260827060622.GC189659@coredump.intra.peff.net>

On Wed, Aug 26, 2026 at 11:06 PM Jeff King <peff@peff.net> wrote:
>
> On Tue, Aug 25, 2026 at 07:00:29PM +0000, Elijah Newren via GitGitGadget wrote:
>
> > diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
> > index 399acd0f22..30ad7d822c 100644
> > --- a/builtin/pack-objects.c
> > +++ b/builtin/pack-objects.c
> > @@ -1786,7 +1786,7 @@ static int want_object_in_pack_mtime(const struct object_id *oid,
> >               struct multi_pack_index *m = get_multi_pack_index(files->packed);
> >               struct pack_entry e;
> >
> > -             if (m && fill_midx_entry(m, oid, &e, NULL)) {
> > +             if (m && fill_midx_entry(m, oid, &e, NULL) == MIDX_FILL_HIT) {
> >                       want = want_object_in_pack_one(e.p, oid, exclude, found_pack, found_offset, found_mtime);
> >                       if (want != -1)
> >                               return want;
>
> We've changed the return value semantics without changing the signature
> (or name). So we need to make sure we adjust all callers, as here.
> That's _probably_ OK in practice for such a specialized function. But we
> could also rename it if we wanted to be paranoid (especially about
> new callers added on parallel branches).

Any suggestions for alternate names?  fill_midx_entry_result?  midx_fill_entry?

> > +enum midx_fill_result fill_midx_entry(struct multi_pack_index *m,
> > +                                   const struct object_id *oid,
> > +                                   struct pack_entry *e,
> > +                                   struct packed_git **bad_pack)
>
> OK, so this is our tri-state fix. Mostly looks as expected, though:
>
> >       if (prepare_midx_pack(m, pack_int_id))
> > -             return 0;
> > +             goto owner_unavailable;
>
> I'd have expected just "return MIDX_FILL_OWNER_UNAVAILABLE" here. But
> then, I'm not sure I buy the need for this stale_packs_detected stuff
> from patch 3.

Yeah, with the drop of patch 3 it becomes that.

> >       p = m->packs[pack_int_id - m->num_packs_in_base];
> >
> > -     /*
> > -     * We are about to tell the caller where they can locate the
> > -     * requested object.  We better make sure the packfile is
> > -     * still here and can be accessed before supplying that
> > -     * answer, as it may have been deleted since the MIDX was
> > -     * loaded!
> > -     */
> > +     /* Make sure the pack is still present before pointing at it. */
> >       if (!is_pack_valid(p))
> > -             return 0;
> > +             goto owner_unavailable;
>
> This comment rewrite seems superfluous at best. Can we try to keep such
> patch fluff to a minimum?

Yes, sorry.

> > +     /*
> > +      * 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.
> > +      */
> > +     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;
> > +             }
> > +     }
>
> OK, and this is as-before but now gated on the SECOND_READ flag. As
> expected in this revision.

Thanks for taking a look!