| Message ID | 20200213022125.GI1126038@coredump.intra.peff.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | combining object filters and bitmaps | expand |
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; > }
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
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; }
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(-)