From 3244398e89300be4e1e58ac2b0e84b8919dcd280 Mon Sep 17 00:00:00 2001 From: Ilya Grigoriev Date: Sun, 11 Sep 2022 14:23:46 -0700 Subject: [PATCH] Fix `rebase -r` of a parent of a merge commit Previously, using `rebase -r` on the parent of a merge commit turned it into a non-merge commit. In other words, starting with ``` o d |\ o | c o | b | o a |/ o ``` and doing `rebase -r c -d a` resulted in ``` o d o b | o c | o a |/ o ``` where `d` is no longer a merge commit. For reference, here's a complete test that passed before this commit (but should NOT pass; see the diff for a test that should pass): ``` #[test] fn test_rebase_single_revision_merge_parent() { let test_env = TestEnvironment::default(); test_env.jj_cmd_success(test_env.env_root(), &["init", "repo", "--git"]); let repo_path = test_env.env_root().join("repo"); create_commit(&test_env, &repo_path, "a", &[]); create_commit(&test_env, &repo_path, "b", &[]); create_commit(&test_env, &repo_path, "c", &["b"]); create_commit(&test_env, &repo_path, "d", &["a", "c"]); // Test the setup insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###" @ o d |\ o | c o | b | o a |/ o "###); let stdout = test_env.jj_cmd_success(&repo_path, &["rebase", "-r", "c", "-d", "a"]); insta::assert_snapshot!(stdout, @r###" Also rebased 2 descendant commits onto parent of rebased commit Working copy now at: 3e176b54d680 (no description set) Added 0 files, modified 0 files, removed 2 files "###); insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###" @ o d | o c o | b | o a |/ o "###); } ``` --- src/commands.rs | 39 ++++++++++++++++++++-- tests/test_rebase_command.rs | 64 +++++++++++++++++++++++++++++++----- 2 files changed, 93 insertions(+), 10 deletions(-) diff --git a/src/commands.rs b/src/commands.rs index c9a829a88..63ddd3382 100644 --- a/src/commands.rs +++ b/src/commands.rs @@ -4161,6 +4161,7 @@ fn rebase_revision( let children_expression = RevsetExpression::commit(old_commit.id().clone()).children(); let mut num_rebased_descendants = 0; let store = workspace_command.repo.store(); + for child_commit in children_expression .evaluate( workspace_command.repo().as_repo_ref(), @@ -4170,11 +4171,45 @@ fn rebase_revision( .iter() .commits(store) { + let child_commit = child_commit?; + let new_child_parent_ids: Vec = child_commit + .parents() + .iter() + .flat_map(|c| { + if c == &old_commit { + old_commit + .parents() + .iter() + .map(|c| c.id().clone()) + .collect() + } else { + [c.id().clone()].to_vec() + } + }) + .collect(); + + // Some of the new parents may be ancestors of others as in + // `test_rebase_single_revision`. + let new_child_parents: Result, BackendError> = RevsetExpression::Difference( + RevsetExpression::commits(new_child_parent_ids.clone()), + RevsetExpression::commits(new_child_parent_ids.clone()) + .parents() + .ancestors(), + ) + .evaluate( + workspace_command.repo().as_repo_ref(), + Some(&workspace_command.workspace_id()), + ) + .unwrap() + .iter() + .commits(store) + .collect(); + rebase_commit( ui.settings(), tx.mut_repo(), - &child_commit?, - &old_commit.parents(), + &child_commit, + &new_child_parents?, ); num_rebased_descendants += 1; } diff --git a/tests/test_rebase_command.rs b/tests/test_rebase_command.rs index de2d9e04c..f6fba98ed 100644 --- a/tests/test_rebase_command.rs +++ b/tests/test_rebase_command.rs @@ -186,30 +186,35 @@ fn test_rebase_single_revision() { o "###); - // Descendants of the rebased commit should be rebased onto parents. First we - // test with a non-merge commit, so the descendants should be rebased onto - // the single parent (commit "a"). Then we test with a merge commit, so the - // descendants should be rebased onto the two parents. + // Descendants of the rebased commit "b" should be rebased onto parents. First + // we test with a non-merge commit. Normally, the descendant "c" would still + // have 2 parents afterwards: the parent of "b" -- the root commit -- and + // "a". However, since the root commit is an ancestor of "a", we don't + // actually want both to be parents of the same commit. So, only "a" becomes + // a parent. let stdout = test_env.jj_cmd_success(&repo_path, &["rebase", "-r", "b", "-d", "a"]); insta::assert_snapshot!(stdout, @r###" Also rebased 3 descendant commits onto parent of rebased commit - Working copy now at: b5ce312dacd3 (no description set) - Added 0 files, modified 0 files, removed 2 files + Working copy now at: e7299ad0c9a7 (no description set) + Added 0 files, modified 0 files, removed 1 files "###); insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###" @ o d o c | o b - | o a |/ + o a o "###); test_env.jj_cmd_success(&repo_path, &["undo"]); + + // Now, let's try moving the merge commit. After, both parents of "c" ("a" and + // "b") should become parents of "d". let stdout = test_env.jj_cmd_success(&repo_path, &["rebase", "-r", "c", "-d", "root"]); insta::assert_snapshot!(stdout, @r###" Also rebased 2 descendant commits onto parent of rebased commit - Working copy now at: 5e21ebf9fd2b (no description set) + Working copy now at: 2d90465bd244 (no description set) Added 0 files, modified 0 files, removed 1 files "###); insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###" @@ -226,6 +231,49 @@ fn test_rebase_single_revision() { "###); } +#[test] +fn test_rebase_single_revision_merge_parent() { + let test_env = TestEnvironment::default(); + test_env.jj_cmd_success(test_env.env_root(), &["init", "repo", "--git"]); + let repo_path = test_env.env_root().join("repo"); + + create_commit(&test_env, &repo_path, "a", &[]); + create_commit(&test_env, &repo_path, "b", &[]); + create_commit(&test_env, &repo_path, "c", &["b"]); + create_commit(&test_env, &repo_path, "d", &["a", "c"]); + // Test the setup + insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###" + @ + o d + |\ + o | c + o | b + | o a + |/ + o + "###); + + // Descendants of the rebased commit should be rebased onto parents, and if + // the descendant is a merge commit, it shouldn't forget its other parents. + let stdout = test_env.jj_cmd_success(&repo_path, &["rebase", "-r", "c", "-d", "a"]); + insta::assert_snapshot!(stdout, @r###" + Also rebased 2 descendant commits onto parent of rebased commit + Working copy now at: 9b0a69a895b4 (no description set) + Added 0 files, modified 0 files, removed 1 files + "###); + insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###" + @ + o d + |\ + | | o c + | |/ + o | b + | o a + |/ + o + "###); +} + #[test] fn test_rebase_multiple_destinations() { let test_env = TestEnvironment::default();