From ca95cfe50b1bf0bdeb9e9f8bc54f188119f95616 Mon Sep 17 00:00:00 2001 From: Yuya Nishihara Date: Fri, 2 Aug 2024 17:25:51 +0900 Subject: [PATCH] ui: switch builtin pager to streampager According to the discussion on Discord, streampager is still maintained. It brings more dependencies, but seems more reliable in our use case. For example, the streampager doesn't consume inputs indefinitely whereas minus does. We can also use OS-level pipe to redirect child stderr to the pager. --- CHANGELOG.md | 4 ++ cli/src/ui.rs | 128 +++++++++++++++++++++++------------------------- docs/config.md | 4 +- docs/windows.md | 7 +-- 4 files changed, 70 insertions(+), 73 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f42cfd4ef8c..d2ce47a34a7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -38,6 +38,10 @@ to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). * `jj --at-op=@` no longer merges concurrent operations if explicitly specified. +* The builtin pager is switched to + [streampager](https://github.com/markbt/streampager/). It can handle large + inputs better. + ### Deprecations * The original configuration syntax for `jj fix` is now deprecated in favor of diff --git a/cli/src/ui.rs b/cli/src/ui.rs index 98a707a0efe..810cfd0e90b 100644 --- a/cli/src/ui.rs +++ b/cli/src/ui.rs @@ -16,10 +16,10 @@ use std::io::{IsTerminal as _, Stderr, StderrLock, Stdout, StdoutLock, Write}; use std::process::{Child, ChildStdin, Stdio}; use std::str::FromStr; use std::thread::JoinHandle; -use std::{env, error, fmt, io, iter, mem}; +use std::{env, error, fmt, io, iter, mem, thread}; use itertools::Itertools as _; -use minus::Pager as MinusPager; +use os_pipe::PipeWriter; use tracing::instrument; use crate::command_error::{config_error_with_message, CommandError}; @@ -40,66 +40,13 @@ enum UiOutput { child_stdin: ChildStdin, }, BuiltinPaged { - pager: BuiltinPager, + out_wr: PipeWriter, + err_wr: PipeWriter, + pager_thread: JoinHandle>, }, } -/// A builtin pager -pub struct BuiltinPager { - pager: MinusPager, - dynamic_pager_thread: JoinHandle<()>, -} - -impl Write for &BuiltinPager { - fn flush(&mut self) -> io::Result<()> { - // no-op since this is being run in a dynamic pager mode. - Ok(()) - } - - fn write(&mut self, buf: &[u8]) -> io::Result { - let string = std::str::from_utf8(buf).map_err(io::Error::other)?; - self.pager.push_str(string).map_err(io::Error::other)?; - Ok(buf.len()) - } -} - -impl Default for BuiltinPager { - fn default() -> Self { - Self::new() - } -} - -impl BuiltinPager { - pub fn finalize(self) { - let dynamic_pager_thread = self.dynamic_pager_thread; - dynamic_pager_thread.join().unwrap(); - } - - pub fn new() -> Self { - let pager = MinusPager::new(); - // Prefer to be cautious and only kill the pager instead of the whole process - // like minus does by default. - pager - .set_exit_strategy(minus::ExitStrategy::PagerQuit) - .expect("Able to set the exit strategy"); - let pager_handle = pager.clone(); - - BuiltinPager { - pager, - dynamic_pager_thread: std::thread::spawn(move || { - // This thread handles the actual paging. - minus::dynamic_paging(pager_handle).unwrap(); - }), - } - } -} - impl UiOutput { - fn new_builtin() -> UiOutput { - UiOutput::BuiltinPaged { - pager: BuiltinPager::new(), - } - } fn new_terminal() -> UiOutput { UiOutput::Terminal { stdout: io::stdout(), @@ -115,6 +62,26 @@ impl UiOutput { Ok(UiOutput::Paged { child, child_stdin }) } + fn new_builtin_paged() -> streampager::Result { + let mut pager = streampager::Pager::new_using_stdio()?; + // TODO: should we set the interface mode to be "less -FRX" like? + // It will override the user-configured values. + + // Use native pipe, which can be attached to child process. The stdout + // stream could be an in-memory channel, but the cost of extra syscalls + // wouldn't matter. + let (out_rd, out_wr) = os_pipe::pipe()?; + let (err_rd, err_wr) = os_pipe::pipe()?; + pager.add_stream(out_rd, "")?; + pager.add_error_stream(err_rd, "stderr")?; + + Ok(UiOutput::BuiltinPaged { + out_wr, + err_wr, + pager_thread: thread::spawn(|| pager.run()), + }) + } + fn finalize(self, ui: &Ui) { match self { UiOutput::Terminal { .. } => { /* no-op */ } @@ -135,8 +102,27 @@ impl UiOutput { .ok(); } } - UiOutput::BuiltinPaged { pager } => { - pager.finalize(); + UiOutput::BuiltinPaged { + out_wr, + err_wr, + pager_thread, + } => { + drop(out_wr); + drop(err_wr); + match pager_thread.join() { + Ok(Ok(())) => {} + Ok(Err(err)) => { + writeln!( + ui.warning_default(), + "Failed to run builtin pager: {err}", + err = format_error_with_sources(&err), + ) + .ok(); + } + Err(_) => { + writeln!(ui.warning_default(), "Builtin pager crashed.").ok(); + } + } } } } @@ -145,13 +131,13 @@ impl UiOutput { pub enum UiStdout<'a> { Terminal(StdoutLock<'static>), Paged(&'a ChildStdin), - Builtin(&'a BuiltinPager), + Builtin(&'a PipeWriter), } pub enum UiStderr<'a> { Terminal(StderrLock<'static>), Paged(&'a ChildStdin), - Builtin(&'a BuiltinPager), + Builtin(&'a PipeWriter), } macro_rules! for_outputs { @@ -332,7 +318,16 @@ impl Ui { let use_builtin_pager = matches!( &self.pager_cmd, CommandNameAndArgs::String(name) if name == BUILTIN_PAGER_NAME); let new_output = if use_builtin_pager { - Some(UiOutput::new_builtin()) + UiOutput::new_builtin_paged() + .inspect_err(|err| { + writeln!( + self.warning_default(), + "Failed to set up builtin pager: {err}", + err = format_error_with_sources(err), + ) + .ok(); + }) + .ok() } else { UiOutput::new_paged(&self.pager_cmd) .inspect_err(|err| { @@ -369,7 +364,7 @@ impl Ui { match &self.output { UiOutput::Terminal { stdout, .. } => UiStdout::Terminal(stdout.lock()), UiOutput::Paged { child_stdin, .. } => UiStdout::Paged(child_stdin), - UiOutput::BuiltinPaged { pager } => UiStdout::Builtin(pager), + UiOutput::BuiltinPaged { out_wr, .. } => UiStdout::Builtin(out_wr), } } @@ -386,7 +381,7 @@ impl Ui { match &self.output { UiOutput::Terminal { stderr, .. } => UiStderr::Terminal(stderr.lock()), UiOutput::Paged { child_stdin, .. } => UiStderr::Paged(child_stdin), - UiOutput::BuiltinPaged { pager } => UiStderr::Builtin(pager), + UiOutput::BuiltinPaged { err_wr, .. } => UiStderr::Builtin(err_wr), } } @@ -400,8 +395,7 @@ impl Ui { match &self.output { UiOutput::Terminal { .. } => Ok(Stdio::inherit()), UiOutput::Paged { child_stdin, .. } => Ok(duplicate_child_stdin(child_stdin)?.into()), - // Stderr does not get redirected through the built-in pager. - UiOutput::BuiltinPaged { .. } => Ok(Stdio::inherit()), + UiOutput::BuiltinPaged { err_wr, .. } => Ok(err_wr.try_clone()?.into()), } } diff --git a/docs/config.md b/docs/config.md index 5d2552004ec..41ea8d1fa02 100644 --- a/docs/config.md +++ b/docs/config.md @@ -399,9 +399,7 @@ a `$`): on Windows where it is `:builtin`. The special value `:builtin` enables usage of the [integrated pager called -`minus`](https://github.com/AMythicDev/minus/). See the [docs for the `minus` -pager](https://docs.rs/minus/latest/minus/#default-keybindings) for the key -bindings and some more details. +`streampager`](https://github.com/markbt/streampager/). If you are using a standard Linux distro, your system likely already has `$PAGER` set and that will be preferred over the built-in. To use the built-in: diff --git a/docs/windows.md b/docs/windows.md index b13b65e94b8..cbec50358e3 100644 --- a/docs/windows.md +++ b/docs/windows.md @@ -33,9 +33,10 @@ especially IDEs, preserve LF line endings. ## Pagination -On Windows, `jj` will use its integrated pager called `minus` by default, unless -the environment variable `%PAGER%` or the config `ui.pager` is explicitly set. -See the [pager section of the config docs](config.md#pager) for more details. +On Windows, `jj` will use its integrated pager called `streampager` by default, +unless the environment variable `%PAGER%` or the config `ui.pager` is explicitly +set. See the [pager section of the config docs](config.md#pager) for more +details. If the built-in pager doesn't meet your needs and you have Git installed, you can switch to using Git's pager as follows: