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

["Hongren (Zenithal) Zheng" <i@zenithal.me>]

On Sun, Oct 03, 2021 at 07:25:29PM +0800, Hongren (Zenithal) Zheng wrote:
> 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 <i@zenithal.me>
> ---
>  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/YR1OszUm08BMAE1N@host1.jankratochvil.net/
> 
> 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
> 

Is there any updates regarding this patch?