pack-bitmap-write: sort pseudo-merge commit lookup table in pack order

The pseudo-merge commit lookup table stores each commit's position in
the pack- or pseudo-pack order, and is used to perform a binary search
in order to determine which pseudo-merge(s) a given commit belongs to.

However, the table was previously sorted in lexical order (via
`oid_array_sort()`), causing the binary search to fail.

While this causes pseudo-merge bitmaps to be de-facto broken for fill-in
traversal, there are a couple of important points to keep in mind:

 * Pseudo-merge application during the initial phases of a bitmap-based
   traversal are applied via `cascade_pseudo_merges_1()`. This function
   enumerates the known pseudo-merges and determines if its parents are
   a subset of the traversal roots.

   This is a different path than the fill-in traversal, where we are
   looking for any pseudo-merges which may be satisfied after visiting
   some commit along an object walk, which involves the aforementioned
   (broken) binary search.

   As a consequence, any pseudo-merges we apply at this stage are done
   so correctly.

 * While this bug makes applying pseudo-merges during fill-in traversal
   effectively broken, it does not produce wrong results. Instead of
   applying the *wrong* pseudo-merge, we will simply fail to find
   satisfied pseudo-merges, leaving the traversal to use the existing
   fill-in routines.

Fix this by sorting the table by bit position before writing, matching
the order that the reader's binary search expects.

This does produce a change the on-disk format insofar as the actual code
now complies with the documented format (for more details, refer to:
Documentation/technical/bitmap-format.adoc). Given that this never
worked in the first place, such a change should be OK to perform.

If an out-of-tree implementation of pseudo-merges happened to generate
bitmaps that comply with the documented format, they will continue to be
read and interpreted as normal.

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

diff --git a/pack-bitmap-write.c b/pack-bitmap-write.c
index 8338d7217ef48f0cd50a91fbec3db16d71b1b6d9..86ed6a5d78cd04a116d7902bd9a7ecddeb282e93 100644 (file)
--- a/pack-bitmap-write.c
+++ b/pack-bitmap-write.c
@@ -819,6 +819,20 @@ static void write_selected_commits_v1(struct bitmap_writer *writer,
        }
 }
 
+static int pseudo_merge_commit_pos_cmp(const void *_va, const void *_vb,
+                                      void *_data)
+{
+       struct bitmap_writer *writer = _data;
+       uint32_t pos_a = find_object_pos(writer, _va, NULL);
+       uint32_t pos_b = find_object_pos(writer, _vb, NULL);
+
+       if (pos_a < pos_b)
+               return -1;
+       if (pos_a > pos_b)
+               return 1;
+       return 0;
+}
+
 static void write_pseudo_merges(struct bitmap_writer *writer,
                                struct hashfile *f)
 {
@@ -876,7 +890,12 @@ static void write_pseudo_merges(struct bitmap_writer *writer,
                oid_array_append(&commits, &kh_key(writer->pseudo_merge_commits, i));
        }
 
-       oid_array_sort(&commits);
+       /*
+        * Sort the commits by their bit position so that the lookup
+        * table can be binary searched by the reader (see
+        * find_pseudo_merge()).
+        */
+       QSORT_S(commits.oid, commits.nr, pseudo_merge_commit_pos_cmp, writer);
 
        /* write lookup table (non-extended) */
        for (i = 0; i < commits.nr; i++) {

diff --git a/t/t5333-pseudo-merge-bitmaps.sh b/t/t5333-pseudo-merge-bitmaps.sh
index 0e9638c31c3d4147e1179710ed3daa5789be355f..3d7a7668121f493c73b4a6da00b5fe42d16b81d7 100755 (executable)
--- a/t/t5333-pseudo-merge-bitmaps.sh
+++ b/t/t5333-pseudo-merge-bitmaps.sh
@@ -462,7 +462,7 @@ test_expect_success 'use pseudo-merge in boundary traversal' '
        )
 '
 
-test_expect_failure 'apply pseudo-merges during fill-in traversal' '
+test_expect_success 'apply pseudo-merges during fill-in traversal' '
        test_when_finished "rm -fr pseudo-merge-fill-in-traversal" &&
        git init pseudo-merge-fill-in-traversal &&
        (