Re: [PATCH 1/2] diff: fix handling of binary rewrite diffs

[Junio C Hamano <gitster@pobox.com>]

Jeff King <peff@peff.net> writes:

> Instead, if we have binary files, then let's just skip emit_rewrite_diff
> altogether. We will already have shown the "dissimilarity index" line,
> so it is really about the diff contents. If binary diffs are turned off,
> the "Binary files a/file and b/file differ" message should be the same
> in either case. If we do have binary patches turned on, there isn't much
> point in making a less-efficient binary patch that does a total rewrite;
> no human is going to read it, and since binary patches don't apply with
> any fuzz anyway, the result of application should be the same.

Makes sense.

>  diff.c                         |    4 ++-
>  t/t4031-diff-rewrite-binary.sh |   42 ++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 45 insertions(+), 1 deletions(-)
>  create mode 100755 t/t4031-diff-rewrite-binary.sh
>
> diff --git a/diff.c b/diff.c
> index f644947..ea958a2 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -1376,7 +1376,9 @@ static void builtin_diff(const char *name_a,
>  		 */
>  		if ((one->mode ^ two->mode) & S_IFMT)
>  			goto free_ab_and_return;
> -		if (complete_rewrite) {
> +		if (complete_rewrite &&
> +		    !diff_filespec_is_binary(one) &&
> +		    !diff_filespec_is_binary(two)) {
>  			emit_rewrite_diff(name_a, name_b, one, two, o);
>  			o->found_changes = 1;
>  			goto free_ab_and_return;

And looks correct.

> diff --git a/t/t4031-diff-rewrite-binary.sh b/t/t4031-diff-rewrite-binary.sh
> new file mode 100755
> index 0000000..4b522f7
> --- /dev/null
> +++ b/t/t4031-diff-rewrite-binary.sh
> @@ -0,0 +1,42 @@
> +#!/bin/sh
> +
> +test_description='rewrite diff on binary file'
> +
> +. ./test-lib.sh
> +
> +# We must be large enough to meet the MINIMUM_BREAK_SIZE
> +# requirement.
> +make_file() {
> +	for i in 1 2 3 4 5 6 7 8 9 10; do
> +		for j in 1 2 3 4 5 6 7 9 10; do
> +			for k in 1 2 3 4 5; do
> +				printf "$1\n"
> +			done
> +		done
> +	done >file
> +}
> +
> +test_expect_success 'create binary file with changes' '
> +	make_file "\\0" &&
> +	git add file &&
> +	make_file "\\01"
> +'

Hmm... "1 2 3 4 5 6 7 9 10"?