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@xxxxxxxxxxx> > --- > 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@xxxxxxxxxxxxxxxxxxxxxxx/ > > 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?
