Subject: Re: [PATCH 2/3] t5309: create failing test for 'git index-pack'

From: Junio C Hamano <[email-masked]>
Date: Wed, 23 Apr 2025 12:37:17 -0700

"Derrick Stolee via GitGitGadget" <[email-masked]> writes:

> From: Derrick Stolee <[email-masked]>
>
> This new test demonstrates some behavior where a valid packfile is being
> rejected by the Git client due to the order in which it is resolving
> REF_DELTAs.
>
> The thin packfile has a REF_DELTA chain A->B->C where C is not included
> in the packfile. However, the client repository contains both C and B
> already. Thus, 'git index-pack' is able to resolve A before resolving B.

In order to reconstitute A, B is needed (which recipient has), and
in order to reconstitute B, C is needed (which recipient also has).

The index-pack sees delta based on B to recreate A; it should be
able to reconstitute A using B that already exists.

OK.

> When resolving B, it then attempts to resolve any other REF_DELTAs that
> are pointing to B as a base. This "revisits" A and complains as if there
> is a cycle, but it did not actually detect a cycle.

That's interesting.

> +test_expect_failure 'index-pack works with thin pack A->B->C with B on disk' '
> +	git init server &&
> +	(
> +		cd server &&
> +		test_commit_bulk 4
> +	) &&
> +
> +	A=$(git -C server rev-parse HEAD^{tree}) &&
> +	B=$(git -C server rev-parse HEAD~1^{tree}) &&
> +	C=$(git -C server rev-parse HEAD~2^{tree}) &&
> +	git -C server reset --hard HEAD~1 &&
> +
> +	cat >in <<-EOF &&
> +	REF_DELTA $A $B
> +	REF_DELTA $B $C
> +	EOF
> +
> +	test-tool -C server pack-deltas 2 <in >thin.pack &&

This is minor, but I somehow find it easier to follow without the
temporary file, i.e.

	test-tool -C server pack-deltas 2 >thin.pack <<-EOF &&
	REF_DELTA $A $B
	REF_DELTA $B $C
	EOF

> +	git clone "file://$(pwd)/server" client &&

This truly loses A from the resulting "client" repository, but the
history still can reach B and C.

> +	(
> +		cd client &&
> +		git index-pack --fix-thin --stdin <../thin.pack
> +	)
> +'

Makes sense.