threads / patch / 57112

v3git-apply: skip threeway in add / rename cases

Subject: [PATCH V3] git-apply: skip threeway in add / rename cases

## tl;dr

4 messages between Dec 17, 2021 and Jan 5, 2022. Diffs are folded; open one to read it.

replies: 3people: 2as markdown or json

Jerry Zhang· Dec 17, 2021, 22:43 UTC · lore

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.

Signed-off-by: Jerry Zhang <jerry@skydio.com>
(test provided by <i@zenithal.me>)
---
V2->V3:
- Updated commit title and message to be more
general, and indicate that it also fixes the
delete-then-new bug. Added test.
 apply.c                   |  4 +++-
 t/t4108-apply-threeway.sh | 14 ++++++++++++++
 2 files changed, 17 insertions(+), 1 deletion(-)
Show changes to 2 files +17 −1

apply.c, t/t4108-apply-threeway.sh

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..daad50d2d2 100755
--- a/t/t4108-apply-threeway.sh
+++ b/t/t4108-apply-threeway.sh
@@ -273,6 +273,20 @@ 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 1 > delnew &&
+	git add delnew &&
+    git commit -m "delnew" &&
+    rm delnew &&
+    git diff >> delete-then-new.patch &&
+    git diff HEAD~ HEAD >> delete-then-new.patch &&
+
+    git checkout -- . &&
+	# Apply must succeed.
+	git apply --3way delete-then-new.patch
+'
+
 test_done

-- 
2.32.0.1314.g6ed4fcc4cc
Jerry Zhang· Dec 17, 2021, 23:29 UTC · re: Jerry Zhang · lore

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

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(-)
Show changes to 2 files +21 −1

apply.c, t/t4108-apply-threeway.sh

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
Zenithal· Dec 22, 2021, 06:41 UTC · re: Jerry Zhang · lore

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

On Fri, Dec 17, 2021 at 03:29:02PM -0800, Jerry Zhang wrote:
Show 91 quoted lines
> 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.
Jerry Zhang· Jan 5, 2022, 23:30 UTC · re: Jerry Zhang · lore

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

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.

Reported-by: Hongren (Zenithal) Zheng <i@zenithal.me>
Signed-off-by: Jerry Zhang <jerry@skydio.com>
---
V5: updated reported-by
 apply.c                   |  4 +++-
 t/t4108-apply-threeway.sh | 18 ++++++++++++++++++
 2 files changed, 21 insertions(+), 1 deletion(-)
Show changes to 2 files +21 −1

apply.c, t/t4108-apply-threeway.sh

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

← back to recent threads