On Thu, May 22, 2025 at 09:31:48PM -0400, Taylor Blau wrote:
> > 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).
> 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
