fix(paging): skip pager when stdout is not a terminal

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.

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-<hash>,
+    /// and the main binary is at target/debug/delta.
+    fn delta_binary() -> Option<std::path::PathBuf> {
+        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)
+        );
+    }
+}