diff --git a/src/env.rs b/src/env.rs index a289d8433..3628e9285 100644 --- a/src/env.rs +++ b/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,20 +70,22 @@ fn hostname() -> Option { #[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> = Arc::new(Mutex::new(())); + pub static ref ENV_ACCESS: Arc> = 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()); @@ -90,9 +93,10 @@ pub mod tests { #[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 { + 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()) + } } diff --git a/src/tests/integration_test_utils.rs b/src/tests/integration_test_utils.rs index 1b2f1dfc1..c2c33d6a7 100644 --- a/src/tests/integration_test_utils.rs +++ b/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, +} + +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); + } + } } diff --git a/src/tests/mod.rs b/src/tests/mod.rs index b6633a3b2..a24f5c8e6 100644 --- a/src/tests/mod.rs +++ b/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))] diff --git a/src/tests/test_pager_integration.rs b/src/tests/test_pager_integration.rs new file mode 100644 index 000000000..d21001e13 --- /dev/null +++ b/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 + ); +}