[PATCH v2 2/4] 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>
---
 midx-write.c | 7 +++++++
 1 file changed, 7 insertions(+)