Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove linedelimiters, improve handling of Windows vs Unix line endings #518

Open
wants to merge 16 commits into
base: master
Choose a base branch
from

Conversation

ExplodingCabbage
Copy link
Collaborator

@ExplodingCabbage ExplodingCabbage commented May 14, 2024

The linedelimiters property was added to patch objects in #141 / #144 but IMO it never made any sense. I take several issues with it:

  • It doesn't add any extra information. Just optionally having a \r character at the end of a line would be simpler.
  • It's cased inconsistently; other properties on hunks like toPos use camelCase, but linedelimiters is all lowercase for some reason.
  • Most importantly: even with linedelimiters added, jsdiff had no sensible support for Windows-Unix interoperability.

To spell out what I mean by the last point, consider what happens when we try to apply this patch, with Unix line endings...

'--- file1.txt   2024-05-10 17:59:04.092510283 +0100\n' +
  '+++ file2.txt   2024-05-10 17:59:16.968023528 +0100\n' +
  '@@ -1,4 +1,4 @@\n' +
  ' foo\n' +
  '-bar\n' +
  '-baz\n' +
  '+barry\n' +
  '+basmati rice\n' +
  ' qux\n'

... to this source file, with Windows line endings:

'foo\r\nbar\r\nbaz\r\nqux\r\n'

We get this result:

> diff.applyPatch(filewin, patch)
'foo\r\nbarry\nbasmati rice\nqux\r\n'

Is this useful to anyone? The logic introduced in #144 means that this doesn't outright fail to apply due to the line ending mismatch, but instead we end up with output that's mixing Windows-style and Unix-style line endings. Surely that's never what anyone's going to want? If we're going to make a Unix-style patch be applicable to a Windows-style text file, then we should go the whole way and have the inserted lines end up with Windows-style line endings too - otherwise we're simply broken in a more complicated way than if the patch outright failed to apply, like I think it would've done before #144.

This PR makes jsdiff go the whole way: we now, by default, automatically convert a patch being applied to use the line ending style of the file being patched.

Resolves #434

@ExplodingCabbage ExplodingCabbage self-assigned this May 14, 2024
@ExplodingCabbage ExplodingCabbage changed the title Remove linedelimiters Remove linedelimiters, improve handling of Windows vs Unix line endings May 31, 2024
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.

Eliminate linedelimiters from structured diff hunks
1 participant