mirrors/jj
1
0
Fork 1
mirror of https://github.com/martinvonz/jj.git synced 2025-02-11 06:42:47 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions 19

rewrite: don't report skipped commits when rebasing descendants

Browse source 
The fact that `DescendantRebaser` visits some commits that don't need
to be rebased is mostly an implementation detail. I can't think of a
reason that callers would care about these commits.
This commit is contained in:
Martin von Zweigbergk 2021-09-19 21:32:58 -07:00
parent ae7f00e7b1
commit 439fe1cfd3
2 changed files with 46 additions and 71 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

53
lib/src/rewrite.rs
View file

@ -109,10 +109,10 @@ pub struct DescendantRebaser<'settings, 'repo> {
mut_repo: &'repo mut MutableRepo, mut_repo: &'repo mut MutableRepo,
replacements: HashMap<CommitId, Vec<CommitId>>, replacements: HashMap<CommitId, Vec<CommitId>>,
// In reverse order, so we can remove the last one to rebase first. // In reverse order, so we can remove the last one to rebase first.
to_rebase: Vec<CommitId>, to_visit: Vec<CommitId>,
// Ancestors of the destinations. These were also in `to_rebase` to start with, but we don't // Ancestors of the destinations. These were also in `to_visit` to start with, but we don't
// actually rebase them. Instead, we record them in `replacements` when we visit them. That // want to rebase them. Instead, we record them in `replacements` when we visit them. That way,
// way, their descendants will be rebased correctly. // their descendants will be rebased correctly.
ancestors: HashSet<CommitId>, ancestors: HashSet<CommitId>,
rebased: HashMap<CommitId, CommitId>, rebased: HashMap<CommitId, CommitId>,
} }
@ -128,20 +128,20 @@ impl<'settings, 'repo> DescendantRebaser<'settings, 'repo> {
let new_commits_expression = let new_commits_expression =
RevsetExpression::commits(replacements.values().flatten().cloned().collect()); RevsetExpression::commits(replacements.values().flatten().cloned().collect());
let to_rebase_expression = old_commits_expression let to_visit_expression = old_commits_expression
.descendants(&RevsetExpression::all_non_obsolete_heads()) .descendants(&RevsetExpression::all_non_obsolete_heads())
.minus(&old_commits_expression); .minus(&old_commits_expression);
let to_rebase_revset = to_rebase_expression let to_visit_revset = to_visit_expression
.evaluate(mut_repo.as_repo_ref()) .evaluate(mut_repo.as_repo_ref())
.unwrap(); .unwrap();
let mut to_rebase = vec![]; let mut to_visit = vec![];
for index_entry in to_rebase_revset.iter() { for index_entry in to_visit_revset.iter() {
to_rebase.push(index_entry.commit_id()); to_visit.push(index_entry.commit_id());
} }
drop(to_rebase_revset); drop(to_visit_revset);
let ancestors_expression = let ancestors_expression =
to_rebase_expression.intersection(&new_commits_expression.ancestors()); to_visit_expression.intersection(&new_commits_expression.ancestors());
let ancestors_revset = ancestors_expression let ancestors_revset = ancestors_expression
.evaluate(mut_repo.as_repo_ref()) .evaluate(mut_repo.as_repo_ref())
.unwrap(); .unwrap();
@ -155,7 +155,7 @@ impl<'settings, 'repo> DescendantRebaser<'settings, 'repo> {
settings, settings,
mut_repo, mut_repo,
replacements, replacements,
to_rebase, to_visit,
ancestors, ancestors,
rebased: Default::default(), rebased: Default::default(),
} }
@ -169,7 +169,7 @@ impl<'settings, 'repo> DescendantRebaser<'settings, 'repo> {
} }
pub fn rebase_next(&mut self) -> Option<RebasedDescendant> { pub fn rebase_next(&mut self) -> Option<RebasedDescendant> {
self.to_rebase.pop().map(|old_commit_id| { while let Some(old_commit_id) = self.to_visit.pop() {
let old_commit = self.mut_repo.store().get_commit(&old_commit_id).unwrap(); let old_commit = self.mut_repo.store().get_commit(&old_commit_id).unwrap();
let mut new_parent_ids = vec![]; let mut new_parent_ids = vec![];
let old_parent_ids = old_commit.parent_ids(); let old_parent_ids = old_commit.parent_ids();
@ -185,10 +185,12 @@ impl<'settings, 'repo> DescendantRebaser<'settings, 'repo> {
if self.ancestors.contains(&old_commit_id) { if self.ancestors.contains(&old_commit_id) {
// Update the `replacements` map so descendants are rebased correctly. // Update the `replacements` map so descendants are rebased correctly.
self.replacements.insert(old_commit_id, new_parent_ids); self.replacements.insert(old_commit_id, new_parent_ids);
RebasedDescendant::AncestorOfDestination(old_commit) continue;
} else if new_parent_ids == old_parent_ids { } else if new_parent_ids == old_parent_ids {
RebasedDescendant::AlreadyInPlace(old_commit) // The commit is already in place.
} else { continue;
}
// Don't create commit where one parent is an ancestor of another. // Don't create commit where one parent is an ancestor of another.
let head_set: HashSet<_> = self let head_set: HashSet<_> = self
.mut_repo .mut_repo
@ -205,15 +207,14 @@ impl<'settings, 'repo> DescendantRebaser<'settings, 'repo> {
.iter() .iter()
.map(|new_parent_id| self.mut_repo.store().get_commit(new_parent_id).unwrap()) .map(|new_parent_id| self.mut_repo.store().get_commit(new_parent_id).unwrap())
.collect_vec(); .collect_vec();
let new_commit = let new_commit = rebase_commit(self.settings, self.mut_repo, &old_commit, &new_parents);
rebase_commit(self.settings, self.mut_repo, &old_commit, &new_parents);
self.rebased.insert(old_commit_id, new_commit.id().clone()); self.rebased.insert(old_commit_id, new_commit.id().clone());
RebasedDescendant::Rebased { return Some(RebasedDescendant {
old_commit, old_commit,
new_commit, new_commit,
});
} }
} None
})
} }
pub fn rebase_all(&mut self) { pub fn rebase_all(&mut self) {
@ -222,13 +223,9 @@ impl<'settings, 'repo> DescendantRebaser<'settings, 'repo> {
} }
#[derive(Debug, PartialEq, Eq, Clone)] #[derive(Debug, PartialEq, Eq, Clone)]
pub enum RebasedDescendant { pub struct RebasedDescendant {
AlreadyInPlace(Commit), pub old_commit: Commit,
AncestorOfDestination(Commit), pub new_commit: Commit,
Rebased {
old_commit: Commit,
new_commit: Commit,
},
} }
pub fn update_branches_after_rewrite(mut_repo: &mut MutableRepo) { pub fn update_branches_after_rewrite(mut_repo: &mut MutableRepo) {

26
lib/tests/test_rewrite.rs
View file

@ -23,28 +23,12 @@ use jujutsu_lib::testutils::CommitGraphBuilder;
use maplit::hashmap; use maplit::hashmap;
use test_case::test_case; use test_case::test_case;
fn assert_in_place(rebased: Option<RebasedDescendant>, expected_old_commit: &Commit) {
if let Some(RebasedDescendant::AlreadyInPlace(old_commit)) = rebased {
assert_eq!(old_commit, *expected_old_commit);
} else {
panic!("expected in-place commit: {:?}", rebased);
}
}
fn assert_ancestor(rebased: Option<RebasedDescendant>, expected_old_commit: &Commit) {
if let Some(RebasedDescendant::AncestorOfDestination(old_commit)) = rebased {
assert_eq!(old_commit, *expected_old_commit);
} else {
panic!("expected ancestor commit: {:?}", rebased);
}
}
fn assert_rebased( fn assert_rebased(
rebased: Option<RebasedDescendant>, rebased: Option<RebasedDescendant>,
expected_old_commit: &Commit, expected_old_commit: &Commit,
expected_new_parents: &[CommitId], expected_new_parents: &[CommitId],
) -> Commit { ) -> Commit {
if let Some(RebasedDescendant::Rebased { if let Some(RebasedDescendant {
old_commit, old_commit,
new_commit, new_commit,
}) = rebased }) = rebased
@ -124,7 +108,7 @@ fn test_rebase_descendants_forward(use_git: bool) {
let commit4 = graph_builder.commit_with_parents(&[&commit2]); let commit4 = graph_builder.commit_with_parents(&[&commit2]);
let commit5 = graph_builder.commit_with_parents(&[&commit4]); let commit5 = graph_builder.commit_with_parents(&[&commit4]);
let commit6 = graph_builder.commit_with_parents(&[&commit4]); let commit6 = graph_builder.commit_with_parents(&[&commit4]);
let commit7 = graph_builder.commit_with_parents(&[&commit6]); let _commit7 = graph_builder.commit_with_parents(&[&commit6]);
let mut rebaser = DescendantRebaser::new( let mut rebaser = DescendantRebaser::new(
&settings, &settings,
@ -135,10 +119,7 @@ fn test_rebase_descendants_forward(use_git: bool) {
}, },
); );
assert_rebased(rebaser.rebase_next(), &commit3, &[commit6.id().clone()]); assert_rebased(rebaser.rebase_next(), &commit3, &[commit6.id().clone()]);
assert_ancestor(rebaser.rebase_next(), &commit4);
assert_rebased(rebaser.rebase_next(), &commit5, &[commit6.id().clone()]); assert_rebased(rebaser.rebase_next(), &commit5, &[commit6.id().clone()]);
assert_ancestor(rebaser.rebase_next(), &commit6);
assert_in_place(rebaser.rebase_next(), &commit7);
assert!(rebaser.rebase_next().is_none()); assert!(rebaser.rebase_next().is_none());
assert_eq!(rebaser.rebased().len(), 2); assert_eq!(rebaser.rebased().len(), 2);
@ -464,9 +445,6 @@ fn test_rebase_descendants_multiple_forward_and_backward(use_git: bool) {
commit6.id().clone() => vec![commit3.id().clone()], commit6.id().clone() => vec![commit3.id().clone()],
}, },
); );
assert_ancestor(rebaser.rebase_next(), &commit3);
assert_ancestor(rebaser.rebase_next(), &commit4);
assert_in_place(rebaser.rebase_next(), &commit5);
assert_rebased(rebaser.rebase_next(), &commit7, &[commit3.id().clone()]); assert_rebased(rebaser.rebase_next(), &commit7, &[commit3.id().clone()]);
assert_rebased(rebaser.rebase_next(), &commit8, &[commit4.id().clone()]); assert_rebased(rebaser.rebase_next(), &commit8, &[commit4.id().clone()]);
assert!(rebaser.rebase_next().is_none()); assert!(rebaser.rebase_next().is_none());