Merge pull request #2805 from Anomalocaridid/refactor-lessopen

Rework `lessopen` implementation to use `execute` crate instead of `run_script`

CHANGELOG.md

@@ -23,6 +23,7 @@
 - Fix panel width when line 10000 wraps, see #2854 (@eth-p)
 - Fix compile issue of `time` dependency caused by standard library regression #3045 (@cyqsimon)
 - Fix override behavior of --plain and --paging, see issue #2731 and PR #3108 (@einfachIrgendwer0815)
+- Fix bugs in `$LESSOPEN` support, see #2805 (@Anomalocaridid)
 
 ## Other
 

Cargo.lock

@@ -124,6 +124,7 @@ dependencies = [
  "content_inspector",
  "encoding_rs",
  "etcetera",
+ "execute",
  "expect-test",
  "flate2",
  "git2",
@@ -135,12 +136,10 @@ dependencies = [
  "nix",
  "nu-ansi-term",
  "once_cell",
- "os_str_bytes",
  "path_abs",
  "plist",
  "predicates",
  "regex",
- "run_script",
  "semver",
  "serde",
  "serde_derive",
@@ -401,12 +400,6 @@ version = "0.3.3"
 source = "registry+https://github.com/rust-lang/crates.io-index"
 checksum = "fea41bba32d969b513997752735605054bc0dfa92b4c56bf1189f2e174be7a10"
 
-[[package]]
-name = "dunce"
-version = "1.0.3"
-source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "0bd4b30a6560bbd9b4620f4de34c3f14f60848e58a9b7216801afcb4c7b31c3c"
-
 [[package]]
 name = "either"
 version = "1.8.0"
@@ -466,6 +459,43 @@ dependencies = [
  "windows-sys 0.48.0",
 ]
 
+[[package]]
+name = "execute"
+version = "0.2.13"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "3a82608ee96ce76aeab659e9b8d3c2b787bffd223199af88c674923d861ada10"
+dependencies = [
+ "execute-command-macro",
+ "execute-command-tokens",
+ "generic-array",
+]
+
+[[package]]
+name = "execute-command-macro"
+version = "0.1.9"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "90dec53d547564e911dc4ff3ecb726a64cf41a6fa01a2370ebc0d95175dd08bd"
+dependencies = [
+ "execute-command-macro-impl",
+]
+
+[[package]]
+name = "execute-command-macro-impl"
+version = "0.1.10"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "ce8cd46a041ad005ab9c71263f9a0ff5b529eac0fe4cc9b4a20f4f0765d8cf4b"
+dependencies = [
+ "execute-command-tokens",
+ "quote",
+ "syn",
+]
+
+[[package]]
+name = "execute-command-tokens"
+version = "0.1.7"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "69dc321eb6be977f44674620ca3aa21703cb20ffbe560e1ae97da08401ffbcad"
+
 [[package]]
 name = "expect-test"
 version = "1.5.0"
@@ -527,24 +557,12 @@ dependencies = [
 ]
 
 [[package]]
-name = "fsio"
+name = "generic-array"
-version = "0.4.0"
+version = "1.1.1"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "dad0ce30be0cc441b325c5d705c8b613a0ca0d92b6a8953d41bd236dc09a36d0"
+checksum = "2cb8bc4c28d15ade99c7e90b219f30da4be5c88e586277e8cbe886beeb868ab2"
 dependencies = [
- "dunce",
+ "typenum",
- "rand",
-]
-
-[[package]]
-name = "getrandom"
-version = "0.2.7"
-source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "4eb1a864a501629691edf6c15a593b7a51eebaa1e8468e9ddc623de7c9b58ec6"
-dependencies = [
- "cfg-if",
- "libc",
- "wasi",
 ]
 
 [[package]]
@@ -858,15 +876,6 @@ dependencies = [
  "pkg-config",
 ]
 
-[[package]]
-name = "os_str_bytes"
-version = "7.0.0"
-source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "7ac44c994af577c799b1b4bd80dc214701e349873ad894d6cdf96f4f7526e0b9"
-dependencies = [
- "memchr",
-]
-
 [[package]]
 name = "parking_lot"
 version = "0.12.1"
@@ -930,12 +939,6 @@ version = "0.2.0"
 source = "registry+https://github.com/rust-lang/crates.io-index"
 checksum = "439ee305def115ba05938db6eb1644ff94165c5ab5e9420d1c1bcedbba909391"
 
-[[package]]
-name = "ppv-lite86"
-version = "0.2.17"
-source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "5b40af805b3121feab8a3c29f04d8ad262fa8e0561883e7653e024ae4479e6de"
-
 [[package]]
 name = "predicates"
 version = "3.1.3"
@@ -993,36 +996,6 @@ dependencies = [
  "proc-macro2",
 ]
 
-[[package]]
-name = "rand"
-version = "0.8.5"
-source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "34af8d1a0e25924bc5b7c43c079c942339d8f0a8b57c39049bef581b46327404"
-dependencies = [
- "libc",
- "rand_chacha",
- "rand_core",
-]
-
-[[package]]
-name = "rand_chacha"
-version = "0.3.1"
-source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "e6c10a63a0fa32252be49d21e7709d4d4baf8d231c2dbce1eaa8141b9b127d88"
-dependencies = [
- "ppv-lite86",
- "rand_core",
-]
-
-[[package]]
-name = "rand_core"
-version = "0.6.4"
-source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "ec0be4795e2f6a28069bec0b5ff3e2ac9bafc99e6a9a7dc3547996c5c816922c"
-dependencies = [
- "getrandom",
-]
-
 [[package]]
 name = "redox_syscall"
 version = "0.2.16"
@@ -1079,15 +1052,6 @@ dependencies = [
  "bytemuck",
 ]
 
-[[package]]
-name = "run_script"
-version = "0.10.1"
-source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "829f98fdc58d78989dd9af83be28bc15c94a7d77f9ecdb54abbbc0b1829ba9c7"
-dependencies = [
- "fsio",
-]
-
 [[package]]
 name = "rustix"
 version = "0.38.21"
@@ -1471,6 +1435,12 @@ dependencies = [
  "winnow",
 ]
 
+[[package]]
+name = "typenum"
+version = "1.17.0"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "42ff0bf0c66b8238c6f3b578df37d0b7848e55df8577b3f74f92a69acceeb825"
+
 [[package]]
 name = "unicode-bidi"
 version = "0.3.18"

Cargo.toml

@@ -33,7 +33,7 @@ minimal-application = [
 ]
 git = ["git2"] # Support indicating git modifications
 paging = ["shell-words", "grep-cli"] # Support applying a pager on the output
-lessopen = ["run_script", "os_str_bytes/conversions"] # Support $LESSOPEN preprocessor
+lessopen = ["execute"] # Support $LESSOPEN preprocessor
 build-assets = ["syntect/yaml-load", "syntect/plist-load", "regex", "walkdir"]
 
 # You need to use one of these if you depend on bat as a library:
@@ -66,8 +66,7 @@ regex = { version = "1.10.6", optional = true }
 walkdir = { version = "2.5", optional = true }
 bytesize = { version = "1.3.0" }
 encoding_rs = "0.8.35"
-os_str_bytes = { version = "~7.0", optional = true }
+execute = { version = "0.2.13", optional = true }
-run_script = { version = "^0.10.1", optional = true}
 terminal-colorsaurus = "0.4"
 
 [dependencies.git2]

src/lessopen.rs

@@ -3,13 +3,12 @@
 use std::convert::TryFrom;
 use std::env;
 use std::fs::File;
-use std::io::{BufRead, BufReader, Cursor, Read, Write};
+use std::io::{BufRead, BufReader, Cursor, Read};
 use std::path::PathBuf;
-use std::str;
+use std::process::{ExitStatus, Stdio};
 
 use clircle::{Clircle, Identifier};
-use os_str_bytes::RawOsString;
+use execute::{shell, Execute};
-use run_script::{IoOptions, ScriptOptions};
 
 use crate::error::Result;
 use crate::{
@@ -21,7 +20,6 @@ use crate::{
 pub(crate) struct LessOpenPreprocessor {
     lessopen: String,
     lessclose: Option<String>,
-    command_options: ScriptOptions,
     kind: LessOpenKind,
     /// Whether or not data piped via stdin is to be preprocessed
     preprocess_stdin: bool,
@@ -52,7 +50,7 @@ impl LessOpenPreprocessor {
         // Otherwise, if output is empty and exit code is nonzero, use original file contents
         let (kind, lessopen) = if lessopen.starts_with("||") {
             (LessOpenKind::Piped, lessopen.chars().skip(2).collect())
-        // "|" means pipe, but ignore exit code, always using preprocessor output
+        // "|" means pipe as above, but ignore exit code and always use preprocessor output even if empty
         } else if lessopen.starts_with('|') {
             (
                 LessOpenKind::PipedIgnoreExitCode,
@@ -70,16 +68,9 @@ impl LessOpenPreprocessor {
             (false, lessopen)
         };
 
-        let mut command_options = ScriptOptions::new();
-        command_options.runner = env::var("SHELL").ok();
-        command_options.input_redirection = IoOptions::Pipe;
-
         Ok(Self {
-            lessopen: lessopen.replacen("%s", "$1", 1),
+            lessopen,
-            lessclose: env::var("LESSCLOSE")
+            lessclose: env::var("LESSCLOSE").ok(),
-                .ok()
-                .map(|str| str.replacen("%s", "$1", 1).replacen("%s", "$2", 1)),
-            command_options,
             kind,
             preprocess_stdin: stdin,
         })
@@ -98,21 +89,21 @@ impl LessOpenPreprocessor {
                     None => return input.open(stdin, stdout_identifier),
                 };
 
-                let (exit_code, lessopen_stdout, _) = match run_script::run(
+                let mut lessopen_command = shell(self.lessopen.replacen("%s", path_str, 1));
-                    &self.lessopen,
+                lessopen_command.stdout(Stdio::piped());
-                    &vec![path_str.to_string()],
+
-                    &self.command_options,
+                let lessopen_output = match lessopen_command.execute_output() {
-                ) {
                     Ok(output) => output,
                     Err(_) => return input.open(stdin, stdout_identifier),
                 };
 
-                if self.fall_back_to_original_file(&lessopen_stdout, exit_code) {
+                if self.fall_back_to_original_file(&lessopen_output.stdout, lessopen_output.status)
+                {
                     return input.open(stdin, stdout_identifier);
                 }
 
                 (
-                    RawOsString::new(lessopen_stdout),
+                    lessopen_output.stdout,
                     path_str.to_string(),
                     OpenedInputKind::OrdinaryFile(path.to_path_buf()),
                 )
@@ -127,47 +118,31 @@ impl LessOpenPreprocessor {
                         }
                     }
 
-                    // stdin isn't Clone, so copy it to a cloneable buffer
+                    // stdin isn't Clone or AsRef<[u8]>, so move it into a cloneable buffer
+                    // so the data can be used multiple times if necessary
+                    // NOTE: stdin will be empty from this point onwards
                     let mut stdin_buffer = Vec::new();
-                    stdin.read_to_end(&mut stdin_buffer).unwrap();
+                    stdin.read_to_end(&mut stdin_buffer)?;
 
-                    let mut lessopen_handle = match run_script::spawn(
+                    let mut lessopen_command = shell(self.lessopen.replacen("%s", "-", 1));
-                        &self.lessopen,
+                    lessopen_command.stdout(Stdio::piped());
-                        &vec!["-".to_string()],
-                        &self.command_options,
-                    ) {
-                        Ok(handle) => handle,
-                        Err(_) => {
-                            return input.open(stdin, stdout_identifier);
-                        }
-                    };
 
-                    if lessopen_handle
+                    let lessopen_output = match lessopen_command.execute_input_output(&stdin_buffer)
-                        .stdin
-                        .as_mut()
-                        .unwrap()
-                        .write_all(&stdin_buffer.clone())
-                        .is_err()
                     {
-                        return input.open(stdin, stdout_identifier);
-                    }
-
-                    let lessopen_output = match lessopen_handle.wait_with_output() {
                         Ok(output) => output,
                         Err(_) => {
                             return input.open(Cursor::new(stdin_buffer), stdout_identifier);
                         }
                     };
 
-                    if lessopen_output.stdout.is_empty()
+                    if self
-                        && (!lessopen_output.status.success()
+                        .fall_back_to_original_file(&lessopen_output.stdout, lessopen_output.status)
-                            || matches!(self.kind, LessOpenKind::PipedIgnoreExitCode))
                     {
                         return input.open(Cursor::new(stdin_buffer), stdout_identifier);
                     }
 
                     (
-                        RawOsString::assert_from_raw_vec(lessopen_output.stdout),
+                        lessopen_output.stdout,
                         "-".to_string(),
                         OpenedInputKind::StdIn,
                     )
@@ -184,13 +159,17 @@ impl LessOpenPreprocessor {
             kind,
             reader: InputReader::new(BufReader::new(
                 if matches!(self.kind, LessOpenKind::TempFile) {
-                    // Remove newline at end of temporary file path returned by $LESSOPEN
+                    let lessopen_string = match String::from_utf8(lessopen_stdout) {
-                    let stdout = match lessopen_stdout.strip_suffix("\n") {
+                        Ok(string) => string,
-                        Some(stripped) => stripped.to_owned(),
+                        Err(_) => {
-                        None => lessopen_stdout,
+                            return input.open(stdin, stdout_identifier);
+                        }
+                    };
+                    // Remove newline at end of temporary file path returned by $LESSOPEN
+                    let stdout = match lessopen_string.strip_suffix("\n") {
+                        Some(stripped) => stripped.to_owned(),
+                        None => lessopen_string,
                     };
-
-                    let stdout = stdout.into_os_string();
 
                     let file = match File::open(PathBuf::from(&stdout)) {
                         Ok(file) => file,
@@ -201,16 +180,18 @@ impl LessOpenPreprocessor {
 
                     Preprocessed {
                         kind: PreprocessedKind::TempFile(file),
-                        lessclose: self.lessclose.clone(),
+                        lessclose: self
-                        command_args: vec![path_str, stdout.to_str().unwrap().to_string()],
+                            .lessclose
-                        command_options: self.command_options.clone(),
+                            .as_ref()
+                            .map(|s| s.replacen("%s", &path_str, 1).replacen("%s", &stdout, 1)),
                     }
                 } else {
                     Preprocessed {
-                        kind: PreprocessedKind::Piped(Cursor::new(lessopen_stdout.into_raw_vec())),
+                        kind: PreprocessedKind::Piped(Cursor::new(lessopen_stdout)),
-                        lessclose: self.lessclose.clone(),
+                        lessclose: self
-                        command_args: vec![path_str, "-".to_string()],
+                            .lessclose
-                        command_options: self.command_options.clone(),
+                            .as_ref()
+                            .map(|s| s.replacen("%s", &path_str, 1).replacen("%s", "-", 1)),
                     }
                 },
             )),
@@ -219,9 +200,9 @@ impl LessOpenPreprocessor {
         })
     }
 
-    fn fall_back_to_original_file(&self, lessopen_output: &str, exit_code: i32) -> bool {
+    fn fall_back_to_original_file(&self, lessopen_stdout: &Vec<u8>, exit_code: ExitStatus) -> bool {
-        lessopen_output.is_empty()
+        lessopen_stdout.is_empty()
-            && (exit_code != 0 || matches!(self.kind, LessOpenKind::PipedIgnoreExitCode))
+            && (!exit_code.success() || matches!(self.kind, LessOpenKind::PipedIgnoreExitCode))
     }
 
     #[cfg(test)]
@@ -261,8 +242,6 @@ impl Read for PreprocessedKind {
 pub struct Preprocessed {
     kind: PreprocessedKind,
     lessclose: Option<String>,
-    command_args: Vec<String>,
-    command_options: ScriptOptions,
 }
 
 impl Read for Preprocessed {
@@ -273,11 +252,20 @@ impl Read for Preprocessed {
 
 impl Drop for Preprocessed {
     fn drop(&mut self) {
-        if let Some(ref command) = self.lessclose {
+        if let Some(lessclose) = self.lessclose.clone() {
-            self.command_options.output_redirection = IoOptions::Inherit;
+            let mut lessclose_command = shell(lessclose);
 
-            run_script::run(command, &self.command_args, &self.command_options)
+            let lessclose_output = match lessclose_command.execute_output() {
-                .expect("failed to run $LESSCLOSE to clean up file");
+                Ok(output) => output,
+                Err(_) => {
+                    bat_warning!("failed to run $LESSCLOSE to clean up temporary file");
+                    return;
+                }
+            };
+
+            if lessclose_output.status.success() {
+                bat_warning!("$LESSCLOSE exited with nonzero exit code",)
+            };
         }
     }
 }
@@ -301,7 +289,7 @@ mod tests {
     fn test_just_lessopen() -> Result<()> {
         let preprocessor = LessOpenPreprocessor::mock_new(Some("|batpipe %s"), None)?;
 
-        assert_eq!(preprocessor.lessopen, "batpipe $1");
+        assert_eq!(preprocessor.lessopen, "batpipe %s");
         assert!(preprocessor.lessclose.is_none());
 
         reset_env_vars();
@@ -327,8 +315,8 @@ mod tests {
         let preprocessor =
             LessOpenPreprocessor::mock_new(Some("lessopen.sh %s"), Some("lessclose.sh %s %s"))?;
 
-        assert_eq!(preprocessor.lessopen, "lessopen.sh $1");
+        assert_eq!(preprocessor.lessopen, "lessopen.sh %s");
-        assert_eq!(preprocessor.lessclose.unwrap(), "lessclose.sh $1 $2");
+        assert_eq!(preprocessor.lessclose.unwrap(), "lessclose.sh %s %s");
 
         reset_env_vars();
 
@@ -340,13 +328,13 @@ mod tests {
     fn test_lessopen_prefixes() -> Result<()> {
         let preprocessor = LessOpenPreprocessor::mock_new(Some("batpipe %s"), None)?;
 
-        assert_eq!(preprocessor.lessopen, "batpipe $1");
+        assert_eq!(preprocessor.lessopen, "batpipe %s");
         assert!(matches!(preprocessor.kind, LessOpenKind::TempFile));
         assert!(!preprocessor.preprocess_stdin);
 
         let preprocessor = LessOpenPreprocessor::mock_new(Some("|batpipe %s"), None)?;
 
-        assert_eq!(preprocessor.lessopen, "batpipe $1");
+        assert_eq!(preprocessor.lessopen, "batpipe %s");
         assert!(matches!(
             preprocessor.kind,
             LessOpenKind::PipedIgnoreExitCode
@@ -355,19 +343,19 @@ mod tests {
 
         let preprocessor = LessOpenPreprocessor::mock_new(Some("||batpipe %s"), None)?;
 
-        assert_eq!(preprocessor.lessopen, "batpipe $1");
+        assert_eq!(preprocessor.lessopen, "batpipe %s");
         assert!(matches!(preprocessor.kind, LessOpenKind::Piped));
         assert!(!preprocessor.preprocess_stdin);
 
         let preprocessor = LessOpenPreprocessor::mock_new(Some("-batpipe %s"), None)?;
 
-        assert_eq!(preprocessor.lessopen, "batpipe $1");
+        assert_eq!(preprocessor.lessopen, "batpipe %s");
         assert!(matches!(preprocessor.kind, LessOpenKind::TempFile));
         assert!(preprocessor.preprocess_stdin);
 
         let preprocessor = LessOpenPreprocessor::mock_new(Some("|-batpipe %s"), None)?;
 
-        assert_eq!(preprocessor.lessopen, "batpipe $1");
+        assert_eq!(preprocessor.lessopen, "batpipe %s");
         assert!(matches!(
             preprocessor.kind,
             LessOpenKind::PipedIgnoreExitCode
@@ -376,7 +364,7 @@ mod tests {
 
         let preprocessor = LessOpenPreprocessor::mock_new(Some("||-batpipe %s"), None)?;
 
-        assert_eq!(preprocessor.lessopen, "batpipe $1");
+        assert_eq!(preprocessor.lessopen, "batpipe %s");
         assert!(matches!(preprocessor.kind, LessOpenKind::Piped));
         assert!(preprocessor.preprocess_stdin);
 
@@ -391,8 +379,8 @@ mod tests {
         let preprocessor =
             LessOpenPreprocessor::mock_new(Some("|echo File:%s"), Some("echo File:%s Temp:%s"))?;
 
-        assert_eq!(preprocessor.lessopen, "echo File:$1");
+        assert_eq!(preprocessor.lessopen, "echo File:%s");
-        assert_eq!(preprocessor.lessclose.unwrap(), "echo File:$1 Temp:$2");
+        assert_eq!(preprocessor.lessclose.unwrap(), "echo File:%s Temp:%s");
 
         reset_env_vars();
 

tests/integration_tests.rs

@@ -313,13 +313,19 @@ fn list_themes_to_piped_output() {
 }
 
 #[test]
-#[cfg_attr(any(not(feature = "git"), target_os = "windows"), ignore)]
+#[cfg_attr(
+    any(not(feature = "git"), feature = "lessopen", target_os = "windows"),
+    ignore
+)]
 fn short_help() {
     test_help("-h", "../doc/short-help.txt");
 }
 
 #[test]
-#[cfg_attr(any(not(feature = "git"), target_os = "windows"), ignore)]
+#[cfg_attr(
+    any(not(feature = "git"), feature = "lessopen", target_os = "windows"),
+    ignore
+)]
 fn long_help() {
     test_help("--help", "../doc/long-help.txt");
 }
@@ -2437,7 +2443,6 @@ fn lessopen_stdin_piped() {
 #[cfg(unix)] // Expected output assumed that tests are run on a Unix-like system
 #[cfg(feature = "lessopen")]
 #[test]
-#[serial] // Randomly fails otherwise
 fn lessopen_and_lessclose_file_temp() {
     // This is mainly to test that $LESSCLOSE gets passed the correct file paths
     // In this case, the original file and the temporary file returned by $LESSOPEN
@@ -2455,7 +2460,6 @@ fn lessopen_and_lessclose_file_temp() {
 #[cfg(unix)] // Expected output assumed that tests are run on a Unix-like system
 #[cfg(feature = "lessopen")]
 #[test]
-#[serial] // Randomly fails otherwise
 fn lessopen_and_lessclose_file_piped() {
     // This is mainly to test that $LESSCLOSE gets passed the correct file paths
     // In these cases, the original file and a dash
@@ -2482,8 +2486,6 @@ fn lessopen_and_lessclose_file_piped() {
 #[cfg(unix)] // Expected output assumed that tests are run on a Unix-like system
 #[cfg(feature = "lessopen")]
 #[test]
-#[serial] // Randomly fails otherwise
-#[ignore = "randomly failing on some systems"]
 fn lessopen_and_lessclose_stdin_temp() {
     // This is mainly to test that $LESSCLOSE gets passed the correct file paths
     // In this case, a dash and the temporary file returned by $LESSOPEN
@@ -2501,7 +2503,6 @@ fn lessopen_and_lessclose_stdin_temp() {
 #[cfg(unix)] // Expected output assumed that tests are run on a Unix-like system
 #[cfg(feature = "lessopen")]
 #[test]
-#[serial] // Randomly fails otherwise
 fn lessopen_and_lessclose_stdin_piped() {
     // This is mainly to test that $LESSCLOSE gets passed the correct file paths
     // In these cases, two dashes