Repository navigation
fix(diff): exit 2 on unreadable operand per POSIX diff convention - #2473
hgunduzoglu wants to merge 2 commits into
Conversation
rtk diff read its operands with `?`, so a missing or unreadable file collapsed to exit 1 in main's error handler — indistinguishable from "files differ". POSIX diff reserves exit 2 for trouble (I/O errors), 1 for differences, 0 for identical. Read operands explicitly, print a diff-style message to stderr, and return 2 on failure. The differing-files case (exit 0 -> 1) was already fixed by rtk-ai#2394. Fixes rtk-ai#2446
KuSh
left a comment
There was a problem hiding this comment.
Requesting changes for one regression and a docs gap. The core is correct and I verified it end-to-end.
Verified: the read_operands error type and propagation are sound; Ok(2) flows through main.rs:1895 → run_cli → std::process::exit(2) as intended; the short-circuit order (file1 before file2) matches system diff; errors go to stderr with nothing on stdout; the identical/differ paths, never_worse and tracking are untouched; no existing test or smoke assertion conflicts; and the "before" hunk matches the current tree so it still applies cleanly.
Also confirmed the bug is live — rtk diff x /nope; echo $? prints 1 on 0.46.0, indistinguishable from "files differ". Note that the other half of #2446 (differ → exit 1) has since been fixed independently, so this PR closes what remains of it.
Two things before merge:
1. Binary files (inline below).
2. The user-facing exit contract is documented in three places that this PR doesn't touch: README.md:166, README_fr.md:138 and docs/usage/FEATURES.md:239 all still say only "exit 1 if files differ". A script written against those docs checks $? -eq 1 and will silently read the new 2 as "no difference" — the same class of failure this PR exists to fix, one layer up. Worth updating all three in the same PR so the contract and its documentation land together.
Thanks for the named constant and for testing the helper rather than run() — that detail is why this PR is the one being taken forward out of three that made the same fix.
| /// is indistinguishable from "files differ" — masking the failure (#2446). | ||
| fn read_operands(file1: &Path, file2: &Path) -> std::result::Result<(String, String), i32> { | ||
| let read = |path: &Path| { | ||
| fs::read_to_string(path).map_err(|e| { |
There was a problem hiding this comment.
Every read_to_string error maps to DIFF_EXIT_TROUBLE here, and that includes io::ErrorKind::InvalidData — which is what you get for a perfectly readable file that simply isn't UTF-8.
So rtk diff a.png b.png now reports "unreadable operand" and exits 2, where system diff prints Binary files a.png and b.png differ and exits 1. Before this PR the ? collapsed it to exit 1, which was accidentally right. That makes it a regression in exactly the POSIX parity the PR is targeting.
Suggested shape: split the two cases at the point of the error.
let read = |path: &Path| {
fs::read_to_string(path).map_err(|e| {
eprintln!("rtk diff: {}: {}", path.display(), e);
DIFF_EXIT_TROUBLE
})
};becomes something that treats InvalidData as "not text" rather than "not readable" — fall back to fs::read and byte-compare, emitting Binary files {a} and {b} differ with exit 0/1, and reserve exit 2 for genuine I/O failures (missing, permission denied, is-a-directory).
If you'd rather keep this PR narrow, the alternative is to let InvalidData keep returning Err through ? as it does today and only intercept the other kinds — that preserves the current binary behaviour instead of changing it, and binary support can be its own PR. Either is fine; silently converting 1 into 2 is the one option that isn't.
|
Superseded by #4169, which continues this work and carries both of the review items from August.
The exit-2 design here is what #4169 builds on, and This branch also conflicts with develop now: |
Summary
Completes #2446.
rtk diffnow follows the full POSIX diff exit-code convention:The differing-files row was resolved by #2394 (merged). This PR closes the remaining gap: I/O errors.
Root cause
run()read both operands with?. A read failure propagated up tomain()'s error handler, which returns exit 1 for anyErr— indistinguishable from "files differ". So a typo'd path or a deleted file was silently reported as a difference (exit 1) instead of trouble (exit 2).Fix
Read the operands explicitly via a small
read_operandshelper: on any I/O error it prints a diff-style message to stderr and returns exit code 2, mirroring systemdiff. No change to the identical/differ paths.Testing
CARGO_MANIFEST_DIRso they don't depend on cwd.cargo fmt --all --check && cargo clippy --all-targets && cargo test— clean, 2203 passed.difffor all three exit codes (0/1/2).No output-format change, so token savings are unaffected (the error path prints a one-line stderr message, same as before).