From: Patrick Steinhardt <ps@pks.im>
Date: Wed, 10 Dec 2025 09:40:10 GMT
Subject: Re: [PATCH 1/2] midx: fix `BUG()` when getting preferred pack without a reverse index
Message-ID: <aTk_-pvNA31ScZyu@pks.im>
In-Reply-To: <aTi9M/F0sZzK/usA@nand.local>

On Tue, Dec 09, 2025 at 07:22:11PM -0500, Taylor Blau wrote:
> On Mon, Dec 08, 2025 at 07:27:14PM +0100, Patrick Steinhardt wrote:
> > The function `midx_preferred_pack()` returns the preferred pack for a
> > given multi-pack index. To compute the preferred pack we:
> >
> >   1. Look up the position of the first object indexed by the multi-pack
> >      index.
> >
> >   2. Convert this position from pseudo-pack order into MIDX order.
> >
> >   3. We then look up pack that corresponds to this MIDX index.
> 
> I think the implementation of midx_preferred_pack() works a little bit
> differently than is described here. I often get confused when working in
> this area juggling between the various object/pack orderings in my head.

Hm, I feel like I am missing something.

> midx_preferred_pack() cares about converting from the first position in
> pseudo-pack order back into MIDX object order. To do that, we convert
> the pseudo-pack position into a MIDX one, and then lookup the pack that
> represents that object.

Isn't that what I say in (2) and (3)? Or is this about (1) being
inaccurate? Would this sequence be more accurate:

  1. Take the first position indexed by the MIDX in pseudo-pack order.

  2. Convert this pseudo-pack position into the MIDX position.

  3. We then look up the pack that corresponds to this MIDX position.

In any case, I agree with you that juggling these different positions is
quite something :)

> > [...] But we only check for negative
> > return values there, even though the function returns a positive error
> > code in case the reverse index does not exist.
> 
> Ah. It looks like that was changed in 5a6072f631d (fsck: validate .rev
> file header, 2023-04-17), but it looks like the caller here did not
> learn about that change. It may be worth mentioning that commit in your
> patch message.

The caller was introduced at a later point though, via b1e3333068 (midx:
implement `midx_preferred_pack()`, 2023-12-14). So there wasn't really
any overlap here where both topics were cooking at the same point in
time, at least not upstream. And the commit that changed the return
value of `load_midx_revindex()` did update all callsites.

> While reviewing, I wanted to make sure that there weren't any other
> callers of load_midx_revindex() that were also missing this check. The
> return value of that function is propagated through the two expected
> functions:
> 
>     $ git grep -p load_revindex_from_disk
>     pack-revindex.c=struct revindex_header {
>     pack-revindex.c:static int load_revindex_from_disk(const struct git_hash_algo *algo,
>     pack-revindex.c=int load_pack_revindex_from_disk(struct packed_git *p)
>     pack-revindex.c:        ret = load_revindex_from_disk(p->repo->hash_algo,
>     pack-revindex.c=int load_midx_revindex(struct multi_pack_index *m)
>     pack-revindex.c:        ret = load_revindex_from_disk(m->source->odb->repo->hash_algo,
> 
> , and checking through the callers of those two functions, all are
> prepared to handle a >0 return value.

Yup, thanks for double checking.

Patrick