Skip to content

Commit

Permalink
rev-list: skip bitmap traversal for --left-right
Browse files Browse the repository at this point in the history
Running:

  git rev-list --left-right --use-bitmap-index one...two

will produce output without any left-right markers, since the bitmap
traversal returns only a single set of reachable commits. Instead we
should refuse to use bitmaps here and produce the correct output using a
traditional traversal.

This is probably not the only remaining option that misbehaves with
bitmaps, but it's particularly egregious in that it feels like it
_could_ work. Doing two separate traversals for the left/right sides and
then taking the symmetric set differences should yield the correct
answer, but our traversal code doesn't know how to do that.

It's not clear if naively doing two separate traversals would always be
a performance win. A traditional traversal only needs to walk down to
the merge base, but bitmaps always fill out the full reachability set.
So depending on your bitmap coverage, we could end up walking old bits
of history twice to fill out the same uninteresting bits on both sides.
We'd also of course end up with a very large --boundary set, if the user
asked for that.

So this might or might not be something worth implementing later. But
for now, let's make sure we don't produce the wrong answer if somebody
tries it.

The test covers this, but also the same thing with "--count" (which is
what I originally tried in a real-world case). Ironically the
try_bitmap_count() code already realizes that "--left-right" won't work
there. But that just causes us to fall back to the regular bitmap
traversal code, which itself doesn't handle counting (we produce a list
of objects rather than a count).

Signed-off-by: Jeff King <[email protected]>
Signed-off-by: Taylor Blau <[email protected]>
  • Loading branch information
peff authored and ttaylorr committed Nov 1, 2024
1 parent 23d289d commit 16a186f
Show file tree
Hide file tree
Showing 2 changed files with 19 additions and 0 deletions.
7 changes: 7 additions & 0 deletions builtin/rev-list.c
Original file line number Diff line number Diff line change
Expand Up @@ -485,6 +485,13 @@ static int try_bitmap_traversal(struct rev_info *revs,
if (revs->max_count >= 0)
return -1;

/*
* We can't know which commits were left/right in a single traversal,
* and we don't yet know how to traverse them separately.
*/
if (revs->left_right)
return -1;

bitmap_git = prepare_bitmap_walk(revs, filter_provided_objects);
if (!bitmap_git)
return -1;
Expand Down
12 changes: 12 additions & 0 deletions t/t5310-pack-bitmaps.sh
Original file line number Diff line number Diff line change
Expand Up @@ -503,6 +503,18 @@ test_expect_success 'boundary-based traversal is used when requested' '
done
'

test_expect_success 'left-right not confused by bitmap index' '
git rev-list --left-right other...HEAD >expect &&
git rev-list --use-bitmap-index --left-right other...HEAD >actual &&
test_cmp expect actual
'

test_expect_success 'left-right count not confused by bitmap-index' '
git rev-list --left-right --count other...HEAD >expect &&
git rev-list --use-bitmap-index --left-right --count other...HEAD >actual &&
test_cmp expect actual
'

test_bitmap_cases "pack.writeBitmapLookupTable"

test_expect_success 'verify writing bitmap lookup table when enabled' '
Expand Down

0 comments on commit 16a186f

Please sign in to comment.