Re: [PATCH v2 2/2] midx: stop repeatedly looking up nonexistent packfiles

From: Jeff King <peff@peff.net>
Date: May 23, 2025, 02:18 UTC
Message-ID: <20250523021828.GC559000@coredump.intra.peff.net>
In-Reply-To: <aC/QBHBbmYaVUzHV@nand.local>

On Thu, May 22, 2025 at 09:31:48PM -0400, Taylor Blau wrote:

Show 15 quoted lines

> > Yuck, yet another spot that needs to be aware of the new tri-state
> > value. One alternative is using an auxiliary array to cache the errors,
> > and then only the lookup function needs to care. Like:
> 
> I like this direction, though I dislike having a separate array that we
> need to keep in sync with m->packs. It might be nice to have an array
> like:
> 
>   struct {
>       struct packed_git *p;
>       unsigned err:1;
>   } *packs;
> 
> , which would allow you to keep the error state next to the packed_git
> itself.

In general, yes, I think array-of-struct is better than struct-of-array. In this particular case the latter is not too bad because the management is all handled centrally in the midx constructor.

The downside of doing array-of-struct as you propose is that every site that uses it will need to be modified. But that is at least a one-time pain and not an ongoing maintenance burden (unlike the "magic" value approach).

Show 11 quoted lines

> I wonder if changing the signature to:
> 
>     int prepare_midx_pack(struct repository *r,
>                           struct multi_pack_index *m,
>                           uint32_t pack_int_id,
>                           struct packed_git **p_out);
> 
> would be a good idea. It allows you to pass garbage input (like a
> non-existent pack_int_id) and get a useful error back. It also allows
> you to pass a pack_int_id that is valid, but cannot be loaded and get a
> useful error back via the return value.

Would the return value be a richer set of values than the current success/fail? If not, then I think just returning the pack pointer does that fine.

I was also tempted to suggest that it should take a "struct multi_pack_index **", to return the matching midx as an out-parameter. That would "just work" for callers that want to look at the surrounding midx, too.

But maybe it gets weird for ones that are (correctly) expecting to find the pack within the same midx. I.e., code like this:

  for (i = 0; i < m->num_packs; i++) {
	if (prepare_midx_pack(r, m, i + m->num_packs_in_base))
		die(...);
	/* do something with m->pack[i] */
  }

is correct now, and we _shouldn't_ ever need to switch to a different "m" inside prepare_midx_pack(). But if we ever did, propagating that up to the caller would be mighty confusing.

So perhaps better to leave it as-is, and let the caller explicitly do midx_for_pack() or whatever to find the right "m" as necessary.

> But I think without actually trying it and seeing what the fallout looks
> like, it's hard to say whether or not the above is a step in the right
> direction.

Yup. All of this can wait, too. Patrick's series is fixing its own localized issue (however he wants to structure the extra bit of storage). Most of what we're talking about here is future-proofing so it can happen at our leisure. I think the only other actual bug is the want_included_pack() one discussed in the other part of the thread.

-Peff