On Thu, Feb 27, 2025 at 10:29 AM Taylor Blau <me@xxxxxxxxxxxx> wrote: > > This short series contains a fix for a bug I noticed while rolling out > multi-cruft packs (via 'git repack --max-cruft-size') within GitHub's > infrastructure. > > The series is structured as follows: > > - The first patch simplifies how 'repack' aggregates cruft packs > together when their size is below the '--max-cruft-size' or > '--max-pack-size' threshold. This simplification changes behavior > slightly, but not in a meaningful way. It occurred to me while > writing the second patch. > > - The second patch describes and fixes the main bug. The gist here is > that objects which are (a) unreachable, (b) exist in a cruft pack > being retained, and (c) were freshened to have a more recent mtime > than any existing cruft copy are unable to be freshened. > > The fix pursued in the second patch changes the rules around when we > want to retain an object via builtin/pack-objects.c::want_found_object() > when at least one cruft pack will survive the repack. > > Previously the rule was to discard any object which appears in any > surviving pack, regardless of mtime. The rule now is to only discard an > object if it appears in either (a) a non-cruft pack which will survive > the repack, or (b) a cruft pack whose mtime for that object is older > than the one we are trying to pack. I think in (b) you got the meaning reversed, and instead mean s/older than/at least as new as/ ? > I think that this is the right behavior, but admittedly putting this > series together hurt my brain trying to think through all of the cases. > I'm fairly confident in the testing here as I remember it being fairly > exhaustive of all interesting cases. But I'd appreciate a sanity check > from others that they too are convinced this is the right approach. > > Thanks in advance for your review! > > Taylor Blau (2): > builtin/repack.c: simplify cruft pack aggregation > builtin/pack-objects.c: freshen objects from existing cruft packs > > builtin/pack-objects.c | 118 ++++++++++++++++++++++++++++++++++------ > builtin/repack.c | 38 +------------ > packfile.c | 3 +- > packfile.h | 2 + > t/t7704-repack-cruft.sh | 106 ++++++++++++++++++++++-------------- > 5 files changed, 171 insertions(+), 96 deletions(-) Code changes look good to me, but I had some wording suggestions in a few places for commit messages and comments. (Sorry for missing some of those in my preliminary review before you sent this series to the list.)
