From 6d2cc82bef3ba2ead7f1ada303503479fe1d3926 Mon Sep 17 00:00:00 2001 From: Christoph Koehler Date: Wed, 17 Apr 2024 19:28:29 -0600 Subject: [PATCH] config: refactor config_path() to be clearer --- cli/src/cli_util.rs | 7 +- cli/src/config.rs | 197 ++++++++++++++++++++++++++++++-------------- 2 files changed, 138 insertions(+), 66 deletions(-) diff --git a/cli/src/cli_util.rs b/cli/src/cli_util.rs index 9e7001f2a51..e685c994010 100644 --- a/cli/src/cli_util.rs +++ b/cli/src/cli_util.rs @@ -83,7 +83,7 @@ use crate::command_error::{ }; use crate::commit_templater::{CommitTemplateLanguage, CommitTemplateLanguageExtension}; use crate::config::{ - new_config_path, AnnotatedValue, CommandNameAndArgs, ConfigSource, LayeredConfigs, + new_or_existing_config_path, AnnotatedValue, CommandNameAndArgs, ConfigSource, LayeredConfigs, }; use crate::formatter::{FormatRecorder, Formatter, PlainTextFormatter}; use crate::git_util::{ @@ -1984,9 +1984,8 @@ pub fn get_new_config_file_path( ) -> Result { let edit_path = match config_source { // TODO(#531): Special-case for editors that can't handle viewing directories? - ConfigSource::User => { - new_config_path()?.ok_or_else(|| user_error("No repo config path found to edit"))? - } + ConfigSource::User => new_or_existing_config_path()? + .ok_or_else(|| user_error("No repo config path found to edit"))?, ConfigSource::Repo => command.workspace_loader()?.repo_path().join("config.toml"), _ => { return Err(user_error(format!( diff --git a/cli/src/config.rs b/cli/src/config.rs index f934405d6cc..5e50172cceb 100644 --- a/cli/src/config.rs +++ b/cli/src/config.rs @@ -31,6 +31,8 @@ pub enum ConfigError { ConfigReadError(#[from] config::ConfigError), #[error("Both {0} and {1} exist. Please consolidate your configs in one of them.")] AmbiguousSource(PathBuf, PathBuf), + #[error("Configs found in {0}, {1}, and {2}. Please consolidate your configs in one of them.")] + AmbiguousSource3(PathBuf, PathBuf, PathBuf), #[error(transparent)] ConfigCreateError(#[from] std::io::Error), #[error(transparent)] @@ -198,26 +200,6 @@ impl LayeredConfigs { } } -enum ConfigPath { - /// Existing config file path. - Existing(PathBuf), - /// Could not find any config file, but a new file can be created at the - /// specified location. - New(PathBuf), - /// Could not find any config file. - Unavailable, -} - -impl ConfigPath { - fn new(path: Option) -> Self { - match path { - Some(path) if path.exists() => ConfigPath::Existing(path), - Some(path) => ConfigPath::New(path), - None => ConfigPath::Unavailable, - } - } -} - /// Like std::fs::create_dir_all but creates new directories to be accessible to /// the user only on Unix (chmod 700). fn create_dir_all(path: &Path) -> std::io::Result<()> { @@ -246,13 +228,21 @@ fn create_config_file(path: &Path) -> std::io::Result { // The struct exists so that we can mock certain global values in unit tests. #[derive(Clone, Default, Debug)] struct ConfigEnv { - config_dir: Option, + xdg_config_dir: Option, + platform_config_dir: Option, home_dir: Option, jj_config: Option, } impl ConfigEnv { fn new() -> Result { + // get XDG_CONFIG_HOME + let xdg_config_dir = Some( + etcetera::base_strategy::Xdg::new() + .map_err(ConfigError::HomeDirError)? + .config_dir(), + ); + // get the current platform's config dir. On MacOS that's actually the data_dir #[cfg(not(target_os = "macos"))] let platform_config_dir = Some(etcetera::choose_base_strategy()?.config_dir()); @@ -260,67 +250,109 @@ impl ConfigEnv { let platform_config_dir = Some(etcetera::base_strategy::Apple::new()?.data_dir()); Ok(ConfigEnv { - config_dir: platform_config_dir, + xdg_config_dir, + platform_config_dir, home_dir: etcetera::home_dir().ok(), jj_config: env::var("JJ_CONFIG").ok(), }) } - fn config_path(self) -> Result { - if let Some(path) = self.jj_config { - // TODO: We should probably support colon-separated (std::env::split_paths) - return Ok(ConfigPath::new(Some(PathBuf::from(path)))); - } - // TODO: Should we drop the final `/config.toml` and read all files in the - // directory? - let platform_config_path = ConfigPath::new(self.config_dir.map(|mut config_dir| { + /// looks for all three possible config locations and returns the one that + /// exists; or None if there are none; or an Error if there exist more + /// than one, or something else went wrong. + fn actual_configs(&self) -> Result, ConfigError> { + let platform_config = self.platform_config_dir.as_ref().map(|config_dir| { + let mut config_dir = config_dir.clone(); + config_dir.push("jj"); + config_dir.push("config.toml"); + config_dir + }); + let xdg_config = self.xdg_config_dir.as_ref().map(|config_dir| { + let mut config_dir = config_dir.clone(); config_dir.push("jj"); config_dir.push("config.toml"); config_dir - })); - let home_config_path = ConfigPath::new(self.home_dir.map(|mut home_dir| { - home_dir.push(".jjconfig.toml"); - home_dir - })); - use ConfigPath::*; - match (platform_config_path, home_config_path) { - (Existing(platform_config_path), Existing(home_config_path)) => Err( - ConfigError::AmbiguousSource(platform_config_path, home_config_path), - ), - (Existing(path), _) | (_, Existing(path)) => Ok(Existing(path)), - (New(path), _) | (_, New(path)) => Ok(New(path)), - (Unavailable, Unavailable) => Ok(Unavailable), + }); + let home_config = self.home_dir.as_ref().map(|config_dir| { + let mut config_dir = config_dir.clone(); + config_dir.push(".jjconfig.toml"); + config_dir + }); + let paths: Vec = vec![&platform_config, &xdg_config, &home_config] + .into_iter() + .unique() + .flatten() // pops all T out of Some + .filter(|config| config.exists()) + .map(|c| c.to_owned()) + .collect(); + + match paths.len() { + 0 => Ok(None), + 1 => Ok(Some(paths[0].to_path_buf())), + 2 => Err(ConfigError::AmbiguousSource( + paths[0].clone(), + paths[1].clone(), + )), + 3 => Err(ConfigError::AmbiguousSource3( + paths[0].clone(), + paths[1].clone(), + paths[2].clone(), + )), + _ => unreachable!(), } } + /// Just a wrapper around `actual_configs`, but it also checks for the + /// config env var and prioritizes that if set. fn existing_config_path(self) -> Result, ConfigError> { - match self.config_path()? { - ConfigPath::Existing(path) => Ok(Some(path)), - _ => Ok(None), + if let Some(path) = self.jj_config { + return Ok(Some(PathBuf::from(path))); } + // look for existing configs first + self.actual_configs() } + /// Similar to `existing_configs`, but creates the config if it doesn't + /// exist (where the env var points, if set), or the platform_config + /// location if not. For > 1 existing configs, it behaves like + /// `existing_configs`. fn new_or_existing_config_path(self) -> Result, ConfigError> { - match self.config_path()? { - ConfigPath::Existing(path) => Ok(Some(path)), - ConfigPath::New(path) => { + // if the env var is set, use its path and create it if it doesn't exist. + if let Some(path) = self.jj_config { + // TODO: We should probably support colon-separated (std::env::split_paths) + let path = PathBuf::from(path); + if !path.exists() { + create_config_file(&path)?; + } + + return Ok(Some(path)); + } + // then look for existing configs and return if found + let existing_configs = self.actual_configs()?; + if let Some(config) = existing_configs { + Ok(Some(config)) + // otherwise, create one and then return it + } else { + // we use the platform_config by default + if let Some(mut path) = self.platform_config_dir { + path.push("jj"); + path.push("config.toml"); create_config_file(&path)?; Ok(Some(path)) + } else { + Ok(None) } - ConfigPath::Unavailable => Ok(None), } } } +/// public wrapper around `existing_config_path`, see its docs. pub fn existing_config_path() -> Result, ConfigError> { ConfigEnv::new()?.existing_config_path() } -/// Returns a path to the user-specific config file. If no config file is found, -/// tries to guess a reasonable new location for it. If a path to a new config -/// file is returned, the parent directory may be created as a result of this -/// call. -pub fn new_config_path() -> Result, ConfigError> { +/// public wrapper around `new_or_existing_config_path`, see its docs. +pub fn new_or_existing_config_path() -> Result, ConfigError> { ConfigEnv::new()?.new_or_existing_config_path() } @@ -786,7 +818,7 @@ mod tests { TestCase { files: vec![], cfg: ConfigEnv { - config_dir: Some("config".into()), + platform_config_dir: Some("config".into()), home_dir: Some("home".into()), ..Default::default() }, @@ -800,7 +832,7 @@ mod tests { TestCase { files: vec!["config/jj/config.toml"], cfg: ConfigEnv { - config_dir: Some("config".into()), + platform_config_dir: Some("config".into()), ..Default::default() }, want: Want::Existing("config/jj/config.toml"), @@ -814,7 +846,7 @@ mod tests { TestCase { files: vec![], cfg: ConfigEnv { - config_dir: Some("config".into()), + platform_config_dir: Some("config".into()), home_dir: Some("home".into()), ..Default::default() }, @@ -823,6 +855,21 @@ mod tests { .run() } + #[test] + fn test_config_path_platform_equals_xdg_is_ok() -> anyhow::Result<()> { + TestCase { + files: vec!["config/jj/config.toml"], + cfg: ConfigEnv { + platform_config_dir: Some("config".into()), + xdg_config_dir: Some("config".into()), + home_dir: Some("home".into()), + ..Default::default() + }, + want: Want::Existing("config/jj/config.toml"), + } + .run() + } + #[test] fn test_config_path_jj_config_existing() -> anyhow::Result<()> { TestCase { @@ -855,7 +902,7 @@ mod tests { files: vec!["config/jj/config.toml"], cfg: ConfigEnv { home_dir: Some("home".into()), - config_dir: Some("config".into()), + platform_config_dir: Some("config".into()), ..Default::default() }, want: Want::Existing("config/jj/config.toml"), @@ -869,7 +916,7 @@ mod tests { files: vec!["home/.jjconfig.toml"], cfg: ConfigEnv { home_dir: Some("home".into()), - config_dir: Some("config".into()), + platform_config_dir: Some("config".into()), ..Default::default() }, want: Want::Existing("home/.jjconfig.toml"), @@ -892,7 +939,7 @@ mod tests { let tmp = setup_config_fs(&vec!["home/.jjconfig.toml", "config/jj/config.toml"])?; let cfg = ConfigEnv { home_dir: Some(tmp.path().join("home")), - config_dir: Some(tmp.path().join("config")), + platform_config_dir: Some(tmp.path().join("config")), ..Default::default() }; use assert_matches::assert_matches; @@ -907,6 +954,31 @@ mod tests { Ok(()) } + #[test] + fn test_config_path_ambiguous3() -> anyhow::Result<()> { + let tmp = setup_config_fs(&vec![ + "home/.jjconfig.toml", + "config/jj/config.toml", + "bar/jj/config.toml", + ])?; + let cfg = ConfigEnv { + home_dir: Some(tmp.path().join("home")), + platform_config_dir: Some(tmp.path().join("config")), + xdg_config_dir: Some(tmp.path().join("bar")), + ..Default::default() + }; + use assert_matches::assert_matches; + assert_matches!( + cfg.clone().existing_config_path(), + Err(ConfigError::AmbiguousSource3(_, _, _)) + ); + assert_matches!( + cfg.clone().new_or_existing_config_path(), + Err(ConfigError::AmbiguousSource3(_, _, _)) + ); + Ok(()) + } + fn setup_config_fs(files: &Vec<&'static str>) -> anyhow::Result { let tmp = testutils::new_temp_dir(); for file in files { @@ -934,7 +1006,8 @@ mod tests { impl TestCase { fn config(&self, root: &Path) -> ConfigEnv { ConfigEnv { - config_dir: self.cfg.config_dir.as_ref().map(|p| root.join(p)), + xdg_config_dir: self.cfg.xdg_config_dir.as_ref().map(|p| root.join(p)), + platform_config_dir: self.cfg.platform_config_dir.as_ref().map(|p| root.join(p)), home_dir: self.cfg.home_dir.as_ref().map(|p| root.join(p)), jj_config: self .cfg