config: add --when.commands scope

[bryceberger]

Closes #5217

Adds a new config scope resolution --when.commands. This is a list of space-separated commands:

--when.command = ["file"]        # matches `jj file show`, `jj file list`, etc
--when.command = ["file show"]   # matches `jj file show`, but *NOT* `jj file list`
--when.command = ["file", "log"] # matches `jj file` *OR* `jj log` (or subcommand of either)

Checklist

If applicable:

• I have updated CHANGELOG.md
• I have updated the documentation (README.md, docs/, demos/)
• I have updated the config schema (cli/src/config-schema.json)
• I have added tests to cover my changes

[bryceberger]

Unsure how to update config-schema.json for this. The current entry for pager just says "type": "string", which isn't true.

Also unsure if / where there are tests for pagers. I was unable to find any on a cursory glance.

[yuja]

[ui]
pager.status = ":none"
pager.log = { command = ["less", "-FRX"], env = { LESSCHARSET = "utf-8" } }
pager.diff = ["delta", "..."]
pager.default = "..."

I think it's better to not overuse ":<type>" syntax. How about adding a separate table for per-table pager configuration?

[ui]
paginate = "auto"  # default
pager = "..."  # default

[<table-for-per-command-pager-options>]
status.paginate = "never"
diff.pager = ["delta", "..."]

# or generic per-command table like this?
[commands.status]
ui.paginate = "never"

# or extend the conditional table?
[[--scope]]
--when.commands = ["status"]
ui.paginate = "never"

The full name of the highest level subcommand is used, i.e. jj operation diff uses ui.pager.operation.

I think "file show" and "file list" are quite different, and the user might want to enable pager only for one of them.

[bryceberger]

I'm intrigued by extending the [[--scope]] rules.

The rule for --when.repositories = ["a", "b"] is to match "a" | "b" --- would have to put some thought into how to match operation, operation diff, and diff separately. Maybe "operation.diff"?

Currently, jj_lib::config_resolver::resolve() throws away the layer if its --when doesn't match. Thinking of changing ScopeCondition::matches() to return Option<bool>, with None meaning "not enough information to resolve", i.e. resolving --when.repository during cli_util::run_internal but waiting until commands::run_command to resolve --when.commands.

[yuja]

The rule for --when.repositories = ["a", "b"] is to match "a" | "b" --- would have to put some thought into how to match operation, operation diff, and diff separately. Maybe "operation.diff"?

or "operation diff"? The [colors] table uses space-separated string.

Currently, jj_lib::config_resolver::resolve() throws away the layer if its --when doesn't match. Thinking of changing ScopeCondition::matches() to return Option<bool>, with None meaning "not enough information to resolve", i.e. resolving --when.repository during cli_util::run_internal but waiting until commands::run_command to resolve --when.commands.

run_command() calls resolve() multiple times, so I think it's okay to omit --when.commands conditionals until the command name becomes available. That's how --repository path is resolved.

[ilyagr]

Just saw this, nice! 🎉

FYI, I'm planning to add config for the built-in pager. My original thought was to simply create a ui.builtin-pager table. I'm not sure how it'll look if we make it configurable per-command, but we could figure it out later.

[yuja]

simply create a ui.builtin-pager table.

nit: might be better to include "streampager" in the key (e.g. stream-pager, builtin-stream-pager, builtin-pager-sp) so we can freely switch the underlying implementations in future.

I'm not sure how it'll look if we make it configurable per-command,

The current idea is to extend the scoped table, so any parameters (except for [aliases]) can be set per-command.

[ilyagr]

simply create a ui.builtin-pager table.

nit: might be better to include "streampager" in the key (e.g. stream-pager, builtin-stream-pager, builtin-pager-sp) so we can freely switch the underlying implementations in future.

I went back and forth on this. I went into the direction of making the options more generic, see #5415 . Your suggestion might still be better, I'm not sure; it's easy to change. If we go with that, perhaps :builtin should be an alias for :builtin-sp.

I'm not sure how it'll look if we make it configurable per-command,

The current idea is to extend the scoped table, so any parameters (except for [aliases]) can be set per-command.

That sounds great!

[bryceberger]

Completely changed the implementation, as reflected in the title. Now implements --when.commands = [...].

Also changed how config resolution works --- a condition can either be "resolved true", "resolved false", or "unknown". If any field is "unknown", the entire expression is unknown, and kept around. If any field is "resolved false", the entire expression is false, and discarded. Otherwise, added to current config.

This means that if a config layer still contains a --when key after resolution, it was resolved to "unknown". These layers are skipped in config::get_merged_item().

[yuja]

Also changed how config resolution works --- a condition can either be "resolved true", "resolved false", or "unknown". If any field is "unknown", the entire expression is unknown, and kept around. If any field is "resolved false", the entire expression is false, and discarded. Otherwise, added to current config.

Do we really need this? As I said, config layers are resolved multiple times in run_internal(), and the last resolution wins. If conditional variables have --when.commands, I don't think they should be disabled until the command name becomes available.

[bryceberger]

Do we really need this? As I said, config layers are resolved multiple times in run_internal(), and the last resolution wins. If conditional variables have --when.commands, I don't think they should be disabled until the command name becomes available.

... oooh, the RawConfig is kept unchanged through resolution. I had it in my mind that pop_scope_condition() would destructively remove the table, didn't realize it was clone-on-write.

[yuja]

LG (except for prefix match issue), thanks.

[martinvonz]

I just noticed that the documentation has not been updated. Are you able to fix that, @bryceberger?

[ilyagr]

The prerelease docs have some information. I'm not sure whether something is missing in there as well.

[martinvonz]

No, that looks good. Sorry, I don't know why I went to the 0.25 docs.