On Thu, Jun 27, 2019 at 01:35:17PM -0400, Derrick Stolee wrote:
> > pack-bitmap-write.c
> > 05805d74 378) static struct ewah_bitmap *find_reused_bitmap(const struct object_id *oid)
> > d2bc62b1 385) hash_pos = kh_get_oid_map(writer.reused, *oid);
> > 05805d74 425) reused_bitmap = find_reused_bitmap(&chosen->object.oid);
> > 05805d74 432) reused_bitmap = find_reused_bitmap(&cm->object.oid);
>
> Peff: it is interesting that these portions are not covered previously. (Your change
> is clearly mechanical and does not change the correctness.) In particular, lines 425
> and 432 are in two blocks of an if/else with one further inside a loop. The loop
> should always have at least one run, so this if/else isn't even covered.
One of 425 or 432 must run if we enter that "for(;;)" loop (in the
latter case, "next" is non-zero, so we enter the inner loop at least
once).
I think that the whole loop starting at line 409 is not exercised by the
test suite, because we hit the early return above it when there are
fewer than 100 commits to index.
I think this would exercise it, at the cost of making the test more
expensive:
diff --git a/t/t5310-pack-bitmaps.sh b/t/t5310-pack-bitmaps.sh
index 82d7f7f6a5..8ed6982dcb 100755
--- a/t/t5310-pack-bitmaps.sh
+++ b/t/t5310-pack-bitmaps.sh
@@ -21,7 +21,7 @@ has_any () {
}
test_expect_success 'setup repo with moderate-sized history' '
- for i in $(test_seq 1 10)
+ for i in $(test_seq 1 100)
do
test_commit $i
done &&
It would be nice if we had a "test_commits_bulk" that used fast-import
to create larger numbers of commits.
-Peff
