Re: [PATCH v5 0/8] Repack objects into separate packfiles based on a filter

[Christian Couder <christian.couder@gmail.com>]

On Wed, Aug 16, 2023 at 1:09 AM Taylor Blau <me@ttaylorr.com> wrote:
>
> On Tue, Aug 15, 2023 at 03:32:23PM -0700, Junio C Hamano wrote:
> > Taylor Blau <me@ttaylorr.com> writes:

> > > but I wonder if a more complete fix would be something like:
> > > ...
> > > @@ -966,6 +972,9 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
> > >     if (write_bitmaps && !(pack_everything & ALL_INTO_ONE) && !write_midx)
> > >             die(_(incremental_bitmap_conflict_error));
> > >
> > > +   if (write_bitmaps && po_args.filter_options.choice)
> > > +           die(_(filtered_bitmap_conflict_error));
> > > +
> >
> > It sounds like the most direct fix.

I would be Ok with such a fix, if we think that we don't want to fix
the underlying issue, or if we think that fixing the underlying issue
is not enough...

> I agree.
>
> I think that we would be OK to not change the implementation of
> rebuild_bitmap(), or its caller in fill_bitmap_commit(), since this only
> bites us when bitmapping a filtered pack, and we should catch that case
> well before getting this deep into the bitmap code.
>
> 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.

...however I think that fixing this underlying issue is important, as
it might cause other tricky issues in the future, for example if other
bitmap code is copying or reusing this code.

So I just sent a version 6 of this series with this change in a new
patch. I hope my explanations in the commit message are good enough.

Thanks for finding the cause of the CI test failures and suggesting this fix,
Christian.