Fix: preserve arguments in complex PAGER commands

src/env.rs

@@ -1,4 +1,5 @@
 use std::env;
+use std::path::Path;
 
 const COLORTERM: &str = "COLORTERM";
 const BAT_THEME: &str = "BAT_THEME";
@@ -40,11 +41,11 @@ impl DeltaEnv {
         let current_dir = env::current_dir().ok();
         let pagers = (
             env::var(DELTA_PAGER).ok(),
-            // We're using `bat::config::get_pager_executable` here instead of just returning
-            // the pager from the environment variables, because we want to make sure
-            // that the pager is a valid pager from env and handle the case of
-            // the PAGER being set to something invalid like "most" and "more".
-            bat::config::get_pager_executable(None),
+            // Reimplement bat's pager detection logic to preserve full PAGER commands.
+            // This fixes the bug where bat::config::get_pager_executable(None) was stripping
+            // arguments from complex PAGER commands like '/bin/sh -c "head -10000 | cat"'.
+            // We can't use bat::pager::get_pager directly because the pager module is private.
+            get_pager_from_env(),
         );
 
         Self {
@@ -69,30 +70,33 @@ fn hostname() -> Option<String> {
 #[cfg(test)]
 pub mod tests {
     use super::DeltaEnv;
+    use crate::tests::integration_test_utils::EnvVarGuard;
     use lazy_static::lazy_static;
     use std::env;
     use std::sync::{Arc, Mutex};
 
     lazy_static! {
-        static ref ENV_ACCESS: Arc<Mutex<()>> = Arc::new(Mutex::new(()));
+        pub static ref ENV_ACCESS: Arc<Mutex<()>> = Arc::new(Mutex::new(()));
     }
 
     #[test]
     fn test_env_parsing() {
-        let _guard = ENV_ACCESS.lock().unwrap();
+        let guard = ENV_ACCESS.lock().unwrap();
         let feature = "Awesome Feature";
-        env::set_var("DELTA_FEATURES", feature);
+        let _env_guard = EnvVarGuard::new("DELTA_FEATURES", feature);
         let env = DeltaEnv::init();
+        drop(guard);
         assert_eq!(env.features, Some(feature.into()));
         // otherwise `current_dir` is not used in the test cfg:
         assert_eq!(env.current_dir, env::current_dir().ok());
     }
 
     #[test]
     fn test_env_parsing_with_pager_set_to_bat() {
-        let _guard = ENV_ACCESS.lock().unwrap();
-        env::set_var("PAGER", "bat");
+        let guard = ENV_ACCESS.lock().unwrap();
+        let _env_guard = EnvVarGuard::new("PAGER", "bat");
         let env = DeltaEnv::init();
+        drop(guard);
         assert_eq!(
             env.pagers.1,
             Some("bat".into()),
@@ -103,17 +107,126 @@ pub mod tests {
 
     #[test]
     fn test_env_parsing_with_pager_set_to_more() {
-        let _guard = ENV_ACCESS.lock().unwrap();
-        env::set_var("PAGER", "more");
+        let guard = ENV_ACCESS.lock().unwrap();
+        let _env_guard = EnvVarGuard::new("PAGER", "more");
         let env = DeltaEnv::init();
+        drop(guard);
         assert_eq!(env.pagers.1, Some("less".into()));
     }
 
     #[test]
     fn test_env_parsing_with_pager_set_to_most() {
-        let _guard = ENV_ACCESS.lock().unwrap();
-        env::set_var("PAGER", "most");
+        let guard = ENV_ACCESS.lock().unwrap();
+        let _env_guard = EnvVarGuard::new("PAGER", "most");
         let env = DeltaEnv::init();
+        drop(guard);
         assert_eq!(env.pagers.1, Some("less".into()));
     }
+
+    #[test]
+    fn test_env_parsing_with_complex_shell_pager_command() {
+        // This test verifies the core bug fix: complex PAGER commands with arguments
+        // should be preserved, not stripped down to just the executable path.
+        let guard = ENV_ACCESS.lock().unwrap();
+        let _env_guard = EnvVarGuard::new("PAGER", "/bin/sh -c \"head -10000 | cat\"");
+        let env = DeltaEnv::init();
+        drop(guard);
+        assert_eq!(
+            env.pagers.1,
+            Some("/bin/sh -c \"head -10000 | cat\"".into()),
+            "Complex shell pager command should be preserved with arguments"
+        );
+    }
+
+    #[test]
+    fn test_env_parsing_with_simple_shell_pager_command() {
+        let guard = ENV_ACCESS.lock().unwrap();
+        let _env_guard = EnvVarGuard::new("PAGER", "/bin/sh -c \"cat\"");
+        let env = DeltaEnv::init();
+        drop(guard);
+        assert_eq!(
+            env.pagers.1,
+            Some("/bin/sh -c \"cat\"".into()),
+            "Simple shell pager command should be preserved with arguments"
+        );
+    }
+
+    #[test]
+    fn test_env_parsing_with_pager_arguments_preserved() {
+        // Test that pager commands with various argument styles are preserved
+        let guard = ENV_ACCESS.lock().unwrap();
+        let _env_guard = EnvVarGuard::new("PAGER", "less -R -F -X");
+        let env = DeltaEnv::init();
+        drop(guard);
+        assert_eq!(
+            env.pagers.1,
+            Some("less -R -F -X".into()),
+            "Pager arguments should be preserved"
+        );
+    }
+
+    #[test]
+    fn test_env_parsing_delta_pager_takes_precedence() {
+        // Test that DELTA_PAGER takes precedence over PAGER
+        let guard = ENV_ACCESS.lock().unwrap();
+        let _pager_guard = EnvVarGuard::new("PAGER", "cat");
+        let _delta_pager_guard = EnvVarGuard::new("DELTA_PAGER", "/bin/sh -c \"head -1 | cat\"");
+        let env = DeltaEnv::init();
+        drop(guard);
+        assert_eq!(
+            env.pagers.0,
+            Some("/bin/sh -c \"head -1 | cat\"".into()),
+            "DELTA_PAGER should be preserved exactly as set"
+        );
+        assert_eq!(
+            env.pagers.1,
+            Some("cat".into()),
+            "PAGER should also be preserved for fallback"
+        );
+    }
+}
+
+/// Get pager from environment variables using bat's logic.
+/// This reimplements bat's pager::get_pager function to preserve full PAGER commands
+/// including arguments, while still handling problematic pagers properly.
+fn get_pager_from_env() -> Option<String> {
+    let bat_pager = env::var("BAT_PAGER");
+    let pager = env::var("PAGER");
+
+    let (cmd, from_pager_env) = match (&bat_pager, &pager) {
+        (Ok(bat_pager), _) => (bat_pager.as_str(), false),
+        (_, Ok(pager)) => (pager.as_str(), true),
+        _ => ("less", false),
+    };
+
+    // Parse the command using shell_words to split into binary and arguments
+    let parts = match shell_words::split(cmd) {
+        Ok(parts) if !parts.is_empty() => parts,
+        // Fallback for malformed or empty commands
+        _ => return Some("less".to_string()),
+    };
+
+    let bin = &parts[0];
+    // Determine what kind of pager this is
+    let pager_bin = Path::new(bin).file_stem();
+    let current_bin = env::args_os().next();
+
+    let is_current_bin_pager = current_bin
+        .map(|s| Path::new(&s).file_stem() == pager_bin)
+        .unwrap_or(false);
+
+    // Only replace problematic pagers when they come from PAGER env var
+    let is_problematic_pager = from_pager_env
+        && (matches!(
+            pager_bin.map(|s| s.to_string_lossy()).as_deref(),
+            Some("more") | Some("most")
+        ) || is_current_bin_pager);
+
+    if is_problematic_pager {
+        // Replace problematic pagers with "less"
+        Some("less".to_string())
+    } else {
+        // Preserve the original command string unmodified to maintain proper quoting
+        Some(cmd.to_string())
+    }
 }

src/tests/integration_test_utils.rs

@@ -1,6 +1,7 @@
 #![cfg(test)]
 
 use std::borrow::Cow;
+use std::env;
 use std::fs::File;
 use std::io::{BufReader, Write};
 use std::path::Path;
@@ -418,4 +419,72 @@ ignored!  2
                 (green)+2(normal)",
             );
     }
+
+    #[test]
+    fn test_env_var_guard() {
+        use super::EnvVarGuard;
+        use std::env;
+
+        const TEST_VAR: &str = "DELTA_TEST_ENV_VAR";
+
+        // Ensure the test var is not set initially
+        env::remove_var(TEST_VAR);
+        assert!(env::var(TEST_VAR).is_err());
+
+        {
+            // Create a guard that sets the variable
+            let _guard = EnvVarGuard::new(TEST_VAR, "test_value");
+            assert_eq!(env::var(TEST_VAR).unwrap(), "test_value");
+        } // Guard goes out of scope here
+
+        // Variable should be removed since it wasn't set originally
+        assert!(env::var(TEST_VAR).is_err());
+
+        // Test with existing variable
+        env::set_var(TEST_VAR, "original_value");
+        assert_eq!(env::var(TEST_VAR).unwrap(), "original_value");
+
+        {
+            let _guard = EnvVarGuard::new(TEST_VAR, "modified_value");
+            assert_eq!(env::var(TEST_VAR).unwrap(), "modified_value");
+        } // Guard goes out of scope here
+
+        // Variable should be restored to original value
+        assert_eq!(env::var(TEST_VAR).unwrap(), "original_value");
+
+        // Clean up
+        env::remove_var(TEST_VAR);
+    }
+}
+
+/// RAII guard for environment variables that ensures proper cleanup
+/// even if the test panics.
+pub struct EnvVarGuard {
+    key: &'static str,
+    original_value: Option<String>,
+}
+
+impl EnvVarGuard {
+    /// Create a new environment variable guard that sets the variable
+    /// and remembers its original value for restoration.
+    pub fn new(key: &'static str, value: &str) -> Self {
+        let original_value = env::var(key).ok();
+        env::set_var(key, value);
+        Self {
+            key,
+            original_value,
+        }
+    }
+}
+
+impl Drop for EnvVarGuard {
+    /// Restore the environment variable to its original state.
+    /// If it wasn't set originally, remove it. If it was set, restore the original value.
+    fn drop(&mut self) {
+        if let Some(val) = &self.original_value {
+            env::set_var(self.key, val);
+        } else {
+            env::remove_var(self.key);
+        }
+    }
 }

src/tests/mod.rs

@@ -1,6 +1,7 @@
 pub mod ansi_test_utils;
 pub mod integration_test_utils;
 pub mod test_example_diffs;
+pub mod test_pager_integration;
 pub mod test_utils;
 
 #[cfg(not(test))]

src/tests/test_pager_integration.rs

@@ -0,0 +1,57 @@
+#![cfg(test)]
+
+use std::io::Write;
+use std::process::{Command, Stdio};
+
+use crate::tests::integration_test_utils::EnvVarGuard;
+
+#[test]
+fn test_pager_integration_with_complex_command() {
+    // This test demonstrates a bug where complex PAGER commands with arguments
+    // cause "command not found" errors because bat::config::get_pager_executable
+    // strips the arguments, leaving only the executable path.
+
+    let mut delta_cmd = {
+        // Acquire the environment access lock to prevent race conditions with other tests
+        let _lock = crate::env::tests::ENV_ACCESS.lock().unwrap();
+
+        // Use RAII guard to ensure environment variable is properly restored even if test panics
+        let _env_guard = EnvVarGuard::new("PAGER", "/bin/sh -c \"head -10000 | cat\"");
+
+        // Run delta as a subprocess with paging enabled - this will spawn the actual pager
+        Command::new("cargo")
+            .args(&["run", "--bin", "delta", "--", "--paging=always"])
+            .stdin(Stdio::piped())
+            .stdout(Stdio::piped())
+            .stderr(Stdio::piped())
+            .spawn()
+            .expect("delta to start successfully")
+    };
+
+    // Send test input to delta
+    if let Some(stdin) = delta_cmd.stdin.as_mut() {
+        stdin
+            .write_all(b"line1\nline2\nline3\nline4\nline5\n")
+            .unwrap();
+    }
+
+    // Wait for delta to complete and capture output
+    let output = delta_cmd
+        .wait_with_output()
+        .expect("delta to finish and produce output");
+
+    let stdout = String::from_utf8_lossy(&output.stdout);
+    let stderr = String::from_utf8_lossy(&output.stderr);
+
+    // The bug: when bat strips arguments from "/bin/sh -c \"head -10000 | cat\""
+    // to just "/bin/sh", the shell tries to execute each input line as a command,
+    // resulting in "command not found" errors in stderr
+    assert!(
+        !stderr.contains("command not found"),
+        "Pager integration failed: 'command not found' errors in stderr indicate that \
+         bat::config::get_pager_executable stripped arguments from the PAGER command. \
+         Stderr: {}\nStdout: {}",
+        stderr,
+        stdout
+    );
+}