From a9456705f1884c95e597b25df9435b602caa07e5 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Wed, 14 Oct 2026 12:49:19 +0200 Subject: [PATCH] pack-check: fix bogus corruption report with core.bigFileThreshold MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Original-patch-by: André Düwel Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- 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 5adfb3f272..a3d84bccee 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 54e81c2636..9b1fd4c969 100755 --- a/t/t1450-fsck.sh +++ b/t/t1450-fsck.sh @@ -1130,4 +1130,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