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@xxxxxxxxxxxx> 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
