Skip to content

fix(git): preserve context lines before first change in compact_diff - #1855

Closed
paolomainardi wants to merge 1 commit into
rtk-ai:developfrom
paolomainardi:fix/compact-diff-context-lines
Closed

paolomainardi wants to merge 1 commit into
rtk-ai:developfrom
paolomainardi:fix/compact-diff-context-lines

Conversation

@paolomainardi

Copy link
Copy Markdown

🤖 This was written by an AI agent on behalf of @paolomainardi.

Summary

  • Removes the if hunk_shown > 0 guard in compact_diff() that silently dropped context lines before the first +/- in each hunk
  • Adds regression test test_compact_diff_preserves_context_before_first_change

Before / After

Before fix After fix
Context lines before change Dropped (0 shown) Preserved (3 shown)
Context lines after change Shown Shown (unchanged)
Token savings on large diffs ~60% ~60% (caps still active)

All existing tests pass (1732 total).

Fixes #1852

@CLAassistant

CLAassistant commented May 12, 2026 •

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

Assisted-by: opencode/github-copilot/claude-opus-4.6

@KuSh KuSh left a comment

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.

Requesting changes, and apologies this sat so long without a response.

The bug you found is real: context lines before the first +/- in a hunk were silently dropped, and if hunk_shown > 0 is the cause. That much is confirmed. The fix needs one more piece, though, because removing the guard without giving context its own budget trades one silent loss for a worse one — details inline. I built both HEAD and your patch and compared real output through rtk pipe -f git-diff to check this rather than reasoning about it; all 2659 unit tests pass with your patch applied.

Suggested direction: keep the fix, but bound leading context separately — cap it at ~3 lines per hunk, or reserve budget for +/- lines so a change can never be crowded out by its own context.

Two notes on the surrounding state, so you know what you're rebasing into:

  • The branch is currently conflicting with develop.
  • #3269 is an open PR touching these same lines (it removes the two-space indent so grep '^-' anchors again). It leaves the hunk_shown > 0 guard alone, so the two changes are complementary — but whichever lands second will need a rebase.

A third thing, pre-existing but newly routine once leading context is emitted: ... (N lines truncated) undercounts, because context lines dropped past max_hunk_lines never reach hunk_skipped (there's no else arm on the context branch). In my repro it reported "2 lines truncated" when 22 were dropped. A one-line else { hunk_skipped += 1; } fixes it and would be welcome in the same PR.

Comment thread src/cmds/git/git.rs
hunk_shown += 1;
}
// Context line — always emit (before and after changes)
result.push(format!(" {}", line));

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.

Emitting every context line unconditionally means leading context now consumes the per-hunk max_hunk_lines budget (100). A hunk with 100 or more leading context lines therefore emits only context and truncates away every actual +/- line.

Repro: 120 context lines followed by one -/+ pair. HEAD shows the change. With this patch the output is let v98, let v99, let v100, ... (2 lines truncated) — the change itself is gone.

That shape is not exotic: git diff -W, --function-context and -U<big> all produce it, and it reaches users through gh pr diff and glab mr diff as well as rtk git diff.

The global max_lines cap is affected the same way — it now fills with context, truncating real changes earlier. On a real 13.8 KB diff from this repo at max_lines=200, changed lines shown drop from 183 to 168 and the file summary goes from +166 -16 to +151 -16.

never_worse() doesn't rescue either case, since the filtered output here is smaller than raw, not larger — so the guard is happy while content is being dropped.

@KuSh

KuSh commented Sep 6, 2026

Copy link
Copy Markdown
Collaborator

Closing in favour of #3022, which makes the same leading context fix and also removes the per hunk and diff wide truncation caps at every call site, with integration coverage. The leading context behaviour has meanwhile also landed on develop through a separate commit. Review and comments are welcome on #3022, or reply here if you think the scope differs.

@KuSh KuSh closed this Sep 6, 2026
@KuSh KuSh added the duplicate This issue or pull request already exists label Sep 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

duplicate This issue or pull request already exists

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(git): compact_diff drops context lines before first change in hunk

3 participants