config: set `$LESS` and `$LESSCHARSET` even if `$PAGER` is set #3657

martinvonz · 2024-05-09T22:50:13Z

Our current default for ui.pager is this:

ui.pager = { command = ["less", "-FRX"], env = { LESSCHARSET = "utf-8" } }

If the user has $PAGER set, we take that value and replace the above table by a scalar set to the value from the environment variable. That means that anyone who has set $PAGER to just less will lose both the -FRX and the charset, making e.g. colored output from jj result in escaped ANSI codes rendered by less. The lack of those options might not matter for other tools they use so they might not have realized that they wanted those options.

This patch attempts to improve the situation by changing the default ui.pager to pass -FRX via $LESS, and by setting the value from $PAGER in ui.pager.command, so the rest of the config is left alone.

The default config will still be ignored if you set the scalar ui.pager to e.g. less, since that overrides the table.

Closes #2926

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

martinvonz · 2024-05-09T22:51:23Z

cli/src/config.rs

+                config::Value::new(
+                    None,
+                    config::ValueKind::Array(vec![config::Value::new(
+                        None,
+                        config::ValueKind::String(value),
+                    )]),
+                ),


This is pretty ugly. Is there a more convenient API in the config crate?

.set_override("ui.pager.command", vec![value]) appears to work.
https://docs.rs/config/0.13.4/config/enum.ValueKind.html#impl-From%3CVec%3CT%3E%3E-for-ValueKind

Actually, I'm not sure this is correct if PAGER contains a command with arguments, e.g. PAGER="less -FRX".

I'm not sure whether that is considered a kosher value for PAGER, but it usually worked for most programs, I think. Something did make me switch to using PAGER=less and LESS=... for the options, though.

It's possible this is still worth doing even if it doesn't work for PAGER="less -FRX", I'm unsure how common that is.

Or, we could allow ui.pager.command to be a string (perhaps in a future PR).

Actually, I'm not sure this is correct if PAGER contains a command with arguments, e.g. PAGER="less -FRX"?

Good point. PAGER needs to be parsed as if it were executed by the shell. It might be simpler to just special case PAGER=less.

I'll wait a bit to hear from others if we actually want this PR before I spend more time on it.

cli/src/config/misc.toml

@ilyagr

This lets you configure e.g. some environment variables to pass to a pager that will only be used if they're not already set in the user's environment. Thanks to @ilyagr for the suggestion.

Our current default for `ui.pager` is this: ```toml ui.pager = { command = ["less"], env_default = { LESS = "-FRX", LESSCHARSET = "utf-8" } } ``` If the user has `$PAGER` set, we take that value and replace the above table by a scalar set to the value from the environment variable. That means that anyone who has set `$PAGER` to just `less` will lose both the `-FRX` and the charset, making e.g. colored output from `jj` result in escaped ANSI codes rendered by `less`. The lack of those options might not matter for other tools they use so they might not have realized that they wanted those options. This patch attempts to improve the situation by setting the value from `$PAGER` in `ui.pager.command` so the rest of the config is left alone. The default config will still be ignored if you set the scalar `ui.pager` to e.g. `less`, since that overrides the table. Closes #2926

yuja · 2024-05-10T05:40:36Z

cli/src/config.rs

+                config::Value::new(
+                    None,
+                    config::ValueKind::Array(vec![config::Value::new(
+                        None,
+                        config::ValueKind::String(value),
+                    )]),
+                ),


.set_override("ui.pager.command", vec![value]) appears to work.
https://docs.rs/config/0.13.4/config/enum.ValueKind.html#impl-From%3CVec%3CT%3E%3E-for-ValueKind

yuja · 2024-05-10T05:47:22Z

cli/src/config/misc.toml

@@ -13,7 +13,7 @@ allow-filesets = false
 always-allow-large-revsets = false
 diff-instructions = true
 paginate = "auto"
-pager = { command = ["less", "-FRX"], env = { LESSCHARSET = "utf-8" } }
+pager = { command = ["less"], env_default = { LESS="-FRX", LESSCHARSET = "utf-8" } }


Isn't it better to override the LESS variable as before?

I don't remember exactly, but some default-installed ~/.bashrc might set LESS=... If it didn't contain -R, jj's default would break.

Firstly, I think you don't need the - in LESS.

I'm not sure about Yuya's point. I have LESS set to FRX --mouse --wheel-lines=2, and I wouldn't like it overridden entirely. Perhaps we could print a warning after less quits if FRX was not in LESS and it was set? Perhaps we could prepend FRX to the variable?

I also think that, while it might make sense to add -R to LESS, forcibly adding -F (and possibly also -X) might be wrong if the user explicitly did not include them. For example, let's say we launch jj from a file manager like Midnight Commander or ranger. The user might then be relying on less not quitting after one screen to see the output at all. If we forcibly add -F, that would break this use-case. Forcibly adding -X might break it too, but I'm less sure.

So, in my opinion, if we want to hack around weird default values for LESS, we could do the analogue of LESS="R $LESS", but probably no more than that.

ilyagr · 2024-05-10T05:36:44Z

cli/src/config.rs

@@ -533,21 +533,18 @@ mod tests {

    #[test]
    fn test_command_args() {
+        let config_text = r#"


Optional: I'd call this config_toml.

ilyagr · 2024-05-10T05:43:56Z

cli/src/config.rs

+                env,
+                command,
+            } => {
+                for (k, v) in env_default {


Optional: Perhaps k.=v would work here? Or k~=v? Or (k=v)?

ilyagr · 2024-05-10T05:49:03Z

cli/src/config.rs

@@ -590,10 +610,24 @@ with_env = { env = { KEY1 = "value1", KEY2 = "value2" }, command = ["emacs", "-n
        assert_eq!(name, "emacs");
        assert_eq!(args, ["-nw"].as_ref());

+        let command_args: CommandNameAndArgs = config.get("with_env_default").unwrap();


Optional: I was going to suggest adding a test with https://doc.rust-lang.org/std/env/fn.set_var.html here, but I'm not so sure after reading the safety warning there. cargo-nextest is, in principle, multithreaded.

You could also set up a test that sets some variable that very likely exists, like PATH or CARGO. I guess to make the test reliable, you could check if that variable is set first, and skip the test if it isn't.

ilyagr · 2024-05-10T05:52:32Z

cli/src/config/misc.toml

@@ -13,7 +13,7 @@ allow-filesets = false
 always-allow-large-revsets = false
 diff-instructions = true
 paginate = "auto"
-pager = { command = ["less", "-FRX"], env = { LESSCHARSET = "utf-8" } }
+pager = { command = ["less"], env_default = { LESS="-FRX", LESSCHARSET = "utf-8" } }


Firstly, I think you don't need the - in LESS.

I'm not sure about Yuya's point. I have LESS set to FRX --mouse --wheel-lines=2, and I wouldn't like it overridden entirely. Perhaps we could print a warning after less quits if FRX was not in LESS and it was set? Perhaps we could prepend FRX to the variable?

martinvonz commented May 9, 2024

View reviewed changes

ilyagr reviewed May 10, 2024

View reviewed changes

cli/src/config/misc.toml Outdated Show resolved Hide resolved

martinvonz force-pushed the push-syxxkrvnzlox branch from 2f2836b to f195670 Compare May 10, 2024 05:28

martinvonz added 5 commits May 9, 2024 22:34

config: rewrite test using TOML input for readability

fdcce0c

cli: don't require env field in structured command config

cee473b

cli: allow non-overriding env vars for configured commands

7038f91

This lets you configure e.g. some environment variables to pass to a pager that will only be used if they're not already set in the user's environment. Thanks to @ilyagr for the suggestion.

cli: don't override $LESS and $LESSCHARSET

a965297

martinvonz force-pushed the push-syxxkrvnzlox branch from f195670 to aa675b9 Compare May 10, 2024 05:35

yuja reviewed May 10, 2024

View reviewed changes

ilyagr reviewed May 10, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

config: set `$LESS` and `$LESSCHARSET` even if `$PAGER` is set #3657

config: set `$LESS` and `$LESSCHARSET` even if `$PAGER` is set #3657

martinvonz commented May 9, 2024

martinvonz May 9, 2024

yuja May 10, 2024

ilyagr May 10, 2024 •

edited

ilyagr May 10, 2024 •

edited

yuja May 10, 2024

martinvonz May 11, 2024

yuja May 10, 2024

yuja May 10, 2024

ilyagr May 10, 2024

ilyagr May 10, 2024

ilyagr May 10, 2024

ilyagr May 10, 2024

ilyagr May 10, 2024 •

edited

ilyagr May 10, 2024

config: set $LESS and $LESSCHARSET even if $PAGER is set #3657

Are you sure you want to change the base?

config: set $LESS and $LESSCHARSET even if $PAGER is set #3657

Conversation

martinvonz commented May 9, 2024

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ilyagr May 10, 2024 • edited

Choose a reason for hiding this comment

ilyagr May 10, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ilyagr May 10, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

config: set `$LESS` and `$LESSCHARSET` even if `$PAGER` is set #3657

config: set `$LESS` and `$LESSCHARSET` even if `$PAGER` is set #3657

ilyagr May 10, 2024 •

edited

ilyagr May 10, 2024 •

edited

ilyagr May 10, 2024 •

edited