ok/jj
1
0
Fork 0
forked from mirrors/jj
Code Pull requests Activity

rewrite: teach DescendantRebaser to handle abandoned commits specially

Browse source 
Descendants of abandoned commits should be rebased onto their parents,
or the rewritten parents if they had been rewritten. This patch
teaches `DescendantRebaser` to do that. It updates `jj rebase -r` to
use the functionality. I plan to also use it in `jj abandon`
(naturally, given the name), and for rebasing descendants of deleted
refs imported from `jj git refresh/fetch/push`.
This commit is contained in:
Martin von Zweigbergk 2021-09-19 20:29:53 -07:00
parent 439fe1cfd3
commit d4004fcb6f
3 changed files with 138 additions and 47 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

44
lib/src/rewrite.rs
View file

@ -107,13 +107,13 @@ pub fn back_out_commit(
pub struct DescendantRebaser<'settings, 'repo> {
settings: &'settings UserSettings,
mut_repo: &'repo mut MutableRepo,
replacements: HashMap<CommitId, Vec<CommitId>>,
new_parents: HashMap<CommitId, Vec<CommitId>>,
// In reverse order, so we can remove the last one to rebase first.
to_visit: Vec<CommitId>,
// Ancestors of the destinations. These were also in `to_visit` to start with, but we don't
// Commits to visit but skip. These were also in `to_visit` to start with, but we don't
// want to rebase them. Instead, we record them in `replacements` when we visit them. That way,
// their descendants will be rebased correctly.
ancestors: HashSet<CommitId>,
to_skip: HashSet<CommitId>,
rebased: HashMap<CommitId, CommitId>,
}
@ -121,16 +121,18 @@ impl<'settings, 'repo> DescendantRebaser<'settings, 'repo> {
pub fn new(
settings: &'settings UserSettings,
mut_repo: &'repo mut MutableRepo,
replacements: HashMap<CommitId, Vec<CommitId>>,
replaced: HashMap<CommitId, CommitId>,
abandoned: HashSet<CommitId>,
) -> DescendantRebaser<'settings, 'repo> {
let old_commits_expression =
RevsetExpression::commits(replacements.keys().cloned().collect());
let old_commits_expression = RevsetExpression::commits(replaced.keys().cloned().collect())
.union(&RevsetExpression::commits(
abandoned.iter().cloned().collect(),
));
let new_commits_expression =
RevsetExpression::commits(replacements.values().flatten().cloned().collect());
RevsetExpression::commits(replaced.values().cloned().collect());
let to_visit_expression = old_commits_expression
.descendants(&RevsetExpression::all_non_obsolete_heads())
.minus(&old_commits_expression);
let to_visit_expression =
old_commits_expression.descendants(&RevsetExpression::all_non_obsolete_heads());
let to_visit_revset = to_visit_expression
.evaluate(mut_repo.as_repo_ref())
.unwrap();
@ -145,18 +147,23 @@ impl<'settings, 'repo> DescendantRebaser<'settings, 'repo> {
let ancestors_revset = ancestors_expression
.evaluate(mut_repo.as_repo_ref())
.unwrap();
let mut ancestors = HashSet::new();
let mut to_skip = abandoned;
for index_entry in ancestors_revset.iter() {
ancestors.insert(index_entry.commit_id());
to_skip.insert(index_entry.commit_id());
}
drop(ancestors_revset);
let mut new_parents = HashMap::new();
for (old_commit, new_commit) in replaced {
new_parents.insert(old_commit, vec![new_commit]);
}
DescendantRebaser {
settings,
mut_repo,
replacements,
new_parents,
to_visit,
ancestors,
to_skip,
rebased: Default::default(),
}
}
@ -170,11 +177,14 @@ impl<'settings, 'repo> DescendantRebaser<'settings, 'repo> {
pub fn rebase_next(&mut self) -> Option<RebasedDescendant> {
while let Some(old_commit_id) = self.to_visit.pop() {
if self.new_parents.contains_key(&old_commit_id) {
continue;
}
let old_commit = self.mut_repo.store().get_commit(&old_commit_id).unwrap();
let mut new_parent_ids = vec![];
let old_parent_ids = old_commit.parent_ids();
for old_parent_id in &old_parent_ids {
if let Some(replacements) = self.replacements.get(old_parent_id) {
if let Some(replacements) = self.new_parents.get(old_parent_id) {
new_parent_ids.extend(replacements.clone());
} else if let Some(new_parent_id) = self.rebased.get(old_parent_id) {
new_parent_ids.push(new_parent_id.clone());
@ -182,9 +192,9 @@ impl<'settings, 'repo> DescendantRebaser<'settings, 'repo> {
new_parent_ids.push(old_parent_id.clone());
};
}
if self.ancestors.contains(&old_commit_id) {
if self.to_skip.contains(&old_commit_id) {
// Update the `replacements` map so descendants are rebased correctly.
self.replacements.insert(old_commit_id, new_parent_ids);
self.new_parents.insert(old_commit_id, new_parent_ids);
continue;
} else if new_parent_ids == old_parent_ids {
// The commit is already in place.

131
lib/tests/test_rewrite.rs
View file

@ -20,7 +20,7 @@ use jujutsu_lib::repo_path::RepoPath;
use jujutsu_lib::rewrite::{update_branches_after_rewrite, DescendantRebaser, RebasedDescendant};
use jujutsu_lib::testutils;
use jujutsu_lib::testutils::CommitGraphBuilder;
use maplit::hashmap;
use maplit::{hashmap, hashset};
use test_case::test_case;
fn assert_rebased(
@ -70,8 +70,9 @@ fn test_rebase_descendants_sideways(use_git: bool) {
&settings,
tx.mut_repo(),
hashmap! {
commit2.id().clone() => vec![commit6.id().clone()]
commit2.id().clone() => commit6.id().clone()
},
hashset! {},
);
let new_commit3 = assert_rebased(rebaser.rebase_next(), &commit3, &[commit6.id().clone()]);
assert_rebased(rebaser.rebase_next(), &commit4, &[new_commit3.id().clone()]);
@ -115,8 +116,9 @@ fn test_rebase_descendants_forward(use_git: bool) {
tx.mut_repo(),
hashmap! {
commit2.id().clone() =>
vec![commit6.id().clone()]
commit6.id().clone()
},
hashset! {},
);
assert_rebased(rebaser.rebase_next(), &commit3, &[commit6.id().clone()]);
assert_rebased(rebaser.rebase_next(), &commit5, &[commit6.id().clone()]);
@ -149,8 +151,9 @@ fn test_rebase_descendants_backward(use_git: bool) {
&settings,
tx.mut_repo(),
hashmap! {
commit3.id().clone() => vec![commit2.id().clone()]
commit3.id().clone() => commit2.id().clone()
},
hashset! {},
);
assert_rebased(rebaser.rebase_next(), &commit4, &[commit2.id().clone()]);
assert!(rebaser.rebase_next().is_none());
@ -188,8 +191,9 @@ fn test_rebase_descendants_internal_merge(use_git: bool) {
&settings,
tx.mut_repo(),
hashmap! {
commit2.id().clone() => vec![commit6.id().clone()]
commit2.id().clone() => commit6.id().clone()
},
hashset! {},
);
let new_commit3 = assert_rebased(rebaser.rebase_next(), &commit3, &[commit6.id().clone()]);
let new_commit4 = assert_rebased(rebaser.rebase_next(), &commit4, &[commit6.id().clone()]);
@ -234,8 +238,9 @@ fn test_rebase_descendants_external_merge(use_git: bool) {
&settings,
tx.mut_repo(),
hashmap! {
commit3.id().clone() => vec![commit6.id().clone()]
commit3.id().clone() => commit6.id().clone()
},
hashset! {},
);
assert_rebased(
rebaser.rebase_next(),
@ -250,12 +255,86 @@ fn test_rebase_descendants_external_merge(use_git: bool) {
#[test_case(false ; "local backend")]
#[test_case(true ; "git backend")]
fn test_rebase_descendants_degenerate_merge(use_git: bool) {
fn test_rebase_descendants_abandon(use_git: bool) {
let settings = testutils::user_settings();
let (_temp_dir, repo) = testutils::init_repo(&settings, use_git);
// Commit 2 was replaced by commit 1 (maybe it was abandoned). Commit 4 should
// get rebased to have only 3 as parent (not 1 and 3).
// Commit 2 and commit 5 were abandoned. Commit 3 and commit 4 should get
// rebased onto commit 1. Commit 6 should get rebased onto the new commit 4.
//
// 6
// 5
// 4 3
// |/
// 2
// 1
let mut tx = repo.start_transaction("test");
let mut graph_builder = CommitGraphBuilder::new(&settings, tx.mut_repo());
let commit1 = graph_builder.initial_commit();
let commit2 = graph_builder.commit_with_parents(&[&commit1]);
let commit3 = graph_builder.commit_with_parents(&[&commit2]);
let commit4 = graph_builder.commit_with_parents(&[&commit2]);
let commit5 = graph_builder.commit_with_parents(&[&commit4]);
let commit6 = graph_builder.commit_with_parents(&[&commit5]);
let mut rebaser = DescendantRebaser::new(
&settings,
tx.mut_repo(),
hashmap! {},
hashset! {commit2.id().clone(), commit5.id().clone()},
);
assert_rebased(rebaser.rebase_next(), &commit3, &[commit1.id().clone()]);
let new_commit4 = assert_rebased(rebaser.rebase_next(), &commit4, &[commit1.id().clone()]);
assert_rebased(rebaser.rebase_next(), &commit6, &[new_commit4.id().clone()]);
assert!(rebaser.rebase_next().is_none());
assert_eq!(rebaser.rebased().len(), 3);
tx.discard();
}
#[test_case(false ; "local backend")]
#[test_case(true ; "git backend")]
fn test_rebase_descendants_abandon_and_replace(use_git: bool) {
let settings = testutils::user_settings();
let (_temp_dir, repo) = testutils::init_repo(&settings, use_git);
// Commit 2 was replaced by commit 5. Commit 3 was abandoned. Commit 4 should
// get rebased onto commit 5.
//
// 4
// 3
// 5 2
// |/
// 1
let mut tx = repo.start_transaction("test");
let mut graph_builder = CommitGraphBuilder::new(&settings, tx.mut_repo());
let commit1 = graph_builder.initial_commit();
let commit2 = graph_builder.commit_with_parents(&[&commit1]);
let commit3 = graph_builder.commit_with_parents(&[&commit2]);
let commit4 = graph_builder.commit_with_parents(&[&commit3]);
let commit5 = graph_builder.commit_with_parents(&[&commit1]);
let mut rebaser = DescendantRebaser::new(
&settings,
tx.mut_repo(),
hashmap! {commit2.id().clone() => commit5.id().clone()},
hashset! {commit3.id().clone()},
);
assert_rebased(rebaser.rebase_next(), &commit4, &[commit5.id().clone()]);
assert!(rebaser.rebase_next().is_none());
assert_eq!(rebaser.rebased().len(), 1);
tx.discard();
}
#[test_case(false ; "local backend")]
#[test_case(true ; "git backend")]
fn test_rebase_descendants_abandon_degenerate_merge(use_git: bool) {
let settings = testutils::user_settings();
let (_temp_dir, repo) = testutils::init_repo(&settings, use_git);
// Commit 2 was abandoned. Commit 4 should get rebased to have only 3 as parent
// (not 1 and 3).
//
// 4
// |\
@ -272,9 +351,8 @@ fn test_rebase_descendants_degenerate_merge(use_git: bool) {
let mut rebaser = DescendantRebaser::new(
&settings,
tx.mut_repo(),
hashmap! {
commit2.id().clone() => vec![commit1.id().clone()]
},
hashmap! {},
hashset! {commit2.id().clone()},
);
assert_rebased(rebaser.rebase_next(), &commit4, &[commit3.id().clone()]);
assert!(rebaser.rebase_next().is_none());
@ -285,12 +363,12 @@ fn test_rebase_descendants_degenerate_merge(use_git: bool) {
#[test_case(false ; "local backend")]
#[test_case(true ; "git backend")]
fn test_rebase_descendants_widen_merge(use_git: bool) {
fn test_rebase_descendants_abandon_widen_merge(use_git: bool) {
let settings = testutils::user_settings();
let (_temp_dir, repo) = testutils::init_repo(&settings, use_git);
// Commit 5 was replaced by commits 2 and 3 (maybe 5 was abandoned). Commit 6
// should get rebased to have 2, 3, and 4 as parents (in that order).
// Commit 5 was abandoned. Commit 6 should get rebased to have 2, 3, and 4 as
// parents (in that order).
//
// 6
// |\
@ -311,9 +389,8 @@ fn test_rebase_descendants_widen_merge(use_git: bool) {
let mut rebaser = DescendantRebaser::new(
&settings,
tx.mut_repo(),
hashmap! {
commit5.id().clone() => vec![commit2.id().clone(), commit3.id().clone()]
},
hashmap! {},
hashset! {commit5.id().clone()},
);
assert_rebased(
rebaser.rebase_next(),
@ -357,9 +434,10 @@ fn test_rebase_descendants_multiple_sideways(use_git: bool) {
&settings,
tx.mut_repo(),
hashmap! {
commit2.id().clone() => vec![commit6.id().clone()],
commit4.id().clone() => vec![commit6.id().clone()],
commit2.id().clone() => commit6.id().clone(),
commit4.id().clone() => commit6.id().clone(),
},
hashset! {},
);
assert_rebased(rebaser.rebase_next(), &commit3, &[commit6.id().clone()]);
assert_rebased(rebaser.rebase_next(), &commit5, &[commit6.id().clone()]);
@ -394,9 +472,10 @@ fn test_rebase_descendants_multiple_swap(use_git: bool) {
&settings,
tx.mut_repo(),
hashmap! {
commit2.id().clone() => vec![commit4.id().clone()],
commit4.id().clone() => vec![commit2.id().clone()],
commit2.id().clone() => commit4.id().clone(),
commit4.id().clone() => commit2.id().clone(),
},
hashset! {},
);
assert_rebased(rebaser.rebase_next(), &commit3, &[commit4.id().clone()]);
assert_rebased(rebaser.rebase_next(), &commit5, &[commit2.id().clone()]);
@ -441,9 +520,10 @@ fn test_rebase_descendants_multiple_forward_and_backward(use_git: bool) {
&settings,
tx.mut_repo(),
hashmap! {
commit2.id().clone() => vec![commit4.id().clone()],
commit6.id().clone() => vec![commit3.id().clone()],
commit2.id().clone() => commit4.id().clone(),
commit6.id().clone() => commit3.id().clone(),
},
hashset! {},
);
assert_rebased(rebaser.rebase_next(), &commit7, &[commit3.id().clone()]);
assert_rebased(rebaser.rebase_next(), &commit8, &[commit4.id().clone()]);
@ -492,8 +572,9 @@ fn test_rebase_descendants_contents(use_git: bool) {
&settings,
tx.mut_repo(),
hashmap! {
commit2.id().clone() => vec![commit4.id().clone()]
commit2.id().clone() => commit4.id().clone()
},
hashset! {},
);
rebaser.rebase_all();
let rebased = rebaser.rebased();

10
src/commands.rs
View file

@ -62,7 +62,7 @@ use jujutsu_lib::transaction::Transaction;
use jujutsu_lib::tree::{Diff, DiffSummary};
use jujutsu_lib::working_copy::{CheckoutStats, WorkingCopy};
use jujutsu_lib::{conflicts, files, git, revset};
use maplit::hashmap;
use maplit::{hashmap, hashset};
use pest::Parser;
use self::chrono::{FixedOffset, TimeZone, Utc};
@ -2585,8 +2585,9 @@ fn cmd_rebase(
ui.settings(),
tx.mut_repo(),
hashmap! {
old_commit.id().clone() => vec![new_commit.id().clone()]
old_commit.id().clone() => new_commit.id().clone()
},
hashset! {},
);
rebaser.rebase_all();
let num_rebased = rebaser.rebased().len() + 1;
@ -2602,9 +2603,8 @@ fn cmd_rebase(
let mut rebaser = DescendantRebaser::new(
ui.settings(),
tx.mut_repo(),
hashmap! {
old_commit.id().clone() => old_commit.parent_ids()
},
hashmap! {},
hashset! {old_commit.id().clone()},
);
rebaser.rebase_all();
let num_rebased = rebaser.rebased().len();