Fix parse patch bug (kpdecker#529)

* Fix patch parser, making most tests pass (but why do I have a weird new failure?...)

* Fix bug I introduced, caught by test failure

* Fix bugs with my tests

* Remove mention of leadingGarbage (introduced by cherry-picking changes from kpdecker#522)

* Improve test coverage

* Add release notes

Author: ExplodingCabbage

release-notes.md

@@ -26,6 +26,7 @@
 - [#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.
 - [#521](https://github.com/kpdecker/jsdiff/pull/521) **the `callback` option is now supported by `structuredPatch`, `createPatch`, and `createTwoFilesPatch`**
+- [#529](https://github.com/kpdecker/jsdiff/pull/522) **`parsePatch` can now parse patches where lines starting with `--` or `++` are deleted/inserted**; previously, there were edge cases where the parser would choke on valid patches or give wrong results.
 
 ## v5.2.0
 

src/patch/parse.js

@@ -92,17 +92,12 @@ export function parsePatch(uniDiff) {
 
     let addCount = 0,
         removeCount = 0;
-    for (; i < diffstr.length; i++) {
-      // Lines starting with '---' could be mistaken for the "remove line" operation
-      // But they could be the header for the next file. Therefore prune such cases out.
-      if (diffstr[i].indexOf('--- ') === 0
-            && (i + 2 < diffstr.length)
-            && diffstr[i + 1].indexOf('+++ ') === 0
-            && diffstr[i + 2].indexOf('@@') === 0) {
-          break;
-      }
+    for (
+      ;
+      i < diffstr.length && (removeCount < hunk.oldLines || addCount < hunk.newLines || diffstr[i]?.startsWith('\\'));
+      i++
+    ) {
       let operation = (diffstr[i].length == 0 && i != (diffstr.length - 1)) ? ' ' : diffstr[i][0];
-
       if (operation === '+' || operation === '-' || operation === ' ' || operation === '\\') {
         hunk.lines.push(diffstr[i]);
 
@@ -115,7 +110,7 @@ export function parsePatch(uniDiff) {
           removeCount++;
         }
       } else {
-        break;
+        throw new Error(`Hunk at line ${chunkHeaderIndex + 1} contained invalid line ${diffstr[i]}`);
       }
     }
 

test/patch/parse.js

@@ -430,8 +430,8 @@ Index: test2
       // Regression test for https://github.com/kpdecker/jsdiff/issues/524
       // Not only are these considered valid by GNU patch, but jsdiff's own formatPatch method
       // emits patches like this, which jsdiff used to then be unable to parse!
-      const patchStr = `--- foo 2024-06-14 22:16:31.444276792 +0100
-+++ bar 2024-06-14 22:17:14.910611219 +0100
+      const patchStr = `--- foo\t2024-06-14 22:16:31.444276792 +0100
++++ bar\t2024-06-14 22:17:14.910611219 +0100
 @@ -1,7 +1,7 @@
  first
  second
@@ -503,8 +503,8 @@ Index: test2
       // determine where a hunk or file ends in a unified diff patch without heeding those line
       // counts.
 
-      const patchStr = `--- foo 2024-06-14 21:57:04.341065736 +0100
-+++ bar 2024-06-14 22:00:57.988080321 +0100
+      const patchStr = `--- foo\t2024-06-14 21:57:04.341065736 +0100
++++ bar\t2024-06-14 22:00:57.988080321 +0100
 @@ -4 +4 @@
 --- bla
 +++ bla
@@ -513,15 +513,35 @@ Index: test2
 `;
 
       expect(parsePatch(patchStr)).to.eql([{
-        oldFileName: 'foo 2024-06-14 21:57:04.341065736 +0100',
-        oldHeader: '',
-        newFileName: 'bar 2024-06-14 22:00:57.988080321 +0100',
-        newHeader: '',
+        oldFileName: 'foo',
+        oldHeader: '2024-06-14 21:57:04.341065736 +0100',
+        newFileName: 'bar',
+        newHeader: '2024-06-14 22:00:57.988080321 +0100',
         hunks: [
           { oldStart: 4, oldLines: 1, newStart: 4, newLines: 1, lines: ['--- bla', '+++ bla'] },
           { oldStart: 7, oldLines: 0, newStart: 7, newLines: 1, lines: ['+seventh'] }
         ]
       }]);
     });
+
+    it('should emit an error if a hunk contains an invalid line', () => {
+      // Within a hunk, every line must either start with '+' (insertion), '-' (deletion),
+      // ' ' (context line, i.e. not deleted nor inserted) or a backslash (for
+      // '\\ No newline at end of file' lines). Seeing anything else before the end of the hunk is
+      // an error.
+
+      const patchStr = `Index: test
+===================================================================
+--- from\theader1
++++ to\theader2
+@@ -1,3 +1,4 @@
+ line2
+line3
++line4
+ line5`;
+
+      // eslint-disable-next-line dot-notation
+      expect(() => {parsePatch(patchStr);}).to.throw('Hunk at line 5 contained invalid line line3');
+    });
   });
 });