patchpack-check: fix bogus corruption report with core.bigFileThreshold

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(-)

Patrick Steinhardt <ps@pks.im> writes:

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.

Looks obviously correct.  Will queue.  Thanks.