On Mon, Apr 15, 2019 at 11:00:45AM -0400, Derrick Stolee wrote:
> > void mark_reachable_objects(struct rev_info *revs, int mark_reflog,
> > timestamp_t mark_recent, struct progress *progress)
> [...]
> > cp.progress = progress;
> > cp.count = 0;
> >
> > + bitmap_git = prepare_bitmap_walk(revs);
> > + if (bitmap_git) {
> > + traverse_bitmap_commit_list(bitmap_git, mark_object_seen);
> > + free_bitmap_index(bitmap_git);
> > + return;
> > + }
> > +
>
> This block after "if (bitmap_git)" is not exercised by the (non-performance)
> test suite, so the rest of the code above is not tested, either. Could a test
> of this "prune" capability be added to the regression tests around the bitmaps?
I have somewhat mixed feelings here. We can add a test with a trivial
bitmap to get code coverage here. But as with many optimizations
(bitmaps, but also your commit graph work), we get a much more thorough
correctness test by re-running the whole test suite (though we do not
yet have one for running with bitmaps on), and a better test that the
optimization is kicking in and working via the t/perf tests.
I dunno. I guess it does not hurt to at least to at least make sure this
code is running in the normal suite. I don't think that will find the
more interesting regressions, but at least saves us the from the most
egregious ones ("oops, the whole thing segfaults because somebody
changed the API" kinds of things).
-Peff
