From 6d149225a0544c8c04f2b46b77a1fdcf80cb31aa Mon Sep 17 00:00:00 2001 From: Yuya Nishihara Date: Tue, 17 Dec 2024 17:49:53 +0900 Subject: [PATCH] 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 | 28 ++++++++++++++++----- cli/src/commands/config/mod.rs | 2 +- cli/src/complete.rs | 12 +++++---- cli/src/config.rs | 46 +++++++++++++++++++++++----------- 4 files changed, 62 insertions(+), 26 deletions(-) diff --git a/cli/src/cli_util.rs b/cli/src/cli_util.rs index 794ac75b66..49c5f1ee30 100644 --- a/cli/src/cli_util.rs +++ b/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, commit_template_extensions: Vec>, @@ -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); diff --git a/cli/src/commands/config/mod.rs b/cli/src/commands/config/mod.rs index 2e98d9e363..b703c22dab 100644 --- a/cli/src/commands/config/mod.rs +++ b/cli/src/commands/config/mod.rs @@ -83,7 +83,7 @@ impl ConfigLevelArgs { fn edit_config_file(&self, command: &CommandHelper) -> Result { let config_env = command.config_env(); - let config = command.settings().config(); + let config = command.raw_config(); let pick_one = |mut files: Vec, not_found_error: &str| { if files.len() > 1 { // TODO: prompt or pick the last? diff --git a/cli/src/complete.rs b/cli/src/complete.rs index ecd7393e26..e92578b9a4 100644 --- a/cli/src/complete.rs +++ b/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 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); diff --git a/cli/src/config.rs b/cli/src/config.rs index 0bf738f00f..1a83e3ebab 100644 --- a/cli/src/config.rs +++ b/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 for RawConfig { + fn as_ref(&self) -> &StackedConfig { + &self.0 + } +} + +impl AsMut 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, 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, 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, ConfigLoadError>, ) -> Result, 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, -) -> StackedConfig { +pub fn config_from_environment(default_layers: impl IntoIterator) -> 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