config: use XDG_CONFIG_HOME on MacOS by default

This replaces the `dirs` crate with the more lightweight and flexible `etcetera` crate, which also lets us use XDG config on MacOS. Also errors if is finds config in the old place in `Application Support`, so this is a BREAKING change. Fixes #3434
jj-vcs · Apr 12, 2024 · cb93ef5 · cb93ef5
1 parent a786802
commit cb93ef5
Show file tree

Hide file tree

Showing 8 changed files with 108 additions and 87 deletions.
diff --git 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 look for its config in `$XDG_CONFIG_HOME` and error if config is found in `~/Library/Application Support/jj/`
+
 ### New features
 
 * The list of conflicted paths is printed whenever the working copy changes.

diff --git a/Cargo.lock b/Cargo.lock
diff --git 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
@@ -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/home/jj/config.toml b/cli/home/jj/config.toml
diff --git 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)]
@@ -243,44 +248,95 @@ fn create_config_file(path: &Path) -> std::io::Result<std::fs::File> {
 // The struct exists so that we can mock certain global values in unit tests.
 #[derive(Clone, Default, Debug)]
 struct ConfigEnv {
-    config_dir: Option<PathBuf>,
+    xdg_config_dir: Option<PathBuf>,
+    platform_config_dir: Option<PathBuf>,
     home_dir: Option<PathBuf>,
     jj_config: Option<String>,
 }
 
 impl ConfigEnv {
-    fn new() -> Self {
-        ConfigEnv {
-            config_dir: dirs::config_dir(),
-            home_dir: dirs::home_dir(),
+    fn new() -> Result<Self, ConfigError> {
+        // 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<ConfigPath, ConfigError> {
+        // if JJ_CONFIG is defined, use that
         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))));
         }
+
+        dbg!(&self);
+
         // 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| {
+
+        Ok(ConfigPath::new(self.actual_configs()?))
+
+        // 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 actual_configs(self) -> Result<Option<PathBuf>, ConfigError> {
+        let platform_config = self.platform_config_dir.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
-        }));
-        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.map(|mut config_dir| {
+            config_dir.push(".jjconfig.toml");
+            config_dir
+        });
+        let paths: Vec<PathBuf> = vec![&platform_config, &xdg_config, &home_config]
+            .into_iter()
+            .flatten() // pops all T out of Some<T>
+            .filter(|config| config.exists())
+            .map(|c| c.to_owned())
+            .collect();
+
+        match paths.len() {
+            0 => Ok(None),
+            1 => Ok(paths.first().cloned()),
+            2 => Err(ConfigError::AmbiguousSource(
+                paths[0].clone(),
+                paths[1].clone(),
+            )),
+            3 => Err(ConfigError::AmbiguousSource3(
+                paths[0].clone(),
+                paths[1].clone(),
+                paths[2].clone(),
+            )),
+            _ => unreachable!(),
         }
     }
 
@@ -304,15 +360,15 @@ impl ConfigEnv {
 }
 
 pub fn existing_config_path() -> Result<Option<PathBuf>, 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<Option<PathBuf>, ConfigError> {
-    ConfigEnv::new().new_config_path()
+    ConfigEnv::new()?.new_config_path()
 }
 
 /// Environment variables that should be overridden by config values
@@ -777,7 +833,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::ExistingAndNew("config/jj/config.toml"),
@@ -790,7 +846,7 @@ mod tests {
         TestCase {
             files: vec![],
             cfg: ConfigEnv {
-                config_dir: Some("config".into()),
+                platform_config_dir: Some("config".into()),
                 ..Default::default()
             },
             want: Want::New("config/jj/config.toml"),
@@ -803,7 +859,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()
             },
@@ -844,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::ExistingAndNew("config/jj/config.toml"),
@@ -858,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::ExistingAndNew("home/.jjconfig.toml"),
@@ -881,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;
@@ -923,7 +979,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/cli/src/git_util.rs b/cli/src/git_util.rs
@@ -121,7 +121,7 @@ fn pinentry_get_pw(url: &str) -> Option<String> {
 #[tracing::instrument]
 fn get_ssh_keys(_username: &str) -> Vec<PathBuf> {
     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
@@ -744,11 +744,11 @@ On all platforms, the user's global `jj` configuration file is located at either
 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`           |
+| Platform | Value                                    | Example                                       |
+| :------- | ---------------------------------------- | --------------------------------------------- |
+| Linux    | `$XDG_CONFIG_HOME/jj/config.toml`          | `/home/alice/.config/jj/config.toml`            |
+| macOS    | `$XDG_CONFIG_HOME/jj/config.toml`          | `/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