Prefer to order deletions before insertions when the edit cost is the same, like the Myers diff paper does (#439)

* Add test for Unix diff-style ordering of deletions & insertions

* Tweak base.js to prefer extending paths that have previously done more deletions

* Remove now-redundant code to flip the order of immediately consecutive add and remove operations

* Fix existing tests

* add release notes

Author: ExplodingCabbage

Diff for: release-notes.md

@@ -5,6 +5,7 @@
 [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.
 
 ## Development
 

Diff for: src/diff/base.js

@@ -71,9 +71,7 @@ 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)) {
+        if (!canRemove || (canAdd && removePath.oldPos < addPath.oldPos)) {
           basePath = self.addToPath(addPath, true, undefined, 0);
         } else {
           basePath = self.addToPath(removePath, undefined, true, 1);
@@ -223,15 +221,6 @@ 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;
-      }
     }
   }
 

Diff for: test/diff/array.js

@@ -10,27 +10,27 @@ describe('diff/array', function() {
       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: [b], removed: true, added: undefined},
+          {count: 1, value: [c]},
+          {count: 1, value: [b], removed: undefined, 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: 2, value: [a, b], removed: true, added: undefined},
         {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: 2, value: [a, b]},
+        {count: 1, value: [b], removed: true, added: undefined},
         {count: 1, value: [a]},
-        {count: 1, value: [c], removed: true, added: undefined}
+        {count: 1, value: [c], removed: undefined, added: true}
       ]);
     });
     describe('anti-aliasing', function() {

Diff for: test/diff/line.js

@@ -47,6 +47,21 @@ describe('diff/line', function() {
       expect(convertChangesToXML(diffResult)).to.equal('<del>line\n\nold value \n\nline</del>');
     });
 
+    it('Should prefer to do deletions before insertions, like Unix diff does', function() {
+      const diffResult = diffLines('a\nb\nc\nd\n', 'a\nc\nb\nd\n');
+
+      // There are two possible diffs with equal edit distance here; either we
+      // can delete the "b" and insert it again later, or we can insert a "c"
+      // before the "b" and then delete the original "c" later.
+      // For consistency with the convention of other diff tools, we want to
+      // prefer the diff where we delete and then later insert over the one
+      // where we insert and then later delete.
+      expect(convertChangesToXML(diffResult)).to.equal('a\n<del>b\n</del>c\n<ins>b\n</ins>d\n');
+
+      const diffResult2 = diffLines('a\nc\nb\nd\n', 'a\nb\nc\nd\n');
+      expect(convertChangesToXML(diffResult2)).to.equal('a\n<del>c\n</del>b\n<ins>c\n</ins>d\n');
+    });
+
     describe('given options.maxEditLength', function() {
       it('terminates early', function() {
         const diffResult = diffLines(

Diff for: test/diff/word.js

@@ -181,7 +181,7 @@ describe('WordDiff', function() {
 
     it('should diff multiple whitespace values', function() {
       const diffResult = diffWordsWithSpace('New Value  ', 'New  ValueMoreData ');
-      expect(convertChangesToXML(diffResult)).to.equal('New<ins>  ValueMoreData</ins> <del>Value  </del>');
+      expect(convertChangesToXML(diffResult)).to.equal('New<del> Value</del>  <ins>ValueMoreData </ins>');
     });
 
     it('should inserts values in parenthesis', function() {
@@ -220,7 +220,7 @@ describe('WordDiff', function() {
 
     it('should perform async operations', function(done) {
       diffWordsWithSpace('New Value  ', 'New  ValueMoreData ', function(err, diffResult) {
-        expect(convertChangesToXML(diffResult)).to.equal('New<ins>  ValueMoreData</ins> <del>Value  </del>');
+        expect(convertChangesToXML(diffResult)).to.equal('New<del> Value</del>  <ins>ValueMoreData </ins>');
         done();
       });
     });