From 1bc9de001261e6d65d8380f4ae476db69f299856 Mon Sep 17 00:00:00 2001 From: Benjamin Tan <benjamin@dev.ofcr.se> Date: Fri, 5 Jul 2024 17:46:05 +0800 Subject: [PATCH] describe: allow updating the description of multiple commits If multiple commits are provided, the description of each commit will be combined into a single file for editing. --- CHANGELOG.md | 2 + cli/src/commands/describe.rs | 168 ++++++++++++++++---- cli/src/description_util.rs | 149 +++++++++++++++++- cli/tests/cli-reference@.md.snap | 10 +- cli/tests/test_describe_command.rs | 236 ++++++++++++++++++++++++++++- 5 files changed, 519 insertions(+), 46 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 878a8fceb0..81907052c3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -33,6 +33,8 @@ to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). * `jj backout` now includes the backed out commit's subject in the new commit message. +* `jj describe` can now update the description of multiple commits. + ### Fixed bugs ## [0.19.0] - 2024-07-03 diff --git a/cli/src/commands/describe.rs b/cli/src/commands/describe.rs index a944eee50b..616e6a9494 100644 --- a/cli/src/commands/describe.rs +++ b/cli/src/commands/describe.rs @@ -12,35 +12,44 @@ // See the License for the specific language governing permissions and // limitations under the License. -use std::io::{self, Read, Write}; +use std::collections::HashMap; +use std::io::{self, Read}; +use itertools::Itertools; +use jj_lib::commit::CommitIteratorExt; use jj_lib::object_id::ObjectId; use tracing::instrument; use crate::cli_util::{CommandHelper, RevisionArg}; -use crate::command_error::CommandError; +use crate::command_error::{user_error, CommandError}; use crate::description_util::{ - description_template_for_describe, edit_description, join_message_paragraphs, + edit_multiple_descriptions, join_message_paragraphs, EditMultipleDescriptionsResult, }; use crate::ui::Ui; /// Update the change description or other metadata /// -/// Starts an editor to let you edit the description of a change. The editor +/// Starts an editor to let you edit the description of changes. The editor /// will be $EDITOR, or `pico` if that's not defined (`Notepad` on Windows). #[derive(clap::Args, Clone, Debug)] #[command(visible_aliases = &["desc"])] pub(crate) struct DescribeArgs { - /// The revision whose description to edit + /// The revision(s) whose description to edit #[arg(default_value = "@")] - revision: RevisionArg, + revisions: Vec<RevisionArg>, /// Ignored (but lets you pass `-r` for consistency with other commands) - #[arg(short = 'r', hide = true)] - unused_revision: bool, + #[arg(short = 'r', hide = true, action = clap::ArgAction::Count)] + unused_revision: u8, /// The change description to use (don't open editor) + /// + /// If multiple revisions are specified, the same description will be used + /// for all of them. #[arg(long = "message", short, value_name = "MESSAGE")] message_paragraphs: Vec<String>, /// Read the change description from stdin + /// + /// If multiple revisions are specified, the same description will be used + /// for all of them. #[arg(long)] stdin: bool, /// Don't open an editor @@ -67,35 +76,134 @@ pub(crate) fn cmd_describe( args: &DescribeArgs, ) -> Result<(), CommandError> { let mut workspace_command = command.workspace_helper(ui)?; - let commit = workspace_command.resolve_single_rev(&args.revision)?; - workspace_command.check_rewritable([commit.id()])?; - let description = if args.stdin { + let commits: Vec<_> = workspace_command + .parse_union_revsets(&args.revisions)? + .evaluate_to_commits()? + .try_collect()?; // in reverse topological order + if commits.is_empty() { + writeln!(ui.status(), "No revisions to describe.")?; + return Ok(()); + } + workspace_command.check_rewritable(commits.iter().ids())?; + + let shared_description = if args.stdin { let mut buffer = String::new(); io::stdin().read_to_string(&mut buffer).unwrap(); - buffer + Some(buffer) } else if !args.message_paragraphs.is_empty() { - join_message_paragraphs(&args.message_paragraphs) - } else if args.no_edit { - commit.description().to_owned() + Some(join_message_paragraphs(&args.message_paragraphs)) } else { - let template = - description_template_for_describe(ui, command.settings(), &workspace_command, &commit)?; - edit_description(workspace_command.repo(), &template, command.settings())? + None }; - if description == *commit.description() && !args.reset_author { - writeln!(ui.status(), "Nothing changed.")?; + let commit_descriptions: HashMap<_, _> = if args.no_edit || shared_description.is_some() { + commits + .iter() + .map(|commit| { + let new_description = shared_description + .as_deref() + .unwrap_or_else(|| commit.description()); + (commit, new_description.to_owned()) + }) + .collect() } else { - let mut tx = workspace_command.start_transaction(); - let mut commit_builder = tx - .mut_repo() - .rewrite_commit(command.settings(), &commit) - .set_description(description); - if args.reset_author { - let new_author = commit_builder.committer().clone(); - commit_builder = commit_builder.set_author(new_author); + let EditMultipleDescriptionsResult { + descriptions, + missing, + duplicates, + unexpected, + } = edit_multiple_descriptions( + ui, + command.settings(), + &workspace_command, + workspace_command.repo(), + // Edit descriptions in topological order + &commits.iter().rev().collect_vec(), + )?; + if !missing.is_empty() { + return Err(user_error(format!( + "The description for the following commits were not found in the edited message: \ + {}", + missing.join(", ") + ))); } - commit_builder.write()?; - tx.finish(ui, format!("describe commit {}", commit.id().hex()))?; + 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 commit_descriptions = commits + .iter() + .filter_map(|commit| { + descriptions + .get(commit.id()) + .map(|description| (commit, description.to_owned())) + }) + .collect(); + + commit_descriptions + }; + let commit_descriptions: HashMap<_, _> = commit_descriptions + .into_iter() + .filter_map(|(commit, new_description)| { + if *new_description == *commit.description() && !args.reset_author { + None + } else { + Some((commit.id(), new_description)) + } + }) + .collect(); + + let mut tx = workspace_command.start_transaction(); + let tx_description = if commits.len() == 1 { + format!("describe commit {}", commits[0].id().hex()) + } else { + format!( + "describe commit {} and {} more", + commits[0].id().hex(), + commits.len() - 1 + ) + }; + + let mut num_described = 0; + let mut num_rebased = 0; + tx.mut_repo().transform_descendants( + command.settings(), + commit_descriptions + .keys() + .map(|&id| id.clone()) + .collect_vec(), + |rewriter| { + let old_commit_id = rewriter.old_commit().id().clone(); + let mut commit_builder = rewriter.rebase(command.settings())?; + if let Some(description) = commit_descriptions.get(&old_commit_id) { + commit_builder = commit_builder.set_description(description); + if args.reset_author { + let new_author = commit_builder.committer().clone(); + commit_builder = commit_builder.set_author(new_author); + } + num_described += 1; + } else { + num_rebased += 1; + } + commit_builder.write()?; + Ok(()) + }, + )?; + if num_described > 1 { + writeln!(ui.status(), "Updated {} commits", num_described)?; + } + if num_rebased > 0 { + writeln!(ui.status(), "Rebased {} descendant commits", num_rebased)?; } + tx.finish(ui, tx_description)?; Ok(()) } diff --git a/cli/src/description_util.rs b/cli/src/description_util.rs index e55f6785bb..ced2cf9a5f 100644 --- a/cli/src/description_util.rs +++ b/cli/src/description_util.rs @@ -1,17 +1,30 @@ +use std::collections::HashMap; + use itertools::Itertools; +use jj_lib::backend::CommitId; use jj_lib::commit::Commit; use jj_lib::matchers::EverythingMatcher; use jj_lib::merged_tree::MergedTree; use jj_lib::repo::ReadonlyRepo; use jj_lib::settings::UserSettings; -use crate::cli_util::{edit_temp_file, WorkspaceCommandHelper}; +use crate::cli_util::{edit_temp_file, short_commit_hash, WorkspaceCommandHelper}; use crate::command_error::CommandError; use crate::diff_util::DiffFormat; use crate::formatter::PlainTextFormatter; use crate::text_util; use crate::ui::Ui; +/// Cleanup a description by normalizing line endings, and removing leading and +/// trailing blank lines. +fn cleanup_description(description: &str) -> String { + let description = description + .lines() + .filter(|line| !line.starts_with("JJ: ")) + .join("\n"); + text_util::complete_newline(description.trim_matches('\n')) +} + pub fn edit_description( repo: &ReadonlyRepo, description: &str, @@ -32,12 +45,134 @@ JJ: Lines starting with "JJ: " (like this one) will be removed. settings, )?; - // Normalize line ending, remove leading and trailing blank lines. - let description = description - .lines() - .filter(|line| !line.starts_with("JJ: ")) - .join("\n"); - Ok(text_util::complete_newline(description.trim_matches('\n'))) + Ok(cleanup_description(&description)) +} + +#[derive(Debug)] +pub struct EditMultipleDescriptionsResult { + /// The parsed, formatted descriptions. + pub descriptions: HashMap<CommitId, String>, + /// Commit IDs that were expected while parsing the edited messages, but + /// which were not found. + pub missing: Vec<String>, + /// Commit IDs that were found multiple times while parsing the edited + /// messages. + pub duplicates: Vec<String>, + /// Commit IDs that were found while parsing the edited messages, but which + /// were not originally being edited. + pub unexpected: Vec<String>, +} + +/// Edits the descriptions of the given commits in a single editor session. +pub fn edit_multiple_descriptions( + ui: &Ui, + settings: &UserSettings, + workspace_command: &WorkspaceCommandHelper, + repo: &ReadonlyRepo, + commits: &[&Commit], +) -> Result<EditMultipleDescriptionsResult, CommandError> { + let mut commits_map = HashMap::new(); + let mut output_chunks = Vec::new(); + + for &commit in commits.iter() { + let commit_hash = short_commit_hash(commit.id()); + if commits.len() > 1 { + output_chunks.push(format!("JJ: describe {} -------\n", commit_hash.clone())); + } + commits_map.insert(commit_hash, commit.id()); + let template = description_template_for_describe(ui, settings, workspace_command, commit)?; + output_chunks.push(template); + output_chunks.push("\n".to_owned()); + } + output_chunks + .push("JJ: Lines starting with \"JJ: \" (like this one) will be removed.\n".to_owned()); + let bulk_message = output_chunks.join(""); + + let bulk_message = edit_temp_file( + "description", + ".jjdescription", + repo.repo_path(), + &bulk_message, + settings, + )?; + + if commits.len() == 1 { + return Ok(EditMultipleDescriptionsResult { + descriptions: HashMap::from([( + commits[0].id().clone(), + cleanup_description(&bulk_message), + )]), + missing: vec![], + duplicates: vec![], + unexpected: vec![], + }); + } + + Ok(parse_bulk_edit_message(&bulk_message, &commits_map)) +} + +/// Parse the bulk message of edited commit descriptions. +fn parse_bulk_edit_message( + message: &str, + commit_ids_map: &HashMap<String, &CommitId>, +) -> EditMultipleDescriptionsResult { + let mut descriptions = HashMap::new(); + let mut duplicates = Vec::new(); + let mut unexpected = Vec::new(); + + let messages = message.lines().fold(vec![], |mut accum, line| { + if let Some(commit_id_prefix) = line.strip_prefix("JJ: describe ") { + let commit_id_prefix = commit_id_prefix + .strip_suffix(" -------") + .unwrap_or(commit_id_prefix); + accum.push((commit_id_prefix, vec![])); + } else if let Some((_, lines)) = accum.last_mut() { + lines.push(line); + }; + accum + }); + + for (commit_id_prefix, description_lines) in messages { + let commit_id = match commit_ids_map.get(commit_id_prefix) { + Some(&commit_id) => commit_id, + None => { + unexpected.push(commit_id_prefix.to_string()); + continue; + } + }; + if descriptions.contains_key(commit_id) { + duplicates.push(commit_id_prefix.to_string()); + continue; + } + descriptions.insert( + commit_id.clone(), + cleanup_description(&description_lines.join("\n")), + ); + } + + let missing: Vec<_> = commit_ids_map + .keys() + .filter_map(|commit_id_prefix| { + let commit_id = match commit_ids_map.get(commit_id_prefix) { + Some(&commit_id) => commit_id, + None => { + return None; + } + }; + if !descriptions.contains_key(commit_id) { + Some(commit_id_prefix.to_string()) + } else { + None + } + }) + .collect(); + + EditMultipleDescriptionsResult { + descriptions, + missing, + duplicates, + unexpected, + } } /// Combines the descriptions from the input commits. If only one is non-empty, diff --git a/cli/tests/cli-reference@.md.snap b/cli/tests/cli-reference@.md.snap index 9547c31583..c5e5058f57 100644 --- a/cli/tests/cli-reference@.md.snap +++ b/cli/tests/cli-reference@.md.snap @@ -569,20 +569,24 @@ Update config file to set the given option to a given value Update the change description or other metadata -Starts an editor to let you edit the description of a change. The editor will be $EDITOR, or `pico` if that's not defined (`Notepad` on Windows). +Starts an editor to let you edit the description of changes. The editor will be $EDITOR, or `pico` if that's not defined (`Notepad` on Windows). -**Usage:** `jj describe [OPTIONS] [REVISION]` +**Usage:** `jj describe [OPTIONS] [REVISIONS]...` ###### **Arguments:** -* `<REVISION>` — The revision whose description to edit +* `<REVISIONS>` — The revision(s) whose description to edit Default value: `@` ###### **Options:** * `-m`, `--message <MESSAGE>` — The change description to use (don't open editor) + + If multiple revisions are specified, the same description will be used for all of them. * `--stdin` — Read the change description from stdin + + If multiple revisions are specified, the same description will be used for all of them. * `--no-edit` — Don't open an editor This is mainly useful in combination with e.g. `--reset-author`. diff --git a/cli/tests/test_describe_command.rs b/cli/tests/test_describe_command.rs index b9606933ed..97dc82307c 100644 --- a/cli/tests/test_describe_command.rs +++ b/cli/tests/test_describe_command.rs @@ -12,7 +12,9 @@ // See the License for the specific language governing permissions and // limitations under the License. -use std::path::PathBuf; +use std::path::{Path, PathBuf}; + +use indoc::indoc; use crate::common::{get_stderr_string, TestEnvironment}; @@ -173,6 +175,180 @@ fn test_describe() { assert!(get_stderr_string(&assert).contains("bad-jj-editor-from-jj-editor-env")); } +#[test] +fn test_describe_multiple_commits() { + let mut test_env = TestEnvironment::default(); + test_env.jj_cmd_ok(test_env.env_root(), &["git", "init", "repo"]); + let repo_path = test_env.env_root().join("repo"); + + let edit_script = test_env.set_up_fake_editor(); + + // Initial setup + test_env.jj_cmd_ok(&repo_path, &["new"]); + test_env.jj_cmd_ok(&repo_path, &["new"]); + insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###" + @ c6349e79bbfd + ◉ 65b6b74e0897 + ◉ 230dd059e1b0 + ◉ 000000000000 + "###); + + // Set the description of multiple commits using `-m` flag + let (stdout, stderr) = test_env.jj_cmd_ok( + &repo_path, + &["describe", "@", "@--", "-m", "description from CLI"], + ); + insta::assert_snapshot!(stdout, @""); + insta::assert_snapshot!(stderr, @r###" + Updated 2 commits + Rebased 1 descendant commits + Working copy now at: kkmpptxz 41659b84 (empty) description from CLI + Parent commit : rlvkpnrz 8d650510 (empty) (no description set) + "###); + insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###" + @ 41659b846096 description from CLI + ◉ 8d650510daad + ◉ a42f5755e688 description from CLI + ◉ 000000000000 + "###); + + // Check that the text file gets initialized with the current description of + // each commit and doesn't update commits if no changes are made. + // Commit descriptions are edited in topological order + std::fs::write(&edit_script, "dump editor0").unwrap(); + let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["describe", "@", "@-"]); + insta::assert_snapshot!(stdout, @""); + insta::assert_snapshot!(stderr, @r###" + Nothing changed. + "###); + insta::assert_snapshot!( + std::fs::read_to_string(test_env.env_root().join("editor0")).unwrap(), @r###" + JJ: describe 8d650510daad ------- + + JJ: describe 41659b846096 ------- + description from CLI + + JJ: Lines starting with "JJ: " (like this one) will be removed. + "###); + + // Set the description of multiple commits in the editor + std::fs::write( + &edit_script, + indoc! {" + write + JJ: describe 8d650510daad ------- + description from editor of @- + + further commit message of @- + + JJ: describe 41659b846096 ------- + description from editor of @ + + further commit message of @ + + JJ: Lines starting with \"JJ: \" (like this one) will be removed. + "}, + ) + .unwrap(); + let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["describe", "@", "@-"]); + insta::assert_snapshot!(stdout, @""); + insta::assert_snapshot!(stderr, @r###" + Updated 2 commits + Working copy now at: kkmpptxz f203494a (empty) description from editor of @ + Parent commit : rlvkpnrz 0d76a92c (empty) description from editor of @- + "###); + insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###" + @ f203494a4507 description from editor of @ + │ + │ further commit message of @ + ◉ 0d76a92ca7cc description from editor of @- + │ + │ further commit message of @- + ◉ a42f5755e688 description from CLI + ◉ 000000000000 + "###); + + // Fails if the edited message has a commit has multiple descriptions + std::fs::write( + &edit_script, + indoc! {" + write + JJ: describe 0d76a92ca7cc ------- + first description from editor of @- + + further commit message of @- + + JJ: describe 0d76a92ca7cc ------- + second description from editor of @- + + further commit message of @- + + JJ: describe f203494a4507 ------- + updated description from editor of @ + + further commit message of @ + + JJ: Lines starting with \"JJ: \" (like this one) will be removed. + "}, + ) + .unwrap(); + let stderr = test_env.jj_cmd_failure(&repo_path, &["describe", "@", "@-"]); + insta::assert_snapshot!(stderr, @r###" + Error: The following commits were found in the edited message multiple times: 0d76a92ca7cc + "###); + + // Fails if the edited message has unexpected commit IDs + std::fs::write( + &edit_script, + indoc! {" + write + JJ: describe 000000000000 ------- + unexpected commit ID + + JJ: describe 0d76a92ca7cc ------- + description from editor of @- + + further commit message of @- + + JJ: describe f203494a4507 ------- + description from editor of @ + + further commit message of @ + + JJ: Lines starting with \"JJ: \" (like this one) will be removed. + "}, + ) + .unwrap(); + let stderr = test_env.jj_cmd_failure(&repo_path, &["describe", "@", "@-"]); + insta::assert_snapshot!(stderr, @r###" + Error: The following commits were not being edited, but were found in the edited message: 000000000000 + "###); + + // Fails if the edited message has missing commit messages + std::fs::write( + &edit_script, + indoc! {" + write + JJ: describe f203494a4507 ------- + description from editor of @ + + further commit message of @ + + JJ: Lines starting with \"JJ: \" (like this one) will be removed. + "}, + ) + .unwrap(); + let stderr = test_env.jj_cmd_failure(&repo_path, &["describe", "@", "@-"]); + insta::assert_snapshot!(stderr, @r###" + Error: The description for the following commits were not found in the edited message: 0d76a92ca7cc + "###); + + // Fails if the editor fails + std::fs::write(&edit_script, "fail").unwrap(); + let stderr = test_env.jj_cmd_failure(&repo_path, &["describe", "@", "@-"]); + assert!(stderr.contains("exited with an error")); +} + #[test] fn test_multiple_message_args() { let test_env = TestEnvironment::default(); @@ -295,19 +471,30 @@ fn test_describe_author() { &repo_path, &[ "log", - "-r@", + "-r..", "-T", r#"format_signature(author) ++ "\n" ++ format_signature(committer)"#, ], ) }; + + // Initial setup + test_env.jj_cmd_ok(&repo_path, &["new"]); + test_env.jj_cmd_ok(&repo_path, &["new"]); + test_env.jj_cmd_ok(&repo_path, &["new"]); insta::assert_snapshot!(get_signatures(), @r###" - @ Test User test.user@example.com 2001-02-03 04:05:07.000 +07:00 + @ Test User test.user@example.com 2001-02-03 04:05:10.000 +07:00 + │ Test User test.user@example.com 2001-02-03 04:05:10.000 +07:00 + ◉ Test User test.user@example.com 2001-02-03 04:05:09.000 +07:00 + │ Test User test.user@example.com 2001-02-03 04:05:09.000 +07:00 + ◉ Test User test.user@example.com 2001-02-03 04:05:08.000 +07:00 + │ Test User test.user@example.com 2001-02-03 04:05:08.000 +07:00 + ◉ Test User test.user@example.com 2001-02-03 04:05:07.000 +07:00 │ Test User test.user@example.com 2001-02-03 04:05:07.000 +07:00 ~ "###); - // Reset the author (the committer is always reset) + // Reset the author for the latest commit (the committer is always reset) test_env.jj_cmd_ok( &repo_path, &[ @@ -320,8 +507,40 @@ fn test_describe_author() { ], ); insta::assert_snapshot!(get_signatures(), @r###" - @ Ove Ridder ove.ridder@example.com 2001-02-03 04:05:09.000 +07:00 - │ Ove Ridder ove.ridder@example.com 2001-02-03 04:05:09.000 +07:00 + @ Ove Ridder ove.ridder@example.com 2001-02-03 04:05:12.000 +07:00 + │ Ove Ridder ove.ridder@example.com 2001-02-03 04:05:12.000 +07:00 + ◉ Test User test.user@example.com 2001-02-03 04:05:09.000 +07:00 + │ Test User test.user@example.com 2001-02-03 04:05:09.000 +07:00 + ◉ Test User test.user@example.com 2001-02-03 04:05:08.000 +07:00 + │ Test User test.user@example.com 2001-02-03 04:05:08.000 +07:00 + ◉ Test User test.user@example.com 2001-02-03 04:05:07.000 +07:00 + │ Test User test.user@example.com 2001-02-03 04:05:07.000 +07:00 + ~ + "###); + + // Reset the author for multiple commits (the committer is always reset) + test_env.jj_cmd_ok( + &repo_path, + &[ + "describe", + "@---", + "@-", + "--config-toml", + r#"user.name = "Ove Ridder" + user.email = "ove.ridder@example.com""#, + "--no-edit", + "--reset-author", + ], + ); + insta::assert_snapshot!(get_signatures(), @r###" + @ Ove Ridder ove.ridder@example.com 2001-02-03 04:05:14.000 +07:00 + │ Ove Ridder ove.ridder@example.com 2001-02-03 04:05:14.000 +07:00 + ◉ Ove Ridder ove.ridder@example.com 2001-02-03 04:05:14.000 +07:00 + │ Ove Ridder ove.ridder@example.com 2001-02-03 04:05:14.000 +07:00 + ◉ Test User test.user@example.com 2001-02-03 04:05:08.000 +07:00 + │ Ove Ridder ove.ridder@example.com 2001-02-03 04:05:14.000 +07:00 + ◉ Ove Ridder ove.ridder@example.com 2001-02-03 04:05:14.000 +07:00 + │ Ove Ridder ove.ridder@example.com 2001-02-03 04:05:14.000 +07:00 ~ "###); } @@ -343,3 +562,8 @@ fn test_describe_avoids_unc() { // over 260 chars. assert_eq!(edited_path, dunce::simplified(&edited_path)); } + +fn get_log_output(test_env: &TestEnvironment, repo_path: &Path) -> String { + let template = r#"commit_id.short() ++ " " ++ description"#; + test_env.jj_cmd_success(repo_path, &["log", "-T", template]) +}