From c0ce88003e86c81c2ca437339d927bef477948b0 Mon Sep 17 00:00:00 2001 From: Yuya Nishihara <yuya@tcha.org> Date: Sun, 1 Dec 2024 12:33:29 +0900 Subject: [PATCH] config: introduce our ConfigGetError type Since most callers don't need to handle loading/parsing errors, it's probably better to add a separate error type for "get" operations. The other uses of ConfigError will be migrated later. Since ConfigGetError can preserve the source name/path information, this patch also removes some ad-hock error handling codes. --- cli/src/cli_util.rs | 35 ++++++------- cli/src/command_error.rs | 15 ++++++ cli/src/commands/config/get.rs | 35 +++++-------- cli/src/commands/git/fetch.rs | 2 +- cli/src/commands/git/push.rs | 2 +- cli/src/commands/log.rs | 6 +-- cli/src/commands/operation/log.rs | 6 +-- cli/src/diff_util.rs | 35 +++++++------ cli/src/formatter.rs | 47 ++++++++++-------- cli/src/graphlog.rs | 4 +- cli/src/merge_tools/mod.rs | 24 +++++---- cli/src/revset_util.rs | 13 ++++- cli/src/ui.rs | 17 +++---- cli/tests/test_config_command.rs | 32 ++++++------ cli/tests/test_global_opts.rs | 3 +- cli/tests/test_log_command.rs | 7 +-- lib/src/config.rs | 81 +++++++++++++++++++++---------- lib/src/fsmonitor.rs | 27 ++++++----- lib/src/gpg_signing.rs | 6 +-- lib/src/settings.rs | 24 ++++----- lib/src/signing.rs | 4 +- lib/src/ssh_signing.rs | 6 +-- 22 files changed, 243 insertions(+), 188 deletions(-) diff --git a/cli/src/cli_util.rs b/cli/src/cli_util.rs index 89d3119afe..d1d8a523f8 100644 --- a/cli/src/cli_util.rs +++ b/cli/src/cli_util.rs @@ -57,9 +57,9 @@ use jj_lib::backend::CommitId; use jj_lib::backend::MergedTreeId; use jj_lib::backend::TreeValue; use jj_lib::commit::Commit; -use jj_lib::config::ConfigError; +use jj_lib::config::ConfigGetError; +use jj_lib::config::ConfigGetResultExt as _; use jj_lib::config::ConfigNamePathBuf; -use jj_lib::config::ConfigResultExt as _; use jj_lib::config::StackedConfig; use jj_lib::conflicts::ConflictMarkerStyle; use jj_lib::file_util; @@ -2691,8 +2691,16 @@ fn load_template_aliases( let table = match layer.look_up_table(&table_name) { Ok(Some(table)) => table, Ok(None) => continue, - // TODO: rewrite error construction after migrating to toml_edit - Err(item) => return Err(item.clone().into_table().unwrap_err().into()), + Err(item) => { + // TODO: rewrite error construction after migrating to toml_edit + let error = item.clone().into_table().unwrap_err(); + return Err(ConfigGetError::Type { + name: table_name.to_string(), + error: error.into(), + source_path: layer.path.clone(), + } + .into()); + } }; // TODO: remove sorting after migrating to toml_edit for (decl, value) in table.iter().sorted_by_key(|&(decl, _)| decl) { @@ -2721,7 +2729,7 @@ pub struct LogContentFormat { impl LogContentFormat { /// Creates new formatting helper for the terminal. - pub fn new(ui: &Ui, settings: &UserSettings) -> Result<Self, ConfigError> { + pub fn new(ui: &Ui, settings: &UserSettings) -> Result<Self, ConfigGetError> { Ok(LogContentFormat { width: ui.term_width(), word_wrap: settings.get_bool("ui.log-word-wrap")?, @@ -2763,9 +2771,7 @@ pub fn run_ui_editor(settings: &UserSettings, edit_path: &Path) -> Result<(), Co // Work around UNC paths not being well supported on Windows (no-op for // non-Windows): https://github.com/martinvonz/jj/issues/3986 let edit_path = dunce::simplified(edit_path); - let editor: CommandNameAndArgs = settings - .get("ui.editor") - .map_err(|err| config_error_with_message("Invalid `ui.editor`", err))?; + let editor: CommandNameAndArgs = settings.get("ui.editor")?; let mut cmd = editor.to_command(); cmd.arg(edit_path); tracing::info!(?cmd, "running editor"); @@ -3084,7 +3090,7 @@ impl ValueParserFactory for RevisionArg { fn get_string_or_array( config: &StackedConfig, key: &'static str, -) -> Result<Vec<String>, ConfigError> { +) -> Result<Vec<String>, ConfigGetError> { config .get(key) .map(|string| vec![string]) @@ -3539,16 +3545,7 @@ impl CliRunner { config_env.reset_repo_path(loader.repo_path()); config_env.reload_repo_config(&mut config)?; } - ui.reset(&config).map_err(|e| { - let user_config_path = config_env.existing_user_config_path(); - let repo_config_path = config_env.existing_repo_config_path(); - let paths = [repo_config_path, user_config_path] - .into_iter() - .flatten() - .map(|path| format!("- {}", path.display())) - .join("\n"); - e.hinted(format!("Check the following config files:\n{paths}")) - })?; + ui.reset(&config)?; if env::var_os("COMPLETE").is_some() { return handle_shell_completion(ui, &self.app, &config, &cwd); diff --git a/cli/src/command_error.rs b/cli/src/command_error.rs index 67ec8d9e96..d98d9acf9b 100644 --- a/cli/src/command_error.rs +++ b/cli/src/command_error.rs @@ -23,6 +23,7 @@ use std::sync::Arc; use itertools::Itertools as _; use jj_lib::backend::BackendError; use jj_lib::config::ConfigError; +use jj_lib::config::ConfigGetError; use jj_lib::dsl_util::Diagnostics; use jj_lib::fileset::FilePatternParseError; use jj_lib::fileset::FilesetParseError; @@ -251,6 +252,20 @@ impl From<ConfigEnvError> for CommandError { } } +impl From<ConfigGetError> for CommandError { + fn from(err: ConfigGetError) -> Self { + let hint = match &err { + ConfigGetError::NotFound { .. } => None, + ConfigGetError::Type { source_path, .. } => source_path + .as_ref() + .map(|path| format!("Check the config file: {}", path.display())), + }; + let mut cmd_err = config_error(err); + cmd_err.extend_hints(hint); + cmd_err + } +} + impl From<RewriteRootCommit> for CommandError { fn from(err: RewriteRootCommit) -> Self { internal_error_with_message("Attempted to rewrite the root commit", err) diff --git a/cli/src/commands/config/get.rs b/cli/src/commands/config/get.rs index 3ebf579e42..8db273f202 100644 --- a/cli/src/commands/config/get.rs +++ b/cli/src/commands/config/get.rs @@ -13,14 +13,15 @@ // limitations under the License. use std::io::Write as _; +use std::path::PathBuf; use clap_complete::ArgValueCandidates; use jj_lib::config::ConfigError; +use jj_lib::config::ConfigGetError; use jj_lib::config::ConfigNamePathBuf; use tracing::instrument; use crate::cli_util::CommandHelper; -use crate::command_error::config_error; use crate::command_error::CommandError; use crate::complete; use crate::ui::Ui; @@ -48,29 +49,19 @@ pub fn cmd_config_get( args: &ConfigGetArgs, ) -> Result<(), CommandError> { let value = command.settings().get_value(&args.name)?; - let stringified = value.into_string().map_err(|err| match err { - ConfigError::Type { - origin, - unexpected, - expected, - key, - } => { - let expected = format!("a value convertible to {expected}"); - // Copied from `impl fmt::Display for ConfigError`. We can't use - // the `Display` impl directly because `expected` is required to - // be a `'static str`. - let mut buf = String::new(); - use std::fmt::Write; - write!(buf, "invalid type: {unexpected}, expected {expected}").unwrap(); - if let Some(key) = key { - write!(buf, " for key `{key}`").unwrap(); + let stringified = value.into_string().map_err(|err| -> CommandError { + match err { + ConfigError::Type { + origin, unexpected, .. + } => ConfigGetError::Type { + name: args.name.to_string(), + error: format!("Expected a value convertible to a string, but is {unexpected}") + .into(), + source_path: origin.map(PathBuf::from), } - if let Some(origin) = origin { - write!(buf, " in {origin}").unwrap(); - } - config_error(buf) + .into(), + err => err.into(), } - err => err.into(), })?; writeln!(ui.stdout(), "{stringified}")?; Ok(()) diff --git a/cli/src/commands/git/fetch.rs b/cli/src/commands/git/fetch.rs index 9ec64b233b..bf1226d436 100644 --- a/cli/src/commands/git/fetch.rs +++ b/cli/src/commands/git/fetch.rs @@ -14,7 +14,7 @@ use clap_complete::ArgValueCandidates; use itertools::Itertools; -use jj_lib::config::ConfigResultExt as _; +use jj_lib::config::ConfigGetResultExt as _; use jj_lib::repo::Repo; use jj_lib::settings::UserSettings; use jj_lib::str_util::StringPattern; diff --git a/cli/src/commands/git/push.rs b/cli/src/commands/git/push.rs index bf3d8ea9e7..d835023940 100644 --- a/cli/src/commands/git/push.rs +++ b/cli/src/commands/git/push.rs @@ -21,7 +21,7 @@ use clap::ArgGroup; use clap_complete::ArgValueCandidates; use itertools::Itertools; use jj_lib::backend::CommitId; -use jj_lib::config::ConfigResultExt as _; +use jj_lib::config::ConfigGetResultExt as _; use jj_lib::git; use jj_lib::git::GitBranchPushTargets; use jj_lib::git::GitPushError; diff --git a/cli/src/commands/log.rs b/cli/src/commands/log.rs index 2009d9dd5f..5d514cfc83 100644 --- a/cli/src/commands/log.rs +++ b/cli/src/commands/log.rs @@ -14,8 +14,8 @@ use clap_complete::ArgValueCandidates; use jj_lib::backend::CommitId; -use jj_lib::config::ConfigError; -use jj_lib::config::ConfigResultExt as _; +use jj_lib::config::ConfigGetError; +use jj_lib::config::ConfigGetResultExt as _; use jj_lib::graph::GraphEdgeType; use jj_lib::graph::ReverseGraphIterator; use jj_lib::graph::TopoGroupedGraphIterator; @@ -331,7 +331,7 @@ pub(crate) fn cmd_log( pub fn get_node_template( style: GraphStyle, settings: &UserSettings, -) -> Result<String, ConfigError> { +) -> Result<String, ConfigGetError> { let symbol = settings.get_string("templates.log_node").optional()?; let default = if style.is_ascii() { "builtin_log_node_ascii" diff --git a/cli/src/commands/operation/log.rs b/cli/src/commands/operation/log.rs index 31d848c202..7d5d245c45 100644 --- a/cli/src/commands/operation/log.rs +++ b/cli/src/commands/operation/log.rs @@ -15,8 +15,8 @@ use std::slice; use itertools::Itertools as _; -use jj_lib::config::ConfigError; -use jj_lib::config::ConfigResultExt as _; +use jj_lib::config::ConfigGetError; +use jj_lib::config::ConfigGetResultExt as _; use jj_lib::op_walk; use jj_lib::operation::Operation; use jj_lib::repo::RepoLoader; @@ -247,7 +247,7 @@ fn do_op_log( Ok(()) } -fn get_node_template(style: GraphStyle, settings: &UserSettings) -> Result<String, ConfigError> { +fn get_node_template(style: GraphStyle, settings: &UserSettings) -> Result<String, ConfigGetError> { let symbol = settings.get_string("templates.op_log_node").optional()?; let default = if style.is_ascii() { "builtin_op_log_node_ascii" diff --git a/cli/src/diff_util.rs b/cli/src/diff_util.rs index 77187caccb..d6a5e9e1f1 100644 --- a/cli/src/diff_util.rs +++ b/cli/src/diff_util.rs @@ -32,8 +32,8 @@ use jj_lib::backend::CommitId; use jj_lib::backend::CopyRecord; use jj_lib::backend::TreeValue; use jj_lib::commit::Commit; -use jj_lib::config::ConfigError; -use jj_lib::config::ConfigResultExt as _; +use jj_lib::config::ConfigGetError; +use jj_lib::config::ConfigGetResultExt as _; use jj_lib::conflicts::materialize_merge_result_to_bytes; use jj_lib::conflicts::materialized_diff_stream; use jj_lib::conflicts::ConflictMarkerStyle; @@ -147,7 +147,7 @@ pub enum DiffFormat { pub fn diff_formats_for( settings: &UserSettings, args: &DiffFormatArgs, -) -> Result<Vec<DiffFormat>, ConfigError> { +) -> Result<Vec<DiffFormat>, ConfigGetError> { let formats = diff_formats_from_args(settings, args)?; if formats.is_empty() { Ok(vec![default_diff_format(settings, args)?]) @@ -162,7 +162,7 @@ pub fn diff_formats_for_log( settings: &UserSettings, args: &DiffFormatArgs, patch: bool, -) -> Result<Vec<DiffFormat>, ConfigError> { +) -> Result<Vec<DiffFormat>, ConfigGetError> { let mut formats = diff_formats_from_args(settings, args)?; // --patch implies default if no format other than --summary is specified if patch && matches!(formats.as_slice(), [] | [DiffFormat::Summary]) { @@ -175,7 +175,7 @@ pub fn diff_formats_for_log( fn diff_formats_from_args( settings: &UserSettings, args: &DiffFormatArgs, -) -> Result<Vec<DiffFormat>, ConfigError> { +) -> Result<Vec<DiffFormat>, ConfigGetError> { let mut formats = Vec::new(); if args.summary { formats.push(DiffFormat::Summary); @@ -209,7 +209,7 @@ fn diff_formats_from_args( fn default_diff_format( settings: &UserSettings, args: &DiffFormatArgs, -) -> Result<DiffFormat, ConfigError> { +) -> Result<DiffFormat, ConfigGetError> { if let Some(args) = settings.get("ui.diff.tool").optional()? { // External "tool" overrides the internal "format" option. let tool = if let CommandNameAndArgs::String(name) = &args { @@ -243,7 +243,11 @@ fn default_diff_format( let options = DiffStatOptions::from_args(args); Ok(DiffFormat::Stat(Box::new(options))) } - _ => Err(ConfigError::Message(format!("invalid diff format: {name}"))), + _ => Err(ConfigGetError::Type { + name: "ui.diff.format".to_owned(), + error: format!("Invalid diff format: {name}").into(), + source_path: None, + }), } } @@ -544,15 +548,16 @@ impl ColorWordsDiffOptions { fn from_settings_and_args( settings: &UserSettings, args: &DiffFormatArgs, - ) -> Result<Self, ConfigError> { + ) -> Result<Self, ConfigGetError> { let max_inline_alternation = { - let key = "diff.color-words.max-inline-alternation"; - match settings.get_int(key)? { + let name = "diff.color-words.max-inline-alternation"; + match settings.get_int(name)? { -1 => None, // unlimited - n => Some( - usize::try_from(n) - .map_err(|err| ConfigError::Message(format!("invalid {key}: {err}")))?, - ), + n => Some(usize::try_from(n).map_err(|err| ConfigGetError::Type { + name: name.to_owned(), + error: err.into(), + source_path: None, + })?), } }; let context = args @@ -1252,7 +1257,7 @@ impl UnifiedDiffOptions { fn from_settings_and_args( settings: &UserSettings, args: &DiffFormatArgs, - ) -> Result<Self, ConfigError> { + ) -> Result<Self, ConfigGetError> { let context = args .context .map_or_else(|| settings.get("diff.git.context"), Ok)?; diff --git a/cli/src/formatter.rs b/cli/src/formatter.rs index 9b88a8fa37..d2d15c24dd 100644 --- a/cli/src/formatter.rs +++ b/cli/src/formatter.rs @@ -29,7 +29,7 @@ use crossterm::style::SetAttribute; use crossterm::style::SetBackgroundColor; use crossterm::style::SetForegroundColor; use itertools::Itertools; -use jj_lib::config::ConfigError; +use jj_lib::config::ConfigGetError; use jj_lib::config::StackedConfig; // Lets the caller label strings and translates the labels to colors @@ -160,7 +160,7 @@ impl FormatterFactory { FormatterFactory { kind } } - pub fn color(config: &StackedConfig, debug: bool) -> Result<Self, ConfigError> { + pub fn color(config: &StackedConfig, debug: bool) -> Result<Self, ConfigGetError> { let rules = Arc::new(rules_from_config(config)?); let kind = FormatterFactoryKind::Color { rules, debug }; Ok(FormatterFactory { kind }) @@ -297,7 +297,11 @@ impl<W: Write> ColorFormatter<W> { } } - pub fn for_config(output: W, config: &StackedConfig, debug: bool) -> Result<Self, ConfigError> { + pub fn for_config( + output: W, + config: &StackedConfig, + debug: bool, + ) -> Result<Self, ConfigGetError> { let rules = rules_from_config(config)?; Ok(Self::new(output, Arc::new(rules), debug)) } @@ -401,10 +405,15 @@ impl<W: Write> ColorFormatter<W> { } } -fn rules_from_config(config: &StackedConfig) -> Result<Rules, ConfigError> { +fn rules_from_config(config: &StackedConfig) -> Result<Rules, ConfigGetError> { let mut result = vec![]; let table = config.get_table("colors")?; for (key, value) in table { + let to_config_err = |message: String| ConfigGetError::Type { + name: format!("colors.'{key}'"), + error: message.into(), + source_path: None, + }; let labels = key .split_whitespace() .map(ToString::to_string) @@ -412,7 +421,7 @@ fn rules_from_config(config: &StackedConfig) -> Result<Rules, ConfigError> { match value.kind { config::ValueKind::String(color_name) => { let style = Style { - fg_color: Some(color_for_name_or_hex(&color_name)?), + fg_color: Some(color_for_name_or_hex(&color_name).map_err(to_config_err)?), bg_color: None, bold: None, underlined: None, @@ -423,12 +432,14 @@ fn rules_from_config(config: &StackedConfig) -> Result<Rules, ConfigError> { let mut style = Style::default(); if let Some(value) = style_table.get("fg") { if let config::ValueKind::String(color_name) = &value.kind { - style.fg_color = Some(color_for_name_or_hex(color_name)?); + style.fg_color = + Some(color_for_name_or_hex(color_name).map_err(to_config_err)?); } } if let Some(value) = style_table.get("bg") { if let config::ValueKind::String(color_name) = &value.kind { - style.bg_color = Some(color_for_name_or_hex(color_name)?); + style.bg_color = + Some(color_for_name_or_hex(color_name).map_err(to_config_err)?); } } if let Some(value) = style_table.get("bold") { @@ -449,7 +460,7 @@ fn rules_from_config(config: &StackedConfig) -> Result<Rules, ConfigError> { Ok(result) } -fn color_for_name_or_hex(name_or_hex: &str) -> Result<Color, ConfigError> { +fn color_for_name_or_hex(name_or_hex: &str) -> Result<Color, String> { match name_or_hex { "default" => Ok(Color::Reset), "black" => Ok(Color::Black), @@ -468,8 +479,7 @@ fn color_for_name_or_hex(name_or_hex: &str) -> Result<Color, ConfigError> { "bright magenta" => Ok(Color::Magenta), "bright cyan" => Ok(Color::Cyan), "bright white" => Ok(Color::White), - _ => color_for_hex(name_or_hex) - .ok_or_else(|| ConfigError::Message(format!("invalid color: {name_or_hex}"))), + _ => color_for_hex(name_or_hex).ok_or_else(|| format!("Invalid color: {name_or_hex}")), } } @@ -699,6 +709,7 @@ fn write_sanitized(output: &mut impl Write, buf: &[u8]) -> Result<(), Error> { #[cfg(test)] mod tests { + use std::error::Error as _; use std::str; use indoc::indoc; @@ -1018,11 +1029,9 @@ mod tests { "#, ); let mut output: Vec<u8> = vec![]; - let err = ColorFormatter::for_config(&mut output, &config, false) - .unwrap_err() - .to_string(); - insta::assert_snapshot!(err, - @"invalid color: bloo"); + let err = ColorFormatter::for_config(&mut output, &config, false).unwrap_err(); + insta::assert_snapshot!(err, @"Invalid type or value for colors.'outer inner'"); + insta::assert_snapshot!(err.source().unwrap(), @"Invalid color: bloo"); } #[test] @@ -1035,11 +1044,9 @@ mod tests { "##, ); let mut output: Vec<u8> = vec![]; - let err = ColorFormatter::for_config(&mut output, &config, false) - .unwrap_err() - .to_string(); - insta::assert_snapshot!(err, - @"invalid color: #ffgggg"); + let err = ColorFormatter::for_config(&mut output, &config, false).unwrap_err(); + insta::assert_snapshot!(err, @"Invalid type or value for colors.'outer inner'"); + insta::assert_snapshot!(err.source().unwrap(), @"Invalid color: #ffgggg"); } #[test] diff --git a/cli/src/graphlog.rs b/cli/src/graphlog.rs index 550592b02e..de168e43dc 100644 --- a/cli/src/graphlog.rs +++ b/cli/src/graphlog.rs @@ -17,7 +17,7 @@ use std::io; use std::io::Write; use itertools::Itertools; -use jj_lib::config::ConfigError; +use jj_lib::config::ConfigGetError; use jj_lib::settings::UserSettings; use renderdag::Ancestor; use renderdag::GraphRowRenderer; @@ -113,7 +113,7 @@ pub enum GraphStyle { } impl GraphStyle { - pub fn from_settings(settings: &UserSettings) -> Result<Self, ConfigError> { + pub fn from_settings(settings: &UserSettings) -> Result<Self, ConfigGetError> { settings.get("ui.graph.style") } diff --git a/cli/src/merge_tools/mod.rs b/cli/src/merge_tools/mod.rs index 83002607b7..25d829936a 100644 --- a/cli/src/merge_tools/mod.rs +++ b/cli/src/merge_tools/mod.rs @@ -19,9 +19,9 @@ mod external; use std::sync::Arc; use jj_lib::backend::MergedTreeId; -use jj_lib::config::ConfigError; +use jj_lib::config::ConfigGetError; +use jj_lib::config::ConfigGetResultExt as _; use jj_lib::config::ConfigNamePathBuf; -use jj_lib::config::ConfigResultExt as _; use jj_lib::conflicts::extract_as_single_hunk; use jj_lib::conflicts::ConflictMarkerStyle; use jj_lib::gitignore::GitIgnoreFile; @@ -62,7 +62,7 @@ pub enum DiffEditError { #[error("Failed to snapshot changes")] Snapshot(#[from] SnapshotError), #[error(transparent)] - Config(#[from] ConfigError), + Config(#[from] ConfigGetError), } #[derive(Debug, Error)] @@ -104,7 +104,7 @@ pub enum ConflictResolveError { #[derive(Debug, Error)] pub enum MergeToolConfigError { #[error(transparent)] - Config(#[from] ConfigError), + Config(#[from] ConfigGetError), #[error("The tool `{tool_name}` cannot be used as a merge tool with `jj resolve`")] MergeArgsNotConfigured { tool_name: String }, } @@ -127,7 +127,7 @@ fn editor_args_from_settings( ui: &Ui, settings: &UserSettings, key: &'static str, -) -> Result<CommandNameAndArgs, ConfigError> { +) -> Result<CommandNameAndArgs, ConfigGetError> { // TODO: Make this configuration have a table of possible editors and detect the // best one here. if let Some(args) = settings.get(key).optional()? { @@ -146,7 +146,10 @@ fn editor_args_from_settings( /// Resolves builtin merge tool name or loads external tool options from /// `[merge-tools.<name>]`. -fn get_tool_config(settings: &UserSettings, name: &str) -> Result<Option<MergeTool>, ConfigError> { +fn get_tool_config( + settings: &UserSettings, + name: &str, +) -> Result<Option<MergeTool>, ConfigGetError> { if name == BUILTIN_EDITOR_NAME { Ok(Some(MergeTool::Builtin)) } else { @@ -158,14 +161,9 @@ fn get_tool_config(settings: &UserSettings, name: &str) -> Result<Option<MergeTo pub fn get_external_tool_config( settings: &UserSettings, name: &str, -) -> Result<Option<ExternalMergeTool>, ConfigError> { +) -> Result<Option<ExternalMergeTool>, ConfigGetError> { let full_name = ConfigNamePathBuf::from_iter(["merge-tools", name]); - let Some(mut tool) = settings - .get::<ExternalMergeTool>(&full_name) - .optional() - // add config key, deserialize error is otherwise unclear - .map_err(|e| ConfigError::Message(format!("{full_name}: {e}")))? - else { + let Some(mut tool) = settings.get::<ExternalMergeTool>(&full_name).optional()? else { return Ok(None); }; if tool.program.is_empty() { diff --git a/cli/src/revset_util.rs b/cli/src/revset_util.rs index 42a206b97f..32af07a205 100644 --- a/cli/src/revset_util.rs +++ b/cli/src/revset_util.rs @@ -21,6 +21,7 @@ use std::sync::Arc; use itertools::Itertools as _; use jj_lib::backend::CommitId; use jj_lib::commit::Commit; +use jj_lib::config::ConfigGetError; use jj_lib::config::ConfigNamePathBuf; use jj_lib::config::ConfigSource; use jj_lib::config::StackedConfig; @@ -174,8 +175,16 @@ pub fn load_revset_aliases( let table = match layer.look_up_table(&table_name) { Ok(Some(table)) => table, Ok(None) => continue, - // TODO: rewrite error construction after migrating to toml_edit - Err(item) => return Err(item.clone().into_table().unwrap_err().into()), + Err(item) => { + // TODO: rewrite error construction after migrating to toml_edit + let error = item.clone().into_table().unwrap_err(); + return Err(ConfigGetError::Type { + name: table_name.to_string(), + error: error.into(), + source_path: layer.path.clone(), + } + .into()); + } }; // TODO: remove sorting after migrating to toml_edit for (decl, value) in table.iter().sorted_by_key(|&(decl, _)| decl) { diff --git a/cli/src/ui.rs b/cli/src/ui.rs index 333bce96a1..cd4ec6fb2f 100644 --- a/cli/src/ui.rs +++ b/cli/src/ui.rs @@ -32,13 +32,12 @@ use std::thread::JoinHandle; use indoc::indoc; use itertools::Itertools as _; -use jj_lib::config::ConfigError; +use jj_lib::config::ConfigGetError; use jj_lib::config::StackedConfig; use minus::MinusError; use minus::Pager as MinusPager; use tracing::instrument; -use crate::command_error::config_error_with_message; use crate::command_error::CommandError; use crate::config::CommandNameAndArgs; use crate::formatter::Formatter; @@ -313,7 +312,7 @@ fn color_setting(config: &StackedConfig) -> ColorChoice { fn prepare_formatter_factory( config: &StackedConfig, stdout: &Stdout, -) -> Result<FormatterFactory, ConfigError> { +) -> Result<FormatterFactory, ConfigGetError> { let terminal = stdout.is_terminal(); let (color, debug) = match color_setting(config) { ColorChoice::Always => (true, false), @@ -344,16 +343,12 @@ pub enum PaginationChoice { Auto, } -fn pagination_setting(config: &StackedConfig) -> Result<PaginationChoice, CommandError> { - config - .get::<PaginationChoice>("ui.paginate") - .map_err(|err| config_error_with_message("Invalid `ui.paginate`", err)) +fn pagination_setting(config: &StackedConfig) -> Result<PaginationChoice, ConfigGetError> { + config.get::<PaginationChoice>("ui.paginate") } -fn pager_setting(config: &StackedConfig) -> Result<CommandNameAndArgs, CommandError> { - config - .get::<CommandNameAndArgs>("ui.pager") - .map_err(|err| config_error_with_message("Invalid `ui.pager`", err)) +fn pager_setting(config: &StackedConfig) -> Result<CommandNameAndArgs, ConfigGetError> { + config.get::<CommandNameAndArgs>("ui.pager") } impl Ui { diff --git a/cli/tests/test_config_command.rs b/cli/tests/test_config_command.rs index ede468a359..06fd7b08c5 100644 --- a/cli/tests/test_config_command.rs +++ b/cli/tests/test_config_command.rs @@ -820,10 +820,10 @@ fn test_config_get() { ); let stdout = test_env.jj_cmd_failure(test_env.env_root(), &["config", "get", "nonexistent"]); - insta::assert_snapshot!(stdout, @r###" - Config error: configuration property "nonexistent" not found + insta::assert_snapshot!(stdout, @r" + Config error: Value not found for nonexistent For help, see https://martinvonz.github.io/jj/latest/config/. - "###); + "); let stdout = test_env.jj_cmd_success(test_env.env_root(), &["config", "get", "table.string"]); insta::assert_snapshot!(stdout, @r###" @@ -837,15 +837,18 @@ fn test_config_get() { let stdout = test_env.jj_cmd_failure(test_env.env_root(), &["config", "get", "table.list"]); insta::assert_snapshot!(stdout.replace('\\', "/"), @r" - Config error: invalid type: sequence, expected a value convertible to a string in config/config0002.toml + Config error: Invalid type or value for table.list + Caused by: Expected a value convertible to a string, but is sequence + Hint: Check the config file: config/config0002.toml For help, see https://martinvonz.github.io/jj/latest/config/. "); let stdout = test_env.jj_cmd_failure(test_env.env_root(), &["config", "get", "table"]); - insta::assert_snapshot!(stdout, @r###" - Config error: invalid type: map, expected a value convertible to a string + insta::assert_snapshot!(stdout, @r" + Config error: Invalid type or value for table + Caused by: Expected a value convertible to a string, but is map For help, see https://martinvonz.github.io/jj/latest/config/. - "###); + "); let stdout = test_env.jj_cmd_success(test_env.env_root(), &["config", "get", "table.overridden"]); @@ -896,10 +899,10 @@ fn test_config_path_syntax() { Warning: No matching config key for a.'b()'.x "###); let stderr = test_env.jj_cmd_failure(test_env.env_root(), &["config", "get", "a.'b()'.x"]); - insta::assert_snapshot!(stderr, @r###" - Config error: configuration property "a.'b()'.x" not found + insta::assert_snapshot!(stderr, @r" + Config error: Value not found for a.'b()'.x For help, see https://martinvonz.github.io/jj/latest/config/. - "###); + "); // "-" and "_" are valid TOML keys let stdout = test_env.jj_cmd_success(test_env.env_root(), &["config", "list", "-"]); @@ -970,13 +973,12 @@ fn test_config_show_paths() { &["config", "set", "--user", "ui.paginate", ":builtin"], ); let stderr = test_env.jj_cmd_failure(test_env.env_root(), &["st"]); - insta::assert_snapshot!(stderr, @r###" - Config error: Invalid `ui.paginate` + insta::assert_snapshot!(stderr, @r" + Config error: Invalid type or value for ui.paginate Caused by: enum PaginationChoice does not have variant constructor :builtin - Hint: Check the following config files: - - $TEST_ENV/config/config.toml + Hint: Check the config file: $TEST_ENV/config/config.toml For help, see https://martinvonz.github.io/jj/latest/config/. - "###); + "); } #[test] diff --git a/cli/tests/test_global_opts.rs b/cli/tests/test_global_opts.rs index 475d388dfa..3f7b476ed0 100644 --- a/cli/tests/test_global_opts.rs +++ b/cli/tests/test_global_opts.rs @@ -604,7 +604,8 @@ fn test_invalid_config_value() { &["status", "--config-toml=snapshot.auto-track=[0]"], ); insta::assert_snapshot!(stderr, @r" - Config error: invalid type: sequence, expected a string for key `snapshot.auto-track` + Config error: Invalid type or value for snapshot.auto-track + Caused by: invalid type: sequence, expected a string For help, see https://martinvonz.github.io/jj/latest/config/. "); } diff --git a/cli/tests/test_log_command.rs b/cli/tests/test_log_command.rs index 3bf63a9cc5..48e7c706c3 100644 --- a/cli/tests/test_log_command.rs +++ b/cli/tests/test_log_command.rs @@ -1324,10 +1324,11 @@ fn test_graph_styles() { &repo_path, &["log", "--config-toml=ui.graph.style='unknown'"], ); - insta::assert_snapshot!(stderr, @r###" - Config error: enum GraphStyle does not have variant constructor unknown + insta::assert_snapshot!(stderr, @r" + Config error: Invalid type or value for ui.graph.style + Caused by: enum GraphStyle does not have variant constructor unknown For help, see https://martinvonz.github.io/jj/latest/config/. - "###); + "); } #[test] diff --git a/lib/src/config.rs b/lib/src/config.rs index e69cb8333b..382fd33ae6 100644 --- a/lib/src/config.rs +++ b/lib/src/config.rs @@ -15,6 +15,7 @@ //! Configuration store helpers. use std::borrow::Borrow; +use std::convert::Infallible; use std::fmt; use std::ops::Range; use std::path::Path; @@ -24,6 +25,7 @@ use std::str::FromStr; use itertools::Itertools as _; use serde::Deserialize; +use thiserror::Error; use crate::file_util::IoResultExt as _; @@ -36,17 +38,39 @@ pub type ConfigValue = config::Value; // TODO: will be replaced with our custom error type pub type ConfigError = config::ConfigError; -/// Extension methods for `Result<T, ConfigError>`. -pub trait ConfigResultExt<T> { +/// Error that can occur when looking up config variable. +#[derive(Debug, Error)] +pub enum ConfigGetError { + /// Config value is not set. + #[error("Value not found for {name}")] + NotFound { + /// Dotted config name path. + name: String, + }, + /// Config value cannot be converted to the expected type. + #[error("Invalid type or value for {name}")] + Type { + /// Dotted config name path. + name: String, + /// Source error. + #[source] + error: Box<dyn std::error::Error + Send + Sync>, + /// Source file path where the value is defined. + source_path: Option<PathBuf>, + }, +} + +/// Extension methods for `Result<T, ConfigGetError>`. +pub trait ConfigGetResultExt<T> { /// Converts `NotFound` error to `Ok(None)`, leaving other errors. - fn optional(self) -> Result<Option<T>, ConfigError>; + fn optional(self) -> Result<Option<T>, ConfigGetError>; } -impl<T> ConfigResultExt<T> for Result<T, ConfigError> { - fn optional(self) -> Result<Option<T>, ConfigError> { +impl<T> ConfigGetResultExt<T> for Result<T, ConfigGetError> { + fn optional(self) -> Result<Option<T>, ConfigGetError> { match self { Ok(value) => Ok(Some(value)), - Err(ConfigError::NotFound(_)) => Ok(None), + Err(ConfigGetError::NotFound { .. }) => Ok(None), Err(err) => Err(err), } } @@ -396,37 +420,42 @@ impl StackedConfig { pub fn get<'de, T: Deserialize<'de>>( &self, name: impl ToConfigNamePath, - ) -> Result<T, ConfigError> { + ) -> Result<T, ConfigGetError> { self.get_item_with(name, T::deserialize) } /// Looks up value from all layers, merges sub fields as needed. - pub fn get_value(&self, name: impl ToConfigNamePath) -> Result<ConfigValue, ConfigError> { - self.get_item_with(name, Ok) + pub fn get_value(&self, name: impl ToConfigNamePath) -> Result<ConfigValue, ConfigGetError> { + self.get_item_with::<_, Infallible>(name, Ok) } /// Looks up sub table from all layers, merges fields as needed. // TODO: redesign this to attach better error indication? - pub fn get_table(&self, name: impl ToConfigNamePath) -> Result<ConfigTable, ConfigError> { + pub fn get_table(&self, name: impl ToConfigNamePath) -> Result<ConfigTable, ConfigGetError> { self.get(name) } - fn get_item_with<T>( + fn get_item_with<T, E: Into<Box<dyn std::error::Error + Send + Sync>>>( &self, name: impl ToConfigNamePath, - convert: impl FnOnce(ConfigValue) -> Result<T, ConfigError>, - ) -> Result<T, ConfigError> { + convert: impl FnOnce(ConfigValue) -> Result<T, E>, + ) -> Result<T, ConfigGetError> { let name = name.into_name_path(); let name = name.borrow(); - let (item, _layer_index) = get_merged_item(&self.layers, name) - .ok_or_else(|| ConfigError::NotFound(name.to_string()))?; - // TODO: Add source type/path to the error message. If the value is - // a table, the error might come from lower layers. We cannot report - // precise source information in that case. However, toml_edit captures - // dotted keys in the error object. If the keys field were public, we - // can look up the source information. This is probably simpler than - // reimplementing Deserializer. - convert(item).map_err(|err| err.extend_with_key(&name.to_string())) + let (item, layer_index) = + get_merged_item(&self.layers, name).ok_or_else(|| ConfigGetError::NotFound { + name: name.to_string(), + })?; + // If the value is a table, the error might come from lower layers. We + // cannot report precise source information in that case. However, + // toml_edit captures dotted keys in the error object. If the keys field + // were public, we can look up the source information. This is probably + // simpler than reimplementing Deserializer. + convert(item).map_err(|err| ConfigGetError::Type { + name: name.to_string(), + error: err.into(), + source_path: self.layers[layer_index].path.clone(), + }) } } @@ -587,19 +616,19 @@ mod tests { // Table "a.b" exists, but key doesn't assert_matches!( config.get::<String>("a.b.missing"), - Err(ConfigError::NotFound(name)) if name == "a.b.missing" + Err(ConfigGetError::NotFound { name }) if name == "a.b.missing" ); // Node "a.b.c" is not a table assert_matches!( config.get::<String>("a.b.c.d"), - Err(ConfigError::NotFound(name)) if name == "a.b.c.d" + Err(ConfigGetError::NotFound { name }) if name == "a.b.c.d" ); // Type error assert_matches!( config.get::<String>("a.b"), - Err(ConfigError::Type { key: Some(name), .. }) if name == "a.b" + Err(ConfigGetError::Type { name, .. }) if name == "a.b" ); } @@ -618,7 +647,7 @@ mod tests { assert_matches!( config.get::<String>("a.b.c"), - Err(ConfigError::NotFound(name)) if name == "a.b.c" + Err(ConfigGetError::NotFound { name }) if name == "a.b.c" ); } diff --git a/lib/src/fsmonitor.rs b/lib/src/fsmonitor.rs index 767620da27..9fa864f457 100644 --- a/lib/src/fsmonitor.rs +++ b/lib/src/fsmonitor.rs @@ -24,8 +24,8 @@ use std::path::PathBuf; -use crate::config::ConfigError; -use crate::config::ConfigResultExt as _; +use crate::config::ConfigGetError; +use crate::config::ConfigGetResultExt as _; use crate::settings::UserSettings; /// Config for Watchman filesystem monitor (<https://facebook.github.io/watchman/>). @@ -58,8 +58,9 @@ pub enum FsmonitorSettings { impl FsmonitorSettings { /// Creates an `FsmonitorSettings` from a `config`. - pub fn from_settings(settings: &UserSettings) -> Result<FsmonitorSettings, ConfigError> { - match settings.get_string("core.fsmonitor") { + pub fn from_settings(settings: &UserSettings) -> Result<FsmonitorSettings, ConfigGetError> { + let name = "core.fsmonitor"; + match settings.get_string(name) { Ok(s) => match s.as_str() { "watchman" => Ok(Self::Watchman(WatchmanConfig { register_trigger: settings @@ -67,15 +68,19 @@ impl FsmonitorSettings { .optional()? .unwrap_or_default(), })), - "test" => Err(ConfigError::Message( - "cannot use test fsmonitor in real repository".to_string(), - )), + "test" => Err(ConfigGetError::Type { + name: name.to_owned(), + error: "Cannot use test fsmonitor in real repository".into(), + source_path: None, + }), "none" => Ok(Self::None), - other => Err(ConfigError::Message(format!( - "unknown fsmonitor kind: {other}", - ))), + other => Err(ConfigGetError::Type { + name: name.to_owned(), + error: format!("Unknown fsmonitor kind: {other}").into(), + source_path: None, + }), }, - Err(ConfigError::NotFound(_)) => Ok(Self::None), + Err(ConfigGetError::NotFound { .. }) => Ok(Self::None), Err(err) => Err(err), } } diff --git a/lib/src/gpg_signing.rs b/lib/src/gpg_signing.rs index 654b4ba680..6ba6dfb2f0 100644 --- a/lib/src/gpg_signing.rs +++ b/lib/src/gpg_signing.rs @@ -25,8 +25,8 @@ use std::str; use thiserror::Error; -use crate::config::ConfigError; -use crate::config::ConfigResultExt as _; +use crate::config::ConfigGetError; +use crate::config::ConfigGetResultExt as _; use crate::settings::UserSettings; use crate::signing::SigStatus; use crate::signing::SignError; @@ -146,7 +146,7 @@ impl GpgBackend { self } - pub fn from_settings(settings: &UserSettings) -> Result<Self, ConfigError> { + pub fn from_settings(settings: &UserSettings) -> Result<Self, ConfigGetError> { let program = settings .get_string("signing.backends.gpg.program") .optional()? diff --git a/lib/src/settings.rs b/lib/src/settings.rs index 6968690db4..b15bf5b1b5 100644 --- a/lib/src/settings.rs +++ b/lib/src/settings.rs @@ -27,8 +27,8 @@ use crate::backend::ChangeId; use crate::backend::Commit; use crate::backend::Signature; use crate::backend::Timestamp; -use crate::config::ConfigError; -use crate::config::ConfigResultExt as _; +use crate::config::ConfigGetError; +use crate::config::ConfigGetResultExt as _; use crate::config::ConfigTable; use crate::config::ConfigValue; use crate::config::StackedConfig; @@ -166,7 +166,7 @@ impl UserSettings { self.get_string("user.email").unwrap_or_default() } - pub fn fsmonitor_settings(&self) -> Result<FsmonitorSettings, ConfigError> { + pub fn fsmonitor_settings(&self) -> Result<FsmonitorSettings, ConfigGetError> { FsmonitorSettings::from_settings(self) } @@ -234,14 +234,14 @@ impl UserSettings { GitSettings::from_settings(self) } - pub fn max_new_file_size(&self) -> Result<u64, ConfigError> { + pub fn max_new_file_size(&self) -> Result<u64, ConfigGetError> { let cfg = self .get::<HumanByteSize>("snapshot.max-new-file-size") .map(|x| x.0); match cfg { Ok(0) => Ok(u64::MAX), x @ Ok(_) => x, - Err(ConfigError::NotFound(_)) => Ok(1024 * 1024), + Err(ConfigGetError::NotFound { .. }) => Ok(1024 * 1024), e @ Err(_) => e, } } @@ -257,7 +257,7 @@ impl UserSettings { SignSettings::from_settings(self) } - pub fn conflict_marker_style(&self) -> Result<ConflictMarkerStyle, ConfigError> { + pub fn conflict_marker_style(&self) -> Result<ConflictMarkerStyle, ConfigGetError> { Ok(self .get("ui.conflict-marker-style") .optional()? @@ -271,32 +271,32 @@ impl UserSettings { pub fn get<'de, T: Deserialize<'de>>( &self, name: impl ToConfigNamePath, - ) -> Result<T, ConfigError> { + ) -> Result<T, ConfigGetError> { self.config.get(name) } /// Looks up string value by `name`. - pub fn get_string(&self, name: impl ToConfigNamePath) -> Result<String, ConfigError> { + pub fn get_string(&self, name: impl ToConfigNamePath) -> Result<String, ConfigGetError> { self.get(name) } /// Looks up integer value by `name`. - pub fn get_int(&self, name: impl ToConfigNamePath) -> Result<i64, ConfigError> { + pub fn get_int(&self, name: impl ToConfigNamePath) -> Result<i64, ConfigGetError> { self.get(name) } /// Looks up boolean value by `name`. - pub fn get_bool(&self, name: impl ToConfigNamePath) -> Result<bool, ConfigError> { + pub fn get_bool(&self, name: impl ToConfigNamePath) -> Result<bool, ConfigGetError> { self.get(name) } /// Looks up generic value by `name`. - pub fn get_value(&self, name: impl ToConfigNamePath) -> Result<ConfigValue, ConfigError> { + pub fn get_value(&self, name: impl ToConfigNamePath) -> Result<ConfigValue, ConfigGetError> { self.config.get_value(name) } /// Looks up sub table by `name`. - pub fn get_table(&self, name: impl ToConfigNamePath) -> Result<ConfigTable, ConfigError> { + pub fn get_table(&self, name: impl ToConfigNamePath) -> Result<ConfigTable, ConfigGetError> { self.config.get_table(name) } } diff --git a/lib/src/signing.rs b/lib/src/signing.rs index d0d594988e..6a23410b8e 100644 --- a/lib/src/signing.rs +++ b/lib/src/signing.rs @@ -22,7 +22,7 @@ use std::sync::RwLock; use thiserror::Error; use crate::backend::CommitId; -use crate::config::ConfigError; +use crate::config::ConfigGetError; use crate::gpg_signing::GpgBackend; use crate::settings::UserSettings; use crate::ssh_signing::SshBackend; @@ -122,7 +122,7 @@ pub enum SignInitError { UnknownBackend(String), /// Failed to load backend configuration. #[error("Failed to configure signing backend")] - BackendConfig(#[source] ConfigError), + BackendConfig(#[source] ConfigGetError), } /// A enum that describes if a created/rewritten commit should be signed or not. diff --git a/lib/src/ssh_signing.rs b/lib/src/ssh_signing.rs index 554b2fd2a5..e19f489fbf 100644 --- a/lib/src/ssh_signing.rs +++ b/lib/src/ssh_signing.rs @@ -26,8 +26,8 @@ use std::process::Stdio; use either::Either; use thiserror::Error; -use crate::config::ConfigError; -use crate::config::ConfigResultExt as _; +use crate::config::ConfigGetError; +use crate::config::ConfigGetResultExt as _; use crate::settings::UserSettings; use crate::signing::SigStatus; use crate::signing::SignError; @@ -119,7 +119,7 @@ impl SshBackend { } } - pub fn from_settings(settings: &UserSettings) -> Result<Self, ConfigError> { + pub fn from_settings(settings: &UserSettings) -> Result<Self, ConfigGetError> { let program = settings .get_string("signing.backends.ssh.program") .optional()?