-
Notifications
You must be signed in to change notification settings - Fork 532
config: Use XDG-standard config location on macOS #6300
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
nit: Is the old behavior actually incorrect? Say "Follow XDG spec for macOS config location" or something like that if that's the spec we're talking about. |
e0d7710
to
7050863
Compare
@martinvonz I personally believe strongly that it is incorrect. User-editable configuration should not be placed in It is not incorrect in that it will work, but it breaks the assumptions that normal people have for CLI programs on macOS. macOS is a Unix, and it is expected that configuration goes in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just some minor deprecation stuff but the patch SGTM
7050863
to
8546437
Compare
8546437
to
ddf454c
Compare
Late to the party here, but the NeXTStep-derived Conversely, the XDG Base Directory Specification carries no official weight on macOS. (Nor has it been universally adopted beyond Linux.) Now of course, I personally prefer using XDG (almost as much as I like a plain As long as all three options are possible, everything is probably fine! |
@wjv I think the arguments against using XDG, while not an official standard on macOS, has become a de-facto standard among the vast majority of CLI application developers, and as a person who has used macOS for [redacted] decades at this point, it is significantly more correct to use XDG rather than |
ddf454c
to
d661d7b
Compare
@strega-nil To be fair, Apple ships 3rd party tools that originate outside the macOS ecosystem that happen to support XDG. :) I see your arguments, here and elsewhere. I even agree. But as someone who has also used macOS for [redacted] decades, I think it would be a mistake to completely remove support for the directory that is, as much as anything, the standard. And that certainly some Mac users would expect. Look, as long as a simple config file in |
I'll work on this more this week maybe, but easter has been a bit hectic, so no work on this for now :) |
48540c5
to
6b5cb55
Compare
Alright, it has been worked on! |
5685af2
to
b1f1dfe
Compare
868c10f
to
96fe150
Compare
@martinvonz I believe I have resolved all of your concerns :) I split the new ConfigPath representation into its own commit, going before everything else. |
96fe150
to
8caf647
Compare
4b6a27c
to
d1110e1
Compare
Added some more info, and removed range-diff@@ Metadata
## Commit message ##
config: change how config paths are represented
+ This change, from an enum to a struct, is a more accurate representation
+ of the actual way that a ConfigPath works; additionally, it lets us add
+ different information without modifying every single enumeration field.
+
+ The change to `config_paths()`, to return a `ConfigPath` instead of a
+ `Path`, is less interesting, but in my opinion results in slightly more
+ obvious code. It also makes the test code significantly easier.
+
## cli/src/commands/config/mod.rs ##
@@ cli/src/commands/config/mod.rs: mod path;
mod set; @@ cli/Cargo.toml: clap_complete_nushell = { workspace = true }
gix = { workspace = true, optional = true }
## cli/src/config.rs ##
+@@ cli/src/config.rs: impl UnresolvedConfigEnv {
+
+ // theoretically this should be an `if let Some(...) = ... && ..., but that
+ // isn't stable
+- let get_existing = |opt: Option<ConfigPath>| match opt {
+- Some(path) if path.exists() => Some(path),
+- _ => None,
+- };
+- if let Some(path) = get_existing(platform_config_dir) {
+- paths.push(path);
++ if let Some(path) = platform_config_dir {
++ if path.exists() {
++ paths.push(path);
++ }
+ }
+ paths
+ }
@@ cli/src/config.rs: pub struct ConfigEnv {
impl ConfigEnv {
/// Initializes configuration loader based on environment variables. @@ cli/src/config.rs: impl UnresolvedConfigEnv {
}
}
@@ cli/src/config.rs: impl UnresolvedConfigEnv {
- if let Some(path) = get_existing(platform_config_dir) {
paths.push(path);
}
+
+- // theoretically this should be an `if let Some(...) = ... && ..., but that
++ // theoretically these should be an `if let Some(...) = ... && ..., but that
+ // isn't stable
+ if let Some(path) = platform_config_dir {
+ if path.exists() {
+ paths.push(path);
+ }
+ }
+
-+ if let Some(path) = get_existing(legacy_platform_config_path) {
-+ paths.push(path);
++ if let Some(path) = legacy_platform_config_path {
++ if path.exists() {
++ paths.push(path);
++ }
+ }
-+ if let Some(path) = get_existing(legacy_platform_config_dir) {
-+ paths.push(path);
++ if let Some(path) = legacy_platform_config_dir {
++ if path.exists() {
++ paths.push(path);
++ }
+ }
+
paths |
d1110e1
to
fd20633
Compare
fd20633
to
41a110a
Compare
I really hope that I've resolved all the nits now 😅 range-diff@@ Commit message
of the actual way that a ConfigPath works; additionally, it lets us add
different information without modifying every single enumeration field.
- The change to `config_paths()`, to return a `ConfigPath` instead of a
- `Path`, is less interesting, but in my opinion results in slightly more
- obvious code. It also makes the test code significantly easier.
-
- ## cli/src/commands/config/mod.rs ##
-@@ cli/src/commands/config/mod.rs: mod path;
- mod set;
- mod unset;
-
--use std::path::Path;
--
- use itertools::Itertools as _;
- use jj_lib::config::ConfigFile;
- use jj_lib::config::ConfigSource;
-@@ cli/src/commands/config/mod.rs: use crate::cli_util::CommandHelper;
- use crate::command_error::user_error;
- use crate::command_error::CommandError;
- use crate::config::ConfigEnv;
-+use crate::config::ConfigPath;
- use crate::ui::Ui;
-
- #[derive(clap::Args, Clone, Debug)]
-@@ cli/src/commands/config/mod.rs: impl ConfigLevelArgs {
- }
- }
-
-- fn config_paths<'a>(&self, config_env: &'a ConfigEnv) -> Result<Vec<&'a Path>, CommandError> {
-+ fn config_paths<'a>(
-+ &self,
-+ config_env: &'a ConfigEnv,
-+ ) -> Result<Vec<&'a ConfigPath>, CommandError> {
- if self.user {
- let paths = config_env.user_config_paths().collect_vec();
- if paths.is_empty() {
-
- ## cli/src/commands/config/path.rs ##
-@@ cli/src/commands/config/path.rs: pub fn cmd_config_path(
- ui.stdout(),
- "{}",
- config_path
-+ .path()
- .to_str()
- .ok_or_else(|| user_error("The config path is not valid UTF-8"))?
- )?;
-
## cli/src/config.rs ##
@@ cli/src/config.rs: impl AsMut<StackedConfig> for RawConfig {
}
@@ cli/src/config.rs: impl AsMut<StackedConfig> for RawConfig {
+/// - !exists(): a config file doesn't exist here, but a new file _can_ be
+/// created at this path
+#[derive(Clone, Debug)]
-+pub struct ConfigPath {
++struct ConfigPath {
+ path: PathBuf,
+ state: ConfigPathState,
}
@@ cli/src/config.rs: impl AsMut<StackedConfig> for RawConfig {
}
}
-- fn as_path(&self) -> &Path {
+ fn as_path(&self) -> &Path {
- match self {
- ConfigPath::Existing(path) | ConfigPath::New(path) => path,
-+ pub fn path(&self) -> &Path {
+ &self.path
+ }
+
-+ pub(crate) fn exists(&self) -> bool {
++ fn exists(&self) -> bool {
+ match self.state {
+ ConfigPathState::Exists => true,
+ ConfigPathState::New => false,
@@ cli/src/config.rs: impl UnresolvedConfigEnv {
}
paths
@@ cli/src/config.rs: impl ConfigEnv {
- }
-
- /// Returns the paths to the user-specific config files or directories.
-- pub fn user_config_paths(&self) -> impl Iterator<Item = &Path> {
-- self.user_config_paths.iter().map(|p| p.as_path())
-+ pub fn user_config_paths(&self) -> impl Iterator<Item = &ConfigPath> {
-+ self.user_config_paths.iter()
- }
-
/// Returns the paths to the existing user-specific config files or
/// directories.
-- pub fn existing_user_config_paths(&self) -> impl Iterator<Item = &Path> {
+ pub fn existing_user_config_paths(&self) -> impl Iterator<Item = &Path> {
- self.user_config_paths.iter().filter_map(|p| match p {
- ConfigPath::Existing(path) => Some(path.as_path()),
- _ => None,
- })
-+ pub fn existing_user_config_paths(&self) -> impl Iterator<Item = &ConfigPath> {
-+ self.user_config_paths().filter(|p| p.exists())
++ self.user_config_paths
++ .iter()
++ .filter_map(|p| if p.exists() { Some(p.as_path()) } else { None })
}
/// Returns user configuration files for modification. Instantiates one if
@@ cli/src/config.rs: impl ConfigEnv {
- .map(|path| {
- // No need to propagate io::Error here. If the directory
- // couldn't be created, file.save() would fail later.
-- if let Some(dir) = path.parent() {
-+ if let Some(dir) = path.path().parent() {
- create_dir_all(dir).ok();
- }
- // The path doesn't usually exist, but we shouldn't overwrite it
- // with an empty config if it did exist.
-- ConfigFile::load_or_empty(ConfigSource::User, path)
-+ ConfigFile::load_or_empty(ConfigSource::User, path.path())
- })
- .transpose()
- }
-@@ cli/src/config.rs: impl ConfigEnv {
- pub fn reload_user_config(&self, config: &mut RawConfig) -> Result<(), ConfigLoadError> {
- config.as_mut().remove_layers(ConfigSource::User);
- for path in self.existing_user_config_paths() {
-- if path.is_dir() {
-- config.as_mut().load_dir(ConfigSource::User, path)?;
-+ if path.path().is_dir() {
-+ config.as_mut().load_dir(ConfigSource::User, path.path())?;
- } else {
-- config.as_mut().load_file(ConfigSource::User, path)?;
-+ config.as_mut().load_file(ConfigSource::User, path.path())?;
- }
- }
- Ok(())
-@@ cli/src/config.rs: impl ConfigEnv {
- }
/// Returns a path to the repo-specific config file.
-- pub fn repo_config_path(&self) -> Option<&Path> {
+ pub fn repo_config_path(&self) -> Option<&Path> {
- self.repo_config_path.as_ref().map(ConfigPath::as_path)
-+ pub fn repo_config_path(&self) -> Option<&ConfigPath> {
-+ self.repo_config_path.as_ref()
++ self.repo_config_path.as_ref().map(|p| p.as_path())
}
/// Returns a path to the existing repo-specific config file.
@@ cli/src/config.rs: impl ConfigEnv {
- match &self.repo_config_path {
- Some(ConfigPath::Existing(path)) => Some(path),
+ match self.repo_config_path {
-+ Some(ref path) if path.exists() => Some(path.path()),
++ Some(ref path) if path.exists() => Some(path.as_path()),
_ => None,
}
}
-@@ cli/src/config.rs: impl ConfigEnv {
- self.repo_config_path()
- // The path doesn't usually exist, but we shouldn't overwrite it
- // with an empty config if it did exist.
-- .map(|path| ConfigFile::load_or_empty(ConfigSource::Repo, path))
-+ .map(|path| ConfigFile::load_or_empty(ConfigSource::Repo, path.path()))
- .transpose()
- }
-
@@ cli/src/config.rs: mod tests {
struct TestCase {
files: &'static [&'static str],
@@ cli/src/config.rs: mod tests {
+ }
+
+ fn matches(&self, root: &Path, path: &ConfigPath) -> bool {
-+ if root.join(self.path) != path.path() {
++ if root.join(self.path) != path.as_path() {
+ return false;
+ }
+
@@ cli/src/config.rs: mod tests {
- env.existing_user_config_paths().collect_vec(),
- expected_existing
- );
-+ let config_paths = env.user_config_paths().cloned().collect_vec();
++ let config_paths = env.user_config_paths.clone();
+ let correct = case.wants.len() == config_paths.len()
+ && case
+ .wants @@ cli/Cargo.toml: clap_complete_nushell = { workspace = true }
gix = { workspace = true, optional = true }
## cli/src/config.rs ##
+@@ cli/src/config.rs: use std::path::Path;
+ use std::path::PathBuf;
+ use std::process::Command;
+
++use etcetera::BaseStrategy as _;
+ use itertools::Itertools as _;
+ use jj_lib::config::ConfigFile;
+ use jj_lib::config::ConfigGetError;
@@ cli/src/config.rs: impl UnresolvedConfigEnv {
// theoretically this should be an `if let Some(...) = ... && ..., but that
@@ cli/src/config.rs: pub struct ConfigEnv {
+ etcetera::base_strategy::choose_native_strategy()
+ .ok()
+ .map(|s| {
-+ use etcetera::BaseStrategy as _;
+ // note that etcetera calls Library/Application Support the "data dir",
+ // Library/Preferences is supposed to be exclusively plists
+ s.data_dir()
+ })
+ } else {
-+ etcetera::choose_base_strategy().ok().map(|s| {
-+ use etcetera::BaseStrategy as _;
-+ s.config_dir()
-+ })
++ etcetera::choose_base_strategy()
++ .ok()
++ .map(|s| s.config_dir())
+ };
+
// Canonicalize home as we do canonicalize cwd in CliRunner. $HOME might @@ cli/src/config.rs: pub struct ConfigEnv {
/// Initializes configuration loader based on environment variables.
pub fn from_environment() -> Self {
- let config_dir = if cfg!(target_os = "macos") {
-+ let config_dir = etcetera::choose_base_strategy().ok().map(|s| {
-+ use etcetera::BaseStrategy as _;
-+ s.config_dir()
-+ });
++ let config_dir = etcetera::choose_base_strategy()
++ .ok()
++ .map(|s| s.config_dir());
+
+ // older versions of jj used a more "GUI" config option,
+ // which is not designed for user-editable configuration of CLI utilities.
@@ cli/src/config.rs: impl ConfigEnv {
s.data_dir()
})
} else {
-- etcetera::choose_base_strategy().ok().map(|s| {
-- use etcetera::BaseStrategy as _;
-- s.config_dir()
-- })
+- etcetera::choose_base_strategy()
+- .ok()
+- .map(|s| s.config_dir())
+ None
};
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, thanks!
This change, from an enum to a struct, is a more accurate representation of the actual way that a ConfigPath works; additionally, it lets us add different information without modifying every single enumeration field.
41a110a
to
0847672
Compare
range-diff
@@ cli/src/config.rs: impl UnresolvedConfigEnv {
paths.push(path);
}
- if let Some(path @ Existing(_)) = platform_config_dir {
+- paths.push(path);
+
+ // theoretically this should be an `if let Some(...) = ... && ..., but that
+ // isn't stable
-+ let get_existing = |opt: Option<ConfigPath>| match opt {
-+ Some(path) if path.exists() => Some(path),
-+ _ => None,
-+ };
-+ if let Some(path) = get_existing(platform_config_dir) {
- paths.push(path);
++ if let Some(path) = platform_config_dir {
++ if path.exists() {
++ paths.push(path);
++ }
}
paths
+ }
@@ cli/src/config.rs: impl ConfigEnv {
+
+ /// Returns the paths to the user-specific config files or directories.
+ pub fn user_config_paths(&self) -> impl Iterator<Item = &Path> {
+- self.user_config_paths.iter().map(|p| p.as_path())
++ self.user_config_paths.iter().map(ConfigPath::as_path)
+ }
+
/// Returns the paths to the existing user-specific config files or
/// directories.
pub fn existing_user_config_paths(&self) -> impl Iterator<Item = &Path> {
@@ cli/src/config.rs: impl ConfigEnv {
- })
+ self.user_config_paths
+ .iter()
-+ .filter_map(|p| if p.exists() { Some(p.as_path()) } else { None })
++ .filter(|p| p.exists())
++ .map(ConfigPath::as_path)
}
/// Returns user configuration files for modification. Instantiates one if
@@ cli/src/config.rs: mod tests {
+ }
+ }
+
-+ fn matches(&self, root: &Path, path: &ConfigPath) -> bool {
-+ if root.join(self.path) != path.as_path() {
-+ return false;
-+ }
++ fn rooted_path(&self, root: &Path) -> PathBuf {
++ root.join(self.path)
++ }
+
-+ match self.state {
-+ WantState::New => !path.exists(),
-+ WantState::Existing => path.exists(),
-+ }
++ fn exists(&self) -> bool {
++ matches!(self.state, WantState::Existing)
+ }
}
@@ cli/src/config.rs: mod tests {
let env = resolve_config_env(&case.env, tmp.path());
- let expected_existing: Vec<PathBuf> = case
-- .wants
-- .iter()
++ let all_expected_paths = case
+ .wants
+ .iter()
- .filter_map(|want| match want {
- Want::Existing(path) => Some(tmp.path().join(path)),
- _ => None,
@@ cli/src/config.rs: mod tests {
- env.existing_user_config_paths().collect_vec(),
- expected_existing
- );
-+ let config_paths = env.user_config_paths.clone();
-+ let correct = case.wants.len() == config_paths.len()
-+ && case
-+ .wants
-+ .iter()
-+ .zip(&config_paths)
-+ .all(|(w, p)| w.matches(tmp.path(), p));
++ .map(|w| w.rooted_path(tmp.path()))
++ .collect_vec();
++ let exists_expected_paths = case
++ .wants
++ .iter()
++ .filter(|w| w.exists())
++ .map(|w| w.rooted_path(tmp.path()))
++ .collect_vec();
- let expected_paths: Vec<PathBuf> = case
- .wants
@@ cli/src/config.rs: mod tests {
- })
- .collect();
- assert_eq!(env.user_config_paths().collect_vec(), expected_paths);
-+ assert!(correct, "{:#?} != {:#?}", case.wants, config_paths);
++ let all_paths = env.user_config_paths().collect_vec();
++ let exists_paths = env.existing_user_config_paths().collect_vec();
++
++ assert_eq!(all_paths, all_expected_paths);
++ assert_eq!(exists_paths, exists_expected_paths);
}
fn setup_config_fs(files: &[&str]) -> tempfile::TempDir {
@@ cli/src/config.rs: impl UnresolvedConfigEnv {
}
+
+ fn warn_for_deprecated_path(ui: &Ui, path: &Path, old: &str, new: &str) {
-+ let _ = writeln!(
++ let _ = indoc::writedoc!(
+ ui.warning_default(),
-+ "Deprecated configuration file `{}`:",
-+ path.display()
-+ );
-+ let _ = writeln!(
-+ ui.warning_default(),
-+ "Configuration files in `{old}` are deprecated, and support will be removed in a \
-+ future release.",
-+ );
-+ let _ = writeln!(
-+ ui.warning_default(),
-+ "Instead, move your configuration files to `{new}`.",
++ r"
++ Deprecated configuration file `{}`.
++ Configuration files in `{old}` are deprecated, and support will be removed in a future release.
++ Instead, move your configuration files to `{new}`.
++ ",
++ path.display(),
+ );
+ }
} |
Support existing users with the "legacy" config directory, as well. This will be deprecated in a latter commit.
0847672
to
0f89f2e
Compare
Following jj-vcs/jj#6300, Jujutsu has deprecated support for configuration files in `~/Library/Application Support` for darwin. The XDG-standard configuration location can be used instead, for all platforms.
Following jj-vcs/jj#6300, Jujutsu has deprecated support for configuration files in `~/Library/Application Support` for darwin. The XDG-standard configuration location can be used instead, for all platforms.
Following jj-vcs/jj#6300, Jujutsu has deprecated support for configuration files in `~/Library/Application Support` for darwin. The XDG-standard configuration location can be used instead, for all platforms.
Following jj-vcs/jj#6300, Jujutsu has deprecated support for configuration files in `~/Library/Application Support` for darwin. The XDG-standard configuration location can be used instead, for all platforms.
Following jj-vcs/jj#6300, Jujutsu has deprecated support for configuration files in `~/Library/Application Support` for darwin. The XDG-standard configuration location can be used instead, for all platforms.
…6994) Following jj-vcs/jj#6300, Jujutsu has deprecated support for configuration files in `~/Library/Application Support` for darwin. The XDG-standard configuration location can be used instead, for all platforms.
Changing where the default config location is on macOS, switching to etcetera from dirs.
Some points for discussion:
~/Library/Application Support
, ever, and certainly not for CLI applicationsCurrently, if you runjj config edit --user
, and you already have a config in~/Library/Application Support
, you'll get a picker for both; this seems less than ideal? Perhaps advice would be good? Or we could ignore the normal file if the legacy file exists?This has been changed as of latest patch set – it may be desireable to pull that patch out into its own PR. Let me know.This bugfix has been pulled into its own PR: config: only add new when no other config exists #6315This PR takes a lot of inspiration from @ckoehler's #3466, but has been rewritten since the current codebase is a hell of a lot more amenable to these changes.
Fixes #3434
Checklist
If applicable:
CHANGELOG.md
I have updated the config schema (cli/src/config-schema.json)