Re: [PATCH v2 4/4] midx: return a `packed_git` pointer from `prepare_midx_pack()`

From: Jeff King <peff@peff.net>
Date: May 30, 2025, 06:50 UTC
Message-ID: <20250530065034.GC1321283@coredump.intra.peff.net>
In-Reply-To: <e3e21db673f3ae6e574333d4678a0450fa44fb9c.1748473122.git.me@ttaylorr.com>

On Wed, May 28, 2025 at 06:59:09PM -0400, Taylor Blau wrote:

Show 11 quoted lines

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

Show 12 quoted lines

> @@ -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).

Show 5 quoted lines

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