From 79b5b8d681573847e715c30cdbed1dd7635ec904 Mon Sep 17 00:00:00 2001 From: Martin von Zweigbergk Date: Thu, 13 May 2021 22:24:04 -0700 Subject: [PATCH] evolution: rewrite divergence resolution as iterator This commit rewites the divergence-resolution part of `evolve()` as an iterator (though not implementing the `Iterator` trait). Iterators are just much easier to work with: they can easily be stopped, and errors are easy to propagate. This patch therefore lets us propagate errors from writing to stdout (typically pipe errors). --- lib/src/evolution.rs | 78 ++++++++++++++++------------------ lib/tests/test_evolution.rs | 65 ++++++++++------------------ src/commands.rs | 85 ++++++++++++++++--------------------- 3 files changed, 95 insertions(+), 133 deletions(-) diff --git a/lib/src/evolution.rs b/lib/src/evolution.rs index 23e79ba67..cf585cf39 100644 --- a/lib/src/evolution.rs +++ b/lib/src/evolution.rs @@ -375,18 +375,6 @@ pub struct ReadonlyEvolution { pub trait EvolveListener { fn orphan_evolved(&mut self, mut_repo: &mut MutableRepo, orphan: &Commit, new_commit: &Commit); fn orphan_target_ambiguous(&mut self, mut_repo: &mut MutableRepo, orphan: &Commit); - fn divergent_resolved( - &mut self, - mut_repo: &mut MutableRepo, - divergents: &[Commit], - resolved: &Commit, - ); - fn divergent_no_common_predecessor( - &mut self, - mut_repo: &mut MutableRepo, - commit1: &Commit, - commit2: &Commit, - ); } impl ReadonlyEvolution { @@ -463,7 +451,42 @@ impl MutableEvolution { } } -enum DivergenceResolution { +pub struct DivergenceResolver<'settings> { + user_settings: &'settings UserSettings, + remaining_changes: Vec>, +} + +impl<'settings> DivergenceResolver<'settings> { + pub fn new(user_settings: &'settings UserSettings, mut_repo: &MutableRepo) -> Self { + // TODO: Put them in some defined order + let divergent_changes: Vec<_> = mut_repo + .evolution() + .state + .non_obsoletes_by_changeid + .values() + .filter(|non_obsoletes| non_obsoletes.len() > 1) + .cloned() + .collect(); + DivergenceResolver { + user_settings, + remaining_changes: divergent_changes, + } + } + + pub fn resolve_next(&mut self, mut_repo: &mut MutableRepo) -> Option { + self.remaining_changes.pop().map(|commit_ids| { + let store = mut_repo.store().clone(); + let commits = commit_ids + .iter() + .map(|id| store.get_commit(&id).unwrap()) + .collect(); + evolve_divergent_change(self.user_settings, &store, mut_repo, &commits) + }) + } +} + +#[derive(PartialEq, Eq, Clone, Hash, Debug)] +pub enum DivergenceResolution { Resolved { divergents: Vec, resolved: Commit, @@ -479,34 +502,6 @@ pub fn evolve( mut_repo: &mut MutableRepo, listener: &mut dyn EvolveListener, ) { - let store = mut_repo.store().clone(); - - // Resolving divergence can creates new orphans but not vice versa, so resolve - // divergence first. - let divergent_changes: Vec<_> = mut_repo - .evolution() - .state - .non_obsoletes_by_changeid - .values() - .filter(|non_obsoletes| non_obsoletes.len() > 1) - .cloned() - .collect(); - for commit_ids in divergent_changes { - let commits: HashSet = commit_ids - .iter() - .map(|id| store.get_commit(&id).unwrap()) - .collect(); - match evolve_divergent_change(user_settings, &store, mut_repo, &commits) { - DivergenceResolution::Resolved { - divergents, - resolved, - } => listener.divergent_resolved(mut_repo, &divergents, &resolved), - DivergenceResolution::NoCommonPredecessor { .. } => {} - } - } - - // Don't reuse the state from above, since the divergence-resolution may have - // created new orphans, or resolved existing orphans. let orphans_topo_order: Vec<_> = mut_repo .index() .topo_order(mut_repo.evolution().state.orphan_commits.iter()) @@ -514,6 +509,7 @@ pub fn evolve( .map(|entry| entry.position()) .collect(); + let store = mut_repo.store().clone(); for orphan_pos in orphans_topo_order { let orphan_entry = mut_repo.index().entry_by_pos(orphan_pos); let mut new_parents = vec![]; diff --git a/lib/tests/test_evolution.rs b/lib/tests/test_evolution.rs index 075b4d69b..748e57f2f 100644 --- a/lib/tests/test_evolution.rs +++ b/lib/tests/test_evolution.rs @@ -12,9 +12,11 @@ // See the License for the specific language governing permissions and // limitations under the License. +#![feature(assert_matches)] + use jujube_lib::commit::Commit; use jujube_lib::commit_builder::CommitBuilder; -use jujube_lib::evolution::{evolve, EvolveListener}; +use jujube_lib::evolution::{evolve, DivergenceResolution, DivergenceResolver, EvolveListener}; use jujube_lib::repo::{MutableRepo, ReadonlyRepo}; use jujube_lib::repo_path::FileRepoPath; use jujube_lib::settings::UserSettings; @@ -506,14 +508,12 @@ fn test_new_parent_split_forked_pruned(use_git: bool) { struct RecordingEvolveListener { evolved_orphans: Vec<(Commit, Commit)>, - evolved_divergents: Vec<(Vec, Commit)>, } impl Default for RecordingEvolveListener { fn default() -> Self { RecordingEvolveListener { evolved_orphans: Default::default(), - evolved_divergents: Default::default(), } } } @@ -533,26 +533,6 @@ impl EvolveListener for RecordingEvolveListener { // TODO: Record this too and add tests panic!("unexpected call to orphan_target_ambiguous"); } - - fn divergent_resolved( - &mut self, - _mut_repo: &mut MutableRepo, - sources: &[Commit], - resolved: &Commit, - ) { - self.evolved_divergents - .push((sources.to_vec(), resolved.clone())); - } - - fn divergent_no_common_predecessor( - &mut self, - _mut_repo: &mut MutableRepo, - _commit1: &Commit, - _commit2: &Commit, - ) { - // TODO: Record this too and add tests - panic!("unexpected call to divergent_no_common_predecessor"); - } } #[test_case(false ; "local store")] @@ -574,7 +554,6 @@ fn test_evolve_orphan(use_git: bool) { let mut listener = RecordingEvolveListener::default(); evolve(&settings, mut_repo, &mut listener); - assert_eq!(listener.evolved_divergents.len(), 0); assert_eq!(listener.evolved_orphans.len(), 2); assert_eq!(&listener.evolved_orphans[0].0, &child); assert_eq!(&listener.evolved_orphans[0].1.parents(), &vec![rewritten]); @@ -609,7 +588,6 @@ fn test_evolve_pruned_orphan(use_git: bool) { let mut listener = RecordingEvolveListener::default(); evolve(&settings, mut_repo, &mut listener); - assert_eq!(listener.evolved_divergents.len(), 0); assert_eq!(listener.evolved_orphans.len(), 1); assert_eq!(listener.evolved_orphans[0].0.id(), child.id()); @@ -636,7 +614,6 @@ fn test_evolve_multiple_orphans(use_git: bool) { let mut listener = RecordingEvolveListener::default(); evolve(&settings, mut_repo, &mut listener); - assert_eq!(listener.evolved_divergents.len(), 0); assert_eq!(listener.evolved_orphans.len(), 3); assert_eq!(&listener.evolved_orphans[0].0, &child); assert_eq!(&listener.evolved_orphans[0].1.parents(), &vec![rewritten]); @@ -721,24 +698,26 @@ fn test_evolve_divergent(use_git: bool) { .set_committer(later_time) .write_to_repo(mut_repo); - let mut listener = RecordingEvolveListener::default(); - evolve(&settings, mut_repo, &mut listener); - assert_eq!(listener.evolved_orphans.len(), 0); - assert_eq!(listener.evolved_divergents.len(), 1); - assert_eq!( - listener.evolved_divergents[0].0, - &[commit6.clone(), commit4.clone()] - ); - let resolved = listener.evolved_divergents[0].1.clone(); - assert_eq!(resolved.predecessors(), &[commit6, commit4]); + let mut resolver = DivergenceResolver::new(&settings, mut_repo); + let resolution = resolver.resolve_next(mut_repo); + assert_eq!(resolver.resolve_next(mut_repo), None); + assert_matches!(resolution, Some(DivergenceResolution::Resolved { .. })); + if let Some(DivergenceResolution::Resolved { + divergents, + resolved, + }) = resolution + { + assert_eq!(divergents, vec![commit6.clone(), commit4.clone()]); + assert_eq!(resolved.predecessors(), &[commit6, commit4]); - let tree = resolved.tree(); - let entries: Vec<_> = tree.entries().collect(); - assert_eq!(entries.len(), 4); - assert_eq!(tree.value("A").unwrap(), tree5.value("A").unwrap()); - assert_eq!(tree.value("X").unwrap(), tree2.value("X").unwrap()); - assert_eq!(tree.value("Y").unwrap(), tree4.value("Y").unwrap()); - assert_eq!(tree.value("Z").unwrap(), tree6.value("Z").unwrap()); + let tree = resolved.tree(); + let entries: Vec<_> = tree.entries().collect(); + assert_eq!(entries.len(), 4); + assert_eq!(tree.value("A").unwrap(), tree5.value("A").unwrap()); + assert_eq!(tree.value("X").unwrap(), tree2.value("X").unwrap()); + assert_eq!(tree.value("Y").unwrap(), tree4.value("Y").unwrap()); + assert_eq!(tree.value("Z").unwrap(), tree6.value("Z").unwrap()); + } tx.discard(); } diff --git a/src/commands.rs b/src/commands.rs index 71c308d0d..bd9c1a524 100644 --- a/src/commands.rs +++ b/src/commands.rs @@ -31,7 +31,7 @@ use criterion::Criterion; use jujube_lib::commit::Commit; use jujube_lib::commit_builder::CommitBuilder; use jujube_lib::dag_walk::topo_order_reverse; -use jujube_lib::evolution::{evolve, EvolveListener}; +use jujube_lib::evolution::{evolve, DivergenceResolution, DivergenceResolver, EvolveListener}; use jujube_lib::files::DiffLine; use jujube_lib::git::GitFetchError; use jujube_lib::index::HexPrefix; @@ -1766,6 +1766,41 @@ fn cmd_evolve<'s>( ) -> Result<(), CommandError> { let mut repo_command = command.repo_helper(ui)?; + // TODO: This clone is unnecessary. Maybe ui.write() etc should not require a + // mutable borrow? But the mutable borrow might be useful for making sure we + // have only one Ui instance we write to across threads? + let user_settings = ui.settings().clone(); + let mut tx = repo_command.start_transaction("evolve"); + let mut_repo = tx.mut_repo(); + let mut divergence_resolver = DivergenceResolver::new(&user_settings, mut_repo); + while let Some(resolution) = divergence_resolver.resolve_next(mut_repo) { + match resolution { + DivergenceResolution::Resolved { + divergents, + resolved, + } => { + ui.write("Resolving divergent commits:\n").unwrap(); + for source in divergents { + ui.write(" ")?; + ui.write_commit_summary(mut_repo.as_repo_ref(), &source)?; + ui.write("\n")?; + } + ui.write("Resolved as: ")?; + ui.write_commit_summary(mut_repo.as_repo_ref(), &resolved)?; + ui.write("\n")?; + } + DivergenceResolution::NoCommonPredecessor { commit1, commit2 } => { + ui.write("Skipping divergent commits with no common predecessor:\n")?; + ui.write(" ")?; + ui.write_commit_summary(mut_repo.as_repo_ref(), &commit1)?; + ui.write("\n")?; + ui.write(" ")?; + ui.write_commit_summary(mut_repo.as_repo_ref(), &commit2)?; + ui.write("\n")?; + } + } + } + struct Listener<'a, 's> { ui: &'a mut Ui<'s>, } @@ -1801,56 +1836,8 @@ fn cmd_evolve<'s>( .unwrap(); self.ui.write("\n").unwrap(); } - - fn divergent_resolved( - &mut self, - mut_repo: &mut MutableRepo, - sources: &[Commit], - resolved: &Commit, - ) { - self.ui.write("Resolving divergent commits:\n").unwrap(); - for source in sources { - self.ui.write(" ").unwrap(); - self.ui - .write_commit_summary(mut_repo.as_repo_ref(), &source) - .unwrap(); - self.ui.write("\n").unwrap(); - } - self.ui.write("Resolved as: ").unwrap(); - self.ui - .write_commit_summary(mut_repo.as_repo_ref(), &resolved) - .unwrap(); - self.ui.write("\n").unwrap(); - } - - fn divergent_no_common_predecessor( - &mut self, - mut_repo: &mut MutableRepo, - commit1: &Commit, - commit2: &Commit, - ) { - self.ui - .write("Skipping divergent commits with no common predecessor:\n") - .unwrap(); - self.ui.write(" ").unwrap(); - self.ui - .write_commit_summary(mut_repo.as_repo_ref(), &commit1) - .unwrap(); - self.ui.write("\n").unwrap(); - self.ui.write(" ").unwrap(); - self.ui - .write_commit_summary(mut_repo.as_repo_ref(), &commit2) - .unwrap(); - self.ui.write("\n").unwrap(); - } } - - // TODO: This clone is unnecessary. Maybe ui.write() etc should not require a - // mutable borrow? But the mutable borrow might be useful for making sure we - // have only one Ui instance we write to across threads? - let user_settings = ui.settings().clone(); let mut listener = Listener { ui }; - let mut tx = repo_command.start_transaction("evolve"); evolve(&user_settings, tx.mut_repo(), &mut listener); repo_command.finish_transaction(ui, tx)?;