config: inline TOML tables are processed differently from non-inline tables

[ErichDonGubler]

Description

Setting core.fsmonitor to "watchman" and core.watchman.register_snapshot_trigger using a table section in configuration:

# Can be placed anywhere in configuration, stopped working with 0.25.0.
[core]
fsmonitor = "watchman"
watchman = { register_snapshot_trigger = true }

…rather than fully qualified key paths, as documentation suggests:

# Must be placed before any config. sections, still works.
core.fsmonitor = "watchman"
core.watchman.register_snapshot_trigger = true

…now causes a configuration error that blocks most invocations of jj. 😱 This is a problem, because these are supposed to be equivalent in TOML, but jj apparently treats them differently.

I've bisected this issue to e5361c6 (CC @yuja).

Steps to Reproduce the Problem

1. With jj 0.25.0, use a configuration with the following core section:

   [core]
   fsmonitor = "watchman"
   watchman = { register_snapshot_trigger = true }

2. Run a jj command that loads configuration as one of its steps, i.e., jj log. Observe that only the following message is printed, with a non-zero exit code:

   Config error: Value not found for core.watchman.register_snapshot_trigger
   For help, see https://jj-vcs.github.io/jj/latest/config/.
   

3. Comment out configuration of one of these core keys. Observe that the error goes away, and jj works as before.

Expected Behavior

I expect "bare" key paths and key paths in sections to have the same config. behavior, in general.

Specifications

Reproduced on macOS 15.1.1 and Windows 11.

[yuja]

It's actually "broken" by 0de3691, and the change was intentional. We have to merge multiple TOML documents into a single table, which is something that is TOML but duplicated keys are allowed. I decided to not take inline table as a merge-able table because it would probably make sense to override the default colors.<name> = { style .. } by user-specified value. (And for consistency reasons, inner values of inline table aren't addressable by dotted key.)

That said, if the new behavior is super confusing, maybe we should revert the change and take inline table as mergeable item.

[emilazy]

FWIW I find this behaviour extremely surprising, as I see the difference as being purely syntactic sugar.

[yuja]

We can revert the merging/lookup change, but we'll still need some behavior differences in CLI. For example, jj config set ui whatever shouldn't be allowed if ui is a (non-inline) table. It's footgun if entire table were overwritten or deleted. It's also nice that jj config list shows inline-table values as such.

% jj config list --include-overridden --include-defaults 'colors."diff token"'
# colors."diff token" = { underline = true }
colors."diff token" = { bold = true }

(This depends on the current merging rules. If inner values of inline tables were merged, each inner value would have "overridden" flag.)

[martinvonz]

That said, if the new behavior is super confusing, maybe we should revert the change and take inline table as mergeable item.

I do find it very confusing, so it seems to me like we should revert it, even if that makes some other things worse again.

[yuja]

Ok. Things would get worse if we had an array of tables, but that's not a problem right now.

For consistency, maybe we can also disable jj config unset table-like, and add some flag to remove table items recursively.

[ErichDonGubler]

As a CLI tool author myself, it's very strange to treat user TOML documents as anything other than a way to write a layer of data. Actually using syntax for anything other than specifying the map/dictionary structure that may get merged against something else seems a recipe for confusion.

[yuja]

Actually using syntax for anything other than specifying the map/dictionary structure that may get merged against something else seems a recipe for confusion.

Just to clarify, you mean, in TOML-based configuration:

• non-inline tables and tables should be merged
• arrays of tables and inline arrays shouldn't be merged ?