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

[Elijah Newren <newren@gmail.com>]

On Wed, Apr 16, 2025 at 3:16 PM Taylor Blau <me@ttaylorr.com> wrote:
>
> 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.

Okay, but can we simply call out that --stdin-packs=follow is not yet
used by default in the commit message to make clear that we're talking
about the state without it?

And while we're at it, fix up the end of the paragraph as I mentioned before?

So, something like:

While the previous patch added the --stdin-packs=follow option to
pack-objects, it is not on by default.  Imagine 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' (and `--stdin-packs=follow` is not on). 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 by including cruft pack(s); without them,
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 didn't feel as strongly about this second suggestion, but
incorporated it into the alternate wording I provided above for the
full paragraph since I think it's still an improvement.

As a sidenote, though, it's not just a comma and semicolon swap --
there's also the addition of a full stop and some extra clarifying
words.

> I'm going to leave it as-is unless you feel strongly about it, if that's
> alright with you.

I originally read this thinking that you were only turning down fixing
the second issue I raised, not the first.  Sorry for misunderstanding.
I would like to see a fix for the first issue I raised, at least.

Besides, since Junio didn't take your fixup 2/9 patch, I think you
should re-roll at a minimum to get a correct 2/9 out there (seen still
has the broken one).  And, if you're re-rolling the series, it'd be
really nice to incorporate the suggestions above in 9/9.  If you do
that, I'll put my stamp on the series and encourage Junio to merge it.
:-)