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