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
2 changes: 1 addition & 1 deletion src/cmds/git/gh_cmd.rs
Original file line number Diff line number Diff line change
Expand Up @@ -950,7 +950,7 @@ fn pr_diff(args: &[String], _verbose: u8) -> Result<i32> {
if raw.trim().is_empty() {
"No diff".to_string()
} else {
git::compact_diff(raw, 500)
git::compact_diff(raw)
}
},
RunOptions::stdout_only().early_exit_on_failure(),
Expand Down
168 changes: 83 additions & 85 deletions src/cmds/git/git.rs
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,7 @@ pub fn run(

fn run_diff(
args: &[String],
max_lines: Option<usize>,
_max_lines: Option<usize>,
verbose: u8,
global_args: &[String],
) -> Result<i32> {
Expand Down Expand Up @@ -195,7 +195,7 @@ fn run_diff(
let diff_result = exec_capture(&mut diff_cmd).context("Failed to run git diff")?;

let printed = if !diff_result.stdout.is_empty() {
let compacted = compact_diff(&diff_result.stdout, max_lines.unwrap_or(500));
let compacted = compact_diff(&diff_result.stdout);
format!("{}\n\nChanges:\n{}", result.stdout.trim(), compacted)
} else {
result.stdout.trim().to_string()
Expand All @@ -217,7 +217,7 @@ fn run_diff(

fn run_show(
args: &[String],
max_lines: Option<usize>,
_max_lines: Option<usize>,
verbose: u8,
global_args: &[String],
) -> Result<i32> {
Expand Down Expand Up @@ -312,7 +312,7 @@ fn run_show(
if verbose > 0 {
printed.push_str("\n\nChanges:");
}
let compacted = compact_diff(diff_text, max_lines.unwrap_or(500));
let compacted = compact_diff(diff_text);
printed.push('\n');
printed.push_str(&compacted);
}
Expand All @@ -335,25 +335,25 @@ fn is_blob_show_arg(arg: &str) -> bool {
!arg.starts_with('-') && arg.contains(':')
}

pub(crate) fn compact_diff(diff: &str, max_lines: usize) -> String {
/// Compacts a unified diff by stripping metadata (index/mode/rename lines,
/// `---`/`+++` file markers) while preserving every `+`/`-`/context line and
/// hunk header in full.
///
/// This never drops diff content. Silent truncation caused agents (and users)

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.

"This never drops diff content" isn't accurate, and a doc comment is the wrong place to overstate it, since it's what future readers will trust.

Still dropped after this change: \ No newline at end of file (line 378), and everything between diff --git and the first @@ — Binary files a/x and b/x differ, similarity index / rename from / rename to, new file mode / deleted file mode. Verified: a staged binary change plus a rename compacts to two bare filenames, with no indication of what happened to either.

Separately, gh pr diff output above RAW_CAP (10 MiB) is still silently line-dropped in src/core/stream.rs:418, upstream of compact_diff — so "completes the never-truncate fix" overstates the reach even for the callers this does cover.

Suggest "drops diff metadata" rather than "never drops content", plus a sentence naming which metadata.

One more, low severity, but it belongs next to this comment because the comment now explicitly promises these lines survive: the !line.starts_with("+++") / !line.starts_with("---") guards mean a content line ++ foo (which arrives as +++ foo) or -- foo (arriving as --- foo) falls through to the context branch — printed, but never counted in added/removed. Verified: a diff turning -- old dashes into ++ new pluses produces no +N -M line at all, while git diff --stat reports one insertion and one deletion. Pre-existing, and orthogonal to the caps, but it undercuts the same promise.

/// to make decisions on incomplete diffs and to get empty results when piping
/// the compacted output into `grep`/`rg` — see #827, which applied the same
/// "never truncate" rule to the sibling `condense_unified_diff` (stdin path).
/// The only compression here is lossless: dropping diff metadata and appending
/// a per-file `+N -M` summary.
pub(crate) fn compact_diff(diff: &str) -> String {

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.

Measured, not inferred. With the caps removed, every retained line still gets format!(" {}", line) — two bytes per line — while the only bytes removed are roughly 140 per file (index, ---, +++, the shortened diff --git). Past about fifteen changed lines the compacted string exceeds the raw input, and never_worse(&raw, &printed) (git.rs:203, git.rs:318, runner.rs:141 for gh pr diff / glab mr diff, pipe_cmd.rs:276) discards it.

Bytes emitted by rtk git show <sha> on this repo:

commit raw pre-patch post-patch
87702bf 31524 19375 31140
4147e99 31548 20162 31549 (= raw)
ab48a68 26121 13112 26122 (= raw)
d3f31dd 15385 11661 15386 (= raw)

And on this PR's own 1,200-line scenario, rtk git diff emits 42,641 bytes against 42,504 raw — the output carries diff --git/index headers and zero +/ - indented lines, i.e. it is the raw fallback.

Net effect: roughly 0-1% reduction instead of 35-50% on the main path, and compact_diff becomes dead code for anything non-trivial. A second-order consequence worth knowing about: the emitted format now flips between "stat + Changes: + indented" for small diffs and plain git output for large ones, so anything parsing rtk's diff output has to handle both shapes.

This also means the new regression test at tests/guard_integration_test.rs:270 passes through the raw fallback and never exercises compact_diff at all — CHANGED_0, CHANGED_1199, _finalize and "no truncation marker" are all satisfied by unfiltered git diff output, so re-introducing a cap inside compact_diff would not fail it. Once the prefix is gone, assert on the compacted shape (the +N -M summary) so the test can see the function it's guarding.

let mut result = Vec::new();
let mut current_file = String::new();
let mut added = 0;
let mut removed = 0;
let mut in_hunk = false;
let mut hunk_shown = 0;
let mut hunk_skipped = 0usize;
let max_hunk_lines = 100;
let mut was_truncated = false;

for line in diff.lines() {
if line.starts_with("diff --git") {
// Flush hunk truncation before starting a new file
if hunk_skipped > 0 {
result.push(format!(" ... ({} lines truncated)", hunk_skipped));
was_truncated = true;
hunk_skipped = 0;
}
if !current_file.is_empty() && (added > 0 || removed > 0) {
result.push(format!(" +{} -{}", added, removed));
}
Expand All @@ -362,66 +362,29 @@ pub(crate) fn compact_diff(diff: &str, max_lines: usize) -> String {
added = 0;
removed = 0;
in_hunk = false;
hunk_shown = 0;
} else if line.starts_with("@@") {
// Flush hunk truncation before starting a new hunk
if hunk_skipped > 0 {
result.push(format!(" ... ({} lines truncated)", hunk_skipped));
was_truncated = true;
hunk_skipped = 0;
}
in_hunk = true;
hunk_shown = 0;
// Preserve the full unified diff hunk header, including trailing
// function / symbol context after the second @@ marker.
result.push(format!(" {}", line));
} else if in_hunk {
if line.starts_with('+') && !line.starts_with("+++") {
added += 1;
if hunk_shown < max_hunk_lines {
result.push(format!(" {}", line));
hunk_shown += 1;
} else {
hunk_skipped += 1;
}
result.push(format!(" {}", line));
} else if line.starts_with('-') && !line.starts_with("---") {
removed += 1;
if hunk_shown < max_hunk_lines {
result.push(format!(" {}", line));
hunk_shown += 1;
} else {
hunk_skipped += 1;
}
} else if hunk_shown < max_hunk_lines && !line.starts_with("\\") {
// Context line
if hunk_shown > 0 {
result.push(format!(" {}", line));
hunk_shown += 1;
}
result.push(format!(" {}", line));
} else if !line.starts_with('\\') {
// Context line — kept in full (including leading context, #1852).
result.push(format!(" {}", line));
}
}

if result.len() >= max_lines {
result.push("\n... (more changes truncated)".to_string());
was_truncated = true;
break;
}
}

// Flush last hunk
if hunk_skipped > 0 {
result.push(format!(" ... ({} lines truncated)", hunk_skipped));
was_truncated = true;
}

if !current_file.is_empty() && (added > 0 || removed > 0) {
result.push(format!(" +{} -{}", added, removed));
}

if was_truncated {
result.push("[full diff: rtk git diff --no-compact]".to_string());
}

result.join("\n")
}

Expand Down Expand Up @@ -1763,7 +1726,7 @@ fn run_stash(
}

let filtered = if patch_mode {
compact_diff(&result.stdout, 100)
compact_diff(&result.stdout)
} else {
compact_stash_stat(&result.stdout)
};
Expand Down Expand Up @@ -2220,7 +2183,7 @@ mod tests {
+ println!("hello");
}
"#;
let result = compact_diff(diff, 100);
let result = compact_diff(diff);
assert!(result.contains("foo.rs"));
assert!(result.contains("+"));
}
Expand All @@ -2235,7 +2198,7 @@ mod tests {
+ println!("hello");
}
"#;
let result = compact_diff(diff, 100);
let result = compact_diff(diff);
assert!(
result.contains("@@ -10,3 +10,4 @@ fn important_context() {"),
"Expected full hunk header with trailing context, got:\n{}",
Expand All @@ -2244,36 +2207,49 @@ mod tests {
}

#[test]
fn test_compact_diff_increased_hunk_limit() {
// Build a hunk with 25 changed lines — should NOT be truncated with limit 30
fn test_compact_diff_large_hunk_not_truncated() {
// A single hunk far larger than the old 100-line per-hunk cap must be
// emitted in full — no content dropped, no truncation marker.
let mut diff =
"diff --git a/big.rs b/big.rs\n--- a/big.rs\n+++ b/big.rs\n@@ -1,25 +1,25 @@\n"
"diff --git a/big.rs b/big.rs\n--- a/big.rs\n+++ b/big.rs\n@@ -1,250 +1,250 @@\n"
.to_string();
for i in 1..=25 {
for i in 1..=250 {
diff.push_str(&format!("+line{}\n", i));
}
let result = compact_diff(&diff, 500);
let result = compact_diff(&diff);
assert!(
!result.contains("truncated"),
"large hunk must not be truncated, got:\n{}",
result
);
assert!(result.contains("+line1"));
assert!(
!result.contains("... (truncated)"),
"25 lines should not be truncated with max_hunk_lines=30"
result.contains("+line250"),
"last line of a 250-line hunk must survive"
);
assert!(result.contains("+line25"));
}

#[test]
fn test_compact_diff_increased_total_limit() {
// Build a diff with 150 output result lines across multiple files — should NOT be cut at 100
fn test_compact_diff_many_files_not_truncated() {
// A diff whose compacted output far exceeds the old 500-line global cap
// must be emitted in full — the last file's content must survive so a
// downstream `grep` over the compacted output still matches (#827).
let mut diff = String::new();
for f in 1..=5 {
for f in 1..=40 {
diff.push_str(&format!("diff --git a/file{f}.rs b/file{f}.rs\n--- a/file{f}.rs\n+++ b/file{f}.rs\n@@ -1,20 +1,20 @@\n"));
for i in 1..=20 {
diff.push_str(&format!("+line{f}_{i}\n"));
}
}
let result = compact_diff(&diff, 500);
let result = compact_diff(&diff);
assert!(
!result.contains("more changes truncated"),
"5 files × 20 lines should not exceed max_lines=500"
"40 files must not be truncated"
);
assert!(
result.contains("+line40_20"),
"content in the last file (past the old 500-line cap) must survive, got tail:\n{}",
&result[result.len().saturating_sub(200)..]
);
}

Expand Down Expand Up @@ -3119,9 +3095,9 @@ no changes added to commit (use "git add" and/or "git commit -a")
}

#[test]
fn test_compact_diff_recovery_hint_present() {
// A hunk with 110 lines exceeds max_hunk_lines (100), triggers truncation
// The recovery hint must appear so LLMs can re-fetch the full diff
fn test_compact_diff_no_recovery_hint_when_complete() {
// compact_diff no longer truncates (#827), so it must never emit the
// "[full diff: ...]" recovery hint — the content is already complete.
let mut diff = String::new();
diff.push_str("diff --git a/large.rs b/large.rs\n");
diff.push_str("--- a/large.rs\n");
Expand All @@ -3130,30 +3106,52 @@ no changes added to commit (use "git add" and/or "git commit -a")
for i in 0..110 {
diff.push_str(&format!("+added line {}\n", i));
}
let result = compact_diff(&diff, 500);
let result = compact_diff(&diff);
assert!(
result.contains("[full diff: rtk git diff --no-compact]"),
"Expected recovery hint when hunk is truncated, got:\n{}",
!result.contains("[full diff:"),
"no recovery hint should appear when nothing is dropped, got:\n{}",
result
);
}

#[test]
fn test_compact_diff_hunk_truncation_count_accurate() {
// 150 change lines in one hunk: 100 shown, 50 silently dropped
// Must report the exact count, not just "(truncated)"
fn test_compact_diff_all_hunk_lines_preserved() {
// 150 change lines in one hunk: all 150 must survive (previously 100
// shown, 50 silently dropped with a "50 lines truncated" marker).
let mut diff = String::from(
"diff --git a/large.rs b/large.rs\n--- a/large.rs\n+++ b/large.rs\n@@ -1,150 +1,150 @@\n",
);
for i in 0..150 {
diff.push_str(&format!("+line {}\n", i));
}
let result = compact_diff(&diff, 500);
let result = compact_diff(&diff);
assert!(
!result.contains("truncated"),
"no truncation marker expected, got:\n{}",
result
);
for i in 0..150 {
assert!(
result.contains(&format!("+line {}", i)),
"line {} must be present in full diff",
i
);
}
}

#[test]
fn test_compact_diff_keeps_leading_context() {
// Regression for #1852: context lines that appear *before* the first
// +/- change in a hunk must be kept, not dropped.
let diff = "diff --git a/foo.rs b/foo.rs\n--- a/foo.rs\n+++ b/foo.rs\n@@ -1,5 +1,5 @@\n ctx above 1\n ctx above 2\n-old\n+new\n ctx below\n";
let result = compact_diff(diff);
assert!(
result.contains("50 lines truncated"),
"Expected '50 lines truncated' (150 - 100 = 50), got:\n{}",
result.contains("ctx above 1") && result.contains("ctx above 2"),
"leading context before the first change must be preserved, got:\n{}",
result
);
assert!(result.contains("-old") && result.contains("+new"));
assert!(result.contains("ctx below"));
}

#[test]
Expand Down
2 changes: 1 addition & 1 deletion src/cmds/git/glab_cmd.rs
Original file line number Diff line number Diff line change
Expand Up @@ -486,7 +486,7 @@ fn mr_diff(args: &[String], _verbose: u8) -> Result<i32> {
if stdout.trim().is_empty() {
"No diff\n".to_string()
} else {
git::compact_diff(stdout, 500)
git::compact_diff(stdout)
}
},
RunOptions::stdout_only().early_exit_on_failure(),
Expand Down
2 changes: 1 addition & 1 deletion src/cmds/system/pipe_cmd.rs
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ fn git_log_wrapper(input: &str) -> String {
}

fn git_diff_wrapper(input: &str) -> String {
crate::cmds::git::git::compact_diff(input, 200)
crate::cmds::git::git::compact_diff(input)
}

fn phpstan_wrapper(input: &str) -> String {
Expand Down
37 changes: 37 additions & 0 deletions tests/guard_integration_test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -267,3 +267,40 @@ fn git_checkout_dirty_tree_error_keeps_file_list() {
"dirty checkout failure should keep abort line: {combined:?}"
);
}

#[test]
fn git_diff_large_change_preserves_all_content() {
// Regression for #827: `rtk git diff` routes through compact_diff, which
// previously capped output at 500 lines / 100 lines-per-hunk and silently
// dropped the rest. A large diff must survive in full so that content near
// the end is still present (and a downstream grep over the compacted output
// still matches). This is the same code path `gh pr diff` uses.
let dir = init_git_repo();

// Commit a large baseline file, then rewrite every line so the diff is huge.
let baseline: String = (0..1200).map(|i| format!("line_{i} = {i}\n")).collect();
std::fs::write(dir.path().join("big.py"), &baseline).expect("write baseline");
git_in_dir(dir.path(), &["add", "big.py"]);
git_in_dir(dir.path(), &["commit", "-q", "-m", "add big.py"]);

let mut changed: String = (0..1200).map(|i| format!("CHANGED_{i} = {i}\n")).collect();
// A distinctive marker near the very end — well past the old 500-line cap.
changed.push_str("def _finalize(x):\n return x + 1\n");
std::fs::write(dir.path().join("big.py"), &changed).expect("write changed");

let (out, code) = rtk_in_dir(dir.path(), &["git", "diff"]);

assert_eq!(code, Some(0), "rtk git diff should succeed");
assert!(
!out.contains("more changes truncated") && !out.contains("lines truncated"),
"large diff must not be truncated, got tail:\n{}",
&out[out.len().saturating_sub(300)..]
);
assert!(
out.contains("_finalize"),
"a marker near the end of a large diff must survive compaction"
);
// First and last changed lines both present → nothing dropped from either end.
assert!(out.contains("CHANGED_0 = 0"));
assert!(out.contains("CHANGED_1199 = 1199"));
}