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

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

[Kristofer Karlsson via GitGitGadget]

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

[PATCH 1/2] unpack-objects: pass the size of a streamed blob as size_t

[Kristofer Karlsson via GitGitGadget]

From: Kristofer Karlsson <krka@spotify.com>

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.

Signed-off-by: Kristofer Karlsson <krka@spotify.com>
---
 builtin/unpack-objects.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/unpack-objects.c b/builtin/unpack-objects.c
index 351948724a..d2f25bed49 100644
--- a/builtin/unpack-objects.c
+++ b/builtin/unpack-objects.c
@@ -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

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

[Kristofer Karlsson via GitGitGadget]

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

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

[Patrick Steinhardt]

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

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

[Kristofer Karlsson]

On Fri, 6 Nov 2026 at 15:54, Patrick Steinhardt <ps@pks.im> wrote:
>
> > 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?

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.

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

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.

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

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.

> > +     # 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?

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.

> Other than that this looks good to me, thanks.
>
> Patrick

Thanks for the review!
Kristofer