# git diff --exit-code returns 0 when binary files differ 4 messages from 2024-09-21 to 2024-09-25. Participants: Kohei Shibata, René Scharfe, Thomas Braun, Junio C Hamano. Thread: https://gitlist.dev/t/62163 ## Kohei Shibata, 2024-09-21 04:26 Subject: git diff --exit-code returns 0 when binary files differ Message-ID: URL: https://gitlist.dev/e/CACpkL8WsNqhQ7SP27-XQwp1bzKjyUT6m2idFarZ2Z5rLVYg4pQ%40mail.gmail.com ``` I've encountered an issue with `git diff --exit-code` where it returns 0 for binary files that have actual changes. > What did you do before the bug happened? (Steps to reproduce your issue) 1. Initialize a new git repository: ˋˋˋ git init ˋˋˋ 2. Create a binary file and commit it: ˋˋˋ echo '*.bin binary' > .gitattributes dd if=/dev/urandom of=a.bin bs=32 count=1 git add . git commit -m 'commit' ˋˋˋ 3. Modify the binary file: ˋˋˋ echo a > a.bin git diff --exit-code # says "Binary files a/a.bin and b/a.bin differ" echo $? # returns 0 ˋˋˋ > What did you expect to happen? (Expected behavior) `git diff --exit-code` should exit with 1 > What happened instead? (Actual behavior) `git diff --exit-code` returns 0 even when the binary file is modified. > Anything else you want to add: I could not find the exact condition to change exit code. In some cases, depending on the content of the file, `git diff --exit-code` does return 1 as expected. I don't use an external diff tool. [System Info] git version: git version 2.46.1 cpu: x86_64 no commit associated with this build sizeof-long: 8 sizeof-size_t: 8 shell-path: /bin/sh libcurl: 7.68.0 zlib: 1.2.11 uname: Linux 5.15.153.1-microsoft-standard-WSL2 #1 SMP Fri Mar 29 23:14:13 UTC 2024 x86_64 compiler info: gnuc: 9.4 libc info: glibc: 2.31 $SHELL (typically, interactive shell): /bin/bash [Enabled Hooks] Best regards, Kohei ``` ## René Scharfe, 2024-09-21 15:09 Subject: [PATCH] diff: report modified binary files as changes in builtin_diff() Message-ID: <500a8e0a-9fbd-4b7b-b2f2-026a4293bc9f@web.de> URL: https://gitlist.dev/e/500a8e0a-9fbd-4b7b-b2f2-026a4293bc9f%40web.de In-Reply-To: ``` The diff machinery has two ways to detect changes to set the exit code: Just comparing hashes and comparing blob contents. The latter is needed if certain changes have to be ignored, e.g. with --ignore-space-change or --ignore-matching-lines. It's enabled by the diff_options flag diff_from_contents. The code for handling binary files added by 1aaf69e669 (diff: shortcut for diff'ing two binary SHA-1 objects, 2014-08-16) always uses a quick hash-only comparison, even if the slow way is taken. We need it to report a hash difference as a change for the purpose of setting the exit code, though, but it never did. Fix that. d7b97b7185 (diff: let external diffs report that changes are uninteresting, 2024-06-09) set diff_from_contents if external diff programs are allowed. This is the default e.g. for git diff, and so that change exposed the inconsistency much more widely. Reported-by: Kohei Shibata Signed-off-by: René Scharfe --- Thank you for the report! diff.c | 1 + t/t4017-diff-retval.sh | 8 ++++++++ 2 files changed, 9 insertions(+) diff --git a/diff.c b/diff.c index 3be927b073..84a6bb0868 100644 --- a/diff.c +++ b/diff.c @@ -3675,6 +3675,7 @@ static void builtin_diff(const char *name_a, emit_diff_symbol(o, DIFF_SYMBOL_BINARY_FILES, sb.buf, sb.len, 0); strbuf_release(&sb); + o->found_changes = 1; goto free_ab_and_return; } if (fill_mmfile(o->repo, &mf1, one) < 0 || diff --git a/t/t4017-diff-retval.sh b/t/t4017-diff-retval.sh index d644310e22..1cea73ef5a 100755 --- a/t/t4017-diff-retval.sh +++ b/t/t4017-diff-retval.sh @@ -145,6 +145,14 @@ test_expect_success 'option errors are not confused by --exit-code' ' for option in --exit-code --quiet do + test_expect_success "git diff $option returns 1 for changed binary file" " + test_when_finished 'rm -f .gitattributes' && + git reset --hard && + echo a binary >.gitattributes && + echo 2 >>a && + test_expect_code 1 git diff $option + " + test_expect_success "git diff $option returns 1 for copied file" " git reset --hard && cp a copy && -- 2.46.0 ``` ## Thomas Braun, 2024-09-25 21:24 Subject: Re: [PATCH] diff: report modified binary files as changes in builtin_diff() Message-ID: <3192d8f4-4c7f-4b32-b564-7e075132c41c@virtuell-zuhause.de> URL: https://gitlist.dev/e/3192d8f4-4c7f-4b32-b564-7e075132c41c%40virtuell-zuhause.de In-Reply-To: <500a8e0a-9fbd-4b7b-b2f2-026a4293bc9f@web.de> ``` Am 21.09.2024 um 17:09 schrieb René Scharfe: Hi René, > diff --git a/diff.c b/diff.c > index 3be927b073..84a6bb0868 100644 > --- a/diff.c > +++ b/diff.c > @@ -3675,6 +3675,7 @@ static void builtin_diff(const char *name_a, > emit_diff_symbol(o, DIFF_SYMBOL_BINARY_FILES, > sb.buf, sb.len, 0); > strbuf_release(&sb); > + o->found_changes = 1; > goto free_ab_and_return; > } > if (fill_mmfile(o->repo, &mf1, one) < 0 || I poked at the same issue in parallel and had the same fix, but ... > diff --git a/t/t4017-diff-retval.sh b/t/t4017-diff-retval.sh > index d644310e22..1cea73ef5a 100755 > --- a/t/t4017-diff-retval.sh > +++ b/t/t4017-diff-retval.sh > @@ -145,6 +145,14 @@ test_expect_success 'option errors are not confused by --exit-code' ' > > for option in --exit-code --quiet > do > + test_expect_success "git diff $option returns 1 for changed binary file" " > + test_when_finished 'rm -f .gitattributes' && > + git reset --hard && > + echo a binary >.gitattributes && > + echo 2 >>a && > + test_expect_code 1 git diff $option > + " > + > test_expect_success "git diff $option returns 1 for copied file" " > git reset --hard && > cp a copy && your test is nicer. The patch works here locally. For what it's worth: Reviewed-by: Thomas Braun Thomas ``` ## Junio C Hamano, 2024-09-25 21:52 Subject: Re: [PATCH] diff: report modified binary files as changes in builtin_diff() Message-ID: URL: https://gitlist.dev/e/xmqq7cazo0z8.fsf%40gitster.g In-Reply-To: <3192d8f4-4c7f-4b32-b564-7e075132c41c@virtuell-zuhause.de> ``` Thomas Braun writes: > Am 21.09.2024 um 17:09 schrieb René Scharfe: > > Hi René, > >> diff --git a/diff.c b/diff.c >> index 3be927b073..84a6bb0868 100644 >> --- a/diff.c >> +++ b/diff.c >> @@ -3675,6 +3675,7 @@ static void builtin_diff(const char *name_a, >> emit_diff_symbol(o, DIFF_SYMBOL_BINARY_FILES, >> sb.buf, sb.len, 0); >> strbuf_release(&sb); >> + o->found_changes = 1; >> goto free_ab_and_return; >> } >> if (fill_mmfile(o->repo, &mf1, one) < 0 || > > I poked at the same issue in parallel and had the same fix, but ... > >> ... >> test_expect_success "git diff $option returns 1 for copied file" " >> git reset --hard && >> cp a copy && > > your test is nicer. > > The patch works here locally. > > For what it's worth: > > Reviewed-by: Thomas Braun > > Thomas Thanks. ```