On Thu, May 22, 2025 at 01:32:35AM -0400, Jeff King wrote:
> On Tue, May 20, 2025 at 11:53:10AM +0200, Patrick Steinhardt wrote:
>
> > @@ -458,6 +458,8 @@ int prepare_midx_pack(struct repository *r, struct multi_pack_index *m,
> >
> > pack_int_id = midx_for_pack(&m, pack_int_id);
> >
> > + if (m->packs[pack_int_id] == (void *)(intptr_t)-1)
> > + return 1;
> > if (m->packs[pack_int_id])
> > return 0;
>
> I did wonder while writing this if we might be able to hide the magic
> number and gross casting inside a constant or macro. I think just:
>
> #define MIDX_PACK_ERROR ((void *)(intptr_t)-1)
>
> would be enough?
>
> Though...
I agree with the longer-term goal of having prepare_midx_pack() just
return a pointer to a struct packed_git. But in the meantime, I do think
having a #define for the "oops, I tried to load this packfile and it was
broken" case is a good idea.
> > @@ -495,6 +499,8 @@ struct packed_git *nth_midxed_pack(struct multi_pack_index *m,
> > uint32_t pack_int_id)
> > {
> > uint32_t local_pack_int_id = midx_for_pack(&m, pack_int_id);
> > + if (m->packs[local_pack_int_id] == (void *)(intptr_t)-1)
> > + return NULL;
> > return m->packs[local_pack_int_id];
>
> 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.
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.
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.
Thanks,
Taylor
