From d3699c2327301eed7c757cb567276eaece83096f Mon Sep 17 00:00:00 2001 From: Benjamin Brittain Date: Sat, 10 Feb 2024 21:55:02 -0500 Subject: [PATCH] Use `minus` as a builtin pager --- CHANGELOG.md | 4 ++ Cargo.lock | 19 +++++++ Cargo.toml | 1 + cli/Cargo.toml | 1 + cli/src/config/windows.toml | 2 +- cli/src/ui.rs | 109 +++++++++++++++++++++++++++++++----- docs/config.md | 18 ++++-- 7 files changed, 134 insertions(+), 20 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a6b4da742..9edf62e44 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -35,11 +35,15 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 * `jj next/prev` now infer `--edit` when you're already editing a non-head commit (a commit with children). +* Built-in pager based on [minus](https://github.com/arijit79/minus/) + ### Fixed bugs * On Windows, symlinks in the repo are now materialized as regular files in the working copy (instead of resulting in a crash). +* On Windows, the pager will now be the built-in instead of disabled. + ## [0.14.0] - 2024-02-07 ### Deprecations diff --git a/Cargo.lock b/Cargo.lock index 567976aa0..e50132105 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1620,6 +1620,7 @@ dependencies = [ "jj-lib", "libc", "maplit", + "minus", "once_cell", "pest", "pest_derive", @@ -1858,6 +1859,21 @@ dependencies = [ "adler", ] +[[package]] +name = "minus" +version = "5.5.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "14b5f31d6666667f707078608f25e7615c48c2243a06b66ca0fa6c4ecb96362d" +dependencies = [ + "crossbeam-channel", + "crossterm", + "once_cell", + "parking_lot", + "regex", + "textwrap", + "thiserror", +] + [[package]] name = "mio" version = "0.8.10" @@ -1938,6 +1954,9 @@ name = "once_cell" version = "1.19.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "3fdb12b2476b595f9358c5161aa467c2438859caa136dec86c26fdd2efe17b92" +dependencies = [ + "parking_lot_core", +] [[package]] name = "oorandom" diff --git a/Cargo.toml b/Cargo.toml index ca690c9b5..3f7f41e48 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -58,6 +58,7 @@ insta = { version = "1.34.0", features = ["filters"] } itertools = "0.12.1" libc = { version = "0.2.153" } maplit = "1.0.2" +minus = { version = "5.5.0", features = [ "dynamic_output", "search" ] } num_cpus = "1.16.0" once_cell = "1.19.0" ouroboros = "0.18.0" diff --git a/cli/Cargo.toml b/cli/Cargo.toml index 470bb5efe..a9c044203 100644 --- a/cli/Cargo.toml +++ b/cli/Cargo.toml @@ -52,6 +52,7 @@ indexmap = { workspace = true } itertools = { workspace = true } jj-lib = { workspace = true } maplit = { workspace = true } +minus = { workspace = true } once_cell = { workspace = true } pest = { workspace = true } pest_derive = { workspace = true } diff --git a/cli/src/config/windows.toml b/cli/src/config/windows.toml index 52991d554..deca7a381 100644 --- a/cli/src/config/windows.toml +++ b/cli/src/config/windows.toml @@ -1,3 +1,3 @@ [ui] -paginate = "never" +pager = ":builtin" editor = "Notepad" diff --git a/cli/src/ui.rs b/cli/src/ui.rs index 0ecb4edb1..d7d19a4a4 100644 --- a/cli/src/ui.rs +++ b/cli/src/ui.rs @@ -15,14 +15,18 @@ 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, fmt, io, mem}; +use minus::Pager as MinusPager; use tracing::instrument; use crate::cli_util::CommandError; use crate::config::CommandNameAndArgs; use crate::formatter::{Formatter, FormatterFactory, LabeledWriter}; +const BUILTIN_PAGER_NAME: &str = ":builtin"; + enum UiOutput { Terminal { stdout: Stdout, @@ -32,9 +36,67 @@ enum UiOutput { child: Child, child_stdin: ChildStdin, }, + BuiltinPaged { + pager: BuiltinPager, + }, +} + +/// A builtin pager +pub struct BuiltinPager { + pager: MinusPager, + dynamic_pager_thread: JoinHandle<()>, +} + +impl std::io::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(std::io::Error::other)?; + self.pager.push_str(string).map_err(std::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(), @@ -49,16 +111,16 @@ impl UiOutput { } } -#[derive(Debug)] pub enum UiStdout<'a> { Terminal(StdoutLock<'static>), Paged(&'a ChildStdin), + Builtin(&'a BuiltinPager), } -#[derive(Debug)] pub enum UiStderr<'a> { Terminal(StderrLock<'static>), Paged(&'a ChildStdin), + Builtin(&'a BuiltinPager), } macro_rules! for_outputs { @@ -66,6 +128,7 @@ macro_rules! for_outputs { match $output { $ty::Terminal($pat) => $expr, $ty::Paged($pat) => $expr, + $ty::Builtin($pat) => $expr, } }; } @@ -217,6 +280,11 @@ impl Ui { match self.output { UiOutput::Terminal { .. } if io::stdout().is_terminal() => { + if self.pager_cmd == CommandNameAndArgs::String(BUILTIN_PAGER_NAME.into()) { + self.output = UiOutput::new_builtin(); + return; + } + match UiOutput::new_paged(&self.pager_cmd) { Ok(pager_output) => { self.output = pager_output; @@ -225,14 +293,15 @@ impl Ui { // The pager executable couldn't be found or couldn't be run writeln!( self.warning(), - "Failed to spawn pager '{name}': {e}", + "Failed to spawn pager '{name}': {e}. Consider using the `:builtin` \ + pager.", name = self.pager_cmd.split_name(), ) .ok(); } } } - UiOutput::Terminal { .. } | UiOutput::Paged { .. } => {} + UiOutput::Terminal { .. } | UiOutput::BuiltinPaged { .. } | UiOutput::Paged { .. } => {} } } @@ -252,6 +321,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), } } @@ -268,6 +338,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), } } @@ -281,6 +352,8 @@ 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()), } } @@ -290,6 +363,7 @@ impl Ui { match &self.output { UiOutput::Terminal { stderr, .. } => self.progress_indicator && stderr.is_terminal(), UiOutput::Paged { .. } => false, + UiOutput::BuiltinPaged { .. } => false, } } @@ -314,18 +388,23 @@ impl Ui { /// Waits for the pager exits. #[instrument(skip_all)] pub fn finalize_pager(&mut self) { - if let UiOutput::Paged { - mut child, - child_stdin, - } = mem::replace(&mut self.output, UiOutput::new_terminal()) - { - drop(child_stdin); - if let Err(e) = child.wait() { - // It's possible (though unlikely) that this write fails, but - // this function gets called so late that there's not much we - // can do about it. - writeln!(self.error(), "Failed to wait on pager: {e}").ok(); + match mem::replace(&mut self.output, UiOutput::new_terminal()) { + UiOutput::Paged { + mut child, + child_stdin, + } => { + drop(child_stdin); + if let Err(e) = child.wait() { + // It's possible (though unlikely) that this write fails, but + // this function gets called so late that there's not much we + // can do about it. + writeln!(self.error(), "Failed to wait on pager: {e}").ok(); + } } + UiOutput::BuiltinPaged { pager } => { + pager.finalize(); + } + _ => { /* no-op */ } } } diff --git a/docs/config.md b/docs/config.md index b42b664e1..39b580bb8 100644 --- a/docs/config.md +++ b/docs/config.md @@ -300,16 +300,26 @@ Can be customized by the `format_short_signature()` template alias. ## Pager -Windows users: Note that pagination is disabled by default on Windows for now -([#2040](https://github.com/martinvonz/jj/issues/2040)). - The default pager is can be set via `ui.pager` or the `PAGER` environment variable. The priority is as follows (environment variables are marked with a `$`): `ui.pager` > `$PAGER` -`less -FRX` is the default pager in the absence of any other setting. +`less -FRX` is the default pager in the absence of any other setting, except +on Windows where it is `:builtin`. + +The special value `:builtin` enables usage of the +[integrated pager](https://github.com/arijit79/minus/). It is likely if you +are using a standard Linux distro, your system has `$PAGER` set already +and that will be preferred over the built-in. To use the built-in: + +``` +jj config set --user ui.pager :builtin +``` + +It is possible the default will change to `:builtin` for all platforms in the +future. Additionally, paging behavior can be toggled via `ui.paginate` like so: