Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rewrite rebasing in jj split using transform_descendants() #3550

Merged
merged 2 commits into from
Apr 21, 2024
Merged

Rewrite rebasing in jj split using transform_descendants() #3550

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6204ebc
Add checks for stderr and stdout to `jj split` tests
emesterhazy Apr 20, 2024
dd643c9
Rewrite rebasing in `jj split` using `transform_descendants()`
emesterhazy Apr 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion cli/src/commands/rebase.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -286,7 +286,7 @@ fn rebase_branch(
}

/// Rebases `old_commits` onto `new_parents`.
pub fn rebase_descendants(
fn rebase_descendants(
tx: &mut WorkspaceCommandTransaction,
settings: &UserSettings,
new_parents: Vec<Commit>,
Expand Down
68 changes: 19 additions & 49 deletions cli/src/commands/split.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,18 +13,13 @@
// limitations under the License.
use std::io::Write;

use itertools::Itertools;
use jj_lib::commit::Commit;
use jj_lib::object_id::ObjectId;
use jj_lib::repo::Repo;
use jj_lib::revset::{RevsetExpression, RevsetIteratorExt};
use jj_lib::rewrite::merge_commit_trees;
use jj_lib::settings::UserSettings;
use tracing::instrument;

use crate::cli_util::{CommandHelper, RevisionArg, WorkspaceCommandTransaction};
use crate::cli_util::{CommandHelper, RevisionArg};
use crate::command_error::CommandError;
use crate::commands::rebase::rebase_descendants;
use crate::description_util::{description_template_for_commit, edit_description};
use crate::ui::Ui;

Expand Down Expand Up @@ -179,16 +174,24 @@ the operation will be aborted.
// commit.
tx.mut_repo()
.set_rewritten_commit(commit.id().clone(), second_commit.id().clone());
let num_rebased = if args.siblings {
rebase_children_for_siblings_split(
&mut tx,
command.settings(),
&commit,
vec![first_commit.clone(), second_commit.clone()],
)?
} else {
tx.mut_repo().rebase_descendants(command.settings())?
};
let mut num_rebased = 0;
tx.mut_repo().transform_descendants(
command.settings(),
vec![commit.id().clone()],
|mut rewriter| {
num_rebased += 1;
if args.siblings {
rewriter.replace_parent(
second_commit.id(),
&[first_commit.id().clone(), second_commit.id().clone()],
);
}
// We don't need to do anything special for the non-siblings case
// since we already marked the original commit as rewritten.
rewriter.rebase(command.settings())?.write()?;
Ok(())
},
)?;

if let Some(mut formatter) = ui.status_formatter() {
if num_rebased > 0 {
Expand All @@ -203,36 +206,3 @@ the operation will be aborted.
tx.finish(ui, format!("split commit {}", commit.id().hex()))?;
Ok(())
}

// Rebases the children of `original_commit` by replacing `original_commit` with
// `new_siblings`. Any parents other than `original_commit` will remain after
// the rebase.
fn rebase_children_for_siblings_split(
tx: &mut WorkspaceCommandTransaction,
settings: &UserSettings,
original_commit: &Commit,
new_siblings: Vec<Commit>,
) -> Result<usize, CommandError> {
let children: Vec<Commit> = RevsetExpression::commit(original_commit.id().clone())
.children()
.evaluate_programmatic(tx.base_repo().as_ref())?
.iter()
.commits(tx.base_repo().store())
.try_collect()?;
let mut num_rebased = 0;
for child in children {
let new_parents = child
.parents()
.into_iter()
.flat_map(|c| {
if c.id() == original_commit.id() {
new_siblings.clone()
} else {
vec![c]
}
})
.collect_vec();
num_rebased += rebase_descendants(tx, settings, new_parents, &[child], Default::default())?;
}
Ok(num_rebased)
}
100 changes: 78 additions & 22 deletions cli/tests/test_split_command.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -173,7 +173,14 @@ fn test_split_with_non_empty_description() {
.join("\0"),
)
.unwrap();
test_env.jj_cmd_ok(&workspace_path, &["split", "file1"]);
let (stdout, stderr) = test_env.jj_cmd_ok(&workspace_path, &["split", "file1"]);
insta::assert_snapshot!(stdout, @"");
insta::assert_snapshot!(stderr, @r###"
First part: qpvuntsm 41e04d04 part 1
Second part: kkmpptxz 093b6c0d part 2
Working copy now at: kkmpptxz 093b6c0d part 2
Parent commit : qpvuntsm 41e04d04 part 1
"###);

assert_eq!(
std::fs::read_to_string(test_env.env_root().join("editor1")).unwrap(),
Expand Down Expand Up @@ -224,7 +231,14 @@ fn test_split_with_default_description() {
["dump editor1", "next invocation\n", "dump editor2"].join("\0"),
)
.unwrap();
test_env.jj_cmd_ok(&workspace_path, &["split", "file1"]);
let (stdout, stderr) = test_env.jj_cmd_ok(&workspace_path, &["split", "file1"]);
insta::assert_snapshot!(stdout, @"");
insta::assert_snapshot!(stderr, @r###"
First part: qpvuntsm 5afe936c TESTED=TODO
Second part: kkmpptxz 0e09a2df test_branch | (no description set)
Working copy now at: kkmpptxz 0e09a2df test_branch | (no description set)
Parent commit : qpvuntsm 5afe936c TESTED=TODO
"###);

// Since the commit being split has no description, the user will only be
// prompted to add a description to the first commit, which will use the
Expand Down Expand Up @@ -281,7 +295,17 @@ fn test_split_with_merge_child() {
["write\nAdd file1", "next invocation\n", "write\nAdd file2"].join("\0"),
)
.unwrap();
test_env.jj_cmd_ok(&workspace_path, &["split", "-r", "description(a)", "file1"]);
let (stdout, stderr) =
test_env.jj_cmd_ok(&workspace_path, &["split", "-r", "description(a)", "file1"]);
insta::assert_snapshot!(stdout, @"");
insta::assert_snapshot!(stderr, @r###"
Rebased 1 descendant commits
First part: kkmpptxz e8006b47 Add file1
Second part: royxmykx 5e1b793d Add file2
Working copy now at: zsuskuln 0315e471 (empty) 2
Parent commit : qpvuntsm dc0e5d61 (empty) 1
Parent commit : royxmykx 5e1b793d Add file2
"###);
insta::assert_snapshot!(get_log_output(&test_env, &workspace_path), @r###"
@ zsuskulnrvyr true 2
├─╮
Expand Down Expand Up @@ -319,7 +343,21 @@ fn test_split_siblings_no_descendants() {
["dump editor1", "next invocation\n", "dump editor2"].join("\0"),
)
.unwrap();
test_env.jj_cmd_ok(&workspace_path, &["split", "--siblings", "file1"]);
let (stdout, stderr) = test_env.jj_cmd_ok(&workspace_path, &["split", "--siblings", "file1"]);
insta::assert_snapshot!(stdout, @"");
insta::assert_snapshot!(stderr, @r###"
First part: qpvuntsm 8d2b7558 TESTED=TODO
Second part: zsuskuln acd41528 test_branch | (no description set)
Working copy now at: zsuskuln acd41528 test_branch | (no description set)
Parent commit : zzzzzzzz 00000000 (empty) (no description set)
Added 0 files, modified 0 files, removed 1 files
"###);
insta::assert_snapshot!(get_log_output(&test_env, &workspace_path), @r###"
@ zsuskulnrvyr false test_branch
│ ◉ qpvuntsmwlqt false TESTED=TODO
├─╯
◉ zzzzzzzzzzzz true
"###);

// Since the commit being split has no description, the user will only be
// prompted to add a description to the first commit, which will use the
Expand All @@ -338,12 +376,6 @@ JJ: Lines starting with "JJ: " (like this one) will be removed.
"#
);
assert!(!test_env.env_root().join("editor2").exists());
insta::assert_snapshot!(get_log_output(&test_env, &workspace_path), @r###"
@ zsuskulnrvyr false test_branch
│ ◉ qpvuntsmwlqt false TESTED=TODO
├─╯
◉ zzzzzzzzzzzz true
"###);
}

#[test]
Expand All @@ -368,6 +400,12 @@ fn test_split_siblings_with_descendants() {
// to the split command.
test_env.jj_cmd_ok(&workspace_path, &["prev", "--edit"]);
test_env.jj_cmd_ok(&workspace_path, &["prev", "--edit"]);
insta::assert_snapshot!(get_log_output(&test_env, &workspace_path), @r###"
◉ kkmpptxzrspx false Add file4
◉ rlvkpnrzqnoo false Add file3
@ qpvuntsmwlqt false Add file1 & file2
◉ zzzzzzzzzzzz true
"###);

// Set up the editor and do the split.
let edit_script = test_env.set_up_fake_editor();
Expand All @@ -383,7 +421,25 @@ fn test_split_siblings_with_descendants() {
.join("\0"),
)
.unwrap();
test_env.jj_cmd_ok(&workspace_path, &["split", "--siblings", "file1"]);
let (stdout, stderr) = test_env.jj_cmd_ok(&workspace_path, &["split", "--siblings", "file1"]);
insta::assert_snapshot!(stdout, @"");
insta::assert_snapshot!(stderr, @r###"
Rebased 2 descendant commits
First part: qpvuntsm 27b151c3 Add file1
Second part: vruxwmqv c0857cfb Add file2
Working copy now at: vruxwmqv c0857cfb Add file2
Parent commit : zzzzzzzz 00000000 (empty) (no description set)
Added 0 files, modified 0 files, removed 1 files
"###);
insta::assert_snapshot!(get_log_output(&test_env, &workspace_path), @r###"
◉ kkmpptxzrspx false Add file4
◉ rlvkpnrzqnoo false Add file3
├─╮
│ @ vruxwmqvtpmx false Add file2
◉ │ qpvuntsmwlqt false Add file1
├─╯
◉ zzzzzzzzzzzz true
"###);

// The commit we're splitting has a description, so the user will be
// prompted to enter a description for each of the sibling commits.
Expand All @@ -409,16 +465,6 @@ JJ: A file2
JJ: Lines starting with "JJ: " (like this one) will be removed.
"#
);

insta::assert_snapshot!(get_log_output(&test_env, &workspace_path), @r###"
◉ kkmpptxzrspx false Add file4
◉ rlvkpnrzqnoo false Add file3
├─╮
│ @ yqosqzytrlsw false Add file2
◉ │ qpvuntsmwlqt false Add file1
├─╯
◉ zzzzzzzzzzzz true
"###);
}

// This test makes sure that the children of the commit being split retain any
Expand Down Expand Up @@ -452,10 +498,20 @@ fn test_split_siblings_with_merge_child() {
["write\nAdd file1", "next invocation\n", "write\nAdd file2"].join("\0"),
)
.unwrap();
test_env.jj_cmd_ok(
let (stdout, stderr) = test_env.jj_cmd_ok(
&workspace_path,
&["split", "-r", "description(a)", "--siblings", "file1"],
);
insta::assert_snapshot!(stdout, @"");
insta::assert_snapshot!(stderr, @r###"
Rebased 1 descendant commits
First part: kkmpptxz e8006b47 Add file1
Second part: royxmykx 2cc60f3d Add file2
Working copy now at: zsuskuln 2f04d1d1 (empty) 2
Parent commit : qpvuntsm dc0e5d61 (empty) 1
Parent commit : kkmpptxz e8006b47 Add file1
Parent commit : royxmykx 2cc60f3d Add file2
"###);
insta::assert_snapshot!(get_log_output(&test_env, &workspace_path), @r###"
@ zsuskulnrvyr true 2
├─┬─╮
Expand Down