Skip to content

Commit

Permalink
WIP: Add a --siblings option to the jj split command
Browse files Browse the repository at this point in the history 
If the --siblings option is used, the target commit is split into two sibling
commits instead of parent and child commits. Any children of the original commit
will have both sibilings as their new parents.


ISSUE=#2274
  • Loading branch information
@emesterhazy
emesterhazy committed Feb 13, 2024
1 parent 2701791 commit e265980
Show file tree
Hide file tree
Showing 3 changed files with 196 additions and 20 deletions.
53 changes: 43 additions & 10 deletions cli/src/commands/split.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,9 @@ pub(crate) struct SplitArgs {
/// The revision to split
#[arg(long, short, default_value = "@")]
revision: RevisionArg,
/// Split the revision into two siblings instead of a parent and child.
#[arg(long, short)]
siblings: bool,
/// Put these paths in the first commit
#[arg(value_hint = clap::ValueHint::AnyPath)]
paths: Vec<String>,
Expand All @@ -69,12 +72,13 @@ You are splitting a commit in two: {}
The diff initially shows the changes in the commit you're splitting.
Adjust the right side until it shows the contents you want for the first
(parent) commit. The remainder will be in the second commit. If you
{} commit. The remainder will be in the {} commit. If you
don't make any changes, then the operation will be aborted.
",
tx.format_commit_summary(&commit)
tx.format_commit_summary(&commit),
if args.siblings { "" } else { "(parent)" },
if args.siblings { "sibling" } else { "child" }
);

// Prompt the user to select the changes they want for the first commit.
let selected_tree_id = tx.select_diff(
ui,
Expand Down Expand Up @@ -104,7 +108,7 @@ don't make any changes, then the operation will be aborted.
ui,
command.settings(),
tx.base_workspace_helper(),
"Enter commit description for the first part (parent).",
"Enter a description for the first commit.",
commit.description(),
&base_tree,
&selected_tree,
Expand All @@ -118,6 +122,20 @@ don't make any changes, then the operation will be aborted.
.write()?;

// Create the second commit, which includes everything the user didn't select.
// TODO(emesterhazy): Maybe there's a prettier way to write this or we could
// move the commit creation code to helper functions.
let (second_tree, second_base_tree, second_commit_parents) = if args.siblings {
(
// Merge the original commit tree with its parent using the tree
// containing the user selected changes as the base for the merge.
// This results in a tree with the changes the user didn't select.
end_tree.merge(&selected_tree, &base_tree)?,
&base_tree,
commit.parent_ids().to_vec(),
)
} else {
(end_tree, &selected_tree, vec![first_commit.id().clone()])
};
let second_description = if commit.description().is_empty() {
// If there was no description before, don't ask for one for the second commit.
"".to_string()
Expand All @@ -126,28 +144,43 @@ don't make any changes, then the operation will be aborted.
ui,
command.settings(),
tx.base_workspace_helper(),
"Enter commit description for the second part (child).",
"Enter a description for the second commit.",
commit.description(),
&selected_tree,
&end_tree,
second_base_tree,
&second_tree,
)?;
edit_description(tx.base_repo(), &second_template, command.settings())?
};
let second_commit = tx
.mut_repo()
.rewrite_commit(command.settings(), &commit)
.set_parents(vec![first_commit.id().clone()])
.set_tree_id(commit.tree_id().clone())
.set_parents(second_commit_parents)
.set_tree_id(second_tree.id())
// Generate a new change id so that the commit being split doesn't
// become divergent.
.generate_new_change_id()
.set_description(second_description)
.write()?;

// Currently, `rebase_descendents` would treat `commit` as being rewritten to
// *both* `first_commit` and `second_commit`, as if it was becoming divergent.
// However, we want only the `second_commit` to inherit `commit`'s branches and
// descendants.
// descendants, so we omit the first commit from the `new_ids` argument.
// TODO(emesterhazy): This comment is a little bit misleading. From what I
// can tell, if we remove the call to set_rewritten_commit here
// rebase_descendants doesn't mark the commit as divergent. However,
// without this call, descendants won't be rebased onto the second commit,
// and if the --siblings argument is used @ won't be updated to the second
// commit instead of the rewritten first commit, which is the current
// behavior when --siblings isn't specified.
tx.mut_repo()
.set_rewritten_commit(commit.id().clone(), [second_commit.id().clone()]);
if args.siblings {
// TODO(emesterhazy): We need to rebase the descendents of the commit
// being split so that the children of the split commit have both
// siblings as parents.
// let new_parents = [first_commit.id().clone(), second_commit.id().clone()];
}
let num_rebased = tx.mut_repo().rebase_descendants(command.settings())?;
if num_rebased > 0 {
writeln!(ui.stderr(), "Rebased {num_rebased} descendant commits")?;
Expand Down
4 changes: 4 additions & 0 deletions cli/tests/[email protected]
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1650,6 +1650,10 @@ If the change you split had a description, you will be asked to enter a change d
* `-r`, `--revision <REVISION>` — The revision to split
Default value: `@`
* `-s`, `--siblings` — Split the revision into two siblings instead of a parent and child
Possible values: `true`, `false`
Expand Down
159 changes: 149 additions & 10 deletions cli/tests/test_split_command.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,16 @@ use std::path::Path;

use crate::common::TestEnvironment;

fn get_log_output(test_env: &TestEnvironment, cwd: &Path) -> String {
let template = r#"separate(" ", change_id.short(), empty, description)"#;
test_env.jj_cmd_success(cwd, &["log", "-T", template])
}

fn get_recorded_dates(test_env: &TestEnvironment, cwd: &Path, revset: &str) -> String {
let template = r#"separate("\n", "Author date: " ++ author.timestamp(), "Committer date: " ++ committer.timestamp())"#;
test_env.jj_cmd_success(cwd, &["log", "--no-graph", "-T", template, "-r", revset])
}

#[test]
fn test_split_by_paths() {
let mut test_env = TestEnvironment::default();
Expand Down Expand Up @@ -51,7 +61,7 @@ fn test_split_by_paths() {
"###);
insta::assert_snapshot!(
std::fs::read_to_string(test_env.env_root().join("editor0")).unwrap(), @r###"
JJ: Enter commit description for the first part (parent).
JJ: Enter a description for the first commit.
JJ: This commit contains the following changes:
JJ: A file2
Expand Down Expand Up @@ -167,7 +177,7 @@ fn test_split_with_non_empty_description() {

assert_eq!(
std::fs::read_to_string(test_env.env_root().join("editor1")).unwrap(),
r#"JJ: Enter commit description for the first part (parent).
r#"JJ: Enter a description for the first commit.
test
JJ: This commit contains the following changes:
Expand All @@ -178,7 +188,7 @@ JJ: Lines starting with "JJ: " (like this one) will be removed.
);
assert_eq!(
std::fs::read_to_string(test_env.env_root().join("editor2")).unwrap(),
r#"JJ: Enter commit description for the second part (child).
r#"JJ: Enter a description for the second commit.
test
JJ: This commit contains the following changes:
Expand Down Expand Up @@ -211,9 +221,13 @@ fn test_split_with_default_description() {
.unwrap();
test_env.jj_cmd_ok(&workspace_path, &["split", "file1"]);

// 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
// default value we set. The second commit will inherit the empty
// description from the commit being split.
assert_eq!(
std::fs::read_to_string(test_env.env_root().join("editor1")).unwrap(),
r#"JJ: Enter commit description for the first part (parent).
r#"JJ: Enter a description for the first commit.
TESTED=TODO
Expand All @@ -231,12 +245,137 @@ JJ: Lines starting with "JJ: " (like this one) will be removed.
"###);
}

fn get_log_output(test_env: &TestEnvironment, cwd: &Path) -> String {
let template = r#"separate(" ", change_id.short(), empty, description)"#;
test_env.jj_cmd_success(cwd, &["log", "-T", template])
#[test]
// Split a commit with no descendants into siblings. Also tests that the default
// description is set correctly on the first commit.
fn test_split_siblings_no_descendants() {
let mut test_env = TestEnvironment::default();
test_env.jj_cmd_ok(test_env.env_root(), &["init", "repo", "--git"]);
test_env.add_config(r#"ui.default-description = "\n\nTESTED=TODO""#);
let workspace_path = test_env.env_root().join("repo");

std::fs::write(workspace_path.join("file1"), "foo\n").unwrap();
std::fs::write(workspace_path.join("file2"), "bar\n").unwrap();
let edit_script = test_env.set_up_fake_editor();
std::fs::write(
edit_script,
["dump editor1", "next invocation\n", "dump editor2"].join("\0"),
)
.unwrap();
test_env.jj_cmd_ok(&workspace_path, &["split", "--siblings", "file1"]);

// 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
// default value we set. The second commit will inherit the empty
// description from the commit being split.
assert_eq!(
std::fs::read_to_string(test_env.env_root().join("editor1")).unwrap(),
r#"JJ: Enter a description for the first commit.
TESTED=TODO
JJ: This commit contains the following changes:
JJ: A file1
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###"
@ rlvkpnrzqnoo false
│ ◉ qpvuntsmwlqt false TESTED=TODO
├─╯
◉ zzzzzzzzzzzz true
"###);
}

fn get_recorded_dates(test_env: &TestEnvironment, cwd: &Path, revset: &str) -> String {
let template = r#"separate("\n", "Author date: " ++ author.timestamp(), "Committer date: " ++ committer.timestamp())"#;
test_env.jj_cmd_success(cwd, &["log", "--no-graph", "-T", template, "-r", revset])
#[test]
fn test_split_siblings_with_descendants() {
// Configure the environment and make the initial commits.
let mut test_env = TestEnvironment::default();
test_env.jj_cmd_ok(test_env.env_root(), &["init", "repo", "--git"]);
// test_env.add_config(r#"ui.default-description = "\n\nTESTED=TODO""#);
let workspace_path = test_env.env_root().join("repo");

// First commit. This is the one we will split later.
std::fs::write(workspace_path.join("file1"), "foo\n").unwrap();
std::fs::write(workspace_path.join("file2"), "bar\n").unwrap();
test_env.jj_cmd_ok(&workspace_path, &["commit", "-m", "Add file1 & file2"]);
// Second commit. This will be the child of the sibling commits after the split.
std::fs::write(workspace_path.join("file3"), "baz\n").unwrap();
test_env.jj_cmd_ok(&workspace_path, &["commit", "-m", "Add file3"]);
// Third commit.
std::fs::write(workspace_path.join("file4"), "foobarbaz\n").unwrap();
test_env.jj_cmd_ok(&workspace_path, &["describe", "-m", "Add file4"]);
// Move back to the previous commit so that we don't have to pass a revision
// to the split command.
test_env.jj_cmd_ok(&workspace_path, &["prev", "--edit"]);
test_env.jj_cmd_ok(&workspace_path, &["prev", "--edit"]);

// Set up the editor and do the split.
let edit_script = test_env.set_up_fake_editor();
std::fs::write(
edit_script,
[
"dump editor1",
"write\nAdd file1",
"next invocation\n",
"dump editor2",
"write\nAdd file2",
]
.join("\0"),
)
.unwrap();
test_env.jj_cmd_ok(&workspace_path, &["split", "--siblings", "file1"]);

// TODO(emesterhazy): Delete these commands. They were only used to generate the
// expected output before I finished implementing the feature.
// test_env.jj_cmd_ok(&workspace_path, &["rebase", "-s", "yqosq", "-d", "zzzz"]);
// test_env.jj_cmd_ok(
// &workspace_path,
// &["rebase", "-s", "rlvk", "-d", "qpvun", "-d", "yqos"],
// );

// The commit we're splitting has a description, so the user will be
// prompted to enter a description for each of the sibling commits.
assert_eq!(
std::fs::read_to_string(test_env.env_root().join("editor1")).unwrap(),
r#"JJ: Enter a description for the first commit.
Add file1 & file2
JJ: This commit contains the following changes:
JJ: A file1
JJ: Lines starting with "JJ: " (like this one) will be removed.
"#
);
assert_eq!(
std::fs::read_to_string(test_env.env_root().join("editor2")).unwrap(),
r#"JJ: Enter a description for the second commit.
Add file1 & file2
JJ: This commit contains the following changes:
JJ: A file2
JJ: Lines starting with "JJ: " (like this one) will be removed.
"#
);

// TODO(emesterhazy): Replace with the log output below. Until the feature
// is working, it's easier to see what's happening with an empty output.
insta::assert_snapshot!(get_log_output(&test_env, &workspace_path), @"");

// TODO(emesterhazy): I think that the final state should look like this,
// but I'm not 100% sure this is correct.
/*
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
"###);
*/
}

0 comments on commit e265980

Please sign in to comment.