From ee5629169dfe530231298df47c1e7b1c621ea1c2 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 | 2 + Cargo.lock | 18 ++++++++ Cargo.toml | 2 + blah2 | 0 cli/Cargo.toml | 1 + cli/src/config-schema.json | 2 +- cli/src/config/misc.toml | 2 +- cli/src/config/windows.toml | 2 +- cli/src/ui.rs | 89 +++++++++++++++++++++++++++++++++++-- docs/config.md | 12 +++-- test | 0 11 files changed, 119 insertions(+), 11 deletions(-) create mode 100644 blah2 create mode 100644 test diff --git a/CHANGELOG.md b/CHANGELOG.md index fcf54f08e0..d3352993f9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,11 +16,13 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### New features * Templates now support logical operators: `||`, `&&`, `!` +* Builtin 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 builtin instead of disabled. ## [0.14.0] - 2024-02-07 diff --git a/Cargo.lock b/Cargo.lock index 4ec4d3601b..a1d489fd17 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,20 @@ dependencies = [ "adler", ] +[[package]] +name = "minus" +version = "5.5.1" +source = "git+https://github.com/benbrittain/minus?branch=persist-alternate#28158b69f3004cd634f8b31dc404223bd76411bc" +dependencies = [ + "crossbeam-channel", + "crossterm", + "once_cell", + "parking_lot", + "regex", + "textwrap", + "thiserror", +] + [[package]] name = "mio" version = "0.8.10" @@ -1938,6 +1953,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 40934b16bd..c06ef2e258 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -58,6 +58,8 @@ insta = { version = "1.34.0", features = ["filters"] } itertools = "0.12.1" libc = { version = "0.2.153" } maplit = "1.0.2" +# TODO use upstream +minus = { git = "https://github.com/benbrittain/minus", branch = "persist-alternate", features = [ "dynamic_output", "search" ] } num_cpus = "1.16.0" once_cell = "1.19.0" ouroboros = "0.18.0" diff --git a/blah2 b/blah2 new file mode 100644 index 0000000000..e69de29bb2 diff --git a/cli/Cargo.toml b/cli/Cargo.toml index 470bb5efef..a9c0442034 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-schema.json b/cli/src/config-schema.json index 08b84189f8..e6bdd5c8ce 100644 --- a/cli/src/config-schema.json +++ b/cli/src/config-schema.json @@ -72,7 +72,7 @@ "pager": { "type": "string", "description": "Pager to use for displaying command output", - "default": "less -FRX" + "default": ":builtin" }, "diff": { "type": "object", diff --git a/cli/src/config/misc.toml b/cli/src/config/misc.toml index d4684ee202..103d24bfe3 100644 --- a/cli/src/config/misc.toml +++ b/cli/src/config/misc.toml @@ -6,7 +6,7 @@ tree-level-conflicts = true [ui] paginate = "auto" -pager = { command = ["less", "-FRX"], env = { LESSCHARSET = "utf-8" } } +pager = ":builtin" log-word-wrap = false [snapshot] diff --git a/cli/src/config/windows.toml b/cli/src/config/windows.toml index 52991d5549..deca7a3816 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 0ecb4edb1b..564f14da53 100644 --- a/cli/src/ui.rs +++ b/cli/src/ui.rs @@ -14,15 +14,20 @@ use std::io::{IsTerminal as _, Stderr, StderrLock, Stdout, StdoutLock, Write}; use std::process::{Child, ChildStdin, Stdio}; +use std::rc::Rc; 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 +37,69 @@ enum UiOutput { child: Child, child_stdin: ChildStdin, }, + Builtin { + pager: BuiltinPager, + }, +} + +/// A builtin pager +#[derive(Clone)] +pub struct BuiltinPager { + pager: MinusPager, + dynamic_pager_thread: Rc>, +} + +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 { + self.pager + .push_str(std::str::from_utf8(buf).unwrap()) + .map_err(std::io::Error::other)?; + Ok(buf.len()) + } +} + +impl BuiltinPager { + pub fn finalize(self) { + let dynamic_pager_thread = self.dynamic_pager_thread; + if let Some(thread) = Rc::into_inner(dynamic_pager_thread) { + thread.join().unwrap(); + } + } + + pub fn new() -> Self { + let pager = MinusPager::new(); + // Keep the alternate screen around to emulate the $LESS `-X` flag. + pager + .persist_alternate_screen(true) + .expect("Able to set screen persistence"); + // 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: Rc::new(std::thread::spawn(move || { + // This thread handles the actual paging. + minus::dynamic_paging(pager_handle).unwrap(); + })), + } + } } impl UiOutput { + fn new_builtin() -> UiOutput { + UiOutput::Builtin { + pager: BuiltinPager::new(), + } + } fn new_terminal() -> UiOutput { UiOutput::Terminal { stdout: io::stdout(), @@ -49,21 +114,22 @@ impl UiOutput { } } -#[derive(Debug)] pub enum UiStdout<'a> { Terminal(StdoutLock<'static>), Paged(&'a ChildStdin), + Builtin(BuiltinPager), } -#[derive(Debug)] pub enum UiStderr<'a> { Terminal(StderrLock<'static>), Paged(&'a ChildStdin), + Builtin(BuiltinPager), } macro_rules! for_outputs { ($ty:ident, $output:expr, $pat:pat => $expr:expr) => { match $output { + $ty::Builtin($pat) => $expr, $ty::Terminal($pat) => $expr, $ty::Paged($pat) => $expr, } @@ -217,6 +283,11 @@ impl Ui { match self.output { UiOutput::Terminal { .. } if io::stdout().is_terminal() => { + if self.pager_cmd == 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 +296,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::Builtin { .. } | UiOutput::Paged { .. } => {} } } @@ -252,6 +324,7 @@ impl Ui { match &self.output { UiOutput::Terminal { stdout, .. } => UiStdout::Terminal(stdout.lock()), UiOutput::Paged { child_stdin, .. } => UiStdout::Paged(child_stdin), + UiOutput::Builtin { pager } => UiStdout::Builtin(pager.clone()), } } @@ -268,6 +341,7 @@ impl Ui { match &self.output { UiOutput::Terminal { stderr, .. } => UiStderr::Terminal(stderr.lock()), UiOutput::Paged { child_stdin, .. } => UiStderr::Paged(child_stdin), + UiOutput::Builtin { pager } => UiStderr::Builtin(pager.clone()), } } @@ -281,6 +355,7 @@ impl Ui { match &self.output { UiOutput::Terminal { .. } => Ok(Stdio::inherit()), UiOutput::Paged { child_stdin, .. } => Ok(duplicate_child_stdin(child_stdin)?.into()), + UiOutput::Builtin { .. } => Ok(Stdio::inherit()), } } @@ -290,6 +365,7 @@ impl Ui { match &self.output { UiOutput::Terminal { stderr, .. } => self.progress_indicator && stderr.is_terminal(), UiOutput::Paged { .. } => false, + UiOutput::Builtin { .. } => false, } } @@ -314,6 +390,11 @@ impl Ui { /// Waits for the pager exits. #[instrument(skip_all)] pub fn finalize_pager(&mut self) { + if let UiOutput::Builtin { pager } = + mem::replace(&mut self.output, UiOutput::new_terminal()) + { + pager.finalize(); + } if let UiOutput::Paged { mut child, child_stdin, diff --git a/docs/config.md b/docs/config.md index b42b664e1b..aac887e533 100644 --- a/docs/config.md +++ b/docs/config.md @@ -300,16 +300,20 @@ 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. +In the absence of any setting, the default is the special value `:builtin`, +which uses an [integrated pager](https://github.com/arijit79/minus/). It is likely +that 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 +``` Additionally, paging behavior can be toggled via `ui.paginate` like so: diff --git a/test b/test new file mode 100644 index 0000000000..e69de29bb2