Re: [PATCH v5 3/6] pack-bitmap-write: learn pack.writeBitmapLookupTable and add tests

From: Taylor Blau <me@ttaylorr.com>
Date: Aug 17, 2022, 20:38 UTC
Message-ID: <Yv1RtjTkByAdOvjE@nand.local>
In-Reply-To: <CAPOJW5znYngr4n4tBBCgqZY4Hr38NArHC7Go=ujDkmsFXY57mQ@mail.gmail.com>

On Wed, Aug 17, 2022 at 03:32:31PM +0530, Abhradeep Chakraborty wrote:

> Hello Taylor, extremely thanks for finding the reason for this failure.

No problem. I appreciate all of the time and effort that you, Dscho, and Stolee all put into looking into this (especially while I was out).

My experience with the bitmap code is that it can be somewhat difficult to work in, because there are both (a) many ways to introduce bugs, and (b) the effect of a bug can occur far away from the source of the bug. Those two together make debugging difficult, at least for me.

I think that having a test-tool (like Dscho suggested) to dump some basic information about a bitmap's structure would be quite helpful in the future.

Show 5 quoted lines

> On Wed, Aug 17, 2022 at 3:28 AM Taylor Blau <me@ttaylorr.com> wrote:
> I am not able to understand this modification.
> `info[preferred_pack].orig_pack_int_id` and `preferred_pack` have the
> same value, right? I see `ctx.info` getting sorted only after calling
> `get_sorted_entries()` function.

Yeah, I realized that this is bogus. For one, (as you note) those have the same value before setting up the pack_perm array. But it also goes against the grain of what we're trying to do: the point is that the prefered-ness of objects in an existing MIDX should be discarded when generating a new pseudo-pack order.

Show 14 quoted lines

> > I think there are a couple of ways to fix this. The easiest thing to do
> > would be to force the identity of the preferred pack to be the same when
> > generating a MIDX bitmap *while reusing an existing MIDX*, since that is
> > the only time this bug can happen.
> >
> > But that's a little magical for my taste. I think a more reasonable fix
> > would be to include copies of all objects from the preferred pack
> > including in the case where that pack was non-preferred in an existing
> > MIDX and at least one object in that pack was selected from a different
> > pack in the existing MIDX.
>
> I think the later approach makes the most sense to me. It might not be
> a good idea to keep the same pack as `preferred` as a better candidate
> would be ignored in that case.

Yep, I agree. Users should feel free to change the identity of the preferred pack when rewriting a MIDX regardless of whether or not they are using `--stdin-packs`.

Show 15 quoted lines

> > Abhradeep -- let me know if this is something you want to look into. I
> > think it's a very worthwhile bug to fix, since it is definitely
> > trigger-able in the wild (notably, only with `git multi-pack-index write
> > --bitmap` without `--stdin-packs` and only under certain circumstances),
> > and not just limited to SHA-256 mode.
> >
> > If you are busy experimenting with CRoaring, that's no problem and I can
> > fix this up, too. Either way, it would be worth you and others weighing
> > in on which fix you think is worth pursuing.
>
> I will be happy to fix it but I can't work on it right now (neither on
> CRoaring) because I am currently preparing for my exam. I can continue
> my work after that (i.e. from 19 aug). If you feel it is getting too
> late then you can do this too. I am also thinking of  writing a patch
> for bitmap specific test dump tool (as Johannes proposed previously).

No problem. I wrote up some patches today myself that implement the above fix. I haven't polished them up yet, but they are available here:

    https://github.com/ttaylorr/git/compare/master...ttaylorr:git:tb/bitmap-use-existing-preferred

I want to add a more direct reproduction that works in both SHA-1 and SHA-256 to demonstrate that these patches fix the issue. But in the meantime, you can use Dscho's reproduction with these patches (based on the tip of `master`) applied on top and observe that it passes consistently.

Thanks, Taylor