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:

quoted

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

quoted

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