{"thread":{"id":"59911","subject":"[PATCH] apply: improve error messages when reading patch","startedAt":"2023-06-26T09:37:41Z","lastAt":"2023-06-26T15:58:35Z","messageCount":2,"participants":["Phillip Wood via GitGitGadget","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"478789","messageId":"pull.1552.git.1687772253869.gitgitgadget@gmail.com","threadId":"59911","inReplyTo":null,"subject":"[PATCH] apply: improve error messages when reading patch","fromName":"Phillip Wood via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-06-26T09:37:33Z","receivedAt":"2023-06-26T09:37:41Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood <phillip.wood@dunelm.org.uk>\n\nCommit f1c0e3946e (apply: reject patches larger than ~1 GiB, 2022-10-25)\nadded a limit on the size of patch that apply will process to avoid\ninteger overflows. The implementation re-used the existing error message\nfor when we are unable to read the patch. This is unfortunate because (a) it\ndoes not signal to the user that the patch is being rejected because it\nis too large and (b) it uses error_errno() without setting errno.\n\nThis patch adds a specific error message for the case when a patch is\ntoo large. It also updates the existing message to make it clearer that\nit is the patch that cannot be read rather than any other file and marks\nboth messages for translation. The \"git apply\" prefix is also dropped to\nmatch most of the rest of the error messages in apply.c (there are still\na few error messages that prefixed with \"git apply\" and are not marked\nfor translation after this patch). The test added in f1c0e3946e is\nupdated accordingly.\n\nReported-by: Premek Vysoky <Premek.Vysoky@microsoft.com>\nSigned-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n---\n    apply: improve error messages when reading patch\n    \n    I haven't changed it but I found the test a bit confusing as it\n    superficially looks like it is generating a valid patch that is too\n    large, but it isn't actually a valid patch because the changed line is\n    lacking a leading '+' and a trailing '\\n'. As we are checking for the\n    error message that does not matter but it might be clearer to just do\n    \n    sz=$((1024 * 1024 * 1023)) &&\n    test-tool genzeros $sz | test_must_fail git apply &&\n    grep \"patch too large\" err\n    \n    \n    if we don't care about the patch being valid. Alternatively we could\n    create a valid patch with\n    \n    sz=$((1024 * 1024 * 1023)) &&\n    {\n        cat <<-\\EOF &&\n        diff --git a/file b/file\n        new file mode 100644\n        --- /dev/null\n        +++ b/file\n        @@ -0,0 +1 @@\n        EOF\n        printf \"+%${sz}s\\n\" x\n    } | test_must_fail git apply 2>err &&\n    grep \"patch too large\" err\n    \n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1552%2Fphillipwood%2Fapply-error-message-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1552/phillipwood/apply-error-message-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1552\n\n apply.c                    | 7 ++++---\n t/t4141-apply-too-large.sh | 2 +-\n 2 files changed, 5 insertions(+), 4 deletions(-)\n\ndiff --git a/apply.c b/apply.c\nindex 6212ab3a1b3..21c7f92ada8 100644\n--- a/apply.c\n+++ b/apply.c\n@@ -410,9 +410,10 @@ static void say_patch_name(FILE *output, const char *fmt, struct patch *patch)\n \n static int read_patch_file(struct strbuf *sb, int fd)\n {\n-\tif (strbuf_read(sb, fd, 0) < 0 || sb->len >= MAX_APPLY_SIZE)\n-\t\treturn error_errno(\"git apply: failed to read\");\n-\n+\tif (strbuf_read(sb, fd, 0) < 0)\n+\t\treturn error_errno(_(\"failed to read patch\"));\n+\telse if (sb->len >= MAX_APPLY_SIZE)\n+\t\treturn error(_(\"patch too large\"));\n \t/*\n \t * Make sure that we have some slop in the buffer\n \t * so that we can do speculative \"memcmp\" etc, and\ndiff --git a/t/t4141-apply-too-large.sh b/t/t4141-apply-too-large.sh\nindex 58742d4fc5d..20cc1209f62 100755\n--- a/t/t4141-apply-too-large.sh\n+++ b/t/t4141-apply-too-large.sh\n@@ -17,7 +17,7 @@ test_expect_success EXPENSIVE 'git apply rejects patches that are too large' '\n \t\tEOF\n \t\ttest-tool genzeros\n \t} | test_copy_bytes $sz | test_must_fail git apply 2>err &&\n-\tgrep \"git apply: failed to read\" err\n+\tgrep \"patch too large\" err\n '\n \n test_done\n\nbase-commit: fe86abd7511a9a6862d5706c6fa1d9b57a63ba09\n-- \ngitgitgadget\n"},{"id":"478798","messageId":"xmqqjzvq5i70.fsf@gitster.g","threadId":"59911","inReplyTo":"pull.1552.git.1687772253869.gitgitgadget@gmail.com","subject":"Re: [PATCH] apply: improve error messages when reading patch","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-06-26T15:58:27Z","receivedAt":"2023-06-26T15:58:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Phillip Wood via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> From: Phillip Wood <phillip.wood@dunelm.org.uk>\n>\n> Commit f1c0e3946e (apply: reject patches larger than ~1 GiB, 2022-10-25)\n> added a limit on the size of patch that apply will process to avoid\n> integer overflows. The implementation re-used the existing error message\n> for when we are unable to read the patch. This is unfortunate because (a) it\n> does not signal to the user that the patch is being rejected because it\n> is too large and (b) it uses error_errno() without setting errno.\n\nGood description of the issue, and ...\n\n>  static int read_patch_file(struct strbuf *sb, int fd)\n>  {\n> -\tif (strbuf_read(sb, fd, 0) < 0 || sb->len >= MAX_APPLY_SIZE)\n> -\t\treturn error_errno(\"git apply: failed to read\");\n> -\n> +\tif (strbuf_read(sb, fd, 0) < 0)\n> +\t\treturn error_errno(_(\"failed to read patch\"));\n> +\telse if (sb->len >= MAX_APPLY_SIZE)\n> +\t\treturn error(_(\"patch too large\"));\n\n... quite obvious improvement.  Nice.\n\nWill queue.  Thanks.\n"}]}