{"thread":{"id":"62163","subject":"git diff --exit-code returns 0 when binary files differ","startedAt":"2024-09-21T04:26:40Z","lastAt":"2024-09-25T21:52:14Z","messageCount":4,"participants":["Kohei Shibata","René Scharfe","Thomas Braun","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"503182","messageId":"CACpkL8WsNqhQ7SP27-XQwp1bzKjyUT6m2idFarZ2Z5rLVYg4pQ@mail.gmail.com","threadId":"62163","inReplyTo":null,"subject":"git diff --exit-code returns 0 when binary files differ","fromName":"Kohei Shibata","fromEmail":"shiba200712@gmail.com","sentAt":"2024-09-21T04:26:27Z","receivedAt":"2024-09-21T04:26:40Z","isPatch":false,"sender":{"key":"shiba200712@gmail.com","avatar":null},"body":"I've encountered an issue with `git diff --exit-code` where it returns\n0 for binary files that have actual changes.\n\n> What did you do before the bug happened? (Steps to reproduce your issue)\n\n1. Initialize a new git repository:\n```\ngit init\n```\n\n2. Create a binary file and commit it:\n```\necho '*.bin binary' > .gitattributes\ndd if=/dev/urandom of=a.bin bs=32 count=1\ngit add .\ngit commit -m 'commit'\n```\n\n3. Modify the binary file:\n```\necho a > a.bin\ngit diff --exit-code # says \"Binary files a/a.bin and b/a.bin differ\"\necho $? # returns 0\n```\n\n> What did you expect to happen? (Expected behavior)\n\n`git diff --exit-code` should exit with 1\n\n> What happened instead? (Actual behavior)\n\n`git diff --exit-code` returns 0 even when the binary file is modified.\n\n> Anything else you want to add:\n\nI could not find the exact condition to change exit code. In some\ncases, depending on the content of the file, `git diff --exit-code`\ndoes return 1 as expected.\nI don't use an external diff tool.\n\n\n[System Info]\ngit version:\ngit version 2.46.1\ncpu: x86_64\nno commit associated with this build\nsizeof-long: 8\nsizeof-size_t: 8\nshell-path: /bin/sh\nlibcurl: 7.68.0\nzlib: 1.2.11\nuname: Linux 5.15.153.1-microsoft-standard-WSL2 #1 SMP Fri Mar 29\n23:14:13 UTC 2024 x86_64\ncompiler info: gnuc: 9.4\nlibc info: glibc: 2.31\n$SHELL (typically, interactive shell): /bin/bash\n\n\n[Enabled Hooks]\n\n\nBest regards,\nKohei\n"},{"id":"503195","messageId":"500a8e0a-9fbd-4b7b-b2f2-026a4293bc9f@web.de","threadId":"62163","inReplyTo":"CACpkL8WsNqhQ7SP27-XQwp1bzKjyUT6m2idFarZ2Z5rLVYg4pQ@mail.gmail.com","subject":"[PATCH] diff: report modified binary files as changes in builtin_diff()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2024-09-21T15:09:54Z","receivedAt":"2024-09-21T15:10:03Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"The diff machinery has two ways to detect changes to set the exit code:\nJust comparing hashes and comparing blob contents. The latter is needed\nif certain changes have to be ignored, e.g. with --ignore-space-change\nor --ignore-matching-lines. It's enabled by the diff_options flag\ndiff_from_contents.\n\nThe code for handling binary files added by 1aaf69e669 (diff: shortcut\nfor diff'ing two binary SHA-1 objects, 2014-08-16) always uses a quick\nhash-only comparison, even if the slow way is taken. We need it to\nreport a hash difference as a change for the purpose of setting the\nexit code, though, but it never did. Fix that.\n\nd7b97b7185 (diff: let external diffs report that changes are\nuninteresting, 2024-06-09) set diff_from_contents if external diff\nprograms are allowed. This is the default e.g. for git diff, and so\nthat change exposed the inconsistency much more widely.\n\nReported-by: Kohei Shibata \nSigned-off-by: René Scharfe \n---\nThank you for the report!\n\n diff.c | 1 +\n t/t4017-diff-retval.sh | 8 ++++++++\n 2 files changed, 9 insertions(+)\n\ndiff --git a/diff.c b/diff.c\nindex 3be927b073..84a6bb0868 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -3675,6 +3675,7 @@ static void builtin_diff(const char *name_a,\n \t\t\temit_diff_symbol(o, DIFF_SYMBOL_BINARY_FILES,\n \t\t\t\t\t sb.buf, sb.len, 0);\n \t\t\tstrbuf_release(&sb);\n+\t\t\to->found_changes = 1;\n \t\t\tgoto free_ab_and_return;\n \t\t}\n \t\tif (fill_mmfile(o->repo, &mf1, one) < 0 ||\ndiff --git a/t/t4017-diff-retval.sh b/t/t4017-diff-retval.sh\nindex d644310e22..1cea73ef5a 100755\n--- a/t/t4017-diff-retval.sh\n+++ b/t/t4017-diff-retval.sh\n@@ -145,6 +145,14 @@ test_expect_success 'option errors are not confused by --exit-code' '\n\n for option in --exit-code --quiet\n do\n+\ttest_expect_success \"git diff $option returns 1 for changed binary file\" \"\n+\t\ttest_when_finished 'rm -f .gitattributes' &&\n+\t\tgit reset --hard &&\n+\t\techo a binary >.gitattributes &&\n+\t\techo 2 >>a &&\n+\t\ttest_expect_code 1 git diff $option\n+\t\"\n+\n \ttest_expect_success \"git diff $option returns 1 for copied file\" \"\n \t\tgit reset --hard &&\n \t\tcp a copy &&\n--\n2.46.0\n"},{"id":"503497","messageId":"3192d8f4-4c7f-4b32-b564-7e075132c41c@virtuell-zuhause.de","threadId":"62163","inReplyTo":"500a8e0a-9fbd-4b7b-b2f2-026a4293bc9f@web.de","subject":"Re: [PATCH] diff: report modified binary files as changes in builtin_diff()","fromName":"Thomas Braun","fromEmail":"thomas.braun@virtuell-zuhause.de","sentAt":"2024-09-25T21:24:18Z","receivedAt":"2024-09-25T21:46:47Z","isPatch":true,"sender":{"key":"thomas.braun@virtuell-zuhause.de","avatar":"https://avatars.githubusercontent.com/u/1185677?v=4"},"body":"Am 21.09.2024 um 17:09 schrieb René Scharfe:\n\nHi René,\n\n> diff --git a/diff.c b/diff.c\n> index 3be927b073..84a6bb0868 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -3675,6 +3675,7 @@ static void builtin_diff(const char *name_a,\n> \t\t\temit_diff_symbol(o, DIFF_SYMBOL_BINARY_FILES,\n> \t\t\t\t\t sb.buf, sb.len, 0);\n> \t\t\tstrbuf_release(&sb);\n> +\t\t\to->found_changes = 1;\n> \t\t\tgoto free_ab_and_return;\n> \t\t}\n> \t\tif (fill_mmfile(o->repo, &mf1, one) < 0 ||\n\nI poked at the same issue in parallel and had the same fix, but ...\n\n> diff --git a/t/t4017-diff-retval.sh b/t/t4017-diff-retval.sh\n> index d644310e22..1cea73ef5a 100755\n> --- a/t/t4017-diff-retval.sh\n> +++ b/t/t4017-diff-retval.sh\n> @@ -145,6 +145,14 @@ test_expect_success 'option errors are not confused by --exit-code' '\n> \n> for option in --exit-code --quiet\n> do\n> +\ttest_expect_success \"git diff $option returns 1 for changed binary file\" \"\n> +\t\ttest_when_finished 'rm -f .gitattributes' &&\n> +\t\tgit reset --hard &&\n> +\t\techo a binary >.gitattributes &&\n> +\t\techo 2 >>a &&\n> +\t\ttest_expect_code 1 git diff $option\n> +\t\"\n> +\n> \ttest_expect_success \"git diff $option returns 1 for copied file\" \"\n> \t\tgit reset --hard &&\n> \t\tcp a copy &&\n\nyour test is nicer.\n\nThe patch works here locally.\n\nFor what it's worth:\n\nReviewed-by: Thomas Braun \n\nThomas\n\n"},{"id":"503498","messageId":"xmqq7cazo0z8.fsf@gitster.g","threadId":"62163","inReplyTo":"3192d8f4-4c7f-4b32-b564-7e075132c41c@virtuell-zuhause.de","subject":"Re: [PATCH] diff: report modified binary files as changes in builtin_diff()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-09-25T21:52:11Z","receivedAt":"2024-09-25T21:52:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thomas Braun writes:\n\n> Am 21.09.2024 um 17:09 schrieb René Scharfe:\n>\n> Hi René,\n>\n>> diff --git a/diff.c b/diff.c\n>> index 3be927b073..84a6bb0868 100644\n>> --- a/diff.c\n>> +++ b/diff.c\n>> @@ -3675,6 +3675,7 @@ static void builtin_diff(const char *name_a,\n>> \t\t\temit_diff_symbol(o, DIFF_SYMBOL_BINARY_FILES,\n>> \t\t\t\t\t sb.buf, sb.len, 0);\n>> \t\t\tstrbuf_release(&sb);\n>> +\t\t\to->found_changes = 1;\n>> \t\t\tgoto free_ab_and_return;\n>> \t\t}\n>> \t\tif (fill_mmfile(o->repo, &mf1, one) < 0 ||\n>\n> I poked at the same issue in parallel and had the same fix, but ...\n>\n>> ...\n>> \ttest_expect_success \"git diff $option returns 1 for copied file\" \"\n>> \t\tgit reset --hard &&\n>> \t\tcp a copy &&\n>\n> your test is nicer.\n>\n> The patch works here locally.\n>\n> For what it's worth:\n>\n> Reviewed-by: Thomas Braun \n>\n> Thomas\n\nThanks.\n"}]}