From 4af39e203836b1601f70bd75b7552f5287247583 Mon Sep 17 00:00:00 2001 From: Yuya Nishihara Date: Tue, 24 Dec 2024 15:08:54 +0900 Subject: [PATCH] cli: config list: mark values overridden by table or parent value This fixes the output of "jj config list --include-overridden --include-defaults ui.pager" and ".. ui.pager.command". The default ui.pager.* should be marked as overridden by ui.pager if set in user configuration. --- cli/src/config.rs | 162 +++++++++++++++++++++++++++++++++++++++------- lib/src/config.rs | 5 ++ 2 files changed, 142 insertions(+), 25 deletions(-) diff --git a/cli/src/config.rs b/cli/src/config.rs index 8273304c79..729c28435f 100644 --- a/cli/src/config.rs +++ b/cli/src/config.rs @@ -13,8 +13,8 @@ // limitations under the License. use std::borrow::Cow; +use std::collections::BTreeSet; use std::collections::HashMap; -use std::collections::HashSet; use std::env; use std::fmt; use std::path::Path; @@ -103,28 +103,40 @@ pub fn resolved_config_values( stacked_config: &StackedConfig, filter_prefix: &ConfigNamePathBuf, ) -> Vec { - // Collect annotated values from each config. + // Collect annotated values in reverse order and mark each value shadowed by + // value or table in upper layers. let mut config_vals = vec![]; - for layer in stacked_config.layers() { - // TODO: Err(item) means all descendant paths are overridden by the - // current layer. For example, the default ui.pager. should be - // marked as overridden if user had ui.pager = [...] set. - let Ok(Some(top_item)) = layer.look_up_item(filter_prefix) else { - continue; + let mut upper_value_names = BTreeSet::new(); + for layer in stacked_config.layers().iter().rev() { + let top_item = match layer.look_up_item(filter_prefix) { + Ok(Some(item)) => item, + Ok(None) => continue, // parent is a table, but no value found + Err(_) => { + // parent is not a table, shadows lower layers + upper_value_names.insert(filter_prefix.clone()); + continue; + } }; - let mut config_stack = vec![(filter_prefix.clone(), top_item)]; - while let Some((name, item)) = config_stack.pop() { + let mut config_stack = vec![(filter_prefix.clone(), top_item, false)]; + while let Some((name, item, is_parent_overridden)) = config_stack.pop() { if let Some(table) = item.as_table() { - // table.iter() does not implement DoubleEndedIterator as of - // toml_edit 0.22.22. - let frame = config_stack.len(); + // current table and children may be shadowed by value in upper layer + let is_overridden = is_parent_overridden || upper_value_names.contains(&name); for (k, v) in table { let mut sub_name = name.clone(); sub_name.push(k); - config_stack.push((sub_name, v)); + config_stack.push((sub_name, v, is_overridden)); // in reverse order } - config_stack[frame..].reverse(); } else { + // current value may be shadowed by value or table in upper layer + let maybe_child = upper_value_names + .range(&name..) + .next() + .filter(|next| next.starts_with(&name)); + let is_overridden = is_parent_overridden || maybe_child.is_some(); + if maybe_child != Some(&name) { + upper_value_names.insert(name.clone()); + } let value = item .clone() .into_value() @@ -133,20 +145,12 @@ pub fn resolved_config_values( name, value, source: layer.source, - // Note: Value updated below. - is_overridden: false, + is_overridden, }); } } } - - // Walk through config values in reverse order and mark each overridden value as - // overridden. - let mut names_found = HashSet::new(); - for val in config_vals.iter_mut().rev() { - val.is_overridden = !names_found.insert(&val.name); - } - + config_vals.reverse(); config_vals } @@ -702,6 +706,8 @@ impl TryFrom> for NonEmptyCommandArgsVec { #[cfg(test)] mod tests { + use std::fmt::Write as _; + use anyhow::anyhow; use assert_matches::assert_matches; use indoc::indoc; @@ -1040,6 +1046,112 @@ mod tests { ); } + #[test] + fn test_resolved_config_values_overridden() { + let list = |layers: &[&ConfigLayer], prefix: &str| -> String { + let mut config = StackedConfig::empty(); + config.extend_layers(layers.iter().copied().cloned()); + let prefix = if prefix.is_empty() { + ConfigNamePathBuf::root() + } else { + prefix.parse().unwrap() + }; + let mut output = String::new(); + for annotated in resolved_config_values(&config, &prefix) { + let AnnotatedValue { name, value, .. } = &annotated; + let sigil = if annotated.is_overridden { '!' } else { ' ' }; + writeln!(output, "{sigil}{name} = {value}").unwrap(); + } + output + }; + + let mut layer0 = ConfigLayer::empty(ConfigSource::User); + layer0.set_value("a.b.e", "0.0").unwrap(); + layer0.set_value("a.b.c.f", "0.1").unwrap(); + layer0.set_value("a.b.d", "0.2").unwrap(); + let mut layer1 = ConfigLayer::empty(ConfigSource::User); + layer1.set_value("a.b", "1.0").unwrap(); + layer1.set_value("a.c", "1.1").unwrap(); + let mut layer2 = ConfigLayer::empty(ConfigSource::User); + layer2.set_value("a.b.g", "2.0").unwrap(); + layer2.set_value("a.b.d", "2.1").unwrap(); + + // a.b.* is shadowed by a.b + let layers = [&layer0, &layer1]; + insta::assert_snapshot!(list(&layers, ""), @r#" + !a.b.e = "0.0" + !a.b.c.f = "0.1" + !a.b.d = "0.2" + a.b = "1.0" + a.c = "1.1" + "#); + insta::assert_snapshot!(list(&layers, "a.b"), @r#" + !a.b.e = "0.0" + !a.b.c.f = "0.1" + !a.b.d = "0.2" + a.b = "1.0" + "#); + insta::assert_snapshot!(list(&layers, "a.b.c"), @r#"!a.b.c.f = "0.1""#); + insta::assert_snapshot!(list(&layers, "a.b.d"), @r#"!a.b.d = "0.2""#); + + // a.b is shadowed by a.b.* + let layers = [&layer1, &layer2]; + insta::assert_snapshot!(list(&layers, ""), @r#" + !a.b = "1.0" + a.c = "1.1" + a.b.g = "2.0" + a.b.d = "2.1" + "#); + insta::assert_snapshot!(list(&layers, "a.b"), @r#" + !a.b = "1.0" + a.b.g = "2.0" + a.b.d = "2.1" + "#); + + // a.b.d is shadowed by a.b.d + let layers = [&layer0, &layer2]; + insta::assert_snapshot!(list(&layers, ""), @r#" + a.b.e = "0.0" + a.b.c.f = "0.1" + !a.b.d = "0.2" + a.b.g = "2.0" + a.b.d = "2.1" + "#); + insta::assert_snapshot!(list(&layers, "a.b"), @r#" + a.b.e = "0.0" + a.b.c.f = "0.1" + !a.b.d = "0.2" + a.b.g = "2.0" + a.b.d = "2.1" + "#); + insta::assert_snapshot!(list(&layers, "a.b.c"), @r#" a.b.c.f = "0.1""#); + insta::assert_snapshot!(list(&layers, "a.b.d"), @r#" + !a.b.d = "0.2" + a.b.d = "2.1" + "#); + + // a.b.* is shadowed by a.b, which is shadowed by a.b.* + let layers = [&layer0, &layer1, &layer2]; + insta::assert_snapshot!(list(&layers, ""), @r#" + !a.b.e = "0.0" + !a.b.c.f = "0.1" + !a.b.d = "0.2" + !a.b = "1.0" + a.c = "1.1" + a.b.g = "2.0" + a.b.d = "2.1" + "#); + insta::assert_snapshot!(list(&layers, "a.b"), @r#" + !a.b.e = "0.0" + !a.b.c.f = "0.1" + !a.b.d = "0.2" + !a.b = "1.0" + a.b.g = "2.0" + a.b.d = "2.1" + "#); + insta::assert_snapshot!(list(&layers, "a.b.c"), @r#"!a.b.c.f = "0.1""#); + } + #[test] fn test_config_path_home_dir_existing() -> anyhow::Result<()> { TestCase { diff --git a/lib/src/config.rs b/lib/src/config.rs index 84a9c8e683..01c29b8254 100644 --- a/lib/src/config.rs +++ b/lib/src/config.rs @@ -146,6 +146,11 @@ impl ConfigNamePathBuf { self.0.is_empty() } + /// Returns true if the `base` is a prefix of this path. + pub fn starts_with(&self, base: impl AsRef<[toml_edit::Key]>) -> bool { + self.0.starts_with(base.as_ref()) + } + /// Returns iterator of path components (or keys.) pub fn components(&self) -> slice::Iter<'_, toml_edit::Key> { self.0.iter()