t5608: assert that the packs involved really are that large

I was looking at which tests take the most time in CI, and t5608 stood out when (unscientifically) spot-checking a single run. Profiling its setup showed that it does not actually create the >2GB pack that the test's name promises. While adding assertions for that, it turned out that the unpack-objects variant of the >4GB test never runs unpack-objects either.

Once it does, it fails on Windows, because unpack-objects narrows the size of a streamed blob to "unsigned long" on its way to the object writer. The first patch fixes that.

Kristofer Karlsson (2):
unpack-objects: pass the size of a streamed blob as size_t
t5608: assert that the packs involved really are that large

 builtin/unpack-objects.c |  2 +-
 t/t5608-clone-2gb.sh     | 54 ++++++++++++++++++++++++++++++----------
 2 files changed, 42 insertions(+), 14 deletions(-)

base-commit: 0ffa3e35c7c82ab8d0de226149fb03b3f7674d05
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2299%2Fspkrka%2Ft5608-real-2gb-pack-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2299/spkrka/t5608-real-2gb-pack-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/2299

gitgitgadget

From: Kristofer Karlsson <[email protected]>

Since f2063855fb (index-pack, unpack-objects: use size_t for object size, 2026-05-08), unpack_one() decodes the size of an object into a size_t. But stream_blob(), which is used for blobs larger than core.bigFileThreshold, still takes that size as "unsigned long". Where that type is only 32 bits wide, such as on Windows, the size of a blob larger than 4GB is truncated, and writing the object fails:

fatal: write stream object 4294967304 != 8

This was found by the next commit, which makes t5608 actually exercise the unpack-objects path for a >4GB blob.

Take the size as size_t instead. The struct odb_stream it is stored in already uses size_t, as does the rest of the path that writes the loose object.

---
 builtin/unpack-objects.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 351948724a..d2f25bed49 100644
@@ -386,7 +386,7 @@ static ssize_t zlib_stream_read(struct odb_stream *in_stream,
 	return buf_len - zstream->avail_out;
 }
 
-static void stream_blob(unsigned long size, unsigned nr)
+static void stream_blob(size_t size, unsigned nr)
 {
 	git_zstream zstream = { 0 };
 	struct zlib_stream in_stream = {

gitgitgadget

From: Kristofer Karlsson <[email protected]>

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.

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.

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.

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.

---
 t/t5608-clone-2gb.sh | 54 +++++++++++++++++++++++++++++++++-----------
 1 file changed, 41 insertions(+), 13 deletions(-)

index 5d56debf1c..9d7152486e 100755
@@ -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
+'
+
 test_expect_success 'clone - bare' '
 	test_when_finished rm -rf clone-bare &&
-	git clone --bare --no-hardlinks 2gb-repo clone-bare
+	git clone --bare --no-hardlinks 2gb-repo clone-bare &&
+	test_has_pack_larger_than clone-bare/objects/pack $size_2gb
 '
 
 test_expect_success 'clone - with worktree, file:// protocol' '
 	test_when_finished rm -rf clone-wt &&
-	git clone "file://$(pwd)/2gb-repo" clone-wt
+	git clone "file://$(pwd)/2gb-repo" clone-wt &&
+	test_has_pack_larger_than clone-wt/.git/objects/pack $size_2gb
 '
 
 rm -rf 2gb-repo 2>/dev/null

@@ -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.
+	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"
 '
 
 test_expect_success SIZE_T_IS_64BIT,EXPENSIVE 'clone with >4GB object via index-pack' '

@@ -84,6 +111,7 @@ test_expect_success SIZE_T_IS_64BIT,EXPENSIVE 'clone with >4GB object via index-
 	# Force fetch-pack to hand the pack to index-pack instead.
 	git -c fetch.unpackLimit=1 clone --bare \
 		"file://$(pwd)/4gb-repo" 4gb-clone-index &&
+	test_has_pack_larger_than 4gb-clone-index/objects/pack $size_4gb &&
 
 	source_blob=$(git -C 4gb-repo rev-parse main^:file) &&
 	clone_blob=$(git -C 4gb-clone-index rev-parse main^:file) &&

gitgitgadget

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

Makes sense.

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

Nice.

Yes, I think that's the case (though I could be wrong about it). In any case, I think regardless of if it was broken from the start or not, it makes sense to ensure it can't be silently broken by any other future optimization that shrinks the pack file sizes.

Exactly, I would have preferred to just send the test improvement but it turned out that fixing the problem was easier than making the test more flexible, so I added that as a prerequisite patch.

I did consider that, but figured this was not too much overhead, and made it slightly easier to read. That said, if I need to reroll for other reasons, I can definitely inline it into the setup instead.

It appears to be intentional (and old). In the commit ad89721508 (fetch-pack: -k option to keep downloaded pack., 2005-12-14) says:

We earlier had something like that on clone-pack side once, but the list discussion resulted in the decision that it makes sense to always keep the pack for clone-pack, so unpacking option is not enabled on the clone-pack side, [...]

and the builtin clone kept that behaviour by always setting TRANS_OPT_KEEP, since 8434c2f1af (Build in clone, 2008-04-27). So the unpack limits have only ever applied to fetch.

Thanks for the review!
Kristofer