-
Notifications
You must be signed in to change notification settings - Fork 349
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
config: set $LESS
and $LESSCHARSET
even if $PAGER
is set
#3657
base: main
Are you sure you want to change the base?
Changes from all commits
fdcce0c
cee473b
7038f91
a965297
aa675b9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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(); | ||
|
@@ -441,7 +452,10 @@ pub enum CommandNameAndArgs { | |
String(String), | ||
Vec(NonEmptyCommandArgsVec), | ||
Structured { | ||
#[serde(default)] | ||
env: HashMap<String, String>, | ||
#[serde(default)] | ||
env_default: HashMap<String, String>, | ||
command: NonEmptyCommandArgsVec, | ||
}, | ||
} | ||
|
@@ -468,6 +482,7 @@ impl CommandNameAndArgs { | |
} | ||
CommandNameAndArgs::Structured { | ||
env: _, | ||
env_default: _, | ||
command: cmd, | ||
} => (Cow::Borrowed(&cmd.0[0]), Cow::Borrowed(&cmd.0[1..])), | ||
} | ||
|
@@ -477,7 +492,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()); | ||
|
@@ -497,8 +520,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 { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Optional: Perhaps |
||
write!(f, "{k}={v} ")?; | ||
} | ||
for (k, v) in env { | ||
write!(f, "{k}={v} ")?; | ||
} | ||
write!(f, "{}", command.0.join(" ")) | ||
|
@@ -533,21 +563,20 @@ mod tests { | |
|
||
#[test] | ||
fn test_command_args() { | ||
let config_text = r#" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Optional: I'd call this |
||
empty_array = [] | ||
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() | ||
.set_override("empty_array", Vec::<String>::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(); | ||
|
||
|
@@ -583,16 +612,40 @@ mod tests { | |
assert_eq!( | ||
command_args, | ||
CommandNameAndArgs::Structured { | ||
env_default: hashmap! {}, | ||
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_default").unwrap(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. 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. |
||
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(), | ||
}, | ||
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] | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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" } } | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Isn't it better to override the I don't remember exactly, but some default-installed There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Firstly, I think you don't need the I'm not sure about Yuya's point. I have LESS set to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I also think that, while it might make sense to add So, in my opinion, if we want to hack around weird default values for LESS, we could do the analogue of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I now think that if some default So, it sounds like I'm converging to something that's very close to this PR as is. (See also https://github.com/martinvonz/jj/pull/3657/files#r1833620765). One thing I'd now do differently is to move |
||
log-word-wrap = false | ||
log-synthetic-elided-nodes = true | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is pretty ugly. Is there a more convenient API in the
config
crate?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point.
PAGER
needs to be parsed as if it were executed by the shell. It might be simpler to just special casePAGER=less
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea to compare to Git. I found
https://github.com/git/git/blob/8f8d6eee531b3fa1a8ef14f169b0cb5035f7a772/Makefile#L357
Not sure what LV is about, but just following what they do seems OK to me.
Update: LV is about https://linux.die.net/man/1/lv. Haven't heard of it before. It seems obsolete (?), though it is still available in Debian.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think git sets it regardless of PAGER, anytime LESS is unset. So, it'd again break
PAGER="less -RX"
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But I somehow can't reproduce this. I unset
LESS
,core.pager
seems unset, butPAGER="less -R" git diff
acts differently fromPAGER="less -R +F" git diff
. OTOH,PAGER="less +R" git diff
prints color. So, I don't know what's going on with Git.Update: Never mind, I think this matches the advertised Git behavior. BTW, it's documented in https://git-scm.com/docs/git-config#Documentation/git-config.txt-corepager.
I suppose if Git breaks "less -RX", we can too. The user can override it with "less -RX +F". At least, at the moment, copying Git seems like the easiest solution. We could be more conservative and only set
LESS=R
, but maybe it's not worth it.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Re @yuja 's options from https://github.com/martinvonz/jj/pull/3657/files#r1833607331, I think setting
LESS
whenever it is unset would be between 2 and 3.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, it would be relatively easy because we just need to update
Ui::new_paged()
.In my list, 1-3 would be handled in config layer. 4 needs more context.