cli: keep separate copy of "raw" StackedConfig loaded from files

The next patch will introduce the resolution stage of conditional tables. Since
it wasn't easy to detect misuse of "raw" StackedConfig, I added a thin newtype.

cli/src/cli_util.rs

@@ -159,6 +159,7 @@ use crate::config::parse_config_args;
 use crate::config::CommandNameAndArgs;
 use crate::config::ConfigArgKind;
 use crate::config::ConfigEnv;
+use crate::config::RawConfig;
 use crate::diff_util;
 use crate::diff_util::DiffFormat;
 use crate::diff_util::DiffFormatArgs;
@@ -291,6 +292,7 @@ struct CommandHelperData {
     matches: ArgMatches,
     global_args: GlobalArgs,
     config_env: ConfigEnv,
+    raw_config: RawConfig,
     settings: UserSettings,
     revset_extensions: Arc<RevsetExtensions>,
     commit_template_extensions: Vec<Arc<dyn CommitTemplateLanguageExtension>>,
@@ -329,6 +331,14 @@ impl CommandHelper {
         &self.data.config_env
     }
 
+    /// Unprocessed (or unresolved) configuration data.
+    ///
+    /// Use this only if the unmodified config data is needed. For example, `jj
+    /// config set` should use this to write updated data back to file.
+    pub fn raw_config(&self) -> &RawConfig {
+        &self.data.raw_config
+    }
+
     pub fn settings(&self) -> &UserSettings {
         &self.data.settings
     }
@@ -3616,7 +3626,7 @@ impl CliRunner {
     }
 
     #[instrument(skip_all)]
-    fn run_internal(self, ui: &mut Ui, mut config: StackedConfig) -> Result<(), CommandError> {
+    fn run_internal(self, ui: &mut Ui, mut raw_config: RawConfig) -> Result<(), CommandError> {
         // `cwd` is canonicalized for consistency with `Workspace::workspace_root()` and
         // to easily compute relative paths between them.
         let cwd = env::current_dir()
@@ -3635,11 +3645,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 config)?;
+        config_env.reload_user_config(&mut raw_config)?;
         if let Ok(loader) = &maybe_cwd_workspace_loader {
             config_env.reset_repo_path(loader.repo_path());
-            config_env.reload_repo_config(&mut config)?;
+            config_env.reload_repo_config(&mut raw_config)?;
         }
+        let mut config = raw_config.as_ref().clone(); // TODO
         ui.reset(&config)?;
 
         if env::var_os("COMPLETE").is_some() {
@@ -3649,7 +3660,8 @@ impl CliRunner {
         let string_args = expand_args(ui, &self.app, env::args_os(), &config)?;
         let (args, config_layers) = parse_early_args(&self.app, &string_args)?;
         if !config_layers.is_empty() {
-            config.extend_layers(config_layers);
+            raw_config.as_mut().extend_layers(config_layers);
+            config = raw_config.as_ref().clone(); // TODO
             ui.reset(&config)?;
         }
         if !args.config_toml.is_empty() {
@@ -3674,7 +3686,8 @@ impl CliRunner {
                 .create(&abs_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 config)?;
+            config_env.reload_repo_config(&mut raw_config)?;
+            config = raw_config.as_ref().clone(); // TODO
             Ok(loader)
         } else {
             maybe_cwd_workspace_loader
@@ -3703,6 +3716,7 @@ impl CliRunner {
             matches,
             global_args: args.global_args,
             config_env,
+            raw_config,
             settings,
             revset_extensions: self.revset_extensions.into(),
             commit_template_extensions: self.commit_template_extensions,
@@ -3726,7 +3740,9 @@ impl CliRunner {
         // Tell crossterm to ignore NO_COLOR (we check it ourselves)
         crossterm::style::force_color_output(true);
         let config = config_from_environment(self.config_layers.drain(..));
-        let mut ui = Ui::with_config(&config)
+        // Set up ui assuming the default config has no conditional variables.
+        // If it had, the configuration will be fixed by the next ui.reset().
+        let mut ui = Ui::with_config(config.as_ref())
             .expect("default config should be valid, env vars are stringly typed");
         let result = self.run_internal(&mut ui, config);
         let exit_code = handle_command_result(&mut ui, result);

cli/src/commands/config/mod.rs

@@ -83,7 +83,7 @@ impl ConfigLevelArgs {
 
     fn edit_config_file(&self, command: &CommandHelper) -> Result<ConfigFile, CommandError> {
         let config_env = command.config_env();
-        let config = command.settings().config();
+        let config = command.raw_config();
         let pick_one = |mut files: Vec<ConfigFile>, not_found_error: &str| {
             if files.len() > 1 {
                 // TODO: prompt or pick the last?

cli/src/complete.rs

@@ -681,18 +681,19 @@ 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_layers());
-    let ui = Ui::with_config(&config).expect("default config should be valid");
+    let mut raw_config = config_from_environment(default_config_layers());
+    let ui = Ui::with_config(raw_config.as_ref()).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 config);
+    let _ = config_env.reload_user_config(&mut raw_config);
     if let Ok(loader) = &maybe_cwd_workspace_loader {
         config_env.reset_repo_path(loader.repo_path());
-        let _ = config_env.reload_repo_config(&mut config);
+        let _ = config_env.reload_repo_config(&mut raw_config);
     }
+    let mut config = raw_config.as_ref().clone(); /* TODO */
     // 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)?;
@@ -708,7 +709,8 @@ fn get_jj_command() -> Result<(JjBuilder, UserSettings), 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 config);
+            let _ = config_env.reload_repo_config(&mut raw_config);
+            config = raw_config.as_ref().clone(); // TODO
         }
         cmd_args.push("--repository".into());
         cmd_args.push(repository);

cli/src/config.rs

@@ -148,6 +148,25 @@ pub fn resolved_config_values(
     config_vals
 }
 
+/// Newtype for unprocessed (or unresolved) [`StackedConfig`].
+///
+/// This doesn't provide any strict guarantee about the underlying config
+/// object. It just requires an explicit cast to access to the config object.
+#[derive(Clone, Debug)]
+pub struct RawConfig(StackedConfig);
+
+impl AsRef<StackedConfig> for RawConfig {
+    fn as_ref(&self) -> &StackedConfig {
+        &self.0
+    }
+}
+
+impl AsMut<StackedConfig> for RawConfig {
+    fn as_mut(&mut self) -> &mut StackedConfig {
+        &mut self.0
+    }
+}
+
 #[derive(Clone, Debug)]
 enum ConfigPath {
     /// Existing config file path.
@@ -267,7 +286,7 @@ impl ConfigEnv {
     /// empty `Vec`.
     pub fn user_config_files(
         &self,
-        config: &StackedConfig,
+        config: &RawConfig,
     ) -> Result<Vec<ConfigFile>, ConfigLoadError> {
         config_files_for(config, ConfigSource::User, || self.new_user_config_file())
     }
@@ -290,13 +309,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 StackedConfig) -> Result<(), ConfigLoadError> {
-        config.remove_layers(ConfigSource::User);
+    pub fn reload_user_config(&self, config: &mut RawConfig) -> Result<(), ConfigLoadError> {
+        config.as_mut().remove_layers(ConfigSource::User);
         if let Some(path) = self.existing_user_config_path() {
             if path.is_dir() {
-                config.load_dir(ConfigSource::User, path)?;
+                config.as_mut().load_dir(ConfigSource::User, path)?;
             } else {
-                config.load_file(ConfigSource::User, path)?;
+                config.as_mut().load_file(ConfigSource::User, path)?;
             }
         }
         Ok(())
@@ -329,7 +348,7 @@ impl ConfigEnv {
     /// have at most one config file.
     pub fn repo_config_files(
         &self,
-        config: &StackedConfig,
+        config: &RawConfig,
     ) -> Result<Vec<ConfigFile>, ConfigLoadError> {
         config_files_for(config, ConfigSource::Repo, || self.new_repo_config_file())
     }
@@ -345,21 +364,22 @@ 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 StackedConfig) -> Result<(), ConfigLoadError> {
-        config.remove_layers(ConfigSource::Repo);
+    pub fn reload_repo_config(&self, config: &mut RawConfig) -> Result<(), ConfigLoadError> {
+        config.as_mut().remove_layers(ConfigSource::Repo);
         if let Some(path) = self.existing_repo_config_path() {
-            config.load_file(ConfigSource::Repo, path)?;
+            config.as_mut().load_file(ConfigSource::Repo, path)?;
         }
         Ok(())
     }
 }
 
 fn config_files_for(
-    config: &StackedConfig,
+    config: &RawConfig,
     source: ConfigSource,
     new_file: impl FnOnce() -> Result<Option<ConfigFile>, ConfigLoadError>,
 ) -> Result<Vec<ConfigFile>, ConfigLoadError> {
     let mut files = config
+        .as_ref()
         .layers_for(source)
         .iter()
         .filter_map(|layer| ConfigFile::from_layer(layer.clone()).ok())
@@ -383,14 +403,12 @@ fn config_files_for(
 /// 7. Command-line arguments `--config`, `--config-toml`, `--config-file`
 ///
 /// This function sets up 1, 2, and 6.
-pub fn config_from_environment(
-    default_layers: impl IntoIterator<Item = ConfigLayer>,
-) -> StackedConfig {
+pub fn config_from_environment(default_layers: impl IntoIterator<Item = ConfigLayer>) -> RawConfig {
     let mut config = StackedConfig::empty();
     config.extend_layers(default_layers);
     config.add_layer(env_base_layer());
     config.add_layer(env_overrides_layer());
-    config
+    RawConfig(config)
 }
 
 /// Environment variables that should be overridden by config values