Taylor Blau <me@xxxxxxxxxxxx> writes:
> But why is that tree marked in the first place? It's because we attempt
> to rebuild the bitmap from the existing .bitmap file, but fail part of
> the way through (when we look up the first blob object in the reposition
> table). But that happens *after* we see the tree object, so its bit
> position is marked, even though we didn't rebuild a complete bitmap.
So, there is another bug lurking, other than the lack of "combining
filtered repack and bitmaps are explicitly forbidden" logic? We see
the tree object, we immediately mark it as "done" even we are not,
then we finish in failure and the "done" mark is left behind? Do we
need two bits, "under review" and "done", or something then?
> But it does seem suspect that we rebuild right into ent->bitmap, so we
> may want to consider doing something like:
>
> --- >8 ---
> diff --git a/pack-bitmap-write.c b/pack-bitmap-write.c
> index f6757c3cbf..f4ecdf8b0e 100644
> --- a/pack-bitmap-write.c
> +++ b/pack-bitmap-write.c
> @@ -413,15 +413,19 @@ static int fill_bitmap_commit(struct bb_commit *ent,
>
> if (old_bitmap && mapping) {
> struct ewah_bitmap *old = bitmap_for_commit(old_bitmap, c);
> + struct bitmap *remapped = bitmap_new();
> /*
> * If this commit has an old bitmap, then translate that
> * bitmap and add its bits to this one. No need to walk
> * parents or the tree for this commit.
> */
> - if (old && !rebuild_bitmap(mapping, old, ent->bitmap)) {
> + if (old && !rebuild_bitmap(mapping, old, remapped)) {
> + bitmap_or(ent->bitmap, remapped);
> + bitmap_free(remapped);
> reused_bitmaps_nr++;
> continue;
> }
> + bitmap_free(remapped);
> }
>
> /*
> --- 8< ---
>
> on top.
>
> Applying that patch and then rerunning the tests with the appropriate
> TEST variables causes the 'git repack' to fail as expected, ensuring
> that the containing test passes.
Interesting.
