Re: [PATCH 2/3] prune: use bitmaps for reachability traversal

From: Jeff King <peff@peff.net>
Date: Apr 18, 2019, 19:49 UTC
Message-ID: <20190418194953.GA15249@sigill.intra.peff.net>
In-Reply-To: <49e2aa9b-ff22-a8d1-ba72-1c881ff5fab4@gmail.com>

On Mon, Apr 15, 2019 at 11:00:45AM -0400, Derrick Stolee wrote:

Show 17 quoted lines

> >  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