From b926fd844a625d7c24cb061a6730360cbc20ab34 Mon Sep 17 00:00:00 2001 From: Yuya Nishihara Date: Thu, 29 Feb 2024 20:05:40 +0900 Subject: [PATCH] merge_tools: extract configuration error to separate type write!(ui.hint(), ..) error is suppressed because it seemed weird if the configuration error had io::Error variant. The write error isn't important anyway. --- cli/src/cli_util.rs | 11 ++++++-- cli/src/merge_tools/external.rs | 8 ------ cli/src/merge_tools/mod.rs | 46 ++++++++++++++++++--------------- 3 files changed, 34 insertions(+), 31 deletions(-) diff --git a/cli/src/cli_util.rs b/cli/src/cli_util.rs index 4b940fdd73..13166011f0 100644 --- a/cli/src/cli_util.rs +++ b/cli/src/cli_util.rs @@ -88,6 +88,7 @@ use crate::git_util::{ }; use crate::merge_tools::{ ConflictResolveError, DiffEditError, DiffEditor, DiffGenerateError, MergeEditor, + MergeToolConfigError, }; use crate::template_parser::{TemplateAliasesMap, TemplateParseError, TemplateParseErrorKind}; use crate::templater::Template; @@ -353,6 +354,12 @@ impl From for CommandError { } } +impl From for CommandError { + fn from(err: MergeToolConfigError) -> Self { + user_error_with_message("Failed to load tool configuration", err) + } +} + impl From for CommandError { fn from(err: git2::Error) -> Self { user_error_with_message("Git operation failed", err) @@ -1110,13 +1117,13 @@ impl WorkspaceCommandHelper { /// Loads diff editor from the settings. // TODO: override settings by --tool= option (#2575) - pub fn diff_editor(&self, ui: &Ui) -> Result { + pub fn diff_editor(&self, ui: &Ui) -> Result { DiffEditor::from_settings(ui, &self.settings) } /// Loads 3-way merge editor from the settings. // TODO: override settings by --tool= option (#2575) - pub fn merge_editor(&self, ui: &Ui) -> Result { + pub fn merge_editor(&self, ui: &Ui) -> Result { MergeEditor::from_settings(ui, &self.settings) } diff --git a/cli/src/merge_tools/external.rs b/cli/src/merge_tools/external.rs index e009b8afc9..d3f9ce2850 100644 --- a/cli/src/merge_tools/external.rs +++ b/cli/src/merge_tools/external.rs @@ -5,7 +5,6 @@ use std::path::{Path, PathBuf}; use std::process::{Command, ExitStatus, Stdio}; use std::sync::Arc; -use config::ConfigError; use futures::StreamExt; use itertools::Itertools; use jj_lib::backend::{FileId, MergedTreeId, TreeValue}; @@ -107,13 +106,6 @@ impl ExternalMergeTool { #[derive(Debug, Error)] pub enum ExternalToolError { - #[error("Invalid config")] - Config(#[from] ConfigError), - #[error( - "To use `{tool_name}` as a merge tool, the config `merge-tools.{tool_name}.merge-args` \ - must be defined (see docs for details)" - )] - MergeArgsNotConfigured { tool_name: String }, #[error("Error setting up temporary directory")] SetUpDir(#[source] std::io::Error), // TODO: Remove the "(run with --debug to see the exact invocation)" diff --git a/cli/src/merge_tools/mod.rs b/cli/src/merge_tools/mod.rs index 1089acc31c..886fd817e0 100644 --- a/cli/src/merge_tools/mod.rs +++ b/cli/src/merge_tools/mod.rs @@ -150,6 +150,17 @@ pub fn edit_diff( } } +#[derive(Debug, Error)] +pub enum MergeToolConfigError { + #[error(transparent)] + Config(#[from] ConfigError), + #[error( + "To use `{tool_name}` as a merge tool, the config `merge-tools.{tool_name}.merge-args` \ + must be defined (see docs for details)" + )] + MergeArgsNotConfigured { tool_name: String }, +} + #[derive(Clone, Debug, Eq, PartialEq)] pub enum MergeTool { Builtin, @@ -161,7 +172,7 @@ fn editor_args_from_settings( ui: &Ui, settings: &UserSettings, key: &str, -) -> Result { +) -> Result { // TODO: Make this configuration have a table of possible editors and detect the // best one here. if let Some(args) = settings.config().get(key).optional()? { @@ -173,7 +184,7 @@ fn editor_args_from_settings( "Using default editor '{default_editor}'; run `jj config set --user {key} :builtin` \ to disable this message." ) - .map_err(ExternalToolError::Io)?; + .ok(); Ok(default_editor.into()) } } @@ -219,11 +230,10 @@ pub struct DiffEditor { impl DiffEditor { /// Loads the default diff editor from the settings. - // TODO: extract tool configuration error from DiffEditError - pub fn from_settings(ui: &Ui, settings: &UserSettings) -> Result { + pub fn from_settings(ui: &Ui, settings: &UserSettings) -> Result { let args = editor_args_from_settings(ui, settings, "ui.diff-editor")?; let tool = if let CommandNameAndArgs::String(name) = &args { - get_tool_config(settings, name).map_err(ExternalToolError::Config)? + get_tool_config(settings, name)? } else { None } @@ -240,20 +250,18 @@ pub struct MergeEditor { impl MergeEditor { /// Loads the default 3-way merge editor from the settings. - // TODO: extract tool configuration error from ConflictResolveError - pub fn from_settings(ui: &Ui, settings: &UserSettings) -> Result { + pub fn from_settings(ui: &Ui, settings: &UserSettings) -> Result { let args = editor_args_from_settings(ui, settings, "ui.merge-editor")?; let tool = if let CommandNameAndArgs::String(name) = &args { - get_tool_config(settings, name).map_err(ExternalToolError::Config)? + get_tool_config(settings, name)? } else { None } .unwrap_or_else(|| MergeTool::External(ExternalMergeTool::with_merge_args(&args))); if matches!(&tool, MergeTool::External(mergetool) if mergetool.merge_args.is_empty()) { - return Err(ExternalToolError::MergeArgsNotConfigured { + return Err(MergeToolConfigError::MergeArgsNotConfigured { tool_name: args.to_string(), - } - .into()); + }); } Ok(MergeEditor { tool }) } @@ -435,11 +443,9 @@ mod tests { // Just program name insta::assert_debug_snapshot!(get(r#"ui.merge-editor = "my-merge""#).unwrap_err(), @r###" - ExternalTool( - MergeArgsNotConfigured { - tool_name: "my-merge", - }, - ) + MergeArgsNotConfigured { + tool_name: "my-merge", + } "###); // String args @@ -527,11 +533,9 @@ mod tests { // List args should never be a merge-tools key insta::assert_debug_snapshot!( get(r#"ui.merge-editor = ["meld"]"#).unwrap_err(), @r###" - ExternalTool( - MergeArgsNotConfigured { - tool_name: "meld", - }, - ) + MergeArgsNotConfigured { + tool_name: "meld", + } "###); // Invalid type