threads / discuss / 62163

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

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

## tl;dr

4 messages between Sep 21, 2024 and Sep 25, 2024.

replies: 3people: 4as markdown or json

Kohei Shibata· Sep 21, 2024, 04:26 UTC · lore

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· Sep 21, 2024, 15:09 UTC · re: Kohei Shibata · lore

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

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
Thomas Braun· Sep 25, 2024, 21:24 UTC · re: René Scharfe · lore

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

Am 21.09.2024 um 17:09 schrieb René Scharfe:
Hi René,
Show 12 quoted lines
> 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 ...
Show 19 quoted lines
> 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
Junio C Hamano· Sep 25, 2024, 21:52 UTC · re: Thomas Braun · lore

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

Thomas Braun <thomas.braun@virtuell-zuhause.de> writes:
Show 33 quoted lines
> 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.

← back to recent threads