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 <peff@peff.net>
Signed-off-by: Taylor Blau <me@ttaylorr.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/midx-write.c b/midx-write.c
index ca2384e291c5c02fc144504a60f217331a75ac64..fc74be813dd12fabc382dcfb66e26218f554c8e0 100644 (file)
--- 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 6705e778810a8543a8c1aefc8067ddc71fe63a6b..a2e7a3ec0eeceb755e75dbac8c2a3d3e0338db74 100644 (file)
--- 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 0fb490f4d4abfabe0a5c8ef571bdcbd4f55319d0..4ac05b82345c159afa61393866efccdff22e1b11 100644 (file)
--- 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 753dd19b1d0f501319e3c924112100e79f406d9b..a43731c13ddd211c9ac68e48febc3e80d563988e 100644 (file)
--- 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;