[09/13] rev-list: use bitmap filters for traversal
diff mbox series

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

Commit Message

Jeff King Feb. 13, 2020, 2:21 a.m. UTC
This just passes the filter-options struct to prepare_bitmap_walk().
Since the bitmap code doesn't actually support any filters yet, it will
fallback to the non-bitmap code if any --filter is specified. But this
lets us exercise that rejection code path, as well as getting us ready
to test filters via rev-list when we _do_ support them.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/rev-list.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

Comments

Junio C Hamano Feb. 13, 2020, 10:22 p.m. UTC | #1
Jeff King <peff@peff.net> writes:

> This just passes the filter-options struct to prepare_bitmap_walk().
> Since the bitmap code doesn't actually support any filters yet, it will
> fallback to the non-bitmap code if any --filter is specified. But this
> lets us exercise that rejection code path, as well as getting us ready
> to test filters via rev-list when we _do_ support them.

So we used to look at filter_options.choice and declared any filter
is incompatible with use_bitmap_index quite early, but now we let
each of the try_bitmap_*() helpers check what is in the filter and
make their own decisions.

Of course, the prepare_bitmap_walk() call at the beginning of these
helpers does not know how to work with any filter at this point in
the series, so all of the above cancel out :-).

Makes sense.

I wonder if the "revs.prune" thing that forces use_bitmap_index off
should also move to prepare_bitmap_walk() at some point in the
series (or after the current series is done).  After all, the point
of introducing try_bitmap_*() helpers was to let these bitmap
specific logic to know what is and is not compatible with the bitmap
routines.

Thanks.

> @@ -441,7 +443,7 @@ static int try_bitmap_traversal(struct rev_info *revs)
>  	if (!revs->tag_objects || !revs->tree_objects || !revs->blob_objects)
>  		return -1;
>  
> -	bitmap_git = prepare_bitmap_walk(revs, NULL);
> +	bitmap_git = prepare_bitmap_walk(revs, filter);
>  	if (!bitmap_git)
>  		return -1;
>  
> @@ -612,7 +614,7 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)
>  	    (revs.left_right || revs.cherry_mark))
>  		die(_("marked counting is incompatible with --objects"));
>  
> -	if (filter_options.choice || revs.prune)
> +	if (revs.prune)
>  		use_bitmap_index = 0;
>  
>  	save_commit_buffer = (revs.verbose_header ||
> @@ -625,9 +627,9 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)
>  		progress = start_delayed_progress(show_progress, 0);
>  
>  	if (use_bitmap_index) {
> -		if (!try_bitmap_count(&revs))
> +		if (!try_bitmap_count(&revs, &filter_options))
>  			return 0;
> -		if (!try_bitmap_traversal(&revs))
> +		if (!try_bitmap_traversal(&revs, &filter_options))
>  			return 0;
>  	}
Jeff King Feb. 13, 2020, 10:34 p.m. UTC | #2
On Thu, Feb 13, 2020 at 02:22:07PM -0800, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > This just passes the filter-options struct to prepare_bitmap_walk().
> > Since the bitmap code doesn't actually support any filters yet, it will
> > fallback to the non-bitmap code if any --filter is specified. But this
> > lets us exercise that rejection code path, as well as getting us ready
> > to test filters via rev-list when we _do_ support them.
> 
> So we used to look at filter_options.choice and declared any filter
> is incompatible with use_bitmap_index quite early, but now we let
> each of the try_bitmap_*() helpers check what is in the filter and
> make their own decisions.
> 
> Of course, the prepare_bitmap_walk() call at the beginning of these
> helpers does not know how to work with any filter at this point in
> the series, so all of the above cancel out :-).
> 
> Makes sense.
> 
> I wonder if the "revs.prune" thing that forces use_bitmap_index off
> should also move to prepare_bitmap_walk() at some point in the
> series (or after the current series is done).  After all, the point
> of introducing try_bitmap_*() helpers was to let these bitmap
> specific logic to know what is and is not compatible with the bitmap
> routines.

Ah, interesting thought. Yeah, we could push it down to that level to
avoid rev-list having to know details about how the bitmap code works.
That could replace the earlier patch to consolidate the filter/prune
logic. And then in this patch, this hunk:

> > @@ -612,7 +614,7 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)
> >  	    (revs.left_right || revs.cherry_mark))
> >  		die(_("marked counting is incompatible with --objects"));
> >  
> > -	if (filter_options.choice || revs.prune)
> > +	if (revs.prune)
> >  		use_bitmap_index = 0;

would just drop this conditional entirely. I like it.

-Peff

Patch
diff mbox series

diff --git a/builtin/rev-list.c b/builtin/rev-list.c
index 1ef180469f..c6850e318b 100644
--- a/builtin/rev-list.c
+++ b/builtin/rev-list.c
@@ -372,7 +372,8 @@  static inline int parse_missing_action_value(const char *value)
 	return 0;
 }
 
-static int try_bitmap_count(struct rev_info *revs)
+static int try_bitmap_count(struct rev_info *revs,
+			    struct list_objects_filter_options *filter)
 {
 	uint32_t commit_count = 0,
 		 tag_count = 0,
@@ -407,7 +408,7 @@  static int try_bitmap_count(struct rev_info *revs)
 	 */
 	max_count = revs->max_count;
 
-	bitmap_git = prepare_bitmap_walk(revs, NULL);
+	bitmap_git = prepare_bitmap_walk(revs, filter);
 	if (!bitmap_git)
 		return -1;
 
@@ -423,7 +424,8 @@  static int try_bitmap_count(struct rev_info *revs)
 	return 0;
 }
 
-static int try_bitmap_traversal(struct rev_info *revs)
+static int try_bitmap_traversal(struct rev_info *revs,
+				struct list_objects_filter_options *filter)
 {
 	struct bitmap_index *bitmap_git;
 
@@ -441,7 +443,7 @@  static int try_bitmap_traversal(struct rev_info *revs)
 	if (!revs->tag_objects || !revs->tree_objects || !revs->blob_objects)
 		return -1;
 
-	bitmap_git = prepare_bitmap_walk(revs, NULL);
+	bitmap_git = prepare_bitmap_walk(revs, filter);
 	if (!bitmap_git)
 		return -1;
 
@@ -612,7 +614,7 @@  int cmd_rev_list(int argc, const char **argv, const char *prefix)
 	    (revs.left_right || revs.cherry_mark))
 		die(_("marked counting is incompatible with --objects"));
 
-	if (filter_options.choice || revs.prune)
+	if (revs.prune)
 		use_bitmap_index = 0;
 
 	save_commit_buffer = (revs.verbose_header ||
@@ -625,9 +627,9 @@  int cmd_rev_list(int argc, const char **argv, const char *prefix)
 		progress = start_delayed_progress(show_progress, 0);
 
 	if (use_bitmap_index) {
-		if (!try_bitmap_count(&revs))
+		if (!try_bitmap_count(&revs, &filter_options))
 			return 0;
-		if (!try_bitmap_traversal(&revs))
+		if (!try_bitmap_traversal(&revs, &filter_options))
 			return 0;
 	}