From 5c4567ac24be0f4f09b4b789d3a03f89e8f71f37 Mon Sep 17 00:00:00 2001 From: Ilya Grigoriev Date: Fri, 22 Dec 2023 16:07:26 -0800 Subject: [PATCH] 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 419e094bd7..260b6c7fbb 100644 --- a/lib/src/repo.rs +++ b/lib/src/repo.rs @@ -925,24 +925,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 01532f6809..f01836e29f 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; @@ -443,11 +442,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!( @@ -456,7 +454,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 @@ -464,32 +471,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 }