From f819dc934552f95d654ec00b449857833b6c42d6 Mon Sep 17 00:00:00 2001 From: Yuya Nishihara Date: Fri, 22 Nov 2024 19:32:34 +0900 Subject: [PATCH] config: extract layer management from cli LayeredConfigs to jj-lib Layers are now constructed per file, not per source type. This will allow us to report precise position where bad configuration variable is set. Because layers are now created per file, it makes sense to require existence of the file, instead of ignoring missing files which would leave an empty layer in the stack. The path existence is tested by ConfigEnv::existing_config_path(), so I simply made the new load_file/dir() methods stricter. However, we still need config::File::required(false) flag in order to accept /dev/null as an empty TOML file. The lib type is renamed to StackedConfig to avoid name conflicts. The cli LayeredConfigs will probably be reorganized as an environment object that builds a StackedConfig. --- cli/src/config.rs | 151 ++++++-------------- cli/tests/test_builtin_aliases.rs | 6 +- lib/src/config.rs | 224 ++++++++++++++++++++++++++++++ 3 files changed, 272 insertions(+), 109 deletions(-) diff --git a/cli/src/config.rs b/cli/src/config.rs index 621d49c0fc..08367ab458 100644 --- a/cli/src/config.rs +++ b/cli/src/config.rs @@ -23,8 +23,10 @@ use std::process::Command; use itertools::Itertools; use jj_lib::config::ConfigError; +use jj_lib::config::ConfigLayer; use jj_lib::config::ConfigNamePathBuf; use jj_lib::config::ConfigSource; +use jj_lib::config::StackedConfig; use jj_lib::settings::ConfigResultExt as _; use regex::Captures; use regex::Regex; @@ -105,33 +107,32 @@ pub struct AnnotatedValue { /// 7. Command-line arguments `--config-toml` #[derive(Clone, Debug)] pub struct LayeredConfigs { - default: config::Config, - env_base: config::Config, - // TODO: Track explicit file paths, especially for when user config is a dir. - user: Option, - repo: Option, - env_overrides: config::Config, - arg_overrides: Option, + inner: StackedConfig, } impl LayeredConfigs { /// Initializes configs with infallible sources. pub fn from_environment(default: config::Config) -> Self { - LayeredConfigs { - default, - env_base: env_base(), - user: None, - repo: None, - env_overrides: env_overrides(), - arg_overrides: None, - } + let mut inner = StackedConfig::empty(); + inner.add_layer(ConfigLayer::with_data(ConfigSource::Default, default)); + inner.add_layer(ConfigLayer::with_data(ConfigSource::EnvBase, env_base())); + inner.add_layer(ConfigLayer::with_data( + ConfigSource::EnvOverrides, + env_overrides(), + )); + LayeredConfigs { inner } } #[instrument] pub fn read_user_config(&mut self) -> Result<(), ConfigEnvError> { - self.user = existing_config_path()? - .map(|path| read_config_path(&path)) - .transpose()?; + self.inner.remove_layers(ConfigSource::User); + if let Some(path) = existing_config_path()? { + if path.is_dir() { + self.inner.load_dir(ConfigSource::User, path)?; + } else { + self.inner.load_file(ConfigSource::User, path)?; + } + } Ok(()) } @@ -141,7 +142,11 @@ impl LayeredConfigs { #[instrument] pub fn read_repo_config(&mut self, repo_path: &Path) -> Result<(), ConfigEnvError> { - self.repo = Some(read_config_file(&self.repo_config_path(repo_path))?); + self.inner.remove_layers(ConfigSource::Repo); + let path = self.repo_config_path(repo_path); + if path.exists() { + self.inner.load_file(ConfigSource::Repo, path)?; + } Ok(()) } @@ -150,41 +155,28 @@ impl LayeredConfigs { } pub fn parse_config_args(&mut self, toml_strs: &[String]) -> Result<(), ConfigEnvError> { + self.inner.remove_layers(ConfigSource::CommandArg); let config = toml_strs .iter() .fold(config::Config::builder(), |builder, s| { builder.add_source(config::File::from_str(s, config::FileFormat::Toml)) }) .build()?; - self.arg_overrides = Some(config); + self.inner + .add_layer(ConfigLayer::with_data(ConfigSource::CommandArg, config)); Ok(()) } /// Creates new merged config. pub fn merge(&self) -> config::Config { - self.sources() - .into_iter() - .map(|(_, config)| config) - .fold(config::Config::builder(), |builder, source| { - builder.add_source(source.clone()) - }) - .build() - .expect("loaded configs should be merged without error") + self.inner.merge() } - pub fn sources(&self) -> Vec<(ConfigSource, &config::Config)> { - let config_sources = [ - (ConfigSource::Default, Some(&self.default)), - (ConfigSource::EnvBase, Some(&self.env_base)), - (ConfigSource::User, self.user.as_ref()), - (ConfigSource::Repo, self.repo.as_ref()), - (ConfigSource::EnvOverrides, Some(&self.env_overrides)), - (ConfigSource::CommandArg, self.arg_overrides.as_ref()), - ]; - config_sources - .into_iter() - .filter_map(|(source, config)| config.map(|c| (source, c))) - .collect_vec() + pub fn sources(&self) -> impl DoubleEndedIterator { + self.inner + .layers() + .iter() + .map(|layer| (layer.source, &layer.data)) } pub fn resolved_config_values( @@ -432,46 +424,6 @@ fn env_overrides() -> config::Config { builder.build().unwrap() } -fn read_config_file(path: &Path) -> Result { - config::Config::builder() - .add_source( - config::File::from(path) - .required(false) - .format(config::FileFormat::Toml), - ) - .build() -} - -fn read_config_path(config_path: &Path) -> Result { - let mut files = vec![]; - if config_path.is_dir() { - if let Ok(read_dir) = config_path.read_dir() { - // TODO: Walk the directory recursively? - for dir_entry in read_dir.flatten() { - let path = dir_entry.path(); - if path.is_file() { - files.push(path); - } - } - } - files.sort(); - } else { - files.push(config_path.to_owned()); - } - - files - .iter() - .fold(config::Config::builder(), |builder, path| { - // TODO: Accept other formats and/or accept only certain file extensions? - builder.add_source( - config::File::from(path.as_ref()) - .required(false) - .format(config::FileFormat::Toml), - ) - }) - .build() -} - fn read_config(path: &Path) -> Result, CommandError> { let config_toml = std::fs::read_to_string(path).or_else(|err| { match err.kind() { @@ -781,14 +733,8 @@ mod tests { #[test] fn test_layered_configs_resolved_config_values_empty() { - let empty_config = config::Config::default(); let layered_configs = LayeredConfigs { - default: empty_config.clone(), - env_base: empty_config.clone(), - user: None, - repo: None, - env_overrides: empty_config, - arg_overrides: None, + inner: StackedConfig::empty(), }; assert_eq!( layered_configs @@ -800,7 +746,6 @@ mod tests { #[test] fn test_layered_configs_resolved_config_values_single_key() { - let empty_config = config::Config::default(); let env_base_config = config::Config::builder() .set_override("user.name", "base-user-name") .unwrap() @@ -813,14 +758,13 @@ mod tests { .unwrap() .build() .unwrap(); - let layered_configs = LayeredConfigs { - default: empty_config.clone(), - env_base: env_base_config, - user: None, - repo: Some(repo_config), - env_overrides: empty_config, - arg_overrides: None, - }; + let mut inner = StackedConfig::empty(); + inner.add_layer(ConfigLayer::with_data( + ConfigSource::EnvBase, + env_base_config, + )); + inner.add_layer(ConfigLayer::with_data(ConfigSource::Repo, repo_config)); + let layered_configs = LayeredConfigs { inner }; // Note: "email" is alphabetized, before "name" from same layer. insta::assert_debug_snapshot!( layered_configs.resolved_config_values(&ConfigNamePathBuf::root()).unwrap(), @@ -947,7 +891,6 @@ mod tests { #[test] fn test_layered_configs_resolved_config_values_filter_path() { - let empty_config = config::Config::default(); let user_config = config::Config::builder() .set_override("test-table1.foo", "user-FOO") .unwrap() @@ -960,14 +903,10 @@ mod tests { .unwrap() .build() .unwrap(); - let layered_configs = LayeredConfigs { - default: empty_config.clone(), - env_base: empty_config.clone(), - user: Some(user_config), - repo: Some(repo_config), - env_overrides: empty_config, - arg_overrides: None, - }; + let mut inner = StackedConfig::empty(); + inner.add_layer(ConfigLayer::with_data(ConfigSource::User, user_config)); + inner.add_layer(ConfigLayer::with_data(ConfigSource::Repo, repo_config)); + let layered_configs = LayeredConfigs { inner }; insta::assert_debug_snapshot!( layered_configs .resolved_config_values(&ConfigNamePathBuf::from_iter(["test-table1"])) diff --git a/cli/tests/test_builtin_aliases.rs b/cli/tests/test_builtin_aliases.rs index e20bd89a71..3ece226076 100644 --- a/cli/tests/test_builtin_aliases.rs +++ b/cli/tests/test_builtin_aliases.rs @@ -151,9 +151,9 @@ fn test_builtin_user_redefines_builtin_immutable_heads() { │ (empty) description 1 ~ "###); - insta::assert_snapshot!(stderr, @r###" + insta::assert_snapshot!(stderr, @r" Warning: Redefining `revset-aliases.builtin_immutable_heads()` is not recommended; redefine `immutable_heads()` instead - Warning: Redefining `revset-aliases.immutable()` is not recommended; redefine `immutable_heads()` instead Warning: Redefining `revset-aliases.mutable()` is not recommended; redefine `immutable_heads()` instead - "###); + Warning: Redefining `revset-aliases.immutable()` is not recommended; redefine `immutable_heads()` instead + "); } diff --git a/lib/src/config.rs b/lib/src/config.rs index 20595ddb4d..250645de5c 100644 --- a/lib/src/config.rs +++ b/lib/src/config.rs @@ -16,12 +16,17 @@ use std::borrow::Cow; use std::fmt; +use std::ops::Range; +use std::path::Path; +use std::path::PathBuf; use std::slice; use std::str::FromStr; use config::Source as _; use itertools::Itertools as _; +use crate::file_util::IoResultExt as _; + /// Error that can occur when accessing configuration. // TODO: will be replaced with our custom error type pub type ConfigError = config::ConfigError; @@ -145,6 +150,158 @@ pub enum ConfigSource { CommandArg, } +/// Set of configuration variables with source information. +#[derive(Clone, Debug)] +pub struct ConfigLayer { + /// Source type of this layer. + pub source: ConfigSource, + /// Source file path of this layer if any. + pub path: Option, + /// Configuration variables. + pub data: config::Config, +} + +impl ConfigLayer { + /// Creates new layer with the configuration variables `data`. + pub fn with_data(source: ConfigSource, data: config::Config) -> Self { + ConfigLayer { + source, + path: None, + data, + } + } + + fn load_from_file(source: ConfigSource, path: PathBuf) -> Result { + // TODO: will be replaced with toml_edit::DocumentMut or ImDocument + let data = config::Config::builder() + .add_source( + config::File::from(path.clone()) + // TODO: The path should exist, but the config crate refuses + // to read a special file (e.g. /dev/null) as TOML. + .required(false) + .format(config::FileFormat::Toml), + ) + .build()?; + Ok(ConfigLayer { + source, + path: Some(path), + data, + }) + } + + fn load_from_dir(source: ConfigSource, path: &Path) -> Result, ConfigError> { + // TODO: Walk the directory recursively? + let mut file_paths: Vec<_> = path + .read_dir() + .and_then(|dir_entries| { + dir_entries + .map(|entry| Ok(entry?.path())) + // TODO: Accept only certain file extensions? + .filter_ok(|path| path.is_file()) + .try_collect() + }) + .context(path) + .map_err(|err| ConfigError::Foreign(err.into()))?; + file_paths.sort_unstable(); + file_paths + .into_iter() + .map(|path| Self::load_from_file(source, path)) + .try_collect() + } +} + +/// Stack of configuration layers which can be merged as needed. +#[derive(Clone, Debug)] +pub struct StackedConfig { + /// Layers sorted by `source` (the lowest precedence one first.) + layers: Vec, +} + +impl StackedConfig { + /// Creates an empty stack of configuration layers. + pub fn empty() -> Self { + StackedConfig { layers: vec![] } + } + + /// Loads config file from the specified `path`, inserts it at the position + /// specified by `source`. The file should exist. + pub fn load_file( + &mut self, + source: ConfigSource, + path: impl Into, + ) -> Result<(), ConfigError> { + let layer = ConfigLayer::load_from_file(source, path.into())?; + self.add_layer(layer); + Ok(()) + } + + /// Loads config files from the specified directory `path`, inserts them at + /// the position specified by `source`. The directory should exist. + pub fn load_dir( + &mut self, + source: ConfigSource, + path: impl AsRef, + ) -> Result<(), ConfigError> { + let layers = ConfigLayer::load_from_dir(source, path.as_ref())?; + let index = self.insert_point(source); + self.layers.splice(index..index, layers); + Ok(()) + } + + /// Inserts new layer at the position specified by `layer.source`. + pub fn add_layer(&mut self, layer: ConfigLayer) { + let index = self.insert_point(layer.source); + self.layers.insert(index, layer); + } + + /// Removes layers of the specified `source`. + pub fn remove_layers(&mut self, source: ConfigSource) { + self.layers.drain(self.layer_range(source)); + } + + fn layer_range(&self, source: ConfigSource) -> Range { + // Linear search since the size of Vec wouldn't be large. + let start = self + .layers + .iter() + .take_while(|layer| layer.source < source) + .count(); + let count = self.layers[start..] + .iter() + .take_while(|layer| layer.source == source) + .count(); + start..(start + count) + } + + fn insert_point(&self, source: ConfigSource) -> usize { + // Search from end since layers are usually added in order, and the size + // of Vec wouldn't be large enough to do binary search. + let skip = self + .layers + .iter() + .rev() + .take_while(|layer| layer.source > source) + .count(); + self.layers.len() - skip + } + + /// Layers sorted by precedence. + pub fn layers(&self) -> &[ConfigLayer] { + &self.layers + } + + /// Creates new merged config. + pub fn merge(&self) -> config::Config { + self.layers + .iter() + .fold(config::Config::builder(), |builder, layer| { + builder.add_source(layer.data.clone()) + }) + .build() + .expect("loaded configs should be merged without error") + } +} + #[cfg(test)] mod tests { use super::*; @@ -185,4 +342,71 @@ mod tests { (Some("foo.bar".into()), [key("baz()")].as_slice()) ); } + + #[test] + fn test_stacked_config_layer_order() { + let empty_data = || config::Config::builder().build().unwrap(); + let layer_sources = |config: &StackedConfig| { + config + .layers() + .iter() + .map(|layer| layer.source) + .collect_vec() + }; + + // Insert in reverse order + let mut config = StackedConfig::empty(); + config.add_layer(ConfigLayer::with_data(ConfigSource::Repo, empty_data())); + config.add_layer(ConfigLayer::with_data(ConfigSource::User, empty_data())); + config.add_layer(ConfigLayer::with_data(ConfigSource::Default, empty_data())); + assert_eq!( + layer_sources(&config), + vec![ + ConfigSource::Default, + ConfigSource::User, + ConfigSource::Repo, + ] + ); + + // Insert some more + config.add_layer(ConfigLayer::with_data( + ConfigSource::CommandArg, + empty_data(), + )); + config.add_layer(ConfigLayer::with_data(ConfigSource::EnvBase, empty_data())); + config.add_layer(ConfigLayer::with_data(ConfigSource::User, empty_data())); + assert_eq!( + layer_sources(&config), + vec![ + ConfigSource::Default, + ConfigSource::EnvBase, + ConfigSource::User, + ConfigSource::User, + ConfigSource::Repo, + ConfigSource::CommandArg, + ] + ); + + // Remove last, first, middle + config.remove_layers(ConfigSource::CommandArg); + config.remove_layers(ConfigSource::Default); + config.remove_layers(ConfigSource::User); + assert_eq!( + layer_sources(&config), + vec![ConfigSource::EnvBase, ConfigSource::Repo] + ); + + // Remove unknown + config.remove_layers(ConfigSource::Default); + config.remove_layers(ConfigSource::EnvOverrides); + assert_eq!( + layer_sources(&config), + vec![ConfigSource::EnvBase, ConfigSource::Repo] + ); + + // Remove remainders + config.remove_layers(ConfigSource::EnvBase); + config.remove_layers(ConfigSource::Repo); + assert_eq!(layer_sources(&config), vec![]); + } }