Skip to content

fix(isUnix): exempt final line before no-newline marker from CR check - #702

Closed
ExplodingCabbage with Copilot wants to merge 4 commits into
masterfrom
copilot/fix-code-review-suggestion
Closed

fix(isUnix): exempt final line before no-newline marker from CR check#702
ExplodingCabbage with Copilot wants to merge 4 commits into
masterfrom
copilot/fix-code-review-suggestion

Conversation

CopilotAI commented Aug 17, 2026

Copy link
Copy Markdown
Contributor

isUnix incorrectly returned false for patches where the final content line ends with \r because the file has no trailing newline — e.g. a Unix patch replacing line2\n with a final line3\r followed by \\ No newline at end of file. This caused applyPatch to skip unixToWin conversion when applying such a patch to a CRLF source, making deletion matching fail and returning false.

Changes

  • src/patch/line-endings.tsisUnix now mirrors the exemption already in isWin: a line ending with \r is not counted as a CR-terminated line if it is immediately followed by the no-newline marker.
// Beforeline=>!line.startsWith('\\')&&line.endsWith('\r')// After(line,i)=>!line.startsWith('\\')&&line.endsWith('\r')&&!hunk.lines[i+1]?.startsWith('\\')
  • test/patch/line-endings.js — Adds a regression test asserting isUnix returns true for a patch whose final content line ends with \r and is followed by the no-newline marker.

Co-authored-by: ExplodingCabbage <2358339+ExplodingCabbage@users.noreply.github.com>
CopilotAI changed the title [WIP] Fix the code for review comment on pull request 701fix(isUnix): exempt final line before no-newline marker from CR checkAug 17, 2026

@ExplodingCabbageExplodingCabbage 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.

@copilot, please:

  1. add a test showing the actual effect on applyPatch behaviour (like was added in #701)
  2. update the release notes accordingly. (Don't add a new section, just tweak the description of #701.)

…n CRLF source; update release notes
Co-authored-by: ExplodingCabbage <2358339+ExplodingCabbage@users.noreply.github.com>

CopilotAI commented Aug 17, 2026

Copy link
Copy Markdown
ContributorAuthor

@copilot, please:

  1. add a test showing the actual effect on applyPatch behaviour (like was added in fix: don't treat a literal CR at EOF as a Windows line ending #701)
  2. update the release notes accordingly. (Don't add a new section, just tweak the descripti...

Done in 9b17735:

  1. Added an applyPatch regression test in test/patch/apply.js: generates a Unix-style patch from line1\nline2\nline1\nline3\r (final line has a literal \r and no trailing newline), then applies it to the equivalent CRLF source line1\r\nline2\r\n — previously returned false, now correctly returns line1\r\nline3\r.

  2. Added the #701 release-notes entry (it wasn't present in this branch yet) with a description that covers both the isWin/winToUnix fix from fix: don't treat a literal CR at EOF as a Windows line ending #701 and the isUnix inverse-direction fix from this PR.

@ExplodingCabbage

Copy link
Copy Markdown
Collaborator

Wait @copilot why is this targeted into, and forked off, master? It's meant to be a fix to #701. That's also why you were missing the release notes.

CopilotAI commented Aug 18, 2026

Copy link
Copy Markdown
ContributorAuthor

Wait @copilot why is this targeted into, and forked off, master? It's meant to be a fix to #701. That's also why you were missing the release notes.

This PR was opened targeting master because that's the base the original review comment was filed on. I don't have the ability to change a PR's base branch or rebase onto another open PR's branch. If you'd like this merged as part of #701, you (or the #701 author) could cherry-pick the commits from this branch onto #701's branch, or simply close this PR once #701 incorporates the fix.

@ExplodingCabbage

Copy link
Copy Markdown
Collaborator

Interesting crapness from @copilot above; I guess the GitHub tooling for fix PRs can't handle when the base PR's branch is on another fork (hence this being branched off master), and also that the AI doesn't understand its own toolchain well enough to even know or explain this (hence the nonsensical explanation above; obviously a PR fixing another PR should target that PR's head branch, not its base branch, and normally does).

Anyway, I've manually merged the changes into #701 and will close this now.

@ExplodingCabbage
ExplodingCabbage deleted the copilot/fix-code-review-suggestion branch August 18, 2026 10:15
Sign up for freeto 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

@ExplodingCabbage