Re: [PATCH] git-apply: fix --3way with binary patch

From: "lilinchao@oschina.cn" <lilinchao@oschina.cn>
To: "jerry@skydio.com" <jerry@skydio.com>, git <git@vger.kernel.org>,
	"Junio C Hamano" <gitster@pobox.com>
Cc: "jerry@skydio.com" <jerry@skydio.com>
Subject: Re: [PATCH] git-apply: fix --3way with binary patch
Date: Wed, 28 Jul 2021 12:45:53 +0800	[thread overview]
Message-ID: <b37a28faef5e11ebb9270024e87935e7@oschina.cn> (raw)
In-Reply-To: ee1d1a82ef4d11ebbb4cd4ae5278bc123046@skydio.com

I am happy to see that you fix this problem in time.

Some application platform(like Gitlab, Gitee) will use "git apply --index xxx.patch" to merge patch file,
in order to compatible with most types of files, they will use "--3way", so  although as Juno said
"We refuse to merge binary files, so I would not be surprised if we failed the 3way in this case",
we would like it still behave as before that falling back to normal patch application, so thanks again to fix this.

>Binary patches applied with "--3way" will
>always return a conflict even if the patch
>should cleanly apply because the low level
>merge function considers all binary merges
>without a variant to be conflicting.
>
>Fix by falling back to normal patch application
>for all binary patches.
>
>Add tests for --3way and normal applications
>of binary patches.
>
>Fixes: 923cd87ac8 ("git-apply: try threeway first when "--3way" is used")
>Signed-off-by: Jerry Zhang <jerry@skydio.com>
>---
> apply.c                   |  3 ++-
> t/t4108-apply-threeway.sh | 45 +++++++++++++++++++++++++++++++++++++++
> 2 files changed, 47 insertions(+), 1 deletion(-)
>
>diff --git a/apply.c b/apply.c
>index 1d2d7e124e..78e52f0dc1 100644
>--- a/apply.c
>+++ b/apply.c
>@@ -3638,7 +3638,8 @@ static int apply_data(struct apply_state *state, struct patch *patch,
> if (load_preimage(state, &image, patch, st, ce) < 0)
> return -1;
>
>-	if (!state->threeway || try_threeway(state, &image, patch, st, ce) < 0) {
>+	if (!state->threeway || patch->is_binary ||
>+	try_threeway(state, &image, patch, st, ce) < 0) {
> if (state->apply_verbosity > verbosity_silent &&
>     state->threeway && !patch->direct_to_threeway)
> fprintf(stderr, _("Falling back to direct application...\n"));
>diff --git a/t/t4108-apply-threeway.sh b/t/t4108-apply-threeway.sh
>index 65147efdea..d32748f899 100755
>--- a/t/t4108-apply-threeway.sh
>+++ b/t/t4108-apply-threeway.sh
>@@ -230,4 +230,49 @@ test_expect_success 'apply with --3way --cached and conflicts' '
> test_cmp expect.diff actual.diff
> '
>
>+test_expect_success 'apply binary file patch' '
>+	git reset --hard main &&
>+	cp $TEST_DIRECTORY/test-binary-1.png bin.png &&
>+	git add bin.png &&
>+	git commit -m "add binary file" &&
>+
>+	cp $TEST_DIRECTORY/test-binary-2.png bin.png &&
>+
>+	git diff --binary >bin.diff &&
>+	git reset --hard &&
>+
>+	# Apply must succeed.
>+	git apply bin.diff
>+'
>+
>+test_expect_success 'apply binary file patch with 3way' '
>+	git reset --hard main &&
>+	cp $TEST_DIRECTORY/test-binary-1.png bin.png &&
>+	git add bin.png &&
>+	git commit -m "add binary file" &&
>+
>+	cp $TEST_DIRECTORY/test-binary-2.png bin.png &&
>+
>+	git diff --binary >bin.diff &&
>+	git reset --hard &&
>+
>+	# Apply must succeed.
>+	git apply --3way --index bin.diff
>+'
>+
>+test_expect_success 'apply full-index patch with 3way' '
>+	git reset --hard main &&
>+	cp $TEST_DIRECTORY/test-binary-1.png bin.png &&
>+	git add bin.png &&
>+	git commit -m "add binary file" &&
>+
>+	cp $TEST_DIRECTORY/test-binary-2.png bin.png &&
>+
>+	git diff --full-index >bin.diff &&
>+	git reset --hard &&
>+
>+	# Apply must succeed.
>+	git apply --3way --index bin.diff
>+'
>+
> test_done
>--
>2.32.0.1314.g6ed4fcc4cc
>