merge-ort: fix a crash in process_renames

Commit Message

Dmitry Goncharov July 6, 2024, 9:14 p.m. UTC

From: Dmitry Goncharov <[e-mail]>

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 <[e-mail]>
---
 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

Patch

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