[PATCH] pack-check: fix bogus corruption report with core.bigFileThreshold

[PATCH] pack-check: fix bogus corruption report with core.bigFileThreshold

[Patrick Steinhardt]

When verifying packed blobs for consistency we have two different paths:

  - When the blob is larger than or equal to "core.bigFileThreshold"
    we verify its object ID by streaming it.

  - Otherwise we read the object's data into memory and then hash it.

When a packed blob is exactly at the threshold then we'll also take the
streaming path. This is at odds with the rest of the codebase though,
where we consider an object large only when it exceeds the threshold.
This off-by-one error was introduced via ec9d224903 (fsck: use streaming
interface for large blobs in pack, 2016-07-13), but was mostly harmless.

But starting with 13eb65d366 (pack-check: fix verification of large
objects, 2026-02-23), we use `packfile_read_object_stream()` directly to
read the blob, and that function will always return an error when
invoked with a blob smaller than or equal to the threshold. So in the
particular case where object size and threshold match, we'll try to take
the streaming path, but the streaming path will always refuse creating
the stream. Consequently, we'll treat the object as corrupt during
git-fsck(1).

Now it's somewhat dubious that `packfile_read_object_stream()` would
refuse creating the stream in the first place. The intent of this is for
`istream_source()` (and thus `odb_stream_from_object()`, its only
caller) to fall back to reading the whole object into memory when the
blob is below the big file threshold. This allows us to inflate smallish
objects in a single go, which is supposedly more efficient. But even so,
this is a policy-based decision, and arguably the backend should not
worry about such decisions but just honor the request.

The most sensible fix for this would be to lift the logic out of
`packfile_read_object_stream()` and into `istream_source()`, and if so
the call to this function in `verify_packfile()` would always succeed.
But that would require us to do two reads in `istream_source()`: one to
derive the object's size, and one to construct either the in-core or
the proper stream. And that would negatively impact performance. It's
unlikely that anybody really cares about the performance impact, but I'm
still a bit hesitant.

Instead, let's do the dumb-and-easy fix for now, which is to correct the
threshold to be inclusive, same as everywhere else in our codebase.

Reported-by: André Düwel <duewel1982@googlemail.com>
Original-patch-by: André Düwel <duewel1982@googlemail.com>
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
Hi,

this was reported via [1]. The original reporter has also proposed a fix
that I mostly took as-is while rewriting the commit message, but we
agreed that I'll take it from there.

Thanks!

Patrick

[1]: https://gitlab.com/gitlab-org/gitaly/-/work_items/7397
---
 pack-check.c    |  2 +-
 t/t1450-fsck.sh | 13 +++++++++++++
 2 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/pack-check.c b/pack-check.c
index c7275f4b92..605811b7b0 100644
--- a/pack-check.c
+++ b/pack-check.c
@@ -134,7 +134,7 @@ static int verify_packfile(struct repository *r,
 		unuse_pack(w_curs);
 
 		if (type == OBJ_BLOB &&
-		    repo_settings_get_big_file_threshold(r) <= size) {
+		    repo_settings_get_big_file_threshold(r) < size) {
 			/*
 			 * Let stream_object_signature() check it with
 			 * the streaming interface; no point slurping
diff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh
index 1b4074304c..16cc5d5dcb 100755
--- a/t/t1450-fsck.sh
+++ b/t/t1450-fsck.sh
@@ -1152,4 +1152,17 @@ test_expect_success 'fsck reports problems in current worktree index without fil
 	test_cmp expect actual
 '
 
+test_expect_success 'fsck with blob size equal to core.bigFileThreshold' '
+	test_when_finished rm -rf big-blob-fsck &&
+	git init big-blob-fsck &&
+	(
+		cd big-blob-fsck &&
+		test-tool genzeros 1024 >blob &&
+		git add blob &&
+		git commit -m "blob equal to threshold" &&
+		git repack -ad &&
+		git -c core.bigFileThreshold=1k fsck --no-dangling
+	)
+'
+
 test_done

---
base-commit: 6de20f6092dcf9bdb1c8efe03db4b70c82b423dd
change-id: 20261014-b4-pks-pack-check-matching-large-file-threshold-fd7be721d6a4

Re: [PATCH] pack-check: fix bogus corruption report with core.bigFileThreshold

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> When verifying packed blobs for consistency we have two different paths:
>
>   - When the blob is larger than or equal to "core.bigFileThreshold"
>     we verify its object ID by streaming it.
>
>   - Otherwise we read the object's data into memory and then hash it.
>
> When a packed blob is exactly at the threshold then we'll also take the
> streaming path. This is at odds with the rest of the codebase though,
> where we consider an object large only when it exceeds the threshold.

> diff --git a/pack-check.c b/pack-check.c
> index c7275f4b92..605811b7b0 100644
> --- a/pack-check.c
> +++ b/pack-check.c
> @@ -134,7 +134,7 @@ static int verify_packfile(struct repository *r,
>  		unuse_pack(w_curs);
>  
>  		if (type == OBJ_BLOB &&
> -		    repo_settings_get_big_file_threshold(r) <= size) {
> +		    repo_settings_get_big_file_threshold(r) < size) {
>  			/*
>  			 * Let stream_object_signature() check it with
>  			 * the streaming interface; no point slurping

OK, this signals a later step with the (data==NULL, data_valid==0)
that the size of the pack entry is sufficiently large that we do not
want to hold it in-core only to verify the object name.  And the later
step calls packfile_read_object_stream() to prepare an istream.

I agree with you that "Use the streaming interface only for objects
larger than what we are willing to hold in core" is a policy thing,
and checking the policy in packfile_read_object_stream() is dubious,
but since the above is checking the same policy, so we should do the
comparison the same way.

> diff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh
> index 1b4074304c..16cc5d5dcb 100755
> --- a/t/t1450-fsck.sh
> +++ b/t/t1450-fsck.sh
> @@ -1152,4 +1152,17 @@ test_expect_success 'fsck reports problems in current worktree index without fil
>  	test_cmp expect actual
>  '
>  
> +test_expect_success 'fsck with blob size equal to core.bigFileThreshold' '
> +	test_when_finished rm -rf big-blob-fsck &&
> +	git init big-blob-fsck &&
> +	(
> +		cd big-blob-fsck &&
> +		test-tool genzeros 1024 >blob &&
> +		git add blob &&
> +		git commit -m "blob equal to threshold" &&
> +		git repack -ad &&
> +		git -c core.bigFileThreshold=1k fsck --no-dangling
> +	)
> +'

Looks obviously correct.  Will queue.  Thanks.

> +
>  test_done
>
> ---
> base-commit: 6de20f6092dcf9bdb1c8efe03db4b70c82b423dd
> change-id: 20261014-b4-pks-pack-check-matching-large-file-threshold-fd7be721d6a4