On Wed, May 28, 2025 at 06:59:09PM -0400, Taylor Blau wrote:
> Let's instead have prepare_midx_pack() return a pointer to the
> packed_git structure itself, hiding the above as an implementation
> detail of prepare_midx_pack(). This patch turns the above snippet into:
>
> struct packed_git *p = prepare_midx_pack(the_repository, some_midx,
> some_pack_int_id);
> if (!p)
> die("could not load pack xyz");
>
> making it far easier and less error-prone to access packs by their
> pack_int_id in a MIDX chain.
So obviously I like this direction, but a few small comments:
> (In the future, we may want to consider similar treatment for, e.g., the
> pack_names array. Likewise, it might make sense to rename the "packs"
> member of the MIDX structure to suggest that it shouldn't be accessed
> directly outside of midx.c.)
Is this note still valid for v2? It looks like patch 1 adds
nth_midxed_pack_name() and tries to use it everywhere.
> @@ -1649,9 +1646,9 @@ static int want_included_pack(struct repository *r,
>
> ASSERT(m && !m->base_midx);
>
> - if (prepare_midx_pack(r, m, pack_int_id))
> + p = prepare_midx_pack(r, m, pack_int_id);
> + if (!p)
> return 0;
> - p = m->packs[pack_int_id];
> if (!pack_kept_objects && p->pack_keep)
> return 0;
> if (p->is_cruft)
The ASSERT() in the context is from earlier in the series. But do we
need it once we have this patch? We no longer look at pack_int_id except
to pass it to prepare_midx_pack(), which handles non-base midx slices
just fine.
So we could loosen the assertion now. Or we could wait for later when
somebody wants/needs to do so, but I'm not sure how easy they would find
it to dig in the history. They would find the commit that added the
ASSERT(), but may not realize that this later commit made it OK to
loosen.
I didn't check the other ASSERT() spots from that earlier patch (IIRC,
some of them may actually look use the pack_int_id for other things, and
wouldn't be ready for non-base slices).
> -int prepare_midx_pack(struct repository *r, struct multi_pack_index *m,
> - uint32_t pack_int_id)
> +struct packed_git *prepare_midx_pack(struct repository *r,
> + struct multi_pack_index *m,
> + uint32_t pack_int_id)
We used to return "1" for failure and "0" for success. Now we're
reversed: we return NULL for failure and non-zero for success.
So code like:
if (prepare_midx_pack(...))
return error("yikes");
needs to be updated, but the compiler won't help us because it is happy
to convert both an int and a pointer into a boolean check.
Should we rename the function to make sure we catch any callers for
topics in flight?
I'd have thought we could call it nth_midxed_pack(), but that seems to
exist already, with the caveat that it never prepares the pack, but only
serves what's in the cache. I wonder if we could simply replace that
with what prepare_midx_pack() does, but it may be more conservative to
leave the two separate. So I guess nth_midxed_pack_load() or something.
-Peff
