On Fri, Feb 14, 2020 at 01:22:18PM -0500, Jeff King wrote:
> There are a few operations in rev-list that are optimized for bitmaps.
> Rather than having the code inline in cmd_rev_list(), let's move them
> into helpers. This not only makes the flow of the main function simpler,
> but it lets us replace the complex "can we do the optimization?"
> conditionals with a series of early returns from the functions. That
> also makes it easy to add comments explaining those conditions.
Makes sense.
> Signed-off-by: Jeff King <peff@xxxxxxxx>
> ---
> builtin/rev-list.c | 88 +++++++++++++++++++++++++++++++++++-----------
> 1 file changed, 67 insertions(+), 21 deletions(-)
>
> diff --git a/builtin/rev-list.c b/builtin/rev-list.c
> index 4cb5a52dee..38c5ca5603 100644
> --- a/builtin/rev-list.c
> +++ b/builtin/rev-list.c
> @@ -364,6 +364,69 @@ static inline int parse_missing_action_value(const char *value)
> return 0;
> }
>
> +static int try_bitmap_count(struct rev_info *revs)
> +{
> + uint32_t commit_count;
> + int max_count;
> + struct bitmap_index *bitmap_git;
> +
> + /* This function only handles counting, not general traversal. */
> + if (!revs->count)
> + return -1;
> +
> + /*
> + * A bitmap result can't know left/right, etc, because we don't
> + * actually traverse.
> + */
> + if (revs->left_right || revs->cherry_mark)
> + return -1;
> +
> + /*
> + * This must be saved before doing any walking, since the revision
> + * machinery will count it down to zero while traversing.
> + */
> + max_count = revs->max_count;
> +
> + bitmap_git = prepare_bitmap_walk(revs);
> + if (!bitmap_git)
> + return -1;
> +
> + count_bitmap_commit_list(bitmap_git, &commit_count, NULL, NULL, NULL);
> + if (max_count >= 0 && max_count < commit_count)
> + commit_count = max_count;
> +
> + printf("%d\n", commit_count);
> + free_bitmap_index(bitmap_git);
> + return 0;
> +}
> +
> +static int try_bitmap_traversal(struct rev_info *revs)
> +{
> + struct bitmap_index *bitmap_git;
> +
> + /*
> + * We can't use a bitmap result with a traversal limit, since the set
> + * of commits we'd get would be essentially random.
> + */
> + if (revs->max_count >= 0)
> + return -1;
> +
> + /*
> + * Our bitmap result will return all objects, and we're not
> + * yet prepared to show only particular types.
> + */
> + if (!revs->tag_objects || !revs->tree_objects || !revs->blob_objects)
> + return -1;
> +
> + bitmap_git = prepare_bitmap_walk(revs);
> + if (!bitmap_git)
> + return -1;
> +
> + traverse_bitmap_commit_list(bitmap_git, &show_object_fast);
> + free_bitmap_index(bitmap_git);
> + return 0;
> +}
> +
> int cmd_rev_list(int argc, const char **argv, const char *prefix)
> {
> struct rev_info revs;
> @@ -534,27 +597,10 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)
> progress = start_delayed_progress(show_progress, 0);
>
> if (use_bitmap_index) {
> - if (revs.count && !revs.left_right && !revs.cherry_mark) {
> - uint32_t commit_count;
> - int max_count = revs.max_count;
> - struct bitmap_index *bitmap_git;
> - if ((bitmap_git = prepare_bitmap_walk(&revs))) {
> - count_bitmap_commit_list(bitmap_git, &commit_count, NULL, NULL, NULL);
> - if (max_count >= 0 && max_count < commit_count)
> - commit_count = max_count;
> - printf("%d\n", commit_count);
> - free_bitmap_index(bitmap_git);
> - return 0;
> - }
> - } else if (revs.max_count < 0 &&
> - revs.tag_objects && revs.tree_objects && revs.blob_objects) {
> - struct bitmap_index *bitmap_git;
> - if ((bitmap_git = prepare_bitmap_walk(&revs))) {
> - traverse_bitmap_commit_list(bitmap_git, &show_object_fast);
> - free_bitmap_index(bitmap_git);
> - return 0;
> - }
> - }
> + if (!try_bitmap_count(&revs))
> + return 0;
> + if (!try_bitmap_traversal(&revs))
> + return 0;
> }
>
> if (prepare_revision_walk(&revs))
> --
> 2.25.0.796.gcc29325708
The refactoring here is straightforward, and looks correct to my
reading.
Thanks,
Taylor
