{"thread":{"id":"62162","subject":"[PATCH] merge-ort: fix a crash in process_renames","startedAt":"2024-09-21T02:46:01Z","lastAt":"2025-03-06T17:20:01Z","messageCount":4,"participants":["dgoncharov@users.sf.net","Elijah Newren"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"503180","messageId":"20240921024533.15249-1-dgoncharov@users.sf.net","threadId":"62162","inReplyTo":null,"subject":"[PATCH] merge-ort: fix a crash in process_renames","fromName":"","fromEmail":"dgoncharov@users.sf.net","sentAt":"2024-09-21T02:45:32Z","receivedAt":"2024-09-21T02:46:01Z","isPatch":true,"sender":{"key":"dgoncharov@users.sf.net","avatar":"https://avatars.githubusercontent.com/u/328089?v=4"},"body":"\nGood morning.\n\nResending\nhttps://lore.kernel.org/git/20240706211407.512652-1-dgoncharov@users.sf.net/.\n\n"},{"id":"503181","messageId":"20240921024533.15249-2-dgoncharov@users.sf.net","threadId":"62162","inReplyTo":"20240921024533.15249-1-dgoncharov@users.sf.net","subject":"[PATCH] merge-ort: fix a crash in process_renames","fromName":"","fromEmail":"dgoncharov@users.sf.net","sentAt":"2024-09-21T02:45:33Z","receivedAt":"2024-09-21T02:46:13Z","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"},{"id":"513706","messageId":"CABPp-BFX-HewJWXMRNVMasGOUAuhSn7Pwx9Ls4Virz7fnOqxEA@mail.gmail.com","threadId":"62162","inReplyTo":"20240921024533.15249-2-dgoncharov@users.sf.net","subject":"Re: [PATCH] merge-ort: fix a crash in process_renames","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2025-03-06T15:27:25Z","receivedAt":"2025-03-06T15:27:38Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"I apologize for the very delayed response...\n\nOn Fri, Sep 20, 2024 at 7:46 PM <dgoncharov@users.sf.net> wrote:\n>\n> From: Dmitry Goncharov <dgoncharov@users.sf.net>\n>\n> cherry-pick --strategy=ort (the default at the moment) crashes in the following\n> scenario\n\nGood job, you found a testcase that caused both `ort` and `recursive`\nto fail.  :-)\n\nFor other readers, I'll note that this isn't special to cherry-pick; I\ncan also reproduce using merge for example.\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\n> git: merge-ort.c:3006: process_renames: Assertion `source_deleted || oldinfo->filemask & old_sidemask' failed.\n> Aborted (core dumped)\n\nThanks for putting together a testcase; very helpful.  And you even\nprovided it in the form of a patch, and provided an attempted fix;\nvery nice!\n\n> While cherry picking the top commit from side2 to side1 collect_renames is\n> confused by the preceding move from \"tools/hello\" to \"hello\" that took place on\n> side1. This move from \"tools/hello\" to \"hello\" causes the logic in\n> check_for_directory_rename to incorrectly conclude that \"tools/world\" should be\n> renamed to \"world\".  detect_and_process_renames proceeds with \"world\" instead\n> of \"tools/world\" and ends up tripping on an assertion in process_renames.\n>\n> In 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\n> 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.\n> CONFLICT (content): Merge conflict in world\n\nYes, this is the correct resolution...\n\n> error: cache entry has null sha1: world\n> error: cherry-pick: Unable to write new index file\n> fatal: cherry-pick failed\n\n...but it looks like the recursive backend still trips up on it, just\nnot until after it prints the conflict message.  It shouldn't fail to\nwrite out a new index file; while it got further than ort, that's\nstill pretty bad.\n\n> There really is a merge conflict and the goal of this commit is to have\n> cherry-pick --strategy=ort detect the conflict.  This commit modifies\n> collect_renames to ignore an implicit directory rename that suggests moving a\n> file to itself.\n>\n> Also, see test t3515-cherry-pick-move.sh.\n>\n> Signed-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>\n> diff --git a/merge-ort.c b/merge-ort.c\n> index 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>                                                       collisions,\n>                                                       &clean);\n>\n> +               if (new_path && !strcmp(new_path, p->one->path)) {\n> +                       /* Ignore an implicit directory rename that suggests replacing a move\n> +                        * from one->path to two->path with a move\n> +                        * from one->path to one->path.\n> +                        */\n> +                       free(new_path);\n> +                       new_path = NULL;\n> +               }\n\nUnfortunately, this solution makes it display the wrong conflict\nmessage, which I think could be quite confusing for the user:\n\nCONFLICT (rename/delete): world renamed to tools/world in dac8a10\n(Move world into tools/), but deleted in HEAD.\n\nThe file was not deleted in HEAD.  HEAD moved `tools/hello` to\n`hello`, so the only thing it could have been said to delete was\n`tools/hello`, not `world`.  So I appreciate the attempt to fix, but I\ndon't think this solution is quite right.\n\n> +\n>                 possibly_cache_new_pair(renames, p, side_index, new_path);\n>                 if (p->status != 'R' && !new_path) {\n>                         pool_diff_free_filepair(&opt->priv->pool, p);\n> diff --git a/t/t3515-cherry-pick-move.sh b/t/t3515-cherry-pick-move.sh\n> new file mode 100755\n> index 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> +       mkdir tools &&\n> +\n> +       echo hello >tools/hello &&\n> +\n> +       git add tools/hello &&\n> +       git commit -m\"Add tools/hello.\" tools/hello &&\n> +\n> +       git branch side1 &&\n> +\n> +       # This commit is the base of the fatal cherry-pick merge.\n> +       echo world >world &&\n> +       git add world &&\n> +       git commit -m\"Add world.\" &&\n> +\n> +       # Cherry picking this commit crashes git.\n> +       # This commit is side 2 of the fatal cherry-pick merge.\n> +       git mv -v world tools/world &&\n> +       git commit -m\"mv world tools/world.\" &&\n> +\n> +       git checkout side1 &&\n> +       # This commit is side 1 of the fatal cherry-pick merge.\n> +       git mv -v tools/hello hello &&\n> +       git commit -m\"mv tools/hello hello\"\n> +'\n\nThanks for including this.  I have a slight preference to include this\nin a related testsuite rather than introducing a new testsuite file\njust for it.\n\n> +\n> +test_expect_success 'recursive cherry-pick of a move commit' '\n> +       test_must_fail git cherry-pick --strategy=recursive side2\n> +'\n\nYes, but this doesn't really test that the `recursive` strategy fails\nappropriately.  In particular, the error messages you showed above\npointed out that the recursive backend failed to write a new index\nfile, leaving the working tree and index out of sync.\n\nHowever, since I have some patches to delete the recursive backend\n(and remap it to ort), I think it makes sense to just drop this and\nnot worry about recursive.\n\n> +\n> +test_expect_success 'ort cherry-pick of a move commit' '\n> +       rm -f world &&\n> +       git reset --hard &&\n> +       test_must_fail git cherry-pick --strategy=ort side2\n> +'\n\nThanks for sending this in and even for pinging on it.  I kept it in\nmy notes, even though I didn't have time back then to respond.  I did\nforget about it for a while, but came back after re-checking my notes.\n\nAnyway, I've got a couple patches, the first with your testcase moved\nand adjusted slightly to fit into the existing t6423 with you as the\nauthor, and a second patch with an alternate fix.  I'll submit them\nshortly.\n"},{"id":"513720","messageId":"CABPp-BF-wzhydmEd4_LWRnbdNQJi1UMEJv_quBQX0+ikbS9A8A@mail.gmail.com","threadId":"62162","inReplyTo":"CABPp-BFX-HewJWXMRNVMasGOUAuhSn7Pwx9Ls4Virz7fnOqxEA@mail.gmail.com","subject":"Re: [PATCH] merge-ort: fix a crash in process_renames","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2025-03-06T17:19:47Z","receivedAt":"2025-03-06T17:20:01Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Thu, Mar 6, 2025 at 7:27 AM Elijah Newren <newren@gmail.com> wrote:\n[...]\n> Anyway, I've got a couple patches, the first with your testcase moved\n> and adjusted slightly to fit into the existing t6423 with you as the\n> author, and a second patch with an alternate fix.  I'll submit them\n> shortly.\n\nFor those scrolling the archives later, the fixes are at:\nhttps://lore.kernel.org/git/pull.1873.git.1741275027.gitgitgadget@gmail.com/T/#t\n"}]}