git diff --exit-code returns 0 when binary files differ

git diff --exit-code returns 0 when binary files differ

[Kohei Shibata]

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

[PATCH] diff: report modified binary files as changes in builtin_diff()

[René Scharfe]

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 <shiba200712@gmail.com>
Signed-off-by: René Scharfe <l.s.r@web.de>
---
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

Re: [PATCH] diff: report modified binary files as changes in builtin_diff()

[Thomas Braun]

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.braun@virtuell-zuhause.de>

Thomas

Re: [PATCH] diff: report modified binary files as changes in builtin_diff()

[Junio C Hamano]

Thomas Braun <thomas.braun@virtuell-zuhause.de> 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.braun@virtuell-zuhause.de>
>
> Thomas

Thanks.