From: Taylor Blau Date: Wed, 28 May 2025 22:59:09 +0000 (-0400) Subject: midx: return a `packed_git` pointer from `prepare_midx_pack()` X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=00fd49db6f6cba5a299737d18da6af5af842094d;p=thirdparty%2Fgit.git midx: return a `packed_git` pointer from `prepare_midx_pack()` 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 Signed-off-by: Taylor Blau Signed-off-by: Junio C Hamano --- diff --git a/midx-write.c b/midx-write.c index ca2384e291..fc74be813d 100644 --- a/midx-write.c +++ b/midx-write.c @@ -943,25 +943,23 @@ static int fill_packs_from_midx_1(struct write_midx_context *ctx, int prepare_packs) { for (uint32_t i = 0; i < m->num_packs; i++) { - /* - * If generating a reverse index, need to have - * packed_git's loaded to compare their - * mtimes and object count. - */ + struct packed_git *p = NULL; + + ALLOC_GROW(ctx->info, ctx->nr + 1, ctx->alloc); if (prepare_packs) { - 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], - m->pack_names[i], + fill_pack_info(&ctx->info[ctx->nr++], p, m->pack_names[i], m->num_packs_in_base + i); } @@ -1588,20 +1586,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)) - continue; - - if (m->packs[i]->pack_keep || m->packs[i]->is_cruft) + p = prepare_midx_pack(r, m, i); + if (!p || p->pack_keep || p->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); @@ -1649,9 +1646,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) @@ -1697,12 +1694,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++) { - pack_info[i].pack_int_id = i; + struct packed_git *p = prepare_midx_pack(r, m, i); - if (prepare_midx_pack(r, m, i)) - continue; - - pack_info[i].mtime = m->packs[i]->mtime; + pack_info[i].pack_int_id = i; + 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 6705e77881..a2e7a3ec0e 100644 --- a/midx.c +++ b/midx.c @@ -451,50 +451,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; - - pack_int_id = midx_for_pack(&m, pack_int_id); - - if (m->packs[pack_int_id] == MIDX_PACK_ERROR) - return 1; - if (m->packs[pack_int_id]) - return 0; - - 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); + uint32_t pack_pos = 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; + + strbuf_addf(&pack_name, "%s/pack/%s", m->object_dir, + m->pack_names[pack_pos]); + + /* 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] = MIDX_PACK_ERROR; - return 1; + m->packs[pack_pos] = p ? p : MIDX_PACK_ERROR; + 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] == MIDX_PACK_ERROR) + return NULL; + return m->packs[pack_pos]; } struct packed_git *nth_midxed_pack(struct multi_pack_index *m, @@ -523,10 +521,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 + @@ -623,9 +622,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 @@ -926,7 +925,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 0fb490f4d4..4ac05b8234 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); const char *nth_midxed_pack_name(struct multi_pack_index *m, diff --git a/pack-bitmap.c b/pack-bitmap.c index 753dd19b1d..a43731c13d 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"), nth_midxed_pack_name(bitmap_git->midx, i)); goto cleanup;