Skip to content

Commit da407dc

Browse files
kylehgcclaude
andcommitted
fix(git): correct the machine-output flag set and stop diluting gain stats
Fork-authored amendment on top of 46d16ea (cherry-picked from upstream rtk-ai#2573). Kept separate so the contributor's commit stays intact and attributable. Three defects found by code review on PR #68, each reproduced against native git before and after. 1. `--word-porcelain` is not a git flag. `git diff --word-porcelain` exits 129 (unknown option). The real machine-readable spelling is `--word-diff=porcelain`, and because the guard uses `matches!` on exact equality it never matched. So the adopted list had one dead arm and one real gap. Replaced the former with the latter. `--word-diff=plain` and `--word-diff=color` are human-facing and stay compacted; only the porcelain variant passes through. before: rtk git diff --word-diff=porcelain -> compacted after : native 6706 bytes | rtk 6706 bytes 2. `-z` was unhandled for `git log`. Issue #48 names `-z` explicitly, but log_args_request_machine_output only matched --format/--pretty variants. `git log -z --name-only` carries no custom format, so it fell to the compact path — which injects its own --pretty=format:<MARKER>, -10 and --no-merges and then filters. That is precisely the corruption this guard exists to prevent. before: native 2267 bytes | rtk 564 bytes (decorated) after : native 2267 bytes | rtk 2267 bytes 3. `--no-color` was treated as machine output. It only suppresses ANSI and says nothing about the format, so listing it bought a full passthrough and silently disabled compaction for a commonly passed flag — a cost with no faithfulness gain. Removed. before: native 6456 bytes | rtk 6456 bytes (passthrough) after : native 6456 bytes | rtk 6365 bytes (compacted) Also switches the three new log/status tracking calls from `timer.track(.., &stdout, &stdout)` to `timer.track_passthrough(..)`. Equal non-zero token counts record 0%-savings rows that dilute `rtk gain`; tracking.rs documents track_passthrough for exactly this, and run_passthrough in this same file already uses it. The pre-existing calls in run_diff and run_show are left alone — they predate this change. Tests: the contributor's diff-flag test asserted `--word-porcelain` is machine output, encoding the belief that it is a git flag. Updated to assert the intent instead — the real flag matches, the non-flag does not, and the human-facing word-diff modes stay compacted. Adds tests/git_machine_output_test.rs. The existing unit tests only cover the flag predicates, so swapping `print!` back to `println!("{}", stdout.trim())` would still pass all of them while dropping the trailing newline. The new test runs the built binary (CARGO_BIN_EXE_rtk, no install needed) against real git and compares bytes, with a counterweight case asserting bare `git status` and `git diff --no-color` stay compacted so the guard can't be widened until byte-equality passes trivially. Not addressed, deliberately: the ~90 lines duplicating run_passthrough, and the pre-existing loss of stdout on non-zero exit in run_diff (verified present before this change on --numstat/--name-only/--raw alike). Both are separate concerns from this adoption. Quality gate: fmt clean, clippy 0 warnings, cargo test --all 2585 passed, 0 failed (LASTEXITCODE=0). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1 parent 46d16ea commit da407dc

2 files changed

Lines changed: 156 additions & 13 deletions

File tree

‎src/cmds/git/git.rs‎

Lines changed: 53 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -83,7 +83,11 @@ fn status_args_request_machine_output(args: &[String]) -> bool {
8383

8484
fn log_args_request_machine_output(args: &[String]) -> bool {
8585
args.iter().enumerate().any(|(idx, arg)| {
86-
arg == "--format"
86+
// `-z` NUL-delimits records for programmatic consumers, independently of
87+
// any --format/--pretty flag: `git log -z --name-only` is machine output
88+
// with no custom format at all.
89+
arg == "-z"
90+
|| arg == "--format"
8791
|| arg.starts_with("--format=")
8892
|| arg.starts_with("--pretty=format:")
8993
|| arg.starts_with("--pretty=tformat:")
@@ -99,12 +103,13 @@ fn diff_args_request_machine_output(args: &[String]) -> bool {
99103
matches!(
100104
arg.as_str(),
101105
"-z"
102-
| "--no-color"
103106
| "--name-only"
104107
| "--name-status"
105108
| "--numstat"
106109
| "--raw"
107-
| "--word-porcelain"
110+
// `--word-diff=porcelain` is the machine-readable word-diff; the
111+
// plain/color variants are for humans, so they stay compacted.
112+
| "--word-diff=porcelain"
108113
)
109114
})
110115
}
@@ -498,11 +503,9 @@ fn run_log(
498503

499504
print!("{}", result.stdout);
500505

501-
timer.track(
506+
timer.track_passthrough(
502507
&format!("git log {}", args.join(" ")),
503508
&format!("rtk git log {} (passthrough)", args.join(" ")),
504-
&result.stdout,
505-
&result.stdout,
506509
);
507510

508511
return Ok(0);
@@ -930,11 +933,9 @@ fn run_status(args: &[String], verbose: u8, global_args: &[String]) -> Result<i3
930933
if !result.stderr.trim().is_empty() {
931934
eprint!("{}", result.stderr);
932935
}
933-
timer.track(
936+
timer.track_passthrough(
934937
&format!("git status {}", args.join(" ")),
935938
&format!("rtk git status {} (passthrough)", args.join(" ")),
936-
&result.stdout,
937-
&result.stdout,
938939
);
939940
return Ok(result.exit_code);
940941
}
@@ -944,11 +945,9 @@ fn run_status(args: &[String], verbose: u8, global_args: &[String]) -> Result<i3
944945
}
945946
print!("{}", result.stdout);
946947

947-
timer.track(
948+
timer.track_passthrough(
948949
&format!("git status {}", args.join(" ")),
949950
&format!("rtk git status {} (passthrough)", args.join(" ")),
950-
&result.stdout,
951-
&result.stdout,
952951
);
953952

954953
return Ok(0);
@@ -2297,21 +2296,62 @@ mod tests {
22972296
assert!(!log_args_request_machine_output(&["--oneline".to_string()]));
22982297
}
22992298

2299+
#[test]
2300+
fn test_git_log_z_is_machine_output_without_format_flag() {
2301+
// `git log -z --name-only` is machine output with no custom format at all.
2302+
// Before this, -z fell to the compact path, which injects its own
2303+
// --pretty=format:<MARKER>, -10 and --no-merges and then filters —
2304+
// exactly the corruption the machine-output guard exists to prevent.
2305+
assert!(log_args_request_machine_output(&["-z".to_string()]));
2306+
assert!(log_args_request_machine_output(&[
2307+
"-z".to_string(),
2308+
"--name-only".to_string()
2309+
]));
2310+
}
2311+
23002312
#[test]
23012313
fn test_git_diff_machine_output_args_passthrough() {
23022314
for args in [
23032315
vec!["--name-only".to_string()],
23042316
vec!["--name-status".to_string()],
23052317
vec!["--numstat".to_string()],
23062318
vec!["--raw".to_string()],
2307-
vec!["--word-porcelain".to_string()],
2319+
vec!["--word-diff=porcelain".to_string()],
23082320
vec!["-z".to_string(), "--name-only".to_string()],
23092321
] {
23102322
assert!(diff_args_request_machine_output(&args), "{args:?}");
23112323
}
23122324
assert!(!diff_args_request_machine_output(&[]));
23132325
}
23142326

2327+
#[test]
2328+
fn test_git_diff_word_diff_only_porcelain_is_machine_output() {
2329+
// `--word-porcelain` is not a git flag at all (`git diff --word-porcelain`
2330+
// exits 129); the real machine-readable spelling is `--word-diff=porcelain`.
2331+
assert!(!diff_args_request_machine_output(&[
2332+
"--word-porcelain".to_string()
2333+
]));
2334+
assert!(diff_args_request_machine_output(&[
2335+
"--word-diff=porcelain".to_string()
2336+
]));
2337+
// The human-facing word-diff modes must stay compacted.
2338+
for human in ["--word-diff=plain", "--word-diff=color", "--word-diff"] {
2339+
assert!(
2340+
!diff_args_request_machine_output(&[human.to_string()]),
2341+
"{human} is for humans and must stay compacted"
2342+
);
2343+
}
2344+
}
2345+
2346+
#[test]
2347+
fn test_git_diff_no_color_is_not_machine_output() {
2348+
// --no-color only suppresses ANSI; it says nothing about the output format,
2349+
// so treating it as machine output would disable compaction for free.
2350+
assert!(!diff_args_request_machine_output(&[
2351+
"--no-color".to_string()
2352+
]));
2353+
}
2354+
23152355
#[test]
23162356
fn test_build_status_command_default_compact() {
23172357
let cmd = build_status_command(&[], &[]);

‎tests/git_machine_output_test.rs‎

Lines changed: 103 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,103 @@
1+
//! `rtk git` must hand machine-readable formats back byte-for-byte.
2+
//!
3+
//! The unit tests in `src/cmds/git/git.rs` only cover the flag predicates —
4+
//! whether an arg list *counts* as machine output. They cannot catch the actual
5+
//! defect this guards, which lives in the print path: swapping `print!` back to
6+
//! `println!("{}", stdout.trim())` still satisfies every predicate test while
7+
//! silently dropping the trailing newline and breaking `while read` loops.
8+
//!
9+
//! So these run the real binary against real git and compare bytes.
10+
11+
use std::path::Path;
12+
use std::process::Command;
13+
14+
/// Run a program in the repo root, returning raw stdout bytes.
15+
fn run(program: &str, args: &[&str]) -> Option<Vec<u8>> {
16+
let out = Command::new(program)
17+
.args(args)
18+
.current_dir(Path::new(env!("CARGO_MANIFEST_DIR")))
19+
.output()
20+
.ok()?;
21+
Some(out.stdout)
22+
}
23+
24+
/// These compare against real `git` in this working tree. Skip rather than fail
25+
/// where that isn't available, so the suite stays green off a git checkout.
26+
fn git_available() -> bool {
27+
run("git", &["rev-parse", "--git-dir"]).is_some_and(|o| !o.is_empty())
28+
}
29+
30+
#[test]
31+
fn machine_readable_git_output_is_byte_identical_to_native() {
32+
if !git_available() {
33+
eprintln!("skipping: not a git checkout or git not on PATH");
34+
return;
35+
}
36+
37+
// Each case must survive rtk untouched — same bytes, trailing newline included.
38+
let cases: &[&[&str]] = &[
39+
&["status", "--porcelain"],
40+
&["status", "--porcelain=v2"],
41+
&["status", "-z"],
42+
&["log", "--format=%H", "-3"],
43+
&["log", "--pretty=format:%H", "-3"],
44+
&["log", "-z", "--name-only", "-3"],
45+
&["diff", "--name-only", "HEAD~1..HEAD"],
46+
&["diff", "--name-status", "HEAD~1..HEAD"],
47+
&["diff", "--numstat", "HEAD~1..HEAD"],
48+
&["diff", "--raw", "HEAD~1..HEAD"],
49+
];
50+
51+
for args in cases {
52+
let Some(native) = run("git", args) else {
53+
continue;
54+
};
55+
let rtk = run(env!("CARGO_BIN_EXE_rtk"), &[&["git"], *args].concat())
56+
.expect("rtk binary must run");
57+
58+
assert_eq!(
59+
String::from_utf8_lossy(&rtk),
60+
String::from_utf8_lossy(&native),
61+
"`rtk git {args:?}` diverged from native git ({} bytes vs {})",
62+
rtk.len(),
63+
native.len()
64+
);
65+
}
66+
}
67+
68+
#[test]
69+
fn human_facing_git_output_is_still_compacted() {
70+
if !git_available() {
71+
eprintln!("skipping: not a git checkout or git not on PATH");
72+
return;
73+
}
74+
75+
// The counterweight: widening the machine-output guard until everything
76+
// passes through would make the byte-exactness test above pass trivially
77+
// while destroying the compaction rtk exists for.
78+
let native = run("git", &["status"]).expect("git status");
79+
let rtk = run(env!("CARGO_BIN_EXE_rtk"), &["git", "status"]).expect("rtk git status");
80+
81+
assert!(
82+
rtk.len() < native.len(),
83+
"bare `git status` must still be compacted (rtk {} bytes vs native {})",
84+
rtk.len(),
85+
native.len()
86+
);
87+
88+
// --no-color only suppresses ANSI; it must not buy a passthrough.
89+
let native_nc = run("git", &["diff", "--no-color", "HEAD~1..HEAD"]).expect("git diff");
90+
if !native_nc.is_empty() {
91+
let rtk_nc = run(
92+
env!("CARGO_BIN_EXE_rtk"),
93+
&["git", "diff", "--no-color", "HEAD~1..HEAD"],
94+
)
95+
.expect("rtk git diff");
96+
assert!(
97+
rtk_nc.len() < native_nc.len(),
98+
"`git diff --no-color` must stay compacted (rtk {} bytes vs native {})",
99+
rtk_nc.len(),
100+
native_nc.len()
101+
);
102+
}
103+
}

0 commit comments

Comments
 (0)