From d6f1ab697a82f9d39458ff06846c9d3f6f30d92f Mon Sep 17 00:00:00 2001 From: Yuya Nishihara Date: Tue, 23 May 2023 12:45:11 +0900 Subject: [PATCH] cli: load revset/template aliases in order of config layers Since we abuse TOML table syntax to define function aliases, an identical function alias can be found more than once in the merged config. The merged config doesn't preserve the definition order, so we need to load aliases table per layer. --- src/cli_util.rs | 58 +++++++++++++++++++++++-------------- src/config.rs | 2 +- tests/test_revset_output.rs | 30 +++++++++++++++++++ tests/test_templater.rs | 30 +++++++++++++++++++ 4 files changed, 98 insertions(+), 22 deletions(-) diff --git a/src/cli_util.rs b/src/cli_util.rs index 8e3a71888..e6e70cd5b 100644 --- a/src/cli_util.rs +++ b/src/cli_util.rs @@ -50,7 +50,7 @@ use jujutsu_lib::revset::{ RevsetIteratorExt, RevsetParseError, RevsetParseErrorKind, RevsetResolutionError, RevsetWorkspaceContext, }; -use jujutsu_lib::settings::UserSettings; +use jujutsu_lib::settings::{ConfigResultExt as _, UserSettings}; use jujutsu_lib::transaction::Transaction; use jujutsu_lib::tree::{Tree, TreeMergeError}; use jujutsu_lib::working_copy::{ @@ -579,8 +579,8 @@ impl WorkspaceCommandHelper { workspace: Workspace, repo: Arc, ) -> Result { - let revset_aliases_map = load_revset_aliases(ui, &command.settings)?; - let template_aliases_map = load_template_aliases(ui, &command.settings)?; + let revset_aliases_map = load_revset_aliases(ui, &command.layered_configs)?; + let template_aliases_map = load_template_aliases(ui, &command.layered_configs)?; // Parse commit_summary template early to report error before starting mutable // operation. // TODO: Parsed template can be cached if it doesn't capture repo @@ -1626,18 +1626,26 @@ fn resolve_single_op_from_store( fn load_revset_aliases( ui: &mut Ui, - settings: &UserSettings, + layered_configs: &LayeredConfigs, ) -> Result { const TABLE_KEY: &str = "revset-aliases"; let mut aliases_map = RevsetAliasesMap::new(); - let table = settings.config().get_table(TABLE_KEY)?; - for (decl, value) in table.into_iter().sorted_by(|a, b| a.0.cmp(&b.0)) { - let r = value - .into_string() - .map_err(|e| e.to_string()) - .and_then(|v| aliases_map.insert(&decl, v).map_err(|e| e.to_string())); - if let Err(s) = r { - writeln!(ui.warning(), r#"Failed to load "{TABLE_KEY}.{decl}": {s}"#)?; + // 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()? { + table + } else { + continue; + }; + for (decl, value) in table.into_iter().sorted_by(|a, b| a.0.cmp(&b.0)) { + let r = value + .into_string() + .map_err(|e| e.to_string()) + .and_then(|v| aliases_map.insert(&decl, v).map_err(|e| e.to_string())); + if let Err(s) = r { + writeln!(ui.warning(), r#"Failed to load "{TABLE_KEY}.{decl}": {s}"#)?; + } } } Ok(aliases_map) @@ -1733,18 +1741,26 @@ pub fn update_working_copy( fn load_template_aliases( ui: &mut Ui, - settings: &UserSettings, + layered_configs: &LayeredConfigs, ) -> Result { const TABLE_KEY: &str = "template-aliases"; let mut aliases_map = TemplateAliasesMap::new(); - let table = settings.config().get_table(TABLE_KEY)?; - for (decl, value) in table.into_iter().sorted_by(|a, b| a.0.cmp(&b.0)) { - let r = value - .into_string() - .map_err(|e| e.to_string()) - .and_then(|v| aliases_map.insert(&decl, v).map_err(|e| e.to_string())); - if let Err(s) = r { - writeln!(ui.warning(), r#"Failed to load "{TABLE_KEY}.{decl}": {s}"#)?; + // 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()? { + table + } else { + continue; + }; + for (decl, value) in table.into_iter().sorted_by(|a, b| a.0.cmp(&b.0)) { + let r = value + .into_string() + .map_err(|e| e.to_string()) + .and_then(|v| aliases_map.insert(&decl, v).map_err(|e| e.to_string())); + if let Err(s) = r { + writeln!(ui.warning(), r#"Failed to load "{TABLE_KEY}.{decl}": {s}"#)?; + } } } Ok(aliases_map) diff --git a/src/config.rs b/src/config.rs index 27d5b6c0a..148d1d18d 100644 --- a/src/config.rs +++ b/src/config.rs @@ -117,7 +117,7 @@ impl LayeredConfigs { .expect("loaded configs should be merged without error") } - fn sources(&self) -> Vec<(ConfigSource, &config::Config)> { + pub fn sources(&self) -> Vec<(ConfigSource, &config::Config)> { let config_sources = [ (ConfigSource::Default, Some(&self.default)), (ConfigSource::Env, Some(&self.env_base)), diff --git a/tests/test_revset_output.rs b/tests/test_revset_output.rs index 0da5fae02..a3e9d792b 100644 --- a/tests/test_revset_output.rs +++ b/tests/test_revset_output.rs @@ -337,6 +337,36 @@ fn test_alias() { "###); } +#[test] +fn test_alias_override() { + let test_env = TestEnvironment::default(); + test_env.jj_cmd_success(test_env.env_root(), &["init", "repo", "--git"]); + let repo_path = test_env.env_root().join("repo"); + + test_env.add_config( + r###" + [revset-aliases] + 'f(x)' = 'user' + "###, + ); + + // 'f(x)' should be overridden by --config-toml 'f(a)'. If aliases were sorted + // purely by name, 'f(a)' would come first. + let stderr = test_env.jj_cmd_failure( + &repo_path, + &[ + "log", + "-r", + "f(_)", + "--config-toml", + "revset-aliases.'f(a)' = 'arg'", + ], + ); + insta::assert_snapshot!(stderr, @r###" + Error: Revision "arg" doesn't exist + "###); +} + #[test] fn test_bad_alias_decl() { let test_env = TestEnvironment::default(); diff --git a/tests/test_templater.rs b/tests/test_templater.rs index fe76fad8c..93e155d25 100644 --- a/tests/test_templater.rs +++ b/tests/test_templater.rs @@ -808,6 +808,36 @@ fn test_templater_alias() { "###); } +#[test] +fn test_templater_alias_override() { + let test_env = TestEnvironment::default(); + test_env.jj_cmd_success(test_env.env_root(), &["init", "repo", "--git"]); + let repo_path = test_env.env_root().join("repo"); + + test_env.add_config( + r###" + [template-aliases] + 'f(x)' = '"user"' + "###, + ); + + // 'f(x)' should be overridden by --config-toml 'f(a)'. If aliases were sorted + // purely by name, 'f(a)' would come first. + let stdout = test_env.jj_cmd_success( + &repo_path, + &[ + "log", + "--no-graph", + "-r@", + "-T", + r#"f(_)"#, + "--config-toml", + r#"template-aliases.'f(a)' = '"arg"'"#, + ], + ); + insta::assert_snapshot!(stdout, @"arg"); +} + #[test] fn test_templater_bad_alias_decl() { let test_env = TestEnvironment::default();