From a0e501c1af966dba67584b195a3d903afc87740c Mon Sep 17 00:00:00 2001 From: Mark Amery Date: Mon, 29 Jul 2024 09:13:15 +0100 Subject: [PATCH 1/6] Add (failing) test case from https://github.com/kpdecker/jsdiff/issues/531 --- test/patch/create.js | 32 ++++++++++++++++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/test/patch/create.js b/test/patch/create.js index 7811b3b8..2ca27308 100644 --- a/test/patch/create.js +++ b/test/patch/create.js @@ -106,6 +106,38 @@ 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( + '===================================================================\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' + ); + }); + 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' From d0aec10b3e8a7e57dcde56953dfec9ace1dddf52 Mon Sep 17 00:00:00 2001 From: Mark Amery Date: Mon, 29 Jul 2024 13:54:43 +0100 Subject: [PATCH 2/6] Fix test I just added (the test itself, not the jsdiff behaviour) --- test/patch/create.js | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/test/patch/create.js b/test/patch/create.js index 2ca27308..599fbcf0 100644 --- a/test/patch/create.js +++ b/test/patch/create.js @@ -120,7 +120,8 @@ describe('patch/create', function() { ); expect(diff).to.equal( - '===================================================================\n' + 'Index: a.txt\n' + + '===================================================================\n' + '--- a.txt\n' + '+++ a.txt\n' + '@@ -1,3 +1,7 @@\n' @@ -134,7 +135,7 @@ describe('patch/create', function() { + '+3rd line.\n' + '+\n' + '+SOMETHING ELSE.\n' - + '\\ No newline at end of file' + + '\\ No newline at end of file\n' ); }); From 981736f60073dd5dc5556dd68348a91495e5aabc Mon Sep 17 00:00:00 2001 From: Mark Amery Date: Mon, 29 Jul 2024 14:06:41 +0100 Subject: [PATCH 3/6] Fix the new test (but make an old one fail as a side effect) --- src/patch/create.js | 49 +++++++++++++++++++++++++++++++-------------- 1 file changed, 34 insertions(+), 15 deletions(-) diff --git a/src/patch/create.js b/src/patch/create.js index ed3a2718..888e76e7 100644 --- a/src/patch/create.js +++ b/src/patch/create.js @@ -29,6 +29,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 +47,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 +94,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 +106,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 +199,20 @@ 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) { + if (!text) { + return []; + } + 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; +} From 604081990b3c670bd2f2b743204c0b15bc948729 Mon Sep 17 00:00:00 2001 From: Mark Amery Date: Mon, 29 Jul 2024 14:15:39 +0100 Subject: [PATCH 4/6] Stop allowing newlineIsToken to be passed to patch-generation functions (... and causing them to generate patches that can't be applied) --- README.md | 1 - src/patch/create.js | 3 +++ test/patch/create.js | 43 +++++-------------------------------------- 3 files changed, 8 insertions(+), 39 deletions(-) diff --git a/README.md b/README.md index 0916884e..81a96fe6 100644 --- a/README.md +++ b/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. diff --git a/src/patch/create.js b/src/patch/create.js index 888e76e7..de46af63 100644 --- a/src/patch/create.js +++ b/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)); diff --git a/test/patch/create.js b/test/patch/create.js index 599fbcf0..4bee8743 100644 --- a/test/patch/create.js +++ b/test/patch/create.js @@ -702,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', From acddb5de7a20521f890baac7bde4590ed9f15249 Mon Sep 17 00:00:00 2001 From: Mark Amery Date: Mon, 29 Jul 2024 14:20:29 +0100 Subject: [PATCH 5/6] Remove impossible case to placate the coverage checker --- src/patch/create.js | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/patch/create.js b/src/patch/create.js index de46af63..cd6af7e3 100644 --- a/src/patch/create.js +++ b/src/patch/create.js @@ -207,9 +207,6 @@ export function createPatch(fileName, oldStr, newStr, oldHeader, newHeader, opti * Split `text` into an array of lines, including the trailing newline character (where present) */ function splitLines(text) { - if (!text) { - return []; - } const hasTrailingNl = text.endsWith('\n'); const result = text.split('\n').map(line => line + '\n'); if (hasTrailingNl) { From 1917df604178fd1af30e90d693ef0e1a8dca5aab Mon Sep 17 00:00:00 2001 From: Mark Amery Date: Mon, 29 Jul 2024 14:29:38 +0100 Subject: [PATCH 6/6] Add release notes --- release-notes.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/release-notes.md b/release-notes.md index e254a056..7e237cb6 100644 --- a/release-notes.md +++ b/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