From 86cbfac37e1d3ee8d1f2115526ae43b4ec50fa18 Mon Sep 17 00:00:00 2001 From: Christoph Koehler Date: Wed, 17 Apr 2024 16:31:39 -0600 Subject: [PATCH 1/4] config: switch from dirs to etcetera Add a cfg to make etctera use the correct config directory on MacOS (which etcetera calls the `data_dir`). --- Cargo.lock | 62 +++++++++------------------------------------ Cargo.toml | 2 +- cli/Cargo.toml | 2 +- cli/src/config.rs | 15 +++++++++-- cli/src/git_util.rs | 2 +- 5 files changed, 28 insertions(+), 55 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index ca51a1b2ae..7d0451ba27 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 a57c784225..d27b277dfd 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -43,8 +43,8 @@ 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.11.0" +etcetera = "0.8" esl01-renderdag = "0.3.0" futures = "0.3.30" git2 = "0.18.3" diff --git a/cli/Cargo.toml b/cli/Cargo.toml index 8d2b41c514..5038de9b8b 100644 --- a/cli/Cargo.toml +++ b/cli/Cargo.toml @@ -55,8 +55,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/config.rs b/cli/src/config.rs index 453d7dabed..2dba485787 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; @@ -250,9 +251,19 @@ struct ConfigEnv { impl ConfigEnv { fn new() -> Self { + // get the current platform's config dir. On MacOS that's actually the data_dir + #[cfg(not(target_os = "macos"))] + let platform_config_dir = etcetera::choose_base_strategy() + .ok() + .map(|c| c.config_dir()); + #[cfg(target_os = "macos")] + let platform_config_dir = etcetera::base_strategy::Apple::new() + .ok() + .map(|c| c.data_dir()); + ConfigEnv { - config_dir: dirs::config_dir(), - home_dir: dirs::home_dir(), + config_dir: platform_config_dir, + home_dir: etcetera::home_dir().ok(), jj_config: env::var("JJ_CONFIG").ok(), } } diff --git a/cli/src/git_util.rs b/cli/src/git_util.rs index 05f4ba6192..df54804c30 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); From ca1ab1594fbabed9dda7e6dd542d2f3a5bd8fc29 Mon Sep 17 00:00:00 2001 From: Christoph Koehler Date: Wed, 17 Apr 2024 18:51:35 -0600 Subject: [PATCH 2/4] config: refactor tests to reflect our desired behavior Rename the test enum and `new_config_path` function to reflect that we never have just "new": we either look for existing or create new, or just look for existing. --- cli/src/cli_util.rs | 7 ++-- cli/src/config.rs | 86 +++++++++++++++++++++++++-------------------- 2 files changed, 50 insertions(+), 43 deletions(-) diff --git a/cli/src/cli_util.rs b/cli/src/cli_util.rs index 9e7001f2a5..e685c99401 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 2dba485787..cd4eaa071b 100644 --- a/cli/src/config.rs +++ b/cli/src/config.rs @@ -302,7 +302,7 @@ impl ConfigEnv { } } - fn new_config_path(self) -> Result, ConfigError> { + fn new_or_existing_config_path(self) -> Result, ConfigError> { match self.config_path()? { ConfigPath::Existing(path) => Ok(Some(path)), ConfigPath::New(path) => { @@ -322,8 +322,8 @@ pub fn existing_config_path() -> Result, ConfigError> { /// 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() +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 @@ -765,52 +765,54 @@ mod tests { home_dir: Some("home".into()), ..Default::default() }, - want: Want::ExistingAndNew("home/.jjconfig.toml"), + want: Want::Existing("home/.jjconfig.toml"), } .run() } #[test] - fn test_config_path_home_dir_new() -> anyhow::Result<()> { + fn test_config_path_home_dir_doesnt_create_new() -> anyhow::Result<()> { TestCase { files: vec![], cfg: ConfigEnv { home_dir: Some("home".into()), ..Default::default() }, - want: Want::New("home/.jjconfig.toml"), + want: Want::None, } .run() } #[test] - fn test_config_path_config_dir_existing() -> anyhow::Result<()> { + fn test_config_path_config_dir_new() -> anyhow::Result<()> { TestCase { - files: vec!["config/jj/config.toml"], + files: vec![], cfg: ConfigEnv { config_dir: Some("config".into()), + home_dir: Some("home".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<()> { + fn test_config_path_config_dir_existing() -> anyhow::Result<()> { TestCase { - files: vec![], + files: vec!["config/jj/config.toml"], cfg: ConfigEnv { config_dir: Some("config".into()), ..Default::default() }, - want: Want::New("config/jj/config.toml"), + want: Want::Existing("config/jj/config.toml"), } .run() } #[test] - fn test_config_path_new_prefer_config_dir() -> anyhow::Result<()> { + // if no config exists, make one in the config dir location + fn test_config_path_new_default_to_config_dir() -> anyhow::Result<()> { TestCase { files: vec![], cfg: ConfigEnv { @@ -818,7 +820,7 @@ mod tests { home_dir: Some("home".into()), ..Default::default() }, - want: Want::New("config/jj/config.toml"), + want: Want::ExistingOrNew("config/jj/config.toml"), } .run() } @@ -831,20 +833,20 @@ mod tests { jj_config: Some("custom.toml".into()), ..Default::default() }, - want: Want::ExistingAndNew("custom.toml"), + want: Want::Existing("custom.toml"), } .run() } #[test] - fn test_config_path_jj_config_new() -> anyhow::Result<()> { + fn test_config_path_jj_config_doesnt_create_new() -> anyhow::Result<()> { TestCase { files: vec![], cfg: ConfigEnv { jj_config: Some("custom.toml".into()), ..Default::default() }, - want: Want::New("custom.toml"), + want: Want::None, } .run() } @@ -858,7 +860,7 @@ mod tests { config_dir: Some("config".into()), ..Default::default() }, - want: Want::ExistingAndNew("config/jj/config.toml"), + want: Want::Existing("config/jj/config.toml"), } .run() } @@ -872,7 +874,7 @@ mod tests { config_dir: Some("config".into()), ..Default::default() }, - want: Want::ExistingAndNew("home/.jjconfig.toml"), + want: Want::Existing("home/.jjconfig.toml"), } .run() } @@ -901,7 +903,7 @@ mod tests { Err(ConfigError::AmbiguousSource(_, _)) ); assert_matches!( - cfg.clone().new_config_path(), + cfg.clone().new_or_existing_config_path(), Err(ConfigError::AmbiguousSource(_, _)) ); Ok(()) @@ -921,8 +923,8 @@ mod tests { enum Want { None, - New(&'static str), - ExistingAndNew(&'static str), + Existing(&'static str), + ExistingOrNew(&'static str), } struct TestCase { @@ -958,25 +960,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(()) } } From 8107d8c5298bb1d456801030088440635acf020f Mon Sep 17 00:00:00 2001 From: Christoph Koehler Date: Wed, 17 Apr 2024 19:28:29 -0600 Subject: [PATCH 3/4] config: refactor config_path() * rename it to make clearer what it does * it returns an existing config or None if there isn't one * if told to create missing, it does so * as a result, `ConfigPath` is no longer necessary and removed --- cli/src/config.rs | 135 +++++++++++++++++++++++++--------------------- 1 file changed, 75 insertions(+), 60 deletions(-) diff --git a/cli/src/config.rs b/cli/src/config.rs index cd4eaa071b..017f64ce9e 100644 --- a/cli/src/config.rs +++ b/cli/src/config.rs @@ -196,26 +196,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<()> { @@ -241,6 +221,15 @@ fn create_config_file(path: &Path) -> std::io::Result { .open(path) } +// Used in the argument for the `new_or_existing_config_path` function. Defines +// whether we create a new config or not. This is more descriptive than a bool +// on the function. +#[derive(PartialEq)] +enum HandleMissingConfig { + Ignore, + Create, +} + // The struct exists so that we can mock certain global values in unit tests. #[derive(Clone, Default, Debug)] struct ConfigEnv { @@ -268,54 +257,75 @@ impl ConfigEnv { } } - fn config_path(self) -> Result { + // Returns a config path, potentially creating a new one if told. Can return + // None if no config exists and we're not creating a new one. + // Also bubbles up errors if encountered. + fn new_or_existing_config_path( + self, + handle_missing: HandleMissingConfig, + ) -> Result, ConfigError> { 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)))); + let path = PathBuf::from(path); + if handle_missing == HandleMissingConfig::Create && !path.exists() { + create_config_file(&path)?; + } + + return Ok(Some(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| { + // look for existing configs first + // cloning `self.config_dir` because we need it again below + let platform_config = self.config_dir.clone().map(|mut config_dir| { 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), - } - } - - fn existing_config_path(self) -> Result, ConfigError> { - match self.config_path()? { - ConfigPath::Existing(path) => Ok(Some(path)), - _ => Ok(None), - } - } + }); + let home_config = self.home_dir.map(|mut config_dir| { + config_dir.push(".jjconfig.toml"); + config_dir + }); + let paths: Vec = vec![&platform_config, &home_config] + .into_iter() + .unique() + .flatten() // pops all T out of Some + .filter(|config| config.exists()) + .map(|c| c.to_owned()) + .collect(); + + let existing_configs = match paths.len() { + 0 => Ok(None), + 1 => Ok(Some(paths[0].to_path_buf())), + 2 => Err(ConfigError::AmbiguousSource( + paths[0].clone(), + paths[1].clone(), + )), + _ => unreachable!(), + }; - fn new_or_existing_config_path(self) -> Result, ConfigError> { - match self.config_path()? { - ConfigPath::Existing(path) => Ok(Some(path)), - ConfigPath::New(path) => { + // then look for existing configs and return if found + if let Some(config) = existing_configs? { + Ok(Some(config)) + // otherwise, create one and then return it + } else if handle_missing == HandleMissingConfig::Create { + // we use the platform_config by default + if let Some(mut path) = self.config_dir { + path.push("jj"); + path.push("config.toml"); create_config_file(&path)?; Ok(Some(path)) + } else { + Ok(None) } - ConfigPath::Unavailable => Ok(None), + // but if we haven't found a config and weren't told to create one, + // return None + } else { + Ok(None) } } } pub fn existing_config_path() -> Result, ConfigError> { - ConfigEnv::new().existing_config_path() + ConfigEnv::new().new_or_existing_config_path(HandleMissingConfig::Ignore) } /// Returns a path to the user-specific config file. If no config file is found, @@ -323,7 +333,7 @@ pub fn existing_config_path() -> Result, ConfigError> { /// file is returned, the parent directory may be created as a result of this /// call. pub fn new_or_existing_config_path() -> Result, ConfigError> { - ConfigEnv::new().new_or_existing_config_path() + ConfigEnv::new().new_or_existing_config_path(HandleMissingConfig::Create) } /// Environment variables that should be overridden by config values @@ -899,11 +909,13 @@ mod tests { }; use assert_matches::assert_matches; assert_matches!( - cfg.clone().existing_config_path(), + cfg.clone() + .new_or_existing_config_path(HandleMissingConfig::Ignore), Err(ConfigError::AmbiguousSource(_, _)) ); assert_matches!( - cfg.clone().new_or_existing_config_path(), + cfg.clone() + .new_or_existing_config_path(HandleMissingConfig::Create), Err(ConfigError::AmbiguousSource(_, _)) ); Ok(()) @@ -950,8 +962,11 @@ mod tests { use anyhow::anyhow; let tmp = setup_config_fs(&self.files)?; - let check = |name, f: fn(ConfigEnv) -> Result<_, _>, want: Option<_>| { - let got = f(self.config(tmp.path())).map_err(|e| anyhow!("{name}: {e}"))?; + let check = |name, handle_missing: HandleMissingConfig, want: Option<_>| { + let got = self + .config(tmp.path()) + .new_or_existing_config_path(handle_missing) + .map_err(|e| anyhow!("{name}: {e}"))?; let want = want.map(|p| tmp.path().join(p)); if got != want { Err(anyhow!("{name}: got {got:?}, want {want:?}")) @@ -965,14 +980,14 @@ mod tests { Want::Existing(want) => { check( "existing_config_path", - ConfigEnv::existing_config_path, + HandleMissingConfig::Ignore, Some(want), )?; } Want::ExistingOrNew(want) => { let got = check( "new_or_existing_config_path", - ConfigEnv::new_or_existing_config_path, + HandleMissingConfig::Create, Some(want), )?; if let Some(path) = got { From 6e79eb8434b938bf9e8e973a32a6a8a5862805d5 Mon Sep 17 00:00:00 2001 From: Christoph Koehler Date: Wed, 17 Apr 2024 19:28:29 -0600 Subject: [PATCH 4/4] config: add XDG_CONFIG_HOME config location for MacOS --- CHANGELOG.md | 3 ++ cli/src/config.rs | 91 +++++++++++++++++++++++++++++++++++++++-------- docs/config.md | 25 ++++++------- 3 files changed, 92 insertions(+), 27 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f1a673d3dc..f010699ddb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -48,6 +48,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 * You can check whether Watchman fsmonitor is enabled or installed with the new `jj debug watchman status` command. +* On MacOS, jj will now look for its config in `$XDG_CONFIG_HOME` in addition + to `~/Library/Application Support/jj/` + ### Fixed bugs * Revsets now support `\`-escapes in string literal. diff --git a/cli/src/config.rs b/cli/src/config.rs index 017f64ce9e..e4252d84b2 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), } @@ -233,13 +235,19 @@ enum HandleMissingConfig { // 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 { + // get XDG_CONFIG_HOME + let xdg_config_dir = etcetera::base_strategy::Xdg::new() + .ok() + .map(|c| c.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 = etcetera::choose_base_strategy() @@ -251,7 +259,8 @@ impl ConfigEnv { .map(|c| c.data_dir()); 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(), } @@ -274,8 +283,13 @@ impl ConfigEnv { return Ok(Some(path)); } // look for existing configs first - // cloning `self.config_dir` because we need it again below - let platform_config = self.config_dir.clone().map(|mut config_dir| { + // cloning `self.platform_config_dir` because we need it again below + let platform_config = self.platform_config_dir.clone().map(|mut config_dir| { + config_dir.push("jj"); + config_dir.push("config.toml"); + config_dir + }); + let xdg_config = self.xdg_config_dir.map(|mut config_dir| { config_dir.push("jj"); config_dir.push("config.toml"); config_dir @@ -284,7 +298,7 @@ impl ConfigEnv { config_dir.push(".jjconfig.toml"); config_dir }); - let paths: Vec = vec![&platform_config, &home_config] + let paths: Vec = vec![&platform_config, &xdg_config, &home_config] .into_iter() .unique() .flatten() // pops all T out of Some @@ -299,6 +313,11 @@ impl ConfigEnv { paths[0].clone(), paths[1].clone(), )), + 3 => Err(ConfigError::AmbiguousSource3( + paths[0].clone(), + paths[1].clone(), + paths[2].clone(), + )), _ => unreachable!(), }; @@ -308,7 +327,7 @@ impl ConfigEnv { // otherwise, create one and then return it } else if handle_missing == HandleMissingConfig::Create { // we use the platform_config by default - if let Some(mut path) = self.config_dir { + if let Some(mut path) = self.platform_config_dir { path.push("jj"); path.push("config.toml"); create_config_file(&path)?; @@ -798,7 +817,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() }, @@ -812,7 +831,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"), @@ -821,12 +840,12 @@ mod tests { } #[test] - // if no config exists, make one in the config dir location - fn test_config_path_new_default_to_config_dir() -> anyhow::Result<()> { + // if no config exists, make one in the platform_config dir location + fn test_config_path_new_default_to_platform_config_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() }, @@ -834,6 +853,20 @@ 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<()> { @@ -867,7 +900,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"), @@ -881,7 +914,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"), @@ -904,7 +937,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; @@ -921,6 +954,33 @@ 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() + .new_or_existing_config_path(HandleMissingConfig::Ignore), + Err(ConfigError::AmbiguousSource3(_, _, _)) + ); + assert_matches!( + cfg.clone() + .new_or_existing_config_path(HandleMissingConfig::Create), + Err(ConfigError::AmbiguousSource3(_, _, _)) + ); + Ok(()) + } + fn setup_config_fs(files: &Vec<&'static str>) -> anyhow::Result { let tmp = testutils::new_temp_dir(); for file in files { @@ -948,7 +1008,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 diff --git a/docs/config.md b/docs/config.md index 7b9597b499..d355443807 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