Skip to content

Commit

Permalink
test_rewrite.rs: stop using DescendantRebaser when testing EmptyBehavior
Browse files Browse the repository at this point in the history 
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`.
  • Loading branch information
@ilyagr
ilyagr committed Jan 2, 2024
1 parent 7cef879 commit 45cd0bf
Show file tree
Hide file tree
Showing 3 changed files with 96 additions and 95 deletions.
33 changes: 27 additions & 6 deletions lib/src/repo.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -929,24 +929,45 @@ impl MutableRepo {
Ok(result)
}

pub fn rebase_descendants(&mut self, settings: &UserSettings) -> Result<usize, TreeMergeError> {
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<HashMap<CommitId, CommitId>, 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<usize, TreeMergeError> {
self.rebase_descendants_with_options(settings, Default::default())
}

pub fn rebase_descendants_return_map(
&mut self,
settings: &UserSettings,
) -> Result<HashMap<CommitId, CommitId>, TreeMergeError> {
self.rebase_descendants_with_options_return_map(settings, Default::default())
}

pub fn set_wc_commit(
&mut self,
workspace_id: WorkspaceId,
Expand Down
103 changes: 41 additions & 62 deletions lib/tests/test_rewrite.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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]
Expand Down Expand Up @@ -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<RebasedDescendant>,
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;
Expand Down Expand Up @@ -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(),
Expand Down
55 changes: 28 additions & 27 deletions lib/testutils/src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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<CommitId, CommitId>,
expected_old_commit: &Commit,
expected_new_parent_ids: &[&CommitId],
) -> Commit {
let new_commit_id = rebased.get(expected_old_commit.id()).unwrap_or_else(|| {
panic!(
Expand All @@ -466,40 +464,43 @@ 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<CommitId, CommitId>,
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
.iter()
.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<RebasedDescendant>,
/// 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<CommitId, CommitId>,
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
}

0 comments on commit 45cd0bf

Please sign in to comment.