Re: [PATCH v2 3/8] repack: fix generating multi-pack-index with only non-local packs

From: Taylor Blau <me@ttaylorr.com>
Date: Apr 12, 2023, 20:39 UTC
Message-ID: <ZDcW7LjZkyeGjEsV@nand.local>
In-Reply-To: <f82e44f1da47890b7ef6ee2f5c1cdbbe20fa6684.1681294715.git.ps@pks.im>

On Wed, Apr 12, 2023 at 12:22:39PM +0200, Patrick Steinhardt wrote:

Show 16 quoted lines

> When writing the multi-pack-index with geometric repacking we will add
> all packfiles to the index that are part of the geometric sequence. This
> can potentially also include packfiles borrowed from an alternate object
> directory. But given that a multi-pack-index can only ever include packs
> that are part of the main object database this does not make much sense
> whatsoever.
>
> In the edge case where all packfiles are contained in the alternate
> object database and the local repository has none itself this bug can
> cause us to invoke git-multi-pack-index(1) with only non-local packfiles
> that it ultimately cannot find. This causes it to return an error and
> thus causes the geometric repack to fail.
>
> Fix the code to skip non-local packfiles.
>
> Co-authored-by: Taylor Blau <me@ttaylorr.com>

Thanks for listing me as a co-author. Doing so probably requires my S-o-b, which you are more than free to forge here.

Show 7 quoted lines

> +	# Assert that we wrote neither a new packfile nor a multi-pack-index.
> +	# We should not have a packfile because the single packfile in the
> +	# alternate object database does not invalidate the geometric sequence.
> +	# And we should not have a multi-pack-index because these only index
> +	# local packfiles, and there are none.
> +	find member/.git/objects/pack -type f >actual &&
> +	test_must_be_empty actual

This test looks good, though one small opportunity for cleanup might be to replace this with a single:

    test_dir_is_empty member/$packdir

Thanks, Taylor