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

From: Jeff King <peff@peff.net>
Date: Feb 4, 2025, 15:22 UTC
Message-ID: <20250204152236.GB620055@coredump.intra.peff.net>
In-Reply-To: <20250204-b4-pks-repack-unreachable-objects-wo-packfiles-v2-1-1eae23366711@pks.im>

On Tue, Feb 04, 2025 at 08:00:41AM +0100, Patrick Steinhardt wrote:

Show 6 quoted lines

> 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:

Show 15 quoted lines

> +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