{"thread":{"id":"61746","subject":"[PATCH] merge-ort: fix a crash in process_renames","startedAt":"2024-07-06T21:14:12Z","lastAt":"2024-07-06T21:14:12Z","messageCount":1,"participants":["dgoncharov@users.sf.net"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"498175","messageId":"20240706211407.512652-1-dgoncharov@users.sf.net","threadId":"61746","inReplyTo":null,"subject":"[PATCH] merge-ort: fix a crash in process_renames","fromName":"","fromEmail":"dgoncharov@users.sf.net","sentAt":"2024-07-06T21:14:07Z","receivedAt":"2024-07-06T21:14:12Z","isPatch":true,"sender":{"key":"dgoncharov@users.sf.net","avatar":"https://avatars.githubusercontent.com/u/328089?v=4"},"body":"From: Dmitry Goncharov <dgoncharov@users.sf.net>\n\ncherry-pick --strategy=ort (the default at the moment) crashes in the following\nscenario\n\n$ ls -a\n.  ..\n$ mkdir tools\n$ git init -q -b side2\n$ echo hello>tools/hello\n$ git add  tools/hello\n$ git commit -q tools/hello -m'Add tools/hello.'\n$ git branch side1\n$ echo world>world\n$ git add world\n$ git commit -q world -m'Add world.'\n$ git mv  world tools/world\n$ git commit -q -m'mv world tools/world.'\n$ git checkout -q side1\n$ git mv tools/hello hello\n$ git commit -q -m'mv tools/hello hello.'\n$ git cherry-pick --strategy=ort side2\ngit: merge-ort.c:3006: process_renames: Assertion `source_deleted || oldinfo->filemask & old_sidemask' failed.\nAborted (core dumped)\n\nWhile cherry picking the top commit from side2 to side1 collect_renames is\nconfused by the preceding move from \"tools/hello\" to \"hello\" that took place on\nside1. This move from \"tools/hello\" to \"hello\" causes the logic in\ncheck_for_directory_rename to incorrectly conclude that \"tools/world\" should be\nrenamed to \"world\".  detect_and_process_renames proceeds with \"world\" instead\nof \"tools/world\" and ends up tripping on an assertion in process_renames.\n\nIn the same scenario cherry-pick --strategy=recursive detects a merge conflict.\n\n$ rm .git/index.lock\n$ git reset -q --hard\n$ git cherry-pick --strategy=recursive side2\nCONFLICT (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.\nCONFLICT (content): Merge conflict in world\nerror: cache entry has null sha1: world\nerror: cherry-pick: Unable to write new index file\nfatal: cherry-pick failed\n\nThere really is a merge conflict and the goal of this commit is to have\ncherry-pick --strategy=ort detect the conflict.  This commit modifies\ncollect_renames to ignore an implicit directory rename that suggests moving a\nfile to itself.\n\nAlso, see test t3515-cherry-pick-move.sh.\n\nSigned-off-by: Dmitry Goncharov <dgoncharov@users.sf.net>\n---\n merge-ort.c                 |  9 +++++++\n t/t3515-cherry-pick-move.sh | 48 +++++++++++++++++++++++++++++++++++++\n 2 files changed, 57 insertions(+)\n create mode 100755 t/t3515-cherry-pick-move.sh\n\ndiff --git a/merge-ort.c b/merge-ort.c\nindex 691db9050e..e58fb7a7fa 100644\n--- a/merge-ort.c\n+++ b/merge-ort.c\n@@ -3369,6 +3369,15 @@ static int collect_renames(struct merge_options *opt,\n \t\t\t\t\t\t      collisions,\n \t\t\t\t\t\t      &clean);\n \n+\t\tif (new_path && !strcmp(new_path, p->one->path)) {\n+\t\t\t/* Ignore an implicit directory rename that suggests replacing a move\n+\t\t\t * from one->path to two->path with a move\n+\t\t\t * from one->path to one->path.\n+\t\t\t */\n+\t\t\tfree(new_path);\n+\t\t\tnew_path = NULL;\n+\t\t}\n+\n \t\tpossibly_cache_new_pair(renames, p, side_index, new_path);\n \t\tif (p->status != 'R' && !new_path) {\n \t\t\tpool_diff_free_filepair(&opt->priv->pool, p);\ndiff --git a/t/t3515-cherry-pick-move.sh b/t/t3515-cherry-pick-move.sh\nnew file mode 100755\nindex 0000000000..20af478d4e\n--- /dev/null\n+++ b/t/t3515-cherry-pick-move.sh\n@@ -0,0 +1,48 @@\n+#!/bin/sh\n+\n+test_description='Test cherry-picking a move commit.'\n+\n+\n+GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=side2\n+export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n+\n+TEST_PASSES_SANITIZE_LEAK=true\n+. ./test-lib.sh\n+\n+test_expect_success setup '\n+\tmkdir tools &&\n+\n+\techo hello >tools/hello &&\n+\n+\tgit add tools/hello &&\n+\tgit commit -m\"Add tools/hello.\" tools/hello &&\n+\n+\tgit branch side1 &&\n+\n+\t# This commit is the base of the fatal cherry-pick merge.\n+\techo world >world &&\n+\tgit add world &&\n+\tgit commit -m\"Add world.\" &&\n+\n+\t# Cherry picking this commit crashes git.\n+\t# This commit is side 2 of the fatal cherry-pick merge.\n+\tgit mv -v world tools/world &&\n+\tgit commit -m\"mv world tools/world.\" &&\n+\n+\tgit checkout side1 &&\n+\t# This commit is side 1 of the fatal cherry-pick merge.\n+\tgit mv -v tools/hello hello &&\n+\tgit commit -m\"mv tools/hello hello\"\n+'\n+\n+test_expect_success 'recursive cherry-pick of a move commit' '\n+\ttest_must_fail git cherry-pick --strategy=recursive side2\n+'\n+\n+test_expect_success 'ort cherry-pick of a move commit' '\n+\trm -f world &&\n+\tgit reset --hard &&\n+\ttest_must_fail git cherry-pick --strategy=ort side2\n+'\n+\n+test_done\n-- \n2.34.1\n\n"}]}