From 50b375da20563e3a7daf152f63516e7b25543dcc Mon Sep 17 00:00:00 2001 From: Scott Taylor Date: Mon, 8 Jul 2024 08:28:40 -0500 Subject: [PATCH] cli: skip formatting instructions if not required `tx.format_commit_summary()` can be expensive because it needs to build an IdPrefixContext now, so it's best to avoid formatting instruction messages unless they are actually required. --- cli/src/cli_util.rs | 4 +-- cli/src/commands/commit.rs | 12 ++++---- cli/src/commands/diffedit.rs | 12 ++++---- cli/src/commands/split.rs | 12 ++++---- cli/src/commands/squash.rs | 14 +++++---- cli/src/commands/unsquash.rs | 14 +++++---- cli/src/merge_tools/mod.rs | 6 ++-- cli/testing/fake-diff-editor.rs | 6 +++- cli/tests/test_commit_command.rs | 12 +++++++- cli/tests/test_diffedit_command.rs | 16 +++++++++- cli/tests/test_split_command.rs | 48 ++++++++++++++++++++++++++++++ cli/tests/test_squash_command.rs | 16 ++++++++++ cli/tests/test_unsquash_command.rs | 15 ++++++++++ 13 files changed, 152 insertions(+), 35 deletions(-) diff --git a/cli/src/cli_util.rs b/cli/src/cli_util.rs index 9bf5ad47ac..08f54cd23e 100644 --- a/cli/src/cli_util.rs +++ b/cli/src/cli_util.rs @@ -2248,12 +2248,12 @@ impl DiffSelector { left_tree: &MergedTree, right_tree: &MergedTree, matcher: &dyn Matcher, - instructions: Option<&str>, + format_instructions: impl FnOnce() -> String, ) -> Result { match self { DiffSelector::NonInteractive => Ok(restore_tree(right_tree, left_tree, matcher)?), DiffSelector::Interactive(editor) => { - Ok(editor.edit(left_tree, right_tree, matcher, instructions)?) + Ok(editor.edit(left_tree, right_tree, matcher, format_instructions)?) } } } diff --git a/cli/src/commands/commit.rs b/cli/src/commands/commit.rs index e173032153..6ec05a9036 100644 --- a/cli/src/commands/commit.rs +++ b/cli/src/commands/commit.rs @@ -71,21 +71,23 @@ pub(crate) fn cmd_commit( workspace_command.diff_selector(ui, args.tool.as_deref(), args.interactive)?; let mut tx = workspace_command.start_transaction(); let base_tree = commit.parent_tree(tx.repo())?; - let instructions = format!( - "\ + let format_instructions = || { + format!( + "\ You are splitting the working-copy commit: {} The diff initially shows all changes. Adjust the right side until it shows the contents you want for the first commit. The remainder will be included in the new working-copy commit. ", - tx.format_commit_summary(&commit) - ); + tx.format_commit_summary(&commit) + ) + }; let tree_id = diff_selector.select( &base_tree, &commit.tree()?, matcher.as_ref(), - Some(&instructions), + format_instructions, )?; let middle_tree = tx.repo().store().get_root_tree(&tree_id)?; if !args.paths.is_empty() && middle_tree.id() == base_tree.id() { diff --git a/cli/src/commands/diffedit.rs b/cli/src/commands/diffedit.rs index 7aba4a0c12..3e5efefefd 100644 --- a/cli/src/commands/diffedit.rs +++ b/cli/src/commands/diffedit.rs @@ -89,19 +89,21 @@ pub(crate) fn cmd_diffedit( let diff_editor = workspace_command.diff_editor(ui, args.tool.as_deref())?; let mut tx = workspace_command.start_transaction(); - let instructions = format!( - "\ + let format_instructions = || { + format!( + "\ You are editing changes in: {} {diff_description} Adjust the right side until it shows the contents you want. If you don't make any changes, then the operation will be aborted.", - tx.format_commit_summary(&target_commit), - ); + tx.format_commit_summary(&target_commit), + ) + }; let base_tree = merge_commit_trees(tx.repo(), base_commits.as_slice())?; let tree = target_commit.tree()?; - let tree_id = diff_editor.edit(&base_tree, &tree, &EverythingMatcher, Some(&instructions))?; + let tree_id = diff_editor.edit(&base_tree, &tree, &EverythingMatcher, format_instructions)?; if tree_id == *target_commit.tree_id() { writeln!(ui.status(), "Nothing changed.")?; } else { diff --git a/cli/src/commands/split.rs b/cli/src/commands/split.rs index f0d2b27932..41ba9ce281 100644 --- a/cli/src/commands/split.rs +++ b/cli/src/commands/split.rs @@ -88,8 +88,9 @@ pub(crate) fn cmd_split( let mut tx = workspace_command.start_transaction(); let end_tree = commit.tree()?; let base_tree = commit.parent_tree(tx.repo())?; - let instructions = format!( - "\ + let format_instructions = || { + format!( + "\ You are splitting a commit into two: {} The diff initially shows the changes in the commit you're splitting. @@ -98,12 +99,13 @@ Adjust the right side until it shows the contents you want for the first commit. The remainder will be in the second commit. If you don't make any changes, then the operation will be aborted. ", - tx.format_commit_summary(&commit) - ); + tx.format_commit_summary(&commit) + ) + }; // Prompt the user to select the changes they want for the first commit. let selected_tree_id = - diff_selector.select(&base_tree, &end_tree, matcher.as_ref(), Some(&instructions))?; + diff_selector.select(&base_tree, &end_tree, matcher.as_ref(), format_instructions)?; if &selected_tree_id == commit.tree_id() && diff_selector.is_interactive() { // The user selected everything from the original commit. writeln!(ui.status(), "Nothing changed.")?; diff --git a/cli/src/commands/squash.rs b/cli/src/commands/squash.rs index c325a584d5..84fa140e44 100644 --- a/cli/src/commands/squash.rs +++ b/cli/src/commands/squash.rs @@ -196,8 +196,9 @@ pub fn move_diff( for source in sources { let parent_tree = source.parent_tree(tx.repo())?; let source_tree = source.tree()?; - let instructions = format!( - "\ + let format_instructions = || { + format!( + "\ You are moving changes from: {} into commit: {} @@ -209,11 +210,12 @@ 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) - ); + tx.format_commit_summary(source), + tx.format_commit_summary(destination) + ) + }; let selected_tree_id = - diff_selector.select(&parent_tree, &source_tree, matcher, Some(&instructions))?; + diff_selector.select(&parent_tree, &source_tree, matcher, format_instructions)?; let selected_tree = tx.repo().store().get_root_tree(&selected_tree_id)?; let abandon = !keep_emptied && selected_tree.id() == source_tree.id(); if !abandon && selected_tree_id == parent_tree.id() { diff --git a/cli/src/commands/unsquash.rs b/cli/src/commands/unsquash.rs index 3cb3c9aad8..1b6c1ea0fc 100644 --- a/cli/src/commands/unsquash.rs +++ b/cli/src/commands/unsquash.rs @@ -71,8 +71,9 @@ pub(crate) fn cmd_unsquash( let parent_base_tree = parent.parent_tree(tx.repo())?; let new_parent_tree_id; if let Some(diff_editor) = &interactive_editor { - let instructions = format!( - "\ + let format_instructions = || { + format!( + "\ You are moving changes from: {} into its child: {} @@ -83,15 +84,16 @@ the parent commit. The changes you edited out will be moved into the child commit. If you don't make any changes, then the operation will be aborted. ", - tx.format_commit_summary(&parent), - tx.format_commit_summary(&commit) - ); + tx.format_commit_summary(&parent), + tx.format_commit_summary(&commit) + ) + }; let parent_tree = parent.tree()?; new_parent_tree_id = diff_editor.edit( &parent_base_tree, &parent_tree, &EverythingMatcher, - Some(&instructions), + format_instructions, )?; if new_parent_tree_id == parent_base_tree.id() { return Err(user_error("No changes selected")); diff --git a/cli/src/merge_tools/mod.rs b/cli/src/merge_tools/mod.rs index b23832fc63..e73304e73b 100644 --- a/cli/src/merge_tools/mod.rs +++ b/cli/src/merge_tools/mod.rs @@ -219,20 +219,20 @@ impl DiffEditor { left_tree: &MergedTree, right_tree: &MergedTree, matcher: &dyn Matcher, - instructions: Option<&str>, + format_instructions: impl FnOnce() -> String, ) -> Result { match &self.tool { MergeTool::Builtin => { Ok(edit_diff_builtin(left_tree, right_tree, matcher).map_err(Box::new)?) } MergeTool::External(editor) => { - let instructions = self.use_instructions.then_some(instructions).flatten(); + let instructions = self.use_instructions.then(format_instructions); edit_diff_external( editor, left_tree, right_tree, matcher, - instructions, + instructions.as_deref(), self.base_ignores.clone(), ) } diff --git a/cli/testing/fake-diff-editor.rs b/cli/testing/fake-diff-editor.rs index da7a5c79f8..f398ff2362 100644 --- a/cli/testing/fake-diff-editor.rs +++ b/cli/testing/fake-diff-editor.rs @@ -57,7 +57,7 @@ fn files_recursively(p: &Path) -> HashSet { fn main() { let args: Args = Args::parse(); let edit_script_path = PathBuf::from(std::env::var_os("DIFF_EDIT_SCRIPT").unwrap()); - let edit_script = String::from_utf8(std::fs::read(edit_script_path).unwrap()).unwrap(); + let edit_script = String::from_utf8(std::fs::read(&edit_script_path).unwrap()).unwrap(); for instruction in edit_script.split('\0') { let (command, payload) = instruction.split_once('\n').unwrap_or((instruction, "")); let parts = command.split(' ').collect_vec(); @@ -111,6 +111,10 @@ fn main() { std::fs::remove_file(args.after.join(file)).unwrap(); } } + ["dump", file, dest] => { + let dest_path = edit_script_path.parent().unwrap().join(dest); + std::fs::copy(args.after.join(file), dest_path).unwrap(); + } ["write", file] => { std::fs::write(args.after.join(file), payload).unwrap(); } diff --git a/cli/tests/test_commit_command.rs b/cli/tests/test_commit_command.rs index ffe797cd09..370787a020 100644 --- a/cli/tests/test_commit_command.rs +++ b/cli/tests/test_commit_command.rs @@ -104,11 +104,21 @@ fn test_commit_interactive() { std::fs::write(edit_script, ["dump editor"].join("\0")).unwrap(); let diff_editor = test_env.set_up_fake_diff_editor(); - std::fs::write(diff_editor, "rm file2").unwrap(); + let diff_script = ["rm file2", "dump JJ-INSTRUCTIONS instrs"].join("\0"); + std::fs::write(diff_editor, diff_script).unwrap(); // Create a commit interactively and select only file1 test_env.jj_cmd_ok(&workspace_path, &["commit", "-i"]); + insta::assert_snapshot!( + std::fs::read_to_string(test_env.env_root().join("instrs")).unwrap(), @r###" + You are splitting the working-copy commit: qpvuntsm 4219467e add files + + The diff initially shows all changes. Adjust the right side until it shows the + contents you want for the first commit. The remainder will be included in the + new working-copy commit. + "###); + insta::assert_snapshot!( std::fs::read_to_string(test_env.env_root().join("editor")).unwrap(), @r###" add files diff --git a/cli/tests/test_diffedit_command.rs b/cli/tests/test_diffedit_command.rs index 28985c9b8a..1469001802 100644 --- a/cli/tests/test_diffedit_command.rs +++ b/cli/tests/test_diffedit_command.rs @@ -33,7 +33,12 @@ fn test_diffedit() { // Test the setup; nothing happens if we make no changes std::fs::write( &edit_script, - "files-before file1 file2\0files-after JJ-INSTRUCTIONS file2", + [ + "files-before file1 file2", + "files-after JJ-INSTRUCTIONS file2", + "dump JJ-INSTRUCTIONS instrs", + ] + .join("\0"), ) .unwrap(); let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["diffedit"]); @@ -41,6 +46,15 @@ fn test_diffedit() { insta::assert_snapshot!(stderr, @r###" Nothing changed. "###); + insta::assert_snapshot!( + std::fs::read_to_string(test_env.env_root().join("instrs")).unwrap(), @r###" + You are editing changes in: kkmpptxz 3d4cce89 (no description set) + + The diff initially shows the commit's changes. + + Adjust the right side until it shows the contents you want. If you + don't make any changes, then the operation will be aborted. + "###); let stdout = test_env.jj_cmd_success(&repo_path, &["diff", "-s"]); insta::assert_snapshot!(stdout, @r###" D file1 diff --git a/cli/tests/test_split_command.rs b/cli/tests/test_split_command.rs index 86906a05b3..715032a1ce 100644 --- a/cli/tests/test_split_command.rs +++ b/cli/tests/test_split_command.rs @@ -559,3 +559,51 @@ fn test_split_message_editor_avoids_unc() { // over 260 chars. assert_eq!(edited_path, dunce::simplified(&edited_path)); } + +#[test] +fn test_split_interactive() { + let mut test_env = TestEnvironment::default(); + test_env.jj_cmd_ok(test_env.env_root(), &["git", "init", "repo"]); + 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 editor"].join("\0")).unwrap(); + + let diff_editor = test_env.set_up_fake_diff_editor(); + let diff_script = ["rm file2", "dump JJ-INSTRUCTIONS instrs"].join("\0"); + std::fs::write(diff_editor, diff_script).unwrap(); + + // Split the working commit interactively and select only file1 + let (stdout, stderr) = test_env.jj_cmd_ok(&workspace_path, &["split"]); + + insta::assert_snapshot!( + std::fs::read_to_string(test_env.env_root().join("instrs")).unwrap(), @r###" + You are splitting a commit into two: qpvuntsm 44af2155 (no description set) + + 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 commit. + The remainder will be in the second commit. If you don't make any changes, then + the operation will be aborted. + "###); + + insta::assert_snapshot!( + std::fs::read_to_string(test_env.env_root().join("editor")).unwrap(), @r###" + JJ: Enter a description for the first commit. + + JJ: This commit contains the following changes: + JJ: A file1 + + JJ: Lines starting with "JJ: " (like this one) will be removed. + "###); + + insta::assert_snapshot!(stdout, @""); + insta::assert_snapshot!(stderr, @r###" + First part: qpvuntsm 0e15949e (no description set) + Second part: rlvkpnrz 9ed12e4c (no description set) + Working copy now at: rlvkpnrz 9ed12e4c (no description set) + Parent commit : qpvuntsm 0e15949e (no description set) + "###); +} diff --git a/cli/tests/test_squash_command.rs b/cli/tests/test_squash_command.rs index 058113bd5d..57d2ab6583 100644 --- a/cli/tests/test_squash_command.rs +++ b/cli/tests/test_squash_command.rs @@ -157,6 +157,7 @@ fn test_squash_partial() { // If we don't make any changes in the diff-editor, the whole change is moved // into the parent let edit_script = test_env.set_up_fake_diff_editor(); + std::fs::write(&edit_script, "dump JJ-INSTRUCTIONS instrs").unwrap(); let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["squash", "-r", "b", "-i"]); insta::assert_snapshot!(stdout, @""); insta::assert_snapshot!(stderr, @r###" @@ -164,6 +165,21 @@ fn test_squash_partial() { Working copy now at: mzvwutvl 3c633226 c | (no description set) Parent commit : qpvuntsm 38ffd8b9 a b | (no description set) "###); + + insta::assert_snapshot!( + std::fs::read_to_string(test_env.env_root().join("instrs")).unwrap(), @r###" + You are moving changes from: kkmpptxz d117da27 b | (no description set) + into commit: qpvuntsm 54d3c1c0 a | (no description set) + + 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. + "###); + insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###" @ 3c6332267ea8 c ◉ 38ffd8b98578 a b diff --git a/cli/tests/test_unsquash_command.rs b/cli/tests/test_unsquash_command.rs index 2e638965eb..9234755a4a 100644 --- a/cli/tests/test_unsquash_command.rs +++ b/cli/tests/test_unsquash_command.rs @@ -156,6 +156,7 @@ fn test_unsquash_partial() { // If we don't make any changes in the diff-editor, the whole change is moved // from the parent let edit_script = test_env.set_up_fake_diff_editor(); + std::fs::write(&edit_script, "dump JJ-INSTRUCTIONS instrs").unwrap(); let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["unsquash", "-r", "b", "-i"]); insta::assert_snapshot!(stdout, @""); insta::assert_snapshot!(stderr, @r###" @@ -163,6 +164,20 @@ fn test_unsquash_partial() { Working copy now at: mzvwutvl 8802263d c | (no description set) Parent commit : kkmpptxz 5bd83140 b | (no description set) "###); + + insta::assert_snapshot!( + std::fs::read_to_string(test_env.env_root().join("instrs")).unwrap(), @r###" + You are moving changes from: qpvuntsm 54d3c1c0 a | (no description set) + into its child: kkmpptxz d117da27 b | (no description set) + + The diff initially shows the parent commit's changes. + + Adjust the right side until it shows the contents you want to keep in + the parent commit. The changes you edited out will be moved into the + child commit. If you don't make any changes, then the operation will be + aborted. + "###); + insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###" @ 8802263dbd92 c ◉ 5bd83140fd47 b