From: Thomas Braun <thomas.braun@virtuell-zuhause.de>
Date: Wed, 25 Sep 2024 21:24:18 GMT
Subject: Re: [PATCH] diff: report modified binary files as changes in builtin_diff()
Message-ID: <3192d8f4-4c7f-4b32-b564-7e075132c41c@virtuell-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.braun@virtuell-zuhause.de>

Thomas