Skip to content

fix(git): keep the trailing newline on filtered 'git status' output - #3682

Open
strickdd wants to merge 1 commit into
rtk-ai:developfrom
strickdd:fix/git-status-args-trailing-newline
Open

strickdd wants to merge 1 commit into
rtk-ai:developfrom
strickdd:fix/git-status-args-trailing-newline

Conversation

@strickdd

Copy link
Copy Markdown

Summary

  • rtk git status drops the trailing newline on the arg-carrying path (--short / -s / --porcelain), so the last entry glues to whatever prints next.
  • Worse than cosmetic: any pipeline that counts lines undercounts by one, so rtk git status --porcelain | wc -l reports 0 — a false "clean" — on a tree with exactly one change. Anything gating a commit/no-commit decision on piped porcelain output takes the wrong branch.
  • Fixed by normalizing to exactly one trailing newline in a pure, unit-tested helper. Empty output stays empty, so --porcelain on a clean tree still correctly prints nothing.

Root cause

In run_status, the non-compact path ends with:

let filtered = filter_status_with_args(&result.stdout);
let filtered = never_worse(&result.stdout, &filtered).to_string();
print!("{}", filtered);

filter_status_with_args joins its kept lines with \n and so has no terminal newline, while never_worse may instead hand back git's raw stdout, which does. print! is therefore wrong in exactly one of the two cases. This is also why bare status, diff --stat, log and branch are unaffected — they end in println!.

with_trailing_newline handles both inputs, which is why the fix is a helper rather than swapping print! for println! (that would double the newline whenever never_worse falls back to raw).

Reproduction

$ /usr/bin/git status --porcelain | cat -A
?? handoff/$
$ rtk git status --porcelain | cat -A
?? handoff/
$ /usr/bin/git status --porcelain | wc -l
1
$ rtk git status --porcelain | wc -l
0          # <- reads as a clean tree

Test plan

  • cargo fmt --all -- --check — clean
  • cargo clippy --all-targets — clean
  • cargo test — 2708 passed, 8 ignored (9 suites)
  • 4 new unit tests: newline appended when missing, existing newline left alone, empty output stays empty, and a line-count-parity test pinning the false-clean regression directly.
  • Manual: built --release and compared against /usr/bin/git by md5 and line count for --short / -s / --porcelain / --porcelain=v1 across clean, single-untracked, 10-modified, staged, deleted, untracked-directory and 252-entry trees, plus inside a linked worktree. Byte-identical in every case.
  • Confirmed --long and bare status still take the compact path and are unchanged — this PR does not touch rtk's intended compaction, only restores fidelity where rtk already passes git's own output through.

Scope

One function plus its tests, in src/cmds/git/git.rs. No behavior change to any other command, and no change to how much rtk compacts.

`filter_status_with_args` joins kept lines with \n and has no terminal
newline; `never_worse` may instead return git's raw stdout, which does.
`print!` on either is wrong in one of the two cases, so the arg-carrying
status path (--short / -s / --porcelain) dropped the final newline.

The last entry then glues to whatever prints next, and — worse than
cosmetic — any pipeline that counts lines undercounts by one:

    git status --porcelain | cat -A   ->  ?? handoff/$      (1 line)
    rtk git status --porcelain | cat -A -> ?? handoff/       (0 lines)

On a tree with a single change, `rtk git status --porcelain | wc -l`
therefore reports 0 — a false 'clean' on the exact question the command
is usually asked. Anything gating a commit/no-commit decision on piped
porcelain output takes the wrong branch.

Normalize to exactly one trailing newline via a pure, unit-tested
`with_trailing_newline`. Empty output stays empty: --porcelain on a
clean tree correctly prints nothing.
@CLAassistant

CLAassistant commented Aug 23, 2026 •

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@strickdd
strickdd marked this pull request as ready for review August 23, 2026 20:39
kylehgc added a commit to kylehgc/rtk that referenced this pull request Sep 7, 2026
…aches

Amendment to the adopted upstream rtk-ai#3682 commit, kept separate to preserve
its authorship. On this fork `--porcelain` and `-z` are machine-output
passthrough (`status_args_request_machine_output`) and never reach
`with_trailing_newline`; the forms that undercounted were `-s`,
`--short` and the path-operand forms. The doc comment and the test
comments said `--porcelain`, so a later bisect running that repro on
develop would have found it already correct and read the fix as a no-op.

Also: `str::lines()` ignores a trailing newline, so the line-count
assertion passed without the fix -- count `\n` like `wc -l` does; and pin
the composed clean-tree case (`filter_status_with_args("")` is `"ok"`,
only `never_worse` returns git's empty stdout) so a clean `git status -s`
never prints `ok`.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
kylehgc added a commit to kylehgc/rtk that referenced this pull request Sep 7, 2026
fix(git): keep the trailing newline on filtered 'git status' output (adopt upstream rtk-ai#3682)
@kylehgc

kylehgc commented Oct 9, 2026

Copy link
Copy Markdown
Contributor

One case this changes that it probably shouldn't: git status -z.

-z output is NUL-terminated and has no final \n, and it takes the same filter_status_with_args → never_worse path, so with_trailing_newline appends a \n after the last NUL. Built from this PR's head (353fdb7), in a repo with one modified and one untracked file:

$ git status -z | od -c | tail -2
0000020   t   x   t  \0
$ rtk git status -z | od -c | tail -2
0000020   t   x   t  \0  \n

$ rtk git status -z | xargs -0 -n1 printf '[%s]\n'
[ M f.txt]
[?? new.txt]
[
]

xargs -0 and similar consumers read the extra byte as a third record. Current develop (e4f0509) prints the same bytes as git here, so this would be a regression. The call site this PR changes is unchanged on develop, so a rebase doesn't avoid it.

Leaving NUL-terminated output alone fixes it:

if output.is_empty() || output.ends_with('\n') || output.ends_with('\0') {

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.

3 participants