Skip to content

fix(git): preserve non-LF characters in structured diffs - #253

Merged
Zongwei9888 merged 1 commit into
HKUDS:mainfrom
KennyMcSimpson:fix/git-diff-line-boundaries
Oct 12, 2026
Merged

Zongwei9888 merged 1 commit into
HKUDS:mainfrom
KennyMcSimpson:fix/git-diff-line-boundaries

Conversation

@KennyMcSimpson

Copy link
Copy Markdown
Contributor

Description

Structured Git diffs split file content at characters that Git does not treat as line endings. For example, alpha\u2028beta\nnext\n is two lines to Git, but the untracked-file preview reports three additions. In a tracked patch, the text after U+2028 becomes a metadata row and subsequent line numbers are wrong.

Use LF boundaries for both patch records and untracked content. This keeps Unicode separators, form feeds, vertical tabs and carriage returns in the original line, while handling empty files and final newlines correctly.

Related Issues

No linked issue; reproduced in a temporary Git repository through application.git.diff.

Changes Made

  • Share LF-only line splitting between tracked and untracked diffs.
  • Add regression tests for content, line numbers and counts, with Git's own --numstat as a reference for tracked changes.
  • Cover empty files, missing final newlines, trailing blank lines and CRLF content.

Checklist

  • Changes tested locally
  • Code reviewed
  • Documentation updated (if necessary) — internal comment; no API or usage change
  • Unit tests added (if applicable)

Additional Notes

  • python -m pytest tests/application/test_p4_code_workbench.py -q — 29 passed; 15 new cases fail before the fix.
  • Ruff 0.15.21 lint and formatting checks pass for both changed files; git diff --check passes.
  • Full Python suite: 2249 passed, 28 skipped, 16 failed. These 16 failures also reproduce on the unchanged upstream commit in this container: Bubblewrap cannot create a namespace (Operation not permitted).

@KennyMcSimpson
KennyMcSimpson marked this pull request as ready for review October 5, 2026 11:20
@Zongwei9888
Zongwei9888 merged commit c6e3a5b into HKUDS:main Oct 12, 2026
@Zongwei9888

Copy link
Copy Markdown
Collaborator

Merged in c6e3a5b. Thank you for the fix and the detailed reproduction, @KennyMcSimpson.

What I checked against main before merging:

  • The bug reproduces. For a tracked file changed from before\u2028old\nkeep\u2028 value\n, _parse_file_patch emitted old/new as meta rows and value as an extra context row, so the following line numbers were off by one. An untracked alpha\u2028beta\nnext\n reported 3 additions instead of 2. \f and a lone \r behave the same way.
  • _split_git_lines gives the same result as splitlines() on LF-only text (empty file, \n, missing final newline, trailing blank line). Git always delimits diff records with LF, so hunk headers and the \ No newline at end of file marker are handled as before.
  • Your new tests fail on main and pass with this change; the full suite passes after the merge.

One visible difference, for the record: lines of CRLF files now keep their trailing \r in DiffLine.text, where main stripped it. That matches what Git reports. The desktop Changes panel renders the text in a white-space: pre block, and I checked in Chromium that such a line renders at the same height as a plain one.

The red Windows job you may have seen was unrelated (the runner image's preinstalled pipx conflicted with our CI lock); it is fixed on main in f6ab0e7.

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.

2 participants