From fdcce0cc5a5b342c931664cd2420e52aa4c11ce0 Mon Sep 17 00:00:00 2001 From: Martin von Zweigbergk Date: Thu, 9 May 2024 21:48:09 -0700 Subject: [PATCH 1/5] config: rewrite test using TOML input for readability --- cli/src/config.rs | 25 +++++++++++-------------- 1 file changed, 11 insertions(+), 14 deletions(-) diff --git a/cli/src/config.rs b/cli/src/config.rs index 453d7dabed..291dd39037 100644 --- a/cli/src/config.rs +++ b/cli/src/config.rs @@ -533,21 +533,18 @@ mod tests { #[test] fn test_command_args() { + let config_text = r#" +empty_array = [] +empty_string = "" +"array" = ["emacs", "-nw"] +"string" = "emacs -nw" +structured = { env = { KEY1 = "value1", KEY2 = "value2" }, command = ["emacs", "-nw"] } +"#; let config = config::Config::builder() - .set_override("empty_array", Vec::::new()) - .unwrap() - .set_override("empty_string", "") - .unwrap() - .set_override("array", vec!["emacs", "-nw"]) - .unwrap() - .set_override("string", "emacs -nw") - .unwrap() - .set_override("structured.env.KEY1", "value1") - .unwrap() - .set_override("structured.env.KEY2", "value2") - .unwrap() - .set_override("structured.command", vec!["emacs", "-nw"]) - .unwrap() + .add_source(config::File::from_str( + config_text, + config::FileFormat::Toml, + )) .build() .unwrap(); From cee473b0c7269db03b9f132c1aa32c2b2d1227f8 Mon Sep 17 00:00:00 2001 From: Martin von Zweigbergk Date: Thu, 9 May 2024 21:45:07 -0700 Subject: [PATCH 2/5] cli: don't require `env` field in structured command config --- cli/src/config.rs | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-) diff --git a/cli/src/config.rs b/cli/src/config.rs index 291dd39037..7ca5e00ce2 100644 --- a/cli/src/config.rs +++ b/cli/src/config.rs @@ -441,6 +441,7 @@ pub enum CommandNameAndArgs { String(String), Vec(NonEmptyCommandArgsVec), Structured { + #[serde(default)] env: HashMap, command: NonEmptyCommandArgsVec, }, @@ -538,7 +539,8 @@ empty_array = [] empty_string = "" "array" = ["emacs", "-nw"] "string" = "emacs -nw" -structured = { env = { KEY1 = "value1", KEY2 = "value2" }, command = ["emacs", "-nw"] } +structured = { command = ["emacs", "-nw"] } +with_env = { env = { KEY1 = "value1", KEY2 = "value2" }, command = ["emacs", "-nw"] } "#; let config = config::Config::builder() .add_source(config::File::from_str( @@ -577,6 +579,18 @@ structured = { env = { KEY1 = "value1", KEY2 = "value2" }, command = ["emacs", " assert_eq!(args, ["-nw"].as_ref()); let command_args: CommandNameAndArgs = config.get("structured").unwrap(); + assert_eq!( + command_args, + CommandNameAndArgs::Structured { + env: hashmap! {}, + command: NonEmptyCommandArgsVec(["emacs", "-nw",].map(|s| s.to_owned()).to_vec()) + } + ); + let (name, args) = command_args.split_name_and_args(); + assert_eq!(name, "emacs"); + assert_eq!(args, ["-nw"].as_ref()); + + let command_args: CommandNameAndArgs = config.get("with_env").unwrap(); assert_eq!( command_args, CommandNameAndArgs::Structured { @@ -587,9 +601,6 @@ structured = { env = { KEY1 = "value1", KEY2 = "value2" }, command = ["emacs", " command: NonEmptyCommandArgsVec(["emacs", "-nw",].map(|s| s.to_owned()).to_vec()) } ); - let (name, args) = command_args.split_name_and_args(); - assert_eq!(name, "emacs"); - assert_eq!(args, ["-nw"].as_ref()); } #[test] From 7038f9188675d8e5ab4a36da4d9d4da1b01edcf0 Mon Sep 17 00:00:00 2001 From: Martin von Zweigbergk Date: Thu, 9 May 2024 21:01:10 -0700 Subject: [PATCH 3/5] cli: allow non-overriding env vars for configured commands 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/src/config.rs | 40 +++++++++++++++++++++++++++++++++++++--- 1 file changed, 37 insertions(+), 3 deletions(-) diff --git a/cli/src/config.rs b/cli/src/config.rs index 7ca5e00ce2..de7b5fe8ac 100644 --- a/cli/src/config.rs +++ b/cli/src/config.rs @@ -443,6 +443,8 @@ pub enum CommandNameAndArgs { Structured { #[serde(default)] env: HashMap, + #[serde(default)] + env_default: HashMap, command: NonEmptyCommandArgsVec, }, } @@ -469,6 +471,7 @@ impl CommandNameAndArgs { } CommandNameAndArgs::Structured { env: _, + env_default: _, command: cmd, } => (Cow::Borrowed(&cmd.0[0]), Cow::Borrowed(&cmd.0[1..])), } @@ -478,7 +481,15 @@ impl CommandNameAndArgs { pub fn to_command(&self) -> Command { let (name, args) = self.split_name_and_args(); let mut cmd = Command::new(name.as_ref()); - if let CommandNameAndArgs::Structured { env, .. } = self { + if let CommandNameAndArgs::Structured { + env_default, env, .. + } = self + { + for (key, val) in env_default { + if std::env::var(key).is_err() { + cmd.env(key, val); + } + } cmd.envs(env); } cmd.args(args.as_ref()); @@ -498,8 +509,15 @@ impl fmt::Display for CommandNameAndArgs { CommandNameAndArgs::String(s) => write!(f, "{s}"), // TODO: format with shell escapes CommandNameAndArgs::Vec(a) => write!(f, "{}", a.0.join(" ")), - CommandNameAndArgs::Structured { env, command } => { - for (k, v) in env.iter() { + CommandNameAndArgs::Structured { + env_default, + env, + command, + } => { + for (k, v) in env_default { + write!(f, "{k}={v} ")?; + } + for (k, v) in env { write!(f, "{k}={v} ")?; } write!(f, "{}", command.0.join(" ")) @@ -540,6 +558,7 @@ empty_string = "" "array" = ["emacs", "-nw"] "string" = "emacs -nw" structured = { command = ["emacs", "-nw"] } +with_env_default = { env_default = { KEY1 = "value1", KEY2 = "value2" }, command = ["emacs", "-nw"] } with_env = { env = { KEY1 = "value1", KEY2 = "value2" }, command = ["emacs", "-nw"] } "#; let config = config::Config::builder() @@ -582,6 +601,7 @@ with_env = { env = { KEY1 = "value1", KEY2 = "value2" }, command = ["emacs", "-n assert_eq!( command_args, CommandNameAndArgs::Structured { + env_default: hashmap! {}, env: hashmap! {}, command: NonEmptyCommandArgsVec(["emacs", "-nw",].map(|s| s.to_owned()).to_vec()) } @@ -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(); + assert_eq!( + command_args, + CommandNameAndArgs::Structured { + env_default: hashmap! { + "KEY1".to_string() => "value1".to_string(), + "KEY2".to_string() => "value2".to_string(), + }, + env: hashmap! {}, + command: NonEmptyCommandArgsVec(["emacs", "-nw",].map(|s| s.to_owned()).to_vec()) + } + ); + let command_args: CommandNameAndArgs = config.get("with_env").unwrap(); assert_eq!( command_args, CommandNameAndArgs::Structured { + env_default: hashmap! {}, env: hashmap! { "KEY1".to_string() => "value1".to_string(), "KEY2".to_string() => "value2".to_string(), From a9652979a7bedcdcf48af93cc58ebd6d18fb01c1 Mon Sep 17 00:00:00 2001 From: Martin von Zweigbergk Date: Thu, 9 May 2024 22:06:03 -0700 Subject: [PATCH 4/5] cli: don't override `$LESS` and `$LESSCHARSET` --- CHANGELOG.md | 3 +++ cli/src/config/misc.toml | 2 +- 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3404c54673..afa8f40917 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,6 +15,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 * The `commit_summary_no_branches` template is superseded by `templates.branch_list`. +* The `$LESS` and `$LESSCHARSET` environment variables are now respected, unless + you have configured a pager (via `ui.pager` or `$PAGER`). + ### Deprecations ### New features diff --git a/cli/src/config/misc.toml b/cli/src/config/misc.toml index dc279216ac..16453b8d87 100644 --- a/cli/src/config/misc.toml +++ b/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" } } log-word-wrap = false log-synthetic-elided-nodes = true From aa675b9cdec06514741069605ef2102413f878de Mon Sep 17 00:00:00 2001 From: Martin von Zweigbergk Date: Thu, 9 May 2024 13:16:39 -0700 Subject: [PATCH 5/5] config: set `$LESS` and `$LESSCHARSET` even if `$PAGER` is set 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 --- cli/src/config.rs | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/cli/src/config.rs b/cli/src/config.rs index de7b5fe8ac..63ca5dab08 100644 --- a/cli/src/config.rs +++ b/cli/src/config.rs @@ -324,7 +324,18 @@ fn env_base() -> config::Config { builder = builder.set_override("ui.color", "never").unwrap(); } if let Ok(value) = env::var("PAGER") { - builder = builder.set_override("ui.pager", value).unwrap(); + builder = builder + .set_override( + "ui.pager.command", + config::Value::new( + None, + config::ValueKind::Array(vec![config::Value::new( + None, + config::ValueKind::String(value), + )]), + ), + ) + .unwrap(); } if let Ok(value) = env::var("VISUAL") { builder = builder.set_override("ui.editor", value).unwrap();