[PATCH 5/5] midx: return a `packed_git` pointer from `prepare_midx_pack()`
- From
Taylor Blau <me@ttaylorr.com>
- Date
- May 25, 2025, 18:42 UTC
- Message-ID
- <80699bb3ee35f8c4728e7efe8dc53d17134a5fe5.1748198489.git.me@ttaylorr.com>
- In-Reply-To
- <cover.1748198489.git.me@ttaylorr.com>
The prepare_midx_pack() function is designed to convert a MIDX-specific pack_int_id for a given pack into a pointer into an actual `packed_git` structure.
In general, these calls look something like:
struct packed_git *p;
if (prepare_midx_pack(the_repository, some_midx, some_pack_int_id))
die("could not load pack xyz");
p = some_midx->packs[some_pack_int_id];, and in a pre-incremental MIDX world, this pattern works well. However, in a post-incremental MIDX world, this pattern is a little more prone to errors.
These errors can happen when the given 'pack_int_id' is not usable as an index into the 'm->packs' array. And this happens in all layers but the bottom-most one in an incremental MIDX chain. Each layer stores only the packs that are local to that layer of the chain, and offsets them by the total number of packs in the base MIDX(s).
But there is other awkwardness here. Thinking back to the above snippet, suppose that the pack with ID 'some_pack_int_id' is in a layer in the middle of the MIDX chain. Then it is still invalid to do:
p = some_midx->packs[some_pack_int_id - some_midx->num_packs_in_base];
, becuase the top-most layer (here 'some_midx') may not even have that pack! So we would have to chase down the '->base_midx' pointer in order to get the correct result. midx.c has a helper to do this (called 'midx_for_pack()'), but it is meant only for internal use.
That means that a full, working version of the above adjusted to handle incremental MIDXs would look something like:
struct packed_git *p;
if (prepare_midx_pack(the_repository, some_midx, some_pack_int_id))
die("could not load pack xyz"); while (m && pack_int_id < m->num_packs_in_base)
m = m->base_midx; if (!m)
BUG("broken midx?");
if (pack_int_id >= m->num_packs + m->num_packs_in_base)
BUG("broken pack_int_id?");p = m->packs[pack_int_id - m->num_packs_in_base];
, which is far too verbose to access a single pack by its pack_int_id in a MIDX chain.
Let's instead have prepare_midx_pack() return a pointer to the packed_git structure itself, hiding the above as an implementation detail of prepare_midx_pack(). This patch turns the above snippet into:
struct packed_git *p = prepare_midx_pack(the_repository, some_midx,
some_pack_int_id);
if (!p)
die("could not load pack xyz");making it far easier and less error-prone to access packs by their pack_int_id in a MIDX chain.
(In the future, we may want to consider similar treatment for, e.g., the pack_names array. Likewise, it might make sense to rename the "packs" member of the MIDX structure to suggest that it shouldn't be accessed directly outside of midx.c.)
Suggested-by: Jeff King <peff@peff.net> Signed-off-by: Taylor Blau <me@ttaylorr.com> --- midx-write.c | 37 +++++++++++------------ midx.c | 81 +++++++++++++++++++++++++-------------------------- midx.h | 4 ++- pack-bitmap.c | 2 +- 4 files changed, 63 insertions(+), 61 deletions(-)
diff --git a/midx-write.c b/midx-write.c index 2afb67d728..e8dc771665 100644 --- a/midx-write.c +++ b/midx-write.c @@ -943,6 +943,8 @@ static int fill_packs_from_midx_1(struct write_midx_context *ctx, uint32_t flags) { for (uint32_t i = 0; i < m->num_packs; i++) { + struct packed_git *p = NULL; + ALLOC_GROW(ctx->info, ctx->nr + 1, ctx->alloc); /* @@ -951,18 +953,19 @@ static int fill_packs_from_midx_1(struct write_midx_context *ctx, * mtimes and object count. */ if (flags & MIDX_WRITE_REV_INDEX) { - if (prepare_midx_pack(ctx->repo, m, - m->num_packs_in_base + i)) { + p = prepare_midx_pack(ctx->repo, m, + m->num_packs_in_base + i); + if (!p) { error(_("could not load pack")); return 1; } - if (open_pack_index(m->packs[i])) + if (open_pack_index(p)) die(_("could not open index for %s"), - m->packs[i]->pack_name); + p->pack_name); } - fill_pack_info(&ctx->info[ctx->nr++], m->packs[i], + fill_pack_info(&ctx->info[ctx->nr++], p, m->pack_names[i], m->num_packs_in_base + i); } @@ -1596,20 +1599,19 @@ int expire_midx_packs(struct repository *r, const char *object_dir, unsigned fla _("Finding and deleting unreferenced packfiles"), m->num_packs); for (i = 0; i < m->num_packs; i++) { + struct packed_git *p; char *pack_name; display_progress(progress, i + 1); if (count[i]) continue; - if (prepare_midx_pack(r, m, i)) + p = prepare_midx_pack(r, m, i); + if (!p || p->pack_keep || p->is_cruft) continue; - if (m->packs[i]->pack_keep || m->packs[i]->is_cruft) - continue; - - pack_name = xstrdup(m->packs[i]->pack_name); - close_pack(m->packs[i]); + pack_name = xstrdup(p->pack_name); + close_pack(p); string_list_insert(&packs_to_drop, m->pack_names[i]); unlink_pack_path(pack_name, 0); @@ -1657,9 +1659,9 @@ static int want_included_pack(struct repository *r, ASSERT(m && !m->base_midx); - if (prepare_midx_pack(r, m, pack_int_id)) + p = prepare_midx_pack(r, m, pack_int_id); + if (!p) return 0; - p = m->packs[pack_int_id]; if (!pack_kept_objects && p->pack_keep) return 0; if (p->is_cruft) @@ -1705,12 +1707,11 @@ static void fill_included_packs_batch(struct repository *r, repo_config_get_bool(r, "repack.packkeptobjects", &pack_kept_objects); for (i = 0; i < m->num_packs; i++) { + struct packed_git *p = prepare_midx_pack(r, m, i); + pack_info[i].pack_int_id = i; - - if (prepare_midx_pack(r, m, i)) - continue; - - pack_info[i].mtime = m->packs[i]->mtime; + if (p) + pack_info[i].mtime = p->mtime; } for (i = 0; i < m->num_objects; i++) { diff --git a/midx.c b/midx.c index fbce88bd46..f7f509cf46 100644 --- a/midx.c +++ b/midx.c @@ -449,50 +449,48 @@ static uint32_t midx_for_pack(struct multi_pack_index **_m, return pack_int_id - m->num_packs_in_base; } -int prepare_midx_pack(struct repository *r, struct multi_pack_index *m, - uint32_t pack_int_id) +struct packed_git *prepare_midx_pack(struct repository *r, + struct multi_pack_index *m, + uint32_t pack_int_id) { - struct strbuf pack_name = STRBUF_INIT; - struct strbuf key = STRBUF_INIT; - struct packed_git *p; + uint32_t pack_pos = midx_for_pack(&m, pack_int_id); - pack_int_id = midx_for_pack(&m, pack_int_id); + if (!m->packs[pack_pos]) { + struct strbuf pack_name = STRBUF_INIT; + struct strbuf key = STRBUF_INIT; + struct packed_git *p; - if (m->packs[pack_int_id] == (void *)(intptr_t)-1) - return 1; - if (m->packs[pack_int_id]) - return 0; + strbuf_addf(&pack_name, "%s/pack/%s", m->object_dir, + m->pack_names[pack_pos]); - strbuf_addf(&pack_name, "%s/pack/%s", m->object_dir, - m->pack_names[pack_int_id]); - - /* pack_map holds the ".pack" name, but we have the .idx */ - strbuf_addbuf(&key, &pack_name); - strbuf_strip_suffix(&key, ".idx"); - strbuf_addstr(&key, ".pack"); - p = hashmap_get_entry_from_hash(&r->objects->pack_map, - strhash(key.buf), key.buf, - struct packed_git, packmap_ent); - if (!p) { - p = add_packed_git(r, pack_name.buf, pack_name.len, m->local); - if (p) { - install_packed_git(r, p); - list_add_tail(&p->mru, &r->objects->packed_git_mru); + /* pack_map holds the ".pack" name, but we have the .idx */ + strbuf_addbuf(&key, &pack_name); + strbuf_strip_suffix(&key, ".idx"); + strbuf_addstr(&key, ".pack"); + p = hashmap_get_entry_from_hash(&r->objects->pack_map, + strhash(key.buf), key.buf, + struct packed_git, packmap_ent); + if (!p) { + p = add_packed_git(r, pack_name.buf, pack_name.len, + m->local); + if (p) { + install_packed_git(r, p); + list_add_tail(&p->mru, + &r->objects->packed_git_mru); + } } - } - strbuf_release(&pack_name); - strbuf_release(&key); + strbuf_release(&pack_name); + strbuf_release(&key); - if (!p) { - m->packs[pack_int_id] = (void *)(intptr_t)-1; - return 1; + m->packs[pack_pos] = p ? p : (void *)(intptr_t)-1; + if (p) + p->multi_pack_index = 1; } - p->multi_pack_index = 1; - m->packs[pack_int_id] = p; - - return 0; + if (m->packs[pack_pos] == (void *)(intptr_t)-1) + return NULL; + return m->packs[pack_pos]; } struct packed_git *nth_midxed_pack(struct multi_pack_index *m, @@ -514,10 +512,11 @@ int nth_bitmapped_pack(struct repository *r, struct multi_pack_index *m, if (!m->chunk_bitmapped_packs) return error(_("MIDX does not contain the BTMP chunk")); - if (prepare_midx_pack(r, m, pack_int_id)) - return error(_("could not load bitmapped pack %"PRIu32), pack_int_id); + bp->p = prepare_midx_pack(r, m, pack_int_id); + if (!bp->p) + return error(_("could not load bitmapped pack %"PRIu32), + pack_int_id); - bp->p = m->packs[local_pack_int_id]; bp->bitmap_pos = get_be32((char *)m->chunk_bitmapped_packs + MIDX_CHUNK_BITMAPPED_PACKS_WIDTH * local_pack_int_id); bp->bitmap_nr = get_be32((char *)m->chunk_bitmapped_packs + @@ -614,9 +613,9 @@ int fill_midx_entry(struct repository *r, midx_for_object(&m, pos); pack_int_id = nth_midxed_pack_int_id(m, pos); - if (prepare_midx_pack(r, m, pack_int_id)) + p = prepare_midx_pack(r, m, pack_int_id); + if (!p) return 0; - p = m->packs[pack_int_id - m->num_packs_in_base]; /* * We are about to tell the caller where they can locate the @@ -917,7 +916,7 @@ int verify_midx_file(struct repository *r, const char *object_dir, unsigned flag _("Looking for referenced packfiles"), m->num_packs + m->num_packs_in_base); for (i = 0; i < m->num_packs + m->num_packs_in_base; i++) { - if (prepare_midx_pack(r, m, i)) + if (!prepare_midx_pack(r, m, i)) midx_report("failed to load pack in position %d", i); display_progress(progress, i + 1); diff --git a/midx.h b/midx.h index 9d1374cbd5..612999fe6a 100644 --- a/midx.h +++ b/midx.h @@ -104,7 +104,9 @@ void get_split_midx_filename_ext(const struct git_hash_algo *hash_algo, struct multi_pack_index *load_multi_pack_index(struct repository *r, const char *object_dir, int local); -int prepare_midx_pack(struct repository *r, struct multi_pack_index *m, uint32_t pack_int_id); +struct packed_git *prepare_midx_pack(struct repository *r, + struct multi_pack_index *m, + uint32_t pack_int_id); struct packed_git *nth_midxed_pack(struct multi_pack_index *m, uint32_t pack_int_id); int nth_bitmapped_pack(struct repository *r, struct multi_pack_index *m, diff --git a/pack-bitmap.c b/pack-bitmap.c index 99c4927e9c..648b9c0212 100644 --- a/pack-bitmap.c +++ b/pack-bitmap.c @@ -488,7 +488,7 @@ static int open_midx_bitmap_1(struct bitmap_index *bitmap_git, } for (i = 0; i < bitmap_git->midx->num_packs + bitmap_git->midx->num_packs_in_base; i++) { - if (prepare_midx_pack(bitmap_repo(bitmap_git), bitmap_git->midx, i)) { + if (!prepare_midx_pack(bitmap_repo(bitmap_git), bitmap_git->midx, i)) { warning(_("could not open pack %s"), bitmap_git->midx->pack_names[i - bitmap_git->midx->num_packs_in_base]); goto cleanup;
-- 2.49.0.641.gb9c9c4c3bd