fix(diff): return exit code 2 for unreadable files

[TaKO8Ki]

Summary

This PR continues the work in #2473 and addresses KuSh's review feedback on non-UTF-8 handling and documentation.

• Return exit code 2 for file read failures, distinguishing them from file differences.
• Compare readable non-UTF-8 files byte for byte, returning 0 or 1.
• Add focused regression tests and document the exit-code contract.

Test plan

• cargo fmt --all && cargo clippy --all-targets && cargo test
• Manual testing: rtk <command> output inspected

Important: All PRs must target the develop branch (not master).
See CONTRIBUTING.md for details.

[rtk-wshm-sync-bot]

wshm · Automated triage by AI

📊 Automated PR Analysis

🐛 Type	bug-fix
🟡 Risk	medium

Summary

Fixes rtk diff to return exit code 2 when a file cannot be read, distinguishing read failures from actual file differences (which still return 0 or 1). Non-UTF-8 files are now compared byte-for-byte instead of erroring out as unreadable, and documentation/CLI help text is updated to describe the new exit-code contract.

Review Checklist

• Tests present
• Breaking change
• Docs updated

---

Analyzed automatically by wshm · This is an automated analysis, not a human review.

[KuSh]

Round 1 of 3 — APPROVED, follow-up filed

Claim: rtk diff returns 2 when an operand cannot be read, compares readable non-UTF-8 files byte for byte (0/1), and documents the exit contract.
Scope: accept. This supersedes #2473 — it carries both of that PR's requested changes and rebases cleanly, where #2473 now conflicts against a since-restructured diff_cmd.rs. It collides with #3781 on the same two lines; see Next for the ordering.
Ran: rtk @develop b748a5f × rtk @bdee7f20 × rtk @2de82b9d × 16 operand shapes × before/after, each against /usr/bin/diff under LC_ALL=C for POSIX parity; 4 mutations of the new code against the new tests; 3 tracking-database failure states; full gate on the merge with latest develop. Invariants: split streams, every branch of the new decision (read error / both-invalid-equal / both-invalid-differ / mixed), under the hook (rtk rewrite "diff a b" → rtk diff a b), savings.
Matrix: executed this round.
CI @2de82b9d: all green — 11 pass, 0 failing, across ubuntu, macos and windows. It was also all green on bdee7f2, the head before my fixup, which is what made the fixup safe to push.

Blocks merge (0)

Nothing. The exit codes match /usr/bin/diff on every shape I could construct.

Checked and correct — no need to re-verify

Both requested changes from the #2473 review are met, by execution.

1 — binary files. That review asked for InvalidData to stop meaning "unreadable": fall back to fs::read and byte-compare, emit Binary files {a} and {b} differ at exit 0/1, and reserve 2 for genuine I/O failure. That is what this does, and the message is byte-identical to GNU's.

case	develop	this PR	/usr/bin/diff
binary, differ	exit 1, rtk: stream did not contain valid UTF-8, no output	exit 1, Binary files bin_a.bin and bin_bdiff.bin differ	exit 1, same line
binary, identical	exit 1, same error	exit 0, silent	exit 0, silent
binary vs text	exit 1, same error	exit 1, Binary files bin_a.bin and same_a.txt differ	exit 1, same line
latin-1, identical	exit 1, same error	exit 0, silent	exit 0
missing operand	exit 1, rtk: No such file or directory (os error 2)	exit 2, rtk diff: nope.txt: No such file or directory (os error 2)	exit 2, names the path
permission denied	exit 1	exit 2	exit 2
is-a-directory	exit 1	exit 2	exit 2
identical / differing text	0 / 1	0 / 1	0 / 1

The identical-binary row is the one worth naming on its own: develop answered "these two files are not the same" about two files that are the same, with an exit code a caller reads as a difference. That is now 0, and the path is also named in the error where develop left the caller guessing which of the two operands was bad.

2 — the docs gap. The three places named in that review are the only three in the tree: grepping every *.md for the exit contract finds README.md, README_fr.md and the rtk diff section of docs/usage/FEATURES.md, and all three now state 0/1/2. The clap long_about in src/main.rs was added on top, which is where a caller looks first.

The new tests are real. Four mutations of the implementation, each one compiling, each killed by the test that names the behaviour:

mutation	killed by
read error returns 1 instead of 2	missing_operand_exits_two_and_names_the_failed_path
error message drops the path	missing_operand_exits_two_and_names_the_failed_path
differing binaries return 0	non_utf8_files_are_compared_instead_of_reported_as_io_errors
Binary files … sent to stderr	non_utf8_files_are_compared_instead_of_reported_as_io_errors

Savings are untouched. On the text path the two binaries emit byte-identical output (452 = 452 bytes on a 200-line source pair, cmp clean), so the never_worse guard and the 20% floor sit exactly where develop left them. The render path is not touched.

Tracking on the two new returns is honest. Both record input == output, i.e. 0%. That is accurate rather than dilution: on the binary path rtk emits the same line GNU diff does, and on the error path a one-line message. The identical-binary case records 0/0 tokens, which rtk gain sums without dividing by zero (checked against a scratch DB).

Green after merge with latest develop. Re-checked against develop at 2bf3eb7, not just the base this branch was cut from: the merge is clean, and on the merged tree cargo clippy --all-targets is at 0 warnings and cargo test --all is 3937 passed / 0 failed, both new tests among them. Nine commits have landed on develop since this branch's base and none of them touch these six files.

Fixup pushed — 2de82b9d

refactor(diff): name the trouble exit code. It adds const DIFF_EXIT_TROUBLE: i32 = 2 and uses it at the one read-failure return, so the site says which of diff's three codes it is rather than a bare Ok(2). Nothing else changed. That constant is the one detail the #2473 review singled out, and this keeps it.

Verified as if it were an author push: the 16-row matrix is identical to bdee7f2 on every row, and a second pass by someone who had not seen any of the above ran 36 invocations across both binaries — zero differences in exit code, stdout or stderr, with the 14 rows that actually reach the modified return covering all three documented causes on both operand positions. Clippy is at 0, re-checked after a cargo clean -p rtk because the first run was a 0.15s cached no-op and proved nothing.

Optional — will not hold merge

• Identical non-UTF-8 files print nothing, where identical text files print [ok] Files are identical. Two contracts inside one command — though silence is what GNU diff does, so for a PR whose claim is POSIX parity it is arguably the right half that is inconsistent. Left alone deliberately; worth a decision later, not here.
• rtk diff nope1 nope2 names only the first bad path, where GNU names both before exiting 2. The exit code agrees, and the short-circuit order (file1 before file2) is the one GNU uses.

Filed as follow-ups

• #4231 — non-UTF-8 but NUL-free text (latin-1 and other legacy 8-bit encodings) is reported as Binary files … differ where GNU prints the line diff, because rtk's "is this binary" test is UTF-8 validity while GNU's is NUL presence. Sharpest form: rtk diff a b says Binary files a and b differ while rtk diff -u a b passes through to real diff and prints the changed lines, for the same two files. Not blocking and not a regression — develop printed nothing at all for that pair — but the content is still suppressed where GNU shows it.
• rtk diff - <file> is unchanged in substance: develop exits 1, this exits 2, GNU exits 0. Both are wrong and both are loud. That is #3780, and #3781 is its fix.
• The suite's run_rtk_diff spawns the binary without RTK_DB_PATH, so cargo test writes rows into the developer's own analytics database. Pre-existing in that helper rather than added here, and #3758 already covers this exact file — no new issue.

Hypotheses built and dropped

• Does the new assert!(output.stderr.is_empty()) depend on developer-local state? Reproduced three tracking-database failure states — a database missing the commands table, a non-SQLite file, and an unwritable file. None put anything on stderr: the missing table is recreated by the migration, and an unopenable database is skipped silently by if let Ok(tracker) = Tracker::new(). The warn_if_missing_table path needs a table that is still missing after a successful open, which I could not construct. Dropped.
• Do files with NUL bytes that are nonetheless valid UTF-8 regress? No — both binaries render them as a text diff at exit 1, identical before and after, so this is the same classification gap in the other direction and it is pre-existing.
• Does exit 2 leak into the single-operand stdin form? No. rtk diff <file> routes to run_stdin, untouched here; exit 0 before and after on a real unified-diff payload.
• Does main.rs throw away an exit code on that stdin route, so a piped diff with differences reports success? No — run_stdin returns Result<()>, so there is no code to discard, and that route is a filter for a diff payload rather than a comparison of two files, which is what makes 0 the right answer for it. (The docs/usage/FEATURES.md line calling stdin "le second fichier" describes it less well than the code does, but that predates this PR.)
• Does the error path dilute rtk gain? No, see above.
• Is Binary files … differ missing its trailing newline? No, it carries one.

Your questions

None asked since the PR was opened.

Next

Nothing on this PR — approving once CI is green on 2de82b9d. One ordering note that is not this PR's to fix: #3781 rewrites the same two lines through read_path_operands(file1, file2)?, and that ? sends an unreadable operand into main's handler, which exits 1 — the develop behaviour this PR replaces. Whichever lands second has to carry the other's contract. Landing this first is the cheaper direction: missing_operand_exits_two_and_names_the_failed_path then fails loudly if a rebase drops exit 2, so #3781 only has to grow the exit-2 and binary arms alongside its stdin resolution.

Rounds: 1/3. Threads: 0 open, 0 resolved this round.