From d6e97883dfa86ca6dbd11962dc7aded4965c1848 Mon Sep 17 00:00:00 2001 From: Yuya Nishihara Date: Sat, 20 Jul 2024 17:46:50 +0900 Subject: [PATCH] cli: port description template to templater This implements a building block of "signed-off-by line" #1399 and "commit --verbose" #1946. We'll probably need an easy way to customize the diff part, but I'm not sure if it can be as simple as a template alias function. User might want to embed diffs without "JJ: " prefixes? Perhaps, we can deprecate "ui.default-description", but it's not addressed in this patch. It could be replaced with "default_description" template alias, but we might want to configure default per command. Suppose we add a default "backout_description" template, it would have to be rendered against the source commit, not the newly-created backout commit. The template key is named as "draft_commit_description" because it is the template to generate an editor template. "templates.commit_description_template" sounds a bit odd. There's one minor behavior change: the default description is now terminated by "\n". Closes #1354 --- CHANGELOG.md | 3 ++ cli/src/cli_util.rs | 4 ++ cli/src/commands/commit.rs | 2 +- cli/src/commands/describe.rs | 2 +- cli/src/commands/split.rs | 6 +-- cli/src/config/templates.toml | 12 ++++++ cli/src/description_util.rs | 57 ++++++++++++--------------- cli/tests/test_commit_command.rs | 62 ++++++++++++++++++++++++++++++ cli/tests/test_describe_command.rs | 1 + cli/tests/test_split_command.rs | 2 + docs/config.md | 21 ++++++++-- 11 files changed, 131 insertions(+), 41 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 186522b66..9e720122e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -82,6 +82,9 @@ to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). * New config setting `git.private-commits` to prevent commits from being pushed. +* [The default commit description template](docs/config.md#default-description) + can now be configured by `templates.draft_commit_description`. + ### Fixed bugs * `jj diff --git` no longer shows the contents of binary files. diff --git a/cli/src/cli_util.rs b/cli/src/cli_util.rs index 0b2b27fe8..0931b40c8 100644 --- a/cli/src/cli_util.rs +++ b/cli/src/cli_util.rs @@ -1638,6 +1638,10 @@ impl WorkspaceCommandTransaction<'_> { self.helper } + pub fn settings(&self) -> &UserSettings { + &self.helper.settings + } + pub fn base_repo(&self) -> &Arc { self.tx.base_repo() } diff --git a/cli/src/commands/commit.rs b/cli/src/commands/commit.rs index 6202554b4..0c6cb58c1 100644 --- a/cli/src/commands/commit.rs +++ b/cli/src/commands/commit.rs @@ -111,7 +111,7 @@ new working-copy commit. commit_builder.set_description(command.settings().default_description()); } let temp_commit = commit_builder.write_hidden()?; - let template = description_template(ui, tx.base_workspace_helper(), "", &temp_commit)?; + let template = description_template(&tx, "", &temp_commit)?; edit_description(tx.base_repo(), &template, command.settings())? }; commit_builder.set_description(description); diff --git a/cli/src/commands/describe.rs b/cli/src/commands/describe.rs index 007fdf20d..e3dc66d5a 100644 --- a/cli/src/commands/describe.rs +++ b/cli/src/commands/describe.rs @@ -90,7 +90,7 @@ pub(crate) fn cmd_describe( commit_builder.set_description(command.settings().default_description()); } let temp_commit = commit_builder.write_hidden()?; - let template = description_template(ui, tx.base_workspace_helper(), "", &temp_commit)?; + let template = description_template(&tx, "", &temp_commit)?; edit_description(tx.base_repo(), &template, command.settings())? }; commit_builder.set_description(description); diff --git a/cli/src/commands/split.rs b/cli/src/commands/split.rs index f7c61d02a..458d12a81 100644 --- a/cli/src/commands/split.rs +++ b/cli/src/commands/split.rs @@ -133,8 +133,7 @@ the operation will be aborted. } let temp_commit = commit_builder.write_hidden()?; let template = description_template( - ui, - tx.base_workspace_helper(), + &tx, "Enter a description for the first commit.", &temp_commit, )?; @@ -176,8 +175,7 @@ the operation will be aborted. } else { let temp_commit = commit_builder.write_hidden()?; let template = description_template( - ui, - tx.base_workspace_helper(), + &tx, "Enter a description for the second commit.", &temp_commit, )?; diff --git a/cli/src/config/templates.toml b/cli/src/config/templates.toml index bc8d0c088..ecc199a0c 100644 --- a/cli/src/config/templates.toml +++ b/cli/src/config/templates.toml @@ -21,6 +21,18 @@ if(overridden, ) ++ "\n" ''' +# TODO: Provide hook point for diff customization (#1946)? We might want a +# syntax to comment out full text diffs without using the "JJ: " prefix. +draft_commit_description = ''' +concat( + description, + surround( + "\nJJ: This commit contains the following changes:\n", "", + indent("JJ: ", diff.summary()), + ), +) +''' + log = 'builtin_log_compact' op_log = 'builtin_op_log_compact' show = 'builtin_log_detailed' diff --git a/cli/src/description_util.rs b/cli/src/description_util.rs index f673c8c54..3e6b14d7a 100644 --- a/cli/src/description_util.rs +++ b/cli/src/description_util.rs @@ -1,15 +1,15 @@ +use std::io::Write as _; + +use bstr::ByteVec as _; use itertools::Itertools; use jj_lib::commit::Commit; -use jj_lib::matchers::EverythingMatcher; 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, WorkspaceCommandTransaction}; use crate::command_error::CommandError; -use crate::diff_util::DiffFormat; use crate::formatter::PlainTextFormatter; use crate::text_util; -use crate::ui::Ui; pub fn edit_description( repo: &ReadonlyRepo, @@ -89,37 +89,30 @@ pub fn join_message_paragraphs(paragraphs: &[String]) -> String { .join("\n") } +/// Renders commit description template, which will be edited by user. pub fn description_template( - ui: &Ui, - workspace_command: &WorkspaceCommandHelper, + tx: &WorkspaceCommandTransaction, intro: &str, commit: &Commit, ) -> Result { - let mut diff_summary_bytes = Vec::new(); - let diff_renderer = workspace_command.diff_renderer(vec![DiffFormat::Summary]); - diff_renderer.show_patch( - ui, - &mut PlainTextFormatter::new(&mut diff_summary_bytes), - commit, - &EverythingMatcher, - )?; - let mut template_chunks = Vec::new(); - if !intro.is_empty() { - template_chunks.push(format!("JJ: {intro}\n")); - } - template_chunks.push(commit.description().to_owned()); - if !diff_summary_bytes.is_empty() { - template_chunks.push("\n".to_owned()); - template_chunks.push(diff_summary_to_description(&diff_summary_bytes)); - } - Ok(template_chunks.concat()) -} + // TODO: Should "ui.default-description" be deprecated? + // We might want default description templates per command instead. For + // example, "backout_description" template will be rendered against the + // commit to be backed out, and the generated description could be set + // without spawning editor. -pub fn diff_summary_to_description(bytes: &[u8]) -> String { - let text = std::str::from_utf8(bytes).expect( - "Summary diffs and repo paths must always be valid UTF8.", - // Double-check this assumption for diffs that include file content. - ); - "JJ: This commit contains the following changes:\n".to_owned() - + &textwrap::indent(text, "JJ: ") + // Named as "draft" because the output can contain "JJ: " comment lines. + let template_key = "templates.draft_commit_description"; + let template_text = tx.settings().config().get_string(template_key)?; + let template = tx.parse_commit_template(&template_text)?; + + let mut output = Vec::new(); + if !intro.is_empty() { + writeln!(output, "JJ: {intro}").unwrap(); + } + template + .format(commit, &mut PlainTextFormatter::new(&mut output)) + .expect("write() to vec backed formatter should never fail"); + // Template output is usually UTF-8, but it can contain file content. + Ok(output.into_string_lossy()) } diff --git a/cli/tests/test_commit_command.rs b/cli/tests/test_commit_command.rs index 2af893e93..7c752c54c 100644 --- a/cli/tests/test_commit_command.rs +++ b/cli/tests/test_commit_command.rs @@ -174,6 +174,7 @@ fn test_commit_with_default_description() { TESTED=TODO + JJ: This commit contains the following changes: JJ: A file1 JJ: A file2 @@ -182,6 +183,67 @@ fn test_commit_with_default_description() { "###); } +#[test] +fn test_commit_with_description_template() { + let mut test_env = TestEnvironment::default(); + test_env.jj_cmd_ok(test_env.env_root(), &["git", "init", "repo"]); + test_env.add_config( + r#" + [templates] + draft_commit_description = ''' + concat( + description, + "\n", + indent( + "JJ: ", + concat( + "Author: " ++ format_detailed_signature(author) ++ "\n", + "Committer: " ++ format_detailed_signature(committer) ++ "\n", + "\n", + diff.stat(76), + ), + ), + ) + ''' + "#, + ); + let workspace_path = test_env.env_root().join("repo"); + + let edit_script = test_env.set_up_fake_editor(); + std::fs::write(edit_script, ["dump editor"].join("\0")).unwrap(); + + std::fs::write(workspace_path.join("file1"), "foo\n").unwrap(); + std::fs::write(workspace_path.join("file2"), "bar\n").unwrap(); + + // Only file1 should be included in the diff + test_env.jj_cmd_ok(&workspace_path, &["commit", "file1"]); + insta::assert_snapshot!( + std::fs::read_to_string(test_env.env_root().join("editor")).unwrap(), @r###" + + JJ: Author: Test User (2001-02-03 08:05:08) + JJ: Committer: Test User (2001-02-03 08:05:08) + + JJ: file1 | 1 + + JJ: 1 file changed, 1 insertion(+), 0 deletions(-) + + JJ: Lines starting with "JJ: " (like this one) will be removed. + "###); + + // Timestamp after the reset should be available to the template + test_env.jj_cmd_ok(&workspace_path, &["commit", "--reset-author"]); + insta::assert_snapshot!( + std::fs::read_to_string(test_env.env_root().join("editor")).unwrap(), @r###" + + JJ: Author: Test User (2001-02-03 08:05:09) + JJ: Committer: Test User (2001-02-03 08:05:09) + + JJ: file2 | 1 + + JJ: 1 file changed, 1 insertion(+), 0 deletions(-) + + JJ: Lines starting with "JJ: " (like this one) will be removed. + "###); +} + #[test] fn test_commit_without_working_copy() { let test_env = TestEnvironment::default(); diff --git a/cli/tests/test_describe_command.rs b/cli/tests/test_describe_command.rs index c0dcc3101..380cff1c8 100644 --- a/cli/tests/test_describe_command.rs +++ b/cli/tests/test_describe_command.rs @@ -271,6 +271,7 @@ fn test_describe_default_description() { TESTED=TODO + JJ: This commit contains the following changes: JJ: A file1 JJ: A file2 diff --git a/cli/tests/test_split_command.rs b/cli/tests/test_split_command.rs index cb8bf3aec..b4a9116ea 100644 --- a/cli/tests/test_split_command.rs +++ b/cli/tests/test_split_command.rs @@ -248,6 +248,7 @@ fn test_split_with_default_description() { TESTED=TODO + JJ: This commit contains the following changes: JJ: A file1 @@ -366,6 +367,7 @@ fn test_split_siblings_no_descendants() { TESTED=TODO + JJ: This commit contains the following changes: JJ: A file1 diff --git a/docs/config.md b/docs/config.md index 8bf105da7..9788cc5db 100644 --- a/docs/config.md +++ b/docs/config.md @@ -158,9 +158,24 @@ ui.default-command = ["log", "--reversed"] ### Default description -The value of the `ui.default-description` setting will be used to prepopulate -the editor when describing changes with an empty description. This could be a -useful reminder to fill in things like BUG=, TESTED= etc. +The editor content of a commit description can be populated by the +`draft_commit_description` template. + +```toml +[templates] +draft_commit_description = ''' +concat( + description, + surround( + "\nJJ: This commit contains the following changes:\n", "", + indent("JJ: ", diff.stat(72)), + ), +) +''' +``` + +The value of the `ui.default-description` setting can also be used in order to +fill in things like BUG=, TESTED= etc. ```toml ui.default-description = "\n\nTESTED=TODO"