I apologize for the very delayed response...
On Fri, Sep 20, 2024 at 7:46 PM <dgoncharov@xxxxxxxxxxxx> wrote:
>
> From: Dmitry Goncharov <dgoncharov@xxxxxxxxxxxx>
>
> 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@xxxxxxxxxxxx>
> ---
> 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.
