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

[Taylor Blau <me@ttaylorr.com>]

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