Skip to content

Commit

Permalink
rewrite: move_commits: do not remove parents of target commits whic…
Browse files Browse the repository at this point in the history 
…h are outside the target set

This ensures consistency between the commands `jj rebase -r a::` and `jj
rebase -s a`.
  • Loading branch information
@bnjmnt4n
bnjmnt4n committed Aug 17, 2024
1 parent e5493e0 commit f258664
Show file tree
Hide file tree
Showing 2 changed files with 118 additions and 38 deletions.
92 changes: 78 additions & 14 deletions cli/tests/test_rebase_command.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 parent "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###"
Expand All @@ -554,13 +556,14 @@ fn test_rebase_multiple_revisions() {
│ │ ○ h: c d
╭─┬─╯
│ ○ d: a
│ │ ○ g: f
│ │ ○ f: c e
╭───┤
│ │ ○ e: a
│ ├─╯
○ │ c: b
○ │ b: a
├─╯
│ ○ g: f
│ ○ f: e
│ ○ e: a
├─╯
○ a
◆
"###);
Expand All @@ -569,7 +572,8 @@ 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". "f" will retain its parent "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", "d", "-r", "f", "-r", "h", "-d", "b"],
Expand All @@ -589,10 +593,11 @@ fn test_rebase_multiple_revisions() {
│ │ ○ g: c e
╭─┬─╯
│ ○ e: a
○ │ c: b
│ │ ○ h: f
│ │ ○ f: d
│ │ ○ f: c d
╭───┤
│ │ ○ d: b
○ │ │ c: b
├───╯
○ │ b: a
├─╯
Expand Down Expand Up @@ -1612,17 +1617,18 @@ 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
○ e: d
○ d: c
○ c: b2
○ b2: b1
○ b1: a b4
○ c: b2 b4
├─╮
○ │ b2: b1
○ │ b1: a b4
├─╮
│ ○ b4: b3
│ ○ b3: a
Expand All @@ -1632,6 +1638,35 @@ fn test_rebase_revisions_after() {
"###);
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
○ e: b1 b4
├─╮
│ ○ b4: b3
│ ○ b3: a
○ │ b1: a
├─╯
○ a: d
○ d: c
○ c: b2
○ b2
◆
"###);
test_env.jj_cmd_ok(&repo_path, &["op", "restore", &setup_opid]);

// Rebase a subgraph with disconnected commits. Since "b2" is an ancestor of
// "e", "b2" should be a parent of "e" after the rebase.
let (stdout, stderr) = test_env.jj_cmd_ok(
Expand All @@ -1642,8 +1677,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###"
Expand Down Expand Up @@ -2055,6 +2090,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
○ e: b1 b4
├─╮
│ ○ b4: b3
│ ○ b3: a
○ │ b1: a
├─╯
○ a: d
○ d: c
○ c: b2
○ 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,
Expand Down
64 changes: 40 additions & 24 deletions lib/src/rewrite.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -487,33 +487,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<CommitId, Vec<CommitId>> = 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<CommitId, Vec<CommitId>> =
HashMap::new();
for commit in connected_target_commits.iter().rev() {
// The roots of the set will not have any parents found in
// `target_commits_internal_parents`, and will be stored in
// `target_commits_internal_parents` as an empty vector.
// `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();

Expand Down Expand Up @@ -710,17 +710,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() {
// 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()
} else {
target_commit_parents.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
Expand Down

0 comments on commit f258664

Please sign in to comment.