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

[Elijah Newren <newren@gmail.com>]

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.

> 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.
"""

[...]
> ---
>  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.