From e40293fe8aa47cee93d19308f731938a69cdba25 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), 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 a9d8264d93..b6090184b6 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; @@ -676,7 +676,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 }