Git development
 help / color / mirror / Atom feed
From: "Kristofer Karlsson via GitGitGadget" <gitgitgadget@gmail.com>
To: git@vger.kernel.org
Cc: Johannes Schindelin <johannes.schindelin@gmx.de>,
	Kristofer Karlsson <krka@spotify.com>,
	Kristofer Karlsson <krka@spotify.com>
Subject: [PATCH 2/2] t5608: assert that the packs involved really are that large
Date: Fri, 06 Nov 2026 12:51:16 +0000	[thread overview]
Message-ID: <6c6abee65cb3987978b982e69f227e408c0bf408.1793969476.git.gitgitgadget@gmail.com> (raw)
In-Reply-To: <pull.2299.git.1793969476.gitgitgadget@gmail.com>

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.

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.

Signed-off-by: Kristofer Karlsson <krka@spotify.com>
---
 t/t5608-clone-2gb.sh | 54 +++++++++++++++++++++++++++++++++-----------
 1 file changed, 41 insertions(+), 13 deletions(-)

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

  parent reply	other threads:[~2026-11-06 12:51 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-11-06 12:51 [PATCH 0/2] t5608: assert that the packs involved really are that large Kristofer Karlsson via GitGitGadget
2026-11-06 12:51 ` [PATCH 1/2] unpack-objects: pass the size of a streamed blob as size_t Kristofer Karlsson via GitGitGadget
2026-11-06 12:51 ` Kristofer Karlsson via GitGitGadget [this message]
2026-11-06 14:54   ` [PATCH 2/2] t5608: assert that the packs involved really are that large Patrick Steinhardt
2026-11-06 15:12     ` Kristofer Karlsson

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=6c6abee65cb3987978b982e69f227e408c0bf408.1793969476.git.gitgitgadget@gmail.com \
    --to=gitgitgadget@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=johannes.schindelin@gmx.de \
    --cc=krka@spotify.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox