From fa5eaa0f94fb5bb8624213c74aa8893d00340fd1 Mon Sep 17 00:00:00 2001 From: Martin von Zweigbergk Date: Sun, 10 Mar 2024 21:46:21 -0700 Subject: [PATCH] squash: accept multiple `--from` revisions Now you can do e.g. `jj squash --from 'foo+::' --into foo` to squash a whole series into one commit. It doesn't need to be linear; you can squash a bunch of siblings into another siblings, for example. --- CHANGELOG.md | 3 +- cli/src/commands/move.rs | 4 +- cli/src/commands/squash.rs | 136 +++++++++------- cli/tests/test_squash_command.rs | 268 +++++++++++++++++++++++++++++++ 4 files changed, 348 insertions(+), 63 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 169f2650d7..e7513f1e11 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -24,7 +24,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 elided. * `jj squash` now accepts `--from` and `--into` (mutually exclusive with `-r`). - It can thereby be for all use cases where `jj move` can be used. + It can thereby be for all use cases where `jj move` can be used. The `--from` + argument accepts a revset that resolves to move than one revision. ### Fixed bugs diff --git a/cli/src/commands/move.rs b/cli/src/commands/move.rs index 8b2f892dcb..0759655be4 100644 --- a/cli/src/commands/move.rs +++ b/cli/src/commands/move.rs @@ -89,8 +89,8 @@ pub(crate) fn cmd_move( ui, &mut tx, command.settings(), - source, - destination, + &[source], + &destination, matcher.as_ref(), &diff_selector, None, diff --git a/cli/src/commands/squash.rs b/cli/src/commands/squash.rs index 918be7515a..bccb0adec5 100644 --- a/cli/src/commands/squash.rs +++ b/cli/src/commands/squash.rs @@ -80,20 +80,26 @@ pub(crate) fn cmd_squash( ) -> Result<(), CommandError> { let mut workspace_command = command.workspace_helper(ui)?; - let source; + let mut sources; let destination; if args.from.is_some() || args.into.is_some() { - source = workspace_command.resolve_single_rev(args.from.as_deref().unwrap_or("@"))?; + sources = workspace_command.resolve_revset(args.from.as_deref().unwrap_or("@"))?; destination = workspace_command.resolve_single_rev(args.into.as_deref().unwrap_or("@"))?; - if source.id() == destination.id() { + if sources.iter().any(|source| source.id() == destination.id()) { return Err(user_error("Source and destination cannot be the same")); } + // Reverse the set so we apply the oldest commits first. It shouldn't affect the + // result, but it avoids creating transient conflicts and is therefore probably + // a little faster. + sources.reverse(); } else { - source = workspace_command.resolve_single_rev(args.revision.as_deref().unwrap_or("@"))?; + let source = + workspace_command.resolve_single_rev(args.revision.as_deref().unwrap_or("@"))?; let mut parents = source.parents(); if parents.len() != 1 { return Err(user_error("Cannot squash merge commits")); } + sources = vec![source]; destination = parents.pop().unwrap(); } @@ -101,15 +107,15 @@ pub(crate) fn cmd_squash( let diff_selector = workspace_command.diff_selector(ui, args.tool.as_deref(), args.interactive)?; let mut tx = workspace_command.start_transaction(); - let tx_description = format!("squash commit {}", source.id().hex()); + let tx_description = format!("squash commits into {}", destination.id().hex()); let description = (!args.message_paragraphs.is_empty()) .then(|| join_message_paragraphs(&args.message_paragraphs)); move_diff( ui, &mut tx, command.settings(), - source, - destination, + &sources, + &destination, matcher.as_ref(), &diff_selector, description, @@ -125,8 +131,8 @@ pub fn move_diff( ui: &mut Ui, tx: &mut WorkspaceCommandTransaction, settings: &UserSettings, - source: Commit, - mut destination: Commit, + sources: &[Commit], + destination: &Commit, matcher: &dyn Matcher, diff_selector: &DiffSelector, description: Option, @@ -134,28 +140,51 @@ pub fn move_diff( path_arg: &[String], ) -> Result<(), CommandError> { tx.base_workspace_helper() - .check_rewritable([&source, &destination])?; - let parent_tree = merge_commit_trees(tx.repo(), &source.parents())?; - let source_tree = source.tree()?; - let instructions = format!( - "\ -You are moving changes from: {} -into commit: {} + .check_rewritable(sources.iter().chain(std::iter::once(destination)))?; + // Tree diffs to apply to the destination + let mut tree_diffs = vec![]; + let mut abandoned_commits = vec![]; + for source in sources { + let parent_tree = merge_commit_trees(tx.repo(), &source.parents())?; + let source_tree = source.tree()?; + let instructions = format!( + "You are moving changes from: {} + into commit: {} + + The left side of the diff shows the contents of the parent commit. The + right side initially shows the contents of the commit you're moving + changes from. + + Adjust the right side until the diff shows the changes you want to move + to the destination. If you don't make any changes, then all the changes + from the source will be moved into the destination. + ", + tx.format_commit_summary(source), + tx.format_commit_summary(destination) + ); + let new_parent_tree_id = + diff_selector.select(&parent_tree, &source_tree, matcher, Some(&instructions))?; + let new_parent_tree = tx.repo().store().get_root_tree(&new_parent_tree_id)?; + // TODO: Do we want to optimize the case of moving to the parent commit (`jj + // squash -r`)? The source tree will be unchanged in that case. -The left side of the diff shows the contents of the parent commit. The -right side initially shows the contents of the commit you're moving -changes from. - -Adjust the right side until the diff shows the changes you want to move -to the destination. If you don't make any changes, then all the changes -from the source will be moved into the destination. -", - tx.format_commit_summary(&source), - tx.format_commit_summary(&destination) - ); - let new_parent_tree_id = - diff_selector.select(&parent_tree, &source_tree, matcher, Some(&instructions))?; - if new_parent_tree_id == parent_tree.id() { + // Apply the reverse of the selected changes onto the source + let new_source_tree = source_tree.merge(&new_parent_tree, &parent_tree)?; + let abandon_source = new_source_tree.id() == parent_tree.id(); + if abandon_source { + abandoned_commits.push(source); + tx.mut_repo().record_abandoned_commit(source.id().clone()); + } else { + tx.mut_repo() + .rewrite_commit(settings, source) + .set_tree_id(new_source_tree.id().clone()) + .write()?; + } + if new_parent_tree_id != parent_tree.id() { + tree_diffs.push((parent_tree, new_parent_tree)); + } + } + if tree_diffs.is_empty() { if diff_selector.is_interactive() { return Err(user_error("No changes selected")); } @@ -176,47 +205,34 @@ from the source will be moved into the destination. } } } - let new_parent_tree = tx.repo().store().get_root_tree(&new_parent_tree_id)?; - // TODO: Do we want to optimize the case of moving to the parent commit (`jj - // squash -r`)? The source tree will be unchanged in that case. - - // Apply the reverse of the selected changes onto the source - let new_source_tree = source_tree.merge(&new_parent_tree, &parent_tree)?; - let abandon_source = new_source_tree.id() == parent_tree.id(); - if abandon_source { - tx.mut_repo().record_abandoned_commit(source.id().clone()); - } else { - tx.mut_repo() - .rewrite_commit(settings, &source) - .set_tree_id(new_source_tree.id().clone()) - .write()?; - } - if tx.repo().index().is_ancestor(source.id(), destination.id()) { + let mut rewritten_destination = destination.clone(); + if sources + .iter() + .any(|source| tx.repo().index().is_ancestor(source.id(), destination.id())) + { // If we're moving changes to a descendant, first rebase descendants onto the - // rewritten source. Otherwise it will likely already have the content + // rewritten sources. Otherwise it will likely already have the content // changes we're moving, so applying them will have no effect and the // changes will disappear. let rebase_map = tx.mut_repo().rebase_descendants_return_map(settings)?; let rebased_destination_id = rebase_map.get(destination.id()).unwrap().clone(); - destination = tx.mut_repo().store().get_commit(&rebased_destination_id)?; + rewritten_destination = tx.mut_repo().store().get_commit(&rebased_destination_id)?; } // Apply the selected changes onto the destination - let destination_tree = destination.tree()?; - let new_destination_tree = destination_tree.merge(&parent_tree, &new_parent_tree)?; + let mut destination_tree = rewritten_destination.tree()?; + for (tree1, tree2) in tree_diffs { + destination_tree = destination_tree.merge(&tree1, &tree2)?; + } let description = match description { Some(description) => description, - None => { - if abandon_source { - combine_messages(tx.base_repo(), &[&source], &destination, settings)? - } else { - destination.description().to_owned() - } - } + None => combine_messages(tx.base_repo(), &abandoned_commits, destination, settings)?, }; + let mut predecessors = vec![destination.id().clone()]; + predecessors.extend(sources.iter().map(|source| source.id().clone())); tx.mut_repo() - .rewrite_commit(settings, &destination) - .set_tree_id(new_destination_tree.id().clone()) - .set_predecessors(vec![destination.id().clone(), source.id().clone()]) + .rewrite_commit(settings, &rewritten_destination) + .set_tree_id(destination_tree.id().clone()) + .set_predecessors(predecessors) .set_description(description) .write()?; Ok(()) diff --git a/cli/tests/test_squash_command.rs b/cli/tests/test_squash_command.rs index ab084e422c..24daa6a5c6 100644 --- a/cli/tests/test_squash_command.rs +++ b/cli/tests/test_squash_command.rs @@ -582,6 +582,274 @@ fn test_squash_from_to_partial() { "###); } +#[test] +fn test_squash_from_multiple() { + let test_env = TestEnvironment::default(); + test_env.jj_cmd_ok(test_env.env_root(), &["init", "repo", "--git"]); + let repo_path = test_env.env_root().join("repo"); + + // Create history like this: + // F + // | + // E + // /|\ + // B C D + // \|/ + // A + let file = repo_path.join("file"); + test_env.jj_cmd_ok(&repo_path, &["branch", "create", "a"]); + std::fs::write(&file, "a\n").unwrap(); + test_env.jj_cmd_ok(&repo_path, &["new"]); + test_env.jj_cmd_ok(&repo_path, &["branch", "create", "b"]); + std::fs::write(&file, "b\n").unwrap(); + test_env.jj_cmd_ok(&repo_path, &["new", "@-"]); + test_env.jj_cmd_ok(&repo_path, &["branch", "create", "c"]); + std::fs::write(&file, "c\n").unwrap(); + test_env.jj_cmd_ok(&repo_path, &["new", "@-"]); + test_env.jj_cmd_ok(&repo_path, &["branch", "create", "d"]); + std::fs::write(&file, "d\n").unwrap(); + test_env.jj_cmd_ok(&repo_path, &["new", "all:visible_heads()"]); + test_env.jj_cmd_ok(&repo_path, &["branch", "create", "e"]); + std::fs::write(&file, "e\n").unwrap(); + test_env.jj_cmd_ok(&repo_path, &["new"]); + test_env.jj_cmd_ok(&repo_path, &["branch", "create", "f"]); + std::fs::write(&file, "f\n").unwrap(); + // Test the setup + insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###" + @ 7c982f87d244 f + ◉ 90fb23310e1d e + ├─┬─╮ + │ │ ◉ 512dff087306 b + │ ◉ │ 5ee503da2262 c + │ ├─╯ + ◉ │ cb214cffd91a d + ├─╯ + ◉ 37941ee54ace a + ◉ 000000000000 + "###); + + // Squash a few commits sideways + let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["squash", "--from=b|c", "--into=d"]); + insta::assert_snapshot!(stdout, @""); + insta::assert_snapshot!(stderr, @r###" + Rebased 2 descendant commits + New conflicts appeared in these commits: + yqosqzyt d5401742 d | (conflict) (no description set) + To resolve the conflicts, start by updating to it: + jj new yqosqzytrlsw + Then use `jj resolve`, or edit the conflict markers in the file directly. + Once the conflicts are resolved, you may want inspect the result with `jj diff`. + Then run `jj squash` to move the resolution into the conflicted commit. + Working copy now at: kpqxywon cc9f4cad f | (no description set) + Parent commit : yostqsxw 9f25b62d e | (no description set) + "###); + insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###" + @ cc9f4cad1a29 f + ◉ 9f25b62ddffc e + ├─╮ + ◉ │ d54017421f3f d + ├─╯ + ◉ 37941ee54ace a b c + ◉ 000000000000 + "###); + // The changes from the sources have been applied + let stdout = test_env.jj_cmd_success(&repo_path, &["print", "-r=d", "file"]); + insta::assert_snapshot!(stdout, @r###" + <<<<<<< + %%%%%%% + -a + +d + %%%%%%% + -a + +b + +++++++ + c + >>>>>>> + "###); + + // Squash a few commits up an down + test_env.jj_cmd_ok(&repo_path, &["undo"]); + let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["squash", "--from=b|c|f", "--into=e"]); + insta::assert_snapshot!(stdout, @""); + insta::assert_snapshot!(stderr, @r###" + Rebased 1 descendant commits + Working copy now at: xznxytkn 59801ce3 (empty) (no description set) + Parent commit : yostqsxw b7bc1dda e f | (no description set) + "###); + insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###" + @ 59801ce3ff81 + ◉ b7bc1dda247e e f + ├─╮ + ◉ │ cb214cffd91a d + ├─╯ + ◉ 37941ee54ace a b c + ◉ 000000000000 + "###); + // The changes from the sources have been applied to the destination + let stdout = test_env.jj_cmd_success(&repo_path, &["print", "-r=e", "file"]); + insta::assert_snapshot!(stdout, @r###" + f + "###); +} + +#[test] +fn test_squash_from_multiple_partial() { + let test_env = TestEnvironment::default(); + test_env.jj_cmd_ok(test_env.env_root(), &["init", "repo", "--git"]); + let repo_path = test_env.env_root().join("repo"); + + // Create history like this: + // F + // | + // E + // /|\ + // B C D + // \|/ + // A + let file1 = repo_path.join("file1"); + let file2 = repo_path.join("file2"); + test_env.jj_cmd_ok(&repo_path, &["branch", "create", "a"]); + std::fs::write(&file1, "a\n").unwrap(); + std::fs::write(&file2, "a\n").unwrap(); + test_env.jj_cmd_ok(&repo_path, &["new"]); + test_env.jj_cmd_ok(&repo_path, &["branch", "create", "b"]); + std::fs::write(&file1, "b\n").unwrap(); + std::fs::write(&file2, "b\n").unwrap(); + test_env.jj_cmd_ok(&repo_path, &["new", "@-"]); + test_env.jj_cmd_ok(&repo_path, &["branch", "create", "c"]); + std::fs::write(&file1, "c\n").unwrap(); + std::fs::write(&file2, "c\n").unwrap(); + test_env.jj_cmd_ok(&repo_path, &["new", "@-"]); + test_env.jj_cmd_ok(&repo_path, &["branch", "create", "d"]); + std::fs::write(&file1, "d\n").unwrap(); + std::fs::write(&file2, "d\n").unwrap(); + test_env.jj_cmd_ok(&repo_path, &["new", "all:visible_heads()"]); + test_env.jj_cmd_ok(&repo_path, &["branch", "create", "e"]); + std::fs::write(&file1, "e\n").unwrap(); + std::fs::write(&file2, "e\n").unwrap(); + test_env.jj_cmd_ok(&repo_path, &["new"]); + test_env.jj_cmd_ok(&repo_path, &["branch", "create", "f"]); + std::fs::write(&file1, "f\n").unwrap(); + std::fs::write(&file2, "f\n").unwrap(); + // Test the setup + insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###" + @ 5adc4b1fb0f9 f + ◉ 8ba764396a28 e + ├─┬─╮ + │ │ ◉ 2a2d19a3283f b + │ ◉ │ 864a16169cef c + │ ├─╯ + ◉ │ 5def0e76dfaf d + ├─╯ + ◉ 47a1e795d146 a + ◉ 000000000000 + "###); + + // Partially squash a few commits sideways + let (stdout, stderr) = + test_env.jj_cmd_ok(&repo_path, &["squash", "--from=b|c", "--into=d", "file1"]); + insta::assert_snapshot!(stdout, @""); + insta::assert_snapshot!(stderr, @r###" + Rebased 2 descendant commits + New conflicts appeared in these commits: + yqosqzyt 13468b54 d | (conflict) (no description set) + To resolve the conflicts, start by updating to it: + jj new yqosqzytrlsw + Then use `jj resolve`, or edit the conflict markers in the file directly. + Once the conflicts are resolved, you may want inspect the result with `jj diff`. + Then run `jj squash` to move the resolution into the conflicted commit. + Working copy now at: kpqxywon 8aaa7910 f | (no description set) + Parent commit : yostqsxw 5aad25ea e | (no description set) + "###); + insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###" + @ 8aaa79109163 f + ◉ 5aad25eae5aa e + ├─┬─╮ + │ │ ◉ ba60ddff2d41 b + │ ◉ │ 8ef5a315bf7d c + │ ├─╯ + ◉ │ 13468b546ba3 d + ├─╯ + ◉ 47a1e795d146 a + ◉ 000000000000 + "###); + // The selected changes have been removed from the sources + let stdout = test_env.jj_cmd_success(&repo_path, &["print", "-r=b", "file1"]); + insta::assert_snapshot!(stdout, @r###" + a + "###); + let stdout = test_env.jj_cmd_success(&repo_path, &["print", "-r=c", "file1"]); + insta::assert_snapshot!(stdout, @r###" + a + "###); + // The selected changes from the sources have been applied + let stdout = test_env.jj_cmd_success(&repo_path, &["print", "-r=d", "file1"]); + insta::assert_snapshot!(stdout, @r###" + <<<<<<< + %%%%%%% + -a + +d + %%%%%%% + -a + +b + +++++++ + c + >>>>>>> + "###); + // The unselected change from the sources have not been applied to the + // destination + let stdout = test_env.jj_cmd_success(&repo_path, &["print", "-r=d", "file2"]); + insta::assert_snapshot!(stdout, @r###" + d + "###); + + // Partially squash a few commits up an down + test_env.jj_cmd_ok(&repo_path, &["undo"]); + let (stdout, stderr) = + test_env.jj_cmd_ok(&repo_path, &["squash", "--from=b|c|f", "--into=e", "file1"]); + insta::assert_snapshot!(stdout, @""); + insta::assert_snapshot!(stderr, @r###" + Rebased 1 descendant commits + Working copy now at: kpqxywon 610a144d f | (no description set) + Parent commit : yostqsxw ac27a136 e | (no description set) + "###); + insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###" + @ 610a144de39b f + ◉ ac27a1361b09 e + ├─┬─╮ + │ │ ◉ 0c8eab864a32 b + │ ◉ │ ad1776ad0b1b c + │ ├─╯ + ◉ │ 5def0e76dfaf d + ├─╯ + ◉ 47a1e795d146 a + ◉ 000000000000 + "###); + // The selected changes have been removed from the sources + let stdout = test_env.jj_cmd_success(&repo_path, &["print", "-r=b", "file1"]); + insta::assert_snapshot!(stdout, @r###" + a + "###); + let stdout = test_env.jj_cmd_success(&repo_path, &["print", "-r=c", "file1"]); + insta::assert_snapshot!(stdout, @r###" + a + "###); + let stdout = test_env.jj_cmd_success(&repo_path, &["print", "-r=f", "file1"]); + insta::assert_snapshot!(stdout, @r###" + f + "###); + // The selected changes from the sources have been applied to the destination + let stdout = test_env.jj_cmd_success(&repo_path, &["print", "-r=e", "file1"]); + insta::assert_snapshot!(stdout, @r###" + f + "###); + // The unselected changes from the sources have not been applied + let stdout = test_env.jj_cmd_success(&repo_path, &["print", "-r=d", "file2"]); + insta::assert_snapshot!(stdout, @r###" + d + "###); +} + fn get_log_output(test_env: &TestEnvironment, repo_path: &Path) -> String { let template = r#"commit_id.short() ++ " " ++ branches"#; test_env.jj_cmd_success(repo_path, &["log", "-T", template])