{"thread":{"id":"56638","subject":"[PATCH] apply: fix delete-then-new patch fail with 3way","startedAt":"2021-10-03T11:25:47Z","lastAt":"2021-12-11T01:53:14Z","messageCount":6,"participants":["Hongren (Zenithal) Zheng","Junio C Hamano","Jerry Zhang"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"437807","messageId":"YVmTKWlOFr+IwzzI@Sun","threadId":"56638","inReplyTo":null,"subject":"[PATCH] apply: fix delete-then-new patch fail with 3way","fromName":"Hongren (Zenithal) Zheng","fromEmail":"i@zenithal.me","sentAt":"2021-10-03T11:25:29Z","receivedAt":"2021-10-03T11:25:47Z","isPatch":true,"sender":{"key":"i@zenithal.me","avatar":null},"body":"For one single patch FILE containing both deletion and creation\nof the same file, applying with three way would fail, which should not.\n\nWhen git-apply processes one single patch FILE, patches inside it\nwould be applied before write_out_results(), thus it may occur\nthat one file being deleted but it is still in the index when\napplying a new patch, in this case, try_threeway() would find\nan old file thus causing merge conflict.\n\nTo avoid this, git-apply should fall back to directly apply\nwhen it turns out to be such cases.\n\nSigned-off-by: Hongren (Zenithal) Zheng <i@zenithal.me>\n---\n apply.c                   | 13 ++++++++++++-\n t/t4108-apply-threeway.sh | 20 ++++++++++++++++++++\n 2 files changed, 32 insertions(+), 1 deletion(-)\n\nMore notes below:\n\nThis patch is a bugfix hence it is based on branch `maint`.\n\nThis bug is caused by a behavior change since 2.32 where\ngit apply --3way would try 3-way first before directly apply.\n\nInterestingly, if the deletion patch and the addition patch are in\ntwo patch files, applying with three way would go on cleanly.\n\nAs indicated in commit msg, if these two patches are in different\npatch files, write_out_results() would be called twice, unlike when\nthey are in the same file, write_out_results() would be called altogether\nafter all patches being applied.\n\nOne way to fix this is to check for this kind of conditions, which\nis presented in this patch.\n\nA side note though, this kind of checks and fixes already exist\nas indicated by variable ok_if_exists in function check_patch().\nSee the comment around this variable for more info.\n\nThis kind of fixes is really dark magic.\n\nAnother way, which I do not adopt because it requires major refactor\nbut it is more clean and understandable, is to change the way\nwrite_out_resultes() is called, namely instead of calling it\nafter all patches being applied in one patch FILE, after each patch\nbeing applied, we write_out_result immediately thus deleting one file\nwould immediately delete the file from the index.\n\nThe man page of `patch` says: If the patch file contains more than\none patch, patch tries to apply each of them as if they came\nfrom separate patch files. So I think this way is more standardized.\n\nHowever, as also indicated by comments around variable\nok_if_exists in function check_patch(), consequtive patches in one\nfile have special meanings as endowed by diff.c::run_diff()\n\nI do not know how to handle this, so I just send it as notes.\n\nMore comment: this problem or this kind of fix may be related to \nhttps://lore.kernel.org/git/YR1OszUm08BMAE1N@host1.jankratochvil.net/\n\ndiff --git a/apply.c b/apply.c\nindex 44bc31d6eb5b42d4077eff458246cde376cb6785..3fa96fcc781bdc27f66a35442f27972a0e84ea77 100644\n--- a/apply.c\n+++ b/apply.c\n@@ -3558,8 +3558,19 @@ 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+\t/*\n+\t * No point using 3-way merge in these cases\n+\t *\n+\t * For patch->is_new, if new_name does not exist in the index,\n+\t * we can directly apply; if new_name exists,\n+\t * according to ok_if_exists in check_patch(),\n+\t * there are cases where new_name gets deleted in previous patches\n+\t * BUT still exists in index, in this case, we can directly apply.\n+\t */\n \tif (patch->is_delete ||\n+\t      (patch->is_new &&\n+\t       (index_name_pos(state->repo->index, patch->new_name, strlen(patch->new_name)) < 0 ||\n+\t\twas_deleted(in_fn_table(state, patch->new_name)))) ||\n \t    S_ISGITLINK(patch->old_mode) || S_ISGITLINK(patch->new_mode))\n \t\treturn -1;\n \ndiff --git a/t/t4108-apply-threeway.sh b/t/t4108-apply-threeway.sh\nindex 65147efdea9a00e30d156e6f4d5d72a3987f230d..14bbb393430ed57a236d25aa568a0fdc6d221a6d 100755\n--- a/t/t4108-apply-threeway.sh\n+++ b/t/t4108-apply-threeway.sh\n@@ -230,4 +230,24 @@ test_expect_success 'apply with --3way --cached and conflicts' '\n \ttest_cmp expect.diff actual.diff\n '\n \n+test_expect_success 'apply delete then new patch with 3way' '\n+\tgit reset --hard main &&\n+\ttest_write_lines 1 > delnew &&\n+\tgit add delnew &&\n+\tgit commit -m \"delnew\" &&\n+\tcat >delete-then-new.patch <<-\\EOF &&\n+\t--- a/delnew\n+\t+++ /dev/null\n+\t@@ -1 +0,0 @@\n+\t-1\n+\t--- /dev/null\n+\t+++ b/delnew\n+\t@@ -0,0 +1 @@\n+\t+2\n+\tEOF\n+\n+\t# Apply must succeed.\n+\tgit apply --3way delete-then-new.patch\n+'\n+\n test_done\n-- \n2.32.0\n\n"},{"id":"438951","messageId":"YWsbcbASLG3QNPyZ@Sun","threadId":"56638","inReplyTo":"YVmTKWlOFr+IwzzI@Sun","subject":"Re: [PATCH] apply: fix delete-then-new patch fail with 3way","fromName":"Hongren (Zenithal) Zheng","fromEmail":"i@zenithal.me","sentAt":"2021-10-16T18:35:29Z","receivedAt":"2021-10-16T18:35:38Z","isPatch":true,"sender":{"key":"i@zenithal.me","avatar":null},"body":"On Sun, Oct 03, 2021 at 07:25:29PM +0800, Hongren (Zenithal) Zheng wrote:\n> For one single patch FILE containing both deletion and creation\n> of the same file, applying with three way would fail, which should not.\n> \n> When git-apply processes one single patch FILE, patches inside it\n> would be applied before write_out_results(), thus it may occur\n> that one file being deleted but it is still in the index when\n> applying a new patch, in this case, try_threeway() would find\n> an old file thus causing merge conflict.\n> \n> To avoid this, git-apply should fall back to directly apply\n> when it turns out to be such cases.\n> \n> Signed-off-by: Hongren (Zenithal) Zheng <i@zenithal.me>\n> ---\n>  apply.c                   | 13 ++++++++++++-\n>  t/t4108-apply-threeway.sh | 20 ++++++++++++++++++++\n>  2 files changed, 32 insertions(+), 1 deletion(-)\n> \n> More notes below:\n> \n> This patch is a bugfix hence it is based on branch `maint`.\n> \n> This bug is caused by a behavior change since 2.32 where\n> git apply --3way would try 3-way first before directly apply.\n> \n> Interestingly, if the deletion patch and the addition patch are in\n> two patch files, applying with three way would go on cleanly.\n> \n> As indicated in commit msg, if these two patches are in different\n> patch files, write_out_results() would be called twice, unlike when\n> they are in the same file, write_out_results() would be called altogether\n> after all patches being applied.\n> \n> One way to fix this is to check for this kind of conditions, which\n> is presented in this patch.\n> \n> A side note though, this kind of checks and fixes already exist\n> as indicated by variable ok_if_exists in function check_patch().\n> See the comment around this variable for more info.\n> \n> This kind of fixes is really dark magic.\n> \n> Another way, which I do not adopt because it requires major refactor\n> but it is more clean and understandable, is to change the way\n> write_out_resultes() is called, namely instead of calling it\n> after all patches being applied in one patch FILE, after each patch\n> being applied, we write_out_result immediately thus deleting one file\n> would immediately delete the file from the index.\n> \n> The man page of `patch` says: If the patch file contains more than\n> one patch, patch tries to apply each of them as if they came\n> from separate patch files. So I think this way is more standardized.\n> \n> However, as also indicated by comments around variable\n> ok_if_exists in function check_patch(), consequtive patches in one\n> file have special meanings as endowed by diff.c::run_diff()\n> \n> I do not know how to handle this, so I just send it as notes.\n> \n> More comment: this problem or this kind of fix may be related to \n> https://lore.kernel.org/git/YR1OszUm08BMAE1N@host1.jankratochvil.net/\n> \n> diff --git a/apply.c b/apply.c\n> index 44bc31d6eb5b42d4077eff458246cde376cb6785..3fa96fcc781bdc27f66a35442f27972a0e84ea77 100644\n> --- a/apply.c\n> +++ b/apply.c\n> @@ -3558,8 +3558,19 @@ 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> +\t/*\n> +\t * No point using 3-way merge in these cases\n> +\t *\n> +\t * For patch->is_new, if new_name does not exist in the index,\n> +\t * we can directly apply; if new_name exists,\n> +\t * according to ok_if_exists in check_patch(),\n> +\t * there are cases where new_name gets deleted in previous patches\n> +\t * BUT still exists in index, in this case, we can directly apply.\n> +\t */\n>  \tif (patch->is_delete ||\n> +\t      (patch->is_new &&\n> +\t       (index_name_pos(state->repo->index, patch->new_name, strlen(patch->new_name)) < 0 ||\n> +\t\twas_deleted(in_fn_table(state, patch->new_name)))) ||\n>  \t    S_ISGITLINK(patch->old_mode) || S_ISGITLINK(patch->new_mode))\n>  \t\treturn -1;\n>  \n> diff --git a/t/t4108-apply-threeway.sh b/t/t4108-apply-threeway.sh\n> index 65147efdea9a00e30d156e6f4d5d72a3987f230d..14bbb393430ed57a236d25aa568a0fdc6d221a6d 100755\n> --- a/t/t4108-apply-threeway.sh\n> +++ b/t/t4108-apply-threeway.sh\n> @@ -230,4 +230,24 @@ test_expect_success 'apply with --3way --cached and conflicts' '\n>  \ttest_cmp expect.diff actual.diff\n>  '\n>  \n> +test_expect_success 'apply delete then new patch with 3way' '\n> +\tgit reset --hard main &&\n> +\ttest_write_lines 1 > delnew &&\n> +\tgit add delnew &&\n> +\tgit commit -m \"delnew\" &&\n> +\tcat >delete-then-new.patch <<-\\EOF &&\n> +\t--- a/delnew\n> +\t+++ /dev/null\n> +\t@@ -1 +0,0 @@\n> +\t-1\n> +\t--- /dev/null\n> +\t+++ b/delnew\n> +\t@@ -0,0 +1 @@\n> +\t+2\n> +\tEOF\n> +\n> +\t# Apply must succeed.\n> +\tgit apply --3way delete-then-new.patch\n> +'\n> +\n>  test_done\n> -- \n> 2.32.0\n> \n\nIs there any updates regarding this patch?\n"},{"id":"438966","messageId":"xmqqv91wyyij.fsf@gitster.g","threadId":"56638","inReplyTo":"YWsbcbASLG3QNPyZ@Sun","subject":"Re: [PATCH] apply: fix delete-then-new patch fail with 3way","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-10-17T06:08:04Z","receivedAt":"2021-10-17T06:08:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Hongren (Zenithal) Zheng\" <i@zenithal.me> writes:\n\n> On Sun, Oct 03, 2021 at 07:25:29PM +0800, Hongren (Zenithal) Zheng wrote:\n>> For one single patch FILE containing both deletion and creation\n>> of the same file, applying with three way would fail, which should not.\n>>  ...\n\nSigh.\n\nJerry, it seems that the earlier \"let's be more aggressive to use\n--3way, instead of using it as a fallback\" is turning out to be more\nand more trouble than we hoped.\n\nOne thing to notice about the patch used for this test is that ...\n\n>> +test_expect_success 'apply delete then new patch with 3way' '\n>> +\tgit reset --hard main &&\n>> +\ttest_write_lines 1 > delnew &&\n>> +\tgit add delnew &&\n>> +\tgit commit -m \"delnew\" &&\n>> +\tcat >delete-then-new.patch <<-\\EOF &&\n>> +\t--- a/delnew\n>> +\t+++ /dev/null\n>> +\t@@ -1 +0,0 @@\n>> +\t-1\n>> +\t--- /dev/null\n>> +\t+++ b/delnew\n>> +\t@@ -0,0 +1 @@\n>> +\t+2\n>> +\tEOF\n\n... this is clearly not a patch that was generated by Git.  We do\nnot show two separate patches, to delete and then to create, the\nsame path to express a file modification, and that is true even when\nwe are showing a total-rewrite patch.\n\nIn addition, the above set of two patches lack the \"index\" header\nthat records the old and new blob object name, because it is not a\npatch generated by Git.  Whether 3-way is attempted before or after\nthe normal application, because the object names there are a crucial\ningredient for the 3-way merge logic, there is no way for it to work\nat all.\n\n\n>> +\t# Apply must succeed.\n>> +\tgit apply --3way delete-then-new.patch\n\nSo, one simple and safe answer would be \"Don't do it, --3way is only\nabout Git patches.\"  IOW, the command is failing as designed.\n\nTo extend and automate the solution would be to see, just before\nattempting to do the 3-way, if the incoming patch is a Git generated\none, and do not even bother using the 3-way logic if it is not.\n\n"},{"id":"439039","messageId":"CAMKO5Csj_ck+1+0JsvmF7eSkr514GBUtznKqPvkfEwHGJ34cwQ@mail.gmail.com","threadId":"56638","inReplyTo":"xmqqv91wyyij.fsf@gitster.g","subject":"Re: [PATCH] apply: fix delete-then-new patch fail with 3way","fromName":"Jerry Zhang","fromEmail":"jerry@skydio.com","sentAt":"2021-10-19T18:56:11Z","receivedAt":"2021-10-19T18:56:25Z","isPatch":true,"sender":{"key":"jerry@skydio.com","avatar":"https://avatars.githubusercontent.com/u/81337184?v=4"},"body":"On Sat, Oct 16, 2021 at 11:08 PM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> \"Hongren (Zenithal) Zheng\" <i@zenithal.me> writes:\n>\n> > On Sun, Oct 03, 2021 at 07:25:29PM +0800, Hongren (Zenithal) Zheng wrote:\n> >> For one single patch FILE containing both deletion and creation\n> >> of the same file, applying with three way would fail, which should not.\n> >>  ...\n>\n> Sigh.\n>\n> Jerry, it seems that the earlier \"let's be more aggressive to use\n> --3way, instead of using it as a fallback\" is turning out to be more\n> and more trouble than we hoped.\n>\n> One thing to notice about the patch used for this test is that ...\n>\n> >> +test_expect_success 'apply delete then new patch with 3way' '\n> >> +    git reset --hard main &&\n> >> +    test_write_lines 1 > delnew &&\n> >> +    git add delnew &&\n> >> +    git commit -m \"delnew\" &&\n> >> +    cat >delete-then-new.patch <<-\\EOF &&\n> >> +    --- a/delnew\n> >> +    +++ /dev/null\n> >> +    @@ -1 +0,0 @@\n> >> +    -1\n> >> +    --- /dev/null\n> >> +    +++ b/delnew\n> >> +    @@ -0,0 +1 @@\n> >> +    +2\n> >> +    EOF\n>\n> ... this is clearly not a patch that was generated by Git.  We do\n> not show two separate patches, to delete and then to create, the\n> same path to express a file modification, and that is true even when\n> we are showing a total-rewrite patch.\n>\n> In addition, the above set of two patches lack the \"index\" header\n> that records the old and new blob object name, because it is not a\n> patch generated by Git.  Whether 3-way is attempted before or after\n> the normal application, because the object names there are a crucial\n> ingredient for the 3-way merge logic, there is no way for it to work\n> at all.\n>\n>\n> >> +    # Apply must succeed.\n> >> +    git apply --3way delete-then-new.patch\n>\n> So, one simple and safe answer would be \"Don't do it, --3way is only\n> about Git patches.\"  IOW, the command is failing as designed.\nYeah I do wonder why one would specify \"--3way\" when the behavior that\nthey want is actually \"direct application\". Maybe the OP can elaborate on their\nuse case?\n\nPart of my original assumption was that \"--3way\" users actually *want* 3way,\nand thus the behavior change wouldn't be too controversial. Of course since\ngit has so many users, it shouldn't be that surprising that there are many use\npatterns out there.\n\nOne possible fix-all solution would just be to back out the original change and\nmove the behavior into a new flag \"--actually-3way\" (name tbd) that will apply\nthis behavior and \"--3way\" would keep the old behavior. The downside here\nwould be proliferating more flags that would complicate the api and require\nmaintenance. And of course if users depended on the *new* behavior in the\nmeantime, then we'd be stuck.\n\nBack to the patch at hand, it does seem like it would work, however I\nnotice that\nif a modification patch were added to the end of the file such that it were\ndeleted -> add -> modify, that modify wouldn't benefit from actually\ndoing a 3way\nsince the file would not be in the index due to this short-cut. The\nmore general approach\nof refactoring to write out results after each patch instead of at the\nend *would* fix\nboth things. I guess this goes back to the larger issue of the\nthreeway implementation\nnot being well suited to non-git patches.\n\n\n>\n> To extend and automate the solution would be to see, just before\n> attempting to do the 3-way, if the incoming patch is a Git generated\n> one, and do not even bother using the 3-way logic if it is not.\n>\n"},{"id":"440447","messageId":"YYPA9tKnsfTBOWDM@Sun","threadId":"56638","inReplyTo":"CAMKO5Csj_ck+1+0JsvmF7eSkr514GBUtznKqPvkfEwHGJ34cwQ@mail.gmail.com","subject":"Re: [PATCH] apply: fix delete-then-new patch fail with 3way","fromName":"Hongren (Zenithal) Zheng","fromEmail":"i@zenithal.me","sentAt":"2021-11-04T11:16:06Z","receivedAt":"2021-11-04T11:16:35Z","isPatch":true,"sender":{"key":"i@zenithal.me","avatar":null},"body":"On Tue, Oct 19, 2021 at 11:56:11AM -0700, Jerry Zhang wrote:\n> On Sat, Oct 16, 2021 at 11:08 PM Junio C Hamano <gitster@pobox.com> wrote:\n> >\n> > \"Hongren (Zenithal) Zheng\" <i@zenithal.me> writes:\n> >\n> > > On Sun, Oct 03, 2021 at 07:25:29PM +0800, Hongren (Zenithal) Zheng wrote:\n> > >> For one single patch FILE containing both deletion and creation\n> > >> of the same file, applying with three way would fail, which should not.\n> > >>  ...\n> >\n> > Sigh.\n> >\n> > Jerry, it seems that the earlier \"let's be more aggressive to use\n> > --3way, instead of using it as a fallback\" is turning out to be more\n> > and more trouble than we hoped.\n> >\n> > One thing to notice about the patch used for this test is that ...\n> >\n> > >> +test_expect_success 'apply delete then new patch with 3way' '\n> > >> +    git reset --hard main &&\n> > >> +    test_write_lines 1 > delnew &&\n> > >> +    git add delnew &&\n> > >> +    git commit -m \"delnew\" &&\n> > >> +    cat >delete-then-new.patch <<-\\EOF &&\n> > >> +    --- a/delnew\n> > >> +    +++ /dev/null\n> > >> +    @@ -1 +0,0 @@\n> > >> +    -1\n> > >> +    --- /dev/null\n> > >> +    +++ b/delnew\n> > >> +    @@ -0,0 +1 @@\n> > >> +    +2\n> > >> +    EOF\n> >\n> > ... this is clearly not a patch that was generated by Git.  We do\n> > not show two separate patches, to delete and then to create, the\n> > same path to express a file modification, and that is true even when\n> > we are showing a total-rewrite patch.\n\nI'm aware of such process. This patch is generated by manually concatenating\ntwo patches together.\n\nWhy should I concat patches, you may ask. Well, there are cases where\nI have to distribute a patch series containing dozens of patches (like\npackaging for a Linux distribution), instead of multiple files, one\nfile would be convenient. Also, since the patch series may be out-of-date as\nthe upstream repo progresses, git apply -3 would be better.\n\nDespite the above example. Placing patches inside one file or not should\nnot affect the result of git apply -3\n\nRefer to the man page of patch\n\nFrom my first post:\n> > >> The man page of `patch` says: If the patch file contains more than\n> > >> one patch, patch tries to apply each of them as if they came\n> > >> from separate patch files. So I think this way is more standardized.\n\n> > In addition, the above set of two patches lack the \"index\" header\n> > that records the old and new blob object name, because it is not a\n> > patch generated by Git.  Whether 3-way is attempted before or after\n> > the normal application, because the object names there are a crucial\n> > ingredient for the 3-way merge logic, there is no way for it to work\n> > at all.\n\nIt is my fault that for simplicity, I did not use a way to generate\npatches with an \"index\" header in it. Below is the procedure to\nreproduce this \"bug\" (I still call it \"bug\") even with index in it.\n\nmkdir delete-then-new\npushd delete-then-new\ngit init\necho 1 > a\ngit add a\ngit commit -m \"init\"\ngit rm a\ngit commit -m \"delete\"\necho 2 > a\ngit add a\ngit commit -m \"new\"\ngit format-patch --full-index -o ../ HEAD~2\ngit checkout HEAD~2\ncat ../0001-delete.patch ../0002-new.patch > ../delete-then-new.patch\ngit apply -3 ../delete-then-new.patch # it would fail\n\n> >\n> >\n> > >> +    # Apply must succeed.\n> > >> +    git apply --3way delete-then-new.patch\n> >\n> > So, one simple and safe answer would be \"Don't do it, --3way is only\n> > about Git patches.\"  IOW, the command is failing as designed.\n> Yeah I do wonder why one would specify \"--3way\" when the behavior that\n> they want is actually \"direct application\". Maybe the OP can elaborate on their\n> use case?\n\nI have mentioned the specific 3way usage in the above case.\n\n> Part of my original assumption was that \"--3way\" users actually *want* 3way,\n> and thus the behavior change wouldn't be too controversial. Of course since\n> git has so many users, it shouldn't be that surprising that there are many use\n> patterns out there.\n\nI do want 3way, but there are cases where 3way would not work,\nlike when 3way patches and direct patches (e.g. delete/new, mode change)\nare mixed together in one patch file.\n\nI think we should enumerate all cases where threeway should be avoided\nand we should fallback to directly applying. From my inspection of the\ncode, it seems that it is not sufficient now.\n\n> One possible fix-all solution would just be to back out the original change and\n> move the behavior into a new flag \"--actually-3way\" (name tbd) that will apply\n> this behavior and \"--3way\" would keep the old behavior. The downside here\n> would be proliferating more flags that would complicate the api and require\n> maintenance. And of course if users depended on the *new* behavior in the\n> meantime, then we'd be stuck.\n> \n> Back to the patch at hand, it does seem like it would work, however I\n> notice that\n> if a modification patch were added to the end of the file such that it were\n> deleted -> add -> modify, that modify wouldn't benefit from actually\n> doing a 3way\n> since the file would not be in the index due to this short-cut. The\n> more general approach\n> of refactoring to write out results after each patch instead of at the\n> end *would* fix\n> both things. I guess this goes back to the larger issue of the\n> threeway implementation\n> not being well suited to non-git patches.\n\nYes, the problem comes from the short-cut, and I have mentioned that\nwe should write out results immediately instead of at the end.\n\nFrom my first post:\n> > >> Another way, which I do not adopt because it requires major refactor\n> > >> but it is more clean and understandable, is to change the way\n> > >> write_out_resultes() is called, namely instead of calling it\n> > >> after all patches being applied in one patch FILE, after each patch\n> > >> being applied, we write_out_result immediately thus deleting one file\n> > >> would immediately delete the file from the index.\n\n> >\n> > To extend and automate the solution would be to see, just before\n> > attempting to do the 3-way, if the incoming patch is a Git generated\n> > one, and do not even bother using the 3-way logic if it is not.\n> >\n\nConcating two git-generated patches together would fool this mechanism, I\nsuppose. So the above \"bug\" still exists.\n"},{"id":"443853","messageId":"CAMKO5CtYu6mzthG3LsKS3zGQcQVvjPuKs+-2vxfJm2C723jTgw@mail.gmail.com","threadId":"56638","inReplyTo":"YYPA9tKnsfTBOWDM@Sun","subject":"Re: [PATCH] apply: fix delete-then-new patch fail with 3way","fromName":"Jerry Zhang","fromEmail":"jerry@skydio.com","sentAt":"2021-12-11T01:53:00Z","receivedAt":"2021-12-11T01:53:14Z","isPatch":true,"sender":{"key":"jerry@skydio.com","avatar":"https://avatars.githubusercontent.com/u/81337184?v=4"},"body":"On Thu, Nov 4, 2021 at 4:16 AM Hongren (Zenithal) Zheng <i@zenithal.me> wrote:\n>\n> On Tue, Oct 19, 2021 at 11:56:11AM -0700, Jerry Zhang wrote:\n> > On Sat, Oct 16, 2021 at 11:08 PM Junio C Hamano <gitster@pobox.com> wrote:\n> > >\n> > > \"Hongren (Zenithal) Zheng\" <i@zenithal.me> writes:\n> > >\n> > > > On Sun, Oct 03, 2021 at 07:25:29PM +0800, Hongren (Zenithal) Zheng wrote:\n> > > >> For one single patch FILE containing both deletion and creation\n> > > >> of the same file, applying with three way would fail, which should not.\n> > > >>  ...\n> > >\n> > > Sigh.\n> > >\n> > > Jerry, it seems that the earlier \"let's be more aggressive to use\n> > > --3way, instead of using it as a fallback\" is turning out to be more\n> > > and more trouble than we hoped.\n> > >\n> > > One thing to notice about the patch used for this test is that ...\n> > >\n> > > >> +test_expect_success 'apply delete then new patch with 3way' '\n> > > >> +    git reset --hard main &&\n> > > >> +    test_write_lines 1 > delnew &&\n> > > >> +    git add delnew &&\n> > > >> +    git commit -m \"delnew\" &&\n> > > >> +    cat >delete-then-new.patch <<-\\EOF &&\n> > > >> +    --- a/delnew\n> > > >> +    +++ /dev/null\n> > > >> +    @@ -1 +0,0 @@\n> > > >> +    -1\n> > > >> +    --- /dev/null\n> > > >> +    +++ b/delnew\n> > > >> +    @@ -0,0 +1 @@\n> > > >> +    +2\n> > > >> +    EOF\n> > >\n> > > ... this is clearly not a patch that was generated by Git.  We do\n> > > not show two separate patches, to delete and then to create, the\n> > > same path to express a file modification, and that is true even when\n> > > we are showing a total-rewrite patch.\n>\n> I'm aware of such process. This patch is generated by manually concatenating\n> two patches together.\n>\n> Why should I concat patches, you may ask. Well, there are cases where\n> I have to distribute a patch series containing dozens of patches (like\n> packaging for a Linux distribution), instead of multiple files, one\n> file would be convenient. Also, since the patch series may be out-of-date as\n> the upstream repo progresses, git apply -3 would be better.\n>\n> Despite the above example. Placing patches inside one file or not should\n> not affect the result of git apply -3\n>\n> Refer to the man page of patch\n>\n> From my first post:\n> > > >> The man page of `patch` says: If the patch file contains more than\n> > > >> one patch, patch tries to apply each of them as if they came\n> > > >> from separate patch files. So I think this way is more standardized.\n>\n> > > In addition, the above set of two patches lack the \"index\" header\n> > > that records the old and new blob object name, because it is not a\n> > > patch generated by Git.  Whether 3-way is attempted before or after\n> > > the normal application, because the object names there are a crucial\n> > > ingredient for the 3-way merge logic, there is no way for it to work\n> > > at all.\n>\n> It is my fault that for simplicity, I did not use a way to generate\n> patches with an \"index\" header in it. Below is the procedure to\n> reproduce this \"bug\" (I still call it \"bug\") even with index in it.\n>\n> mkdir delete-then-new\n> pushd delete-then-new\n> git init\n> echo 1 > a\n> git add a\n> git commit -m \"init\"\n> git rm a\n> git commit -m \"delete\"\n> echo 2 > a\n> git add a\n> git commit -m \"new\"\n> git format-patch --full-index -o ../ HEAD~2\n> git checkout HEAD~2\n> cat ../0001-delete.patch ../0002-new.patch > ../delete-then-new.patch\n> git apply -3 ../delete-then-new.patch # it would fail\n>\n> > >\n> > >\n> > > >> +    # Apply must succeed.\n> > > >> +    git apply --3way delete-then-new.patch\n> > >\n> > > So, one simple and safe answer would be \"Don't do it, --3way is only\n> > > about Git patches.\"  IOW, the command is failing as designed.\n> > Yeah I do wonder why one would specify \"--3way\" when the behavior that\n> > they want is actually \"direct application\". Maybe the OP can elaborate on their\n> > use case?\n>\n> I have mentioned the specific 3way usage in the above case.\n>\n> > Part of my original assumption was that \"--3way\" users actually *want* 3way,\n> > and thus the behavior change wouldn't be too controversial. Of course since\n> > git has so many users, it shouldn't be that surprising that there are many use\n> > patterns out there.\n>\n> I do want 3way, but there are cases where 3way would not work,\n> like when 3way patches and direct patches (e.g. delete/new, mode change)\n> are mixed together in one patch file.\n>\n> I think we should enumerate all cases where threeway should be avoided\n> and we should fallback to directly applying. From my inspection of the\n> code, it seems that it is not sufficient now.\n>\n> > One possible fix-all solution would just be to back out the original change and\n> > move the behavior into a new flag \"--actually-3way\" (name tbd) that will apply\n> > this behavior and \"--3way\" would keep the old behavior. The downside here\n> > would be proliferating more flags that would complicate the api and require\n> > maintenance. And of course if users depended on the *new* behavior in the\n> > meantime, then we'd be stuck.\n> >\n> > Back to the patch at hand, it does seem like it would work, however I\n> > notice that\n> > if a modification patch were added to the end of the file such that it were\n> > deleted -> add -> modify, that modify wouldn't benefit from actually\n> > doing a 3way\n> > since the file would not be in the index due to this short-cut. The\n> > more general approach\n> > of refactoring to write out results after each patch instead of at the\n> > end *would* fix\n> > both things. I guess this goes back to the larger issue of the\n> > threeway implementation\n> > not being well suited to non-git patches.\n>\n> Yes, the problem comes from the short-cut, and I have mentioned that\n> we should write out results immediately instead of at the end.\n>\n> From my first post:\n> > > >> Another way, which I do not adopt because it requires major refactor\n> > > >> but it is more clean and understandable, is to change the way\n> > > >> write_out_resultes() is called, namely instead of calling it\n> > > >> after all patches being applied in one patch FILE, after each patch\n> > > >> being applied, we write_out_result immediately thus deleting one file\n> > > >> would immediately delete the file from the index.\n>\n> > >\n> > > To extend and automate the solution would be to see, just before\n> > > attempting to do the 3-way, if the incoming patch is a Git generated\n> > > one, and do not even bother using the 3-way logic if it is not.\n> > >\n>\n> Concating two git-generated patches together would fool this mechanism, I\n> suppose. So the above \"bug\" still exists.\nI was testing this issue and I found that one of my other patches,\n\"git-apply: silence errors for success cases\", happens to fix your reported\nissue a bit more generally / cleanly by avoiding 3way for all new patches\nwithout an add conflict. Let me add you to that thread and ping for more\nreview.\n"}]}