Merge pull request #446 from kpdecker/6.0.0-staging

Future 6.0.0 release

README.md

@@ -166,10 +166,13 @@ Certain options can be provided in the `options` object of *any* method that cal
 * `callback`: if provided, the diff will be computed in async mode to avoid blocking the event loop while the diff is calculated. The value of the `callback` option should be a function and will be passed the result of the diff as its second argument. The first argument will always be undefined. Only works with functions that return change objects, like `diffLines`, not those that return patches, like `structuredPatch` or `createPatch`.
 
   (Note that if the ONLY option you want to provide is a callback, you can pass the callback function directly as the `options` parameter instead of passing an object with a `callback` property.)
-* `maxEditLength`: a number specifying the maximum edit distance to consider between the old and new texts. If the edit distance is higher than this, jsdiff will return `undefined` instead of a diff. You can use this to limit the computational cost of diffing large, very different texts by giving up early if the cost will be huge. Works for functions that return change objects and also for `structuredPatch`, but not other patch-generation functions.
+
+* `maxEditLength`: a number specifying the maximum edit distance to consider between the old and new texts. You can use this to limit the computational cost of diffing large, very different texts by giving up early if the cost will be huge. This option can be passed either to diffing functions (`diffLines`, `diffChars`, etc) or to patch-creation function (`structuredPatch`, `createPatch`, etc), all of which will indicate that the max edit length was reached by returning `undefined` instead of whatever they'd normally return.
 
 * `timeout`: a number of milliseconds after which the diffing algorithm will abort and return `undefined`. Supported by the same functions as `maxEditLength`.
 
+* `oneChangePerToken`: if `true`, the array of change objects returned will contain one change object per token (e.g. one per line if calling `diffLines`), instead of runs of consecutive tokens that are all added / all removed / all conserved being combined into a single change object.
+
 ### Defining custom diffing behaviors
 
 If you need behavior a little different to what any of the text diffing functions above offer, you can roll your own by customizing both the tokenization behavior used and the notion of equality used to determine if two tokens are equal.
@@ -190,13 +193,11 @@ For even more customisation of the diffing behavior, you can create a `new Diff.
 Many of the methods above return change objects. These objects consist of the following fields:
 
 * `value`: The concatenated content of all the tokens represented by this change object - i.e. generally the text that is either added, deleted, or common, as a single string. In cases where tokens are considered common but are non-identical (e.g. because an option like `ignoreCase` or a custom `comparator` was used), the value from the *new* string will be provided here.
-* `added`: True if the value was inserted into the new string
-* `removed`: True if the value was removed from the old string
+* `added`: true if the value was inserted into the new string, otherwise false
+* `removed`: true if the value was removed from the old string, otherwise false
 * `count`: How many tokens (e.g. chars for `diffChars`, lines for `diffLines`) the value in the change object consists of
 
-(Change objects where `added` and `removed` are both falsey represent content that is common to the old and new strings.)
-
-Note that some cases may omit a particular flag field. Comparison on the flag fields should always be done in a truthy or falsy manner.
+(Change objects where `added` and `removed` are both false represent content that is common to the old and new strings.)
 
 ## Examples
 

release-notes.md

@@ -1,5 +1,17 @@
 # Release Notes
 
+## Future breaking 6.0.0 release
+
+[Commits](https://github.com/kpdecker/jsdiff/compare/master...v6.0.0-staging)
+
+- [#435](https://github.com/kpdecker/jsdiff/pull/435) Fix `parsePatch` handling of control characters. `parsePatch` used to interpret various unusual control characters - namely vertical tabs, form feeds, lone carriage returns without a line feed, and EBCDIC NELs - as line breaks when parsing a patch file. This was inconsistent with the behavior of both JsDiff's own `diffLines` method and also the Unix `diff` and `patch` utils, which all simply treat those control characters as ordinary characters. The result of this discrepancy was that some well-formed patches - produced either by `diff` or by JsDiff itself and handled properly by the `patch` util - would be wrongly parsed by `parsePatch`, with the effect that it would disregard the remainder of a hunk after encountering one of these control characters.
+- [#439](https://github.com/kpdecker/jsdiff/pull/439) Prefer diffs that order deletions before insertions. When faced with a choice between two diffs with an equal total edit distance, the Myers diff algorithm generally prefers one that does deletions before insertions rather than insertions before deletions. For instance, when diffing `abcd` against `acbd`, it will prefer a diff that says to delete the `b` and then insert a new `b` after the `c`, over a diff that says to insert a `c` before the `b` and then delete the existing `c`. JsDiff deviated from the published Myers algorithm in a way that led to it having the opposite preference in many cases, including that example. This is now fixed, meaning diffs output by JsDiff will more accurately reflect what the published Myers diff algorithm would output.
+- [#455](https://github.com/kpdecker/jsdiff/pull/455) The `added` and `removed` properties of change objects are now guaranteed to be set to a boolean value. (Previously, they would be set to `undefined` or omitted entirely instead of setting them to false.)
+- [#464](https://github.com/kpdecker/jsdiff/pull/464) Specifying `{maxEditLength: 0}` now sets a max edit length of 0 instead of no maximum.
+- [#460](https://github.com/kpdecker/jsdiff/pull/460) Added `oneChangePerToken` option.
+- [#467](https://github.com/kpdecker/jsdiff/pull/467) When passing a `comparator(left, right)` to `diffArrays`, values from the old array will now consistently be passed as the first argument (`left`) and values from the new array as the second argument (`right`). Previously this was almost (but not quite) always the other way round.
+- [#480](https://github.com/kpdecker/jsdiff/pull/480) Passing `maxEditLength` to `createPatch` & `createTwoFilesPatch` now works properly (i.e. returns undefined if the max edit distance is exceeded; previous behavior was to crash with a `TypeError` if the edit distance was exceeded).
+
 ## v5.2.0
 
 [Commits](https://github.com/kpdecker/jsdiff/compare/v5.1.0...master)

src/diff/base.js

@@ -30,7 +30,7 @@ Diff.prototype = {
     let newLen = newString.length, oldLen = oldString.length;
     let editLength = 1;
     let maxEditLength = newLen + oldLen;
-    if(options.maxEditLength) {
+    if(options.maxEditLength != null) {
       maxEditLength = Math.min(maxEditLength, options.maxEditLength);
     }
     const maxExecutionTime = options.timeout ?? Infinity;
@@ -42,7 +42,7 @@ Diff.prototype = {
     let newPos = this.extractCommon(bestPath[0], newString, oldString, 0);
     if (bestPath[0].oldPos + 1 >= oldLen && newPos + 1 >= newLen) {
       // Identity per the equality and tokenizer
-      return done([{value: this.join(newString), count: newString.length}]);
+      return done(buildValues(self, bestPath[0].lastComponent, newString, oldString, self.useLongestToken));
     }
 
     // Once we hit the right edge of the edit graph on some diagonal k, we can
@@ -96,12 +96,10 @@ Diff.prototype = {
         // Select the diagonal that we want to branch from. We select the prior
         // path whose position in the old string is the farthest from the origin
         // and does not pass the bounds of the diff graph
-        // TODO: Remove the `+ 1` here to make behavior match Myers algorithm
-        //       and prefer to order removals before insertions.
-        if (!canRemove || (canAdd && removePath.oldPos + 1 < addPath.oldPos)) {
-          basePath = self.addToPath(addPath, true, undefined, 0);
+        if (!canRemove || (canAdd && removePath.oldPos < addPath.oldPos)) {
+          basePath = self.addToPath(addPath, true, false, 0);
         } else {
-          basePath = self.addToPath(removePath, undefined, true, 1);
+          basePath = self.addToPath(removePath, false, true, 1);
         }
 
         newPos = self.extractCommon(basePath, newString, oldString, diagonalPath);
@@ -151,7 +149,7 @@ Diff.prototype = {
 
   addToPath(path, added, removed, oldPosInc) {
     let last = path.lastComponent;
-    if (last && last.added === added && last.removed === removed) {
+    if (last && !this.options.oneChangePerToken && last.added === added && last.removed === removed) {
       return {
         oldPos: path.oldPos + oldPosInc,
         lastComponent: {count: last.count + 1, added: added, removed: removed, previousComponent: last.previousComponent }
@@ -170,14 +168,17 @@ Diff.prototype = {
         newPos = oldPos - diagonalPath,
 
         commonCount = 0;
-    while (newPos + 1 < newLen && oldPos + 1 < oldLen && this.equals(newString[newPos + 1], oldString[oldPos + 1])) {
+    while (newPos + 1 < newLen && oldPos + 1 < oldLen && this.equals(oldString[oldPos + 1], newString[newPos + 1])) {
       newPos++;
       oldPos++;
       commonCount++;
+      if (this.options.oneChangePerToken) {
+        basePath.lastComponent = {count: 1, previousComponent: basePath.lastComponent, added: false, removed: false};
+      }
     }
 
-    if (commonCount) {
-      basePath.lastComponent = {count: commonCount, previousComponent: basePath.lastComponent};
+    if (commonCount && !this.options.oneChangePerToken) {
+      basePath.lastComponent = {count: commonCount, previousComponent: basePath.lastComponent, added: false, removed: false};
     }
 
     basePath.oldPos = oldPos;
@@ -253,26 +254,22 @@ function buildValues(diff, lastComponent, newString, oldString, useLongestToken)
     } else {
       component.value = diff.join(oldString.slice(oldPos, oldPos + component.count));
       oldPos += component.count;
-
-      // Reverse add and remove so removes are output first to match common convention
-      // The diffing algorithm is tied to add then remove output and this is the simplest
-      // route to get the desired output with minimal overhead.
-      if (componentPos && components[componentPos - 1].added) {
-        let tmp = components[componentPos - 1];
-        components[componentPos - 1] = components[componentPos];
-        components[componentPos] = tmp;
-      }
     }
   }
 
   // Special case handle for when one terminal is ignored (i.e. whitespace).
   // For this case we merge the terminal into the prior string and drop the change.
   // This is only available for string mode.
   let finalComponent = components[componentLen - 1];
-  if (componentLen > 1
-      && typeof finalComponent.value === 'string'
-      && (finalComponent.added || finalComponent.removed)
-      && diff.equals('', finalComponent.value)) {
+  if (
+    componentLen > 1
+    && typeof finalComponent.value === 'string'
+    && (
+      (finalComponent.added && diff.equals('', finalComponent.value))
+      ||
+      (finalComponent.removed && diff.equals(finalComponent.value, ''))
+    )
+  ) {
     components[componentLen - 2].value += finalComponent.value;
     components.pop();
   }

src/patch/apply.js

@@ -15,8 +15,8 @@ export function applyPatch(source, uniDiff, options = {}) {
   }
 
   // Apply the diff to the input
-  let lines = source.split(/\r\n|[\n\v\f\r\x85]/),
-      delimiters = source.match(/\r\n|[\n\v\f\r\x85]/g) || [],
+  let lines = source.split(/\r?\n/),
+      delimiters = source.match(/\r?\n/g) || [],
       hunks = uniDiff.hunks,
 
       compareLine = options.compareLine || ((lineNumber, line, operation, patchContent) => line === patchContent),

src/patch/create.js

@@ -140,7 +140,11 @@ export function formatPatch(diff) {
 }
 
 export function createTwoFilesPatch(oldFileName, newFileName, oldStr, newStr, oldHeader, newHeader, options) {
-  return formatPatch(structuredPatch(oldFileName, newFileName, oldStr, newStr, oldHeader, newHeader, options));
+  const patchObj = structuredPatch(oldFileName, newFileName, oldStr, newStr, oldHeader, newHeader, options);
+  if (!patchObj) {
+    return;
+  }
+  return formatPatch(patchObj);
 }
 
 export function createPatch(fileName, oldStr, newStr, oldHeader, newHeader, options) {

src/patch/parse.js

@@ -1,6 +1,6 @@
 export function parsePatch(uniDiff, options = {}) {
-  let diffstr = uniDiff.split(/\r\n|[\n\v\f\r\x85]/),
-      delimiters = uniDiff.match(/\r\n|[\n\v\f\r\x85]/g) || [],
+  let diffstr = uniDiff.split(/\r?\n/),
+      delimiters = uniDiff.match(/\r?\n/g) || [],
       list = [],
       i = 0;
 

test/diff/array.js

@@ -9,35 +9,35 @@ describe('diff/array', function() {
       const diffResult = diffArrays([a, b, c], [a, c, b]);
       console.log(diffResult);
       expect(diffResult).to.deep.equals([
-          {count: 1, value: [a]},
-          {count: 1, value: [c], removed: undefined, added: true},
-          {count: 1, value: [b]},
-          {count: 1, value: [c], removed: true, added: undefined}
+          {count: 1, value: [a], removed: false, added: false},
+          {count: 1, value: [b], removed: true, added: false},
+          {count: 1, value: [c], removed: false, added: false},
+          {count: 1, value: [b], removed: false, added: true}
       ]);
     });
     it('should diff falsey values', function() {
       const a = false;
       const b = 0;
       const c = '';
       // Example sequences from Myers 1986
-      const arrayA = [c, b, a, b, a, c];
-      const arrayB = [a, b, c, a, b, b, a];
+      const arrayA = [a, b, c, a, b, b, a];
+      const arrayB = [c, b, a, b, a, c];
       const diffResult = diffArrays(arrayA, arrayB);
       expect(diffResult).to.deep.equals([
-        {count: 2, value: [a, b], removed: undefined, added: true},
-        {count: 1, value: [c]},
-        {count: 1, value: [b], removed: true, added: undefined},
-        {count: 2, value: [a, b]},
-        {count: 1, value: [b], removed: undefined, added: true},
-        {count: 1, value: [a]},
-        {count: 1, value: [c], removed: true, added: undefined}
+        {count: 2, value: [a, b], removed: true, added: false},
+        {count: 1, value: [c], removed: false, added: false},
+        {count: 1, value: [b], removed: false, added: true},
+        {count: 2, value: [a, b], removed: false, added: false},
+        {count: 1, value: [b], removed: true, added: false},
+        {count: 1, value: [a], removed: false, added: false},
+        {count: 1, value: [c], removed: false, added: true}
       ]);
     });
     describe('anti-aliasing', function() {
       // Test apparent contract that no chunk value is ever an input argument.
       const value = [0, 1, 2];
       const expected = [
-        {count: value.length, value: value}
+        {count: value.length, value: value, removed: false, added: false}
       ];
 
       const input = value.slice();
@@ -70,11 +70,24 @@ describe('diff/array', function() {
       const diffResult = diffArrays([a, b, c], [a, b, d], { comparator: comparator });
       console.log(diffResult);
       expect(diffResult).to.deep.equals([
-          {count: 2, value: [a, b]},
-          {count: 1, value: [c], removed: true, added: undefined},
-          {count: 1, value: [d], removed: undefined, added: true}
+          {count: 2, value: [a, b], removed: false, added: false},
+          {count: 1, value: [c], removed: true, added: false},
+          {count: 1, value: [d], removed: false, added: true}
       ]);
     });
+    it('Should pass old/new tokens as the left/right comparator args respectively', function() {
+      diffArrays(
+        ['a', 'b', 'c'],
+        ['x', 'y', 'z'],
+        {
+          comparator: function(left, right) {
+            expect(left).to.be.oneOf(['a', 'b', 'c']);
+            expect(right).to.be.oneOf(['x', 'y', 'z']);
+            return left === right;
+          }
+        }
+      );
+    });
     it('Should terminate early if execution time exceeds `timeout` ms', function() {
       // To test this, we also pass a comparator that hot sleeps as a way to
       // artificially slow down execution so we reach the timeout.

test/diff/character.js

@@ -6,8 +6,25 @@ import {expect} from 'chai';
 describe('diff/character', function() {
   describe('#diffChars', function() {
     it('Should diff chars', function() {
-      const diffResult = diffChars('New Value.', 'New ValueMoreData.');
-      expect(convertChangesToXML(diffResult)).to.equal('New Value<ins>MoreData</ins>.');
+      const diffResult = diffChars('Old Value.', 'New ValueMoreData.');
+      expect(convertChangesToXML(diffResult)).to.equal('<del>Old</del><ins>New</ins> Value<ins>MoreData</ins>.');
+    });
+
+    describe('oneChangePerToken option', function() {
+      it('emits one change per character', function() {
+        const diffResult = diffChars('Old Value.', 'New ValueMoreData.', {oneChangePerToken: true});
+        expect(diffResult.length).to.equal(21);
+        expect(convertChangesToXML(diffResult)).to.equal('<del>O</del><del>l</del><del>d</del><ins>N</ins><ins>e</ins><ins>w</ins> Value<ins>M</ins><ins>o</ins><ins>r</ins><ins>e</ins><ins>D</ins><ins>a</ins><ins>t</ins><ins>a</ins>.');
+      });
+
+      it('correctly handles the case where the texts are identical', function() {
+        const diffResult = diffChars('foo bar baz qux', 'foo bar baz qux', {oneChangePerToken: true});
+        expect(diffResult).to.deep.equal(
+          ['f', 'o', 'o', ' ', 'b', 'a', 'r', ' ', 'b', 'a', 'z', ' ', 'q', 'u', 'x'].map(
+            char => ({value: char, count: 1, added: false, removed: false})
+          )
+        );
+      });
     });
 
     describe('case insensitivity', function() {