Re: [PATCH 2/2] t5608: assert that the packs involved really are that large

[Patrick Steinhardt <ps@pks.im>]

On Fri, Nov 06, 2026 at 12:51:16PM +0000, Kristofer Karlsson via GitGitGadget wrote:
> From: Kristofer Karlsson <krka@spotify.com>
> 
> t5608 was added in ca31f0b5c2 (Add an (optional, since expensive) test
> for >2gb clones, 2009-03-06) to construct a repository larger than 2GB
> and clone it.  It already anticipated that the setup might stop doing
> what it is meant to do: "Should that break in the future, e.g. when
> pack v4 becomes default, there is a commented-out call to
> test-genrandom which can be substituted".
> 
> It did break, silently, with d9545c7f46 (fast-import: implement unpack
> limit, 2016-04-25).  The setup writes only 24 objects, which is below
> the default fastimport.unpackLimit of 100, so fast-import does not
> keep its pack but turns it into loose objects.  Since the blobs
> compress very well, the repository has less than 10MB of loose
> objects, and the bare clone merely copies those.  Only the file://
> clone still deals with a pack larger than 2GB, because the sending
> side generates a new pack from the loose objects.
> 
> The ">4GB object" tests added in 4b220cafbe (t5608: add regression
> test for >4GB object clone, 2026-05-08) have a similar problem.  They
> are meant to cover a fix in both index-pack and unpack-objects, but
> "git clone" always keeps the received pack, so setting
> fetch.unpackLimit in the unpack-objects variant has no effect, and
> both tests use index-pack.

So this test in particular never exercised what it was designed to?

> Nothing noticed any of this, because the tests only check that the
> commands succeed.  Add assertions on what each test relies on, so
> that it fails when its setup no longer does what it is meant to:
> 
>  - the pack created by the setup, and the packs of both clones, are
>    larger than 2GB;
> 
>  - the synthesized pack with the >4GB object, and the pack written by
>    the index-pack clone, are larger than 4GB;
> 
>  - the unpack-objects variant really runs unpack-objects, as seen in
>    its trace2 output.

Makes sense.

> Some of these assertions fail with the tests as they are, so fix them:
> 
>  - Set fastimport.unpackLimit=0 in the setup.  fast-import then
>    keeps its pack regardless of how few objects it contains, so the
>    repository, and the bare clone that copies it, have a pack larger
>    than 2GB again.
> 
>  - In the unpack-objects variant, use "git fetch" into an empty
>    repository instead of "git clone".  Unlike clone, fetch honors
>    fetch.unpackLimit, so with five objects and a limit of 100 the
>    received pack really is passed to unpack-objects.

And the preceding patch fixes a third assertion that we'd otherwise have
seen fail here, I assume?

> As a bonus, keeping the fast-import pack also makes the test cheaper,
> because unpack-objects no longer hashes and compresses all of the 2GB
> of data a second time.  Locally, the time spent in the setup is
> roughly reduced from 80 to 40 seconds.  The test runs in all CI jobs
> via GIT_TEST_CLONE_2GB, and is among the slowest tests there.

Nice.

> diff --git a/t/t5608-clone-2gb.sh b/t/t5608-clone-2gb.sh
> index 5d56debf1c..9d7152486e 100755
> --- a/t/t5608-clone-2gb.sh
> +++ b/t/t5608-clone-2gb.sh
> @@ -9,12 +9,28 @@ then
>  	test_done
>  fi
>  
> +size_2gb=2147483648
> +size_4gb=4294967296
> +
> +# The tests below are only meaningful if the packs involved really are
> +# that large, so check it explicitly instead of trusting the setup.
> +test_has_pack_larger_than () {
> +	max=$(test-tool path-utils file-size "$1"/*.pack | sort -n | tail -n 1) &&
> +	max=${max:-0} &&
> +	if test "$max" -le "$2"
> +	then
> +		echo >&2 "largest pack in '$1' is $max bytes, expected more than $2" &&
> +		return 1
> +	fi
> +}
> +
>  test_expect_success 'setup' '
>  	git init 2gb-repo &&
>  	(
>  		cd 2gb-repo &&
>  		git config pack.compression 0 &&
>  		git config pack.depth 0 &&
> +		git config fastimport.unpackLimit 0 &&
>  		blobsize=$((100*1024*1024)) &&
>  		blobcount=$((2*1024*1024*1024/$blobsize+1)) &&
>  		i=1 &&
> @@ -39,14 +55,20 @@ test_expect_success 'setup' '
>  	)
>  '
>  
> +test_expect_success 'setup produces a pack larger than 2GB' '
> +	test_has_pack_larger_than 2gb-repo/.git/objects/pack $size_2gb
> +'

Nit, not worth a reroll: I would have folded this into the test itself.

> @@ -59,24 +81,29 @@ test_expect_success SIZE_T_IS_64BIT,EXPENSIVE 'set up repo with >4GB object' '
>  		4gb-repo/objects/pack/test.pack) &&
>  	git -C 4gb-repo index-pack objects/pack/test.pack &&
>  	git -C 4gb-repo update-ref refs/heads/main $head_oid &&
> -	git -C 4gb-repo symbolic-ref HEAD refs/heads/main
> +	git -C 4gb-repo symbolic-ref HEAD refs/heads/main &&
> +	test_has_pack_larger_than 4gb-repo/objects/pack $size_4gb
>  '
>  
> -test_expect_success SIZE_T_IS_64BIT,EXPENSIVE 'clone >4GB object via unpack-objects' '
> -	test_when_finished rm -rf 4gb-clone-unpack &&
> +test_expect_success SIZE_T_IS_64BIT,EXPENSIVE 'fetch >4GB object via unpack-objects' '
> +	test_when_finished rm -rf 4gb-fetch-unpack &&
>  	# The synthesized pack has five objects, so a large unpack limit keeps
> -	# fetch-pack on the unpack-objects path.
> -	git -c fetch.unpackLimit=100 clone --bare \
> -		"file://$(pwd)/4gb-repo" 4gb-clone-unpack &&
> +	# fetch-pack on the unpack-objects path.  This needs "fetch", as
> +	# "clone" always keeps the received pack.

This is a bit of a surprise to me, I thought that it would apply to both
git-clone(1) and git-fetch(1). Did you by any chance check whether this
is intentional or whteher it's another bug?

> +	git init --bare 4gb-fetch-unpack &&
> +	GIT_TRACE2_EVENT="$(pwd)/trace-unpack" \
> +		git -C 4gb-fetch-unpack -c fetch.unpackLimit=100 fetch \
> +		"file://$(pwd)/4gb-repo" main:refs/heads/main &&
> +	test_subcommand_flex git unpack-objects <trace-unpack &&
>  
> -	# Verify the large blob survived the clone by comparing its OID
> -	# between source and clone.  We cannot use "cat-file -s" because
> +	# Verify the large blob survived the fetch by comparing its OID
> +	# between source and destination.  We cannot use "cat-file -s" because
>  	# object_info.sizep is still unsigned long, which truncates >4GB
>  	# sizes on Windows.  OID equality proves content integrity since
> -	# the clone already verified checksums via index-pack/unpack-objects.
> +	# the fetch already verified checksums via unpack-objects.
>  	source_blob=$(git -C 4gb-repo rev-parse main^:file) &&
> -	clone_blob=$(git -C 4gb-clone-unpack rev-parse main^:file) &&
> -	test "$source_blob" = "$clone_blob"
> +	fetched_blob=$(git -C 4gb-fetch-unpack rev-parse main^:file) &&
> +	test "$source_blob" = "$fetched_blob"
>  '

Other than that this looks good to me, thanks.

Patrick