fix(apply-patch): preserve CRLF line endings on Windows when updating files #4017
Conversation
… files (Fixes openai#4003) - Detect CRLF in original file, strip trailing \r for matching, and re-emit using original EOL style - Add test to ensure updated files retain CRLF endings and avoid mixed EOLs
|
All contributors have signed the CLA ✍️ ✅ |
|
I have read the CLA Document and I hereby sign the CLA |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
Codex Review: Here are some suggestions.
About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you open a pull request for review, mark a draft as ready, or comment "@codex review". If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex fix this CI failure" or "@codex address that feedback".
…d unintended newline conversion\n\n- Count total vs CRLF pairs and treat as CRLF only if every newline is CRLF\n- Prevents converting LF-formatted files that contain embedded in literals
|
Thanks for the contribution, and apologies for the slow reply. We received many PRs, and we're just now catching up on the backlog. It looks like the PR has gone stale. If you're still interested in pursuing the PR, could you please resolve the merge conflicts? Thanks! I haven't reviewed the change yet, but once it's in a good state, either I or one of my colleagues will review it. |
|
@etraut-openai Ill resolve the merge conflicts and comment here once finished! Yes, still interested in this PR! That sounds good. Thank you. |
|
We received another PR that addresses this issue, so I'm going to close this one. Thanks again for taking the time to submit this PR. |
|
@etraut-openai I'd love to see this fixed 😄 |
|
@cnaples79 In addition to mixed line endings, there is an interesting anomaly that occurs when using the VS Code extension, where a patch involving multiple files fails to show its content. Here's an example of a patch that involved a line ending change: 
Here's an example of a patch that didn't change line endings: 
Do you know if this pull request also fixes that issue? |
|
@cnaples79 as an FYI, I'm iterating on this PR in #7515 so that we can introduce this fix in a controlled manner. We appreciate your contribution here, and welcome thoughts on the other PR. You'll be credited for your work in the Release Notes once it is merged! |
## Summary This PR is heavily based on #4017, which contains the core logic for the fix. To reduce the risk, we are first introducing it only on windows. We can then expand to wsl / other environments as needed, and then tackle net new files. ## Testing - [x] added unit tests in apply-patch - [x] add integration tests to apply_patch_cli.rs --------- Co-authored-by: Chase Naples <[email protected]>
|
@cnaples79 thanks again for the PR. |
4 participants
Summary
Patched files on Windows could end up with mixed CRLF/LF endings when applying Update File hunks. This change detects CRLF in the original file, normalizes lines for matching, and re-emits updated content using the original EOL style.
Rationale
Issue #4003 reports mixed EOLs after patch application on Windows. split/join used unconditionally, which produced LF for newly inserted lines even when the original file used CRLF.
Changes
Testing
Fixes #4003