midx-write.c: guard against incremental MIDXs in want_included_pack()

The function want_included_pack() is used to determine whether or not a
the packfile corresponding to some given pack_int_id should be included
in a 'git multi-pack-index repack' operation.

This spot looks like it would be broken, particularly in:

    struct packed_git *p;
    if (prepare_midx_pack(r, m, pack_int_id))
        return 0;
    p = m->packs[pack_int_id];

, when pack_int_id is greater than m->num_pack_in_base (supposing that
m->num_packs_in_base is non-zero, or equivalently that m->base_midx is
non-NULL).

Suppose we have two MIDXs in an incremental MIDX chain, each having two
packs:

  - M0 = {packs: [P0, P1], base_midx: NULL}
  - M1 = {packs: [P2, P3], base_midx: M0}

noting that each pack is identified by its global pack_int_id within the
chain.

So if you do something like:

    want_included_pack(the_repository, M1, pack_kept_objects, 2);

that function will call prepare_midx_pack(), which is smart enough to
realize that the pack of interest is in the current layer (M1), and
knows how to adjust its global pack_int_id into an index into the
current layer's 'packs' array.

But the following line:

    p = m->packs[pack_int_id]; /* m is M1, pack_int_id is 2 */

looks broken, since each layer of the MIDX only maintains an array of
the packs stored within that layer, and 'm' wasn't adjusted back to
point at M1->base_midx (M0).

The right thing to do would be:

    struct packed_git *p;
    if (prepare_midx_pack(r, m, pack_int_id))
        return 0;

    /* open-code midx.c::midx_for_pack(), which is private */
    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];

But that would be overkill, since this function never deals with
incremental MIDXs having more than one layer! To see why, observe that
want_included_pack() has two callers:

  - midx-write.c::fill_included_packs_all()
  - midx-write.c::fill_included_packs_batch()

and those functions' sole caller is in midx-write.c::midx_repack(),
which dispatches a call to one or the other depending on whether or not
the batch_size is non-zero.

And at the very top of midx_repack(), we have a guard against
non-trivial incremental MIDX chains:

    if (m->base_midx)
        die(_("cannot repack an incremental multi-pack-index"));

So want_included_pack() is OK because it will never encounter a
situation where it has to chase backwards through the '->base_midx'
pointer. But that is not immediately clear from reading the code, and is
too fragile for my comfort. Make this more clear by adding an ASSERT()
to the above effect.

Apply the same treatment to each of the fill_included_packs-related
functions as well, since those are deceptively OK by the same reasoning.

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 dd3b3070e55dfcea80e04bd969a4928f21df33a3..e4a3830d45db29412b7db2a46f4e0afbb6b4cd58 100644 (file)
--- a/midx-write.c
+++ b/midx-write.c
@@ -1636,6 +1636,9 @@ static int want_included_pack(struct repository *r,
                              uint32_t pack_int_id)
 {
        struct packed_git *p;
+
+       ASSERT(m && !m->base_midx);
+
        if (prepare_midx_pack(r, m, pack_int_id))
                return 0;
        p = m->packs[pack_int_id];
@@ -1655,6 +1658,8 @@ static void fill_included_packs_all(struct repository *r,
        uint32_t i;
        int pack_kept_objects = 0;
 
+       ASSERT(m && !m->base_midx);
+
        repo_config_get_bool(r, "repack.packkeptobjects", &pack_kept_objects);
 
        for (i = 0; i < m->num_packs; i++) {
@@ -1675,6 +1680,8 @@ static void fill_included_packs_batch(struct repository *r,
        struct repack_info *pack_info;
        int pack_kept_objects = 0;
 
+       ASSERT(m && !m->base_midx);
+
        CALLOC_ARRAY(pack_info, m->num_packs);
 
        repo_config_get_bool(r, "repack.packkeptobjects", &pack_kept_objects);