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

@@ -18,6 +18,12 @@ to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
 * The following configuration variables are now parsed strictly:
   `colors.<labels>`, `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.
 

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()))

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));

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

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

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" = [] } }]

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)

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<Option<&dyn toml_edit::TableLike>, &ConfigItem> {
+    ) -> Result<Option<&ConfigTable>, &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<Option<&'a ConfigItem>, &'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<ConfigTable, ConfigGetError> {
-        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<Item = &str> {
         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]