Fix handling of EOF in createPatch (#535)

* Add (failing) test case from #531

* Fix test I just added (the test itself, not the jsdiff behaviour)

* Fix the new test (but make an old one fail as a side effect)

* Stop allowing newlineIsToken to be passed to patch-generation functions (... and causing them to generate patches that can't be applied)

* Remove impossible case to placate the coverage checker

* Add release notes

Author: ExplodingCabbage

README.md

@@ -91,7 +91,6 @@ Broadly, jsdiff's diff functions all take an old text and a new text and perform
       - `context` describes how many lines of context should be included. You can set this to `Number.MAX_SAFE_INTEGER` or `Infinity` to include the entire file content in one hunk.
       - `ignoreWhitespace`: Same as in `diffLines`. Defaults to `false`.
       - `stripTrailingCr`: Same as in `diffLines`. Defaults to `false`.
-      - `newlineIsToken`: Same as in `diffLines`. Defaults to `false`.
 
 * `Diff.createPatch(fileName, oldStr, newStr[, oldHeader[, newHeader[, options]]])` - creates a unified diff patch.
 

release-notes.md

@@ -32,6 +32,8 @@
   * The `fuzzFactor` now indicates the maximum [*Levenshtein* distance](https://en.wikipedia.org/wiki/Levenshtein_distance) that there can be between the context shown in a hunk and the actual file content at a location where we try to apply the hunk. (Previously, it represented a maximum [*Hamming* distance](https://en.wikipedia.org/wiki/Hamming_distance), meaning that a single insertion or deletion in the source file could stop a hunk from applying even with a high `fuzzFactor`.)
   * A hunk containing a deletion can now only be applied in a context where the line to be deleted actually appears verbatim. (Previously, as long as enough context lines in the hunk matched, `applyPatch` would apply the hunk anyway and delete a completely different line.)
   * The context line immediately before and immediately after an insertion must match exactly between the hunk and the file for a hunk to apply. (Previously this was not required.)
+- [#535](https://github.com/kpdecker/jsdiff/pull/535) **A bug in patch generation functions is now fixed** that would sometimes previously cause `\ No newline at end of file` to appear in the wrong place in the generated patch, resulting in the patch being invalid.
+- [#535](https://github.com/kpdecker/jsdiff/pull/535) **Passing `newlineIsToken: true` to *patch*-generation functions is no longer allowed.** (Passing it to `diffLines` is still supported - it's only functions like `createPatch` where passing `newlineIsToken` is now an error.) Allowing it to be passed never really made sense, since in cases where the option had any effect on the output at all, the effect tended to be causing a garbled patch to be created that couldn't actually be applied to the source file.
 
 ## v5.2.0
 

src/patch/create.js

@@ -10,6 +10,9 @@ export function structuredPatch(oldFileName, newFileName, oldStr, newStr, oldHea
   if (typeof options.context === 'undefined') {
     options.context = 4;
   }
+  if (options.newlineIsToken) {
+    throw new Error('newlineIsToken may not be used with patch-generation functions, only with diffing functions');
+  }
 
   if (!options.callback) {
     return diffLinesResultToPatch(diffLines(oldStr, newStr, options));
@@ -29,6 +32,9 @@ export function structuredPatch(oldFileName, newFileName, oldStr, newStr, oldHea
   }
 
   function diffLinesResultToPatch(diff) {
+    // STEP 1: Build up the patch with no "\ No newline at end of file" lines and with the arrays
+    //         of lines containing trailing newline characters. We'll tidy up later...
+
     if(!diff) {
       return;
     }
@@ -44,7 +50,7 @@ export function structuredPatch(oldFileName, newFileName, oldStr, newStr, oldHea
         oldLine = 1, newLine = 1;
     for (let i = 0; i < diff.length; i++) {
       const current = diff[i],
-            lines = current.lines || current.value.replace(/\n$/, '').split('\n');
+            lines = current.lines || splitLines(current.value);
       current.lines = lines;
 
       if (current.added || current.removed) {
@@ -91,20 +97,6 @@ export function structuredPatch(oldFileName, newFileName, oldStr, newStr, oldHea
               newLines: (newLine - newRangeStart + contextSize),
               lines: curRange
             };
-            if (i >= diff.length - 2 && lines.length <= options.context) {
-              // EOF is inside this hunk
-              let oldEOFNewline = ((/\n$/).test(oldStr));
-              let newEOFNewline = ((/\n$/).test(newStr));
-              let noNlBeforeAdds = lines.length == 0 && curRange.length > hunk.oldLines;
-              if (!oldEOFNewline && noNlBeforeAdds && oldStr.length > 0) {
-                // special case: old has no eol and no trailing context; no-nl can end up before adds
-                // however, if the old file is empty, do not output the no-nl line
-                curRange.splice(hunk.oldLines, 0, '\\ No newline at end of file');
-              }
-              if ((!oldEOFNewline && !noNlBeforeAdds) || !newEOFNewline) {
-                curRange.push('\\ No newline at end of file');
-              }
-            }
             hunks.push(hunk);
 
             oldRangeStart = 0;
@@ -117,6 +109,19 @@ export function structuredPatch(oldFileName, newFileName, oldStr, newStr, oldHea
       }
     }
 
+    // Step 2: eliminate the trailing `\n` from each line of each hunk, and, where needed, add
+    //         "\ No newline at end of file".
+    for (const hunk of hunks) {
+      for (let i = 0; i < hunk.lines.length; i++) {
+        if (hunk.lines[i].endsWith('\n')) {
+          hunk.lines[i] = hunk.lines[i].slice(0, -1);
+        } else {
+          hunk.lines.splice(i + 1, 0, '\\ No newline at end of file');
+          i++; // Skip the line we just added, then continue iterating
+        }
+      }
+    }
+
     return {
       oldFileName: oldFileName, newFileName: newFileName,
       oldHeader: oldHeader, newHeader: newHeader,
@@ -197,3 +202,17 @@ export function createTwoFilesPatch(oldFileName, newFileName, oldStr, newStr, ol
 export function createPatch(fileName, oldStr, newStr, oldHeader, newHeader, options) {
   return createTwoFilesPatch(fileName, fileName, oldStr, newStr, oldHeader, newHeader, options);
 }
+
+/**
+ * Split `text` into an array of lines, including the trailing newline character (where present)
+ */
+function splitLines(text) {
+  const hasTrailingNl = text.endsWith('\n');
+  const result = text.split('\n').map(line => line + '\n');
+  if (hasTrailingNl) {
+    result.pop();
+  } else {
+    result.push(result.pop().slice(0, -1));
+  }
+  return result;
+}

test/patch/create.js

@@ -106,6 +106,39 @@ describe('patch/create', function() {
         + '\\ No newline at end of file\n');
     });
 
+    it('should get the "No newline" position right in the case from https://github.com/kpdecker/jsdiff/issues/531', function() {
+      const oldContent = '1st line.\n2nd line.\n3rd line.';
+      const newContent = 'Z11 thing.\nA New thing.\n2nd line.\nNEW LINE.\n3rd line.\n\nSOMETHING ELSE.';
+
+      const diff = createPatch(
+        'a.txt',
+        oldContent,
+        newContent,
+        undefined,
+        undefined,
+        { context: 3 },
+      );
+
+      expect(diff).to.equal(
+        'Index: a.txt\n'
+        + '===================================================================\n'
+        + '--- a.txt\n'
+        + '+++ a.txt\n'
+        + '@@ -1,3 +1,7 @@\n'
+        + '-1st line.\n'
+        + '+Z11 thing.\n'
+        + '+A New thing.\n'
+        + ' 2nd line.\n'
+        + '-3rd line.\n'
+        + '\\ No newline at end of file\n'
+        + '+NEW LINE.\n'
+        + '+3rd line.\n'
+        + '+\n'
+        + '+SOMETHING ELSE.\n'
+        + '\\ No newline at end of file\n'
+      );
+    });
+
     it('should output "no newline" at end of file message on context missing nl', function() {
       expect(createPatch('test', 'line11\nline2\nline3\nline4', 'line1\nline2\nline3\nline4', 'header1', 'header2')).to.equal(
         'Index: test\n'
@@ -669,47 +702,14 @@ describe('patch/create', function() {
     });
 
     describe('newlineIsToken', function() {
-      it('newlineIsToken: false', function() {
-        const expectedResult =
-          'Index: testFileName\n'
-          + '===================================================================\n'
-          + '--- testFileName\n'
-          + '+++ testFileName\n'
-          + '@@ -1,2 +1,2 @@\n'
-
-          // Diff is shown as entire row, even though text is unchanged
-          + '-line\n'
-          + '+line\r\n'
-
-          + ' line\n'
-          + '\\ No newline at end of file\n';
-
-        const diffResult = createPatch('testFileName', 'line\nline', 'line\r\nline', undefined, undefined, {newlineIsToken: false});
-        expect(diffResult).to.equal(expectedResult);
-      });
-
-      it('newlineIsToken: true', function() {
-        const expectedResult =
-          'Index: testFileName\n'
-          + '===================================================================\n'
-          + '--- testFileName\n'
-          + '+++ testFileName\n'
-          + '@@ -1,3 +1,3 @@\n'
-          + ' line\n'
-
-          // Newline change is shown as a single diff
-          + '-\n'
-          + '+\r\n'
-
-          + ' line\n'
-          + '\\ No newline at end of file\n';
-
-        const diffResult = createPatch('testFileName', 'line\nline', 'line\r\nline', undefined, undefined, {newlineIsToken: true});
-        expect(diffResult).to.equal(expectedResult);
+      // See https://github.com/kpdecker/jsdiff/pull/345#issuecomment-2255886105
+      it("isn't allowed any more, since the patches produced were nonsense", function() {
+        expect(() => {
+          createPatch('testFileName', 'line\nline', 'line\r\nline', undefined, undefined, {newlineIsToken: true});
+        }).to['throw']('newlineIsToken may not be used with patch-generation functions, only with diffing functions');
       });
     });
 
-
     it('takes an optional callback option', function(done) {
       createPatch(
         'test',