Re: [PATCH v2 07/15] rev-list: allow bitmaps when counting objects
- From
Taylor Blau <me@ttaylorr.com>
- Date
- Feb 15, 2020, 00:45 UTC
- Message-ID
- <20200215004555.GB15192@syl.local>
- In-Reply-To
- <20200214182222.GG150965@coredump.intra.peff.net>
On Fri, Feb 14, 2020 at 01:22:22PM -0500, Jeff King wrote:
Show 30 quoted lines
> The prior commit taught "--count --objects" to work without bitmaps. We
> should be able to get the same answer much more quickly with bitmaps.
>
> Note that we punt on the max_count case here. This perhaps _could_ be
> made to work if we find all of the boundary commits and treat them as
> UNINTERESTING, subtracting them (and their reachable objects) from the
> set we return. That implies an actual commit traversal, but we'd still
> be faster due to avoiding opening up any trees. Given the complexity and
> the fact that anyone is unlikely to want this, it makes sense to just
> fall back to the non-bitmap case for now.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> builtin/rev-list.c | 21 ++++++++++++++++++---
> t/t5310-pack-bitmaps.sh | 6 ++++++
> 2 files changed, 24 insertions(+), 3 deletions(-)
>
> diff --git a/builtin/rev-list.c b/builtin/rev-list.c
> index 9452123988..70f3207ecc 100644
> --- a/builtin/rev-list.c
> +++ b/builtin/rev-list.c
> @@ -374,7 +374,10 @@ static inline int parse_missing_action_value(const char *value)
>
> static int try_bitmap_count(struct rev_info *revs)
> {
> - uint32_t commit_count;
> + uint32_t commit_count = 0,
> + tag_count = 0,
> + tree_count = 0,
> + blob_count = 0;Hmm, I don't usually see the comma-separated declaration/initialization in git.git. Is there a reason you did it here? Not that I really mind one way or the other, just interested.
Show 14 quoted lines
> int max_count; > struct bitmap_index *bitmap_git; > > @@ -389,6 +392,15 @@ static int try_bitmap_count(struct rev_info *revs) > if (revs->left_right || revs->cherry_mark) > return -1; > > + /* > + * If we're counting reachable objects, we can't handle a max count of > + * commits to traverse, since we don't know which objects go with which > + * commit. > + */ > + if (revs->max_count >= 0 && > + (revs->tag_objects || revs->tree_objects || revs->blob_objects))
An aside unrelated to the patch at hand: the expression
(revs->tag_objects || revs->tree_objects || revs->blob_objects)
does occur in an awful lot of places throughout this file. Do you imagine it'd be useful to pull this check out into its own function, perhaps as a preparatory patch in a later version of this series?
I'm also not fussed if you don't think that such a change would be useful, it's just an observation I had after seeing this expression a few times.
Show 41 quoted lines
> + return -1;
> +
> /*
> * This must be saved before doing any walking, since the revision
> * machinery will count it down to zero while traversing.
> @@ -399,11 +411,14 @@ static int try_bitmap_count(struct rev_info *revs)
> if (!bitmap_git)
> return -1;
>
> - count_bitmap_commit_list(bitmap_git, &commit_count, NULL, NULL, NULL);
> + count_bitmap_commit_list(bitmap_git, &commit_count,
> + revs->tree_objects ? &tree_count : NULL,
> + revs->blob_objects ? &blob_count : NULL,
> + revs->tag_objects ? &tag_count : NULL);
> if (max_count >= 0 && max_count < commit_count)
> commit_count = max_count;
>
> - printf("%d\n", commit_count);
> + printf("%d\n", commit_count + tree_count + blob_count + tag_count);
> free_bitmap_index(bitmap_git);
> return 0;
> }
> diff --git a/t/t5310-pack-bitmaps.sh b/t/t5310-pack-bitmaps.sh
> index 6640329ebf..7ba7d294a5 100755
> --- a/t/t5310-pack-bitmaps.sh
> +++ b/t/t5310-pack-bitmaps.sh
> @@ -74,6 +74,12 @@ rev_list_tests() {
> test_cmp expect actual
> '
>
> + test_expect_success "counting objects via bitmap ($state)" '
> + git rev-list --count --objects HEAD >expect &&
> + git rev-list --use-bitmap-index --count --objects HEAD >actual &&
> + test_cmp expect actual
> + '
> +
> test_expect_success "enumerate --objects ($state)" '
> git rev-list --objects --use-bitmap-index HEAD >tmp &&
> cut -d" " -f1 <tmp >tmp2 &&
> --
> 2.25.0.796.gcc29325708Your tests look good to me, too.
Thanks, Taylor