From 436d224ffb91c204c693e9ace87411452ad02e2e Mon Sep 17 00:00:00 2001 From: Christoph Koehler Date: Sat, 6 Apr 2024 18:41:44 -0600 Subject: [PATCH] config: also check XDG_CONFIG_HOME on MacOS This replaces the `dirs` crate with the more lightweight and flexible `etcetera` crate, which also lets us use XDG config on MacOS. Will search in $HOME, the platform-specific dir, and $XDG_CONFIG_HOME (if different from the platform-specific dir, like on MacOS). I changed some tests to more accurately describe the desired behavior and added a couple of new ones. Fixes #3434 --- CHANGELOG.md | 2 + Cargo.lock | 62 ++-------- Cargo.toml | 2 +- cli/Cargo.toml | 2 +- cli/src/cli_util.rs | 7 +- cli/src/config.rs | 290 +++++++++++++++++++++++++++----------------- cli/src/git_util.rs | 2 +- docs/config.md | 25 ++-- 8 files changed, 210 insertions(+), 182 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ccef85851a4..1adb0a943d6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,6 +15,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 * The default template alias `builtin_log_root(change_id: ChangeId, commit_id: CommitId)` was replaced by `format_root_commit(root: Commit)`. +* On MacOS, jj will now also look for its config in `$XDG_CONFIG_HOME` in addition to `~/Library/Application Support/jj/` and `$HOME/.jjconfig.toml` + ### New features * The list of conflicted paths is printed whenever the working copy changes. diff --git a/Cargo.lock b/Cargo.lock index d8c52486174..a8db2ddfb75 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -672,27 +672,6 @@ dependencies = [ "subtle", ] -[[package]] -name = "dirs" -version = "5.0.1" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "44c45a9d03d6676652bcb5e724c7e988de1acad23a711b5217ab9cbecbec2225" -dependencies = [ - "dirs-sys", -] - -[[package]] -name = "dirs-sys" -version = "0.4.1" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "520f05a5cbd335fae5a99ff7a6ab8627577660ee5cfd6a94a6a929b52ff0321c" -dependencies = [ - "libc", - "option-ext", - "redox_users", - "windows-sys 0.48.0", -] - [[package]] name = "doc-comment" version = "0.3.3" @@ -743,6 +722,17 @@ dependencies = [ "itertools 0.10.5", ] +[[package]] +name = "etcetera" +version = "0.8.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "136d1b5283a1ab77bd9257427ffd09d8667ced0570b6f938942bc7568ed5b943" +dependencies = [ + "cfg-if", + "home", + "windows-sys 0.48.0", +] + [[package]] name = "faster-hex" version = "0.9.0" @@ -1680,8 +1670,8 @@ dependencies = [ "config", "criterion", "crossterm", - "dirs", "esl01-renderdag", + "etcetera", "futures 0.3.30", "git2", "gix", @@ -1837,17 +1827,6 @@ dependencies = [ "pkg-config", ] -[[package]] -name = "libredox" -version = "0.0.1" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "85c833ca1e66078851dba29046874e38f08b2c883700aa29a03ddd3b23814ee8" -dependencies = [ - "bitflags 2.5.0", - "libc", - "redox_syscall", -] - [[package]] name = "libssh2-sys" version = "0.3.0" @@ -2086,12 +2065,6 @@ dependencies = [ "vcpkg", ] -[[package]] -name = "option-ext" -version = "0.2.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "04744f49eae99ab78e0d5c0b603ab218f515ea8cfe5a456d7629ad883a3b6e7d" - [[package]] name = "overload" version = "0.1.1" @@ -2458,17 +2431,6 @@ dependencies = [ "bitflags 1.3.2", ] -[[package]] -name = "redox_users" -version = "0.4.4" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "a18479200779601e498ada4e8c1e1f50e3ee19deb0259c25825a98b5603b2cb4" -dependencies = [ - "getrandom", - "libredox", - "thiserror", -] - [[package]] name = "ref-cast" version = "1.0.22" diff --git a/Cargo.toml b/Cargo.toml index 6dd4413de7b..ac7424dc325 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -43,9 +43,9 @@ config = { version = "0.13.4", default-features = false, features = ["toml"] } criterion = "0.5.1" crossterm = { version = "0.27", default-features = false } digest = "0.10.7" -dirs = "5.0.1" either = "1.10.0" esl01-renderdag = "0.3.0" +etcetera = "0.8.0" futures = "0.3.30" git2 = "0.18.3" gix = { version = "0.61.0", default-features = false, features = [ diff --git a/cli/Cargo.toml b/cli/Cargo.toml index 1444902a565..640931428f8 100644 --- a/cli/Cargo.toml +++ b/cli/Cargo.toml @@ -43,8 +43,8 @@ clap_mangen = { workspace = true } config = { workspace = true } criterion = { workspace = true, optional = true } crossterm = { workspace = true } -dirs = { workspace = true } esl01-renderdag = { workspace = true } +etcetera = { workspace = true } futures = { workspace = true } git2 = { workspace = true } gix = { workspace = true } 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 453d7dabedf..043fe6579d5 100644 --- a/cli/src/config.rs +++ b/cli/src/config.rs @@ -19,6 +19,7 @@ use std::process::Command; use std::{env, fmt}; use config::Source; +use etcetera::BaseStrategy; use itertools::Itertools; use jj_lib::settings::ConfigResultExt as _; use thiserror::Error; @@ -30,8 +31,12 @@ 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)] + HomeDirError(#[from] etcetera::HomeDirError), } #[derive(Clone, Debug, PartialEq, Eq)] @@ -195,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<()> { @@ -243,76 +228,133 @@ 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() -> Self { - ConfigEnv { - config_dir: dirs::config_dir(), - home_dir: dirs::home_dir(), + 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()); + #[cfg(target_os = "macos")] + let platform_config_dir = Some(etcetera::base_strategy::Apple::new()?.data_dir()); + + Ok(ConfigEnv { + 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 { + // TODO: We should probably support colon-separated (std::env::split_paths) + return Ok(Some(PathBuf::from(path))); } + // look for existing configs first + self.actual_configs() } - fn new_config_path(self) -> Result, ConfigError> { - match self.config_path()? { - ConfigPath::Existing(path) => Ok(Some(path)), - ConfigPath::New(path) => { + /// 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> { + // 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() + 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> { - ConfigEnv::new().new_config_path() +/// 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() } /// Environment variables that should be overridden by config values @@ -754,20 +796,7 @@ mod tests { home_dir: Some("home".into()), ..Default::default() }, - want: Want::ExistingAndNew("home/.jjconfig.toml"), - } - .run() - } - - #[test] - fn test_config_path_home_dir_new() -> anyhow::Result<()> { - TestCase { - files: vec![], - cfg: ConfigEnv { - home_dir: Some("home".into()), - ..Default::default() - }, - want: Want::New("home/.jjconfig.toml"), + want: Want::ExistingOrNew("home/.jjconfig.toml"), } .run() } @@ -777,37 +806,40 @@ 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::ExistingAndNew("config/jj/config.toml"), + want: Want::ExistingOrNew("config/jj/config.toml"), } .run() } #[test] - fn test_config_path_config_dir_new() -> anyhow::Result<()> { + // if no config exists, make one in the platform dir location + fn test_config_path_new_default_to_platform_dir() -> anyhow::Result<()> { TestCase { files: vec![], cfg: ConfigEnv { - config_dir: Some("config".into()), + platform_config_dir: Some("config".into()), + home_dir: Some("home".into()), ..Default::default() }, - want: Want::New("config/jj/config.toml"), + want: Want::ExistingOrNew("config/jj/config.toml"), } .run() } #[test] - fn test_config_path_new_prefer_config_dir() -> anyhow::Result<()> { + fn test_config_path_platform_equals_xdg_is_ok() -> anyhow::Result<()> { TestCase { - files: vec![], + files: vec!["config/jj/config.toml"], cfg: ConfigEnv { - config_dir: Some("config".into()), + platform_config_dir: Some("config".into()), + xdg_config_dir: Some("config".into()), home_dir: Some("home".into()), ..Default::default() }, - want: Want::New("config/jj/config.toml"), + want: Want::Existing("config/jj/config.toml"), } .run() } @@ -820,7 +852,7 @@ mod tests { jj_config: Some("custom.toml".into()), ..Default::default() }, - want: Want::ExistingAndNew("custom.toml"), + want: Want::ExistingOrNew("custom.toml"), } .run() } @@ -833,7 +865,7 @@ mod tests { jj_config: Some("custom.toml".into()), ..Default::default() }, - want: Want::New("custom.toml"), + want: Want::Existing("custom.toml"), } .run() } @@ -844,10 +876,10 @@ 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::ExistingAndNew("config/jj/config.toml"), + want: Want::ExistingOrNew("config/jj/config.toml"), } .run() } @@ -858,10 +890,10 @@ 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::ExistingAndNew("home/.jjconfig.toml"), + want: Want::ExistingOrNew("home/.jjconfig.toml"), } .run() } @@ -881,7 +913,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; @@ -890,12 +922,37 @@ mod tests { Err(ConfigError::AmbiguousSource(_, _)) ); assert_matches!( - cfg.clone().new_config_path(), + cfg.clone().new_or_existing_config_path(), Err(ConfigError::AmbiguousSource(_, _)) ); 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 { @@ -910,8 +967,8 @@ mod tests { enum Want { None, - New(&'static str), - ExistingAndNew(&'static str), + Existing(&'static str), + ExistingOrNew(&'static str), } struct TestCase { @@ -923,7 +980,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 @@ -947,25 +1005,31 @@ mod tests { } }; - let (want_existing, want_new) = match self.want { - Want::None => (None, None), - Want::New(want) => (None, Some(want)), - Want::ExistingAndNew(want) => (Some(want), Some(want)), + match self.want { + Want::None => {} + Want::Existing(want) => { + check( + "existing_config_path", + ConfigEnv::existing_config_path, + Some(want), + )?; + } + Want::ExistingOrNew(want) => { + let got = check( + "new_or_existing_config_path", + ConfigEnv::new_or_existing_config_path, + Some(want), + )?; + if let Some(path) = got { + if !Path::new(&path).is_file() { + return Err(anyhow!( + "new_config_path returned {path:?} which is not a file" + )); + } + } + } }; - check( - "existing_config_path", - ConfigEnv::existing_config_path, - want_existing, - )?; - let got = check("new_config_path", ConfigEnv::new_config_path, want_new)?; - if let Some(path) = got { - if !Path::new(&path).is_file() { - return Err(anyhow!( - "new_config_path returned {path:?} which is not a file" - )); - } - } Ok(()) } } diff --git a/cli/src/git_util.rs b/cli/src/git_util.rs index 05f4ba61924..df54804c307 100644 --- a/cli/src/git_util.rs +++ b/cli/src/git_util.rs @@ -121,7 +121,7 @@ fn pinentry_get_pw(url: &str) -> Option { #[tracing::instrument] fn get_ssh_keys(_username: &str) -> Vec { let mut paths = vec![]; - if let Some(home_dir) = dirs::home_dir() { + if let Ok(home_dir) = etcetera::home_dir() { let ssh_dir = Path::new(&home_dir).join(".ssh"); for filename in ["id_ed25519_sk", "id_ed25519", "id_rsa"] { let key_path = ssh_dir.join(filename); diff --git a/docs/config.md b/docs/config.md index 7b9597b4991..d355443807c 100644 --- a/docs/config.md +++ b/docs/config.md @@ -734,7 +734,7 @@ using `jj debug watchman status`. ### User config file -An easy way to find the user config file is: +An easy way to find the user config file (if one exists) is: ```bash jj config path --user @@ -742,17 +742,18 @@ jj config path --user The rest of this section covers the details of where this file can be located. -On all platforms, the user's global `jj` configuration file is located at either -`~/.jjconfig.toml` (where `~` represents `$HOME` on Unix-likes, or -`%USERPROFILE%` on Windows) or in a platform-specific directory. The -platform-specific location is recommended for better integration with platform -services. It is an error for both of these files to exist. - -| Platform | Value | Example | -| :------- | :------------------------------------------------- | :-------------------------------------------------------- | -| Linux | `$XDG_CONFIG_HOME/jj/config.toml` | `/home/alice/.config/jj/config.toml` | -| macOS | `$HOME/Library/Application Support/jj/config.toml` | `/Users/Alice/Library/Application Support/jj/config.toml` | -| Windows | `{FOLDERID_RoamingAppData}\jj\config.toml` | `C:\Users\Alice\AppData\Roaming\jj\config.toml` | +On all platforms, the user's global `jj` configuration file is located at +either `~/.jjconfig.toml` (where `~` represents `$HOME` on Unix-likes, or +`%USERPROFILE%` on Windows), in a platform-specific directory, or +`$XDG_CONFIG_HOME` (if different from the platform-specific directory, like on +MacOS). The platform-specific location is recommended for better integration +with platform services. It is an error for more than one of these files to exist. + +| Platform | Value | Example | +| :------- | ---------------------------------------- | --------------------------------------------- | +| Linux | `$XDG_CONFIG_HOME/jj/config.toml` | `/home/alice/.config/jj/config.toml` | +| macOS | `~/Library/Application Support/jj/config.toml` or `$XDG_CONFIG_HOME/jj/config.toml` | `/Users/alice/Library/Application Support/jj/config.toml` or `/Users/Alice/.config/jj/config.toml` | +| Windows | `{FOLDERID_RoamingAppData}\jj\config.toml` | `C:\Users\Alice\AppData\Roaming\jj\config.toml` | The location of the `jj` config file can also be overridden with the `JJ_CONFIG` environment variable. If it is not empty, it should contain the path