diff --git a/cli/src/commands/new.rs b/cli/src/commands/new.rs index b7d3ec8911..8b30b07604 100644 --- a/cli/src/commands/new.rs +++ b/cli/src/commands/new.rs @@ -140,7 +140,7 @@ Please use `jj new 'all:x|y'` instead of `jj new --allow-large-revsets x y`.", command.settings(), tx.mut_repo(), child_commit, - vec![new_commit.clone()], + vec![new_commit.id().clone()], )?; } } else { @@ -170,17 +170,16 @@ Please use `jj new 'all:x|y'` instead of `jj new --allow-large-revsets x y`.", for child_commit in commits_to_rebase { let commit_parents = RevsetExpression::commits(child_commit.parent_ids().to_owned()); let new_parents = commit_parents.minus(&old_parents); - let mut new_parent_commits: Vec = new_parents + let mut new_parent_ids = new_parents .evaluate_programmatic(tx.base_repo().as_ref())? .iter() - .commits(tx.base_repo().store()) - .try_collect()?; - new_parent_commits.push(new_commit.clone()); + .collect_vec(); + new_parent_ids.push(new_commit.id().clone()); rebase_commit( command.settings(), tx.mut_repo(), child_commit, - new_parent_commits, + new_parent_ids, )?; } } diff --git a/cli/src/commands/rebase.rs b/cli/src/commands/rebase.rs index e4ed022500..e05c79e049 100644 --- a/cli/src/commands/rebase.rs +++ b/cli/src/commands/rebase.rs @@ -296,7 +296,10 @@ pub fn rebase_descendants( settings, tx.mut_repo(), old_commit.borrow().clone(), - new_parents.to_vec(), + new_parents + .iter() + .map(|parent| parent.id().clone()) + .collect(), &rebase_options, )?; } @@ -402,12 +405,11 @@ fn rebase_revision( .parents() .ancestors(), ); - let new_child_parents: Vec = new_child_parents_expression + let new_child_parents = new_child_parents_expression .evaluate_programmatic(tx.base_repo().as_ref()) .unwrap() .iter() - .commits(tx.base_repo().store()) - .try_collect()?; + .collect_vec(); rebased_commit_ids.insert( child_commit.id().clone(), @@ -447,23 +449,22 @@ fn rebase_revision( // above, and then the result would be rebased again by // `rebase_descendants_return_map`. Then, if we were trying to rebase // `old_commit` onto `Q`, new_parents would only account for one of these. - let new_parents: Vec<_> = new_parents + let new_parents = new_parents .iter() .map(|new_parent| { rebased_commit_ids .get(new_parent.id()) - .map_or(Ok(new_parent.clone()), |rebased_new_parent_id| { - tx.repo().store().get_commit(rebased_new_parent_id) - }) + .unwrap_or(new_parent.id()) + .clone() }) - .try_collect()?; + .collect(); let tx_description = format!("rebase commit {}", old_commit.id().hex()); // Finally, it's safe to rebase `old_commit`. We can skip rebasing if it is // already a child of `new_parents`. Otherwise, at this point, it should no // longer have any children; they have all been rebased and the originals // have been abandoned. - let skipped_commit_rebase = if old_commit.parents() == new_parents { + let skipped_commit_rebase = if old_commit.parent_ids() == new_parents { if let Some(mut formatter) = ui.status_formatter() { write!(formatter, "Skipping rebase of commit ")?; tx.write_commit_summary(formatter.as_mut(), &old_commit)?; diff --git a/lib/src/repo.rs b/lib/src/repo.rs index 32c27469df..bfb70a1195 100644 --- a/lib/src/repo.rs +++ b/lib/src/repo.rs @@ -1130,7 +1130,7 @@ impl MutableRepo { // Calculate an order where we rebase parents first, but if the parents were // rewritten, make sure we rebase the rewritten parent first. dag_walk::topo_order_reverse_ok( - to_visit.into_iter().map(|commit| Ok(commit)), + to_visit.into_iter().map(Ok), |commit| commit.id().clone(), |commit| -> Vec> { visited.insert(commit.id().clone()); diff --git a/lib/src/rewrite.rs b/lib/src/rewrite.rs index f98a841941..1769f849b3 100644 --- a/lib/src/rewrite.rs +++ b/lib/src/rewrite.rs @@ -96,7 +96,7 @@ pub fn rebase_commit( settings: &UserSettings, mut_repo: &mut MutableRepo, old_commit: Commit, - new_parents: Vec, + new_parents: Vec, ) -> BackendResult { let rebased_commit = rebase_commit_with_options( settings, @@ -120,27 +120,22 @@ pub fn rebase_commit_with_options( settings: &UserSettings, mut_repo: &mut MutableRepo, old_commit: Commit, - new_parents: Vec, + mut new_parent_ids: Vec, options: &RebaseOptions, ) -> BackendResult { // If specified, don't create commit where one parent is an ancestor of another. - let simplified_new_parents; - let new_parents = if options.simplify_ancestor_merge { - let mut new_parent_ids = new_parents.iter().map(|commit| commit.id()); + if options.simplify_ancestor_merge { let head_set: HashSet<_> = mut_repo .index() - .heads(&mut new_parent_ids) + .heads(&mut new_parent_ids.iter()) .into_iter() .collect(); - simplified_new_parents = new_parents - .iter() - .filter(|commit| head_set.contains(commit.id())) - .cloned() - .collect_vec(); - &simplified_new_parents[..] - } else { - &new_parents + new_parent_ids.retain(|id| head_set.contains(id)) }; + let new_parents: Vec = new_parent_ids + .iter() + .map(|id| mut_repo.store().get_commit(id)) + .try_collect()?; let old_parents = old_commit.parents(); let old_parent_trees = old_parents @@ -162,7 +157,7 @@ pub fn rebase_commit_with_options( ) } else { let old_base_tree = merge_commit_trees(mut_repo, &old_parents)?; - let new_base_tree = merge_commit_trees(mut_repo, new_parents)?; + let new_base_tree = merge_commit_trees(mut_repo, &new_parents)?; let old_tree = old_commit.tree()?; ( Some(old_base_tree.id()), @@ -171,7 +166,7 @@ pub fn rebase_commit_with_options( }; // Ensure we don't abandon commits with multiple parents (merge commits), even // if they're empty. - if let [parent] = new_parents { + if let [parent] = &new_parents[..] { let should_abandon = match options.empty { EmptyBehaviour::Keep => false, EmptyBehaviour::AbandonNewlyEmpty => { @@ -320,15 +315,11 @@ impl<'settings, 'repo> DescendantRebaser<'settings, 'repo> { return Ok(()); } - let new_parents: Vec<_> = new_parent_ids - .iter() - .map(|new_parent_id| self.mut_repo.store().get_commit(new_parent_id)) - .try_collect()?; let rebased_commit: RebasedCommit = rebase_commit_with_options( self.settings, self.mut_repo, old_commit, - new_parents, + new_parent_ids, &self.options, )?; let new_commit = match rebased_commit { diff --git a/lib/tests/test_merge_trees.rs b/lib/tests/test_merge_trees.rs index 3ebc1c13ae..35ba95601d 100644 --- a/lib/tests/test_merge_trees.rs +++ b/lib/tests/test_merge_trees.rs @@ -581,9 +581,20 @@ fn test_simplify_conflict_after_resolving_parent() { .write() .unwrap(); - let commit_b2 = rebase_commit(&settings, tx.mut_repo(), commit_b, vec![commit_d]).unwrap(); - let commit_c2 = - rebase_commit(&settings, tx.mut_repo(), commit_c, vec![commit_b2.clone()]).unwrap(); + let commit_b2 = rebase_commit( + &settings, + tx.mut_repo(), + commit_b, + vec![commit_d.id().clone()], + ) + .unwrap(); + let commit_c2 = rebase_commit( + &settings, + tx.mut_repo(), + commit_c, + vec![commit_b2.id().clone()], + ) + .unwrap(); // Test the setup: Both B and C should have conflicts. let tree_b2 = commit_b2.tree().unwrap(); @@ -599,7 +610,13 @@ fn test_simplify_conflict_after_resolving_parent() { .set_tree_id(tree_b3.id()) .write() .unwrap(); - let commit_c3 = rebase_commit(&settings, tx.mut_repo(), commit_c2, vec![commit_b3]).unwrap(); + let commit_c3 = rebase_commit( + &settings, + tx.mut_repo(), + commit_c2, + vec![commit_b3.id().clone()], + ) + .unwrap(); tx.mut_repo().rebase_descendants(&settings).unwrap(); let repo = tx.commit("test"); diff --git a/lib/tests/test_rewrite.rs b/lib/tests/test_rewrite.rs index 720cd77452..414400c61d 100644 --- a/lib/tests/test_rewrite.rs +++ b/lib/tests/test_rewrite.rs @@ -1730,7 +1730,7 @@ fn test_rebase_abandoning_empty() { &settings, tx.mut_repo(), commit_b, - vec![commit_b2.clone()], + vec![commit_b2.id().clone()], &rebase_options, ) .unwrap();