[PATCH v2] pack-bitmap: handle objects at bitmap position zero

[David Lin <davidzylin@gmail.com>]

`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>
---
Changes since v1:
- Clarify the bitmap disk load wordings.
- Add coverage for the non-pseudo-merge case.

 pack-bitmap.c                   |  2 +-
 t/t5333-pseudo-merge-bitmaps.sh | 14 +++++++++++++-
 2 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/pack-bitmap.c b/pack-bitmap.c
index d8dc4ae8d1..e85bd69ba4 100644
--- a/pack-bitmap.c
+++ b/pack-bitmap.c
@@ -1569,7 +1569,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 305d677108..5b2a17f90a 100755
--- 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
 '
 

base-commit: 9a0c4701dcd5725c4184599322b52933ff5005ca
-- 
2.54.0