Re: [PATCH] merge-ort: fix a crash in process_renames
- From
Elijah Newren <newren@gmail.com>
- Date
- Mar 6, 2025, 15:27 UTC
- Message-ID
- <CABPp-BFX-HewJWXMRNVMasGOUAuhSn7Pwx9Ls4Virz7fnOqxEA@mail.gmail.com>
- In-Reply-To
- <20240921024533.15249-2-dgoncharov@users.sf.net>
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.