On Sun, Oct 03, 2021 at 07:25:29PM +0800, Hongren (Zenithal) Zheng wrote:
Show 119 quoted lines
> 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@zenithal.me>
> ---
> 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@host1.jankratochvil.net/
>
> 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
>