config: add `--when.commands` scope #5311

bryceberger · 2025-01-09T04:59:06Z

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 · 2025-01-09T05:03:02Z

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 · 2025-01-09T07:20:12Z

[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 · 2025-01-09T08:52:13Z

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 · 2025-01-09T09:09:09Z

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 · 2025-01-21T07:09:13Z

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 · 2025-01-21T08:07:35Z

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 · 2025-01-21T08:26:01Z

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 · 2025-01-22T23:09:24Z

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 · 2025-01-23T01:19:49Z

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.

cli/src/cli_util.rs

cli/src/config.rs

yuja · 2025-01-23T01:35:02Z

lib/src/config_resolver.rs

+            let actual = actual.split(' ');
+            let any_match = candidates.iter().any(|candidate| {
+                let candidate = candidate.split(' ');
+                candidate.zip_longest(actual.clone()).all(|e| match e {


Maybe .strip_prefix() and check the first character?

Ah, that is cleaner. Not sure why you would need to check the first character --- "file show".strip_prefix("file") == Some(" show"), "file show".strip_prefix("file show") == Some("").

I meant the first character after stripping (i.e. the separator or end of string.)

Oh, so jj abcdef doesn't accidentally match --when.commands = ["abc"].

bryceberger · 2025-01-23T02:08:10Z

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.

Motivating use case: [[--scope]] --when.command = ["log"] [--scope.ui] pager = "less" This adds a new (optional) field to `--when` config conditions, to inspect the current command. `--when.commands` is a list of space-separated values that matches a space-separated command. To be specific: --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) When both `--when.commands` and `--when.repositories` are set, the intersection is used.

bryceberger marked this pull request as ready for review January 9, 2025 05:06

bryceberger marked this pull request as draft January 10, 2025 18:13

yuja mentioned this pull request Jan 16, 2025

FR: be able to set commit_timestamp contextually #5366

Open

This was referenced Jan 21, 2025

FR: Add --paginate flag. #5413

Closed

FR: selectively enable/disable pagination #2926

Open

ilyagr mentioned this pull request Jan 21, 2025

builtin pager: add configuration options, change default options #5415

Draft

5 tasks

bryceberger force-pushed the bryce/push-mysxqyqourrv branch 2 times, most recently from f50ef2c to 76b1269 Compare January 22, 2025 22:56

bryceberger changed the title ~~cli: make pager configurable per command~~ config: add --when.commands scope Jan 22, 2025

bryceberger marked this pull request as ready for review January 22, 2025 22:57

bryceberger force-pushed the bryce/push-mysxqyqourrv branch from 76b1269 to dedbc47 Compare January 22, 2025 23:02

bryceberger requested a review from yuja January 22, 2025 23:12

yuja reviewed Jan 23, 2025

View reviewed changes

bryceberger added 2 commits January 22, 2025 21:26

config: propogate --when.command through cli

6e9ec50

bryceberger force-pushed the bryce/push-mysxqyqourrv branch from dedbc47 to 6e9ec50 Compare January 23, 2025 02:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

config: add `--when.commands` scope #5311

config: add `--when.commands` scope #5311

bryceberger commented Jan 9, 2025 •

edited

Loading

bryceberger commented Jan 9, 2025

yuja commented Jan 9, 2025 •

edited

Loading

bryceberger commented Jan 9, 2025

yuja commented Jan 9, 2025

ilyagr commented Jan 21, 2025

yuja commented Jan 21, 2025

ilyagr commented Jan 21, 2025 •

edited

Loading

bryceberger commented Jan 22, 2025

yuja commented Jan 23, 2025 •

edited

Loading

yuja Jan 23, 2025

bryceberger Jan 23, 2025

yuja Jan 23, 2025

bryceberger Jan 23, 2025

bryceberger commented Jan 23, 2025

config: add --when.commands scope #5311

Are you sure you want to change the base?

config: add --when.commands scope #5311

Conversation

bryceberger commented Jan 9, 2025 • edited Loading

Checklist

bryceberger commented Jan 9, 2025

yuja commented Jan 9, 2025 • edited Loading

bryceberger commented Jan 9, 2025

yuja commented Jan 9, 2025

ilyagr commented Jan 21, 2025

yuja commented Jan 21, 2025

ilyagr commented Jan 21, 2025 • edited Loading

bryceberger commented Jan 22, 2025

yuja commented Jan 23, 2025 • edited Loading

yuja Jan 23, 2025

Choose a reason for hiding this comment

bryceberger Jan 23, 2025

Choose a reason for hiding this comment

yuja Jan 23, 2025

Choose a reason for hiding this comment

bryceberger Jan 23, 2025

Choose a reason for hiding this comment

bryceberger commented Jan 23, 2025

config: add `--when.commands` scope #5311

config: add `--when.commands` scope #5311

bryceberger commented Jan 9, 2025 •

edited

Loading

yuja commented Jan 9, 2025 •

edited

Loading

ilyagr commented Jan 21, 2025 •

edited

Loading

yuja commented Jan 23, 2025 •

edited

Loading