From 3ca78be715a8ae0759ae2b4c0bd8e424a9217123 Mon Sep 17 00:00:00 2001 From: Benjamin Tan Date: Tue, 30 Apr 2024 03:18:03 +0800 Subject: [PATCH] rewrite: `move_commits`: do not remove parents of target commits which are outside the target set This ensures consistency between the commands `jj rebase -r a::` and `jj rebase -s a`. --- cli/tests/test_rebase_command.rs | 89 +++++++++++++++++++++++++++----- lib/src/rewrite.rs | 67 +++++++++++++++--------- 2 files changed, 118 insertions(+), 38 deletions(-) diff --git a/cli/tests/test_rebase_command.rs b/cli/tests/test_rebase_command.rs index d5812d4176..ecb9bd11dd 100644 --- a/cli/tests/test_rebase_command.rs +++ b/cli/tests/test_rebase_command.rs @@ -538,6 +538,8 @@ fn test_rebase_multiple_revisions() { // Test with a subgraph containing a merge commit. Since the merge commit "f" // was extracted, its descendants which are not part of the subgraph will // inherit its descendants which are not in the subtree ("c" and "d"). + // "f" will retain its parents "c" since "c" is outside the target set, and not + // a descendant of any new children. let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["rebase", "-r", "e::g", "-d", "a"]); insta::assert_snapshot!(stdout, @""); insta::assert_snapshot!(stderr, @r###" @@ -554,13 +556,14 @@ fn test_rebase_multiple_revisions() { │ │ ◉ h ╭─┬─╯ │ ◉ d + │ │ ◉ g + │ │ ◉ f + ╭───┤ + │ │ ◉ e + │ ├─╯ ◉ │ c ◉ │ b ├─╯ - │ ◉ g - │ ◉ f - │ ◉ e - ├─╯ ◉ a ◉ "###); @@ -569,7 +572,7 @@ fn test_rebase_multiple_revisions() { // Test with commits in a disconnected subgraph. The subgraph has the // relationship d->e->f->g->h, but only "d", "f" and "h" are in the set of // rebased commits. "d" should be a new parent of "f", and "f" should be a - // new parent of "g". + // new parent of "h". let (stdout, stderr) = test_env.jj_cmd_ok( &repo_path, &["rebase", "-r", "d", "-r", "f", "-r", "h", "-d", "b"], @@ -589,10 +592,11 @@ fn test_rebase_multiple_revisions() { │ │ ◉ g ╭─┬─╯ │ ◉ e - ◉ │ c │ │ ◉ h │ │ ◉ f + ╭───┤ │ │ ◉ d + ◉ │ │ c ├───╯ ◉ │ b ├─╯ @@ -1612,22 +1616,52 @@ fn test_rebase_revisions_after() { insta::assert_snapshot!(stderr, @r###" Rebased 4 commits onto destination Rebased 2 descendant commits - Working copy now at: xznxytkn 084e0629 f | f - Parent commit : nkmrtpmo 563d78c6 e | e + Working copy now at: xznxytkn 9bc7e54c f | f + Parent commit : nkmrtpmo 0f80251b e | e Added 1 files, modified 0 files, removed 0 files "###); insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###" @ f ◉ e ◉ d - ◉ c - ◉ b2 - ◉ b1 + ◉ c + ├─╮ + ◉ │ b2 + ◉ │ b1 + ├─╮ + │ ◉ b4 + │ ◉ b3 + ├─╯ + ◉ a + ◉ + "###); + test_env.jj_cmd_ok(&repo_path, &["op", "restore", &setup_opid]); + + // Rebase a subgraph before the parents of one of the commits in the subgraph. + // "c" had parents "b2" and "b4", but no longer has "b4" as a parent since + // "b4" would be a descendant of "c" after the rebase. + let (stdout, stderr) = + test_env.jj_cmd_ok(&repo_path, &["rebase", "-r", "b2::d", "--after", "root()"]); + insta::assert_snapshot!(stdout, @""); + insta::assert_snapshot!(stderr, @r###" + Rebased 3 commits onto destination + Rebased 6 descendant commits + Working copy now at: xznxytkn 0875aabc f | f + Parent commit : nkmrtpmo d429661b e | e + Added 1 files, modified 0 files, removed 0 files + "###); + insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###" + @ f + ◉ e ├─╮ │ ◉ b4 │ ◉ b3 + ◉ │ b1 ├─╯ ◉ a + ◉ d + ◉ c + ◉ b2 ◉ "###); test_env.jj_cmd_ok(&repo_path, &["op", "restore", &setup_opid]); @@ -1642,8 +1676,8 @@ fn test_rebase_revisions_after() { insta::assert_snapshot!(stderr, @r###" Rebased 2 commits onto destination Rebased 3 descendant commits - Working copy now at: xznxytkn 4fb2bb60 f | f - Parent commit : kmkuslsw cebde86a c | c + Working copy now at: xznxytkn 3238a418 f | f + Parent commit : kmkuslsw 6a51bd41 c | c Added 0 files, modified 0 files, removed 2 files "###); insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###" @@ -2055,6 +2089,35 @@ fn test_rebase_revisions_before() { "###); test_env.jj_cmd_ok(&repo_path, &["op", "restore", &setup_opid]); + // Rebase a subgraph before the parents of one of the commits in the subgraph. + // "c" had parents "b2" and "b4", but no longer has "b4" as a parent since + // "b4" would be a descendant of "c" after the rebase. + let (stdout, stderr) = + test_env.jj_cmd_ok(&repo_path, &["rebase", "-r", "b2::d", "--before", "a"]); + insta::assert_snapshot!(stdout, @""); + insta::assert_snapshot!(stderr, @r###" + Rebased 3 commits onto destination + Rebased 6 descendant commits + Working copy now at: xznxytkn f5991dc7 f | f + Parent commit : nkmrtpmo 37894e3c e | e + Added 1 files, modified 0 files, removed 0 files + "###); + insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###" + @ f + ◉ e + ├─╮ + │ ◉ b4 + │ ◉ b3 + ◉ │ b1 + ├─╯ + ◉ a + ◉ d + ◉ c + ◉ b2 + ◉ + "###); + test_env.jj_cmd_ok(&repo_path, &["op", "restore", &setup_opid]); + // Should error if a loop will be created. let stderr = test_env.jj_cmd_failure( &repo_path, diff --git a/lib/src/rewrite.rs b/lib/src/rewrite.rs index c799a6eda5..5886f20cf7 100644 --- a/lib/src/rewrite.rs +++ b/lib/src/rewrite.rs @@ -485,32 +485,33 @@ pub fn move_commits( .commits(mut_repo.store()) .try_collect()?; - // Commits in the target set should only have other commits in the set as - // parents, except the roots of the set, which persist their original - // parents. - // If a commit in the set has a parent which is not in the set, but has - // an ancestor which is in the set, then the commit will have that ancestor - // as a parent. - let mut target_commits_internal_parents: HashMap> = HashMap::new(); + // Compute the parents of all commits in the connected target set, + // allowing only commits in the target set as parents. + // The parents of each commit are identical to the ones found using a preorder + // DFS of the node's ancestors, starting from the node itself, and avoiding + // traversing an edge if the parent is in the target set. + let mut connected_target_commits_internal_parents: HashMap> = + HashMap::new(); for commit in connected_target_commits.iter().rev() { - // The roots of the set will not have any parents found in `new_target_parents`, - // and will be stored in `new_target_parents` as an empty vector. + // The roots of the set will not have any parents found in + // `connected_target_commits_internal_parents`, and will be stored as an empty + // vector. let mut new_parents = vec![]; for old_parent in commit.parent_ids() { if target_commit_ids.contains(old_parent) { new_parents.push(old_parent.clone()); - } else if let Some(parents) = target_commits_internal_parents.get(old_parent) { + } else if let Some(parents) = connected_target_commits_internal_parents.get(old_parent) + { new_parents.extend(parents.iter().cloned()); } } - target_commits_internal_parents.insert(commit.id().clone(), new_parents); + connected_target_commits_internal_parents.insert(commit.id().clone(), new_parents); } - target_commits_internal_parents.retain(|id, _| target_commit_ids.contains(id)); // Compute the roots of `target_commits`. - let target_roots: HashSet<_> = target_commits_internal_parents + let target_roots: HashSet<_> = connected_target_commits_internal_parents .iter() - .filter(|(_, parents)| parents.is_empty()) + .filter(|(commit_id, parents)| target_commit_ids.contains(commit_id) && parents.is_empty()) .map(|(commit_id, _)| commit_id.clone()) .collect(); @@ -701,17 +702,33 @@ pub fn move_commits( if let Some(new_child_parents) = new_children_parents.get(commit_id) { new_child_parents.clone() } - // Commits in the target set should persist only rebased parents from the target - // sets. - else if let Some(target_commit_parents) = - target_commits_internal_parents.get(commit_id) - { - // If the commit does not have any parents in the target set, it is one of the - // commits in the root set, and should be rebased onto the new destination. - if target_commit_parents.is_empty() { - new_parent_ids.clone() - } else { - target_commit_parents.clone() + // Commit is in the target set. + else if target_commit_ids.contains(commit_id) { + // If the commit is a root of the target set, it should be rebased onto the new destination. + if target_roots.contains(commit_id) { + new_parent_ids.clone() + } + // Otherwise: + // 1. Keep parents which are within the target set. + // 2. Replace parents which are outside the target set but are part of the + // connected target set with their ancestor commits which are in the target + // set. + // 3. Keep other parents outside the target set if they are not descendants of the + // new children of the target set. + else { + let mut new_parents = vec![]; + for parent_id in commit.parent_ids() { + if target_commit_ids.contains(parent_id) { + new_parents.push(parent_id.clone()); + } else if let Some(parents) = + connected_target_commits_internal_parents.get(parent_id) { + new_parents.extend(parents.iter().cloned()); + } else if !new_children.iter().any(|new_child| { + mut_repo.index().is_ancestor(new_child.id(), parent_id) }) { + new_parents.push(parent_id.clone()); + } + } + new_parents } } // Commits outside the target set should have references to commits inside the set