Skip to content

Commit

Permalink
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
config: refactor config_path() to be clearer
Browse files Browse the repository at this point in the history
ckoehler committed Apr 18, 2024
1 parent e352df8 commit 6d2cc82
Showing 2 changed files with 138 additions and 66 deletions.
7 changes: 3 additions & 4 deletions cli/src/cli_util.rs
Original file line number Diff line number Diff line change
@@ -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<PathBuf, CommandError> {
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!(
197 changes: 135 additions & 62 deletions cli/src/config.rs
Original file line number Diff line number Diff line change
@@ -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<PathBuf>) -> 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,81 +228,131 @@ 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() -> 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 {
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<ConfigPath, 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))));
}
// 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<Option<PathBuf>, 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<PathBuf> = vec![&platform_config, &xdg_config, &home_config]
.into_iter()
.unique()
.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(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<Option<PathBuf>, 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<Option<PathBuf>, 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<Option<PathBuf>, 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<Option<PathBuf>, ConfigError> {
/// public wrapper around `new_or_existing_config_path`, see its docs.
pub fn new_or_existing_config_path() -> Result<Option<PathBuf>, 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<tempfile::TempDir> {
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

0 comments on commit 6d2cc82

Please sign in to comment.