Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
140 changes: 138 additions & 2 deletions src/cmds/git/git.rs
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,39 @@ fn uses_compact_status_path(args: &[String]) -> bool {
saw_branch
}

fn status_args_request_machine_output(args: &[String]) -> bool {
args.iter()
.any(|arg| arg == "-z" || arg == "--porcelain" || arg.starts_with("--porcelain="))
}

fn log_args_request_machine_output(args: &[String]) -> bool {
args.iter().enumerate().any(|(idx, arg)| {
arg == "--format"
|| arg.starts_with("--format=")
|| arg.starts_with("--pretty=format:")
|| arg.starts_with("--pretty=tformat:")
|| (arg == "--pretty"
&& args
.get(idx + 1)
.is_some_and(|next| next.starts_with("format:") || next.starts_with("tformat:")))
})
}
Comment on lines +75 to +86

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[high] --pretty=%… isn't detected, so the bug from #2487 still reproduces.

Git treats --pretty=<string containing a % placeholder> as an implicit --pretty=tformat:<string>, but this only matches --format, --format=, --pretty=format: and --pretty=tformat:.

Verified against a build of this branch, in this repo:

rtk git log --pretty=%H   →     50 commits
git log --pretty=%H       →   1185 commits

The RTK -50 default is still injected and the output is silently truncated — exactly the class of bug this PR sets out to fix. Needs a "value contains %" rule for --pretty= (and for the separated --pretty <value> form below).

Comment on lines +75 to +86

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update after kylehgc/rtk@da407dc.

The amendment adds -z here, which is a genuine catch I missed: git log -z --name-only carries no custom format, so it was falling to the compact path and getting --pretty=format:<MARKER>, -10 and --no-merges injected — exactly the corruption this guard exists to prevent.

But --pretty=%H is still unmatched, so #2487's headline case is unchanged: rtk git log --pretty=%H returns 50 commits where git returns 1185. Git expands --pretty=<string containing %> to --pretty=tformat:<string>; this predicate needs that rule, and so does the separated --pretty <value> form just below.

This is the case the issue is filed about — it should land before merge.


fn diff_args_request_machine_output(args: &[String]) -> bool {
args.iter().any(|arg| {
matches!(
arg.as_str(),
"-z"
| "--no-color"

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[medium] --no-color is not a machine-output signal — this kills diff compaction.

--no-color says nothing about machine readability; it's precisely what an agent passes to avoid ANSI in its capture. Classifying it as machine output means any agent that habitually passes it gets 0% savings on git.

Verified on this branch: rtk git diff --no-color emitted 2602 bytes — byte-identical to raw git diff --no-color — where develop emits the compacted stat+diff form.

The same entry at src/discover/registry.rs:944 makes the rewriter return None for git diff --no-color and git log --no-color -10, so the hook stops rewriting them at all.

Please drop --no-color from both lists.

| "--name-only"
| "--name-status"
| "--numstat"
| "--raw"
| "--word-porcelain"
)
})
}

fn build_status_command(args: &[String], global_args: &[String]) -> Command {
let mut cmd = git_cmd(global_args);
cmd.arg("status");
Expand Down Expand Up @@ -119,10 +152,12 @@ fn run_diff(
.iter()
.any(|arg| arg == "--stat" || arg == "--numstat" || arg == "--shortstat");

let wants_machine_output = diff_args_request_machine_output(args);

// Check if user wants compact diff (default RTK behavior)
let wants_compact = !args.iter().any(|arg| arg == "--no-compact");

if wants_stat || !wants_compact {
if wants_machine_output || wants_stat || !wants_compact {
// User wants stat or explicitly no compacting - pass through directly
let mut cmd = git_cmd(global_args);
cmd.arg("diff");
Expand All @@ -140,7 +175,11 @@ fn run_diff(
return Ok(result.exit_code);
}

println!("{}", result.stdout.trim());
if wants_machine_output {
print!("{}", result.stdout);
} else {
println!("{}", result.stdout.trim());
}

timer.track(
&format!("git diff {}", args.join(" ")),
Expand Down Expand Up @@ -425,6 +464,31 @@ fn run_log(
) -> Result<i32> {
let timer = tracking::TimedExecution::start();

if log_args_request_machine_output(args) {
let mut cmd = git_cmd(global_args);
cmd.arg("log");
for arg in args {
cmd.arg(arg);
}

let result = exec_capture(&mut cmd).context("Failed to run git log")?;
if !result.success() {
eprintln!("{}", result.stderr);
return Ok(result.exit_code);
}

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

timer.track(
&format!("git log {}", args.join(" ")),
&format!("rtk git log {} (passthrough)", args.join(" ")),
&result.stdout,
&result.stdout,
);

return Ok(0);
}
Comment on lines +467 to +490

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[medium] The passthrough drops the implicit commit cap, so output becomes unbounded.

This branch runs git log <args> with no -N and no --no-merges, unlike the path below it which injects -50 / -10.

Verified: rtk git log --format=%H (no explicit limit) now prints the full 1185-commit history in this repo; on develop it printed 50. A single such call can flood the agent's context — the thing RTK exists to prevent.

Suggest keeping a large default cap when the user gave no -n / --max-count, or at minimum documenting the unbounded behaviour.

Comment on lines +467 to +490

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update after kylehgc/rtk@da407dc.

The amendment doesn't change this branch, but it widens what reaches it. Adding -z to log_args_request_machine_output means git log -z --name-only now lands here uncapped, where it previously took the compact path with -10. On this repo that turns a 10-commit summary into the full 1185-commit history, with per-commit file lists.

Both changes are individually right; together they make the missing default cap considerably more reachable. A large default -n when the user gave neither -n nor --max-count keeps the -z fix without the flood.


let mut cmd = git_cmd(global_args);
cmd.arg("log");

Expand Down Expand Up @@ -815,6 +879,40 @@ fn filter_status_with_args(output: &str) -> String {
fn run_status(args: &[String], verbose: u8, global_args: &[String]) -> Result<i32> {
let timer = tracking::TimedExecution::start();

if status_args_request_machine_output(args) {
let mut cmd = git_cmd(global_args);
cmd.arg("status");
cmd.args(args);
let result = exec_capture(&mut cmd).context("Failed to run git status")?;

if !result.success() {
if !result.stderr.trim().is_empty() {
eprint!("{}", result.stderr);
}
timer.track(
&format!("git status {}", args.join(" ")),
&format!("rtk git status {} (passthrough)", args.join(" ")),
&result.stdout,
&result.stdout,
);
return Ok(result.exit_code);
}

if verbose > 0 || !result.stderr.is_empty() {
eprint!("{}", result.stderr);
}
print!("{}", result.stdout);

timer.track(
&format!("git status {}", args.join(" ")),
&format!("rtk git status {} (passthrough)", args.join(" ")),
&result.stdout,
&result.stdout,
);

return Ok(0);
}

// Keep a narrow compact path for no-arg status and branch/short-only flags.
// More complex explicit args still use the existing minimal-filter path.
if !uses_compact_status_path(args) {
Expand Down Expand Up @@ -1852,6 +1950,44 @@ mod tests {
assert!(envs.contains(&("LC_ALL".to_string(), "C".to_string())));
}

#[test]
fn test_git_status_machine_output_args_passthrough() {
assert!(status_args_request_machine_output(&["--porcelain".to_string()]));
assert!(status_args_request_machine_output(&[
"--porcelain=v2".to_string()
]));
assert!(status_args_request_machine_output(&["-z".to_string()]));
assert!(!status_args_request_machine_output(&["--short".to_string()]));
}

#[test]
fn test_git_log_machine_output_args_passthrough() {
assert!(log_args_request_machine_output(&["--format=%H".to_string()]));
assert!(log_args_request_machine_output(&[
"--pretty=format:%H".to_string()
]));
assert!(log_args_request_machine_output(&[
"--format".to_string(),
"%H".to_string()
]));
assert!(!log_args_request_machine_output(&["--oneline".to_string()]));
}

#[test]
fn test_git_diff_machine_output_args_passthrough() {
for args in [
vec!["--name-only".to_string()],
vec!["--name-status".to_string()],
vec!["--numstat".to_string()],
vec!["--raw".to_string()],
vec!["--word-porcelain".to_string()],
vec!["-z".to_string(), "--name-only".to_string()],
] {
assert!(diff_args_request_machine_output(&args), "{args:?}");
}
assert!(!diff_args_request_machine_output(&[]));
}

#[test]
fn test_build_status_command_default_compact() {
let cmd = build_status_command(&[], &[]);
Expand Down
Loading
Loading