From 53a0555490befbe16d40d31c5e8695bfb0f99279 Mon Sep 17 00:00:00 2001 From: Ilya Grigoriev Date: Thu, 21 Dec 2023 21:33:00 -0800 Subject: [PATCH 1/6] tests: (mostly) stop using soon-to-be-private DescendantRebaser-related APIs This removes uses of `DescendantRebaser::new` or `MutRepo::create_descendant_rebaser` from most tests. The exceptions are the tests having to do with abandoning empty commits on rebase, since adjusting those is a bit more elaborate (see follow-up commits). --- lib/tests/test_commit_builder.rs | 25 +- lib/tests/test_mut_repo.rs | 46 ++-- lib/tests/test_rewrite.rs | 420 +++++++++++++++---------------- lib/testutils/src/lib.rs | 31 ++- 4 files changed, 278 insertions(+), 244 deletions(-) diff --git a/lib/tests/test_commit_builder.rs b/lib/tests/test_commit_builder.rs index 4d38162737..1269fb79f2 100644 --- a/lib/tests/test_commit_builder.rs +++ b/lib/tests/test_commit_builder.rs @@ -19,7 +19,7 @@ use jj_lib::repo::Repo; use jj_lib::repo_path::{RepoPath, RepoPathBuf}; use jj_lib::settings::UserSettings; use test_case::test_case; -use testutils::{assert_rebased, create_tree, CommitGraphBuilder, TestRepo, TestRepoBackend}; +use testutils::{assert_rebased_onto, create_tree, CommitGraphBuilder, TestRepo, TestRepoBackend}; fn to_owned_path_vec(paths: &[&RepoPath]) -> Vec { paths.iter().map(|&path| path.to_owned()).collect() @@ -269,8 +269,11 @@ fn test_commit_builder_descendants(backend: TestRepoBackend) { ) .write() .unwrap(); - let mut rebaser = tx.mut_repo().create_descendant_rebaser(&settings); - assert!(rebaser.rebase_next().unwrap().is_none()); + let rebase_map = tx + .mut_repo() + .rebase_descendants_return_map(&settings) + .unwrap(); + assert_eq!(rebase_map.len(), 0); // Test with for_rewrite_from() let mut tx = repo.start_transaction(&settings); @@ -279,9 +282,12 @@ fn test_commit_builder_descendants(backend: TestRepoBackend) { .rewrite_commit(&settings, &commit2) .write() .unwrap(); - let mut rebaser = tx.mut_repo().create_descendant_rebaser(&settings); - assert_rebased(rebaser.rebase_next().unwrap(), &commit3, &[&commit4]); - assert!(rebaser.rebase_next().unwrap().is_none()); + let rebase_map = tx + .mut_repo() + .rebase_descendants_return_map(&settings) + .unwrap(); + assert_rebased_onto(tx.mut_repo(), &rebase_map, &commit3, &[commit4.id()]); + assert_eq!(rebase_map.len(), 1); // Test with for_rewrite_from() but new change id let mut tx = repo.start_transaction(&settings); @@ -290,6 +296,9 @@ fn test_commit_builder_descendants(backend: TestRepoBackend) { .generate_new_change_id() .write() .unwrap(); - let mut rebaser = tx.mut_repo().create_descendant_rebaser(&settings); - assert!(rebaser.rebase_next().unwrap().is_none()); + let rebase_map = tx + .mut_repo() + .rebase_descendants_return_map(&settings) + .unwrap(); + assert!(rebase_map.is_empty()); } diff --git a/lib/tests/test_mut_repo.rs b/lib/tests/test_mut_repo.rs index c28e8785f5..1533f2fd8e 100644 --- a/lib/tests/test_mut_repo.rs +++ b/lib/tests/test_mut_repo.rs @@ -17,7 +17,7 @@ use jj_lib::op_store::{RefTarget, RemoteRef, RemoteRefState, WorkspaceId}; use jj_lib::repo::Repo; use maplit::hashset; use testutils::{ - assert_rebased, create_random_commit, write_random_commit, CommitGraphBuilder, TestRepo, + assert_rebased_onto, create_random_commit, write_random_commit, CommitGraphBuilder, TestRepo, }; #[test] @@ -517,9 +517,7 @@ fn test_has_changed() { #[test] fn test_rebase_descendants_simple() { - // Tests that MutableRepo::create_descendant_rebaser() creates a - // DescendantRebaser that rebases descendants of rewritten and abandoned - // commits. + // There are many additional tests of this functionality in `test_rewrite.rs`. let settings = testutils::user_settings(); let test_repo = TestRepo::init(); let repo = &test_repo.repo; @@ -539,24 +537,29 @@ fn test_rebase_descendants_simple() { let commit6 = graph_builder.commit_with_parents(&[&commit1]); mut_repo.record_rewritten_commit(commit2.id().clone(), commit6.id().clone()); mut_repo.record_abandoned_commit(commit4.id().clone()); - let mut rebaser = mut_repo.create_descendant_rebaser(&settings); + let rebase_map = tx + .mut_repo() + .rebase_descendants_return_map(&settings) + .unwrap(); // Commit 3 got rebased onto commit 2's replacement, i.e. commit 6 - assert_rebased(rebaser.rebase_next().unwrap(), &commit3, &[&commit6]); + assert_rebased_onto(tx.mut_repo(), &rebase_map, &commit3, &[commit6.id()]); // Commit 5 got rebased onto commit 4's parent, i.e. commit 1 - assert_rebased(rebaser.rebase_next().unwrap(), &commit5, &[&commit1]); - assert!(rebaser.rebase_next().unwrap().is_none()); + assert_rebased_onto(tx.mut_repo(), &rebase_map, &commit5, &[commit1.id()]); + assert_eq!(rebase_map.len(), 2); + // No more descendants to rebase if we try again. - assert!(mut_repo - .create_descendant_rebaser(&settings) - .rebase_next() - .unwrap() - .is_none()); + let rebase_map = tx + .mut_repo() + .rebase_descendants_return_map(&settings) + .unwrap(); + assert_eq!(rebase_map.len(), 0); } #[test] fn test_rebase_descendants_conflicting_rewrite() { - // Tests MutableRepo::create_descendant_rebaser() when a commit has been marked - // as rewritten to several other commits. + // Test rebasing descendants when one commit was rewritten to several other + // commits. There are many additional tests of this functionality in + // `test_rewrite.rs`. let settings = testutils::user_settings(); let test_repo = TestRepo::init(); let repo = &test_repo.repo; @@ -575,16 +578,13 @@ fn test_rebase_descendants_conflicting_rewrite() { let commit5 = graph_builder.commit_with_parents(&[&commit1]); mut_repo.record_rewritten_commit(commit2.id().clone(), commit4.id().clone()); mut_repo.record_rewritten_commit(commit2.id().clone(), commit5.id().clone()); - let mut rebaser = mut_repo.create_descendant_rebaser(&settings); // Commit 3 does *not* get rebased because it's unclear if it should go onto // commit 4 or commit 5 - assert!(rebaser.rebase_next().unwrap().is_none()); - // No more descendants to rebase if we try again. - assert!(mut_repo - .create_descendant_rebaser(&settings) - .rebase_next() - .unwrap() - .is_none()); + let rebase_map = tx + .mut_repo() + .rebase_descendants_return_map(&settings) + .unwrap(); + assert!(rebase_map.is_empty()); } #[test] diff --git a/lib/tests/test_rewrite.rs b/lib/tests/test_rewrite.rs index 7e278d6d1e..def386b81a 100644 --- a/lib/tests/test_rewrite.rs +++ b/lib/tests/test_rewrite.rs @@ -25,8 +25,8 @@ use jj_lib::rewrite::{ use maplit::{hashmap, hashset}; use test_case::test_case; use testutils::{ - assert_rebased, create_random_commit, create_tree, write_random_commit, CommitGraphBuilder, - TestRepo, + assert_rebased, assert_rebased_onto, create_random_commit, create_tree, write_random_commit, + CommitGraphBuilder, TestRepo, }; #[test] @@ -87,19 +87,17 @@ fn test_rebase_descendants_sideways() { let commit_e = graph_builder.commit_with_parents(&[&commit_b]); let commit_f = graph_builder.commit_with_parents(&[&commit_a]); - let mut rebaser = DescendantRebaser::new( - &settings, - tx.mut_repo(), - hashmap! { - commit_b.id().clone() => hashset!{commit_f.id().clone()} - }, - hashset! {}, - ); - let new_commit_c = assert_rebased(rebaser.rebase_next().unwrap(), &commit_c, &[&commit_f]); - let new_commit_d = assert_rebased(rebaser.rebase_next().unwrap(), &commit_d, &[&new_commit_c]); - let new_commit_e = assert_rebased(rebaser.rebase_next().unwrap(), &commit_e, &[&commit_f]); - assert!(rebaser.rebase_next().unwrap().is_none()); - assert_eq!(rebaser.rebased().len(), 3); + tx.mut_repo() + .record_rewritten_commit(commit_b.id().clone(), commit_f.id().clone()); + let rebase_map = tx + .mut_repo() + .rebase_descendants_return_map(&settings) + .unwrap(); + assert_eq!(rebase_map.len(), 3); + let new_commit_c = assert_rebased_onto(tx.mut_repo(), &rebase_map, &commit_c, &[commit_f.id()]); + let new_commit_d = + assert_rebased_onto(tx.mut_repo(), &rebase_map, &commit_d, &[new_commit_c.id()]); + let new_commit_e = assert_rebased_onto(tx.mut_repo(), &rebase_map, &commit_e, &[commit_f.id()]); assert_eq!( *tx.mut_repo().view().heads(), @@ -143,21 +141,23 @@ fn test_rebase_descendants_forward() { let commit_f = graph_builder.commit_with_parents(&[&commit_d]); let commit_g = graph_builder.commit_with_parents(&[&commit_f]); - let mut rebaser = DescendantRebaser::new( - &settings, - tx.mut_repo(), - hashmap! { - commit_b.id().clone() => hashset!{commit_f.id().clone()} - }, - hashset! {}, - ); - let new_commit_d = assert_rebased(rebaser.rebase_next().unwrap(), &commit_d, &[&commit_f]); - let new_commit_f = assert_rebased(rebaser.rebase_next().unwrap(), &commit_f, &[&new_commit_d]); - let new_commit_c = assert_rebased(rebaser.rebase_next().unwrap(), &commit_c, &[&new_commit_f]); - let new_commit_e = assert_rebased(rebaser.rebase_next().unwrap(), &commit_e, &[&new_commit_d]); - let new_commit_g = assert_rebased(rebaser.rebase_next().unwrap(), &commit_g, &[&new_commit_f]); - assert!(rebaser.rebase_next().unwrap().is_none()); - assert_eq!(rebaser.rebased().len(), 5); + tx.mut_repo() + .record_rewritten_commit(commit_b.id().clone(), commit_f.id().clone()); + let rebase_map = tx + .mut_repo() + .rebase_descendants_return_map(&settings) + .unwrap(); + let new_commit_d = + assert_rebased_onto(tx.mut_repo(), &rebase_map, &commit_d, &[&commit_f.id()]); + let new_commit_f = + assert_rebased_onto(tx.mut_repo(), &rebase_map, &commit_f, &[&new_commit_d.id()]); + let new_commit_c = + assert_rebased_onto(tx.mut_repo(), &rebase_map, &commit_c, &[&new_commit_f.id()]); + let new_commit_e = + assert_rebased_onto(tx.mut_repo(), &rebase_map, &commit_e, &[&new_commit_d.id()]); + let new_commit_g = + assert_rebased_onto(tx.mut_repo(), &rebase_map, &commit_g, &[&new_commit_f.id()]); + assert_eq!(rebase_map.len(), 5); assert_eq!( *tx.mut_repo().view().heads(), @@ -198,19 +198,19 @@ fn test_rebase_descendants_reorder() { let commit_h = graph_builder.commit_with_parents(&[&commit_f]); let commit_i = graph_builder.commit_with_parents(&[&commit_g]); - let mut rebaser = DescendantRebaser::new( - &settings, - tx.mut_repo(), - hashmap! { - commit_e.id().clone() => hashset!{commit_d.id().clone()}, - commit_c.id().clone() => hashset!{commit_f.id().clone()}, - commit_g.id().clone() => hashset!{commit_h.id().clone()}, - }, - hashset! {}, - ); - let new_commit_i = assert_rebased(rebaser.rebase_next().unwrap(), &commit_i, &[&commit_h]); - assert!(rebaser.rebase_next().unwrap().is_none()); - assert_eq!(rebaser.rebased().len(), 1); + tx.mut_repo() + .record_rewritten_commit(commit_e.id().clone(), commit_d.id().clone()); + tx.mut_repo() + .record_rewritten_commit(commit_c.id().clone(), commit_f.id().clone()); + tx.mut_repo() + .record_rewritten_commit(commit_g.id().clone(), commit_h.id().clone()); + let rebase_map = tx + .mut_repo() + .rebase_descendants_return_map(&settings) + .unwrap(); + let new_commit_i = + assert_rebased_onto(tx.mut_repo(), &rebase_map, &commit_i, &[&commit_h.id()]); + assert_eq!(rebase_map.len(), 1); assert_eq!( *tx.mut_repo().view().heads(), @@ -239,17 +239,15 @@ fn test_rebase_descendants_backward() { let commit_c = graph_builder.commit_with_parents(&[&commit_b]); let commit_d = graph_builder.commit_with_parents(&[&commit_c]); - let mut rebaser = DescendantRebaser::new( - &settings, - tx.mut_repo(), - hashmap! { - commit_c.id().clone() => hashset!{commit_b.id().clone()} - }, - hashset! {}, - ); - let new_commit_d = assert_rebased(rebaser.rebase_next().unwrap(), &commit_d, &[&commit_b]); - assert!(rebaser.rebase_next().unwrap().is_none()); - assert_eq!(rebaser.rebased().len(), 1); + tx.mut_repo() + .record_rewritten_commit(commit_c.id().clone(), commit_b.id().clone()); + let rebase_map = tx + .mut_repo() + .rebase_descendants_return_map(&settings) + .unwrap(); + let new_commit_d = + assert_rebased_onto(tx.mut_repo(), &rebase_map, &commit_d, &[&commit_b.id()]); + assert_eq!(rebase_map.len(), 1); assert_eq!( *tx.mut_repo().view().heads(), @@ -282,19 +280,19 @@ fn test_rebase_descendants_chain_becomes_branchy() { let commit_e = graph_builder.commit_with_parents(&[&commit_a]); let commit_f = graph_builder.commit_with_parents(&[&commit_b]); - let mut rebaser = DescendantRebaser::new( - &settings, - tx.mut_repo(), - hashmap! { - commit_b.id().clone() => hashset!{commit_e.id().clone()}, - commit_c.id().clone() => hashset!{commit_f.id().clone()}, - }, - hashset! {}, - ); - let new_commit_f = assert_rebased(rebaser.rebase_next().unwrap(), &commit_f, &[&commit_e]); - let new_commit_d = assert_rebased(rebaser.rebase_next().unwrap(), &commit_d, &[&new_commit_f]); - assert!(rebaser.rebase_next().unwrap().is_none()); - assert_eq!(rebaser.rebased().len(), 2); + tx.mut_repo() + .record_rewritten_commit(commit_b.id().clone(), commit_e.id().clone()); + tx.mut_repo() + .record_rewritten_commit(commit_c.id().clone(), commit_f.id().clone()); + let rebase_map = tx + .mut_repo() + .rebase_descendants_return_map(&settings) + .unwrap(); + let new_commit_f = + assert_rebased_onto(tx.mut_repo(), &rebase_map, &commit_f, &[&commit_e.id()]); + let new_commit_d = + assert_rebased_onto(tx.mut_repo(), &rebase_map, &commit_d, &[&new_commit_f.id()]); + assert_eq!(rebase_map.len(), 2); assert_eq!( *tx.mut_repo().view().heads(), @@ -329,23 +327,23 @@ fn test_rebase_descendants_internal_merge() { let commit_e = graph_builder.commit_with_parents(&[&commit_c, &commit_d]); let commit_f = graph_builder.commit_with_parents(&[&commit_a]); - let mut rebaser = DescendantRebaser::new( - &settings, + tx.mut_repo() + .record_rewritten_commit(commit_b.id().clone(), commit_f.id().clone()); + let rebase_map = tx + .mut_repo() + .rebase_descendants_return_map(&settings) + .unwrap(); + let new_commit_c = + assert_rebased_onto(tx.mut_repo(), &rebase_map, &commit_c, &[&commit_f.id()]); + let new_commit_d = + assert_rebased_onto(tx.mut_repo(), &rebase_map, &commit_d, &[&commit_f.id()]); + let new_commit_e = assert_rebased_onto( tx.mut_repo(), - hashmap! { - commit_b.id().clone() => hashset!{commit_f.id().clone()} - }, - hashset! {}, - ); - let new_commit_c = assert_rebased(rebaser.rebase_next().unwrap(), &commit_c, &[&commit_f]); - let new_commit_d = assert_rebased(rebaser.rebase_next().unwrap(), &commit_d, &[&commit_f]); - let new_commit_e = assert_rebased( - rebaser.rebase_next().unwrap(), + &rebase_map, &commit_e, - &[&new_commit_c, &new_commit_d], + &[&new_commit_c.id(), &new_commit_d.id()], ); - assert!(rebaser.rebase_next().unwrap().is_none()); - assert_eq!(rebaser.rebased().len(), 3); + assert_eq!(rebase_map.len(), 3); assert_eq!( *tx.mut_repo().view().heads(), @@ -379,21 +377,19 @@ fn test_rebase_descendants_external_merge() { let commit_e = graph_builder.commit_with_parents(&[&commit_c, &commit_d]); let commit_f = graph_builder.commit_with_parents(&[&commit_a]); - let mut rebaser = DescendantRebaser::new( - &settings, + tx.mut_repo() + .record_rewritten_commit(commit_c.id().clone(), commit_f.id().clone()); + let rebase_map = tx + .mut_repo() + .rebase_descendants_return_map(&settings) + .unwrap(); + let new_commit_e = assert_rebased_onto( tx.mut_repo(), - hashmap! { - commit_c.id().clone() => hashset!{commit_f.id().clone()} - }, - hashset! {}, - ); - let new_commit_e = assert_rebased( - rebaser.rebase_next().unwrap(), + &rebase_map, &commit_e, - &[&commit_f, &commit_d], + &[&commit_f.id(), &commit_d.id()], ); - assert!(rebaser.rebase_next().unwrap().is_none()); - assert_eq!(rebaser.rebased().len(), 1); + assert_eq!(rebase_map.len(), 1); assert_eq!( *tx.mut_repo().view().heads(), @@ -425,17 +421,19 @@ fn test_rebase_descendants_abandon() { let commit_e = graph_builder.commit_with_parents(&[&commit_d]); let commit_f = graph_builder.commit_with_parents(&[&commit_e]); - let mut rebaser = DescendantRebaser::new( - &settings, - tx.mut_repo(), - hashmap! {}, - hashset! {commit_b.id().clone(), commit_e.id().clone()}, - ); - let new_commit_c = assert_rebased(rebaser.rebase_next().unwrap(), &commit_c, &[&commit_a]); - let new_commit_d = assert_rebased(rebaser.rebase_next().unwrap(), &commit_d, &[&commit_a]); - let new_commit_f = assert_rebased(rebaser.rebase_next().unwrap(), &commit_f, &[&new_commit_d]); - assert!(rebaser.rebase_next().unwrap().is_none()); - assert_eq!(rebaser.rebased().len(), 3); + tx.mut_repo().record_abandoned_commit(commit_b.id().clone()); + tx.mut_repo().record_abandoned_commit(commit_e.id().clone()); + let rebase_map = tx + .mut_repo() + .rebase_descendants_return_map(&settings) + .unwrap(); + let new_commit_c = + assert_rebased_onto(tx.mut_repo(), &rebase_map, &commit_c, &[&commit_a.id()]); + let new_commit_d = + assert_rebased_onto(tx.mut_repo(), &rebase_map, &commit_d, &[&commit_a.id()]); + let new_commit_f = + assert_rebased_onto(tx.mut_repo(), &rebase_map, &commit_f, &[&new_commit_d.id()]); + assert_eq!(rebase_map.len(), 3); assert_eq!( *tx.mut_repo().view().heads(), @@ -463,14 +461,13 @@ fn test_rebase_descendants_abandon_no_descendants() { let commit_b = graph_builder.commit_with_parents(&[&commit_a]); let commit_c = graph_builder.commit_with_parents(&[&commit_b]); - let mut rebaser = DescendantRebaser::new( - &settings, - tx.mut_repo(), - hashmap! {}, - hashset! {commit_b.id().clone(), commit_c.id().clone()}, - ); - assert!(rebaser.rebase_next().unwrap().is_none()); - assert_eq!(rebaser.rebased().len(), 0); + tx.mut_repo().record_abandoned_commit(commit_b.id().clone()); + tx.mut_repo().record_abandoned_commit(commit_c.id().clone()); + let rebase_map = tx + .mut_repo() + .rebase_descendants_return_map(&settings) + .unwrap(); + assert_eq!(rebase_map.len(), 0); assert_eq!( *tx.mut_repo().view().heads(), @@ -502,15 +499,16 @@ fn test_rebase_descendants_abandon_and_replace() { let commit_d = graph_builder.commit_with_parents(&[&commit_c]); let commit_e = graph_builder.commit_with_parents(&[&commit_a]); - let mut rebaser = DescendantRebaser::new( - &settings, - tx.mut_repo(), - hashmap! {commit_b.id().clone() => hashset!{commit_e.id().clone()}}, - hashset! {commit_c.id().clone()}, - ); - let new_commit_d = assert_rebased(rebaser.rebase_next().unwrap(), &commit_d, &[&commit_e]); - assert!(rebaser.rebase_next().unwrap().is_none()); - assert_eq!(rebaser.rebased().len(), 1); + tx.mut_repo() + .record_rewritten_commit(commit_b.id().clone(), commit_e.id().clone()); + tx.mut_repo().record_abandoned_commit(commit_c.id().clone()); + let rebase_map = tx + .mut_repo() + .rebase_descendants_return_map(&settings) + .unwrap(); + let new_commit_d = + assert_rebased_onto(tx.mut_repo(), &rebase_map, &commit_d, &[&commit_e.id()]); + assert_eq!(rebase_map.len(), 1); assert_eq!( *tx.mut_repo().view().heads(), @@ -518,6 +516,7 @@ fn test_rebase_descendants_abandon_and_replace() { ); } +// TODO(#2600): This behavior may need to change #[test] fn test_rebase_descendants_abandon_degenerate_merge() { let settings = testutils::user_settings(); @@ -539,15 +538,14 @@ fn test_rebase_descendants_abandon_degenerate_merge() { let commit_c = graph_builder.commit_with_parents(&[&commit_a]); let commit_d = graph_builder.commit_with_parents(&[&commit_b, &commit_c]); - let mut rebaser = DescendantRebaser::new( - &settings, - tx.mut_repo(), - hashmap! {}, - hashset! {commit_b.id().clone()}, - ); - let new_commit_d = assert_rebased(rebaser.rebase_next().unwrap(), &commit_d, &[&commit_c]); - assert!(rebaser.rebase_next().unwrap().is_none()); - assert_eq!(rebaser.rebased().len(), 1); + tx.mut_repo().record_abandoned_commit(commit_b.id().clone()); + let rebase_map = tx + .mut_repo() + .rebase_descendants_return_map(&settings) + .unwrap(); + let new_commit_d = + assert_rebased_onto(tx.mut_repo(), &rebase_map, &commit_d, &[&commit_c.id()]); + assert_eq!(rebase_map.len(), 1); assert_eq!( *tx.mut_repo().view().heads(), @@ -580,19 +578,18 @@ fn test_rebase_descendants_abandon_widen_merge() { let commit_e = graph_builder.commit_with_parents(&[&commit_b, &commit_c]); let commit_f = graph_builder.commit_with_parents(&[&commit_e, &commit_d]); - let mut rebaser = DescendantRebaser::new( - &settings, + tx.mut_repo().record_abandoned_commit(commit_e.id().clone()); + let rebase_map = tx + .mut_repo() + .rebase_descendants_return_map(&settings) + .unwrap(); + let new_commit_f = assert_rebased_onto( tx.mut_repo(), - hashmap! {}, - hashset! {commit_e.id().clone()}, - ); - let new_commit_f = assert_rebased( - rebaser.rebase_next().unwrap(), + &rebase_map, &commit_f, - &[&commit_b, &commit_c, &commit_d], + &[&commit_b.id(), &commit_c.id(), &commit_d.id()], ); - assert!(rebaser.rebase_next().unwrap().is_none()); - assert_eq!(rebaser.rebased().len(), 1); + assert_eq!(rebase_map.len(), 1); assert_eq!( *tx.mut_repo().view().heads(), @@ -623,19 +620,19 @@ fn test_rebase_descendants_multiple_sideways() { let commit_e = graph_builder.commit_with_parents(&[&commit_d]); let commit_f = graph_builder.commit_with_parents(&[&commit_a]); - let mut rebaser = DescendantRebaser::new( - &settings, - tx.mut_repo(), - hashmap! { - commit_b.id().clone() => hashset!{commit_f.id().clone()}, - commit_d.id().clone() => hashset!{commit_f.id().clone()}, - }, - hashset! {}, - ); - let new_commit_c = assert_rebased(rebaser.rebase_next().unwrap(), &commit_c, &[&commit_f]); - let new_commit_e = assert_rebased(rebaser.rebase_next().unwrap(), &commit_e, &[&commit_f]); - assert!(rebaser.rebase_next().unwrap().is_none()); - assert_eq!(rebaser.rebased().len(), 2); + tx.mut_repo() + .record_rewritten_commit(commit_b.id().clone(), commit_f.id().clone()); + tx.mut_repo() + .record_rewritten_commit(commit_d.id().clone(), commit_f.id().clone()); + let rebase_map = tx + .mut_repo() + .rebase_descendants_return_map(&settings) + .unwrap(); + let new_commit_c = + assert_rebased_onto(tx.mut_repo(), &rebase_map, &commit_c, &[&commit_f.id()]); + let new_commit_e = + assert_rebased_onto(tx.mut_repo(), &rebase_map, &commit_e, &[&commit_f.id()]); + assert_eq!(rebase_map.len(), 2); assert_eq!( *tx.mut_repo().view().heads(), @@ -668,18 +665,15 @@ fn test_rebase_descendants_multiple_swap() { let commit_d = graph_builder.commit_with_parents(&[&commit_a]); let _commit_e = graph_builder.commit_with_parents(&[&commit_d]); - let mut rebaser = DescendantRebaser::new( - &settings, - tx.mut_repo(), - hashmap! { - commit_b.id().clone() => hashset!{commit_d.id().clone()}, - commit_d.id().clone() => hashset!{commit_b.id().clone()}, - }, - hashset! {}, - ); - let _ = rebaser.rebase_next(); // Panics because of the cycle + tx.mut_repo() + .record_rewritten_commit(commit_b.id().clone(), commit_d.id().clone()); + tx.mut_repo() + .record_rewritten_commit(commit_d.id().clone(), commit_b.id().clone()); + let _ = tx.mut_repo().rebase_descendants_return_map(&settings); // Panics because of the cycle } +// Unlike `test_rebase_descendants_multiple_swap`, this does not currently +// panic, but it would probably be OK if it did. #[test] fn test_rebase_descendants_multiple_no_descendants() { let settings = testutils::user_settings(); @@ -697,17 +691,15 @@ fn test_rebase_descendants_multiple_no_descendants() { let commit_b = graph_builder.commit_with_parents(&[&commit_a]); let commit_c = graph_builder.commit_with_parents(&[&commit_a]); - let mut rebaser = DescendantRebaser::new( - &settings, - tx.mut_repo(), - hashmap! { - commit_b.id().clone() => hashset!{commit_c.id().clone()}, - commit_c.id().clone() => hashset!{commit_b.id().clone()}, - }, - hashset! {}, - ); - assert!(rebaser.rebase_next().unwrap().is_none()); - assert!(rebaser.rebased().is_empty()); + tx.mut_repo() + .record_rewritten_commit(commit_b.id().clone(), commit_c.id().clone()); + tx.mut_repo() + .record_rewritten_commit(commit_c.id().clone(), commit_b.id().clone()); + let rebase_map = tx + .mut_repo() + .rebase_descendants_return_map(&settings) + .unwrap(); + assert!(rebase_map.is_empty()); assert_eq!( *tx.mut_repo().view().heads(), @@ -758,20 +750,24 @@ fn test_rebase_descendants_divergent_rewrite() { let commit_d3 = graph_builder.commit_with_parents(&[&commit_a]); let commit_f2 = graph_builder.commit_with_parents(&[&commit_a]); - let mut rebaser = DescendantRebaser::new( - &settings, - tx.mut_repo(), - hashmap! { - commit_b.id().clone() => hashset!{commit_b2.id().clone()}, - commit_d.id().clone() => hashset!{commit_d2.id().clone(), commit_d3.id().clone()}, - commit_f.id().clone() => hashset!{commit_f2.id().clone()}, - }, - hashset! {}, - ); - let new_commit_c = assert_rebased(rebaser.rebase_next().unwrap(), &commit_c, &[&commit_b2]); - let new_commit_g = assert_rebased(rebaser.rebase_next().unwrap(), &commit_g, &[&commit_f2]); - assert!(rebaser.rebase_next().unwrap().is_none()); - assert_eq!(rebaser.rebased().len(), 2); + tx.mut_repo() + .record_rewritten_commit(commit_b.id().clone(), commit_b2.id().clone()); + // Commit D becomes divergent + tx.mut_repo() + .record_rewritten_commit(commit_d.id().clone(), commit_d2.id().clone()); + tx.mut_repo() + .record_rewritten_commit(commit_d.id().clone(), commit_d3.id().clone()); + tx.mut_repo() + .record_rewritten_commit(commit_f.id().clone(), commit_f2.id().clone()); + let rebase_map = tx + .mut_repo() + .rebase_descendants_return_map(&settings) + .unwrap(); + let new_commit_c = + assert_rebased_onto(tx.mut_repo(), &rebase_map, &commit_c, &[commit_b2.id()]); + let new_commit_g = + assert_rebased_onto(tx.mut_repo(), &rebase_map, &commit_g, &[&commit_f2.id()]); + assert_eq!(rebase_map.len(), 2); // Commit E is not rebased assert_eq!( *tx.mut_repo().view().heads(), @@ -814,10 +810,12 @@ fn test_rebase_descendants_repeated() { .set_description("b2") .write() .unwrap(); - let mut rebaser = tx.mut_repo().create_descendant_rebaser(&settings); - let commit_c2 = assert_rebased(rebaser.rebase_next().unwrap(), &commit_c, &[&commit_b2]); - assert!(rebaser.rebase_next().unwrap().is_none()); - assert_eq!(rebaser.rebased().len(), 1); + let rebase_map = tx + .mut_repo() + .rebase_descendants_return_map(&settings) + .unwrap(); + let commit_c2 = assert_rebased_onto(tx.mut_repo(), &rebase_map, &commit_c, &[commit_b2.id()]); + assert_eq!(rebase_map.len(), 1); assert_eq!( *tx.mut_repo().view().heads(), @@ -827,9 +825,11 @@ fn test_rebase_descendants_repeated() { ); // We made no more changes, so nothing should be rebased. - let mut rebaser = tx.mut_repo().create_descendant_rebaser(&settings); - assert!(rebaser.rebase_next().unwrap().is_none()); - assert_eq!(rebaser.rebased().len(), 0); + let rebase_map = tx + .mut_repo() + .rebase_descendants_return_map(&settings) + .unwrap(); + assert_eq!(rebase_map.len(), 0); // Now mark B3 as rewritten from B2 and rebase descendants again. let commit_b3 = tx @@ -838,10 +838,12 @@ fn test_rebase_descendants_repeated() { .set_description("b3") .write() .unwrap(); - let mut rebaser = tx.mut_repo().create_descendant_rebaser(&settings); - let commit_c3 = assert_rebased(rebaser.rebase_next().unwrap(), &commit_c2, &[&commit_b3]); - assert!(rebaser.rebase_next().unwrap().is_none()); - assert_eq!(rebaser.rebased().len(), 1); + let rebase_map = tx + .mut_repo() + .rebase_descendants_return_map(&settings) + .unwrap(); + let commit_c3 = assert_rebased_onto(tx.mut_repo(), &rebase_map, &commit_c2, &[commit_b3.id()]); + assert_eq!(rebase_map.len(), 1); assert_eq!( *tx.mut_repo().view().heads(), @@ -900,20 +902,16 @@ fn test_rebase_descendants_contents() { .write() .unwrap(); - let mut rebaser = DescendantRebaser::new( - &settings, - tx.mut_repo(), - hashmap! { - commit_b.id().clone() => hashset!{commit_d.id().clone()} - }, - hashset! {}, - ); - rebaser.rebase_all().unwrap(); - let rebased = rebaser.rebased(); - assert_eq!(rebased.len(), 1); + tx.mut_repo() + .record_rewritten_commit(commit_b.id().clone(), commit_d.id().clone()); + let rebase_map = tx + .mut_repo() + .rebase_descendants_return_map(&settings) + .unwrap(); + assert_eq!(rebase_map.len(), 1); let new_commit_c = repo .store() - .get_commit(rebased.get(commit_c.id()).unwrap()) + .get_commit(rebase_map.get(commit_c.id()).unwrap()) .unwrap(); let tree_b = commit_b.tree().unwrap(); diff --git a/lib/testutils/src/lib.rs b/lib/testutils/src/lib.rs index 7fd1f1fd0c..5de0c75836 100644 --- a/lib/testutils/src/lib.rs +++ b/lib/testutils/src/lib.rs @@ -12,6 +12,7 @@ // See the License for the specific language governing permissions and // limitations under the License. +use std::collections::HashMap; use std::env; use std::fs::{self, OpenOptions}; use std::io::{Read, Write}; @@ -20,8 +21,8 @@ use std::sync::{Arc, Once}; use itertools::Itertools; use jj_lib::backend::{ - self, Backend, BackendInitError, ChangeId, FileId, MergedTreeId, MillisSinceEpoch, ObjectId, - Signature, Timestamp, TreeValue, + self, Backend, BackendInitError, ChangeId, CommitId, FileId, MergedTreeId, MillisSinceEpoch, + ObjectId, Signature, Timestamp, TreeValue, }; use jj_lib::commit::Commit; use jj_lib::commit_builder::CommitBuilder; @@ -452,6 +453,32 @@ impl<'settings, 'repo> CommitGraphBuilder<'settings, 'repo> { } } +pub fn assert_rebased_onto( + repo: &impl Repo, + rebased: &HashMap, + expected_old_commit: &Commit, + expected_new_parent_ids: &[&CommitId], +) -> Commit { + let new_commit_id = rebased.get(expected_old_commit.id()).unwrap_or_else(|| { + panic!( + "Expected commit to have been rebased: {}", + expected_old_commit.id().hex() + ) + }); + let new_commit = repo.store().get_commit(new_commit_id).unwrap().clone(); + assert_eq!(new_commit.change_id(), expected_old_commit.change_id()); + assert_eq!( + new_commit.parent_ids().to_vec(), + expected_new_parent_ids + .iter() + .map(|x| (*x).clone()) + .collect_vec() + ); + new_commit +} + +// Short-term TODO: We will delete this function shortly; it will be replaced by +// `assert_rebased_onto` above pub fn assert_rebased( rebased: Option, expected_old_commit: &Commit, From d4a146a10a3b0e4c8ba6189cc8544068321b3395 Mon Sep 17 00:00:00 2001 From: Ilya Grigoriev Date: Sun, 24 Dec 2023 18:43:15 -0800 Subject: [PATCH 2/6] test_rewrite: test branches of descendants of divergent commits A TODO left over from a previous PR --- lib/tests/test_rewrite.rs | 21 ++++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-) diff --git a/lib/tests/test_rewrite.rs b/lib/tests/test_rewrite.rs index def386b81a..181392bfdd 100644 --- a/lib/tests/test_rewrite.rs +++ b/lib/tests/test_rewrite.rs @@ -1112,18 +1112,21 @@ fn test_rebase_descendants_update_branches_after_divergent_rewrite() { // Branch "main" points to commit B. B gets rewritten as B2, B3, B4. Branch main // should become a conflict pointing to all of them. // - // B4 main? - // | B3 main? + // C other + // C other | B4 main? + // | |/B3 main? // B main |/B2 main? // | => |/ // A A - // TODO(ilyagr): Check what happens if B had a descendant with a branch on it. let mut tx = repo.start_transaction(&settings); let mut graph_builder = CommitGraphBuilder::new(&settings, tx.mut_repo()); let commit_a = graph_builder.initial_commit(); let commit_b = graph_builder.commit_with_parents(&[&commit_a]); + let commit_c = graph_builder.commit_with_parents(&[&commit_b]); tx.mut_repo() .set_local_branch_target("main", RefTarget::normal(commit_b.id().clone())); + tx.mut_repo() + .set_local_branch_target("other", RefTarget::normal(commit_c.id().clone())); let repo = tx.commit("test"); let mut tx = repo.start_transaction(&settings); @@ -1148,14 +1151,14 @@ fn test_rebase_descendants_update_branches_after_divergent_rewrite() { .unwrap(); tx.mut_repo().rebase_descendants(&settings).unwrap(); - let target = tx.mut_repo().get_local_branch("main"); - assert!(target.has_conflict()); + let main_target = tx.mut_repo().get_local_branch("main"); + assert!(main_target.has_conflict()); assert_eq!( - target.removed_ids().counts(), + main_target.removed_ids().counts(), hashmap! { commit_b.id() => 2 }, ); assert_eq!( - target.added_ids().counts(), + main_target.added_ids().counts(), hashmap! { commit_b2.id() => 1, commit_b3.id() => 1, @@ -1163,12 +1166,16 @@ fn test_rebase_descendants_update_branches_after_divergent_rewrite() { }, ); + let other_target = tx.mut_repo().get_local_branch("other"); + assert_eq!(other_target.as_normal(), Some(commit_c.id())); + assert_eq!( *tx.mut_repo().view().heads(), hashset! { commit_b2.id().clone(), commit_b3.id().clone(), commit_b4.id().clone(), + commit_c.id().clone(), } ); } From b6058c5b9a6f6185fb06dba953cf86f30db149bd Mon Sep 17 00:00:00 2001 From: Ilya Grigoriev Date: Sun, 17 Dec 2023 21:47:01 -0800 Subject: [PATCH 3/6] lib `repo.rs` & `rewrite.rs`: Move clearing of rewritten/abandoned commits This commit is a little out of place in this sequence, but it seems to make more sense for MutRepo to own these maps. @yuja [pointed out] that any tests written using `create_descendant_rebaser` now need to do this cleanup, but there are no longer any such tests after the previous commits and a follow-up commit removes `create_descendant_rebaser` entirely. [pointed out]: https://github.com/martinvonz/jj/pull/2737#discussion_r1435754370 --- lib/src/repo.rs | 21 ++++++++++++--------- lib/src/rewrite.rs | 2 -- 2 files changed, 12 insertions(+), 11 deletions(-) diff --git a/lib/src/repo.rs b/lib/src/repo.rs index e7bf6e41b4..05050843b5 100644 --- a/lib/src/repo.rs +++ b/lib/src/repo.rs @@ -859,10 +859,6 @@ impl MutableRepo { .insert(new_id); } - pub fn clear_rewritten_commits(&mut self) { - self.rewritten_commits.clear(); - } - /// Record a commit as having been abandoned in this transaction. /// /// This record is used by `rebase_descendants` to know which commits have @@ -877,7 +873,8 @@ impl MutableRepo { self.abandoned_commits.insert(old_id); } - pub fn clear_abandoned_commits(&mut self) { + fn clear_descendant_rebaser_plans(&mut self) { + self.rewritten_commits.clear(); self.abandoned_commits.clear(); } @@ -900,6 +897,8 @@ impl MutableRepo { ) } + /// After the rebaser returned by this function is dropped, + /// self.clear_descendant_rebaser_plans() needs to be called. fn rebase_descendants_return_rebaser<'settings, 'repo>( &'repo mut self, settings: &'settings UserSettings, @@ -923,9 +922,11 @@ impl MutableRepo { settings: &UserSettings, options: RebaseOptions, ) -> Result { - Ok(self + let result = self .rebase_descendants_return_rebaser(settings, options)? - .map_or(0, |rebaser| rebaser.rebased().len())) + .map_or(0, |rebaser| rebaser.rebased().len()); + self.clear_descendant_rebaser_plans(); + Ok(result) } pub fn rebase_descendants(&mut self, settings: &UserSettings) -> Result { @@ -936,12 +937,14 @@ impl MutableRepo { &mut self, settings: &UserSettings, ) -> Result, TreeMergeError> { - Ok(self + let result = Ok(self // We do not set RebaseOptions here, since this function does not currently return // enough information to describe the results of a rebase if some commits got // abandoned .rebase_descendants_return_rebaser(settings, Default::default())? - .map_or(HashMap::new(), |rebaser| rebaser.rebased().clone())) + .map_or(HashMap::new(), |rebaser| rebaser.rebased().clone())); + self.clear_descendant_rebaser_plans(); + result } pub fn set_wc_commit( diff --git a/lib/src/rewrite.rs b/lib/src/rewrite.rs index f6d449b4ee..e179fb29db 100644 --- a/lib/src/rewrite.rs +++ b/lib/src/rewrite.rs @@ -600,8 +600,6 @@ impl<'settings, 'repo> DescendantRebaser<'settings, 'repo> { self.heads_to_remove.clear(); self.heads_to_add.clear(); self.mut_repo.set_view(view); - self.mut_repo.clear_rewritten_commits(); - self.mut_repo.clear_abandoned_commits(); Ok(None) } From e83da9201663ce9c2fad496ebfd9613c58351350 Mon Sep 17 00:00:00 2001 From: Ilya Grigoriev Date: Fri, 22 Dec 2023 16:07:26 -0800 Subject: [PATCH 4/6] test_rewrite.rs: stop using DescendantRebaser when testing EmptyBehavior This completes the process of removing DescendantRebaser-related APIs from tests. It requires creating some new test utils and a new `rebase_descendants_with_option_return_map`. --- lib/src/repo.rs | 33 +++++++++--- lib/tests/test_rewrite.rs | 103 +++++++++++++++----------------------- lib/testutils/src/lib.rs | 55 ++++++++++---------- 3 files changed, 96 insertions(+), 95 deletions(-) diff --git a/lib/src/repo.rs b/lib/src/repo.rs index 05050843b5..5894ece83d 100644 --- a/lib/src/repo.rs +++ b/lib/src/repo.rs @@ -929,24 +929,45 @@ impl MutableRepo { Ok(result) } - pub fn rebase_descendants(&mut self, settings: &UserSettings) -> Result { - self.rebase_descendants_with_options(settings, Default::default()) - } - - pub fn rebase_descendants_return_map( + /// This is similar to `rebase_descendants_return_map`, but the return value + /// needs more explaining. + /// + /// If the `options.empty` is the default, this function will only + /// rebase commits, and the return value is what you'd expect it to be. + /// + /// Otherwise, this function may rebase some commits and abandon others. The + /// behavior is such that only commits with a single parent will ever be + /// abandoned. In the returned map, an abandoned commit will look as a + /// key-value pair where the key is the abandoned commit and the value is + /// **its parent**. One can tell this case apart since the change ids of the + /// key and the value will not match. The parent will inherit the + /// descendants and the branches of the abandoned commit. + pub fn rebase_descendants_with_options_return_map( &mut self, settings: &UserSettings, + options: RebaseOptions, ) -> Result, TreeMergeError> { let result = Ok(self // We do not set RebaseOptions here, since this function does not currently return // enough information to describe the results of a rebase if some commits got // abandoned - .rebase_descendants_return_rebaser(settings, Default::default())? + .rebase_descendants_return_rebaser(settings, options)? .map_or(HashMap::new(), |rebaser| rebaser.rebased().clone())); self.clear_descendant_rebaser_plans(); result } + pub fn rebase_descendants(&mut self, settings: &UserSettings) -> Result { + self.rebase_descendants_with_options(settings, Default::default()) + } + + pub fn rebase_descendants_return_map( + &mut self, + settings: &UserSettings, + ) -> Result, TreeMergeError> { + self.rebase_descendants_with_options_return_map(settings, Default::default()) + } + pub fn set_wc_commit( &mut self, workspace_id: WorkspaceId, diff --git a/lib/tests/test_rewrite.rs b/lib/tests/test_rewrite.rs index 181392bfdd..67882a1ba2 100644 --- a/lib/tests/test_rewrite.rs +++ b/lib/tests/test_rewrite.rs @@ -19,14 +19,12 @@ use jj_lib::merged_tree::MergedTree; use jj_lib::op_store::{RefTarget, RemoteRef, RemoteRefState, WorkspaceId}; use jj_lib::repo::Repo; use jj_lib::repo_path::RepoPath; -use jj_lib::rewrite::{ - restore_tree, DescendantRebaser, EmptyBehaviour, RebaseOptions, RebasedDescendant, -}; +use jj_lib::rewrite::{restore_tree, EmptyBehaviour, RebaseOptions}; use maplit::{hashmap, hashset}; use test_case::test_case; use testutils::{ - assert_rebased, assert_rebased_onto, create_random_commit, create_tree, write_random_commit, - CommitGraphBuilder, TestRepo, + assert_abandoned_with_parent, assert_rebased_onto, create_random_commit, create_tree, + write_random_commit, CommitGraphBuilder, TestRepo, }; #[test] @@ -1474,30 +1472,10 @@ fn test_rebase_descendants_update_checkout_abandoned_merge() { assert_eq!(checkout.parent_ids(), vec![commit_b.id().clone()]); } -fn assert_rebase_skipped( - rebased: Option, - expected_old_commit: &Commit, - expected_new_commit: &Commit, -) -> Commit { - if let Some(RebasedDescendant { - old_commit, - new_commit, - }) = rebased - { - assert_eq!(old_commit, *expected_old_commit,); - assert_eq!(new_commit, *expected_new_commit); - // Since it was abandoned, the change ID should be different. - assert_ne!(old_commit.change_id(), new_commit.change_id()); - new_commit - } else { - panic!("expected rebased commit: {rebased:?}"); - } -} - #[test_case(EmptyBehaviour::Keep; "keep all commits")] #[test_case(EmptyBehaviour::AbandonNewlyEmpty; "abandon newly empty commits")] #[test_case(EmptyBehaviour::AbandonAllEmpty ; "abandon all empty commits")] -fn test_empty_commit_option(empty: EmptyBehaviour) { +fn test_empty_commit_option(empty_behavior: EmptyBehaviour) { let settings = testutils::user_settings(); let test_repo = TestRepo::init(); let repo = &test_repo.repo; @@ -1557,74 +1535,75 @@ fn test_empty_commit_option(empty: EmptyBehaviour) { let commit_h = create_commit(&[&commit_g], &tree_g); let commit_bd = create_commit(&[&commit_a], &tree_d); - let mut rebaser = DescendantRebaser::new( - &settings, - tx.mut_repo(), - hashmap! { - commit_b.id().clone() => hashset!{commit_bd.id().clone()} - }, - hashset! {}, - ); - *rebaser.mut_options() = RebaseOptions { - empty: empty.clone(), - }; + tx.mut_repo() + .record_rewritten_commit(commit_b.id().clone(), commit_bd.id().clone()); + let rebase_map = tx + .mut_repo() + .rebase_descendants_with_options_return_map( + &settings, + RebaseOptions { + empty: empty_behavior.clone(), + }, + ) + .unwrap(); - let new_head = match empty { + let new_head = match empty_behavior { EmptyBehaviour::Keep => { // The commit C isn't empty. let new_commit_c = - assert_rebased(rebaser.rebase_next().unwrap(), &commit_c, &[&commit_bd]); + assert_rebased_onto(tx.mut_repo(), &rebase_map, &commit_c, &[commit_bd.id()]); let new_commit_d = - assert_rebased(rebaser.rebase_next().unwrap(), &commit_d, &[&commit_bd]); + assert_rebased_onto(tx.mut_repo(), &rebase_map, &commit_d, &[commit_bd.id()]); let new_commit_e = - assert_rebased(rebaser.rebase_next().unwrap(), &commit_e, &[&commit_bd]); - let new_commit_f = assert_rebased( - rebaser.rebase_next().unwrap(), + assert_rebased_onto(tx.mut_repo(), &rebase_map, &commit_e, &[commit_bd.id()]); + let new_commit_f = assert_rebased_onto( + tx.mut_repo(), + &rebase_map, &commit_f, - &[&new_commit_c, &new_commit_d, &new_commit_e], + &[&new_commit_c.id(), &new_commit_d.id(), &new_commit_e.id()], ); let new_commit_g = - assert_rebased(rebaser.rebase_next().unwrap(), &commit_g, &[&new_commit_f]); - assert_rebased(rebaser.rebase_next().unwrap(), &commit_h, &[&new_commit_g]) + assert_rebased_onto(tx.mut_repo(), &rebase_map, &commit_g, &[new_commit_f.id()]); + assert_rebased_onto(tx.mut_repo(), &rebase_map, &commit_h, &[new_commit_g.id()]) } EmptyBehaviour::AbandonAllEmpty => { // The commit C isn't empty. let new_commit_c = - assert_rebased(rebaser.rebase_next().unwrap(), &commit_c, &[&commit_bd]); + assert_rebased_onto(tx.mut_repo(), &rebase_map, &commit_c, &[commit_bd.id()]); // D and E are empty, and F is a clean merge with only one child. Thus, F is // also considered empty. - assert_rebase_skipped(rebaser.rebase_next().unwrap(), &commit_d, &commit_bd); - assert_rebase_skipped(rebaser.rebase_next().unwrap(), &commit_e, &commit_bd); - assert_rebase_skipped(rebaser.rebase_next().unwrap(), &commit_f, &new_commit_c); + assert_abandoned_with_parent(tx.mut_repo(), &rebase_map, &commit_d, commit_bd.id()); + assert_abandoned_with_parent(tx.mut_repo(), &rebase_map, &commit_e, commit_bd.id()); + assert_abandoned_with_parent(tx.mut_repo(), &rebase_map, &commit_f, new_commit_c.id()); let new_commit_g = - assert_rebased(rebaser.rebase_next().unwrap(), &commit_g, &[&new_commit_c]); - assert_rebase_skipped(rebaser.rebase_next().unwrap(), &commit_h, &new_commit_g) + assert_rebased_onto(tx.mut_repo(), &rebase_map, &commit_g, &[new_commit_c.id()]); + assert_abandoned_with_parent(tx.mut_repo(), &rebase_map, &commit_h, new_commit_g.id()) } EmptyBehaviour::AbandonNewlyEmpty => { // The commit C isn't empty. let new_commit_c = - assert_rebased(rebaser.rebase_next().unwrap(), &commit_c, &[&commit_bd]); + assert_rebased_onto(tx.mut_repo(), &rebase_map, &commit_c, &[commit_bd.id()]); // The changes in D are included in BD, so D is newly empty. - assert_rebase_skipped(rebaser.rebase_next().unwrap(), &commit_d, &commit_bd); + assert_abandoned_with_parent(tx.mut_repo(), &rebase_map, &commit_d, commit_bd.id()); // E was already empty, so F is a merge commit with C and E as parents. // Although it's empty, we still keep it because we don't want to drop merge // commits. let new_commit_e = - assert_rebased(rebaser.rebase_next().unwrap(), &commit_e, &[&commit_bd]); - let new_commit_f = assert_rebased( - rebaser.rebase_next().unwrap(), + assert_rebased_onto(tx.mut_repo(), &rebase_map, &commit_e, &[commit_bd.id()]); + let new_commit_f = assert_rebased_onto( + tx.mut_repo(), + &rebase_map, &commit_f, - &[&new_commit_c, &new_commit_e], + &[&new_commit_c.id(), &new_commit_e.id()], ); let new_commit_g = - assert_rebased(rebaser.rebase_next().unwrap(), &commit_g, &[&new_commit_f]); - assert_rebased(rebaser.rebase_next().unwrap(), &commit_h, &[&new_commit_g]) + assert_rebased_onto(tx.mut_repo(), &rebase_map, &commit_g, &[&new_commit_f.id()]); + assert_rebased_onto(tx.mut_repo(), &rebase_map, &commit_h, &[&new_commit_g.id()]) } }; - assert!(rebaser.rebase_next().unwrap().is_none()); - assert_eq!(rebaser.rebased().len(), 6); + assert_eq!(rebase_map.len(), 6); assert_eq!( *tx.mut_repo().view().heads(), diff --git a/lib/testutils/src/lib.rs b/lib/testutils/src/lib.rs index 5de0c75836..e8d998401f 100644 --- a/lib/testutils/src/lib.rs +++ b/lib/testutils/src/lib.rs @@ -31,7 +31,6 @@ use jj_lib::local_backend::LocalBackend; use jj_lib::merged_tree::MergedTree; use jj_lib::repo::{MutableRepo, ReadonlyRepo, Repo, RepoLoader, StoreFactories}; use jj_lib::repo_path::{RepoPath, RepoPathBuf}; -use jj_lib::rewrite::RebasedDescendant; use jj_lib::settings::UserSettings; use jj_lib::signing::Signer; use jj_lib::store::Store; @@ -453,11 +452,10 @@ impl<'settings, 'repo> CommitGraphBuilder<'settings, 'repo> { } } -pub fn assert_rebased_onto( +fn assert_in_rebased_map( repo: &impl Repo, rebased: &HashMap, expected_old_commit: &Commit, - expected_new_parent_ids: &[&CommitId], ) -> Commit { let new_commit_id = rebased.get(expected_old_commit.id()).unwrap_or_else(|| { panic!( @@ -466,7 +464,16 @@ pub fn assert_rebased_onto( ) }); let new_commit = repo.store().get_commit(new_commit_id).unwrap().clone(); - assert_eq!(new_commit.change_id(), expected_old_commit.change_id()); + new_commit +} + +pub fn assert_rebased_onto( + repo: &impl Repo, + rebased: &HashMap, + expected_old_commit: &Commit, + expected_new_parent_ids: &[&CommitId], +) -> Commit { + let new_commit = assert_in_rebased_map(repo, rebased, expected_old_commit); assert_eq!( new_commit.parent_ids().to_vec(), expected_new_parent_ids @@ -474,32 +481,26 @@ pub fn assert_rebased_onto( .map(|x| (*x).clone()) .collect_vec() ); + assert_eq!(new_commit.change_id(), expected_old_commit.change_id()); new_commit } -// Short-term TODO: We will delete this function shortly; it will be replaced by -// `assert_rebased_onto` above -pub fn assert_rebased( - rebased: Option, +/// If `expected_old_commit` was abandoned, the `rebased` map indicates the +/// commit the children of `expected_old_commit` should be rebased to, which +/// would have a different change id. This happens when the EmptyBehavior in +/// RebaseOptions is not the default; because of the details of the +/// implementation this returned parent commit is always singular. +pub fn assert_abandoned_with_parent( + repo: &impl Repo, + rebased: &HashMap, expected_old_commit: &Commit, - expected_new_parents: &[&Commit], + expected_new_parent_id: &CommitId, ) -> Commit { - if let Some(RebasedDescendant { - old_commit, - new_commit, - }) = rebased - { - assert_eq!(old_commit, *expected_old_commit); - assert_eq!(new_commit.change_id(), expected_old_commit.change_id()); - assert_eq!( - new_commit.parent_ids(), - expected_new_parents - .iter() - .map(|commit| commit.id().clone()) - .collect_vec() - ); - new_commit - } else { - panic!("expected rebased commit: {rebased:?}"); - } + let new_parent_commit = assert_in_rebased_map(repo, rebased, expected_old_commit); + assert_eq!(new_parent_commit.id(), expected_new_parent_id); + assert_ne!( + new_parent_commit.change_id(), + expected_old_commit.change_id() + ); + new_parent_commit } From 3c2cad5486277853905d38b3d5528b67c8f97c24 Mon Sep 17 00:00:00 2001 From: Ilya Grigoriev Date: Fri, 22 Dec 2023 17:35:56 -0800 Subject: [PATCH 5/6] lib: make `DescendantRebaser`-related APIs private. Finally, there are no test uses of these APIs. `DescendantRebaser` is made `pub(crate)`, since it is used by `MutRepo`. Other functions are made private. --- lib/src/repo.rs | 2 +- lib/src/rewrite.rs | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/src/repo.rs b/lib/src/repo.rs index 5894ece83d..b1f09022d8 100644 --- a/lib/src/repo.rs +++ b/lib/src/repo.rs @@ -885,7 +885,7 @@ impl MutableRepo { /// Creates a `DescendantRebaser` to rebase descendants of the recorded /// rewritten and abandoned commits. // TODO(ilyagr): Inline this. It's only used in tests. - pub fn create_descendant_rebaser<'settings, 'repo>( + fn create_descendant_rebaser<'settings, 'repo>( &'repo mut self, settings: &'settings UserSettings, ) -> DescendantRebaser<'settings, 'repo> { diff --git a/lib/src/rewrite.rs b/lib/src/rewrite.rs index e179fb29db..d2bccc89be 100644 --- a/lib/src/rewrite.rs +++ b/lib/src/rewrite.rs @@ -242,7 +242,7 @@ pub struct RebaseOptions { } /// Rebases descendants of a commit onto a new commit (or several). -pub struct DescendantRebaser<'settings, 'repo> { +pub(crate) struct DescendantRebaser<'settings, 'repo> { settings: &'settings UserSettings, mut_repo: &'repo mut MutableRepo, // The commit identified by the key has been replaced by all the ones in the value, typically @@ -521,7 +521,7 @@ impl<'settings, 'repo> DescendantRebaser<'settings, 'repo> { // TODO: Perhaps change the interface since it's not just about rebasing // commits. - pub fn rebase_next(&mut self) -> Result, TreeMergeError> { + fn rebase_next(&mut self) -> Result, TreeMergeError> { while let Some(old_commit) = self.to_visit.pop() { let old_commit_id = old_commit.id().clone(); if let Some(new_parent_ids) = self.parent_mapping.get(&old_commit_id).cloned() { From 2624f18ea01a7ab23c4fa6d12d47a57b811c9300 Mon Sep 17 00:00:00 2001 From: Ilya Grigoriev Date: Fri, 22 Dec 2023 17:39:21 -0800 Subject: [PATCH 6/6] lib: mild refactoring made possible by previous commit Inline `create_descendant_commits`, move some functionality of `DescendantRebaser::rebase_next` to `rebase_all`, a seemingly more logical location. --- lib/src/repo.rs | 22 ++++++---------------- lib/src/rewrite.rs | 14 ++++++-------- 2 files changed, 12 insertions(+), 24 deletions(-) diff --git a/lib/src/repo.rs b/lib/src/repo.rs index b1f09022d8..a6a0aba055 100644 --- a/lib/src/repo.rs +++ b/lib/src/repo.rs @@ -882,21 +882,6 @@ impl MutableRepo { !(self.rewritten_commits.is_empty() && self.abandoned_commits.is_empty()) } - /// Creates a `DescendantRebaser` to rebase descendants of the recorded - /// rewritten and abandoned commits. - // TODO(ilyagr): Inline this. It's only used in tests. - fn create_descendant_rebaser<'settings, 'repo>( - &'repo mut self, - settings: &'settings UserSettings, - ) -> DescendantRebaser<'settings, 'repo> { - DescendantRebaser::new( - settings, - self, - self.rewritten_commits.clone(), - self.abandoned_commits.clone(), - ) - } - /// After the rebaser returned by this function is dropped, /// self.clear_descendant_rebaser_plans() needs to be called. fn rebase_descendants_return_rebaser<'settings, 'repo>( @@ -908,7 +893,12 @@ impl MutableRepo { // Optimization return Ok(None); } - let mut rebaser = self.create_descendant_rebaser(settings); + let mut rebaser = DescendantRebaser::new( + settings, + self, + self.rewritten_commits.clone(), + self.abandoned_commits.clone(), + ); *rebaser.mut_options() = options; rebaser.rebase_all()?; Ok(Some(rebaser)) diff --git a/lib/src/rewrite.rs b/lib/src/rewrite.rs index d2bccc89be..64c728470f 100644 --- a/lib/src/rewrite.rs +++ b/lib/src/rewrite.rs @@ -587,9 +587,12 @@ impl<'settings, 'repo> DescendantRebaser<'settings, 'repo> { new_commit, })); } - // TODO: As the TODO above says, we should probably change the API. Even if we - // don't, we should at least make this code not do any work if you call - // rebase_next() after we've returned None. + Ok(None) + } + + pub fn rebase_all(&mut self) -> Result<(), TreeMergeError> { + while self.rebase_next()?.is_some() {} + // TODO: As the TODO above says, we should probably change the API. let mut view = self.mut_repo.view().store_view().clone(); for commit_id in &self.heads_to_remove { view.head_ids.remove(commit_id); @@ -600,11 +603,6 @@ impl<'settings, 'repo> DescendantRebaser<'settings, 'repo> { self.heads_to_remove.clear(); self.heads_to_add.clear(); self.mut_repo.set_view(view); - Ok(None) - } - - pub fn rebase_all(&mut self) -> Result<(), TreeMergeError> { - while self.rebase_next()?.is_some() {} Ok(()) } }