From a592815270aa83d6890e82c34f14a8211bf079d8 Mon Sep 17 00:00:00 2001 From: Bryce Berger Date: Fri, 7 Feb 2025 16:18:27 -0500 Subject: [PATCH] cli: split: edit both descriptions at once closes #5432 Since #3828, it's been possible to describe multiple commits in the same editor. This makes that the default method for `jj split`. --- CHANGELOG.md | 2 + cli/src/commands/describe.rs | 35 ++---- cli/src/commands/split.rs | 59 ++++----- cli/src/description_util.rs | 37 +++++- cli/tests/test_split_command.rs | 206 +++++++++++++++++++------------- 5 files changed, 193 insertions(+), 146 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index fc45958b25..dbe25573d5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -23,6 +23,8 @@ to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). `ellipsis` parameter; passing this prepends or appends the ellipsis to the content if it is truncated to fit the maximum width. +* `jj split` now edits both commit descriptions at the same time. + ### Fixed bugs * `jj status` now shows untracked files under untracked directories. diff --git a/cli/src/commands/describe.rs b/cli/src/commands/describe.rs index 814b45a7cb..ad3c324241 100644 --- a/cli/src/commands/describe.rs +++ b/cli/src/commands/describe.rs @@ -25,14 +25,12 @@ use tracing::instrument; use crate::cli_util::CommandHelper; use crate::cli_util::RevisionArg; -use crate::command_error::user_error; use crate::command_error::CommandError; use crate::complete; use crate::description_util::description_template; use crate::description_util::edit_description; use crate::description_util::edit_multiple_descriptions; use crate::description_util::join_message_paragraphs; -use crate::description_util::ParsedBulkEditMessage; use crate::text_util::parse_author; use crate::ui::Ui; @@ -199,32 +197,13 @@ pub(crate) fn cmd_describe( let description = edit_description(&text_editor, &template)?; vec![(&commits[0], description)] } else { - let ParsedBulkEditMessage { - descriptions, - missing, - duplicates, - unexpected, - } = edit_multiple_descriptions(ui, &text_editor, &tx, &temp_commits)?; - if !missing.is_empty() { - return Err(user_error(format!( - "The description for the following commits were not found in the edited \ - message: {}", - missing.join(", ") - ))); - } - if !duplicates.is_empty() { - return Err(user_error(format!( - "The following commits were found in the edited message multiple times: {}", - duplicates.join(", ") - ))); - } - if !unexpected.is_empty() { - return Err(user_error(format!( - "The following commits were not being edited, but were found in the edited \ - message: {}", - unexpected.join(", ") - ))); - } + let descriptions = edit_multiple_descriptions( + ui, + &text_editor, + &tx, + temp_commits.iter().map(|(id, commit)| (*id, commit, "")), + )? + .assert_complete()?; let commit_descriptions = commits .iter() diff --git a/cli/src/commands/split.rs b/cli/src/commands/split.rs index 06e8e57c94..3f224386c9 100644 --- a/cli/src/commands/split.rs +++ b/cli/src/commands/split.rs @@ -24,8 +24,7 @@ use crate::cli_util::RevisionArg; use crate::command_error::user_error_with_hint; use crate::command_error::CommandError; use crate::complete; -use crate::description_util::description_template; -use crate::description_util::edit_description; +use crate::description_util::edit_multiple_descriptions; use crate::ui::Ui; /// Split a revision in two @@ -137,27 +136,18 @@ The remainder will be in the second commit. // Create the first commit, which includes the changes selected by the user. let selected_tree = tx.repo().store().get_root_tree(&selected_tree_id)?; - let first_commit = { + let (first_commit, mut first_builder) = { let mut commit_builder = tx.repo_mut().rewrite_commit(&commit).detach(); commit_builder.set_tree_id(selected_tree_id); if commit_builder.description().is_empty() { commit_builder.set_description(tx.settings().get_string("ui.default-description")?); } - let temp_commit = commit_builder.write_hidden()?; - let template = description_template( - ui, - &tx, - "Enter a description for the first commit.", - &temp_commit, - )?; - let description = edit_description(&text_editor, &template)?; - commit_builder.set_description(description); - commit_builder.write(tx.repo_mut())? + (commit_builder.write_hidden()?, commit_builder) }; // Create the second commit, which includes everything the user didn't // select. - let second_commit = { + let (second_commit, mut second_builder) = { let new_tree = if args.parallel { // Merge the original commit tree with its parent using the tree // containing the user selected changes as the base for the merge. @@ -178,22 +168,33 @@ The remainder will be in the second commit. // Generate a new change id so that the commit being split doesn't // become divergent. .generate_new_change_id(); - let description = if commit.description().is_empty() { - // If there was no description before, don't ask for one for the - // second commit. - "".to_string() - } else { - let temp_commit = commit_builder.write_hidden()?; - let template = description_template( - ui, - &tx, - "Enter a description for the second commit.", - &temp_commit, - )?; - edit_description(&text_editor, &template)? + (commit_builder.write_hidden()?, commit_builder) + }; + + let commits = [ + ( + first_commit.id(), + &first_commit, + "Enter a description for the first commit.", + ), + ( + second_commit.id(), + &second_commit, + "Enter a description for the second commit.", + ), + ]; + let descriptions = + edit_multiple_descriptions(ui, &text_editor, &tx, commits)?.assert_complete()?; + + first_builder.set_description(&descriptions[first_commit.id()]); + let first_commit = first_builder.write(tx.repo_mut())?; + + let second_commit = { + if !args.parallel { + second_builder.set_parents(vec![first_commit.id().clone()]); }; - commit_builder.set_description(description); - commit_builder.write(tx.repo_mut())? + second_builder.set_description(&descriptions[second_commit.id()]); + second_builder.write(tx.repo_mut())? }; // Mark the commit being split as rewritten to the second commit. As a diff --git a/cli/src/description_util.rs b/cli/src/description_util.rs index c81d0c1e3f..bbf9479ca4 100644 --- a/cli/src/description_util.rs +++ b/cli/src/description_util.rs @@ -21,6 +21,7 @@ use thiserror::Error; use crate::cli_util::short_commit_hash; use crate::cli_util::WorkspaceCommandTransaction; +use crate::command_error::user_error; use crate::command_error::CommandError; use crate::config::CommandNameAndArgs; use crate::formatter::PlainTextFormatter; @@ -169,11 +170,11 @@ JJ: Lines starting with "JJ:" (like this one) will be removed. } /// Edits the descriptions of the given commits in a single editor session. -pub fn edit_multiple_descriptions( +pub fn edit_multiple_descriptions<'a>( ui: &Ui, editor: &TextEditor, tx: &WorkspaceCommandTransaction, - commits: &[(&CommitId, Commit)], + commits: impl IntoIterator, ) -> Result, CommandError> { let mut commits_map = IndexMap::new(); let mut bulk_message = String::new(); @@ -185,13 +186,13 @@ pub fn edit_multiple_descriptions( JJ: - The syntax of the separator lines may change in the future. "#}); - for (commit_id, temp_commit) in commits { + for (commit_id, temp_commit, intro) in commits { let commit_hash = short_commit_hash(commit_id); bulk_message.push_str("JJ: describe "); bulk_message.push_str(&commit_hash); bulk_message.push_str(" -------\n"); - commits_map.insert(commit_hash, *commit_id); - let template = description_template(ui, tx, "", temp_commit)?; + commits_map.insert(commit_hash, commit_id); + let template = description_template(ui, tx, intro, temp_commit)?; bulk_message.push_str(&template); bulk_message.push('\n'); } @@ -219,6 +220,32 @@ pub struct ParsedBulkEditMessage { pub unexpected: Vec, } +impl ParsedBulkEditMessage { + pub fn assert_complete(self) -> Result, CommandError> { + if !self.missing.is_empty() { + return Err(user_error(format!( + "The description for the following commits were not found in the edited message: \ + {}", + self.missing.join(", ") + ))); + } + if !self.duplicates.is_empty() { + return Err(user_error(format!( + "The following commits were found in the edited message multiple times: {}", + self.duplicates.join(", ") + ))); + } + if !self.unexpected.is_empty() { + return Err(user_error(format!( + "The following commits were not being edited, but were found in the edited \ + message: {}", + self.unexpected.join(", ") + ))); + } + Ok(self.descriptions) + } +} + #[derive(Debug, Error, PartialEq)] pub enum ParseBulkEditMessageError { #[error(r#"Found the following line without a commit header: "{0}""#)] diff --git a/cli/tests/test_split_command.rs b/cli/tests/test_split_command.rs index dfede90b10..d712bb6bb4 100644 --- a/cli/tests/test_split_command.rs +++ b/cli/tests/test_split_command.rs @@ -15,6 +15,8 @@ use std::path::Path; use std::path::PathBuf; +use indoc::indoc; + use crate::common::TestEnvironment; fn get_log_output(test_env: &TestEnvironment, cwd: &Path) -> String { @@ -47,11 +49,7 @@ fn test_split_by_paths() { "###); let edit_script = test_env.set_up_fake_editor(); - std::fs::write( - edit_script, - ["dump editor0", "next invocation\n", "dump editor1"].join("\0"), - ) - .unwrap(); + std::fs::write(edit_script, "dump editor0").unwrap(); let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["split", "file2"]); insta::assert_snapshot!(stdout, @""); insta::assert_snapshot!(stderr, @r###" @@ -61,15 +59,27 @@ fn test_split_by_paths() { Parent commit : qpvuntsm 65569ca7 (no description set) "###); insta::assert_snapshot!( - std::fs::read_to_string(test_env.env_root().join("editor0")).unwrap(), @r###" + std::fs::read_to_string(test_env.env_root().join("editor0")).unwrap(), @r#" + JJ: Enter or edit commit descriptions after the `JJ: describe` lines. + JJ: Warning: + JJ: - The text you enter will be lost on a syntax error. + JJ: - The syntax of the separator lines may change in the future. + + JJ: describe 65569ca7d29a ------- JJ: Enter a description for the first commit. JJ: This commit contains the following changes: JJ: A file2 - JJ: Lines starting with "JJ:" (like this one) will be removed. - "###); - assert!(!test_env.env_root().join("editor1").exists()); + JJ: describe 709756f072b5 ------- + JJ: Enter a description for the second commit. + + JJ: This commit contains the following changes: + JJ: A file1 + JJ: A file3 + + JJ: Lines starting with "JJ: " (like this one) will be removed. + "#); insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###" @ zsuskulnrvyr false @@ -165,14 +175,13 @@ fn test_split_with_non_empty_description() { let edit_script = test_env.set_up_fake_editor(); std::fs::write( edit_script, - [ - "dump editor1", - "write\npart 1", - "next invocation\n", - "dump editor2", - "write\npart 2", - ] - .join("\0"), + indoc! {" + dump editor0\0write + JJ: describe fa3ee36bd4d9 ------- + part 1 + JJ: describe d05a16a4de09 ------- + part 2 + "}, ) .unwrap(); let (stdout, stderr) = test_env.jj_cmd_ok(&workspace_path, &["split", "file1"]); @@ -185,25 +194,28 @@ fn test_split_with_non_empty_description() { "###); insta::assert_snapshot!( - std::fs::read_to_string(test_env.env_root().join("editor1")).unwrap(), @r###" + std::fs::read_to_string(test_env.env_root().join("editor0")).unwrap(), @r#" + JJ: Enter or edit commit descriptions after the `JJ: describe` lines. + JJ: Warning: + JJ: - The text you enter will be lost on a syntax error. + JJ: - The syntax of the separator lines may change in the future. + + JJ: describe fa3ee36bd4d9 ------- JJ: Enter a description for the first commit. test JJ: This commit contains the following changes: JJ: A file1 - JJ: Lines starting with "JJ:" (like this one) will be removed. - "###); - insta::assert_snapshot!( - std::fs::read_to_string(test_env.env_root().join("editor2")).unwrap(), @r###" + JJ: describe d05a16a4de09 ------- JJ: Enter a description for the second commit. test JJ: This commit contains the following changes: JJ: A file2 - JJ: Lines starting with "JJ:" (like this one) will be removed. - "###); + JJ: Lines starting with "JJ: " (like this one) will be removed. + "#); insta::assert_snapshot!(get_log_output(&test_env, &workspace_path), @r###" @ kkmpptxzrspx false part 2 ○ qpvuntsmwlqt false part 1 @@ -226,11 +238,7 @@ fn test_split_with_default_description() { test_env.jj_cmd_ok(&workspace_path, &["bookmark", "create", "test_bookmark"]); let edit_script = test_env.set_up_fake_editor(); - std::fs::write( - edit_script, - ["dump editor1", "next invocation\n", "dump editor2"].join("\0"), - ) - .unwrap(); + std::fs::write(edit_script, "dump editor1").unwrap(); let (stdout, stderr) = test_env.jj_cmd_ok(&workspace_path, &["split", "file1"]); insta::assert_snapshot!(stdout, @""); insta::assert_snapshot!(stderr, @r###" @@ -245,7 +253,13 @@ fn test_split_with_default_description() { // default value we set. The second commit will inherit the empty // description from the commit being split. insta::assert_snapshot!( - std::fs::read_to_string(test_env.env_root().join("editor1")).unwrap(), @r###" + std::fs::read_to_string(test_env.env_root().join("editor1")).unwrap(), @r#" + JJ: Enter or edit commit descriptions after the `JJ: describe` lines. + JJ: Warning: + JJ: - The text you enter will be lost on a syntax error. + JJ: - The syntax of the separator lines may change in the future. + + JJ: describe 467285938c22 ------- JJ: Enter a description for the first commit. @@ -254,9 +268,14 @@ fn test_split_with_default_description() { 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()); + JJ: describe 08aee9104ba5 ------- + JJ: Enter a description for the second commit. + + JJ: This commit contains the following changes: + 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 test_bookmark ○ qpvuntsmwlqt false TESTED=TODO @@ -292,7 +311,13 @@ fn test_split_with_merge_child() { let edit_script = test_env.set_up_fake_editor(); std::fs::write( edit_script, - ["write\nAdd file1", "next invocation\n", "write\nAdd file2"].join("\0"), + indoc! {" + dump editor0\0write + JJ: describe 3b5c1be21ddf ------- + Add file1 + JJ: describe e378455660e7 ------- + Add file2 + "}, ) .unwrap(); let (stdout, stderr) = @@ -338,11 +363,7 @@ fn test_split_siblings_no_descendants() { "###); let edit_script = test_env.set_up_fake_editor(); - std::fs::write( - edit_script, - ["dump editor1", "next invocation\n", "dump editor2"].join("\0"), - ) - .unwrap(); + std::fs::write(edit_script, "dump editor1").unwrap(); let (stdout, stderr) = test_env.jj_cmd_ok(&workspace_path, &["split", "--parallel", "file1"]); insta::assert_snapshot!(stdout, @""); insta::assert_snapshot!(stderr, @r###" @@ -364,7 +385,13 @@ fn test_split_siblings_no_descendants() { // default value we set. The second commit will inherit the empty // description from the commit being split. insta::assert_snapshot!( - std::fs::read_to_string(test_env.env_root().join("editor1")).unwrap(), @r###" + std::fs::read_to_string(test_env.env_root().join("editor1")).unwrap(), @r#" + JJ: Enter or edit commit descriptions after the `JJ: describe` lines. + JJ: Warning: + JJ: - The text you enter will be lost on a syntax error. + JJ: - The syntax of the separator lines may change in the future. + + JJ: describe f52ac84bdacc ------- JJ: Enter a description for the first commit. @@ -373,9 +400,14 @@ fn test_split_siblings_no_descendants() { 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()); + JJ: describe 0473f01417d0 ------- + JJ: Enter a description for the second commit. + + JJ: This commit contains the following changes: + JJ: A file2 + + JJ: Lines starting with "JJ: " (like this one) will be removed. + "#); } #[test] @@ -383,7 +415,6 @@ 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(), &["git", "init", "repo"]); - // 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. @@ -411,14 +442,13 @@ fn test_split_siblings_with_descendants() { 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"), + indoc! {" + write + JJ: describe 062d3d1685f2 ------- + Add file1 + JJ: describe eb4ac857d4be ------- + Add file2 + "}, ) .unwrap(); let (stdout, stderr) = test_env.jj_cmd_ok(&workspace_path, &["split", "--parallel", "file1"]); @@ -440,29 +470,6 @@ fn test_split_siblings_with_descendants() { ├─╯ ◆ 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. - insta::assert_snapshot!( - 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. - "###); - insta::assert_snapshot!( - 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. - "###); } // This test makes sure that the children of the commit being split retain any @@ -493,7 +500,13 @@ fn test_split_siblings_with_merge_child() { let edit_script = test_env.set_up_fake_editor(); std::fs::write( edit_script, - ["write\nAdd file1", "next invocation\n", "write\nAdd file2"].join("\0"), + indoc! {" + write + JJ: describe 3b5c1be21ddf ------- + Add file1 + JJ: describe 3ca0c93006a0 ------- + Add file2 + "}, ) .unwrap(); let (stdout, stderr) = test_env.jj_cmd_ok( @@ -567,7 +580,7 @@ fn test_split_interactive() { 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(); + std::fs::write(edit_script, "dump editor").unwrap(); let diff_editor = test_env.set_up_fake_diff_editor(); let diff_script = ["rm file2", "dump JJ-INSTRUCTIONS instrs"].join("\0"); @@ -587,14 +600,26 @@ fn test_split_interactive() { "#); insta::assert_snapshot!( - std::fs::read_to_string(test_env.env_root().join("editor")).unwrap(), @r###" + std::fs::read_to_string(test_env.env_root().join("editor")).unwrap(), @r#" + JJ: Enter or edit commit descriptions after the `JJ: describe` lines. + JJ: Warning: + JJ: - The text you enter will be lost on a syntax error. + JJ: - The syntax of the separator lines may change in the future. + + JJ: describe 0e15949eed93 ------- 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. - "###); + JJ: describe 9ed12e4ccec2 ------- + JJ: Enter a description for the second commit. + + JJ: This commit contains the following changes: + JJ: A file2 + + JJ: Lines starting with "JJ: " (like this one) will be removed. + "#); insta::assert_snapshot!(stdout, @""); insta::assert_snapshot!(stderr, @r###" @@ -630,7 +655,7 @@ fn test_split_interactive_with_paths() { std::fs::write(workspace_path.join("file3"), "baz\n").unwrap(); let edit_script = test_env.set_up_fake_editor(); - std::fs::write(edit_script, ["dump editor"].join("\0")).unwrap(); + std::fs::write(edit_script, "dump editor").unwrap(); let diff_editor = test_env.set_up_fake_diff_editor(); let diff_script = [ "files-before file2", @@ -650,14 +675,27 @@ fn test_split_interactive_with_paths() { "); insta::assert_snapshot!( - std::fs::read_to_string(test_env.env_root().join("editor")).unwrap(), @r###" + std::fs::read_to_string(test_env.env_root().join("editor")).unwrap(), @r#" + JJ: Enter or edit commit descriptions after the `JJ: describe` lines. + JJ: Warning: + JJ: - The text you enter will be lost on a syntax error. + JJ: - The syntax of the separator lines may change in the future. + + JJ: describe e3d766b80859 ------- 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. - "###); + JJ: describe 4cf22d3b2366 ------- + JJ: Enter a description for the second commit. + + JJ: This commit contains the following changes: + JJ: M file2 + JJ: M file3 + + JJ: Lines starting with "JJ: " (like this one) will be removed. + "#); let stdout = test_env.jj_cmd_success(&workspace_path, &["log", "--summary"]); insta::assert_snapshot!(stdout, @r"