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

* Remove linedelimiters

* Add a util for converting patches between Windows & Unix line endings

* Add some more utils

* Remove lingering linedelimiters references

* Fix linting errors

* Add patch line ending autoconversion and tests thereof

* Document autoConvertLineEndings

* Add release notes

* Add a failing test I need to fix

* Add stubs of more necessary tests

* More tests, fix a test name

* Make a test pass; fix some test names

* Get tests passing

* Improve coverage

* Add another test to get over the coverage threshold

* Remove meaningless/inappropriate 'Create patch' comment - both from original location and places I dumbly copied it to

Author: ExplodingCabbage

README.md

@@ -125,6 +125,7 @@ Broadly, jsdiff's diff functions all take an old text and a new text and perform
     The optional `options` object may have the following keys:
 
     - `fuzzFactor`: Number of lines that are allowed to differ before rejecting a patch. Defaults to 0.
+    - `autoConvertLineEndings`: If `true`, and if the file to be patched consistently uses different line endings to the patch (i.e. either the file always uses Unix line endings while the patch uses Windows ones, or vice versa), then `applyPatch` will behave as if the line endings in the patch were the same as those in the source file. (If `false`, the patch will usually fail to apply in such circumstances since lines deleted in the patch won't be considered to match those in the source file.) Defaults to `true`.
     - `compareLine(lineNumber, line, operation, patchContent)`: Callback used to compare to given lines to determine if they should be considered equal when patching. Defaults to strict equality but may be overridden to provide fuzzier comparison. Should return false if the lines should be rejected.
 
 * `Diff.applyPatches(patch, options)` - applies one or more patches.

release-notes.md

@@ -24,6 +24,7 @@
 - [#486](https://github.com/kpdecker/jsdiff/pull/486) **The `ignoreWhitespace` option of `diffLines` behaves more sensibly now.** `value`s in returned change objects now include leading/trailing whitespace even when `ignoreWhitespace` is used, just like how with `ignoreCase` the `value`s still reflect the case of one of the original texts instead of being all-lowercase. `ignoreWhitespace` is also now compatible with `newlineIsToken`. Finally, **`diffTrimmedLines` is deprecated** (and removed from the docs) in favour of using `diffLines` with `ignoreWhitespace: true`; the two are, and always have been, equivalent.
 - [#490](https://github.com/kpdecker/jsdiff/pull/490) **When calling diffing functions in async mode by passing a `callback` option, the diff result will now be passed as the *first* argument to the callback instead of the second.** (Previously, the first argument was never used at all and would always have value `undefined`.)
 - [#489](github.com/kpdecker/jsdiff/pull/489) **`this.options` no longer exists on `Diff` objects.** Instead, `options` is now passed as an argument to methods that rely on options, like `equals(left, right, options)`. This fixes a race condition in async mode, where diffing behaviour could be changed mid-execution if a concurrent usage of the same `Diff` instances overwrote its `options`.
+- [#518](https://github.com/kpdecker/jsdiff/pull/518) **`linedelimiters` no longer exists** on patch objects; instead, when a patch with Windows-style CRLF line endings is parsed, **the lines in `lines` will end with `\r`**. There is now a **new `autoConvertLineEndings` option, on by default**, which makes it so that when a patch with Windows-style line endings is applied to a source file with Unix style line endings, the patch gets autoconverted to use Unix-style line endings, and when a patch with Unix-style line endings is applied to a source file with Windows-style line endings, it gets autoconverted to use Windows-style line endings.
 
 ## v5.2.0
 

src/patch/apply.js

@@ -1,3 +1,5 @@
+import {hasOnlyWinLineEndings, hasOnlyUnixLineEndings} from '../util/string';
+import {isWin, isUnix, unixToWin, winToUnix} from './line-endings';
 import {parsePatch} from './parse';
 import distanceIterator from '../util/distance-iterator';
 
@@ -14,9 +16,16 @@ export function applyPatch(source, uniDiff, options = {}) {
     uniDiff = uniDiff[0];
   }
 
+  if (options.autoConvertLineEndings || options.autoConvertLineEndings == null) {
+    if (hasOnlyWinLineEndings(source) && isUnix(uniDiff)) {
+      uniDiff = unixToWin(uniDiff);
+    } else if (hasOnlyUnixLineEndings(source) && isWin(uniDiff)) {
+      uniDiff = winToUnix(uniDiff);
+    }
+  }
+
   // Apply the diff to the input
-  let lines = source.split(/\r?\n/),
-      delimiters = source.match(/\r?\n/g) || [],
+  let lines = source.split('\n'),
       hunks = uniDiff.hunks,
 
       compareLine = options.compareLine || ((lineNumber, line, operation, patchContent) => line === patchContent),
@@ -88,18 +97,15 @@ export function applyPatch(source, uniDiff, options = {}) {
     for (let j = 0; j < hunk.lines.length; j++) {
       let line = hunk.lines[j],
           operation = (line.length > 0 ? line[0] : ' '),
-          content = (line.length > 0 ? line.substr(1) : line),
-          delimiter = hunk.linedelimiters && hunk.linedelimiters[j] || '\n';
+          content = (line.length > 0 ? line.substr(1) : line);
 
       if (operation === ' ') {
         toPos++;
       } else if (operation === '-') {
         lines.splice(toPos, 1);
-        delimiters.splice(toPos, 1);
       /* istanbul ignore else */
       } else if (operation === '+') {
         lines.splice(toPos, 0, content);
-        delimiters.splice(toPos, 0, delimiter);
         toPos++;
       } else if (operation === '\\') {
         let previousOperation = hunk.lines[j - 1] ? hunk.lines[j - 1][0] : null;
@@ -116,16 +122,11 @@ export function applyPatch(source, uniDiff, options = {}) {
   if (removeEOFNL) {
     while (!lines[lines.length - 1]) {
       lines.pop();
-      delimiters.pop();
     }
   } else if (addEOFNL) {
     lines.push('');
-    delimiters.push('\n');
-  }
-  for (let _k = 0; _k < lines.length - 1; _k++) {
-    lines[_k] = lines[_k] + delimiters[_k];
   }
-  return lines.join('');
+  return lines.join('\n');
 }
 
 // Wrapper that supports multiple file patches via callbacks.

src/patch/line-endings.js

@@ -0,0 +1,62 @@
+export function unixToWin(patch) {
+  if (Array.isArray(patch)) {
+    return patch.map(unixToWin);
+  }
+
+  return {
+    ...patch,
+    hunks: patch.hunks.map(hunk => ({
+      ...hunk,
+      lines: hunk.lines.map(
+        (line, i) =>
+          (line.startsWith('\\') || line.endsWith('\r') || hunk.lines[i + 1]?.startsWith('\\'))
+            ? line
+            : line + '\r'
+      )
+    }))
+  };
+}
+
+export function winToUnix(patch) {
+  if (Array.isArray(patch)) {
+    return patch.map(winToUnix);
+  }
+
+  return {
+    ...patch,
+    hunks: patch.hunks.map(hunk => ({
+      ...hunk,
+      lines: hunk.lines.map(line => line.endsWith('\r') ? line.substring(0, line.length - 1) : line)
+    }))
+  };
+}
+
+/**
+ * Returns true if the patch consistently uses Unix line endings (or only involves one line and has
+ * no line endings).
+ */
+export function isUnix(patch) {
+  if (!Array.isArray(patch)) { patch = [patch]; }
+  return !patch.some(
+    index => index.hunks.some(
+      hunk => hunk.lines.some(
+        line => !line.startsWith('\\') && line.endsWith('\r')
+      )
+    )
+  );
+}
+
+/**
+ * Returns true if the patch uses Windows line endings and only Windows line endings.
+ */
+export function isWin(patch) {
+  if (!Array.isArray(patch)) { patch = [patch]; }
+  return patch.some(index => index.hunks.some(hunk => hunk.lines.some(line => line.endsWith('\r'))))
+    && patch.every(
+      index => index.hunks.every(
+        hunk => hunk.lines.every(
+          (line, i) => line.startsWith('\\') || line.endsWith('\r') || hunk.lines[i + 1]?.startsWith('\\')
+        )
+      )
+    );
+}

src/patch/parse.js

@@ -1,6 +1,5 @@
 export function parsePatch(uniDiff) {
-  let diffstr = uniDiff.split(/\r?\n/),
-      delimiters = uniDiff.match(/\r?\n/g) || [],
+  let diffstr = uniDiff.split(/\n/),
       list = [],
       i = 0;
 
@@ -51,7 +50,7 @@ export function parsePatch(uniDiff) {
   // Parses the --- and +++ headers, if none are found, no lines
   // are consumed.
   function parseFileHeader(index) {
-    const fileHeader = (/^(---|\+\+\+)\s+(.*)$/).exec(diffstr[i]);
+    const fileHeader = (/^(---|\+\+\+)\s+(.*)\r?$/).exec(diffstr[i]);
     if (fileHeader) {
       let keyPrefix = fileHeader[1] === '---' ? 'old' : 'new';
       const data = fileHeader[2].split('\t', 2);
@@ -78,8 +77,7 @@ export function parsePatch(uniDiff) {
       oldLines: typeof chunkHeader[2] === 'undefined' ? 1 : +chunkHeader[2],
       newStart: +chunkHeader[3],
       newLines: typeof chunkHeader[4] === 'undefined' ? 1 : +chunkHeader[4],
-      lines: [],
-      linedelimiters: []
+      lines: []
     };
 
     // Unified Diff Format quirk: If the chunk size is 0,
@@ -107,7 +105,6 @@ export function parsePatch(uniDiff) {
 
       if (operation === '+' || operation === '-' || operation === ' ' || operation === '\\') {
         hunk.lines.push(diffstr[i]);
-        hunk.linedelimiters.push(delimiters[i] || '\n');
 
         if (operation === '+') {
           addCount++;

src/patch/reverse.js

@@ -15,7 +15,6 @@ export function reversePatch(structuredPatch) {
         oldStart: hunk.newStart,
         newLines: hunk.oldLines,
         newStart: hunk.oldStart,
-        linedelimiters: hunk.linedelimiters,
         lines: hunk.lines.map(l => {
           if (l.startsWith('-')) { return `+${l.slice(1)}`; }
           if (l.startsWith('+')) { return `-${l.slice(1)}`; }

src/util/string.js

@@ -86,3 +86,18 @@ function overlapCount(a, b) {
   }
   return k;
 }
+
+
+/**
+ * Returns true if the string consistently uses Windows line endings.
+ */
+export function hasOnlyWinLineEndings(string) {
+  return string.includes('\r\n') && !string.match(/(?<!\r)\n/);
+}
+
+/**
+ * Returns true if the string consistently uses Unix line endings.
+ */
+export function hasOnlyUnixLineEndings(string) {
+  return !string.includes('\r\n') && string.includes('\n');
+}

test/patch/apply.js

@@ -201,7 +201,6 @@ describe('patch/apply', function() {
     });
 
     it('should apply patches', function() {
-      // Create patch
       const oldFile =
         'value\n'
         + 'context\n'
@@ -755,6 +754,76 @@ describe('patch/apply', function() {
       expect(applyPatch(fileContents, patch))
         .to.equal('');
     });
+
+    it('should automatically convert a patch with Unix file endings to Windows when patching a Windows file', () => {
+      const oldFile = 'foo\r\nbar\r\nbaz\r\nqux\r\n';
+      const diffFile =
+        'Index: testFileName\n'
+        + '===================================================================\n'
+        + '--- testFileName\tOld Header\n'
+        + '+++ testFileName\tNew Header\n'
+        + '@@ -2,2 +2,3 @@\n'
+        + '-bar\n'
+        + '-baz\n'
+        + '+new\n'
+        + '+two\n'
+        + '+three\n';
+
+      expect(applyPatch(oldFile, diffFile)).to.equal('foo\r\nnew\r\ntwo\r\nthree\r\nqux\r\n');
+    });
+
+    it('should automatically convert a patch with Windows file endings to Unix when patching a Unix file', () => {
+      const oldFile = 'foo\nbar\nbaz\nqux\n';
+      const diffFile =
+        'Index: testFileName\r\n'
+        + '===================================================================\r\n'
+        + '--- testFileName\tOld Header\r\n'
+        + '+++ testFileName\tNew Header\r\n'
+        + '@@ -2,2 +2,3 @@\r\n'
+        + '-bar\r\n'
+        + '-baz\r\n'
+        + '+new\r\n'
+        + '+two\r\n'
+        + '+three\r\n';
+
+      expect(applyPatch(oldFile, diffFile)).to.equal('foo\nnew\ntwo\nthree\nqux\n');
+    });
+
+    it('should leave line endings in the patch alone if the target file has mixed file endings, even if this means the patch does not apply', () => {
+      const oldFile1 = 'foo\r\nbar\nbaz\nqux\n';
+      const oldFile2 = 'foo\nbar\r\nbaz\r\nqux\n';
+      const diffFile =
+        'Index: testFileName\r\n'
+        + '===================================================================\r\n'
+        + '--- testFileName\tOld Header\r\n'
+        + '+++ testFileName\tNew Header\r\n'
+        + '@@ -2,2 +2,3 @@\r\n'
+        + '-bar\r\n'
+        + '-baz\r\n'
+        + '+new\r\n'
+        + '+two\r\n'
+        + '+three\r\n';
+
+      expect(applyPatch(oldFile1, diffFile)).to.equal(false);
+      expect(applyPatch(oldFile2, diffFile)).to.equal('foo\nnew\r\ntwo\r\nthree\r\nqux\n');
+    });
+
+    it('should leave patch file endings alone if autoConvertLineEndings=false', () => {
+      const oldFile = 'foo\r\nbar\r\nbaz\r\nqux\r\n';
+      const diffFile =
+        'Index: testFileName\n'
+        + '===================================================================\n'
+        + '--- testFileName\tOld Header\n'
+        + '+++ testFileName\tNew Header\n'
+        + '@@ -2,2 +2,3 @@\n'
+        + '-bar\n'
+        + '-baz\n'
+        + '+new\n'
+        + '+two\n'
+        + '+three\n';
+
+      expect(applyPatch(oldFile, diffFile, {autoConvertLineEndings: false})).to.equal(false);
+    });
   });
 
   describe('#applyPatches', function() {

test/patch/create.js

@@ -809,10 +809,6 @@ describe('patch/create', function() {
               lines: [
                 '-xxx',
                 '+yyy'
-              ],
-              linedelimiters: [
-                '\n',
-                '\n'
               ]
             }
           ]
@@ -831,10 +827,6 @@ describe('patch/create', function() {
               lines: [
                 '-aaa',
                 '+bbb'
-              ],
-              linedelimiters: [
-                '\n',
-                '\n'
               ]
             }
           ]
@@ -875,18 +867,14 @@ describe('patch/create', function() {
 
       // Check 2: starting with a structuredPatch, does formatting and then
       // parsing again basically round-trip as long as we wrap it in an array
-      // to match the output of parsePatch and delete the linedelimiters that
-      // parsePatch puts in?
+      // to match the output of parsePatch?
       const patchObj = structuredPatch(
         'oldfile', 'newfile',
         'line2\nline3\nline4\n', 'line2\nline3\nline5',
         'header1', 'header2'
       );
 
       const roundTrippedPatch = parsePatch(formatPatch([patchObj]));
-      for (const hunk of roundTrippedPatch[0].hunks) {
-        delete hunk.linedelimiters;
-      }
 
       expect(roundTrippedPatch).to.deep.equal([patchObj]);
     });