Re: [PATCH V4] git-apply: skip threeway in add / rename cases

[Zenithal <i@zenithal.me>]

On Fri, Dec 17, 2021 at 03:29:02PM -0800, Jerry Zhang wrote:
> Certain invocations of "git apply --3way"
> will attempt threeway and fail due to
> missing objects, even though git is able
> to fall back on apply_fragments and
> apply the patch successfully with a return
> value of 0. To fix, return early from
> try_threeway() in the following cases:
> 
> When the patch is a rename and no lines have
> changed. In this case, "git diff" doesn't
> record the blob info, so 3way is neither
> possible nor necessary.
> 
> When the patch is an addition and there is
> no add/add conflict, i.e. direct_to_threeway
> is false. In this case, threeway will fail
> since the preimage is not in cache, but isn't
> necessary anyway since there is no conflict.
> 
> This fixes a few unecessary error prints
> when applying these kinds of patches with
> --3way.
> 
> It also fixes a reported issue where applying
> a concatenation of several git produced patches
> will fail when those patches involve a deletion
> followed by creation of the same file. Added a
> test for this case too.
> (test provided by <i@zenithal.me>)
> 
> Signed-off-by: Jerry Zhang <jerry@skydio.com>
> ---
> V3->V4:
> - Fix test bug where it wasn't actually
> exercising the correct failure mode.
> 
>  apply.c                   |  4 +++-
>  t/t4108-apply-threeway.sh | 18 ++++++++++++++++++
>  2 files changed, 21 insertions(+), 1 deletion(-)
> 
> diff --git a/apply.c b/apply.c
> index fed195250b..afc1c6510e 100644
> --- a/apply.c
> +++ b/apply.c
> @@ -3580,11 +3580,13 @@ 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 */
>  	if (patch->is_delete ||
> -	    S_ISGITLINK(patch->old_mode) || S_ISGITLINK(patch->new_mode))
> +	    S_ISGITLINK(patch->old_mode) || S_ISGITLINK(patch->new_mode) ||
> +	    (patch->is_new && !patch->direct_to_threeway) ||
> +	    (patch->is_rename && !patch->lines_added && !patch->lines_deleted))
>  		return -1;
>  
>  	/* Preimage the patch was prepared for */
>  	if (patch->is_new)
>  		write_object_file("", 0, blob_type, &pre_oid);
> diff --git a/t/t4108-apply-threeway.sh b/t/t4108-apply-threeway.sh
> index cc3aa3314a..c558282bc0 100755
> --- a/t/t4108-apply-threeway.sh
> +++ b/t/t4108-apply-threeway.sh
> @@ -273,6 +273,24 @@ test_expect_success 'apply full-index patch with 3way' '
>  
>  	# Apply must succeed.
>  	git apply --3way --index bin.diff
>  '
>  
> +test_expect_success 'apply delete then new patch with 3way' '
> +	git reset --hard main &&
> +	test_write_lines 2 > delnew &&
> +	git add delnew &&
> +	git diff --cached >> new.patch &&
> +	git reset --hard &&
> +	test_write_lines 1 > delnew &&
> +	git add delnew &&
> +	git commit -m "delnew" &&
> +	rm delnew &&
> +	git diff >> delete-then-new.patch &&
> +	cat new.patch >> delete-then-new.patch &&
> +
> +	git checkout -- . &&
> +	# Apply must succeed.
> +	git apply --3way delete-then-new.patch
> +'
> +
>  test_done
> -- 
> 2.32.0.1314.g6ed4fcc4cc
>

This fully resolved the issue I mentioned in
https://lore.kernel.org/git/YVmTKWlOFr+IwzzI@Sun/

Tested-by: Hongren (Zenithal) Zheng <i@zenithal.me>

Also, I would prefer a
Reported-by: Hongren (Zenithal) Zheng <i@zenithal.me>
tag or even
Co-authored-by: Hongren (Zenithal) Zheng <i@zenithal.me>
if you deem it appropriate.