From 0a4e8dae13a3955166e4c13287b01f7ec589bf38 Mon Sep 17 00:00:00 2001 From: Yuya Nishihara Date: Fri, 29 Nov 2024 14:44:28 +0900 Subject: [PATCH] config: merge and print inline tables as values Before, "jj config get"/"list" and .get() functions processed inline tables as tables (or directories in filesystem analogy), whereas "set"/"unset" processed ones as values (or files.) This patch makes all commands and functions process inline tables as values. We rarely use the inline table syntax, and it's very hard to pack many (unrelated) values into an inline table. TOML doesn't allow newlines between { .. }. Our common use case is to define color styles, which wouldn't be meant to inherit attributes from the default settings. The default pager setting is flattened in case user overrides pager.env without changing the command args. --- CHANGELOG.md | 6 ++ cli/src/cli_util.rs | 2 +- cli/src/config.rs | 5 +- cli/src/config/misc.toml | 4 +- cli/src/revset_util.rs | 2 +- cli/tests/test_config_command.rs | 21 +++++- docs/config.md | 20 ++++-- lib/src/config.rs | 118 ++++++++++++++++++++++--------- 8 files changed, 133 insertions(+), 45 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7178c6c750..aa7cd25efa 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,6 +18,12 @@ to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). * The following configuration variables are now parsed strictly: `colors.`, `ui.color`, `ui.progress-indicator`, `ui.quiet` +* `jj config list` now prints inline tables `{ key = value, .. }` literally. + Inner items of inline tables are no longer merged across configuration files. + See [the table syntax + documentation](docs/config.md#dotted-style-headings-and-inline-tables) for + details. + * The deprecated `[alias]` config section is no longer respected. Move command aliases to the `[aliases]` section. diff --git a/cli/src/cli_util.rs b/cli/src/cli_util.rs index a43c4912c9..a202c681cd 100644 --- a/cli/src/cli_util.rs +++ b/cli/src/cli_util.rs @@ -2760,7 +2760,7 @@ fn load_template_aliases( .into()); } }; - for (decl, item) in table.iter() { + for (decl, item) in table { let r = item .as_str() .ok_or_else(|| format!("Expected a string, but is {}", item.type_name())) diff --git a/cli/src/config.rs b/cli/src/config.rs index fd25038c50..9ca8da9862 100644 --- a/cli/src/config.rs +++ b/cli/src/config.rs @@ -90,12 +90,11 @@ pub fn resolved_config_values( }; let mut config_stack = vec![(filter_prefix.clone(), top_item)]; while let Some((name, item)) = config_stack.pop() { - // TODO: item.as_table() to print inline table as a value? - if let Some(table) = item.as_table_like() { + 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(); - for (k, v) in table.iter() { + for (k, v) in table { let mut sub_name = name.clone(); sub_name.push(k); config_stack.push((sub_name, v)); diff --git a/cli/src/config/misc.toml b/cli/src/config/misc.toml index a315ebfb2d..ee3f0d19f8 100644 --- a/cli/src/config/misc.toml +++ b/cli/src/config/misc.toml @@ -20,8 +20,10 @@ always-allow-large-revsets = false color = "auto" diff-instructions = true graph.style = "curved" +# let user override pager.env independently +pager.command = ["less", "-FRX"] +pager.env = { LESSCHARSET = "utf-8" } paginate = "auto" -pager = { command = ["less", "-FRX"], env = { LESSCHARSET = "utf-8" } } progress-indicator = true quiet = false log-word-wrap = false diff --git a/cli/src/revset_util.rs b/cli/src/revset_util.rs index 7803e55531..53ed8a4a0d 100644 --- a/cli/src/revset_util.rs +++ b/cli/src/revset_util.rs @@ -184,7 +184,7 @@ pub fn load_revset_aliases( .into()); } }; - for (decl, item) in table.iter() { + for (decl, item) in table { warn_user_redefined_builtin(ui, layer.source, decl)?; let r = item diff --git a/cli/tests/test_config_command.rs b/cli/tests/test_config_command.rs index 0284b0e5eb..2c1b9f3c7d 100644 --- a/cli/tests/test_config_command.rs +++ b/cli/tests/test_config_command.rs @@ -84,6 +84,23 @@ fn test_config_list_table() { "###); } +#[test] +fn test_config_list_inline_table() { + let test_env = TestEnvironment::default(); + test_env.add_config( + r#" + test-table = { x = true, y = 1 } + "#, + ); + let stdout = test_env.jj_cmd_success(test_env.env_root(), &["config", "list", "test-table"]); + insta::assert_snapshot!(stdout, @"test-table = { x = true, y = 1 }"); + // Inner value cannot be addressed by a dotted name path + let (stdout, stderr) = + test_env.jj_cmd_ok(test_env.env_root(), &["config", "list", "test-table.x"]); + insta::assert_snapshot!(stdout, @""); + insta::assert_snapshot!(stderr, @"Warning: No matching config key for test-table.x"); +} + #[test] fn test_config_list_array() { let test_env = TestEnvironment::default(); @@ -99,7 +116,7 @@ fn test_config_list_array() { } #[test] -fn test_config_list_inline_table() { +fn test_config_list_array_of_tables() { let test_env = TestEnvironment::default(); test_env.add_config( r#" @@ -110,6 +127,8 @@ fn test_config_list_inline_table() { z."key=with whitespace" = [] "#, ); + // TODO: Perhaps, each value should be listed separately, but there's no + // path notation like "test-table[0].x". let stdout = test_env.jj_cmd_success(test_env.env_root(), &["config", "list", "test-table"]); insta::assert_snapshot!(stdout, @r###" test-table = [{ x = 1 }, { y = ["z"], z = { "key=with whitespace" = [] } }] diff --git a/docs/config.md b/docs/config.md index 78486922fd..3004eb8e93 100644 --- a/docs/config.md +++ b/docs/config.md @@ -20,9 +20,8 @@ located in `.jj/repo/config.toml`. - Settings [specified in the command-line](#specifying-config-on-the-command-line). These are listed in the order they are loaded; the settings from earlier items -in -the list are overridden by the settings from later items if they disagree. Every -type of config except for the built-in settings is optional. +in the list are overridden by the settings from later items if they disagree. +Every type of config except for the built-in settings is optional. See the [TOML site] and the [syntax guide] for a detailed description of the syntax. We cover some of the basics below. @@ -34,7 +33,8 @@ syntax. We cover some of the basics below. The first thing to remember is that the value of a setting (the part to the right of the `=` sign) should be surrounded in quotes if it's a string. -### Dotted style and headings +### Dotted style, headings, and inline tables + In TOML, anything under a heading can be dotted instead. For example, `user.name = "YOUR NAME"` is equivalent to: @@ -48,7 +48,7 @@ For future reference, here are a couple of more complicated examples, ```toml # Dotted style template-aliases."format_short_id(id)" = "id.shortest(12)" -colors."commit_id prefix".bold = true +colors."commit_id prefix" = { bold = true } # is equivalent to: [template-aliases] @@ -58,6 +58,14 @@ colors."commit_id prefix".bold = true "commit_id prefix" = { bold = true } ``` +Dotted and non-inline table items are merged one by one if the same keys exist +in multiple files. In the example above, `template-aliases` and `colors` are +the tables to be merged. `template-aliases."format_short_id(id)"` and +`colors."commit_id prefix"` in the default settings are overridden. On the other +hand, inner items of an inline table are *not* merged. For example, `{ bold = +true }` wouldn't be merged as `{ fg = "blue", bold = true }` even if the default +settings had `colors."commit_id prefix" = { fg = "blue" }`. + The docs below refer to keys in text using dotted notation, but example blocks will use heading notation to be unambiguous. If you are confident with TOML then use whichever suits you in your config. If you mix dotted keys and headings, @@ -146,7 +154,7 @@ commit_id = "green" ``` Parts of the style that are not overridden - such as the foreground color in the -example above - are inherited from the parent style. +example above - are inherited from the style of the parent label. Which elements can be colored is not yet documented, but see the [default color configuration](https://github.com/martinvonz/jj/blob/main/cli/src/config/colors.toml) diff --git a/lib/src/config.rs b/lib/src/config.rs index 65a30320a8..d6722a8a35 100644 --- a/lib/src/config.rs +++ b/lib/src/config.rs @@ -329,17 +329,15 @@ impl ConfigLayer { // Add .get_value(name) if needed. look_up_*() are low-level API. - /// Looks up sub table by the `name` path. Returns `Some(table)` if a table - /// was found at the path. Returns `Err(item)` if middle or leaf node wasn't - /// a table. - // TODO: Perhaps, an inline table will be processed as a value, and this - // function will return &ConfigTable. + /// Looks up sub non-inline table by the `name` path. Returns `Some(table)` + /// if a table was found at the path. Returns `Err(item)` if middle or leaf + /// node wasn't a table. pub fn look_up_table( &self, name: impl ToConfigNamePath, - ) -> Result, &ConfigItem> { + ) -> Result, &ConfigItem> { match self.look_up_item(name) { - Ok(Some(item)) => match item.as_table_like() { + Ok(Some(item)) => match item.as_table() { Some(table) => Ok(Some(table)), None => Err(item), }, @@ -391,15 +389,14 @@ impl ConfigLayer { } /// Looks up item from the `root_item`. Returns `Some(item)` if an item found at -/// the path. Returns `Err(item)` if middle node wasn't a table. +/// the path. Returns `Err(item)` if middle node wasn't a non-inline table. fn look_up_item<'a>( root_item: &'a ConfigItem, name: &ConfigNamePathBuf, ) -> Result, &'a ConfigItem> { let mut cur_item = root_item; for key in name.components() { - // TODO: Should inline tables be addressed by dotted name path? - let Some(table) = cur_item.as_table_like() else { + let Some(table) = cur_item.as_table() else { return Err(cur_item); }; cur_item = match table.get(key) { @@ -426,6 +423,37 @@ fn ensure_parent_table<'a, 'b>( } /// Stack of configuration layers which can be merged as needed. +/// +/// A [`StackedConfig`] is something like a read-only `overlayfs`. Tables and +/// values are directories and files respectively, and tables are merged across +/// layers. Tables and values can be addressed by [dotted name +/// paths](ToConfigNamePath). +/// +/// There's no tombstone notation to remove items from the lower layers. +/// +/// # Inline and non-inline tables +/// +/// An inline table is considered a value (or a file in file-system analogy.) +/// It would probably make sense because the syntax looks like an assignment +/// `key = { .. }`, and "no newlines are allowed between the curly braces." It's +/// unlikely that user defined a large inline table like `ui = { .. }`. +/// +/// - Inline tables will never be merged across layers, and the uppermost table +/// is always taken. +/// - Inner values of an inline table cannot be addressed by a dotted name path. +/// (e.g. `foo.bar` is not a valid path to `foo = { bar = x }`.) +/// - A lower inline table is shadowed by an upper non-inline table, just like a +/// file is shadowed by a directory of the same name. (e.g. `foo = { bar = x +/// }` is not merged, but shadowed by `foo.baz = y`.) +/// - A non-inline table can be converted to an inline table (or a value) on +/// `.get()`, but not the other way around. This specifically allows parsing +/// of a structured value from a merged table. +/// +/// # Array of tables +/// +/// If we employ the "array of tables" notation, array items will be gathered +/// from all layers, as if the array were a directory, and each item had a +/// unique file name. This merging strategy is not implemented yet. #[derive(Clone, Debug)] pub struct StackedConfig { /// Layers sorted by `source` (the lowest precedence one first.) @@ -544,16 +572,15 @@ impl StackedConfig { }) } - /// Looks up sub table from all layers, merges fields as needed. + /// Looks up sub non-inline table from all layers, merges fields as needed. /// /// Use `table_keys(prefix)` and `get([prefix, key])` instead if table /// values have to be converted to non-generic value type. pub fn get_table(&self, name: impl ToConfigNamePath) -> Result { - self.get_item_with(name, |item| { - // TODO: Should inline table be converted to non-inline table? - // .into_table() does this conversion. - item.into_table() - .map_err(|item| format!("Expected a table, but is {}", item.type_name())) + // Not using .into_table() because inline table shouldn't be converted. + self.get_item_with(name, |item| match item { + ConfigItem::Table(table) => Ok(table), + _ => Err(format!("Expected a table, but is {}", item.type_name())), }) } @@ -580,8 +607,8 @@ impl StackedConfig { }) } - /// Returns iterator over sub table keys in order of layer precedence. - /// Duplicated keys are omitted. + /// Returns iterator over sub non-inline table keys in order of layer + /// precedence. Duplicated keys are omitted. pub fn table_keys(&self, name: impl ToConfigNamePath) -> impl Iterator { let name = name.into_name_path(); let name = name.borrow(); @@ -607,8 +634,7 @@ fn get_merged_item( Ok(None) => continue, // parent is a table, but no value found Err(_) => break, // parent is not a table, shadows lower layers }; - // TODO: handle non-inline and inline tables differently? - if item.is_table_like() { + if item.is_table() { to_merge.push((item, index)); } else if to_merge.is_empty() { return Some((item.clone(), index)); // no need to allocate vec @@ -629,11 +655,12 @@ fn get_merged_item( Some((merged, top_index)) } -/// Looks up tables to be merged from `layers`, returns in reverse order. +/// Looks up non-inline tables to be merged from `layers`, returns in reverse +/// order. fn get_tables_to_merge<'a>( layers: &'a [ConfigLayer], name: &ConfigNamePathBuf, -) -> Vec<&'a dyn toml_edit::TableLike> { +) -> Vec<&'a ConfigTable> { let mut to_merge = Vec::new(); for layer in layers.iter().rev() { match layer.look_up_table(name) { @@ -647,17 +674,14 @@ fn get_tables_to_merge<'a>( /// Merges `upper_item` fields into `lower_item` recursively. fn merge_items(lower_item: &mut ConfigItem, upper_item: &ConfigItem) { - // TODO: Inline table will probably be treated as a value, not a table to be - // merged. For example, { fg = "red" } won't inherit other color parameters - // from the lower table. - let (Some(lower_table), Some(upper_table)) = - (lower_item.as_table_like_mut(), upper_item.as_table_like()) + // Inline table is a value, not a table to be merged. + let (Some(lower_table), Some(upper_table)) = (lower_item.as_table_mut(), upper_item.as_table()) else { // Not a table, the upper item wins. *lower_item = upper_item.clone(); return; }; - for (key, upper) in upper_table.iter() { + for (key, upper) in upper_table { match lower_table.entry(key) { toml_edit::Entry::Occupied(entry) => { merge_items(entry.into_mut(), upper); @@ -881,10 +905,23 @@ mod tests { a.b = { d = 'a.b.d #1' } "})); - // Inline tables are merged. This might change later. + // Inline table should override the lower value insta::assert_snapshot!( config.get_value("a.b").unwrap(), - @" { c = 'a.b.c #0' , d = 'a.b.d #1' }"); + @" { d = 'a.b.d #1' }"); + + // For API consistency, inner key of inline table cannot be addressed by + // a dotted name path. This could be supported, but it would be weird if + // a value could sometimes be accessed as a table. + assert_matches!( + config.get_value("a.b.d"), + Err(ConfigGetError::NotFound { name }) if name == "a.b.d" + ); + assert_matches!( + config.get_table("a.b"), + Err(ConfigGetError::Type { name, .. }) if name == "a.b" + ); + assert_eq!(config.table_keys("a.b").collect_vec(), vec![""; 0]); } #[test] @@ -897,12 +934,29 @@ mod tests { a.b.d = 'a.b.d #1' "})); + // Non-inline table is not merged with the lower inline table. It might + // be tempting to merge them, but then the resulting type would become + // unclear. If the merged type were an inline table, the path "a.b.d" + // would be shadowed by the lower layer. If the type were a non-inline + // table, new path "a.b.c" would be born in the upper layer. insta::assert_snapshot!( config.get_value("a.b").unwrap(), - @" { c = 'a.b.c #0' , d = 'a.b.d #1'}"); + @"{ d = 'a.b.d #1' }"); + assert_matches!( + config.get_value("a.b.c"), + Err(ConfigGetError::NotFound { name }) if name == "a.b.c" + ); + insta::assert_snapshot!( + config.get_value("a.b.d").unwrap(), + @" 'a.b.d #1'"); + + insta::assert_snapshot!( + config.get_table("a.b").unwrap(), + @"d = 'a.b.d #1'"); insta::assert_snapshot!( config.get_table("a").unwrap(), - @"b = { c = 'a.b.c #0' , d = 'a.b.d #1'}"); + @"b.d = 'a.b.d #1'"); + assert_eq!(config.table_keys("a.b").collect_vec(), vec!["d"]); } #[test]