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

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 (local)

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.

-Peff