martinvonz/jj
1
1
Fork 0
mirror of https://github.com/martinvonz/jj.git synced 2024-11-10 14:16:24 +03:00
Code Issues Packages Projects Releases Wiki Activity

rewrite: fix auto-rebasing after "branchy" rewrites

Browse Source 
The `DescendantRebaser` was designed to help with rebasing in two
different use cases: 1) after regular rewriting of commits where the
change ID is preserved, and 2) after importing moved branches from
other repo (e.g. backing Git repo or remote). Many of the tests are
for the second use case, such as where a branch was moved
forward. However, I just noticed that there's a pretty common scenario
from the first use case that is not supported.

Let's say you have this history:

```
D
|
C C'
|/
B B'
|/
A
```

Here we want C' to be rebased onto B' and then D to be rebased onto
C''. However, because of the support for moving branches forward, we
would not rebase commits that were already rewritten, such as C' here
(see affected tests for details), which resulted in D getting rebased
onto C', and both B and B' remaining visible.

I think I was thinking when I designed it that it would be nice if you
could just tell `DescendantRebaser` that any descendants of a commit
should be moved forward. That may be useful, but I don't think we'll
want that for the general case of a branch moving forward. Perhaps
we'll want to make it configurable which branches it should happen
for. Either way, the way it was coded by not rebasing already
rewritten commits did not work for the case above. We may be able to
handle both cases better by considering each rewrite separately
instead of all destinations at once. For now, however, I've decided to
keep it simple, so I'm fixing the case above by sacrificing some of
the potentially useful functionality for moving branches forward.

Another fix necessary for the scenario shown above was to make sure we
always rebase C' before D. Before this patch, that depended on the
order in the index. This patch fixes that by modifying the topological
order to take rewrites into account, making D depend not only on C but
also on C'. (I suppose you could instead say that C depends on both B
and C'; I don't know if that'd make a difference.)
This commit is contained in:
Martin von Zweigbergk 2022-01-21 21:50:25 -08:00
parent 5b93ae6d4b
commit 0c8a116771
2 changed files with 160 additions and 79 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

62
lib/src/rewrite.rs
View File

@ -19,6 +19,7 @@ use itertools::Itertools;
use crate::backend::CommitId;
use crate::commit::Commit;
use crate::commit_builder::CommitBuilder;
use crate::dag_walk;
use crate::op_store::RefTarget;
use crate::repo::{MutableRepo, RepoRef};
use crate::repo_path::RepoPath;
@ -165,19 +166,38 @@ impl<'settings, 'repo> DescendantRebaser<'settings, 'repo> {
let to_visit_revset = to_visit_expression
.evaluate(mut_repo.as_repo_ref())
.unwrap();
let to_visit = to_visit_revset.iter().commit_ids().collect_vec();
let to_visit_entries = to_visit_revset.iter().collect_vec();
drop(to_visit_revset);
let new_commits_expression =
RevsetExpression::commits(rewritten.values().flatten().cloned().collect());
let ancestors_expression =
to_visit_expression.intersection(&new_commits_expression.ancestors());
let ancestors_revset = ancestors_expression
.evaluate(mut_repo.as_repo_ref())
.unwrap();
let mut to_skip = abandoned;
to_skip.extend(ancestors_revset.iter().commit_ids());
drop(ancestors_revset);
let index = mut_repo.index();
let to_visit_set: HashSet<CommitId> = to_visit_entries
.iter()
.map(|entry| entry.commit_id())
.collect();
let mut visited = HashSet::new();
// Calculate an order where we rebase parents first, but if the parents were
// rewritten, make sure we rebase the rewritten parent first.
let to_visit = dag_walk::topo_order_reverse(
to_visit_entries,
Box::new(|entry| entry.commit_id()),
Box::new(|entry| {
visited.insert(entry.commit_id());
let mut dependents = vec![];
for parent in entry.parents() {
if let Some(targets) = rewritten.get(&parent.commit_id()) {
for target in targets {
if to_visit_set.contains(target) && !visited.contains(target) {
dependents.push(index.entry_by_id(target).unwrap());
}
}
}
if to_visit_set.contains(&parent.commit_id()) {
dependents.push(parent);
}
}
dependents
}),
);
let to_visit = to_visit.iter().map(|entry| entry.commit_id()).collect_vec();
let new_commits = rewritten.values().flatten().cloned().collect();
@ -219,7 +239,7 @@ impl<'settings, 'repo> DescendantRebaser<'settings, 'repo> {
new_parents,
divergent,
to_visit,
to_skip,
to_skip: abandoned,
new_commits,
rebased: Default::default(),
branches,
@ -239,7 +259,14 @@ impl<'settings, 'repo> DescendantRebaser<'settings, 'repo> {
let mut new_ids = vec![];
for old_id in old_ids {
if let Some(new_parent_ids) = self.new_parents.get(old_id) {
new_ids.extend(new_parent_ids.clone());
for new_parent_id in new_parent_ids {
// The new parent may itself have been rebased earlier in the process
if let Some(newer_parent_id) = self.rebased.get(new_parent_id) {
new_ids.push(newer_parent_id.clone());
} else {
new_ids.push(new_parent_id.clone());
}
}
} else if let Some(new_parent_id) = self.rebased.get(old_id) {
new_ids.push(new_parent_id.clone());
} else {
@ -308,7 +335,7 @@ impl<'settings, 'repo> DescendantRebaser<'settings, 'repo> {
}
self.heads_to_add.remove(&old_commit_id);
if !self.new_commits.contains(&old_commit_id) {
if !self.new_commits.contains(&old_commit_id) || self.rebased.contains_key(&old_commit_id) {
self.heads_to_remove.push(old_commit_id);
}
}
@ -358,8 +385,9 @@ impl<'settings, 'repo> DescendantRebaser<'settings, 'repo> {
.map(|new_parent_id| self.mut_repo.store().get_commit(new_parent_id).unwrap())
.collect_vec();
let new_commit = rebase_commit(self.settings, self.mut_repo, &old_commit, &new_parents);
self.update_references(old_commit_id.clone(), vec![new_commit.id().clone()]);
self.rebased.insert(old_commit_id, new_commit.id().clone());
self.rebased
.insert(old_commit_id.clone(), new_commit.id().clone());
self.update_references(old_commit_id, vec![new_commit.id().clone()]);
return Some(RebasedDescendant {
old_commit,
new_commit,

177
lib/tests/test_rewrite.rs
View File

@ -81,6 +81,11 @@ fn test_rebase_descendants_forward(use_git: bool) {
// Commit D does not get rebased because it's an ancestor of the
// destination. Commit G does not get replaced because it's already in
// place.
// TODO: The above is not what actually happens! The test below shows what
// actually happens: D and F also get rebased onto F, so we end up with
// duplicates. Consider if it's worth supporting the case above better or if
// that decision belongs with the caller (as we currently force it to do by
// not supporting it in DescendantRebaser).
//
// G
// F E
@ -107,18 +112,74 @@ fn test_rebase_descendants_forward(use_git: bool) {
},
hashset! {},
);
let new_commit_c = assert_rebased(rebaser.rebase_next(), &commit_c, &[&commit_f]);
let new_commit_e = assert_rebased(rebaser.rebase_next(), &commit_e, &[&commit_f]);
let new_commit_d = assert_rebased(rebaser.rebase_next(), &commit_d, &[&commit_f]);
let new_commit_f = assert_rebased(rebaser.rebase_next(), &commit_f, &[&new_commit_d]);
let new_commit_c = assert_rebased(rebaser.rebase_next(), &commit_c, &[&new_commit_f]);
let new_commit_e = assert_rebased(rebaser.rebase_next(), &commit_e, &[&new_commit_d]);
let new_commit_g = assert_rebased(rebaser.rebase_next(), &commit_g, &[&new_commit_f]);
assert!(rebaser.rebase_next().is_none());
assert_eq!(rebaser.rebased().len(), 2);
assert_eq!(rebaser.rebased().len(), 5);
assert_eq!(
*tx.mut_repo().view().heads(),
hashset! {
repo.view().checkout().clone(),
commit_g.id().clone(),
new_commit_c.id().clone(),
new_commit_e.id().clone()
new_commit_e.id().clone(),
new_commit_g.id().clone(),
}
);
}
#[test_case(false ; "local backend")]
#[test_case(true ; "git backend")]
fn test_rebase_descendants_reorder(use_git: bool) {
let settings = testutils::user_settings();
let test_workspace = testutils::init_repo(&settings, use_git);
let repo = &test_workspace.repo;
// Commit E was replaced by commit D, and commit C was replaced by commit F
// (attempting to to reorder C and E), and commit G was replaced by commit
// H.
//
// I
// G H
// E F
// C D
// |/
// B
// A
let mut tx = repo.start_transaction("test");
let mut graph_builder = CommitGraphBuilder::new(&settings, tx.mut_repo());
let commit_a = graph_builder.initial_commit();
let commit_b = graph_builder.commit_with_parents(&[&commit_a]);
let commit_c = graph_builder.commit_with_parents(&[&commit_b]);
let commit_d = graph_builder.commit_with_parents(&[&commit_b]);
let commit_e = graph_builder.commit_with_parents(&[&commit_c]);
let commit_f = graph_builder.commit_with_parents(&[&commit_d]);
let commit_g = graph_builder.commit_with_parents(&[&commit_e]);
let commit_h = graph_builder.commit_with_parents(&[&commit_f]);
let commit_i = graph_builder.commit_with_parents(&[&commit_g]);
let mut rebaser = DescendantRebaser::new(
&settings,
tx.mut_repo(),
hashmap! {
commit_e.id().clone() => hashset!{commit_d.id().clone()},
commit_c.id().clone() => hashset!{commit_f.id().clone()},
commit_g.id().clone() => hashset!{commit_h.id().clone()},
},
hashset! {},
);
let new_commit_i = assert_rebased(rebaser.rebase_next(), &commit_i, &[&commit_h]);
assert!(rebaser.rebase_next().is_none());
assert_eq!(rebaser.rebased().len(), 1);
assert_eq!(
*tx.mut_repo().view().heads(),
hashset! {
repo.view().checkout().clone(),
new_commit_i.id().clone(),
}
);
}
@ -161,6 +222,55 @@ fn test_rebase_descendants_backward(use_git: bool) {
);
}
#[test_case(false ; "local backend")]
#[test_case(true ; "git backend")]
fn test_rebase_descendants_chain_becomes_branchy(use_git: bool) {
let settings = testutils::user_settings();
let test_workspace = testutils::init_repo(&settings, use_git);
let repo = &test_workspace.repo;
// Commit B was replaced by commit E and commit C was replaced by commit F.
// Commit F should get rebased onto E, and commit D should get rebased onto
// the rebased F.
//
// D
// C F
// |/
// B E
// |/
// A
let mut tx = repo.start_transaction("test");
let mut graph_builder = CommitGraphBuilder::new(&settings, tx.mut_repo());
let commit_a = graph_builder.initial_commit();
let commit_b = graph_builder.commit_with_parents(&[&commit_a]);
let commit_c = graph_builder.commit_with_parents(&[&commit_b]);
let commit_d = graph_builder.commit_with_parents(&[&commit_c]);
let commit_e = graph_builder.commit_with_parents(&[&commit_a]);
let commit_f = graph_builder.commit_with_parents(&[&commit_b]);
let mut rebaser = DescendantRebaser::new(
&settings,
tx.mut_repo(),
hashmap! {
commit_b.id().clone() => hashset!{commit_e.id().clone()},
commit_c.id().clone() => hashset!{commit_f.id().clone()},
},
hashset! {},
);
let new_commit_f = assert_rebased(rebaser.rebase_next(), &commit_f, &[&commit_e]);
let new_commit_d = assert_rebased(rebaser.rebase_next(), &commit_d, &[&new_commit_f]);
assert!(rebaser.rebase_next().is_none());
assert_eq!(rebaser.rebased().len(), 2);
assert_eq!(
*tx.mut_repo().view().heads(),
hashset! {
repo.view().checkout().clone(),
new_commit_d.id().clone(),
}
);
}
#[test_case(false ; "local backend")]
#[test_case(true ; "git backend")]
fn test_rebase_descendants_internal_merge(use_git: bool) {
@ -596,63 +706,6 @@ fn test_rebase_descendants_multiple_no_descendants(use_git: bool) {
);
}
#[test_case(false ; "local backend")]
#[test_case(true ; "git backend")]
fn test_rebase_descendants_multiple_forward_and_backward(use_git: bool) {
let settings = testutils::user_settings();
let test_workspace = testutils::init_repo(&settings, use_git);
let repo = &test_workspace.repo;
// Commit B was replaced by commit D. Commit F was replaced by commit C.
// Commit G should be rebased onto commit C. Commit H should be rebased onto
// commit D. Commits C-D should be left alone since they're ancestors of D.
// Commit E should be left alone since its already in place (as a descendant of
// D).
//
// G
// F
// E
// D
// C H
// |/
// B
// A
let mut tx = repo.start_transaction("test");
let mut graph_builder = CommitGraphBuilder::new(&settings, tx.mut_repo());
let commit_a = graph_builder.initial_commit();
let commit_b = graph_builder.commit_with_parents(&[&commit_a]);
let commit_c = graph_builder.commit_with_parents(&[&commit_b]);
let commit_d = graph_builder.commit_with_parents(&[&commit_c]);
let commit_e = graph_builder.commit_with_parents(&[&commit_d]);
let commit_f = graph_builder.commit_with_parents(&[&commit_e]);
let commit_g = graph_builder.commit_with_parents(&[&commit_f]);
let commit_h = graph_builder.commit_with_parents(&[&commit_b]);
let mut rebaser = DescendantRebaser::new(
&settings,
tx.mut_repo(),
hashmap! {
commit_b.id().clone() => hashset!{commit_d.id().clone()},
commit_f.id().clone() => hashset!{commit_c.id().clone()},
},
hashset! {},
);
let new_commit_g = assert_rebased(rebaser.rebase_next(), &commit_g, &[&commit_c]);
let new_commit_h = assert_rebased(rebaser.rebase_next(), &commit_h, &[&commit_d]);
assert!(rebaser.rebase_next().is_none());
assert_eq!(rebaser.rebased().len(), 2);
assert_eq!(
*tx.mut_repo().view().heads(),
hashset! {
repo.view().checkout().clone(),
commit_e.id().clone(),
new_commit_g.id().clone(),
new_commit_h.id().clone()
}
);
}
#[test_case(false ; "local backend")]
#[test_case(true ; "git backend")]
fn test_rebase_descendants_divergent_rewrite(use_git: bool) {