midx: do not require packs to be sorted in lexicographic order

The MIDX file format currently requires that pack files be identified by
the lexicographic ordering of their names (that is, a pack having a
checksum beginning with "abc" would have a numeric pack_int_id which is
smaller than the same value for a pack beginning with "bcd").

As a result, it is impossible to combine adjacent MIDX layers together
without permuting bits from bitmaps that are in more recent layer(s).

To see why, consider the following example:

          | packs       | preferred pack
  --------+-------------+---------------
  MIDX #0 | { X, Y, Z } | Y
  MIDX #1 | { A, B, C } | B
  MIDX #2 | { D, E, F } | D

, where MIDX #2's base MIDX is MIDX #1, and so on. Suppose that we want
to combine MIDX layers #0 and #1, to create a new layer #0' containing
the packs from both layers. With the original three MIDX layers, objects
are laid out in the bitmap in the order they appear in their source
pack, and the packs themselves are arranged according to the pseudo-pack
order. In this case, that ordering is Y, X, Z, B, A, C.

But recall that the pseudo-pack ordering is defined by the order that
packs appear in the MIDX, with the exception of the preferred pack,
which sorts ahead of all other packs regardless of its position within
the MIDX. In the above example, that means that pack 'Y' could be placed
anywhere (so long as it is designated as preferred), however, all other
packs must be placed in the location listed above.

Because that ordering isn't sorted lexicographically, it is impossible
to compact MIDX layers in the above configuration without permuting the
object-to-bit-position mapping. Changing this mapping would affect all
bitmaps belonging to newer layers, rendering the bitmaps associated with
MIDX #2 unreadable.

One of the goals of MIDX compaction is that we are able to shrink the
length of the MIDX chain *without* invalidating bitmaps that belong to
newer layers, and the lexicographic ordering constraint is at odds with
this goal.

However, packs do not *need* to be lexicographically ordered within the
MIDX. As far as I can gather, the only reason they are sorted lexically
is to make it possible to perform a binary search over the pack names in
a MIDX, necessary to make `midx_contains_pack()`'s performance
logarithmic in the number of packs rather than linear.

Relax this constraint by allowing MIDX writes to proceed with packs that
are not arranged in lexicographic order. `midx_contains_pack()` will
lazily instantiate a `pack_names_sorted` array on the MIDX, which will
be used to implement the binary search over pack names.

Because this change produces MIDXs which may not be correctly read with
external tools or older versions of Git. Though older versions of Git
know how to gracefully degrade and ignore any MIDX(s) they consider
corrupt, external tools may not be as robust. To avoid unintentionally
breaking any such tools, guard this change behind a version bump in the
MIDX's on-disk format.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/Documentation/gitformat-pack.adoc b/Documentation/gitformat-pack.adoc
index 1b4db4aa611e8364897e1c23cf57d2c65868dbc4..5be2206b20bc5fb90cf4d29fac13d6afc5c5d03c 100644 (file)
--- a/Documentation/gitformat-pack.adoc
+++ b/Documentation/gitformat-pack.adoc
@@ -374,7 +374,7 @@ HEADER:
            The signature is: {'M', 'I', 'D', 'X'}
 
        1-byte version number:
-           Git only writes or recognizes version 1.
+           Git only writes version 2, but recognizes versions 1 and 2.
 
        1-byte Object Id Version
            We infer the length of object IDs (OIDs) from this value:
@@ -413,7 +413,9 @@ CHUNK DATA:
            strings. There is no extra padding between the filenames,
            and they are listed in lexicographic order. The chunk itself
            is padded at the end with between 0 and 3 NUL bytes to make the
-           chunk size a multiple of 4 bytes.
+           chunk size a multiple of 4 bytes. Version 1 MIDXs are required to
+           list their packs in lexicographic order, but version 2 MIDXs may
+           list their packs in any arbitrary order.
 
        Bitmapped Packfiles (ID: {'B', 'T', 'M', 'P'})
            Stores a table of two 4-byte unsigned integers in network order.

diff --git a/midx-write.c b/midx-write.c
index 8a54644e427992a91fb4a91bc6f7e929472d0c32..5c8700065a1d164cd2c96769f5e863f68398005e 100644 (file)
--- a/midx-write.c
+++ b/midx-write.c
@@ -36,10 +36,13 @@ extern int cmp_idx_or_pack_name(const char *idx_or_pack_name,
 
 static size_t write_midx_header(const struct git_hash_algo *hash_algo,
                                struct hashfile *f, unsigned char num_chunks,
-                               uint32_t num_packs)
+                               uint32_t num_packs, int version)
 {
+       if (version != MIDX_VERSION_V1 && version != MIDX_VERSION_V2)
+               BUG("unexpected MIDX version: %d", version);
+
        hashwrite_be32(f, MIDX_SIGNATURE);
-       hashwrite_u8(f, MIDX_VERSION);
+       hashwrite_u8(f, version);
        hashwrite_u8(f, oid_version(hash_algo));
        hashwrite_u8(f, num_chunks);
        hashwrite_u8(f, 0); /* unused */
@@ -105,6 +108,8 @@ struct write_midx_context {
 
        uint32_t preferred_pack_idx;
 
+       int version; /* must be MIDX_VERSION_V1 or _V2 */
+
        int incremental;
        uint32_t num_multi_pack_indexes_before;
 
@@ -410,7 +415,9 @@ static int write_midx_pack_names(struct hashfile *f, void *data)
                if (ctx->info[i].expired)
                        continue;
 
-               if (i && strcmp(ctx->info[i].pack_name, ctx->info[i - 1].pack_name) <= 0)
+               if (ctx->version == MIDX_VERSION_V1 &&
+                   i && strcmp(ctx->info[i].pack_name,
+                               ctx->info[i - 1].pack_name) <= 0)
                        BUG("incorrect pack-file order: %s before %s",
                            ctx->info[i - 1].pack_name,
                            ctx->info[i].pack_name);
@@ -1025,6 +1032,12 @@ static bool midx_needs_update(struct multi_pack_index *midx, struct write_midx_c
        if (!midx_checksum_valid(midx))
                goto out;
 
+       /*
+        * If the version differs, we need to update.
+        */
+       if (midx->version != ctx->version)
+               goto out;
+
        /*
         * Ignore incremental updates for now. The assumption is that any
         * incremental update would be either empty (in which case we will bail
@@ -1100,6 +1113,7 @@ static int write_midx_internal(struct write_midx_opts *opts)
        struct tempfile *incr;
        struct write_midx_context ctx = {
                .preferred_pack_idx = NO_PREFERRED_PACK,
+               .version = MIDX_VERSION_V2,
         };
        struct multi_pack_index *midx_to_free = NULL;
        int bitmapped_packs_concat_len = 0;
@@ -1114,6 +1128,10 @@ static int write_midx_internal(struct write_midx_opts *opts)
        ctx.repo = r;
        ctx.source = opts->source;
 
+       repo_config_get_int(ctx.repo, "midx.version", &ctx.version);
+       if (ctx.version != MIDX_VERSION_V1 && ctx.version != MIDX_VERSION_V2)
+               die(_("unknown MIDX version: %d"), ctx.version);
+
        ctx.incremental = !!(opts->flags & MIDX_WRITE_INCREMENTAL);
 
        if (ctx.incremental)
@@ -1445,7 +1463,7 @@ static int write_midx_internal(struct write_midx_opts *opts)
        }
 
        write_midx_header(r->hash_algo, f, get_num_chunks(cf),
-                         ctx.nr - dropped_packs);
+                         ctx.nr - dropped_packs, ctx.version);
        write_chunkfile(cf, &ctx);
 
        finalize_hashfile(f, midx_hash, FSYNC_COMPONENT_PACK_METADATA,

diff --git a/midx.c b/midx.c
index 19ef230d3fd259038a857f5834dc0580c2b869e5..1327d0a3695aec4d2ccbf2257cdd5ea0640e4f17 100644 (file)
--- a/midx.c
+++ b/midx.c
@@ -149,7 +149,7 @@ static struct multi_pack_index *load_multi_pack_index_one(struct odb_source *sou
                      m->signature, MIDX_SIGNATURE);
 
        m->version = m->data[MIDX_BYTE_FILE_VERSION];
-       if (m->version != MIDX_VERSION)
+       if (m->version != MIDX_VERSION_V1 && m->version != MIDX_VERSION_V2)
                die(_("multi-pack-index version %d not recognized"),
                      m->version);
 
@@ -210,7 +210,8 @@ static struct multi_pack_index *load_multi_pack_index_one(struct odb_source *sou
                        die(_("multi-pack-index pack-name chunk is too short"));
                cur_pack_name = end + 1;
 
-               if (i && strcmp(m->pack_names[i], m->pack_names[i - 1]) <= 0)
+               if (m->version == MIDX_VERSION_V1 &&
+                   i && strcmp(m->pack_names[i], m->pack_names[i - 1]) <= 0)
                        die(_("multi-pack-index pack names out of order: '%s' before '%s'"),
                              m->pack_names[i - 1],
                              m->pack_names[i]);
@@ -411,6 +412,7 @@ void close_midx(struct multi_pack_index *m)
        }
        FREE_AND_NULL(m->packs);
        FREE_AND_NULL(m->pack_names);
+       FREE_AND_NULL(m->pack_names_sorted);
        free(m);
 }
 
@@ -656,17 +658,40 @@ int cmp_idx_or_pack_name(const char *idx_or_pack_name,
        return strcmp(idx_or_pack_name, idx_name);
 }
 
+
+static int midx_pack_names_cmp(const void *a, const void *b, void *m_)
+{
+       struct multi_pack_index *m = m_;
+       return strcmp(m->pack_names[*(const size_t *)a],
+                     m->pack_names[*(const size_t *)b]);
+}
+
 static int midx_contains_pack_1(struct multi_pack_index *m,
                                const char *idx_or_pack_name)
 {
        uint32_t first = 0, last = m->num_packs;
 
+       if (m->version == MIDX_VERSION_V2 && !m->pack_names_sorted) {
+               uint32_t i;
+
+               ALLOC_ARRAY(m->pack_names_sorted, m->num_packs);
+
+               for (i = 0; i < m->num_packs; i++)
+                       m->pack_names_sorted[i] = i;
+
+               QSORT_S(m->pack_names_sorted, m->num_packs, midx_pack_names_cmp,
+                       m);
+       }
+
        while (first < last) {
                uint32_t mid = first + (last - first) / 2;
                const char *current;
                int cmp;
 
-               current = m->pack_names[mid];
+               if (m->pack_names_sorted)
+                       current = m->pack_names[m->pack_names_sorted[mid]];
+               else
+                       current = m->pack_names[mid];
                cmp = cmp_idx_or_pack_name(idx_or_pack_name, current);
                if (!cmp)
                        return 1;

diff --git a/midx.h b/midx.h
index a39bcc9d03fc9c5c21673908e6378d54b251fa6d..aa99a6cb2152f7db2f6c3eb3851a56427c56309b 100644 (file)
--- a/midx.h
+++ b/midx.h
@@ -11,7 +11,8 @@ struct git_hash_algo;
 struct odb_source;
 
 #define MIDX_SIGNATURE 0x4d494458 /* "MIDX" */
-#define MIDX_VERSION 1
+#define MIDX_VERSION_V1 1
+#define MIDX_VERSION_V2 2
 #define MIDX_BYTE_FILE_VERSION 4
 #define MIDX_BYTE_HASH_VERSION 5
 #define MIDX_BYTE_NUM_CHUNKS 6
@@ -71,6 +72,7 @@ struct multi_pack_index {
        uint32_t num_packs_in_base;
 
        const char **pack_names;
+       size_t *pack_names_sorted;
        struct packed_git **packs;
 };
 

diff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh
index efeab4d22b7c7902dbed6941a3e8d39c6a0eadd9..250d21dbd6782588604b4924fcd6377700a85a14 100755 (executable)
--- a/t/t5319-multi-pack-index.sh
+++ b/t/t5319-multi-pack-index.sh
@@ -21,7 +21,7 @@ midx_read_expect () {
        EXTRA_CHUNKS="$5"
        {
                cat <<-EOF &&
-               header: 4d494458 1 $HASH_LEN $NUM_CHUNKS $NUM_PACKS
+               header: 4d494458 2 $HASH_LEN $NUM_CHUNKS $NUM_PACKS
                chunks: pack-names oid-fanout oid-lookup object-offsets$EXTRA_CHUNKS
                num_objects: $NUM_OBJECTS
                packs:
@@ -512,11 +512,6 @@ test_expect_success 'verify invalid chunk offset' '
                "improper chunk offset(s)"
 '
 
-test_expect_success 'verify packnames out of order' '
-       corrupt_midx_and_verify $MIDX_BYTE_PACKNAME_ORDER "z" $objdir \
-               "pack names out of order"
-'
-
 test_expect_success 'verify missing pack' '
        corrupt_midx_and_verify $MIDX_BYTE_PACKNAME_ORDER "a" $objdir \
                "failed to load pack"
@@ -578,6 +573,15 @@ test_expect_success 'verify incorrect checksum' '
                $objdir "incorrect checksum"
 '
 
+test_expect_success 'setup for v1-specific fsck tests' '
+       git -c midx.version=1 multi-pack-index write
+'
+
+test_expect_success 'verify packnames out of order (v1)' '
+       corrupt_midx_and_verify $MIDX_BYTE_PACKNAME_ORDER "z" $objdir \
+               "pack names out of order"
+'
+
 test_expect_success 'repack progress off for redirected stderr' '
        GIT_PROGRESS_DELAY=0 git multi-pack-index --object-dir=$objdir repack 2>err &&
        test_line_count = 0 err