Re: [PATCH v2] builtin/repack: fix `--keep-unreachable` when there are no packs

[Patrick Steinhardt <ps@pks.im>]

On Tue, Feb 04, 2025 at 10:22:36AM -0500, Jeff King wrote:
> On Tue, Feb 04, 2025 at 08:00:41AM +0100, Patrick Steinhardt wrote:
> 
> > this small patch series fixes `git repack -ad --keep-unreachable` when
> > there aren't any preexisting packfiles.
> > 
> > Changes in v2:
> >   - Merge tests into t7701.
> >   - Link to v1: https://lore.kernel.org/r/20250203-b4-pks-repack-unreachable-objects-wo-packfiles-v1-0-7c4d69c5072c@pks.im
> 
> This looks good to me.
> 
> One interesting thing I did notice:
> 
> > +test_expect_success 'repack -k packs unreachable loose objects without existing packfiles' '
> > +	test_when_finished "rm -rf repo" &&
> > +	git init repo &&
> > +	(
> > +		cd repo &&
> > +
> > +		oid=$(echo would-be-deleted-loose | git hash-object -w --stdin) &&
> > +		objpath=.git/objects/$(echo $sha1 | sed "s,..,&/,") &&
> > +		test_path_is_file $objpath &&
> > +
> > +		git repack -ad --keep-unreachable &&
> > +		test_path_is_missing $objpath &&
> > +		git cat-file -p $oid
> > +	)
> > +'
> 
> In the test in v1, we had reachable commits to pack. And here we don't.
> So before your patch, the behavior in the v1 test was that we'd create a
> new pack, but it wouldn't pick up the loose object. But the behavior of
> this test is that we say "Nothing new to pack".
> 
> I originally thought that output meant that we were not running
> pack-objects at all. But looking at builtin/repack.c, we do run it, and
> it simply chooses not to make a pack (which makes sense; how would
> repack even realize if there was stuff to pack, since pack-objects is
> what does the traversal).
> 
> So the two outcomes are both the result of the same bug. In both cases
> we do not correctly pack the loose objects, so whether we make a pack is
> just a question of whether there was other reachable stuff to pack. And
> since your patch is fixing the bug at its root, both outcomes are fixed.
> 
> And when I suggested in my response to v1 that "Nothing new to pack" in
> an empty repo was a separate bug, I was just wrong. ;) There is nothing
> else to fix after your patch.
> 
> Thanks for finding and fixing.

Thanks for your thorough review!

Patrick