From 82b0e88a21008bf2c7e0ee23041c0da8bc9da16a Mon Sep 17 00:00:00 2001 From: Yuya Nishihara Date: Wed, 22 May 2024 15:17:50 +0900 Subject: [PATCH] config: add workaround for config path expression parsing As of config 0.13.4, the path Expression type is private, and there's no escape syntax. This patch adds a fallback to nested HashMap lookup. https://github.com/mehcode/config-rs/blob/v0.13.4/src/path/mod.rs#L10 https://github.com/mehcode/config-rs/blob/v0.13.4/src/path/parser.rs Fixes #1723 --- cli/src/config.rs | 99 +++++++++++++++++++++++++++----- cli/tests/test_config_command.rs | 75 ++++++++++++++++++++++++ 2 files changed, 159 insertions(+), 15 deletions(-) diff --git a/cli/src/config.rs b/cli/src/config.rs index 3d34fdd84f..87986a3ad1 100644 --- a/cli/src/config.rs +++ b/cli/src/config.rs @@ -58,6 +58,51 @@ impl ConfigNamePathBuf { pub fn push(&mut self, key: impl Into) { self.0.push(key.into()); } + + /// Looks up value in the given `config`. + /// + /// This is a workaround for the `config.get()` API, which doesn't support + /// literal path expression. If we implement our own config abstraction, + /// this method should be moved there. + pub fn lookup_value( + &self, + config: &config::Config, + ) -> Result { + // Use config.get() if the TOML keys can be converted to config path + // syntax. This should be cheaper than cloning the whole config map. + let (key_prefix, components) = self.split_safe_prefix(); + let value: config::Value = match &key_prefix { + Some(key) => config.get(key)?, + None => config.collect()?.into(), + }; + components + .iter() + .try_fold(value, |value, key| { + let mut table = value.into_table().ok()?; + table.remove(key.get()) + }) + .ok_or_else(|| config::ConfigError::NotFound(self.to_string())) + } + + /// Splits path to dotted literal expression and remainder. + /// + /// The literal expression part doesn't contain meta characters other than + /// ".", therefore it can be passed in to `config.get()`. + /// https://github.com/mehcode/config-rs/issues/110 + fn split_safe_prefix(&self) -> (Option>, &[toml_edit::Key]) { + // https://github.com/mehcode/config-rs/blob/v0.13.4/src/path/parser.rs#L15 + let is_ident = |key: &str| { + key.chars() + .all(|c| c.is_ascii_alphanumeric() || c == '_' || c == '-') + }; + let pos = self.0.iter().take_while(|&k| is_ident(k)).count(); + let safe_key = match pos { + 0 => None, + 1 => Some(Cow::Borrowed(self.0[0].get())), + _ => Some(Cow::Owned(self.0[..pos].iter().join("."))), + }; + (safe_key, &self.0[pos..]) + } } impl> FromIterator for ConfigNamePathBuf { @@ -201,22 +246,9 @@ impl LayeredConfigs { ) -> Result, ConfigError> { // Collect annotated values from each config. let mut config_vals = vec![]; - - // TODO: don't reinterpret TOML path as config path expression - let prefix_key = match filter_prefix.as_ref() { - &[] => None, - _ => Some(filter_prefix.to_string()), - }; for (source, config) in self.sources() { - let top_value: config::Value = match prefix_key { - Some(ref key) => { - if let Some(val) = config.get(key).optional()? { - val - } else { - continue; - } - } - None => config.collect()?.into(), + let Some(top_value) = filter_prefix.lookup_value(config).optional()? else { + continue; }; let mut config_stack = vec![(filter_prefix.clone(), &top_value)]; while let Some((path, value)) = config_stack.pop() { @@ -655,6 +687,43 @@ mod tests { use super::*; + #[test] + fn test_split_safe_config_name_path() { + let parse = |s| ConfigNamePathBuf::from_str(s).unwrap(); + let key = |s: &str| toml_edit::Key::new(s); + + // Empty (or root) path isn't recognized by config::Config::get() + assert_eq!( + ConfigNamePathBuf::root().split_safe_prefix(), + (None, [].as_slice()) + ); + + assert_eq!( + parse("Foo-bar_1").split_safe_prefix(), + (Some("Foo-bar_1".into()), [].as_slice()) + ); + assert_eq!( + parse("'foo()'").split_safe_prefix(), + (None, [key("foo()")].as_slice()) + ); + assert_eq!( + parse("foo.'bar()'").split_safe_prefix(), + (Some("foo".into()), [key("bar()")].as_slice()) + ); + assert_eq!( + parse("foo.'bar()'.baz").split_safe_prefix(), + (Some("foo".into()), [key("bar()"), key("baz")].as_slice()) + ); + assert_eq!( + parse("foo.bar").split_safe_prefix(), + (Some("foo.bar".into()), [].as_slice()) + ); + assert_eq!( + parse("foo.bar.'baz()'").split_safe_prefix(), + (Some("foo.bar".into()), [key("baz()")].as_slice()) + ); + } + #[test] fn test_command_args() { let config = config::Config::builder() diff --git a/cli/tests/test_config_command.rs b/cli/tests/test_config_command.rs index 690003b566..6eb31bef67 100644 --- a/cli/tests/test_config_command.rs +++ b/cli/tests/test_config_command.rs @@ -713,7 +713,82 @@ fn test_config_get() { #[test] fn test_config_path_syntax() { let test_env = TestEnvironment::default(); + test_env.add_config( + r#" + a.'b()' = 0 + 'b c'.d = 1 + 'b c'.e.'f[]' = 2 + - = 3 + _ = 4 + '.' = 5 + "#, + ); + + let stdout = test_env.jj_cmd_success(test_env.env_root(), &["config", "list", "a.'b()'"]); + insta::assert_snapshot!(stdout, @r###" + a.'b()'=0 + "###); + let stdout = test_env.jj_cmd_success(test_env.env_root(), &["config", "list", "'b c'"]); + insta::assert_snapshot!(stdout, @r###" + 'b c'.d=1 + 'b c'.e."f[]"=2 + "###); + let stdout = test_env.jj_cmd_success(test_env.env_root(), &["config", "list", "'b c'.d"]); + insta::assert_snapshot!(stdout, @r###" + 'b c'.d=1 + "###); + let stdout = test_env.jj_cmd_success(test_env.env_root(), &["config", "list", "'b c'.e.'f[]'"]); + insta::assert_snapshot!(stdout, @r###" + 'b c'.e.'f[]'=2 + "###); + + // Not a table + let (stdout, stderr) = + test_env.jj_cmd_ok(test_env.env_root(), &["config", "list", "a.'b()'.x"]); + insta::assert_snapshot!(stdout, @""); + insta::assert_snapshot!(stderr, @r###" + Warning: No matching config key for a.'b()'.x + "###); + + // "-" and "_" are valid TOML keys + let stdout = test_env.jj_cmd_success(test_env.env_root(), &["config", "list", "-"]); + insta::assert_snapshot!(stdout, @r###" + -=3 + "###); + let stdout = test_env.jj_cmd_success(test_env.env_root(), &["config", "list", "_"]); + insta::assert_snapshot!(stdout, @r###" + _=4 + "###); + + // "." requires quoting + let stdout = test_env.jj_cmd_success(test_env.env_root(), &["config", "list", "'.'"]); + insta::assert_snapshot!(stdout, @r###" + '.'=5 + "###); + let stderr = test_env.jj_cmd_cli_error(test_env.env_root(), &["config", "list", "."]); + insta::assert_snapshot!(stderr, @r###" + error: invalid value '.' for '[NAME]': TOML parse error at line 1, column 1 + | + 1 | . + | ^ + invalid key + + For more information, try '--help'. + "###); + + // Invalid TOML keys + let stderr = test_env.jj_cmd_cli_error(test_env.env_root(), &["config", "list", "b c"]); + insta::assert_snapshot!(stderr, @r###" + error: invalid value 'b c' for '[NAME]': TOML parse error at line 1, column 3 + | + 1 | b c + | ^ + + + + For more information, try '--help'. + "###); let stderr = test_env.jj_cmd_cli_error(test_env.env_root(), &["config", "list", ""]); insta::assert_snapshot!(stderr, @r###" error: invalid value '' for '[NAME]': TOML parse error at line 1, column 1