[PATCH] merge-ort: fix a crash in process_renames

[PATCH] merge-ort: fix a crash in process_renames

[dgoncharov]

Good morning.

Resending
https://lore.kernel.org/git/20240706211407.512652-1-dgoncharov@users.sf.net/.

[PATCH] merge-ort: fix a crash in process_renames

[dgoncharov]

From: 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

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

Re: [PATCH] merge-ort: fix a crash in process_renames

[Elijah Newren]

I apologize for the very delayed response...

On Fri, Sep 20, 2024 at 7:46 PM <dgoncharov@users.sf.net> wrote:
>
> 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.

> $ 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!

> 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.

> 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.

> +
>                 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.

> +
> +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.

Re: [PATCH] merge-ort: fix a crash in process_renames

[Elijah Newren]

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

[PATCH] merge-ort: fix a crash in process_renames

[dgoncharov]

From: 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

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