Subject: [PATCH] apply: fix delete-then-new patch fail with 3way

From: "Hongren (Zenithal) Zheng" <[email-masked]>
Date: Sun, 3 Oct 2021 19:25:29 +0800

For one single patch FILE containing both deletion and creation
of the same file, applying with three way would fail, which should not.

When git-apply processes one single patch FILE, patches inside it
would be applied before write_out_results(), thus it may occur
that one file being deleted but it is still in the index when
applying a new patch, in this case, try_threeway() would find
an old file thus causing merge conflict.

To avoid this, git-apply should fall back to directly apply
when it turns out to be such cases.

Signed-off-by: Hongren (Zenithal) Zheng <[email-masked]>
---
 apply.c                   | 13 ++++++++++++-
 t/t4108-apply-threeway.sh | 20 ++++++++++++++++++++
 2 files changed, 32 insertions(+), 1 deletion(-)

More notes below:

This patch is a bugfix hence it is based on branch `maint`.

This bug is caused by a behavior change since 2.32 where
git apply --3way would try 3-way first before directly apply.

Interestingly, if the deletion patch and the addition patch are in
two patch files, applying with three way would go on cleanly.

As indicated in commit msg, if these two patches are in different
patch files, write_out_results() would be called twice, unlike when
they are in the same file, write_out_results() would be called altogether
after all patches being applied.

One way to fix this is to check for this kind of conditions, which
is presented in this patch.

A side note though, this kind of checks and fixes already exist
as indicated by variable ok_if_exists in function check_patch().
See the comment around this variable for more info.

This kind of fixes is really dark magic.

Another way, which I do not adopt because it requires major refactor
but it is more clean and understandable, is to change the way
write_out_resultes() is called, namely instead of calling it
after all patches being applied in one patch FILE, after each patch
being applied, we write_out_result immediately thus deleting one file
would immediately delete the file from the index.

The man page of `patch` says: If the patch file contains more than
one patch, patch tries to apply each of them as if they came
from separate patch files. So I think this way is more standardized.

However, as also indicated by comments around variable
ok_if_exists in function check_patch(), consequtive patches in one
file have special meanings as endowed by diff.c::run_diff()

I do not know how to handle this, so I just send it as notes.

More comment: this problem or this kind of fix may be related to 
https://lore.kernel.org/git/[email-masked]/

diff --git a/apply.c b/apply.c
index 44bc31d6eb5b42d4077eff458246cde376cb6785..3fa96fcc781bdc27f66a35442f27972a0e84ea77 100644
--- a/apply.c
+++ b/apply.c
@@ -3558,8 +3558,19 @@ static int try_threeway(struct apply_state *state,
 	char *img;
 	struct image tmp_image;
 
-	/* No point falling back to 3-way merge in these cases */
+	/*
+	 * No point using 3-way merge in these cases
+	 *
+	 * For patch->is_new, if new_name does not exist in the index,
+	 * we can directly apply; if new_name exists,
+	 * according to ok_if_exists in check_patch(),
+	 * there are cases where new_name gets deleted in previous patches
+	 * BUT still exists in index, in this case, we can directly apply.
+	 */
 	if (patch->is_delete ||
+	      (patch->is_new &&
+	       (index_name_pos(state->repo->index, patch->new_name, strlen(patch->new_name)) < 0 ||
+		was_deleted(in_fn_table(state, patch->new_name)))) ||
 	    S_ISGITLINK(patch->old_mode) || S_ISGITLINK(patch->new_mode))
 		return -1;
 
diff --git a/t/t4108-apply-threeway.sh b/t/t4108-apply-threeway.sh
index 65147efdea9a00e30d156e6f4d5d72a3987f230d..14bbb393430ed57a236d25aa568a0fdc6d221a6d 100755
--- a/t/t4108-apply-threeway.sh
+++ b/t/t4108-apply-threeway.sh
@@ -230,4 +230,24 @@ test_expect_success 'apply with --3way --cached and conflicts' '
 	test_cmp expect.diff actual.diff
 '
 
+test_expect_success 'apply delete then new patch with 3way' '
+	git reset --hard main &&
+	test_write_lines 1 > delnew &&
+	git add delnew &&
+	git commit -m "delnew" &&
+	cat >delete-then-new.patch <<-\EOF &&
+	--- a/delnew
+	+++ /dev/null
+	@@ -1 +0,0 @@
+	-1
+	--- /dev/null
+	+++ b/delnew
+	@@ -0,0 +1 @@
+	+2
+	EOF
+
+	# Apply must succeed.
+	git apply --3way delete-then-new.patch
+'
+
 test_done
-- 
2.32.0