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

rebase: rewrite rebase_revision to use transform_descendants

Browse source
This commit is contained in:
Benjamin Tan 2024-04-21 19:59:12 +08:00
parent 9f4a7318c7
commit fb7c91ffa8
3 changed files with 57 additions and 163 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

119
cli/src/commands/rebase.rs
View file

@ -20,7 +20,6 @@ use std::sync::Arc;
use clap::ArgGroup; use clap::ArgGroup;
use indexmap::IndexSet; use indexmap::IndexSet;
use itertools::Itertools; use itertools::Itertools;
use jj_lib::backend::CommitId;
use jj_lib::commit::{Commit, CommitIteratorExt}; use jj_lib::commit::{Commit, CommitIteratorExt};
use jj_lib::object_id::ObjectId; use jj_lib::object_id::ObjectId;
use jj_lib::repo::{ReadonlyRepo, Repo}; use jj_lib::repo::{ReadonlyRepo, Repo};
@ -356,96 +355,48 @@ fn rebase_revision(
new_parents: &[Commit], new_parents: &[Commit],
rev_arg: &RevisionArg, rev_arg: &RevisionArg,
) -> Result<(), CommandError> { ) -> Result<(), CommandError> {
let old_commit = workspace_command.resolve_single_rev(rev_arg)?; let to_rebase_commit = workspace_command.resolve_single_rev(rev_arg)?;
workspace_command.check_rewritable([old_commit.id()])?; workspace_command.check_rewritable([to_rebase_commit.id()])?;
if new_parents.contains(&old_commit) { if new_parents.contains(&to_rebase_commit) {
return Err(user_error(format!( return Err(user_error(format!(
"Cannot rebase {} onto itself", "Cannot rebase {} onto itself",
short_commit_hash(old_commit.id()), short_commit_hash(to_rebase_commit.id()),
))); )));
} }
let children_expression = RevsetExpression::commit(old_commit.id().clone()).children();
let child_commits: Vec<_> = children_expression
.evaluate_programmatic(workspace_command.repo().as_ref())
.unwrap()
.iter()
.commits(workspace_command.repo().store())
.try_collect()?;
// Currently, immutable commits are defined so that a child of a rewriteable
// commit is always rewriteable.
debug_assert!(workspace_command
.check_rewritable(child_commits.iter().ids())
.is_ok());
// First, rebase the children of `old_commit`.
let mut tx = workspace_command.start_transaction(); let mut tx = workspace_command.start_transaction();
let mut rebased_commit_ids = HashMap::new(); let mut rebased_commit_ids = HashMap::new();
for child_commit in child_commits {
let new_child_parent_ids: Vec<CommitId> = child_commit
.parents()
.iter()
.flat_map(|c| {
if c == &old_commit {
old_commit.parent_ids().to_vec()
} else {
[c.id().clone()].to_vec()
}
})
.collect();
// Some of the new parents may be ancestors of others as in // First, rebase the descendants of `to_rebase_commit`.
// `test_rebase_single_revision`.
let new_child_parents_expression = RevsetExpression::commits(new_child_parent_ids.clone())
.minus(
&RevsetExpression::commits(new_child_parent_ids.clone())
.parents()
.ancestors(),
);
let new_child_parents = new_child_parents_expression
.evaluate_programmatic(tx.base_repo().as_ref())
.unwrap()
.iter()
.collect_vec();
rebased_commit_ids.insert(
child_commit.id().clone(),
rebase_commit(settings, tx.mut_repo(), child_commit, new_child_parents)?
.id()
.clone(),
);
}
// Now, rebase the descendants of the children.
// TODO(ilyagr): Consider making it possible for these descendants to become // TODO(ilyagr): Consider making it possible for these descendants to become
// emptied, like --skip_empty. This would require writing careful tests. // emptied, like --skip_empty. This would require writing careful tests.
rebased_commit_ids.extend(tx.mut_repo().rebase_descendants_return_map(settings)?); tx.mut_repo().transform_descendants(
settings,
vec![to_rebase_commit.id().clone()],
|mut rewriter| {
let old_commit = rewriter.old_commit();
let old_commit_id = old_commit.id().clone();
// Replace references to `to_rebase_commit` with its parents.
if old_commit.parent_ids().contains(to_rebase_commit.id()) {
rewriter.replace_parent(to_rebase_commit.id(), to_rebase_commit.parent_ids());
rewriter.simplify_ancestor_merge();
}
if rewriter.parents_changed() {
let builder = rewriter.rebase(settings)?;
let commit = builder.write()?;
rebased_commit_ids.insert(old_commit_id, commit.id().clone());
}
Ok(())
},
)?;
let num_rebased_descendants = rebased_commit_ids.len(); let num_rebased_descendants = rebased_commit_ids.len();
// We now update `new_parents` to account for the rebase of all of // We now update `new_parents` to account for the rebase of all of
// `old_commit`'s descendants. Even if some of the original `new_parents` // `to_rebase_commit`'s descendants. Even if some of the original `new_parents`
// were descendants of `old_commit`, this will no longer be the case after // were descendants of `to_rebase_commit`, this will no longer be the case after
// the update. // the update.
//
// To make the update simpler, we assume that each commit was rewritten only
// once; we don't have a situation where both `(A,B)` and `(B,C)` are in
// `rebased_commit_ids`.
//
// TODO(BUG #2650): There is something wrong with this assumption, the next TODO
// seems to be a little optimistic. See the panicked test in
// `test_rebase_with_child_and_descendant_bug_2600`.
//
// TODO(ilyagr): This assumption relies on the fact that, after
// `rebase_descendants`, a descendant of `old_commit` cannot also be a
// direct child of `old_commit`. This fact will likely change, see
// https://github.com/martinvonz/jj/issues/2600. So, the code needs to be
// updated before that happens. This would also affect
// `test_rebase_with_child_and_descendant_bug_2600`.
//
// The issue is that if a child and a descendant of `old_commit` were the
// same commit (call it `Q`), it would be rebased first by `rebase_commit`
// 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 = new_parents let new_parents = new_parents
.iter() .iter()
.map(|new_parent| { .map(|new_parent| {
@ -456,20 +407,20 @@ fn rebase_revision(
}) })
.collect(); .collect();
let tx_description = format!("rebase commit {}", old_commit.id().hex()); let tx_description = format!("rebase commit {}", to_rebase_commit.id().hex());
// Finally, it's safe to rebase `old_commit`. We can skip rebasing if it is // Finally, it's safe to rebase `to_rebase_commit`. We can skip rebasing if it
// already a child of `new_parents`. Otherwise, at this point, it should no // is already a child of `new_parents`. Otherwise, at this point, it should
// longer have any children; they have all been rebased and the originals // no longer have any children; they have all been rebased and the originals
// have been abandoned. // have been abandoned.
let skipped_commit_rebase = if old_commit.parent_ids() == new_parents { let skipped_commit_rebase = if to_rebase_commit.parent_ids() == new_parents {
if let Some(mut formatter) = ui.status_formatter() { if let Some(mut formatter) = ui.status_formatter() {
write!(formatter, "Skipping rebase of commit ")?; write!(formatter, "Skipping rebase of commit ")?;
tx.write_commit_summary(formatter.as_mut(), &old_commit)?; tx.write_commit_summary(formatter.as_mut(), &to_rebase_commit)?;
writeln!(formatter)?; writeln!(formatter)?;
} }
true true
} else { } else {
rebase_commit(settings, tx.mut_repo(), old_commit, new_parents)?; rebase_commit(settings, tx.mut_repo(), to_rebase_commit, new_parents)?;
debug_assert_eq!(tx.mut_repo().rebase_descendants(settings)?, 0); debug_assert_eq!(tx.mut_repo().rebase_descendants(settings)?, 0);
false false
}; };

1
cli/tests/common/mod.rs
View file

@ -227,6 +227,7 @@ impl TestEnvironment {
/// Run a `jj` command, check that it failed with code 101, and return its /// Run a `jj` command, check that it failed with code 101, and return its
/// stderr /// stderr
#[must_use] #[must_use]
#[allow(dead_code)]
pub fn jj_cmd_panic(&self, current_dir: &Path, args: &[&str]) -> String { pub fn jj_cmd_panic(&self, current_dir: &Path, args: &[&str]) -> String {
let assert = self.jj_cmd(current_dir, args).assert().code(101).stdout(""); let assert = self.jj_cmd(current_dir, args).assert().code(101).stdout("");
self.normalize_output(&get_stderr_string(&assert)) self.normalize_output(&get_stderr_string(&assert))

100
cli/tests/test_rebase_command.rs
View file

@ -310,18 +310,18 @@ fn test_rebase_single_revision() {
insta::assert_snapshot!(stdout, @""); insta::assert_snapshot!(stdout, @"");
insta::assert_snapshot!(stderr, @r###" insta::assert_snapshot!(stderr, @r###"
Also rebased 1 descendant commits onto parent of rebased commit Also rebased 1 descendant commits onto parent of rebased commit
Working copy now at: vruxwmqv bf87078f d | d Working copy now at: vruxwmqv ad122686 d | d
Parent commit : zsuskuln d370aee1 b | b
Parent commit : rlvkpnrz 2443ea76 a | a Parent commit : rlvkpnrz 2443ea76 a | a
Parent commit : zsuskuln d370aee1 b | b
Added 0 files, modified 0 files, removed 1 files Added 0 files, modified 0 files, removed 1 files
"###); "###);
insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###" insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###"
◉ c ◉ c
│ @ d │ @ d
│ ├─╮ │ ├─╮
│ │ ◉ a │ │ ◉ b
├───╯ ├───╯
│ ◉ b │ ◉ a
├─╯ ├─╯
◉ ◉
"###); "###);
@ -354,17 +354,17 @@ fn test_rebase_single_revision_merge_parent() {
insta::assert_snapshot!(stdout, @""); insta::assert_snapshot!(stdout, @"");
insta::assert_snapshot!(stderr, @r###" insta::assert_snapshot!(stderr, @r###"
Also rebased 1 descendant commits onto parent of rebased commit Also rebased 1 descendant commits onto parent of rebased commit
Working copy now at: vruxwmqv c62d0789 d | d Working copy now at: vruxwmqv a37531e8 d | d
Parent commit : zsuskuln d370aee1 b | b
Parent commit : rlvkpnrz 2443ea76 a | a Parent commit : rlvkpnrz 2443ea76 a | a
Parent commit : zsuskuln d370aee1 b | b
Added 0 files, modified 0 files, removed 1 files Added 0 files, modified 0 files, removed 1 files
"###); "###);
insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###" insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###"
◉ c ◉ c
│ @ d │ @ d
╭─┤ ╭─┤
◉ │ a
│ ◉ b │ ◉ b
◉ │ a
├─╯ ├─╯
◉ ◉
"###); "###);
@ -849,7 +849,7 @@ fn test_rebase_with_child_and_descendant_bug_2600() {
insta::assert_snapshot!(stdout, @""); insta::assert_snapshot!(stdout, @"");
insta::assert_snapshot!(stderr, @r###" insta::assert_snapshot!(stderr, @r###"
Skipping rebase of commit rlvkpnrz 0c61db1b base | base Skipping rebase of commit rlvkpnrz 0c61db1b base | base
Rebased 4 descendant commits onto parent of commit Rebased 3 descendant commits onto parent of commit
Working copy now at: vruxwmqv 57aaa944 c | c Working copy now at: vruxwmqv 57aaa944 c | c
Parent commit : royxmykx c8495a71 b | b Parent commit : royxmykx c8495a71 b | b
Added 0 files, modified 0 files, removed 1 files Added 0 files, modified 0 files, removed 1 files
@ -872,74 +872,17 @@ fn test_rebase_with_child_and_descendant_bug_2600() {
◉ ◉
"###); "###);
// TODO(#2650) !!!!! The panic here is a BUG !!!
// This tests the algorithm for rebasing onto descendants. The result should be // This tests the algorithm for rebasing onto descendants. The result should be
// simplified if and only if it's simplified in the above case. // simplified if and only if it's simplified in the above case.
test_env.jj_cmd_ok(&repo_path, &["op", "restore", &setup_opid]); test_env.jj_cmd_ok(&repo_path, &["op", "restore", &setup_opid]);
let stderr = test_env.jj_cmd_panic(&repo_path, &["rebase", "-r", "base", "-d", "b"]); let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["rebase", "-r", "base", "-d", "b"]);
assert!(stderr.contains("graph has cycle")); insta::assert_snapshot!(stdout, @"");
// // At time of writing: insta::assert_snapshot!(stderr, @r###"
// insta::assert_snapshot!(stderr, @r###" Also rebased 3 descendant commits onto parent of rebased commit
// thread 'main' panicked at lib/src/dag_walk.rs:113:13: Working copy now at: vruxwmqv a72f0141 c | c
// graph has cycle Parent commit : royxmykx 7033e775 b | b
// stack backtrace: Added 0 files, modified 0 files, removed 1 files
// 0: rust_begin_unwind "###);
// at
// /rustc/6cf088810f66fff15d05bf7135c5f5888b7c93b4/library/std/src/panicking.rs:
// 645:5 1: core::panicking::panic_fmt
// at
// /rustc/6cf088810f66fff15d05bf7135c5f5888b7c93b4/library/core/src/panicking.
// rs:72:14 2: jj_lib::dag_walk::topo_order_forward_ok
// at
// /usr/local/google/home/ilyagr/dev/jj/lib/src/dag_walk.rs:113:13
// 3: jj_lib::dag_walk::topo_order_reverse_ok
// at
// /usr/local/google/home/ilyagr/dev/jj/lib/src/dag_walk.rs:160:22
// 4: jj_lib::dag_walk::topo_order_reverse
// at
// /usr/local/google/home/ilyagr/dev/jj/lib/src/dag_walk.rs:142:5
// 5: jj_lib::rewrite::DescendantRebaser::new
// at
// /usr/local/google/home/ilyagr/dev/jj/lib/src/rewrite.rs:306:24
// 6: jj_lib::repo::MutableRepo::create_descendant_rebaser
// at /usr/local/google/home/ilyagr/dev/jj/lib/src/repo.rs:844:9
// 7: jj_lib::repo::MutableRepo::rebase_descendants_return_rebaser
// at /usr/local/google/home/ilyagr/dev/jj/lib/src/repo.rs:861:27
// 8: jj_lib::repo::MutableRepo::rebase_descendants_with_options
// at /usr/local/google/home/ilyagr/dev/jj/lib/src/repo.rs:872:12
// 9: jj_lib::repo::MutableRepo::rebase_descendants
// at /usr/local/google/home/ilyagr/dev/jj/lib/src/repo.rs:878:9
// 10: jj_cli::commands::rebase::rebase_revision
// at
// /usr/local/google/home/ilyagr/dev/jj/cli/src/commands/rebase.rs:420:22
// 11: jj_cli::commands::rebase::cmd_rebase
// at
// /usr/local/google/home/ilyagr/dev/jj/cli/src/commands/rebase.rs:197:9
// 12: jj_cli::commands::run_command
// at
// /usr/local/google/home/ilyagr/dev/jj/cli/src/commands/mod.rs:187:39 13:
// core::ops::function::FnOnce::call_once at
// /rustc/6cf088810f66fff15d05bf7135c5f5888b7c93b4/library/core/src/ops/
// function.rs:250:5 14: core::ops::function::FnOnce::call_once{{vtable.
// shim}} at
// /rustc/6cf088810f66fff15d05bf7135c5f5888b7c93b4/library/core/src/ops/
// function.rs:250:5 15: <alloc::boxed::Box<F,A> as
// core::ops::function::FnOnce<Args>>::call_once at
// /rustc/6cf088810f66fff15d05bf7135c5f5888b7c93b4/library/alloc/src/boxed.rs:
// 2007:9 16: jj_cli::cli_util::CliRunner::run_internal
// at
// /usr/local/google/home/ilyagr/dev/jj/cli/src/cli_util.rs:2867:9
// 17: jj_cli::cli_util::CliRunner::run
// at
// /usr/local/google/home/ilyagr/dev/jj/cli/src/cli_util.rs:2884:22
// 18: jj::main
// at /usr/local/google/home/ilyagr/dev/jj/cli/src/main.rs:18:5
// 19: core::ops::function::FnOnce::call_once
// at
// /rustc/6cf088810f66fff15d05bf7135c5f5888b7c93b4/library/core/src/ops/
// function.rs:250:5 note: Some details are omitted, run with
// `RUST_BACKTRACE=full` for a verbose backtrace. "###);
//
// Unsimlified ancestry would look like // Unsimlified ancestry would look like
// @ c // @ c
// │ ◉ base // │ ◉ base
@ -950,12 +893,11 @@ fn test_rebase_with_child_and_descendant_bug_2600() {
// ├─╯ // ├─╯
// ◉ // ◉
insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###" insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###"
@ c
◉ b
├─╮
│ ◉ a
├─╯
◉ base ◉ base
│ @ c
├─╯
◉ b
◉ a
◉ ◉
"###); "###);
@ -965,7 +907,7 @@ fn test_rebase_with_child_and_descendant_bug_2600() {
let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["rebase", "-r", "base", "-d", "a"]); let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["rebase", "-r", "base", "-d", "a"]);
insta::assert_snapshot!(stdout, @""); insta::assert_snapshot!(stdout, @"");
insta::assert_snapshot!(stderr, @r###" insta::assert_snapshot!(stderr, @r###"
Also rebased 4 descendant commits onto parent of rebased commit Also rebased 3 descendant commits onto parent of rebased commit
Working copy now at: vruxwmqv 0b91d0eb c | c Working copy now at: vruxwmqv 0b91d0eb c | c
Parent commit : royxmykx fb944989 b | b Parent commit : royxmykx fb944989 b | b
Added 0 files, modified 0 files, removed 1 files Added 0 files, modified 0 files, removed 1 files