From 1abe1dceb55fc0216a4b4b4e1319025d7a31a3ab Mon Sep 17 00:00:00 2001 From: Yuya Nishihara Date: Fri, 29 Nov 2024 14:13:32 +0900 Subject: [PATCH] cli: load default/command-arg layers per source, clean up return types This patch removes pre-merge steps which depend on the ConfigBuilder API. Some of Vec could be changed to Vec (or Vec) to drop the layer parameter, but I'm not sure which is better. parse_config_args() will have to construct ConfigLayer (or ConfigTable + Option) if we add support for --config-file=PATH argument. --- cli/src/cli_util.rs | 29 ++++++++--------- cli/src/complete.rs | 4 +-- cli/src/config.rs | 67 ++++++++++++++++++-------------------- cli/src/merge_tools/mod.rs | 5 +-- 4 files changed, 49 insertions(+), 56 deletions(-) diff --git a/cli/src/cli_util.rs b/cli/src/cli_util.rs index d1d8a523f8..22c3b3b454 100644 --- a/cli/src/cli_util.rs +++ b/cli/src/cli_util.rs @@ -59,7 +59,9 @@ use jj_lib::backend::TreeValue; use jj_lib::commit::Commit; use jj_lib::config::ConfigGetError; use jj_lib::config::ConfigGetResultExt as _; +use jj_lib::config::ConfigLayer; use jj_lib::config::ConfigNamePathBuf; +use jj_lib::config::ConfigSource; use jj_lib::config::StackedConfig; use jj_lib::conflicts::ConflictMarkerStyle; use jj_lib::file_util; @@ -3251,7 +3253,7 @@ fn handle_early_args( args.config_toml.push(r#"ui.paginate="never""#.to_owned()); } if !args.config_toml.is_empty() { - config.add_layer(parse_config_args(&args.config_toml)?); + config.extend_layers(parse_config_args(&args.config_toml)?); ui.reset(config)?; } Ok(()) @@ -3360,7 +3362,7 @@ pub fn format_template(ui: &Ui, arg: &C, template: &TemplateRenderer, + config_layers: Vec, store_factories: StoreFactories, working_copy_factories: WorkingCopyFactories, workspace_loader_factory: Box, @@ -3385,7 +3387,7 @@ impl CliRunner { CliRunner { tracing_subscription, app: crate::commands::default_app(), - extra_configs: vec![], + config_layers: crate::config::default_config_layers(), store_factories: StoreFactories::default(), working_copy_factories: default_working_copy_factories(), workspace_loader_factory: Box::new(DefaultWorkspaceLoaderFactory), @@ -3417,8 +3419,12 @@ impl CliRunner { } /// Adds default configs in addition to the normal defaults. - pub fn add_extra_config(mut self, extra_configs: config::Config) -> Self { - self.extra_configs.push(extra_configs); + /// + /// The `layer.source` must be `Default`. Other sources such as `User` would + /// be replaced by loaded configuration. + pub fn add_extra_config(mut self, layer: ConfigLayer) -> Self { + assert_eq!(layer.source, ConfigSource::Default); + self.config_layers.push(layer); self } @@ -3620,17 +3626,10 @@ impl CliRunner { #[must_use] #[instrument(skip(self))] pub fn run(mut self) -> ExitCode { - let builder = config::Config::builder().add_source(crate::config::default_config()); - let config = self - .extra_configs - .drain(..) - .fold(builder, |builder, config| builder.add_source(config)) - .build() - .unwrap(); - let stacked_config = config_from_environment(config); - let mut ui = Ui::with_config(&stacked_config) + let config = config_from_environment(self.config_layers.drain(..)); + let mut ui = Ui::with_config(&config) .expect("default config should be valid, env vars are stringly typed"); - let result = self.run_internal(&mut ui, stacked_config); + let result = self.run_internal(&mut ui, config); let exit_code = handle_command_result(&mut ui, result); ui.finalize_pager(); exit_code diff --git a/cli/src/complete.rs b/cli/src/complete.rs index 6fd34d37c2..a79c9ed6ee 100644 --- a/cli/src/complete.rs +++ b/cli/src/complete.rs @@ -29,7 +29,7 @@ use crate::cli_util::GlobalArgs; use crate::command_error::user_error; use crate::command_error::CommandError; use crate::config::config_from_environment; -use crate::config::default_config; +use crate::config::default_config_layers; use crate::config::ConfigEnv; use crate::config::CONFIG_SCHEMA; use crate::ui::Ui; @@ -687,7 +687,7 @@ fn get_jj_command() -> Result<(JjBuilder, UserSettings), CommandError> { // child process. This shouldn't fail, since none of the global args are // required. let app = crate::commands::default_app(); - let mut config = config_from_environment(default_config()); + let mut config = config_from_environment(default_config_layers()); let ui = Ui::with_config(&config).expect("default config should be valid"); let cwd = std::env::current_dir() .and_then(|cwd| cwd.canonicalize()) diff --git a/cli/src/config.rs b/cli/src/config.rs index 426409b598..c974b1524b 100644 --- a/cli/src/config.rs +++ b/cli/src/config.rs @@ -333,7 +333,8 @@ impl ConfigEnv { } } -/// Initializes stacked config with the given `default` and infallible sources. +/// Initializes stacked config with the given `default_layers` and infallible +/// sources. /// /// Sources from the lowest precedence: /// 1. Default @@ -345,19 +346,18 @@ impl ConfigEnv { /// 7. Command-line arguments `--config-toml` /// /// This function sets up 1, 2, and 6. -pub fn config_from_environment(default: config::Config) -> StackedConfig { +pub fn config_from_environment( + default_layers: impl IntoIterator, +) -> StackedConfig { let mut config = StackedConfig::empty(); - config.add_layer(ConfigLayer::with_data(ConfigSource::Default, default)); - config.add_layer(ConfigLayer::with_data(ConfigSource::EnvBase, env_base())); - config.add_layer(ConfigLayer::with_data( - ConfigSource::EnvOverrides, - env_overrides(), - )); + config.extend_layers(default_layers); + config.add_layer(env_base_layer()); + config.add_layer(env_overrides_layer()); config } /// Environment variables that should be overridden by config values -fn env_base() -> config::Config { +fn env_base_layer() -> ConfigLayer { let mut builder = config::Config::builder(); if env::var("NO_COLOR").is_ok() { // "User-level configuration files and per-instance command-line arguments @@ -372,35 +372,31 @@ fn env_base() -> config::Config { } else if let Ok(value) = env::var("EDITOR") { builder = builder.set_override("ui.editor", value).unwrap(); } - - builder.build().unwrap() + ConfigLayer::with_data(ConfigSource::EnvBase, builder.build().unwrap()) } -pub fn default_config() -> config::Config { +pub fn default_config_layers() -> Vec { // Syntax error in default config isn't a user error. That's why defaults are // loaded by separate builder. - macro_rules! from_toml { - ($file:literal) => { - config::File::from_str(include_str!($file), config::FileFormat::Toml) - }; - } - let mut builder = config::Config::builder() - .add_source(from_toml!("config/colors.toml")) - .add_source(from_toml!("config/merge_tools.toml")) - .add_source(from_toml!("config/misc.toml")) - .add_source(from_toml!("config/revsets.toml")) - .add_source(from_toml!("config/templates.toml")); + let parse = |text: &'static str| ConfigLayer::parse(ConfigSource::Default, text).unwrap(); + let mut layers = vec![ + parse(include_str!("config/colors.toml")), + parse(include_str!("config/merge_tools.toml")), + parse(include_str!("config/misc.toml")), + parse(include_str!("config/revsets.toml")), + parse(include_str!("config/templates.toml")), + ]; if cfg!(unix) { - builder = builder.add_source(from_toml!("config/unix.toml")); + layers.push(parse(include_str!("config/unix.toml"))); } if cfg!(windows) { - builder = builder.add_source(from_toml!("config/windows.toml")); + layers.push(parse(include_str!("config/windows.toml"))); } - builder.build().unwrap() + layers } /// Environment variables that override config values -fn env_overrides() -> config::Config { +fn env_overrides_layer() -> ConfigLayer { let mut builder = config::Config::builder(); if let Ok(value) = env::var("JJ_USER") { builder = builder.set_override("user.name", value).unwrap(); @@ -432,18 +428,19 @@ fn env_overrides() -> config::Config { if let Ok(value) = env::var("JJ_EDITOR") { builder = builder.set_override("ui.editor", value).unwrap(); } - builder.build().unwrap() + ConfigLayer::with_data(ConfigSource::EnvOverrides, builder.build().unwrap()) } /// Parses `--config-toml` arguments. -pub fn parse_config_args(toml_strs: &[String]) -> Result { - let config = toml_strs +pub fn parse_config_args(toml_strs: &[String]) -> Result, ConfigError> { + // It might look silly that a layer is constructed per argument, but + // --config-toml argument can contain a full TOML document, and it makes + // sense to preserve line numbers within the doc. If we add + // --config=KEY=VALUE, multiple values might be loaded into one layer. + toml_strs .iter() - .fold(config::Config::builder(), |builder, s| { - builder.add_source(config::File::from_str(s, config::FileFormat::Toml)) - }) - .build()?; - Ok(ConfigLayer::with_data(ConfigSource::CommandArg, config)) + .map(|text| ConfigLayer::parse(ConfigSource::CommandArg, text)) + .try_collect() } fn read_config(path: &Path) -> Result, CommandError> { diff --git a/cli/src/merge_tools/mod.rs b/cli/src/merge_tools/mod.rs index 25d829936a..ec42601c8a 100644 --- a/cli/src/merge_tools/mod.rs +++ b/cli/src/merge_tools/mod.rs @@ -364,10 +364,7 @@ mod tests { fn config_from_string(text: &str) -> StackedConfig { let mut config = StackedConfig::empty(); // Load defaults to test the default args lookup - config.add_layer(ConfigLayer::with_data( - ConfigSource::Default, - crate::config::default_config(), - )); + config.extend_layers(crate::config::default_config_layers()); config.add_layer(ConfigLayer::parse(ConfigSource::User, text).unwrap()); config }