Skip to content

fix(git): propagate exit code on git status failure in compact path - #2498

Merged
aeppling merged 3 commits into
rtk-ai:developfrom
hgunduzoglu:fix/git-status-compact-exit-code
Jun 26, 2026
Merged

aeppling merged 3 commits into
rtk-ai:developfrom
hgunduzoglu:fix/git-status-compact-exit-code

Conversation

@hgunduzoglu

Copy link
Copy Markdown
Contributor

Part of #2497 (1/3).

Problem

rtk git status (compact path) reported a clean working tree with exit 0 when git status actually failed for any reason other than "not a git repository" — e.g. a corrupt index, a lock conflict, or unreadable refs. The non-compact path already guards on result.success() (git.rs:824); the compact path only special-cased the not-a-repo string and let every other failure fall through to the formatted summary + Ok(0).

# repo with a corrupted .git/index
$ git status            # native
fatal: .git/index: index file smaller than expected     # exit 128
$ rtk git status        # before this PR
Clean working tree                                       # exit 0  ✗

Fix

Replace the narrow "not a git repository" check with a general if !result.success() guard (mirroring the non-compact path). The friendly Not a git repository message is preserved for that common case; any other failure surfaces git's stderr and returns its real exit code.

$ rtk git status        # after
fatal: .git/index: index file smaller than expected
                        # exit 128  ✓ (matches native git)

Testing

  • New test test_run_status_compact_propagates_non_repo_failure: inits a temp repo, corrupts .git/index, asserts run_status returns a non-zero exit (uses git -C via global args; assertion is != 0 so it's exit-code-agnostic and cross-platform).
  • cargo fmt --all --check && cargo clippy --all-targets && cargo test — clean, 2200 passed.
  • Manual verification vs native git:
    • corrupt index → exit 128 (was 0)
    • clean repo → * main / clean — nothing to commit, exit 0 (no regression)
    • dirty repo → ?? f2, exit 0 (no regression)
    • non-git dir → Not a git repository, exit 128 (friendly message preserved)

Same masking-failure class as #1581 / #1535 / #2446 / #2494. Sibling fixes for run_worktree and run_stash will follow as separate PRs under #2497.

The compact status path only caught the "not a git repository" error and
let every other failure (corrupt index, lock contention, broken refs)
fall through to a formatted "Clean working tree" + Ok(0), masking a real
git error. The non-compact path already guards on result.success(); apply
the same guard to the compact path, keeping the friendly not-a-repo
message and propagating git's real exit code otherwise.

Part of rtk-ai#2497
@aeppling

Copy link
Copy Markdown
Contributor

Hey @hgunduzoglu thanks clean fix, could you remove comment in all your conditions since this is pretty explicit by code ? This would just avoid accumulating noise in code.

I'll merge the three parts then.
Thanks for this contribution, looks good to me by code

@aeppling aeppling self-assigned this Jun 26, 2026
Per review (rtk-ai#2498): the guard is self-explanatory; remove the comment
to avoid noise.
hgunduzoglu added a commit to hgunduzoglu/rtk that referenced this pull request Jun 26, 2026
Per review (rtk-ai#2498): self-explanatory guard; remove comment to avoid noise.
hgunduzoglu added a commit to hgunduzoglu/rtk that referenced this pull request Jun 26, 2026
Per review (rtk-ai#2498): self-explanatory guards; remove comments to avoid noise.
@hgunduzoglu

Copy link
Copy Markdown
Contributor Author

Done — dropped the explanatory comments on the guards across all three (#2498, #2499, #2500); the code speaks for itself and the rationale lives in the commit messages / #2497. Tests still green. Thanks for the review!

@aeppling

Copy link
Copy Markdown
Contributor

ci checks are manually triggered :)

@aeppling aeppling mentioned this pull request Jun 26, 2026
@aeppling
aeppling merged commit d86f007 into rtk-ai:develop Jun 26, 2026
20 of 21 checks passed
@hgunduzoglu

Copy link
Copy Markdown
Contributor Author

ci checks are manually triggered :)

windows tests returns error, I wanted to check whether it was temporary or related to the code

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants