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:

quoted

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 :)

quoted

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