> diff --git a/pack-bitmap-write.c b/pack-bitmap-write.c
> index b0493d971d..3ac90ae410 100644
> --- a/pack-bitmap-write.c
> +++ b/pack-bitmap-write.c
> @@ -195,7 +195,8 @@ struct bitmap_builder {
> };
>
> static void bitmap_builder_init(struct bitmap_builder *bb,
> - struct bitmap_writer *writer)
> + struct bitmap_writer *writer,
> + struct bitmap_index *old_bitmap)
> {
> struct rev_info revs;
> struct commit *commit;
> @@ -234,12 +235,26 @@ static void bitmap_builder_init(struct bitmap_builder *bb,
>
> c_ent = bb_data_at(&bb->data, commit);
>
> + if (old_bitmap && bitmap_for_commit(old_bitmap, commit)) {
> + /*
> + * This commit has an existing bitmap, so we can
> + * get its bits immediately without an object
> + * walk. There is no need to continue walking
> + * beyond this commit.
> + */
OK - as far as I understand, the reason for continuing the walk would be
to find reverse edges that connect this commit and its ancestors so that
this commit's ancestors can contribute bitmaps to this commit, but we do
not need such contributions, so we do not need to continue the walk.
Makes sense.
> + c_ent->maximal = 1;
> + p = NULL;
Here, we're setting maximal without also setting a bit in this commit's
commit_mask. This is fine because we're not propagating this commit's
commit_mask to any parents (we're not continuing the walk from this
commit), but it seems like a code smell. Suggested fix is below.
> + }
> +
> if (c_ent->maximal) {
> num_maximal++;
> ALLOC_GROW(bb->commits, bb->commits_nr + 1, bb->commits_alloc);
> bb->commits[bb->commits_nr++] = commit;
> }
As far as I can tell, this means that this commit occupies a bit
position in the commit mask that it doesn't need. Could this go into a
separate list instead, to be appended to bb->commits at the very end?
We could even skip the whole maximal stuff (for commits with existing
bitmaps) and replace "c_ent->maximal = 1;" above with "add to list that
we're going to append to bb->commits at the very end". That has the
advantage of not having to redefine "maximal".
>
> + if (!c_ent->commit_mask)
> + continue;
I think this should be moved as far up as possible (right after
the call to bb_data_at()) and commented, something like:
If there is no commit_mask, there is no reason to iterate over this
commit; it is not selected (if it were, it would not have a blank
commit mask) and all its children have existing bitmaps (see the
comment starting with "This commit has an existing bitmap" below), so
it does not contribute anything to the final bitmap file or its
descendants.
