From 26e4b26ee54af82615f31c14cbfabf78d7d01484 Mon Sep 17 00:00:00 2001 From: Brian McKelvey Date: Wed, 3 Sep 2025 23:38:15 -0700 Subject: [PATCH] fix: preserve arguments in complex PAGER commands Previously, delta used bat::config::get_pager_executable() which would strip arguments from complex PAGER commands like '/bin/sh -c "head -10000 | cat"', leaving only the executable path. This caused "command not found" errors when the pager tried to execute. This commit: - Reimplements bat's pager detection logic to preserve full PAGER commands - Adds comprehensive test coverage for various pager scenarios - Introduces EnvVarGuard utility for safe environment variable testing - Maintains compatibility with problematic pagers (more, most) by replacing them with 'less' - Preserves DELTA_PAGER precedence over PAGER The fix ensures that complex shell commands used as pagers work correctly while maintaining all existing behavior for standard pagers. Fixes issue where complex PAGER environment variables would fail with "command not found" errors. --- src/env.rs | 141 +++++++++++++++++++++++++--- src/tests/integration_test_utils.rs | 69 ++++++++++++++ src/tests/mod.rs | 1 + src/tests/test_pager_integration.rs | 57 +++++++++++ 4 files changed, 254 insertions(+), 14 deletions(-) create mode 100644 src/tests/test_pager_integration.rs 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 + ); +}