config: add ConfigLoadError, replace uses of config::ConfigError

cli/src/command_error.rs

@@ -22,8 +22,8 @@ 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::config::ConfigLoadError;
 use jj_lib::dsl_util::Diagnostics;
 use jj_lib::fileset::FilePatternParseError;
 use jj_lib::fileset::FilesetParseError;
@@ -240,12 +240,6 @@ impl From<jj_lib::file_util::PathError> for CommandError {
     }
 }
 
-impl From<ConfigError> for CommandError {
-    fn from(err: ConfigError) -> Self {
-        config_error(err)
-    }
-}
-
 impl From<ConfigEnvError> for CommandError {
     fn from(err: ConfigEnvError) -> Self {
         config_error(err)
@@ -266,6 +260,20 @@ impl From<ConfigGetError> for CommandError {
     }
 }
 
+impl From<ConfigLoadError> for CommandError {
+    fn from(err: ConfigLoadError) -> Self {
+        let hint = match &err {
+            ConfigLoadError::Read(_) => None,
+            ConfigLoadError::Parse { 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)

cli/src/config.rs

@@ -22,8 +22,8 @@ use std::path::PathBuf;
 use std::process::Command;
 
 use itertools::Itertools;
-use jj_lib::config::ConfigError;
 use jj_lib::config::ConfigLayer;
+use jj_lib::config::ConfigLoadError;
 use jj_lib::config::ConfigNamePathBuf;
 use jj_lib::config::ConfigSource;
 use jj_lib::config::ConfigValue;
@@ -263,7 +263,7 @@ impl ConfigEnv {
     /// Loads user-specific config files into the given `config`. The old
     /// user-config layers will be replaced if any.
     #[instrument]
-    pub fn reload_user_config(&self, config: &mut StackedConfig) -> Result<(), ConfigError> {
+    pub fn reload_user_config(&self, config: &mut StackedConfig) -> Result<(), ConfigLoadError> {
         config.remove_layers(ConfigSource::User);
         if let Some(path) = self.existing_user_config_path() {
             if path.is_dir() {
@@ -301,7 +301,7 @@ impl ConfigEnv {
     /// Loads repo-specific config file into the given `config`. The old
     /// repo-config layer will be replaced if any.
     #[instrument]
-    pub fn reload_repo_config(&self, config: &mut StackedConfig) -> Result<(), ConfigError> {
+    pub fn reload_repo_config(&self, config: &mut StackedConfig) -> Result<(), ConfigLoadError> {
         config.remove_layers(ConfigSource::Repo);
         if let Some(path) = self.existing_repo_config_path() {
             config.load_file(ConfigSource::Repo, path)?;
@@ -403,7 +403,7 @@ fn env_overrides_layer() -> ConfigLayer {
 }
 
 /// Parses `--config-toml` arguments.
-pub fn parse_config_args(toml_strs: &[String]) -> Result<Vec<ConfigLayer>, ConfigError> {
+pub fn parse_config_args(toml_strs: &[String]) -> Result<Vec<ConfigLayer>, ConfigLoadError> {
     // It might look silly that a layer is constructed per argument, but
     // --config-toml argument can contain a full TOML document, and it makes
     // sense to preserve line numbers within the doc. If we add

cli/tests/test_fix_command.rs

@@ -171,13 +171,15 @@ fn test_config_multiple_tools_with_same_name() {
 
     let stderr = test_env.jj_cmd_failure(&repo_path, &["fix"]);
     insta::assert_snapshot!(stderr, @r"
-    Config error: TOML parse error at line 6, column 9
+    Config error: Configuration cannot be parsed as TOML document
+    Caused by: TOML parse error at line 6, column 9
       |
     6 |         [fix.tools.my-tool]
       |         ^
     invalid table header
     duplicate key `my-tool` in table `fix.tools`
-     in $TEST_ENV/config/config0002.toml
+
+    Hint: Check the config file: $TEST_ENV/config/config0002.toml
     For help, see https://martinvonz.github.io/jj/latest/config/.
     ");
 

cli/tests/test_global_opts.rs

@@ -587,13 +587,15 @@ fn test_invalid_config() {
     test_env.add_config("[section]key = value-missing-quotes");
     let stderr = test_env.jj_cmd_failure(test_env.env_root(), &["init", "repo"]);
     insta::assert_snapshot!(stderr, @r"
-    Config error: TOML parse error at line 1, column 10
+    Config error: Configuration cannot be parsed as TOML document
+    Caused by: TOML parse error at line 1, column 10
       |
     1 | [section]key = value-missing-quotes
       |          ^
     invalid table header
     expected newline, `#`
-     in $TEST_ENV/config/config0002.toml
+
+    Hint: Check the config file: $TEST_ENV/config/config0002.toml
     For help, see https://martinvonz.github.io/jj/latest/config/.
     ");
 }

lib/src/config.rs

@@ -32,6 +32,7 @@ use toml_edit::DocumentMut;
 use toml_edit::ImDocument;
 
 use crate::file_util::IoResultExt as _;
+use crate::file_util::PathError;
 
 /// Config value or table node.
 pub type ConfigItem = toml_edit::Item;
@@ -40,9 +41,22 @@ pub type ConfigTable = toml_edit::Table;
 /// Generic config value.
 pub type ConfigValue = toml_edit::Value;
 
-/// Error that can occur when accessing configuration.
-// TODO: will be replaced with our custom error type
-pub type ConfigError = config::ConfigError;
+/// Error that can occur when parsing or loading config variables.
+#[derive(Debug, Error)]
+pub enum ConfigLoadError {
+    /// Config file or directory cannot be read.
+    #[error("Failed to read configuration file")]
+    Read(#[source] PathError),
+    /// TOML file or text cannot be parsed.
+    #[error("Configuration cannot be parsed as TOML document")]
+    Parse {
+        /// Source error.
+        #[source]
+        error: toml_edit::TomlError,
+        /// Source file path.
+        source_path: Option<PathBuf>,
+    },
+}
 
 /// Error that can occur when looking up config variable.
 #[derive(Debug, Error)]
@@ -270,22 +284,21 @@ impl ConfigLayer {
     }
 
     /// Parses TOML document `text` into new layer.
-    pub fn parse(source: ConfigSource, text: &str) -> Result<Self, ConfigError> {
-        let data = ImDocument::parse(text).map_err(|err| ConfigError::FileParse {
-            uri: None,
-            cause: err.into(),
+    pub fn parse(source: ConfigSource, text: &str) -> Result<Self, ConfigLoadError> {
+        let data = ImDocument::parse(text).map_err(|error| ConfigLoadError::Parse {
+            error,
+            source_path: None,
         })?;
         Ok(Self::with_data(source, data.into_mut()))
     }
 
-    fn load_from_file(source: ConfigSource, path: PathBuf) -> Result<Self, ConfigError> {
-        let text = fs::read_to_string(&path).map_err(|err| ConfigError::FileParse {
-            uri: Some(path.to_string_lossy().into_owned()),
-            cause: err.into(),
-        })?;
-        let data = ImDocument::parse(text).map_err(|err| ConfigError::FileParse {
-            uri: Some(path.to_string_lossy().into_owned()),
-            cause: err.into(),
+    fn load_from_file(source: ConfigSource, path: PathBuf) -> Result<Self, ConfigLoadError> {
+        let text = fs::read_to_string(&path)
+            .context(&path)
+            .map_err(ConfigLoadError::Read)?;
+        let data = ImDocument::parse(text).map_err(|error| ConfigLoadError::Parse {
+            error,
+            source_path: Some(path.clone()),
         })?;
         Ok(ConfigLayer {
             source,
@@ -294,7 +307,7 @@ impl ConfigLayer {
         })
     }
 
-    fn load_from_dir(source: ConfigSource, path: &Path) -> Result<Vec<Self>, ConfigError> {
+    fn load_from_dir(source: ConfigSource, path: &Path) -> Result<Vec<Self>, ConfigLoadError> {
         // TODO: Walk the directory recursively?
         let mut file_paths: Vec<_> = path
             .read_dir()
@@ -306,7 +319,7 @@ impl ConfigLayer {
                     .try_collect()
             })
             .context(path)
-            .map_err(|err| ConfigError::Foreign(err.into()))?;
+            .map_err(ConfigLoadError::Read)?;
         file_paths.sort_unstable();
         file_paths
             .into_iter()
@@ -431,7 +444,7 @@ impl StackedConfig {
         &mut self,
         source: ConfigSource,
         path: impl Into<PathBuf>,
-    ) -> Result<(), ConfigError> {
+    ) -> Result<(), ConfigLoadError> {
         let layer = ConfigLayer::load_from_file(source, path.into())?;
         self.add_layer(layer);
         Ok(())
@@ -443,7 +456,7 @@ impl StackedConfig {
         &mut self,
         source: ConfigSource,
         path: impl AsRef<Path>,
-    ) -> Result<(), ConfigError> {
+    ) -> Result<(), ConfigLoadError> {
         let layers = ConfigLayer::load_from_dir(source, path.as_ref())?;
         self.extend_layers(layers);
         Ok(())