From 0a6938e5c0b13f454ddc85dcf60043b7226f7f83 Mon Sep 17 00:00:00 2001 From: Tom Hale Date: Sat, 9 May 2026 22:53:39 +0700 Subject: [PATCH 1/5] fix(paging): skip pager when stdout is piped MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Delta was starting a pager (less) even when stdout was not a terminal, causing LESSOPEN=highlight-less-wrapper to emit ANSI escape sequences (\x1b[00;39m) on empty stdin input. This fixes issue #2151 where  produced a phantom blank line due to the pager chain emitting an ANSI reset. Changes: - In main.rs: when stdout is not a terminal and paging_mode is not explicitly 'always', force PagingMode::Never - Added tests that spawn delta as subprocess to verify zero output on empty stdin when stdout is piped Fixes #2151 --- REVIEW-2026-05-09T22:12:15+07:00.md | 63 +++++++++++++++++++++++ src/main.rs | 78 +++++++++++++++++++++++++++++ 2 files changed, 141 insertions(+) create mode 100644 REVIEW-2026-05-09T22:12:15+07:00.md diff --git a/REVIEW-2026-05-09T22:12:15+07:00.md b/REVIEW-2026-05-09T22:12:15+07:00.md new file mode 100644 index 000000000..ae47a3150 --- /dev/null +++ b/REVIEW-2026-05-09T22:12:15+07:00.md @@ -0,0 +1,63 @@ +# Production-ready bar for this PR + +1. **Empty stdin produces zero output** — `printf '' | delta | wc -c` must be 0, no phantom blank line. +2. **Normal diff pipeline unchanged** — `git diff | delta` must still work identically (pager still starts when stdout is a terminal). +3. **`--paging=always` overrides terminal check** — user's explicit `--paging=always` is still honored even with piped stdout. +4. **No regression in existing tests** — the 434 passing tests must still pass. +5. **ANSI escape sequences not emitted spuriously** — the fix must not introduce new ANSI output on empty stdin or normal diff pipelines. +6. **Terminal detection path correct** — `io::stdout().is_terminal()` is the right check for paging decision. +7. **Changes minimal and scoped** — only the paging decision is touched, no unrelated refactoring. + +# Findings + +## 1. Correctness & functional completeness + +- No issues found in this area based on the diff and reviewed context. + +## 2. Architecture & boundary integrity + +- No issues found in this area based on the diff and reviewed context. + +## 3. Code clarity, clean code & maintainability + +- No issues found in this area based on the diff and reviewed context. + +## 4. Comments & code documentation + +- No issues found in this area based on the diff and reviewed context. + +## 5. Tests & validation + +### [RESOLVED] No test coverage for the new terminal-detection branch +- Type: Unverified concern +- Evidence: Test infrastructure uses `capture_output=Some(...)` which takes the `PagingMode::Capture` path at `src/main.rs:152`. The new `else if` at `src/main.rs:153` is never exercised in tests. +- Why it matters: A future refactor of the paging logic could break this path without detection. +- Recommendation: Add a test that runs delta with piped stdin and piped stdout and asserts zero output. This can be done via a shell-based integration test or by refactoring `run_app` to accept a flag for testing. +- Resolution: Added `main_tests::test_no_output_on_empty_stdin_when_stdout_piped` and `main_tests::test_empty_stdin_does_not_hang_with_paging_always` to `src/main.rs` (lines 311+). These spawn the delta binary as a subprocess with piped stdin/stdout, exercising the terminal-detection branch directly. Both tests pass. +- Confidence: Low → High (resolved) + +## 6. Performance + +- No issues found in this area based on the diff and reviewed context. + +## 7. Operational risk + +### [NON-BLOCKING] `--paging=always` + piped stdout still affected by LESSOPEN +- Type: Plausible risk +- Evidence: `src/main.rs:153` +- Why it matters: If a user explicitly sets `--paging=always` and pipes output to another process, the pager still starts. The `LESSOPEN=highlight-less-wrapper` pipeline could still emit the ANSI escape on empty stdin. This is the same behavior as before the fix — not a regression. The `Always` path is an explicit user opt-in. +- Recommendation: No action needed. Document in the issue that `--paging=never` is recommended when piping delta output. +- Confidence: Medium + +## 8. Adversarial review + +- No issues found in this area based on the diff and reviewed context. + +# What I could not fully verify + +- The fix's behavior on Windows or other non-Linux platforms. `io::stdout().is_terminal()` is the standard Rust `IsTerminal` trait which is supported on all tier-1 platforms, so this should be equivalent to the Linux behavior, but I could not run on Windows. +- The interaction with all possible `LESSOPEN` or pager wrapper configurations beyond the one in this environment (`highlight-less-wrapper`). + +# Final verdict + +✅ Ready to merge — no blocking issues. diff --git a/src/main.rs b/src/main.rs index e952a3bcb..d2e882891 100644 --- a/src/main.rs +++ b/src/main.rs @@ -150,6 +150,8 @@ pub fn run_app( let pager_cfg = (&config).into(); let paging_mode = if capture_output.is_some() { PagingMode::Capture + } else if config.paging_mode != PagingMode::Always && !io::stdout().is_terminal() { + PagingMode::Never } else { config.paging_mode }; @@ -303,3 +305,79 @@ pub fn run_app( // `output_type` drop impl runs here } + +#[cfg(test)] +mod main_tests { + use std::io::Write; + use std::process::{Command, Stdio}; + + /// Locate the delta binary in the cargo target directory. + /// During `cargo test`, the test binary is at target/debug/deps/git_delta-, + /// and the main binary is at target/debug/delta. + fn delta_binary() -> Option { + let test_bin = std::env::current_exe().ok()?; + let mut path = test_bin; + path.pop(); // deps/ + path.pop(); // debug/ or release/ + path.push("delta"); + if path.exists() { Some(path) } else { None } + } + + #[test] + fn test_no_output_on_empty_stdin_when_stdout_piped() { + let delta_bin = delta_binary().expect( + "delta binary not found at target/debug/delta; run `cargo build --bin delta` first" + ); + + let mut child = Command::new(&delta_bin) + .arg("--no-gitconfig") + .stdin(Stdio::piped()) + .stdout(Stdio::piped()) + .stderr(Stdio::null()) + .spawn() + .expect("failed to spawn delta"); + + child.stdin.take().unwrap().write_all(b"").unwrap(); + let output = child.wait_with_output().unwrap(); + + assert!( + output.status.success(), + "delta exited with error on empty stdin: {}", + String::from_utf8_lossy(&output.stderr) + ); + assert_eq!( + output.stdout.len(), + 0, + "Expected 0 bytes on empty stdin piped to delta, got {} bytes: {:?}", + output.stdout.len(), + String::from_utf8_lossy(&output.stdout) + ); + } + + #[test] + fn test_empty_stdin_does_not_hang_with_paging_always() { + let delta_bin = delta_binary().expect( + "delta binary not found at target/debug/delta; run `cargo build --bin delta` first" + ); + + let mut child = Command::new(&delta_bin) + .arg("--no-gitconfig") + .arg("--paging=always") + .stdin(Stdio::piped()) + .stdout(Stdio::piped()) + .stderr(Stdio::null()) + .spawn() + .expect("failed to spawn delta"); + + child.stdin.take().unwrap().write_all(b"").unwrap(); + let output = child.wait_with_output().unwrap(); + + // --paging=always still starts the pager even when stdout is piped. + // The test verifies this doesn't hang and exits cleanly. + assert!( + output.status.success(), + "delta --paging=always exited with error on empty stdin: {}", + String::from_utf8_lossy(&output.stderr) + ); + } +} From 3dc15fef99082eb31d503d353b4341b87af49765 Mon Sep 17 00:00:00 2001 From: Tom Hale Date: Sat, 9 May 2026 23:47:45 +0700 Subject: [PATCH 2/5] fixup: address Copilot review feedback (PR #2152) - Move subprocess tests from src/main.rs to tests/empty_stdin.rs using CARGO_BIN_EXE_delta for cross-platform binary discovery - Set --pager=cat in paging_always test to prevent hang in CI - Remove review artefact REVIEW-*.md from PR --- REVIEW-2026-05-09T22:12:15+07:00.md | 63 ------------------------ src/main.rs | 75 ----------------------------- tests/empty_stdin.rs | 58 ++++++++++++++++++++++ 3 files changed, 58 insertions(+), 138 deletions(-) delete mode 100644 REVIEW-2026-05-09T22:12:15+07:00.md create mode 100644 tests/empty_stdin.rs diff --git a/REVIEW-2026-05-09T22:12:15+07:00.md b/REVIEW-2026-05-09T22:12:15+07:00.md deleted file mode 100644 index ae47a3150..000000000 --- a/REVIEW-2026-05-09T22:12:15+07:00.md +++ /dev/null @@ -1,63 +0,0 @@ -# Production-ready bar for this PR - -1. **Empty stdin produces zero output** — `printf '' | delta | wc -c` must be 0, no phantom blank line. -2. **Normal diff pipeline unchanged** — `git diff | delta` must still work identically (pager still starts when stdout is a terminal). -3. **`--paging=always` overrides terminal check** — user's explicit `--paging=always` is still honored even with piped stdout. -4. **No regression in existing tests** — the 434 passing tests must still pass. -5. **ANSI escape sequences not emitted spuriously** — the fix must not introduce new ANSI output on empty stdin or normal diff pipelines. -6. **Terminal detection path correct** — `io::stdout().is_terminal()` is the right check for paging decision. -7. **Changes minimal and scoped** — only the paging decision is touched, no unrelated refactoring. - -# Findings - -## 1. Correctness & functional completeness - -- No issues found in this area based on the diff and reviewed context. - -## 2. Architecture & boundary integrity - -- No issues found in this area based on the diff and reviewed context. - -## 3. Code clarity, clean code & maintainability - -- No issues found in this area based on the diff and reviewed context. - -## 4. Comments & code documentation - -- No issues found in this area based on the diff and reviewed context. - -## 5. Tests & validation - -### [RESOLVED] No test coverage for the new terminal-detection branch -- Type: Unverified concern -- Evidence: Test infrastructure uses `capture_output=Some(...)` which takes the `PagingMode::Capture` path at `src/main.rs:152`. The new `else if` at `src/main.rs:153` is never exercised in tests. -- Why it matters: A future refactor of the paging logic could break this path without detection. -- Recommendation: Add a test that runs delta with piped stdin and piped stdout and asserts zero output. This can be done via a shell-based integration test or by refactoring `run_app` to accept a flag for testing. -- Resolution: Added `main_tests::test_no_output_on_empty_stdin_when_stdout_piped` and `main_tests::test_empty_stdin_does_not_hang_with_paging_always` to `src/main.rs` (lines 311+). These spawn the delta binary as a subprocess with piped stdin/stdout, exercising the terminal-detection branch directly. Both tests pass. -- Confidence: Low → High (resolved) - -## 6. Performance - -- No issues found in this area based on the diff and reviewed context. - -## 7. Operational risk - -### [NON-BLOCKING] `--paging=always` + piped stdout still affected by LESSOPEN -- Type: Plausible risk -- Evidence: `src/main.rs:153` -- Why it matters: If a user explicitly sets `--paging=always` and pipes output to another process, the pager still starts. The `LESSOPEN=highlight-less-wrapper` pipeline could still emit the ANSI escape on empty stdin. This is the same behavior as before the fix — not a regression. The `Always` path is an explicit user opt-in. -- Recommendation: No action needed. Document in the issue that `--paging=never` is recommended when piping delta output. -- Confidence: Medium - -## 8. Adversarial review - -- No issues found in this area based on the diff and reviewed context. - -# What I could not fully verify - -- The fix's behavior on Windows or other non-Linux platforms. `io::stdout().is_terminal()` is the standard Rust `IsTerminal` trait which is supported on all tier-1 platforms, so this should be equivalent to the Linux behavior, but I could not run on Windows. -- The interaction with all possible `LESSOPEN` or pager wrapper configurations beyond the one in this environment (`highlight-less-wrapper`). - -# Final verdict - -✅ Ready to merge — no blocking issues. diff --git a/src/main.rs b/src/main.rs index d2e882891..3b0e621c0 100644 --- a/src/main.rs +++ b/src/main.rs @@ -306,78 +306,3 @@ pub fn run_app( // `output_type` drop impl runs here } -#[cfg(test)] -mod main_tests { - use std::io::Write; - use std::process::{Command, Stdio}; - - /// Locate the delta binary in the cargo target directory. - /// During `cargo test`, the test binary is at target/debug/deps/git_delta-, - /// and the main binary is at target/debug/delta. - fn delta_binary() -> Option { - let test_bin = std::env::current_exe().ok()?; - let mut path = test_bin; - path.pop(); // deps/ - path.pop(); // debug/ or release/ - path.push("delta"); - if path.exists() { Some(path) } else { None } - } - - #[test] - fn test_no_output_on_empty_stdin_when_stdout_piped() { - let delta_bin = delta_binary().expect( - "delta binary not found at target/debug/delta; run `cargo build --bin delta` first" - ); - - let mut child = Command::new(&delta_bin) - .arg("--no-gitconfig") - .stdin(Stdio::piped()) - .stdout(Stdio::piped()) - .stderr(Stdio::null()) - .spawn() - .expect("failed to spawn delta"); - - child.stdin.take().unwrap().write_all(b"").unwrap(); - let output = child.wait_with_output().unwrap(); - - assert!( - output.status.success(), - "delta exited with error on empty stdin: {}", - String::from_utf8_lossy(&output.stderr) - ); - assert_eq!( - output.stdout.len(), - 0, - "Expected 0 bytes on empty stdin piped to delta, got {} bytes: {:?}", - output.stdout.len(), - String::from_utf8_lossy(&output.stdout) - ); - } - - #[test] - fn test_empty_stdin_does_not_hang_with_paging_always() { - let delta_bin = delta_binary().expect( - "delta binary not found at target/debug/delta; run `cargo build --bin delta` first" - ); - - let mut child = Command::new(&delta_bin) - .arg("--no-gitconfig") - .arg("--paging=always") - .stdin(Stdio::piped()) - .stdout(Stdio::piped()) - .stderr(Stdio::null()) - .spawn() - .expect("failed to spawn delta"); - - child.stdin.take().unwrap().write_all(b"").unwrap(); - let output = child.wait_with_output().unwrap(); - - // --paging=always still starts the pager even when stdout is piped. - // The test verifies this doesn't hang and exits cleanly. - assert!( - output.status.success(), - "delta --paging=always exited with error on empty stdin: {}", - String::from_utf8_lossy(&output.stderr) - ); - } -} diff --git a/tests/empty_stdin.rs b/tests/empty_stdin.rs new file mode 100644 index 000000000..fb87be859 --- /dev/null +++ b/tests/empty_stdin.rs @@ -0,0 +1,58 @@ +use std::io::Write; +use std::process::{Command, Stdio}; + +fn delta_bin() -> String { + std::env::var("CARGO_BIN_EXE_delta").unwrap_or_else(|_| { + // Fallback for when not run via cargo test + format!("{}/target/debug/delta", env!("CARGO_MANIFEST_DIR")) + }) +} + +#[test] +fn test_no_output_on_empty_stdin_when_stdout_piped() { + let mut child = Command::new(delta_bin()) + .args(["--no-gitconfig"]) + .stdin(Stdio::piped()) + .stdout(Stdio::piped()) + .stderr(Stdio::null()) + .spawn() + .expect("failed to spawn delta"); + + child.stdin.take().unwrap().write_all(b"").unwrap(); + let output = child.wait_with_output().unwrap(); + + assert!( + output.status.success(), + "delta exited with error on empty stdin: {}", + String::from_utf8_lossy(&output.stderr) + ); + assert_eq!( + output.stdout.len(), + 0, + "Expected 0 bytes on empty stdin piped to delta, got {} bytes: {:?}", + output.stdout.len(), + String::from_utf8_lossy(&output.stdout) + ); +} + +#[test] +fn test_empty_stdin_does_not_hang_with_paging_always() { + let mut child = Command::new(delta_bin()) + .args(["--no-gitconfig", "--paging=always", "--pager=cat"]) + .stdin(Stdio::piped()) + .stdout(Stdio::piped()) + .stderr(Stdio::null()) + .spawn() + .expect("failed to spawn delta"); + + child.stdin.take().unwrap().write_all(b"").unwrap(); + let output = child.wait_with_output().unwrap(); + + // --paging=always still starts the pager even when stdout is piped. + // Using --pager=cat avoids hanging on environments where less waits for a TTY. + assert!( + output.status.success(), + "delta --paging=always exited with error on empty stdin: {}", + String::from_utf8_lossy(&output.stderr) + ); +} From 5ba138cdeddc662dfc3c19382dd29363c28abf38 Mon Sep 17 00:00:00 2001 From: Tom Hale Date: Thu, 21 May 2026 12:54:52 +0700 Subject: [PATCH 3/5] fix: address Copilot review feedback on tests/empty_stdin.rs - Use Stdio::piped() for stderr so assertion messages are useful - Add EXE_SUFFIX to delta_bin() fallback for Windows compatibility - Use cross-platform pager: `more` on Windows, `cat` on Unix --- tests/empty_stdin.rs | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/tests/empty_stdin.rs b/tests/empty_stdin.rs index fb87be859..19da2528a 100644 --- a/tests/empty_stdin.rs +++ b/tests/empty_stdin.rs @@ -4,7 +4,9 @@ use std::process::{Command, Stdio}; fn delta_bin() -> String { std::env::var("CARGO_BIN_EXE_delta").unwrap_or_else(|_| { // Fallback for when not run via cargo test - format!("{}/target/debug/delta", env!("CARGO_MANIFEST_DIR")) + let mut path = format!("{}/target/debug/delta", env!("CARGO_MANIFEST_DIR")); + path.push_str(std::env::consts::EXE_SUFFIX); + path }) } @@ -14,7 +16,7 @@ fn test_no_output_on_empty_stdin_when_stdout_piped() { .args(["--no-gitconfig"]) .stdin(Stdio::piped()) .stdout(Stdio::piped()) - .stderr(Stdio::null()) + .stderr(Stdio::piped()) .spawn() .expect("failed to spawn delta"); @@ -37,11 +39,15 @@ fn test_no_output_on_empty_stdin_when_stdout_piped() { #[test] fn test_empty_stdin_does_not_hang_with_paging_always() { + // Use a non-interactive pager that exists on all platforms. + // On Windows, `more` is available; on Unix, `cat` is available. + let pager = if cfg!(windows) { "more" } else { "cat" }; + let mut child = Command::new(delta_bin()) - .args(["--no-gitconfig", "--paging=always", "--pager=cat"]) + .args(["--no-gitconfig", "--paging=always", &format!("--pager={}", pager)]) .stdin(Stdio::piped()) .stdout(Stdio::piped()) - .stderr(Stdio::null()) + .stderr(Stdio::piped()) .spawn() .expect("failed to spawn delta"); @@ -49,7 +55,6 @@ fn test_empty_stdin_does_not_hang_with_paging_always() { let output = child.wait_with_output().unwrap(); // --paging=always still starts the pager even when stdout is piped. - // Using --pager=cat avoids hanging on environments where less waits for a TTY. assert!( output.status.success(), "delta --paging=always exited with error on empty stdin: {}", From 36a830821d62b74fad58452b38d174f72617129f Mon Sep 17 00:00:00 2001 From: Tom Hale Date: Thu, 21 May 2026 13:03:23 +0700 Subject: [PATCH 4/5] fix: use PathBuf for cross-platform path construction in delta_bin() Addresses Copilot review feedback about string formatting being incorrect with custom target-dir, non-debug profiles, or Windows path separators. --- tests/empty_stdin.rs | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/tests/empty_stdin.rs b/tests/empty_stdin.rs index 19da2528a..af3ebda54 100644 --- a/tests/empty_stdin.rs +++ b/tests/empty_stdin.rs @@ -1,12 +1,15 @@ use std::io::Write; +use std::path::PathBuf; use std::process::{Command, Stdio}; fn delta_bin() -> String { std::env::var("CARGO_BIN_EXE_delta").unwrap_or_else(|_| { // Fallback for when not run via cargo test - let mut path = format!("{}/target/debug/delta", env!("CARGO_MANIFEST_DIR")); - path.push_str(std::env::consts::EXE_SUFFIX); - path + let mut path = PathBuf::from(env!("CARGO_MANIFEST_DIR")); + path.push("target"); + path.push("debug"); + path.push(format!("delta{}", std::env::consts::EXE_SUFFIX)); + path.to_string_lossy().into_owned() }) } From 3fad53f4cca4013ab0d90d50cf2c5127f2f9e0b3 Mon Sep 17 00:00:00 2001 From: Tom Hale Date: Thu, 21 May 2026 15:12:59 +0700 Subject: [PATCH 5/5] fix(tests): use var_os for CARGO_BIN_EXE_delta lookup CARGO_BIN_EXE_delta is set by Cargo and is always valid UTF-8 in practice, but var_os is more defensive and avoids the panic path on non-UTF8 values. Returns OsString instead of String since Command::new accepts AsRef. --- tests/empty_stdin.rs | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/tests/empty_stdin.rs b/tests/empty_stdin.rs index af3ebda54..548d8886f 100644 --- a/tests/empty_stdin.rs +++ b/tests/empty_stdin.rs @@ -1,15 +1,16 @@ +use std::ffi::OsString; use std::io::Write; use std::path::PathBuf; use std::process::{Command, Stdio}; -fn delta_bin() -> String { - std::env::var("CARGO_BIN_EXE_delta").unwrap_or_else(|_| { +fn delta_bin() -> OsString { + std::env::var_os("CARGO_BIN_EXE_delta").unwrap_or_else(|| { // Fallback for when not run via cargo test let mut path = PathBuf::from(env!("CARGO_MANIFEST_DIR")); path.push("target"); path.push("debug"); path.push(format!("delta{}", std::env::consts::EXE_SUFFIX)); - path.to_string_lossy().into_owned() + path.into_os_string() }) }