From 4f2c17c589bb07446f6b7505c284c8d4b6b810b1 Mon Sep 17 00:00:00 2001 From: Yuya Nishihara Date: Thu, 29 Feb 2024 23:26:21 +0900 Subject: [PATCH 1/4] tests: move strip_last_line() to common module --- cli/tests/common/mod.rs | 10 ++++++++++ cli/tests/test_git_init.rs | 8 +------- cli/tests/test_global_opts.rs | 8 +------- cli/tests/test_init_command.rs | 8 +------- 4 files changed, 13 insertions(+), 21 deletions(-) diff --git a/cli/tests/common/mod.rs b/cli/tests/common/mod.rs index 6197fae504..b9e4081838 100644 --- a/cli/tests/common/mod.rs +++ b/cli/tests/common/mod.rs @@ -357,3 +357,13 @@ pub fn escaped_fake_diff_editor_path() -> String { // in it diff_editor_path.to_str().unwrap().replace('\\', r"\\") } + +/// Returns a string with the last line removed. +/// +/// Use this to remove the root error message containing platform-specific +/// content for example. +pub fn strip_last_line(s: &str) -> &str { + s.trim_end_matches('\n') + .rsplit_once('\n') + .map_or(s, |(h, _)| &s[..h.len() + 1]) +} diff --git a/cli/tests/test_git_init.rs b/cli/tests/test_git_init.rs index 305fe9a4e4..693d839b3a 100644 --- a/cli/tests/test_git_init.rs +++ b/cli/tests/test_git_init.rs @@ -17,7 +17,7 @@ use std::path::{Path, PathBuf}; use test_case::test_case; -use crate::common::TestEnvironment; +use crate::common::{strip_last_line, TestEnvironment}; fn init_git_repo(git_repo_path: &Path, bare: bool) -> git2::Repository { init_git_repo_with_opts(git_repo_path, git2::RepositoryInitOptions::new().bare(bare)) @@ -67,12 +67,6 @@ fn read_git_target(workspace_root: &Path) -> String { std::fs::read_to_string(path).unwrap() } -fn strip_last_line(s: &str) -> &str { - s.trim_end_matches('\n') - .rsplit_once('\n') - .map_or(s, |(h, _)| &s[..h.len() + 1]) -} - #[test] fn test_git_init_internal() { let test_env = TestEnvironment::default(); diff --git a/cli/tests/test_global_opts.rs b/cli/tests/test_global_opts.rs index 38345026e9..d2dc8dc6f7 100644 --- a/cli/tests/test_global_opts.rs +++ b/cli/tests/test_global_opts.rs @@ -14,7 +14,7 @@ use std::ffi::OsString; -use crate::common::{get_stderr_string, TestEnvironment}; +use crate::common::{get_stderr_string, strip_last_line, TestEnvironment}; #[test] fn test_non_utf8_arg() { @@ -471,9 +471,3 @@ fn test_debug_logging_enabled() { // Luckily, insta will print this in colour when reviewing. insta::assert_snapshot!(log_line, @" INFO jj_cli::cli_util: debug logging enabled"); } - -fn strip_last_line(s: &str) -> &str { - s.trim_end_matches('\n') - .rsplit_once('\n') - .map_or(s, |(h, _)| &s[..h.len() + 1]) -} diff --git a/cli/tests/test_init_command.rs b/cli/tests/test_init_command.rs index 4ca79125db..469def2288 100644 --- a/cli/tests/test_init_command.rs +++ b/cli/tests/test_init_command.rs @@ -16,7 +16,7 @@ use std::path::{Path, PathBuf}; use test_case::test_case; -use crate::common::TestEnvironment; +use crate::common::{strip_last_line, TestEnvironment}; fn init_git_repo(git_repo_path: &Path, bare: bool) -> git2::Repository { init_git_repo_with_opts(git_repo_path, git2::RepositoryInitOptions::new().bare(bare)) @@ -66,12 +66,6 @@ fn read_git_target(workspace_root: &Path) -> String { std::fs::read_to_string(path).unwrap() } -fn strip_last_line(s: &str) -> &str { - s.trim_end_matches('\n') - .rsplit_once('\n') - .map_or(s, |(h, _)| &s[..h.len() + 1]) -} - #[test] fn test_init_git_internal() { let test_env = TestEnvironment::default(); From 3563bedaa3edf5ce02fbf0837567255c3bac0638 Mon Sep 17 00:00:00 2001 From: Yuya Nishihara Date: Wed, 28 Feb 2024 21:21:01 +0900 Subject: [PATCH 2/4] merge_tools: inline get_tool_config_from_args() I'm going to split get_tool_config() to fix "diff --tool=:builtin", and it doesn't make sense to duplicate get_tool_config_from_args() per backing get_tool_config() functions. --- cli/src/diff_util.rs | 9 +++++++-- cli/src/merge_tools/mod.rs | 28 ++++++++++++---------------- 2 files changed, 19 insertions(+), 18 deletions(-) diff --git a/cli/src/diff_util.rs b/cli/src/diff_util.rs index 52ded4013b..b9b2dc586f 100644 --- a/cli/src/diff_util.rs +++ b/cli/src/diff_util.rs @@ -38,6 +38,7 @@ use tracing::instrument; use unicode_width::UnicodeWidthStr as _; use crate::cli_util::{CommandError, WorkspaceCommandHelper}; +use crate::config::CommandNameAndArgs; use crate::formatter::Formatter; use crate::merge_tools::{self, ExternalMergeTool, MergeTool}; use crate::text_util; @@ -144,8 +145,12 @@ fn default_diff_format(settings: &UserSettings) -> Result {} MergeTool::External(tool) => { diff --git a/cli/src/merge_tools/mod.rs b/cli/src/merge_tools/mod.rs index a7681c9477..b45b787aed 100644 --- a/cli/src/merge_tools/mod.rs +++ b/cli/src/merge_tools/mod.rs @@ -210,25 +210,17 @@ pub fn get_tool_config( } } -/// Loads merge tool options from `[merge-tools.]` if `args` is of -/// unstructured string type. -pub fn get_tool_config_from_args( - settings: &UserSettings, - args: &CommandNameAndArgs, -) -> Result, ConfigError> { - match args { - CommandNameAndArgs::String(name) => get_tool_config(settings, name), - CommandNameAndArgs::Vec(_) | CommandNameAndArgs::Structured { .. } => Ok(None), - } -} - fn get_diff_editor_from_settings( ui: &Ui, settings: &UserSettings, ) -> Result { let args = editor_args_from_settings(ui, settings, "ui.diff-editor")?; - let editor = get_tool_config_from_args(settings, &args)? - .unwrap_or_else(|| MergeTool::External(ExternalMergeTool::with_edit_args(&args))); + let editor = if let CommandNameAndArgs::String(name) = &args { + get_tool_config(settings, name)? + } else { + None + } + .unwrap_or_else(|| MergeTool::External(ExternalMergeTool::with_edit_args(&args))); Ok(editor) } @@ -237,8 +229,12 @@ fn get_merge_tool_from_settings( settings: &UserSettings, ) -> Result { let args = editor_args_from_settings(ui, settings, "ui.merge-editor")?; - let mergetool = get_tool_config_from_args(settings, &args)? - .unwrap_or_else(|| MergeTool::External(ExternalMergeTool::with_merge_args(&args))); + let mergetool = if let CommandNameAndArgs::String(name) = &args { + get_tool_config(settings, name)? + } else { + None + } + .unwrap_or_else(|| MergeTool::External(ExternalMergeTool::with_merge_args(&args))); match mergetool { MergeTool::External(mergetool) if mergetool.merge_args.is_empty() => { Err(ExternalToolError::MergeArgsNotConfigured { From 3575c9f6f697a671bd214415843518b198dde68a Mon Sep 17 00:00:00 2001 From: Yuya Nishihara Date: Wed, 28 Feb 2024 20:32:26 +0900 Subject: [PATCH 3/4] merge_tools: extract function that doesn't look up :builtin merge tool The :builtin tool only applies to merge or diff editor. --- cli/src/merge_tools/mod.rs | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/cli/src/merge_tools/mod.rs b/cli/src/merge_tools/mod.rs index b45b787aed..98570c7e8e 100644 --- a/cli/src/merge_tools/mod.rs +++ b/cli/src/merge_tools/mod.rs @@ -183,15 +183,24 @@ fn editor_args_from_settings( } } -/// Loads merge tool options from `[merge-tools.]`. +/// Resolves builtin merge tool name or loads external tool options from +/// `[merge-tools.]`. pub fn get_tool_config( settings: &UserSettings, name: &str, ) -> Result, ConfigError> { if name == BUILTIN_EDITOR_NAME { - return Ok(Some(MergeTool::Builtin)); + Ok(Some(MergeTool::Builtin)) + } else { + Ok(get_external_tool_config(settings, name)?.map(MergeTool::External)) } +} +/// Loads external diff/merge tool options from `[merge-tools.]`. +pub fn get_external_tool_config( + settings: &UserSettings, + name: &str, +) -> Result, ConfigError> { const TABLE_KEY: &str = "merge-tools"; let tools_table = settings.config().get_table(TABLE_KEY)?; if let Some(v) = tools_table.get(name) { @@ -204,7 +213,7 @@ pub fn get_tool_config( if result.program.is_empty() { result.program.clone_from(&name.to_string()); }; - Ok(Some(MergeTool::External(result))) + Ok(Some(result)) } else { Ok(None) } From 96ccb877e2cae15b26646e3d665dce5c116b030b Mon Sep 17 00:00:00 2001 From: Yuya Nishihara Date: Wed, 28 Feb 2024 20:46:57 +0900 Subject: [PATCH 4/4] cli: don't ignore 'diff --tool=:builtin', report error Before, --tool=:builtin argument was ignored and the tool was loaded from "ui.diff.tool" option. Since there is no single builtin diff format, :builtin doesn't make sense here. Maybe we can translate ":" to the internal diff format instead, but that will also mean "ui.diff.tool" and ".format" can be merged. This partially reverts 409356fa5bb "merge_tools: enable `:builtin` as default diff/merge editor." --- cli/src/diff_util.rs | 24 +++++++----------------- cli/src/merge_tools/mod.rs | 5 +---- cli/tests/test_diff_command.rs | 10 +++++++++- 3 files changed, 17 insertions(+), 22 deletions(-) diff --git a/cli/src/diff_util.rs b/cli/src/diff_util.rs index b9b2dc586f..6f52ad294a 100644 --- a/cli/src/diff_util.rs +++ b/cli/src/diff_util.rs @@ -40,7 +40,7 @@ use unicode_width::UnicodeWidthStr as _; use crate::cli_util::{CommandError, WorkspaceCommandHelper}; use crate::config::CommandNameAndArgs; use crate::formatter::Formatter; -use crate::merge_tools::{self, ExternalMergeTool, MergeTool}; +use crate::merge_tools::{self, ExternalMergeTool}; use crate::text_util; use crate::ui::Ui; @@ -129,14 +129,9 @@ fn diff_formats_from_args( .filter_map(|(arg, format)| arg.then_some(format)) .collect_vec(); if let Some(name) = &args.tool { - let tool = merge_tools::get_tool_config(settings, name)? - .unwrap_or_else(|| MergeTool::External(ExternalMergeTool::with_program(name))); - match tool { - MergeTool::Builtin => {} - MergeTool::External(tool) => { - formats.push(DiffFormat::Tool(Box::new(tool))); - } - } + let tool = merge_tools::get_external_tool_config(settings, name)? + .unwrap_or_else(|| ExternalMergeTool::with_program(name)); + formats.push(DiffFormat::Tool(Box::new(tool))); } Ok(formats) } @@ -146,17 +141,12 @@ fn default_diff_format(settings: &UserSettings) -> Result {} - MergeTool::External(tool) => { - return Ok(DiffFormat::Tool(Box::new(tool))); - } - } + .unwrap_or_else(|| ExternalMergeTool::with_diff_args(&args)); + return Ok(DiffFormat::Tool(Box::new(tool))); } let name = if let Some(name) = config.get_string("ui.diff.format").optional()? { name diff --git a/cli/src/merge_tools/mod.rs b/cli/src/merge_tools/mod.rs index 98570c7e8e..a6c3310e80 100644 --- a/cli/src/merge_tools/mod.rs +++ b/cli/src/merge_tools/mod.rs @@ -185,10 +185,7 @@ fn editor_args_from_settings( /// Resolves builtin merge tool name or loads external tool options from /// `[merge-tools.]`. -pub fn get_tool_config( - settings: &UserSettings, - name: &str, -) -> Result, ConfigError> { +fn get_tool_config(settings: &UserSettings, name: &str) -> Result, ConfigError> { if name == BUILTIN_EDITOR_NAME { Ok(Some(MergeTool::Builtin)) } else { diff --git a/cli/tests/test_diff_command.rs b/cli/tests/test_diff_command.rs index e73ceccf4a..c4405ceb01 100644 --- a/cli/tests/test_diff_command.rs +++ b/cli/tests/test_diff_command.rs @@ -14,7 +14,7 @@ use itertools::Itertools; -use crate::common::{escaped_fake_diff_editor_path, TestEnvironment}; +use crate::common::{escaped_fake_diff_editor_path, strip_last_line, TestEnvironment}; #[test] fn test_diff_basic() { @@ -753,6 +753,14 @@ fn test_diff_external_tool() { insta::assert_snapshot!(stderr, @r###" Tool exited with a non-zero code (run with --debug to see the exact invocation). Exit code: 1. "###); + + // --tool=:builtin shouldn't be ignored + let stderr = test_env.jj_cmd_failure(&repo_path, &["diff", "--tool=:builtin"]); + insta::assert_snapshot!(strip_last_line(&stderr), @r###" + Error: Failed to generate diff + Caused by: + 1: Error executing ':builtin' (run with --debug to see the exact invocation) + "###); } #[cfg(unix)]