From: "Hongren (Zenithal) Zheng" <i@zenithal.me>
To: Junio C Hamano <gitster@pobox.com>, Jerry Zhang <jerry@skydio.com>
Cc: git@vger.kernel.org
Subject: Re: [PATCH] apply: fix delete-then-new patch fail with 3way
Date: Sun, 17 Oct 2021 02:35:29 +0800 [thread overview]
Message-ID: <YWsbcbASLG3QNPyZ@Sun> (raw)
In-Reply-To: <YVmTKWlOFr+IwzzI@Sun>
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@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
>
Is there any updates regarding this patch?
next prev parent reply other threads:[~2021-10-16 18:35 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-10-03 11:25 [PATCH] apply: fix delete-then-new patch fail with 3way Hongren (Zenithal) Zheng
2021-10-16 18:35 ` Hongren (Zenithal) Zheng [this message]
2021-10-17 6:08 ` Junio C Hamano
2021-10-19 18:56 ` Jerry Zhang
2021-11-04 11:16 ` Hongren (Zenithal) Zheng
2021-12-11 1:53 ` Jerry Zhang
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=YWsbcbASLG3QNPyZ@Sun \
--to=i@zenithal.me \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=jerry@skydio.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).