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<ConfigLayer> could be changed to Vec<config::Config> (or
Vec<ConfigTable>) to drop the layer parameter, but I'm not sure which is
better. parse_config_args() will have to construct ConfigLayer (or ConfigTable +
Option<PathBuf>) if we add support for --config-file=PATH argument.

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<C: Clone>(ui: &Ui, arg: &C, template: &TemplateRenderer<C
 pub struct CliRunner {
     tracing_subscription: TracingSubscription,
     app: Command,
-    extra_configs: Vec<config::Config>,
+    config_layers: Vec<ConfigLayer>,
     store_factories: StoreFactories,
     working_copy_factories: WorkingCopyFactories,
     workspace_loader_factory: Box<dyn WorkspaceLoaderFactory>,
@@ -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

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())

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<Item = ConfigLayer>,
+) -> 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<ConfigLayer> {
     // 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<ConfigLayer, ConfigError> {
-    let config = toml_strs
+pub fn parse_config_args(toml_strs: &[String]) -> Result<Vec<ConfigLayer>, 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<toml_edit::ImDocument<String>, CommandError> {

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
     }