From 439e174cb38bfad13c61550280d20dd8e56ad01c Mon Sep 17 00:00:00 2001
From: Yuya Nishihara <yuya@tcha.org>
Date: Sat, 23 Nov 2024 19:42:17 +0900
Subject: [PATCH] config: replace LayeredConfigs with StackedConfig

Parsed --config-toml args are simply appended because the caller knows there
should be no preloaded CommandArg layers.
---
 cli/src/cli_util.rs             |  62 +++++++++----------
 cli/src/commands/config/list.rs |   4 +-
 cli/src/complete.rs             |  16 ++---
 cli/src/config.rs               | 104 ++++++++++----------------------
 cli/src/revset_util.rs          |  10 +--
 5 files changed, 78 insertions(+), 118 deletions(-)

diff --git a/cli/src/cli_util.rs b/cli/src/cli_util.rs
index 04cf0cacc3..0454b78909 100644
--- a/cli/src/cli_util.rs
+++ b/cli/src/cli_util.rs
@@ -59,6 +59,7 @@ use jj_lib::backend::TreeValue;
 use jj_lib::commit::Commit;
 use jj_lib::config::ConfigError;
 use jj_lib::config::ConfigSource;
+use jj_lib::config::StackedConfig;
 use jj_lib::conflicts::ConflictMarkerStyle;
 use jj_lib::file_util;
 use jj_lib::fileset;
@@ -147,9 +148,10 @@ use crate::command_error::CommandError;
 use crate::commit_templater::CommitTemplateLanguage;
 use crate::commit_templater::CommitTemplateLanguageExtension;
 use crate::complete;
+use crate::config::config_from_environment;
+use crate::config::parse_config_args;
 use crate::config::CommandNameAndArgs;
 use crate::config::ConfigEnv;
-use crate::config::LayeredConfigs;
 use crate::diff_util;
 use crate::diff_util::DiffFormat;
 use crate::diff_util::DiffFormatArgs;
@@ -279,7 +281,7 @@ struct CommandHelperData {
     global_args: GlobalArgs,
     config_env: ConfigEnv,
     settings: UserSettings,
-    layered_configs: LayeredConfigs,
+    stacked_config: StackedConfig,
     revset_extensions: Arc<RevsetExtensions>,
     commit_template_extensions: Vec<Arc<dyn CommitTemplateLanguageExtension>>,
     operation_template_extensions: Vec<Arc<dyn OperationTemplateLanguageExtension>>,
@@ -322,8 +324,8 @@ impl CommandHelper {
     }
 
     // TODO: will be moved to UserSettings
-    pub fn layered_configs(&self) -> &LayeredConfigs {
-        &self.data.layered_configs
+    pub fn stacked_config(&self) -> &StackedConfig {
+        &self.data.stacked_config
     }
 
     pub fn revset_extensions(&self) -> &Arc<RevsetExtensions> {
@@ -335,7 +337,7 @@ impl CommandHelper {
     /// For most commands that depend on a loaded repo, you should use
     /// `WorkspaceCommandHelper::template_aliases_map()` instead.
     fn load_template_aliases(&self, ui: &Ui) -> Result<TemplateAliasesMap, CommandError> {
-        load_template_aliases(ui, &self.data.layered_configs)
+        load_template_aliases(ui, &self.data.stacked_config)
     }
 
     /// Parses template of the given language into evaluation tree.
@@ -722,7 +724,7 @@ impl WorkspaceCommandEnvironment {
     #[instrument(skip_all)]
     fn new(ui: &Ui, command: &CommandHelper, workspace: &Workspace) -> Result<Self, CommandError> {
         let revset_aliases_map =
-            revset_util::load_revset_aliases(ui, &command.data.layered_configs)?;
+            revset_util::load_revset_aliases(ui, &command.data.stacked_config)?;
         let template_aliases_map = command.load_template_aliases(ui)?;
         let path_converter = RepoPathUiConverter::Fs {
             cwd: command.cwd().to_owned(),
@@ -2689,14 +2691,14 @@ pub fn update_working_copy(
 
 fn load_template_aliases(
     ui: &Ui,
-    layered_configs: &LayeredConfigs,
+    stacked_config: &StackedConfig,
 ) -> Result<TemplateAliasesMap, CommandError> {
     const TABLE_KEY: &str = "template-aliases";
     let mut aliases_map = TemplateAliasesMap::new();
     // Load from all config layers in order. 'f(x)' in default layer should be
     // overridden by 'f(a)' in user.
-    for (_, config) in layered_configs.sources() {
-        let table = if let Some(table) = config.get_table(TABLE_KEY).optional()? {
+    for layer in stacked_config.layers() {
+        let table = if let Some(table) = layer.data.get_table(TABLE_KEY).optional()? {
             table
         } else {
             continue;
@@ -3239,7 +3241,7 @@ fn handle_early_args(
     ui: &mut Ui,
     app: &Command,
     args: &[String],
-    layered_configs: &mut LayeredConfigs,
+    config: &mut StackedConfig,
 ) -> Result<(), CommandError> {
     // ignore_errors() bypasses errors like missing subcommand
     let early_matches = app
@@ -3269,8 +3271,8 @@ fn handle_early_args(
         args.config_toml.push(r#"ui.paginate="never""#.to_owned());
     }
     if !args.config_toml.is_empty() {
-        layered_configs.parse_config_args(&args.config_toml)?;
-        ui.reset(&layered_configs.merge())?;
+        config.add_layer(parse_config_args(&args.config_toml)?);
+        ui.reset(&config.merge())?;
     }
     Ok(())
 }
@@ -3346,9 +3348,9 @@ pub fn parse_args(
     app: &Command,
     tracing_subscription: &TracingSubscription,
     string_args: &[String],
-    layered_configs: &mut LayeredConfigs,
+    config: &mut StackedConfig,
 ) -> Result<(ArgMatches, Args), CommandError> {
-    handle_early_args(ui, app, string_args, layered_configs)?;
+    handle_early_args(ui, app, string_args, config)?;
     let matches = app
         .clone()
         .arg_required_else_help(true)
@@ -3542,7 +3544,7 @@ impl CliRunner {
     fn run_internal(
         self,
         ui: &mut Ui,
-        mut layered_configs: LayeredConfigs,
+        mut stacked_config: StackedConfig,
     ) -> Result<(), CommandError> {
         // `cwd` is canonicalized for consistency with `Workspace::workspace_root()` and
         // to easily compute relative paths between them.
@@ -3562,12 +3564,12 @@ impl CliRunner {
             .workspace_loader_factory
             .create(find_workspace_dir(&cwd))
             .map_err(|err| map_workspace_load_error(err, None));
-        config_env.reload_user_config(&mut layered_configs)?;
+        config_env.reload_user_config(&mut stacked_config)?;
         if let Ok(loader) = &maybe_cwd_workspace_loader {
             config_env.reset_repo_path(loader.repo_path());
-            config_env.reload_repo_config(&mut layered_configs)?;
+            config_env.reload_repo_config(&mut stacked_config)?;
         }
-        let config = layered_configs.merge();
+        let config = stacked_config.merge();
         ui.reset(&config).map_err(|e| {
             let user_config_path = config_env.existing_user_config_path();
             let repo_config_path = config_env.existing_repo_config_path();
@@ -3589,9 +3591,9 @@ impl CliRunner {
             &self.app,
             &self.tracing_subscription,
             &string_args,
-            &mut layered_configs,
+            &mut stacked_config,
         )
-        .map_err(|err| map_clap_cli_error(err, ui, &layered_configs))?;
+        .map_err(|err| map_clap_cli_error(err, ui, &stacked_config))?;
         for process_global_args_fn in self.process_global_args_fns {
             process_global_args_fn(ui, &matches)?;
         }
@@ -3603,14 +3605,14 @@ impl CliRunner {
                 .create(&cwd.join(path))
                 .map_err(|err| map_workspace_load_error(err, Some(path)))?;
             config_env.reset_repo_path(loader.repo_path());
-            config_env.reload_repo_config(&mut layered_configs)?;
+            config_env.reload_repo_config(&mut stacked_config)?;
             Ok(loader)
         } else {
             maybe_cwd_workspace_loader
         };
 
         // Apply workspace configs and --config-toml arguments.
-        let config = layered_configs.merge();
+        let config = stacked_config.merge();
         ui.reset(&config)?;
 
         // If -R is specified, check if the expanded arguments differ. Aliases
@@ -3634,7 +3636,7 @@ impl CliRunner {
             global_args: args.global_args,
             config_env,
             settings,
-            layered_configs,
+            stacked_config,
             revset_extensions: self.revset_extensions.into(),
             commit_template_extensions: self.commit_template_extensions,
             operation_template_extensions: self.operation_template_extensions,
@@ -3661,21 +3663,17 @@ impl CliRunner {
             .fold(builder, |builder, config| builder.add_source(config))
             .build()
             .unwrap();
-        let layered_configs = LayeredConfigs::from_environment(config);
-        let mut ui = Ui::with_config(&layered_configs.merge())
+        let stacked_config = config_from_environment(config);
+        let mut ui = Ui::with_config(&stacked_config.merge())
             .expect("default config should be valid, env vars are stringly typed");
-        let result = self.run_internal(&mut ui, layered_configs);
+        let result = self.run_internal(&mut ui, stacked_config);
         let exit_code = handle_command_result(&mut ui, result);
         ui.finalize_pager();
         exit_code
     }
 }
 
-fn map_clap_cli_error(
-    mut cmd_err: CommandError,
-    ui: &Ui,
-    layered_configs: &LayeredConfigs,
-) -> CommandError {
+fn map_clap_cli_error(mut cmd_err: CommandError, ui: &Ui, config: &StackedConfig) -> CommandError {
     let Some(err) = cmd_err.error.downcast_ref::<clap::Error>() else {
         return cmd_err;
     };
@@ -3685,7 +3683,7 @@ fn map_clap_cli_error(
     ) {
         if arg.as_str() == "--template <TEMPLATE>" && value.is_empty() {
             // Suppress the error, it's less important than the original error.
-            if let Ok(template_aliases) = load_template_aliases(ui, layered_configs) {
+            if let Ok(template_aliases) = load_template_aliases(ui, config) {
                 cmd_err.add_hint(format_template_aliases_hint(&template_aliases));
             }
         }
diff --git a/cli/src/commands/config/list.rs b/cli/src/commands/config/list.rs
index 7f76313885..a7de617df5 100644
--- a/cli/src/commands/config/list.rs
+++ b/cli/src/commands/config/list.rs
@@ -44,7 +44,7 @@ pub struct ConfigListArgs {
     pub include_overridden: bool,
     #[command(flatten)]
     pub level: ConfigLevelArgs,
-    // TODO(#1047): Support --show-origin using LayeredConfigs.
+    // TODO(#1047): Support --show-origin using StackedConfig.
     /// Render each variable using the given template
     ///
     /// The following keywords are defined:
@@ -79,7 +79,7 @@ pub fn cmd_config_list(
     let mut formatter = ui.stdout_formatter();
     let name_path = args.name.clone().unwrap_or_else(ConfigNamePathBuf::root);
     let mut wrote_values = false;
-    for annotated in resolved_config_values(command.layered_configs(), &name_path)? {
+    for annotated in resolved_config_values(command.stacked_config(), &name_path)? {
         // Remove overridden values.
         if annotated.is_overridden && !args.include_overridden {
             continue;
diff --git a/cli/src/complete.rs b/cli/src/complete.rs
index 02ec810d05..ef9a1f513e 100644
--- a/cli/src/complete.rs
+++ b/cli/src/complete.rs
@@ -26,9 +26,9 @@ use crate::cli_util::find_workspace_dir;
 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::ConfigEnv;
-use crate::config::LayeredConfigs;
 use crate::config::CONFIG_SCHEMA;
 use crate::ui::Ui;
 
@@ -397,19 +397,19 @@ fn get_jj_command() -> Result<(std::process::Command, Config), CommandError> {
         .add_source(default_config())
         .build()
         .expect("default config should be valid");
-    let mut layered_configs = LayeredConfigs::from_environment(config);
-    let ui = Ui::with_config(&layered_configs.merge()).expect("default config should be valid");
+    let mut stacked_config = config_from_environment(config);
+    let ui = Ui::with_config(&stacked_config.merge()).expect("default config should be valid");
     let cwd = std::env::current_dir()
         .and_then(|cwd| cwd.canonicalize())
         .map_err(user_error)?;
     let mut config_env = ConfigEnv::from_environment()?;
     let maybe_cwd_workspace_loader = DefaultWorkspaceLoaderFactory.create(find_workspace_dir(&cwd));
-    let _ = config_env.reload_user_config(&mut layered_configs);
+    let _ = config_env.reload_user_config(&mut stacked_config);
     if let Ok(loader) = &maybe_cwd_workspace_loader {
         config_env.reset_repo_path(loader.repo_path());
-        let _ = config_env.reload_repo_config(&mut layered_configs);
+        let _ = config_env.reload_repo_config(&mut stacked_config);
     }
-    let mut config = layered_configs.merge();
+    let mut config = stacked_config.merge();
     // skip 2 because of the clap_complete prelude: jj -- jj <actual args...>
     let args = std::env::args_os().skip(2);
     let args = expand_args(&ui, &app, args, &config)?;
@@ -425,8 +425,8 @@ fn get_jj_command() -> Result<(std::process::Command, Config), CommandError> {
         // Try to update repo-specific config on a best-effort basis.
         if let Ok(loader) = DefaultWorkspaceLoaderFactory.create(&cwd.join(&repository)) {
             config_env.reset_repo_path(loader.repo_path());
-            let _ = config_env.reload_repo_config(&mut layered_configs);
-            config = layered_configs.merge();
+            let _ = config_env.reload_repo_config(&mut stacked_config);
+            config = stacked_config.merge();
         }
         cmd_args.push("--repository".into());
         cmd_args.push(repository);
diff --git a/cli/src/config.rs b/cli/src/config.rs
index b08ed11bef..b489820bfe 100644
--- a/cli/src/config.rs
+++ b/cli/src/config.rs
@@ -101,58 +101,16 @@ pub struct AnnotatedValue {
     pub is_overridden: bool,
 }
 
-/// Set of configs which can be merged as needed.
-///
-/// Sources from the lowest precedence:
-/// 1. Default
-/// 2. Base environment variables
-/// 3. [User config](https://martinvonz.github.io/jj/latest/config/)
-/// 4. Repo config `.jj/repo/config.toml`
-/// 5. TODO: Workspace config `.jj/config.toml`
-/// 6. Override environment variables
-/// 7. Command-line arguments `--config-toml`
-#[derive(Clone, Debug)]
-pub struct LayeredConfigs {
-    inner: StackedConfig,
-}
-
-impl LayeredConfigs {
-    /// Initializes configs with infallible sources.
-    pub fn from_environment(default: config::Config) -> Self {
-        let inner = config_from_environment(default);
-        LayeredConfigs { inner }
-    }
-
-    pub fn parse_config_args(&mut self, toml_strs: &[String]) -> Result<(), ConfigEnvError> {
-        self.inner.remove_layers(ConfigSource::CommandArg);
-        let layer = parse_config_args(toml_strs)?;
-        self.inner.add_layer(layer);
-        Ok(())
-    }
-
-    /// Creates new merged config.
-    pub fn merge(&self) -> config::Config {
-        self.inner.merge()
-    }
-
-    pub fn sources(&self) -> impl DoubleEndedIterator<Item = (ConfigSource, &config::Config)> {
-        self.inner
-            .layers()
-            .iter()
-            .map(|layer| (layer.source, &layer.data))
-    }
-}
-
 /// Collects values under the given `filter_prefix` name recursively, from all
 /// layers.
 pub fn resolved_config_values(
-    layered_configs: &LayeredConfigs,
+    stacked_config: &StackedConfig,
     filter_prefix: &ConfigNamePathBuf,
 ) -> Result<Vec<AnnotatedValue>, ConfigError> {
     // Collect annotated values from each config.
     let mut config_vals = vec![];
-    for (source, config) in layered_configs.sources() {
-        let Some(top_value) = filter_prefix.lookup_value(config).optional()? else {
+    for layer in stacked_config.layers() {
+        let Some(top_value) = filter_prefix.lookup_value(&layer.data).optional()? else {
             continue;
         };
         let mut config_stack = vec![(filter_prefix.clone(), &top_value)];
@@ -170,7 +128,7 @@ pub fn resolved_config_values(
                     config_vals.push(AnnotatedValue {
                         name,
                         value: value.to_owned(),
-                        source,
+                        source: layer.source,
                         // Note: Value updated below.
                         is_overridden: false,
                     });
@@ -320,13 +278,13 @@ impl ConfigEnv {
     /// Loads user-specific config files into the given `config`. The old
     /// user-config layers will be replaced if any.
     #[instrument]
-    pub fn reload_user_config(&self, config: &mut LayeredConfigs) -> Result<(), ConfigError> {
-        config.inner.remove_layers(ConfigSource::User);
+    pub fn reload_user_config(&self, config: &mut StackedConfig) -> Result<(), ConfigError> {
+        config.remove_layers(ConfigSource::User);
         if let Some(path) = self.existing_user_config_path() {
             if path.is_dir() {
-                config.inner.load_dir(ConfigSource::User, path)?;
+                config.load_dir(ConfigSource::User, path)?;
             } else {
-                config.inner.load_file(ConfigSource::User, path)?;
+                config.load_file(ConfigSource::User, path)?;
             }
         }
         Ok(())
@@ -361,16 +319,27 @@ impl ConfigEnv {
     /// Loads repo-specific config file into the given `config`. The old
     /// repo-config layer will be replaced if any.
     #[instrument]
-    pub fn reload_repo_config(&self, config: &mut LayeredConfigs) -> Result<(), ConfigError> {
-        config.inner.remove_layers(ConfigSource::Repo);
+    pub fn reload_repo_config(&self, config: &mut StackedConfig) -> Result<(), ConfigError> {
+        config.remove_layers(ConfigSource::Repo);
         if let Some(path) = self.existing_repo_config_path() {
-            config.inner.load_file(ConfigSource::Repo, path)?;
+            config.load_file(ConfigSource::Repo, path)?;
         }
         Ok(())
     }
 }
 
 /// Initializes stacked config with the given `default` and infallible sources.
+///
+/// Sources from the lowest precedence:
+/// 1. Default
+/// 2. Base environment variables
+/// 3. [User config](https://martinvonz.github.io/jj/latest/config/)
+/// 4. Repo config `.jj/repo/config.toml`
+/// 5. TODO: Workspace config `.jj/config.toml`
+/// 6. Override environment variables
+/// 7. Command-line arguments `--config-toml`
+///
+/// This function sets up 1, 2, and 6.
 pub fn config_from_environment(default: config::Config) -> StackedConfig {
     let mut config = StackedConfig::empty();
     config.add_layer(ConfigLayer::with_data(ConfigSource::Default, default));
@@ -783,11 +752,9 @@ mod tests {
 
     #[test]
     fn test_resolved_config_values_empty() {
-        let layered_configs = LayeredConfigs {
-            inner: StackedConfig::empty(),
-        };
+        let config = StackedConfig::empty();
         assert_eq!(
-            resolved_config_values(&layered_configs, &ConfigNamePathBuf::root()).unwrap(),
+            resolved_config_values(&config, &ConfigNamePathBuf::root()).unwrap(),
             []
         );
     }
@@ -806,16 +773,15 @@ mod tests {
             .unwrap()
             .build()
             .unwrap();
-        let mut inner = StackedConfig::empty();
-        inner.add_layer(ConfigLayer::with_data(
+        let mut config = StackedConfig::empty();
+        config.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 };
+        config.add_layer(ConfigLayer::with_data(ConfigSource::Repo, repo_config));
         // Note: "email" is alphabetized, before "name" from same layer.
         insta::assert_debug_snapshot!(
-            resolved_config_values(&layered_configs, &ConfigNamePathBuf::root()).unwrap(),
+            resolved_config_values(&config, &ConfigNamePathBuf::root()).unwrap(),
             @r#"
         [
             AnnotatedValue {
@@ -951,16 +917,12 @@ mod tests {
             .unwrap()
             .build()
             .unwrap();
-        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 };
+        let mut config = StackedConfig::empty();
+        config.add_layer(ConfigLayer::with_data(ConfigSource::User, user_config));
+        config.add_layer(ConfigLayer::with_data(ConfigSource::Repo, repo_config));
         insta::assert_debug_snapshot!(
-            resolved_config_values(
-                &layered_configs,
-                &ConfigNamePathBuf::from_iter(["test-table1"]),
-            )
-            .unwrap(),
+            resolved_config_values(&config, &ConfigNamePathBuf::from_iter(["test-table1"]))
+                .unwrap(),
             @r#"
         [
             AnnotatedValue {
diff --git a/cli/src/revset_util.rs b/cli/src/revset_util.rs
index 480396a900..938e97d69c 100644
--- a/cli/src/revset_util.rs
+++ b/cli/src/revset_util.rs
@@ -22,6 +22,7 @@ use itertools::Itertools as _;
 use jj_lib::backend::CommitId;
 use jj_lib::commit::Commit;
 use jj_lib::config::ConfigSource;
+use jj_lib::config::StackedConfig;
 use jj_lib::id_prefix::IdPrefixContext;
 use jj_lib::repo::Repo;
 use jj_lib::revset;
@@ -44,7 +45,6 @@ use thiserror::Error;
 
 use crate::command_error::user_error;
 use crate::command_error::CommandError;
-use crate::config::LayeredConfigs;
 use crate::formatter::Formatter;
 use crate::templater::TemplateRenderer;
 use crate::ui::Ui;
@@ -164,20 +164,20 @@ fn warn_user_redefined_builtin(
 
 pub fn load_revset_aliases(
     ui: &Ui,
-    layered_configs: &LayeredConfigs,
+    stacked_config: &StackedConfig,
 ) -> Result<RevsetAliasesMap, CommandError> {
     const TABLE_KEY: &str = "revset-aliases";
     let mut aliases_map = RevsetAliasesMap::new();
     // Load from all config layers in order. 'f(x)' in default layer should be
     // overridden by 'f(a)' in user.
-    for (source, config) in layered_configs.sources() {
-        let table = if let Some(table) = config.get_table(TABLE_KEY).optional()? {
+    for layer in stacked_config.layers() {
+        let table = if let Some(table) = layer.data.get_table(TABLE_KEY).optional()? {
             table
         } else {
             continue;
         };
         for (decl, value) in table.into_iter().sorted_by(|a, b| a.0.cmp(&b.0)) {
-            warn_user_redefined_builtin(ui, source, &decl)?;
+            warn_user_redefined_builtin(ui, layer.source, &decl)?;
 
             let r = value
                 .into_string()