[v2,04/15] pack-bitmap: refuse to do a bitmap traversal with pathspecs
diff mbox series

Message ID 20200214182216.GD150965@coredump.intra.peff.net
State New
Headers show
Series
  • combining object filters and bitmaps
Related show

Commit Message

Jeff King Feb. 14, 2020, 6:22 p.m. UTC
rev-list has refused to use bitmaps with pathspec limiting since
c8a70d3509 (rev-list: disable --use-bitmap-index when pruning commits,
2015-07-01). But this is true not just for rev-list, but for anyone who
calls prepare_bitmap_walk(); the code isn't equipped to handle this
case.  We never noticed because the only other callers would never pass
a pathspec limiter.

But let's push the check down into prepare_bitmap_walk() anyway. That's
a more logical place for it to live, as callers shouldn't need to know
the details (and must be prepared to fall back to a regular traversal
anyway, since there might not be bitmaps in the repository).

It would also prepare us for a day where this case _is_ handled, but
that's pretty unlikely. E.g., we could use bitmaps to generate the set
of commits, and then diff each commit to see if it matches the pathspec.
That would be slightly faster than a naive traversal that actually walks
the commits. But you'd probably do better still to make use of the newer
commit-graph feature to make walking the commits very cheap.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/rev-list.c |  2 +-
 pack-bitmap.c      | 12 +++++++++++-
 2 files changed, 12 insertions(+), 2 deletions(-)

Comments

Junio C Hamano Feb. 14, 2020, 7:03 p.m. UTC | #1
Jeff King <peff@peff.net> writes:

> It would also prepare us for a day where this case _is_ handled, but
> that's pretty unlikely. E.g., we could use bitmaps to generate the set
> of commits, and then diff each commit to see if it matches the pathspec.

Would the gs/commit-graph-path-filter topic possibly help in this regard?
I was wondering how it would fit within the framework this series sets
up to teach object-filtering and reachability-bitmap codepaths to
work well together.
Jeff King Feb. 14, 2020, 8:51 p.m. UTC | #2
On Fri, Feb 14, 2020 at 11:03:26AM -0800, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > It would also prepare us for a day where this case _is_ handled, but
> > that's pretty unlikely. E.g., we could use bitmaps to generate the set
> > of commits, and then diff each commit to see if it matches the pathspec.
> 
> Would the gs/commit-graph-path-filter topic possibly help in this regard?
> I was wondering how it would fit within the framework this series sets
> up to teach object-filtering and reachability-bitmap codepaths to
> work well together.

I think it would make the scheme I described faster, because that diff
becomes faster. So the commit traversal itself becomes proportionally
more expensive. And if we could speed that up with bitmaps, that's some
improvement. OTOH, my later timings in patch 9 showed that commit graphs
already make that part pretty fast. And they do so without a bunch of
weird restrictions and hassles. So I suspect it's not really worth the
effort to implement this half-way bitmaps approach (for a special case
that it's not even clear anybody cares about).

But if somebody _does_ do so, I don't think we'd need to do anything too
special to integrate with commit-graph-path-filter. The nice thing about
that approach is that the pathspec pruning will just magically return
the same answer, but faster.

-Peff

Patch
diff mbox series

diff --git a/builtin/rev-list.c b/builtin/rev-list.c
index bce406bd1e..4cb5a52dee 100644
--- a/builtin/rev-list.c
+++ b/builtin/rev-list.c
@@ -533,7 +533,7 @@  int cmd_rev_list(int argc, const char **argv, const char *prefix)
 	if (show_progress)
 		progress = start_delayed_progress(show_progress, 0);
 
-	if (use_bitmap_index && !revs.prune) {
+	if (use_bitmap_index) {
 		if (revs.count && !revs.left_right && !revs.cherry_mark) {
 			uint32_t commit_count;
 			int max_count = revs.max_count;
diff --git a/pack-bitmap.c b/pack-bitmap.c
index 6c06099dc7..a97b717e55 100644
--- a/pack-bitmap.c
+++ b/pack-bitmap.c
@@ -715,9 +715,19 @@  struct bitmap_index *prepare_bitmap_walk(struct rev_info *revs)
 	struct bitmap *wants_bitmap = NULL;
 	struct bitmap *haves_bitmap = NULL;
 
-	struct bitmap_index *bitmap_git = xcalloc(1, sizeof(*bitmap_git));
+	struct bitmap_index *bitmap_git;
+
+	/*
+	 * We can't do pathspec limiting with bitmaps, because we don't know
+	 * which commits are associated with which object changes (let alone
+	 * even which objects are associated with which paths).
+	 */
+	if (revs->prune)
+		return NULL;
+
 	/* try to open a bitmapped pack, but don't parse it yet
 	 * because we may not need to use it */
+	bitmap_git = xcalloc(1, sizeof(*bitmap_git));
 	if (open_pack_bitmap(revs->repo, bitmap_git) < 0)
 		goto cleanup;