{"thread":{"id":"66532","subject":"[PATCH] pack-check: fix bogus corruption report with core.bigFileThreshold","startedAt":"2026-10-14T10:49:19Z","lastAt":"2026-10-14T17:21:04Z","messageCount":2,"participants":["Patrick Steinhardt","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"555001","messageId":"20261014-b4-pks-pack-check-matching-large-file-threshold-v1-1-de6d8455eaaa@pks.im","threadId":"66532","inReplyTo":null,"subject":"[PATCH] pack-check: fix bogus corruption report with core.bigFileThreshold","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-10-14T10:49:19Z","receivedAt":"2026-10-14T10:49:19Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When verifying packed blobs for consistency we have two different paths:\n\n  - When the blob is larger than or equal to \"core.bigFileThreshold\"\n    we verify its object ID by streaming it.\n\n  - Otherwise we read the object's data into memory and then hash it.\n\nWhen a packed blob is exactly at the threshold then we'll also take the\nstreaming path. This is at odds with the rest of the codebase though,\nwhere we consider an object large only when it exceeds the threshold.\nThis off-by-one error was introduced via ec9d224903 (fsck: use streaming\ninterface for large blobs in pack, 2016-07-13), but was mostly harmless.\n\nBut starting with 13eb65d366 (pack-check: fix verification of large\nobjects, 2026-02-23), we use `packfile_read_object_stream()` directly to\nread the blob, and that function will always return an error when\ninvoked with a blob smaller than or equal to the threshold. So in the\nparticular case where object size and threshold match, we'll try to take\nthe streaming path, but the streaming path will always refuse creating\nthe stream. Consequently, we'll treat the object as corrupt during\ngit-fsck(1).\n\nNow it's somewhat dubious that `packfile_read_object_stream()` would\nrefuse creating the stream in the first place. The intent of this is for\n`istream_source()` (and thus `odb_stream_from_object()`, its only\ncaller) to fall back to reading the whole object into memory when the\nblob is below the big file threshold. This allows us to inflate smallish\nobjects in a single go, which is supposedly more efficient. But even so,\nthis is a policy-based decision, and arguably the backend should not\nworry about such decisions but just honor the request.\n\nThe most sensible fix for this would be to lift the logic out of\n`packfile_read_object_stream()` and into `istream_source()`, and if so\nthe call to this function in `verify_packfile()` would always succeed.\nBut that would require us to do two reads in `istream_source()`: one to\nderive the object's size, and one to construct either the in-core or\nthe proper stream. And that would negatively impact performance. It's\nunlikely that anybody really cares about the performance impact, but I'm\nstill a bit hesitant.\n\nInstead, let's do the dumb-and-easy fix for now, which is to correct the\nthreshold to be inclusive, same as everywhere else in our codebase.\n\nReported-by: André Düwel <duewel1982@googlemail.com>\nOriginal-patch-by: André Düwel <duewel1982@googlemail.com>\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\nHi,\n\nthis was reported via [1]. The original reporter has also proposed a fix\nthat I mostly took as-is while rewriting the commit message, but we\nagreed that I'll take it from there.\n\nThanks!\n\nPatrick\n\n[1]: https://gitlab.com/gitlab-org/gitaly/-/work_items/7397\n---\n pack-check.c    |  2 +-\n t/t1450-fsck.sh | 13 +++++++++++++\n 2 files changed, 14 insertions(+), 1 deletion(-)\n\ndiff --git a/pack-check.c b/pack-check.c\nindex c7275f4b92..605811b7b0 100644\n--- a/pack-check.c\n+++ b/pack-check.c\n@@ -134,7 +134,7 @@ static int verify_packfile(struct repository *r,\n \t\tunuse_pack(w_curs);\n \n \t\tif (type == OBJ_BLOB &&\n-\t\t    repo_settings_get_big_file_threshold(r) <= size) {\n+\t\t    repo_settings_get_big_file_threshold(r) < size) {\n \t\t\t/*\n \t\t\t * Let stream_object_signature() check it with\n \t\t\t * the streaming interface; no point slurping\ndiff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh\nindex 1b4074304c..16cc5d5dcb 100755\n--- a/t/t1450-fsck.sh\n+++ b/t/t1450-fsck.sh\n@@ -1152,4 +1152,17 @@ test_expect_success 'fsck reports problems in current worktree index without fil\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'fsck with blob size equal to core.bigFileThreshold' '\n+\ttest_when_finished rm -rf big-blob-fsck &&\n+\tgit init big-blob-fsck &&\n+\t(\n+\t\tcd big-blob-fsck &&\n+\t\ttest-tool genzeros 1024 >blob &&\n+\t\tgit add blob &&\n+\t\tgit commit -m \"blob equal to threshold\" &&\n+\t\tgit repack -ad &&\n+\t\tgit -c core.bigFileThreshold=1k fsck --no-dangling\n+\t)\n+'\n+\n test_done\n\n---\nbase-commit: 6de20f6092dcf9bdb1c8efe03db4b70c82b423dd\nchange-id: 20261014-b4-pks-pack-check-matching-large-file-threshold-fd7be721d6a4\n\n\n"},{"id":"555051","messageId":"xmqq8q40tczj.fsf@gitster.g","threadId":"66532","inReplyTo":"20261014-b4-pks-pack-check-matching-large-file-threshold-v1-1-de6d8455eaaa@pks.im","subject":"Re: [PATCH] pack-check: fix bogus corruption report with core.bigFileThreshold","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-10-14T17:21:04Z","receivedAt":"2026-10-14T17:21:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n> When verifying packed blobs for consistency we have two different paths:\n>\n>   - When the blob is larger than or equal to \"core.bigFileThreshold\"\n>     we verify its object ID by streaming it.\n>\n>   - Otherwise we read the object's data into memory and then hash it.\n>\n> When a packed blob is exactly at the threshold then we'll also take the\n> streaming path. This is at odds with the rest of the codebase though,\n> where we consider an object large only when it exceeds the threshold.\n\n> diff --git a/pack-check.c b/pack-check.c\n> index c7275f4b92..605811b7b0 100644\n> --- a/pack-check.c\n> +++ b/pack-check.c\n> @@ -134,7 +134,7 @@ static int verify_packfile(struct repository *r,\n>  \t\tunuse_pack(w_curs);\n>  \n>  \t\tif (type == OBJ_BLOB &&\n> -\t\t    repo_settings_get_big_file_threshold(r) <= size) {\n> +\t\t    repo_settings_get_big_file_threshold(r) < size) {\n>  \t\t\t/*\n>  \t\t\t * Let stream_object_signature() check it with\n>  \t\t\t * the streaming interface; no point slurping\n\nOK, this signals a later step with the (data==NULL, data_valid==0)\nthat the size of the pack entry is sufficiently large that we do not\nwant to hold it in-core only to verify the object name.  And the later\nstep calls packfile_read_object_stream() to prepare an istream.\n\nI agree with you that \"Use the streaming interface only for objects\nlarger than what we are willing to hold in core\" is a policy thing,\nand checking the policy in packfile_read_object_stream() is dubious,\nbut since the above is checking the same policy, so we should do the\ncomparison the same way.\n\n> diff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh\n> index 1b4074304c..16cc5d5dcb 100755\n> --- a/t/t1450-fsck.sh\n> +++ b/t/t1450-fsck.sh\n> @@ -1152,4 +1152,17 @@ test_expect_success 'fsck reports problems in current worktree index without fil\n>  \ttest_cmp expect actual\n>  '\n>  \n> +test_expect_success 'fsck with blob size equal to core.bigFileThreshold' '\n> +\ttest_when_finished rm -rf big-blob-fsck &&\n> +\tgit init big-blob-fsck &&\n> +\t(\n> +\t\tcd big-blob-fsck &&\n> +\t\ttest-tool genzeros 1024 >blob &&\n> +\t\tgit add blob &&\n> +\t\tgit commit -m \"blob equal to threshold\" &&\n> +\t\tgit repack -ad &&\n> +\t\tgit -c core.bigFileThreshold=1k fsck --no-dangling\n> +\t)\n> +'\n\nLooks obviously correct.  Will queue.  Thanks.\n\n> +\n>  test_done\n>\n> ---\n> base-commit: 6de20f6092dcf9bdb1c8efe03db4b70c82b423dd\n> change-id: 20261014-b4-pks-pack-check-matching-large-file-threshold-fd7be721d6a4\n\n"}]}