fix(core): detect mixed line endings instead of flagging CRLF on a single match - #28983
fix(core): detect mixed line endings instead of flagging CRLF on a single match#28983shoemoney wants to merge 2 commits into
Conversation
…ngle match 🧵 detectLineEnding() returned '\r\n' if the content contained even one CRLF sequence anywhere, no matter how many plain LF lines surrounded it. edit.ts and write-file.ts both use that single verdict to decide whether to rewrite every line ending in the whole file on write. A file that is mostly LF but picked up one stray CRLF line (a pasted Windows snippet, a partial conversion) would get entirely converted to CRLF the next time any unrelated single-line edit touched it - a 1-line intended change silently becoming a file-wide diff. Make detection purity-based: only report CRLF when every newline in the content is part of a CRLF pair. Mixed content now reports '\n', which is the safe branch - `if (useCRLF)` has no else in either consumer, so when it's false the content is written through unchanged rather than being force-normalized. Added mixed-EOL coverage to line-endings.test.ts, which previously only exercised pure-CRLF and pure-LF fixtures.
|
📊 PR Size: size/M
|
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses an issue where the line ending detection logic was overly aggressive, causing files with mixed line endings to be incorrectly classified as CRLF. By refining the detection heuristic to require purity, the system now avoids unnecessary and destructive wholesale rewriting of line endings during minor file edits, improving the reliability of file modifications. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request refactors the line ending detection logic in detectLineEnding to ensure that files with mixed line endings (e.g., mostly LF with a single stray CRLF) are classified as LF rather than CRLF, preventing unintended wholesale conversions during edits. It also adds comprehensive tests to verify this behavior. The reviewer suggested an excellent performance optimization to replace the double string scanning and array allocations with a single regex test using a negative lookbehind assertion.
| const crlfCount = (content.match(/\r\n/g) || []).length; | ||
| const totalNewlines = (content.match(/\n/g) || []).length; | ||
| return totalNewlines > 0 && crlfCount === totalNewlines ? '\r\n' : '\n'; |
There was a problem hiding this comment.
Using content.match(/\r\n/g) and content.match(/\n/g) scans the entire string twice and allocates two arrays containing all matches. For large files, this can lead to significant memory overhead and garbage collection pressure.
We can optimize this to \n and does not contain any \n that is not preceded by \r (using a negative lookbehind assertion (?<!\r)\n). Since Node.js >= 20 is used, lookbehind assertions are fully supported.
| const crlfCount = (content.match(/\r\n/g) || []).length; | |
| const totalNewlines = (content.match(/\n/g) || []).length; | |
| return totalNewlines > 0 && crlfCount === totalNewlines ? '\r\n' : '\n'; | |
| return content.includes('\n') && !/(?<!\r)\n/.test(content) ? '\r\n' : '\n'; |
There was a problem hiding this comment.
Good catch, took the suggestion as written. Pushed in decad6b.
Checked equivalence with the counting version across the cases that matter: pure CRLF -> '\r\n', pure LF -> '\n', mixed (any lone \n) -> '\n', empty/no-newline -> '\n'. Also checked a lone-CR input (old-Mac \r with no \n at all) since the lookbehind reads differently from a naive glance at the counting code - that one falls through the includes('\n') check and returns '\n', same as before. Added a unit test for it.
Reran the same red/green/red cycle from the PR description against this change: the two regression tests in line-endings.test.ts still fail if you revert just textUtils.ts back to the pre-fix content.includes('\r\n') heuristic, and pass with either the counting version or this one. Full suite (textUtils.test.ts, edit.test.ts, write-file.test.ts, line-endings.test.ts) is 170/170 now (169 + the new lone-CR case). eslint/prettier clean.
…nding ⚡ Replace the match()-based CRLF/LF counting in detectLineEnding with a single includes() check plus a negative-lookbehind test, per review on google-gemini#28983. Same purity semantics (CRLF only when every \n is part of a CRLF pair), no allocations, single pass. Added a lone-CR unit case to lock in that a bare \r with no \n at all still falls back to \n.
|
Hi there! Thank you for your interest in contributing to Gemini CLI. To ensure we maintain high code quality and focus on our prioritized roadmap, we only guarantee review and consideration of pull requests for issues that are explicitly labeled as 'help wanted'. This PR will be closed in 7 days if it remains without that designation. We encourage you to find and contribute to existing 'help wanted' issues in our backlog! Thank you for your understanding. |
Summary
detectLineEnding()inpackages/core/src/utils/textUtils.tsclassifies afile as CRLF if it contains even one
\r\nanywhere in the content, nomatter how many plain LF lines surround it:
Both
edit.tsandwrite-file.tsuse that single verdict to decide whetherto rewrite every line ending in the whole file on write (
edit.tsline~937:
const useCRLF = (!editData.isNewFile && editData.originalLineEnding === '\r\n') || ..., followed byfinalContent.replace(/\r?\n/g, '\r\n')).Concretely: a file with 99 LF lines and one stray CRLF line (a pasted
Windows snippet, or a partial line-ending conversion someone missed) gets
classified as a CRLF file. The next time an unrelated single-line edit
touches that file, every LF line is silently rewritten to CRLF - a 1-line
intended change turns into a 100-line diff, with nothing telling you it
happened.
Details
Made detection purity-based:
'\r\n'is only returned when CRLF is theonly newline style present in the content.
I did not touch
edit.tsorwrite-file.ts. Both calluseCRLFwith noelsebranch - when it's false the content is written through unmodified.So
'\n'is already the safe, non-destructive verdict; this change onlyaffects which files get classified that way. A file that's actually pure
CRLF still round-trips as CRLF (no regression to the existing behavior
that PR #16087 added).
This is a residual gap in the fix from #16087 / issue #9604, which added
detectLineEndingin the first place to stop line endings from being"unpredictably converted." It didn't cover mixed-EOL files. I'm not
claiming to fix #9604 or #16087 - just closing a gap they left open.
The existing
line-endings.test.tsonly had pure-CRLF and pure-LFfixtures, which is exactly why this passed CI. I added:
EditTool: editing one line in amostly-LF file that has a stray CRLF line no longer forces the whole
file to CRLF
Related Issues
Related to #9604 (not fixing it directly - that issue covers the broader
"line endings unpredictably converted" problem; this closes one specific
gap in the
detectLineEndingheuristic that #16087 introduced).How to Validate
From
packages/core:To see the bug reproduce, revert the change in
textUtils.tsonly (keepthe new tests) and rerun - two tests fail, and the failing edit-path test
shows the whole file getting rewritten to CRLF:
I ran the fix through this cycle (tests red against unpatched code, green
against the fix, red again after reverting the fix) before opening this PR.
I also ran the broader adjacent suite to check for regressions:
All 169 tests pass. Also ran
eslintandprettier --checkon bothchanged files, clean.
Pre-Merge Checklist