Re: [PATCH v3 9/9] repack: exclude cruft pack(s) from the MIDX where possible

[Taylor Blau <me@ttaylorr.com>]

On Tue, Apr 15, 2025 at 10:56:59PM -0700, Elijah Newren wrote:
> On Tue, Apr 15, 2025 at 3:47 PM Taylor Blau <me@ttaylorr.com> wrote:
> >
> > In ddee3703b3 (builtin/repack.c: add cruft packs to MIDX during
> > geometric repack, 2022-05-20), repack began adding cruft pack(s) to the
> > MIDX with '--write-midx' to ensure that the resulting MIDX was always
> > closed under reachability in order to generate reachability bitmaps.
> >
> > Suppose (prior to this patch) you have a once-unreachable object packed
> > in a cruft pack, which later on becomes reachable from one or more
> > objects in a geometrically repacked pack. That once-unreachable object
> > *won't* appear in the new pack, since the cruft pack was specified as
> > neither included nor excluded to 'pack-objects --stdin-packs'.
>
> But immediately prior to this patch you implemented
> --stdin-packs=follow, so the once-unreachable object would actually
> appear in the pack if that new option was used.  The "(prior to this
> patch)" addition was meant to help clarify here, but to me it doesn't
> succeed.  (If it had been "(prior to this series)" it would have
> clarified that we aren't yet using the new feature from the previous
> patch.)  Perhaps you meant that geometric repacking doesn't use
> --stdin-packs=follow currently, and therefore the once-unreachable
> object won't be in the new pack, but if so I think it would be helpful
> to call that out explicitly so the reader can more easily follow which
> hypothetical state you are discussing.

Yeah, I am referring to the state of the world from repack's perspective
here. It is the case that prior to this patch (9/9) we don't use
'--stdin-packs=follow' from the repack code when invoking pack-objects,
but I can sympathize with the confusion that this creates since the
distinction between the new mode existing and having real-life callers
from other builtins is subtle.

> > If the
> > new pack is included in a MIDX without the cruft pack, then trying to
> > generate bitmaps for that MIDX may fail. This happens when the bitmap
> > selection process picks one or more commits which reach the
> > once-unreachable objects, commit ddee3703b3 ensures that the MIDX will
> > be closed under reachability. Without it, we would fail to generate a
> > MIDX bitmap.
>
> The comma between objects and commit seems insufficient.  To me, that
> feels like a contrasting thought that should start a new sentence.
> Perhaps the last three lines could read something like:
>
> """
> once-unreachable objects.  Commit ddee3703b3 ensures that the MIDX will
> be closed under reachability by including cruft pack(s); without them,
> we would fail to generate a
> MIDX bitmap.
> """

I think swapping the comma for a semi-colon would have worked as well.
I'm going to leave it as-is unless you feel strongly about it, if that's
alright with you.

> > ---
> >  Documentation/config/repack.adoc |   7 ++
> >  builtin/repack.c                 | 163 +++++++++++++++++++++++++++----
> >  t/t7704-repack-cruft.sh          |  90 +++++++++++++++++
> >  3 files changed, 242 insertions(+), 18 deletions(-)
>
> You addressed the rest of my feedback with this patch, other than the
> two items I highlighted above.  I'm excited to see how this works out.

Thanks, and thanks for the review! I'm curious to see how it turns out
myself, too ;-).

Thanks,
Taylor