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

@@ -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

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<streampager::Result<()>>,
     },
 }
 
-/// 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<usize> {
-        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<UiOutput> {
+        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()),
         }
     }
 

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:

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: