Re: [PATCH 10/24] pack-bitmap: pass `bitmapped_pack` struct to pack-reuse functions
- From
Taylor Blau <me@ttaylorr.com>
- Date
- Dec 7, 2023, 20:34 UTC
- Message-ID
- <ZXIsbO++u9n/yDYi@nand.local>
- In-Reply-To
- <ZXHE6Ym3CICtNxFd@tanuki>
On Thu, Dec 07, 2023 at 02:13:13PM +0100, Patrick Steinhardt wrote:
Show 42 quoted lines
> > + if (bitmap_is_midx(bitmap_git)) {
> > + for (i = 0; i < bitmap_git->midx->num_packs; i++) {
> > + struct bitmapped_pack pack;
> > + if (nth_bitmapped_pack(r, bitmap_git->midx, &pack, i) < 0) {
> > + warning(_("unable to load pack: '%s', disabling pack-reuse"),
> > + bitmap_git->midx->pack_names[i]);
> > + free(packs);
> > + return -1;
> > + }
> > + if (!pack.bitmap_nr)
> > + continue; /* no objects from this pack */
> > + if (pack.bitmap_pos)
> > + continue; /* not preferred pack */
> > +
> > + ALLOC_GROW(packs, packs_nr + 1, packs_alloc);
> > + memcpy(&packs[packs_nr++], &pack, sizeof(pack));
> > +
> > + objects_nr += pack.p->num_objects;
> > + }
> > + } else {
> > + ALLOC_GROW(packs, packs_nr + 1, packs_alloc);
> > +
> > + packs[packs_nr].p = bitmap_git->pack;
> > + packs[packs_nr].bitmap_pos = 0;
> > + packs[packs_nr].bitmap_nr = bitmap_git->pack->num_objects;
> > + packs[packs_nr].disjoint = 1;
> > +
> > + objects_nr = packs[packs_nr++].p->num_objects;
> > + }
> > +
> > + word_alloc = objects_nr / BITS_IN_EWORD;
> > + if (objects_nr % BITS_IN_EWORD)
> > + word_alloc++;
> > + reuse = bitmap_word_alloc(word_alloc);
> > +
> > + if (packs_nr != 1)
> > + BUG("pack reuse not yet implemented for multiple packs");
>
> Can't it happen that we have no pack here? In the MIDX-case we skip all
> packs that either do not have a bitmap or are not preferred. So does it
> mean that in reverse, every preferred packfile must have a a bitmap? I'd
> think that to not be true in case bitmaps are turned off.It's subtle, but this state is indeed not possible. If we have a MIDX and it has a bitmap, we know that there is at least one object at least one pack.
On the "at least one object front", that check was added in eb57277ba3 (midx: prevent writing a .bitmap without any objects, 2022-02-09). And we know that our preferred pack (either explicitly given or the one we infer automatically) is non-empty, via the check added in 5d3cd09a80 (midx: reject empty `--preferred-pack`'s, 2021-08-31).
(As a fun/non-fun aside, looking these up gave me some serious deja-vu and reminded me of how painful discovering and fixing those bugs was!)
So we're OK here. We could add a comment which captures what I wrote above here, but since this is a temporary state (and we're going to change how we select which packs are reuse candidates in a later patch), I think it's OK to avoid (but please let me know if you feel differently).
Thanks, Taylor