Skip to content

fix(git): keep the diff when git diff --no-index exits 1 - #4717

Open
coseto6125 wants to merge 1 commit into
rtk-ai:developfrom
coseto6125:fix/git-diff-no-index-drops-output
Open

coseto6125 wants to merge 1 commit into
rtk-ai:developfrom
coseto6125:fix/git-diff-no-index-drops-output

Conversation

@coseto6125

Copy link
Copy Markdown

Summary

  • rtk git diff --no-index a b printed nothing when the two files differed. --no-index implies --exit-code, so git exits 1 and puts the diff on stdout. The compact path in run_diff read every non-zero exit as a failure, printed only stderr and returned. On two small differing files, plain git diff --no-index printed 106 bytes; rtk git diff --no-index printed 0 bytes and exited 1. An agent behind the hook saw a failed command and no diff.
  • Exit 1 with non-empty stdout now goes through the normal compact path (stat header plus compacted diff) and returns git's exit code. Exit 1 with empty stdout (--no-index on a missing file) and every other non-zero exit keep the old error path. git's stderr is forwarded in both cases, so warnings such as core.autocrlf still reach the caller.
  • Exit codes are unchanged for every input: the error path already returned git's code, and the success path returned 0. Only stdout changes, for the exit-1-with-output case. run_diff has two callers, git_cmd::run (rtk git diff) and gt_cmd::run_other (gt diff), and both pass the code through. A git revert of this commit restores the old behaviour.

Related: #3257 shows the same "prints nothing" symptom with a different trigger (diff size). #1981 introduced the raw passthrough for --exit-code, which is why rtk git diff --exit-code already keeps its output.

Test plan

  • cargo fmt --all && cargo clippy --all-targets && cargo test: fmt and clippy are clean. 4534 passed, 1 failed: git_log_malformed_digit_run_propagates_real_git_error, which fails the same way on develop without this change. It expects newer git's not an integer wording, and local git 2.43.0 prints not a non-negative integer.
  • Manual testing: rtk git diff --no-index x.txt y.txt output inspected (stat header plus the compacted hunk, exit 1).
  • New tests in tests/guard_integration_test.rs: differing files keep the diff and exit 1; identical files exit 0; a missing file surfaces git's error with empty stdout and exit 1; a core.autocrlf warning reaches stderr alongside the diff. With the fix reverted, the differing-files and warning tests fail (the diff must survive git's exit 1: "").
  • Two rows in tests/rewrite_exit_code_test.rs pin the exit codes of git diff --no-index (1 for differing files, 0 for identical) across the hook rewrite.

git diff --no-index implies --exit-code, so differing inputs exit 1 with the
diff on stdout. The compact path treated every non-zero exit as a failure and
printed only stderr, so the diff vanished. Treat exit 1 with non-empty stdout
as a difference, keep forwarding git's stderr (warnings such as core.autocrlf),
and return git's exit code.
@rtk-wshm-sync-bot rtk-wshm-sync-bot Bot added bug Something isn't working git labels Oct 21, 2026
@rtk-wshm-sync-bot

Copy link
Copy Markdown

wshm · Automated triage by AI

📊 Automated PR Analysis

🐛 Type bug-fix
🟢 Risk low

Summary

Fixes rtk git diff --no-index silently dropping diff output when git exits 1 (since --no-index implies --exit-code). The compact diff path now treats exit 1 with non-empty stdout as a normal diff (stat header + compacted hunk, exit code forwarded), while exit 1 with empty stdout or any other non-zero exit still goes through the old error path; stderr is forwarded in both cases.

Review Checklist

  • Tests present
  • Breaking change
  • Docs updated

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

@CLAassistant

CLAassistant commented Oct 26, 2026 •

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working git

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants