{"thread":{"id":"57112","subject":"[PATCH V3] git-apply: skip threeway in add / rename cases","startedAt":"2021-12-17T22:43:35Z","lastAt":"2022-01-05T23:30:51Z","messageCount":4,"participants":["Jerry Zhang","Zenithal"],"isPatch":true,"patchVersion":3,"patchTotal":null},"messages":[{"id":"444399","messageId":"20211217224328.7646-1-jerry@skydio.com","threadId":"57112","inReplyTo":null,"subject":"[PATCH V3] git-apply: skip threeway in add / rename cases","fromName":"Jerry Zhang","fromEmail":"jerry@skydio.com","sentAt":"2021-12-17T22:43:28Z","receivedAt":"2021-12-17T22:43:35Z","isPatch":true,"sender":{"key":"jerry@skydio.com","avatar":"https://avatars.githubusercontent.com/u/81337184?v=4"},"body":"Certain invocations of \"git apply --3way\"\nwill attempt threeway and fail due to\nmissing objects, even though git is able\nto fall back on apply_fragments and\napply the patch successfully with a return\nvalue of 0. To fix, return early from\ntry_threeway() in the following cases:\n\nWhen the patch is a rename and no lines have\nchanged. In this case, \"git diff\" doesn't\nrecord the blob info, so 3way is neither\npossible nor necessary.\n\nWhen the patch is an addition and there is\nno add/add conflict, i.e. direct_to_threeway\nis false. In this case, threeway will fail\nsince the preimage is not in cache, but isn't\nnecessary anyway since there is no conflict.\n\nThis fixes a few unecessary error prints\nwhen applying these kinds of patches with\n--3way.\n\nIt also fixes a reported issue where applying\na concatenation of several git produced patches\nwill fail when those patches involve a deletion\nfollowed by creation of the same file. Added a\ntest for this case too.\n\nSigned-off-by: Jerry Zhang <jerry@skydio.com>\n(test provided by <i@zenithal.me>)\n---\nV2->V3:\n- Updated commit title and message to be more\ngeneral, and indicate that it also fixes the\ndelete-then-new bug. Added test.\n\n apply.c                   |  4 +++-\n t/t4108-apply-threeway.sh | 14 ++++++++++++++\n 2 files changed, 17 insertions(+), 1 deletion(-)\n\ndiff --git a/apply.c b/apply.c\nindex fed195250b..afc1c6510e 100644\n--- a/apply.c\n+++ b/apply.c\n@@ -3580,11 +3580,13 @@ static int try_threeway(struct apply_state *state,\n \tchar *img;\n \tstruct image tmp_image;\n \n \t/* No point falling back to 3-way merge in these cases */\n \tif (patch->is_delete ||\n-\t    S_ISGITLINK(patch->old_mode) || S_ISGITLINK(patch->new_mode))\n+\t    S_ISGITLINK(patch->old_mode) || S_ISGITLINK(patch->new_mode) ||\n+\t    (patch->is_new && !patch->direct_to_threeway) ||\n+\t    (patch->is_rename && !patch->lines_added && !patch->lines_deleted))\n \t\treturn -1;\n \n \t/* Preimage the patch was prepared for */\n \tif (patch->is_new)\n \t\twrite_object_file(\"\", 0, blob_type, &pre_oid);\ndiff --git a/t/t4108-apply-threeway.sh b/t/t4108-apply-threeway.sh\nindex cc3aa3314a..daad50d2d2 100755\n--- a/t/t4108-apply-threeway.sh\n+++ b/t/t4108-apply-threeway.sh\n@@ -273,6 +273,20 @@ test_expect_success 'apply full-index patch with 3way' '\n \n \t# Apply must succeed.\n \tgit apply --3way --index bin.diff\n '\n \n+test_expect_success 'apply delete then new patch with 3way' '\n+\tgit reset --hard main &&\n+    test_write_lines 1 > delnew &&\n+\tgit add delnew &&\n+    git commit -m \"delnew\" &&\n+    rm delnew &&\n+    git diff >> delete-then-new.patch &&\n+    git diff HEAD~ HEAD >> delete-then-new.patch &&\n+\n+    git checkout -- . &&\n+\t# Apply must succeed.\n+\tgit apply --3way delete-then-new.patch\n+'\n+\n test_done\n-- \n2.32.0.1314.g6ed4fcc4cc\n\n"},{"id":"444401","messageId":"20211217232902.7604-1-jerry@skydio.com","threadId":"57112","inReplyTo":"20211217224328.7646-1-jerry@skydio.com","subject":"[PATCH V4] git-apply: skip threeway in add / rename cases","fromName":"Jerry Zhang","fromEmail":"jerry@skydio.com","sentAt":"2021-12-17T23:29:02Z","receivedAt":"2021-12-17T23:29:08Z","isPatch":true,"sender":{"key":"jerry@skydio.com","avatar":"https://avatars.githubusercontent.com/u/81337184?v=4"},"body":"Certain invocations of \"git apply --3way\"\nwill attempt threeway and fail due to\nmissing objects, even though git is able\nto fall back on apply_fragments and\napply the patch successfully with a return\nvalue of 0. To fix, return early from\ntry_threeway() in the following cases:\n\nWhen the patch is a rename and no lines have\nchanged. In this case, \"git diff\" doesn't\nrecord the blob info, so 3way is neither\npossible nor necessary.\n\nWhen the patch is an addition and there is\nno add/add conflict, i.e. direct_to_threeway\nis false. In this case, threeway will fail\nsince the preimage is not in cache, but isn't\nnecessary anyway since there is no conflict.\n\nThis fixes a few unecessary error prints\nwhen applying these kinds of patches with\n--3way.\n\nIt also fixes a reported issue where applying\na concatenation of several git produced patches\nwill fail when those patches involve a deletion\nfollowed by creation of the same file. Added a\ntest for this case too.\n(test provided by <i@zenithal.me>)\n\nSigned-off-by: Jerry Zhang <jerry@skydio.com>\n---\nV3->V4:\n- Fix test bug where it wasn't actually\nexercising the correct failure mode.\n\n apply.c                   |  4 +++-\n t/t4108-apply-threeway.sh | 18 ++++++++++++++++++\n 2 files changed, 21 insertions(+), 1 deletion(-)\n\ndiff --git a/apply.c b/apply.c\nindex fed195250b..afc1c6510e 100644\n--- a/apply.c\n+++ b/apply.c\n@@ -3580,11 +3580,13 @@ static int try_threeway(struct apply_state *state,\n \tchar *img;\n \tstruct image tmp_image;\n \n \t/* No point falling back to 3-way merge in these cases */\n \tif (patch->is_delete ||\n-\t    S_ISGITLINK(patch->old_mode) || S_ISGITLINK(patch->new_mode))\n+\t    S_ISGITLINK(patch->old_mode) || S_ISGITLINK(patch->new_mode) ||\n+\t    (patch->is_new && !patch->direct_to_threeway) ||\n+\t    (patch->is_rename && !patch->lines_added && !patch->lines_deleted))\n \t\treturn -1;\n \n \t/* Preimage the patch was prepared for */\n \tif (patch->is_new)\n \t\twrite_object_file(\"\", 0, blob_type, &pre_oid);\ndiff --git a/t/t4108-apply-threeway.sh b/t/t4108-apply-threeway.sh\nindex cc3aa3314a..c558282bc0 100755\n--- a/t/t4108-apply-threeway.sh\n+++ b/t/t4108-apply-threeway.sh\n@@ -273,6 +273,24 @@ test_expect_success 'apply full-index patch with 3way' '\n \n \t# Apply must succeed.\n \tgit apply --3way --index bin.diff\n '\n \n+test_expect_success 'apply delete then new patch with 3way' '\n+\tgit reset --hard main &&\n+\ttest_write_lines 2 > delnew &&\n+\tgit add delnew &&\n+\tgit diff --cached >> new.patch &&\n+\tgit reset --hard &&\n+\ttest_write_lines 1 > delnew &&\n+\tgit add delnew &&\n+\tgit commit -m \"delnew\" &&\n+\trm delnew &&\n+\tgit diff >> delete-then-new.patch &&\n+\tcat new.patch >> delete-then-new.patch &&\n+\n+\tgit checkout -- . &&\n+\t# Apply must succeed.\n+\tgit apply --3way delete-then-new.patch\n+'\n+\n test_done\n-- \n2.32.0.1314.g6ed4fcc4cc\n\n"},{"id":"444750","messageId":"YcLIjXBdNmMvbqCj@Sun","threadId":"57112","inReplyTo":"20211217232902.7604-1-jerry@skydio.com","subject":"Re: [PATCH V4] git-apply: skip threeway in add / rename cases","fromName":"Zenithal","fromEmail":"i@zenithal.me","sentAt":"2021-12-22T06:41:17Z","receivedAt":"2021-12-22T06:41:24Z","isPatch":true,"sender":{"key":"i@zenithal.me","avatar":null},"body":"On Fri, Dec 17, 2021 at 03:29:02PM -0800, Jerry Zhang wrote:\n> Certain invocations of \"git apply --3way\"\n> will attempt threeway and fail due to\n> missing objects, even though git is able\n> to fall back on apply_fragments and\n> apply the patch successfully with a return\n> value of 0. To fix, return early from\n> try_threeway() in the following cases:\n> \n> When the patch is a rename and no lines have\n> changed. In this case, \"git diff\" doesn't\n> record the blob info, so 3way is neither\n> possible nor necessary.\n> \n> When the patch is an addition and there is\n> no add/add conflict, i.e. direct_to_threeway\n> is false. In this case, threeway will fail\n> since the preimage is not in cache, but isn't\n> necessary anyway since there is no conflict.\n> \n> This fixes a few unecessary error prints\n> when applying these kinds of patches with\n> --3way.\n> \n> It also fixes a reported issue where applying\n> a concatenation of several git produced patches\n> will fail when those patches involve a deletion\n> followed by creation of the same file. Added a\n> test for this case too.\n> (test provided by <i@zenithal.me>)\n> \n> Signed-off-by: Jerry Zhang <jerry@skydio.com>\n> ---\n> V3->V4:\n> - Fix test bug where it wasn't actually\n> exercising the correct failure mode.\n> \n>  apply.c                   |  4 +++-\n>  t/t4108-apply-threeway.sh | 18 ++++++++++++++++++\n>  2 files changed, 21 insertions(+), 1 deletion(-)\n> \n> diff --git a/apply.c b/apply.c\n> index fed195250b..afc1c6510e 100644\n> --- a/apply.c\n> +++ b/apply.c\n> @@ -3580,11 +3580,13 @@ static int try_threeway(struct apply_state *state,\n>  \tchar *img;\n>  \tstruct image tmp_image;\n>  \n>  \t/* No point falling back to 3-way merge in these cases */\n>  \tif (patch->is_delete ||\n> -\t    S_ISGITLINK(patch->old_mode) || S_ISGITLINK(patch->new_mode))\n> +\t    S_ISGITLINK(patch->old_mode) || S_ISGITLINK(patch->new_mode) ||\n> +\t    (patch->is_new && !patch->direct_to_threeway) ||\n> +\t    (patch->is_rename && !patch->lines_added && !patch->lines_deleted))\n>  \t\treturn -1;\n>  \n>  \t/* Preimage the patch was prepared for */\n>  \tif (patch->is_new)\n>  \t\twrite_object_file(\"\", 0, blob_type, &pre_oid);\n> diff --git a/t/t4108-apply-threeway.sh b/t/t4108-apply-threeway.sh\n> index cc3aa3314a..c558282bc0 100755\n> --- a/t/t4108-apply-threeway.sh\n> +++ b/t/t4108-apply-threeway.sh\n> @@ -273,6 +273,24 @@ test_expect_success 'apply full-index patch with 3way' '\n>  \n>  \t# Apply must succeed.\n>  \tgit apply --3way --index bin.diff\n>  '\n>  \n> +test_expect_success 'apply delete then new patch with 3way' '\n> +\tgit reset --hard main &&\n> +\ttest_write_lines 2 > delnew &&\n> +\tgit add delnew &&\n> +\tgit diff --cached >> new.patch &&\n> +\tgit reset --hard &&\n> +\ttest_write_lines 1 > delnew &&\n> +\tgit add delnew &&\n> +\tgit commit -m \"delnew\" &&\n> +\trm delnew &&\n> +\tgit diff >> delete-then-new.patch &&\n> +\tcat new.patch >> delete-then-new.patch &&\n> +\n> +\tgit checkout -- . &&\n> +\t# Apply must succeed.\n> +\tgit apply --3way delete-then-new.patch\n> +'\n> +\n>  test_done\n> -- \n> 2.32.0.1314.g6ed4fcc4cc\n>\n\nThis fully resolved the issue I mentioned in\nhttps://lore.kernel.org/git/YVmTKWlOFr+IwzzI@Sun/\n\nTested-by: Hongren (Zenithal) Zheng <i@zenithal.me>\n\nAlso, I would prefer a\nReported-by: Hongren (Zenithal) Zheng <i@zenithal.me>\ntag or even\nCo-authored-by: Hongren (Zenithal) Zheng <i@zenithal.me>\nif you deem it appropriate.\n"},{"id":"445597","messageId":"20220105233035.27561-1-jerry@skydio.com","threadId":"57112","inReplyTo":"20211217232902.7604-1-jerry@skydio.com","subject":"[PATCH V5] git-apply: skip threeway in add / rename cases","fromName":"Jerry Zhang","fromEmail":"jerry@skydio.com","sentAt":"2022-01-05T23:30:35Z","receivedAt":"2022-01-05T23:30:51Z","isPatch":true,"sender":{"key":"jerry@skydio.com","avatar":"https://avatars.githubusercontent.com/u/81337184?v=4"},"body":"Certain invocations of \"git apply --3way\"\nwill attempt threeway and fail due to\nmissing objects, even though git is able\nto fall back on apply_fragments and\napply the patch successfully with a return\nvalue of 0. To fix, return early from\ntry_threeway() in the following cases:\n\nWhen the patch is a rename and no lines have\nchanged. In this case, \"git diff\" doesn't\nrecord the blob info, so 3way is neither\npossible nor necessary.\n\nWhen the patch is an addition and there is\nno add/add conflict, i.e. direct_to_threeway\nis false. In this case, threeway will fail\nsince the preimage is not in cache, but isn't\nnecessary anyway since there is no conflict.\n\nThis fixes a few unecessary error prints\nwhen applying these kinds of patches with\n--3way.\n\nIt also fixes a reported issue where applying\na concatenation of several git produced patches\nwill fail when those patches involve a deletion\nfollowed by creation of the same file. Added a\ntest for this case too.\n\nReported-by: Hongren (Zenithal) Zheng <i@zenithal.me>\nSigned-off-by: Jerry Zhang <jerry@skydio.com>\n---\nV5: updated reported-by\n\n apply.c                   |  4 +++-\n t/t4108-apply-threeway.sh | 18 ++++++++++++++++++\n 2 files changed, 21 insertions(+), 1 deletion(-)\n\ndiff --git a/apply.c b/apply.c\nindex fed195250b..afc1c6510e 100644\n--- a/apply.c\n+++ b/apply.c\n@@ -3580,11 +3580,13 @@ static int try_threeway(struct apply_state *state,\n \tchar *img;\n \tstruct image tmp_image;\n \n \t/* No point falling back to 3-way merge in these cases */\n \tif (patch->is_delete ||\n-\t    S_ISGITLINK(patch->old_mode) || S_ISGITLINK(patch->new_mode))\n+\t    S_ISGITLINK(patch->old_mode) || S_ISGITLINK(patch->new_mode) ||\n+\t    (patch->is_new && !patch->direct_to_threeway) ||\n+\t    (patch->is_rename && !patch->lines_added && !patch->lines_deleted))\n \t\treturn -1;\n \n \t/* Preimage the patch was prepared for */\n \tif (patch->is_new)\n \t\twrite_object_file(\"\", 0, blob_type, &pre_oid);\ndiff --git a/t/t4108-apply-threeway.sh b/t/t4108-apply-threeway.sh\nindex cc3aa3314a..c558282bc0 100755\n--- a/t/t4108-apply-threeway.sh\n+++ b/t/t4108-apply-threeway.sh\n@@ -273,6 +273,24 @@ test_expect_success 'apply full-index patch with 3way' '\n \n \t# Apply must succeed.\n \tgit apply --3way --index bin.diff\n '\n \n+test_expect_success 'apply delete then new patch with 3way' '\n+\tgit reset --hard main &&\n+\ttest_write_lines 2 > delnew &&\n+\tgit add delnew &&\n+\tgit diff --cached >> new.patch &&\n+\tgit reset --hard &&\n+\ttest_write_lines 1 > delnew &&\n+\tgit add delnew &&\n+\tgit commit -m \"delnew\" &&\n+\trm delnew &&\n+\tgit diff >> delete-then-new.patch &&\n+\tcat new.patch >> delete-then-new.patch &&\n+\n+\tgit checkout -- . &&\n+\t# Apply must succeed.\n+\tgit apply --3way delete-then-new.patch\n+'\n+\n test_done\n-- \n2.32.0.1314.g6ed4fcc4cc\n\n"}]}