pack-bitmap: handle objects at bitmap position zero

`bitmap_position()` only returns a negative value when an object is not
present in the bitmap index.

In `find_objects()`, we have added a check (11d45a6e6a) to avoid
processing a root whose reachability is already represented by the base
bitmap, but accidentally uses `pos > 0`. Consequently, it never performs
the membership test for an object at position zero.

If that object has an individual reachability bitmap, we unnecessarily
OR that bitmap into the base again. Otherwise, we add the object to the
not-mapped list, only for the subsequent pass to recognize that it is
already present. The latter pass correctly treats all non-negative
positions as valid, so this does not change the resulting object set,
but an off-by-one edge case.

Treat position zero as valid by changing the condition to `pos >= 0`.

The existing pseudo-merge traversal test exercises this case. Its
position-zero commit is presented through multiple roots. Before this
change, each occurrence is counted as a bitmap hit; afterwards, only
the first occurrence is counted. Assert the resulting hit count to
cover the boundary condition.

Also cover the non-pseudo-merge case by passing `HEAD` twice. The first
occurrence initializes the base from its stored bitmap, and the second
must recognize that position zero is already present.

Helped-by: Taylor Blau <ttaylorr@openai.com>
Signed-off-by: David Lin <davidlin@stripe.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/pack-bitmap.c b/pack-bitmap.c
index e8a82945cc319e1c71fd3079b64fc1b6ce9c7455..9c9b27b72391316f4409732736a49990d5c712ac 100644 (file)
--- a/pack-bitmap.c
+++ b/pack-bitmap.c
@@ -1559,7 +1559,7 @@ static struct bitmap *find_objects(struct bitmap_index *bitmap_git,
 
                if (base) {
                        int pos = bitmap_position(bitmap_git, &object->oid);
-                       if (pos > 0 && bitmap_get(base, pos)) {
+                       if (pos >= 0 && bitmap_get(base, pos)) {
                                object->flags |= SEEN;
                                continue;
                        }

diff --git a/t/t5333-pseudo-merge-bitmaps.sh b/t/t5333-pseudo-merge-bitmaps.sh
index 305d6771082d557ce6584f636804f41f2886d8c1..5b2a17f90a10918c2a17e034ab42e574c34dd363 100755 (executable)
--- a/t/t5333-pseudo-merge-bitmaps.sh
+++ b/t/t5333-pseudo-merge-bitmaps.sh
@@ -50,7 +50,15 @@ test_expect_success 'bitmap traversal without pseudo-merges' '
        test_pseudo_merges_cascades 0 <trace2.txt &&
        test_pseudo_merges >merges &&
        test_must_be_empty merges &&
-       test_cmp expect actual
+       test_cmp expect actual &&
+
+       : >trace2.txt &&
+       GIT_TRACE2_EVENT=$PWD/trace2.txt \
+               git rev-list --objects --use-bitmap-index HEAD HEAD >/dev/null &&
+
+       # The first HEAD initializes base from its position-zero bitmap. The
+       # duplicate root should not count as another bitmap hit.
+       test_trace2_data bitmap bitmap/hits 1 <trace2.txt
 '
 
 test_expect_success 'pseudo-merges accurately represent their objects' '
@@ -85,6 +93,10 @@ test_expect_success 'bitmap traversal with pseudo-merges' '
 
        test_pseudo_merges_satisfied 8 <trace2.txt &&
        test_pseudo_merges_cascades 1 <trace2.txt &&
+
+       # Position zero is named by HEAD, its branch, and its tag, but it
+       # should count as only one bitmap hit.
+       test_trace2_data bitmap bitmap/hits 1 <trace2.txt &&
        test_cmp expect actual
 '