dgoncharov@users.sf.netGood morning.
Resending https://lore.kernel.org/git/20240706211407.512652-1-dgoncharov@users.sf.net/.
threads / patch / 62162
patchmerge-ort: fix a crash in process_renames
Subject: [PATCH] merge-ort: fix a crash in process_renames
dgoncharov@users.sf.netdgoncharov@users.sf.netFrom: Dmitry Goncharov <dgoncharov@users.sf.net>
cherry-pick --strategy=ort (the default at the moment) crashes in the following scenario
$ ls -a . .. $ mkdir tools $ git init -q -b side2 $ echo hello>tools/hello $ git add tools/hello $ git commit -q tools/hello -m'Add tools/hello.' $ git branch side1 $ echo world>world $ git add world $ git commit -q world -m'Add world.' $ git mv world tools/world $ git commit -q -m'mv world tools/world.' $ git checkout -q side1 $ git mv tools/hello hello $ git commit -q -m'mv tools/hello hello.' $ git cherry-pick --strategy=ort side2 git: merge-ort.c:3006: process_renames: Assertion `source_deleted || oldinfo->filemask & old_sidemask' failed. Aborted (core dumped)
While cherry picking the top commit from side2 to side1 collect_renames is confused by the preceding move from "tools/hello" to "hello" that took place on side1. This move from "tools/hello" to "hello" causes the logic in check_for_directory_rename to incorrectly conclude that "tools/world" should be renamed to "world". detect_and_process_renames proceeds with "world" instead of "tools/world" and ends up tripping on an assertion in process_renames.
In the same scenario cherry-pick --strategy=recursive detects a merge conflict.
$ rm .git/index.lock $ git reset -q --hard $ git cherry-pick --strategy=recursive side2 CONFLICT (file location): world renamed to tools/world in fead592 (mv world tools/world.), inside a directory that was renamed in HEAD, suggesting it should perhaps be moved to world. CONFLICT (content): Merge conflict in world error: cache entry has null sha1: world error: cherry-pick: Unable to write new index file fatal: cherry-pick failed
There really is a merge conflict and the goal of this commit is to have cherry-pick --strategy=ort detect the conflict. This commit modifies collect_renames to ignore an implicit directory rename that suggests moving a file to itself.
Also, see test t3515-cherry-pick-move.sh.
Signed-off-by: Dmitry Goncharov <dgoncharov@users.sf.net> --- merge-ort.c | 9 +++++++ t/t3515-cherry-pick-move.sh | 48 +++++++++++++++++++++++++++++++++++++ 2 files changed, 57 insertions(+) create mode 100755 t/t3515-cherry-pick-move.sh
Show changes to 2 files +57 −0
merge-ort.c, t/t3515-cherry-pick-move.sh
diff --git a/merge-ort.c b/merge-ort.c index 691db9050e..e58fb7a7fa 100644 --- a/merge-ort.c +++ b/merge-ort.c @@ -3369,6 +3369,15 @@ static int collect_renames(struct merge_options *opt, collisions, &clean); + if (new_path && !strcmp(new_path, p->one->path)) { + /* Ignore an implicit directory rename that suggests replacing a move + * from one->path to two->path with a move + * from one->path to one->path. + */ + free(new_path); + new_path = NULL; + } + possibly_cache_new_pair(renames, p, side_index, new_path); if (p->status != 'R' && !new_path) { pool_diff_free_filepair(&opt->priv->pool, p); diff --git a/t/t3515-cherry-pick-move.sh b/t/t3515-cherry-pick-move.sh new file mode 100755 index 0000000000..20af478d4e --- /dev/null +++ b/t/t3515-cherry-pick-move.sh @@ -0,0 +1,48 @@ +#!/bin/sh + +test_description='Test cherry-picking a move commit.' + + +GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=side2 +export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME + +TEST_PASSES_SANITIZE_LEAK=true +. ./test-lib.sh + +test_expect_success setup ' + mkdir tools && + + echo hello >tools/hello && + + git add tools/hello && + git commit -m"Add tools/hello." tools/hello && + + git branch side1 && + + # This commit is the base of the fatal cherry-pick merge. + echo world >world && + git add world && + git commit -m"Add world." && + + # Cherry picking this commit crashes git. + # This commit is side 2 of the fatal cherry-pick merge. + git mv -v world tools/world && + git commit -m"mv world tools/world." && + + git checkout side1 && + # This commit is side 1 of the fatal cherry-pick merge. + git mv -v tools/hello hello && + git commit -m"mv tools/hello hello" +' + +test_expect_success 'recursive cherry-pick of a move commit' ' + test_must_fail git cherry-pick --strategy=recursive side2 +' + +test_expect_success 'ort cherry-pick of a move commit' ' + rm -f world && + git reset --hard && + test_must_fail git cherry-pick --strategy=ort side2 +' + +test_done
-- 2.34.1
Elijah NewrenRe: [PATCH] merge-ort: fix a crash in process_renames
I apologize for the very delayed response...
On Fri, Sep 20, 2024 at 7:46 PM <dgoncharov@users.sf.net> wrote:
Show 5 quoted lines
> > From: Dmitry Goncharov <dgoncharov@users.sf.net> > > cherry-pick --strategy=ort (the default at the moment) crashes in the following > scenario
Good job, you found a testcase that caused both `ort` and `recursive` to fail. :-)
For other readers, I'll note that this isn't special to cherry-pick; I can also reproduce using merge for example.
Show 19 quoted lines
> $ ls -a > . .. > $ mkdir tools > $ git init -q -b side2 > $ echo hello>tools/hello > $ git add tools/hello > $ git commit -q tools/hello -m'Add tools/hello.' > $ git branch side1 > $ echo world>world > $ git add world > $ git commit -q world -m'Add world.' > $ git mv world tools/world > $ git commit -q -m'mv world tools/world.' > $ git checkout -q side1 > $ git mv tools/hello hello > $ git commit -q -m'mv tools/hello hello.' > $ git cherry-pick --strategy=ort side2 > git: merge-ort.c:3006: process_renames: Assertion `source_deleted || oldinfo->filemask & old_sidemask' failed. > Aborted (core dumped)
Thanks for putting together a testcase; very helpful. And you even provided it in the form of a patch, and provided an attempted fix; very nice!
Show 14 quoted lines
> While cherry picking the top commit from side2 to side1 collect_renames is > confused by the preceding move from "tools/hello" to "hello" that took place on > side1. This move from "tools/hello" to "hello" causes the logic in > check_for_directory_rename to incorrectly conclude that "tools/world" should be > renamed to "world". detect_and_process_renames proceeds with "world" instead > of "tools/world" and ends up tripping on an assertion in process_renames. > > In the same scenario cherry-pick --strategy=recursive detects a merge conflict. > > $ rm .git/index.lock > $ git reset -q --hard > $ git cherry-pick --strategy=recursive side2 > CONFLICT (file location): world renamed to tools/world in fead592 (mv world tools/world.), inside a directory that was renamed in HEAD, suggesting it should perhaps be moved to world. > CONFLICT (content): Merge conflict in world
Yes, this is the correct resolution...
> error: cache entry has null sha1: world > error: cherry-pick: Unable to write new index file > fatal: cherry-pick failed
...but it looks like the recursive backend still trips up on it, just not until after it prints the conflict message. It shouldn't fail to write out a new index file; while it got further than ort, that's still pretty bad.
Show 30 quoted lines
> There really is a merge conflict and the goal of this commit is to have
> cherry-pick --strategy=ort detect the conflict. This commit modifies
> collect_renames to ignore an implicit directory rename that suggests moving a
> file to itself.
>
> Also, see test t3515-cherry-pick-move.sh.
>
> Signed-off-by: Dmitry Goncharov <dgoncharov@users.sf.net>
> ---
> merge-ort.c | 9 +++++++
> t/t3515-cherry-pick-move.sh | 48 +++++++++++++++++++++++++++++++++++++
> 2 files changed, 57 insertions(+)
> create mode 100755 t/t3515-cherry-pick-move.sh
>
> diff --git a/merge-ort.c b/merge-ort.c
> index 691db9050e..e58fb7a7fa 100644
> --- a/merge-ort.c
> +++ b/merge-ort.c
> @@ -3369,6 +3369,15 @@ static int collect_renames(struct merge_options *opt,
> collisions,
> &clean);
>
> + if (new_path && !strcmp(new_path, p->one->path)) {
> + /* Ignore an implicit directory rename that suggests replacing a move
> + * from one->path to two->path with a move
> + * from one->path to one->path.
> + */
> + free(new_path);
> + new_path = NULL;
> + }Unfortunately, this solution makes it display the wrong conflict message, which I think could be quite confusing for the user:
CONFLICT (rename/delete): world renamed to tools/world in dac8a10 (Move world into tools/), but deleted in HEAD.
The file was not deleted in HEAD. HEAD moved `tools/hello` to `hello`, so the only thing it could have been said to delete was `tools/hello`, not `world`. So I appreciate the attempt to fix, but I don't think this solution is quite right.
Show 46 quoted lines
> +
> possibly_cache_new_pair(renames, p, side_index, new_path);
> if (p->status != 'R' && !new_path) {
> pool_diff_free_filepair(&opt->priv->pool, p);
> diff --git a/t/t3515-cherry-pick-move.sh b/t/t3515-cherry-pick-move.sh
> new file mode 100755
> index 0000000000..20af478d4e
> --- /dev/null
> +++ b/t/t3515-cherry-pick-move.sh
> @@ -0,0 +1,48 @@
> +#!/bin/sh
> +
> +test_description='Test cherry-picking a move commit.'
> +
> +
> +GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=side2
> +export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
> +
> +TEST_PASSES_SANITIZE_LEAK=true
> +. ./test-lib.sh
> +
> +test_expect_success setup '
> + mkdir tools &&
> +
> + echo hello >tools/hello &&
> +
> + git add tools/hello &&
> + git commit -m"Add tools/hello." tools/hello &&
> +
> + git branch side1 &&
> +
> + # This commit is the base of the fatal cherry-pick merge.
> + echo world >world &&
> + git add world &&
> + git commit -m"Add world." &&
> +
> + # Cherry picking this commit crashes git.
> + # This commit is side 2 of the fatal cherry-pick merge.
> + git mv -v world tools/world &&
> + git commit -m"mv world tools/world." &&
> +
> + git checkout side1 &&
> + # This commit is side 1 of the fatal cherry-pick merge.
> + git mv -v tools/hello hello &&
> + git commit -m"mv tools/hello hello"
> +'Thanks for including this. I have a slight preference to include this in a related testsuite rather than introducing a new testsuite file just for it.
> + > +test_expect_success 'recursive cherry-pick of a move commit' ' > + test_must_fail git cherry-pick --strategy=recursive side2 > +'
Yes, but this doesn't really test that the `recursive` strategy fails appropriately. In particular, the error messages you showed above pointed out that the recursive backend failed to write a new index file, leaving the working tree and index out of sync.
However, since I have some patches to delete the recursive backend (and remap it to ort), I think it makes sense to just drop this and not worry about recursive.
Show 6 quoted lines
> + > +test_expect_success 'ort cherry-pick of a move commit' ' > + rm -f world && > + git reset --hard && > + test_must_fail git cherry-pick --strategy=ort side2 > +'
Thanks for sending this in and even for pinging on it. I kept it in my notes, even though I didn't have time back then to respond. I did forget about it for a while, but came back after re-checking my notes.
Anyway, I've got a couple patches, the first with your testcase moved and adjusted slightly to fit into the existing t6423 with you as the author, and a second patch with an alternate fix. I'll submit them shortly.
Elijah NewrenRe: [PATCH] merge-ort: fix a crash in process_renames
On Thu, Mar 6, 2025 at 7:27 AM Elijah Newren <newren@gmail.com> wrote: [...]
> Anyway, I've got a couple patches, the first with your testcase moved > and adjusted slightly to fit into the existing t6423 with you as the > author, and a second patch with an alternate fix. I'll submit them > shortly.
For those scrolling the archives later, the fixes are at: https://lore.kernel.org/git/pull.1873.git.1741275027.gitgitgadget@gmail.com/T/#t