Skip to content

Commit de93e91

Browse files
Fix core algorithm implementation being mirrored relative to the Myers algorithm
It makes it harder than it needs to be to reason about the algorithm when everything is flipped relative to Myers' paper. In Myers' paper, we keep track in an array of the index each diagonal has reached in the old string, and positive diagonal numbers represent diagonals where we have done more deletions than insertions. In JsDiff as it exists on master, we keep track in an array of the index each diagonal has reached in the NEW string, and positive diagonal numbers represent diagonals where we have done more insertions than deletions. Everything is mirrored, and this also causes an actual behaviour mismatch between JsDiff and an accurate Myers diff implementation - namely that when we diff e.g. 'abcd' against 'acbd' we output a diff that does an insertion and then later a deletion, rather than a deletion first and an insertion later like it should be. This patch makes the code in base.js be a closer match to what's in Myers's paper, which should make comparing this to other implementations or adding any of the optimizations proposed in Myers's paper easier. For now, this DOESN'T change any behaviour (I've added a hack, noted with a TODO, to ensure this) although it'll now be straightforward to fix the bug mentioned above.
1 parent a2dc5ec commit de93e91

File tree

1 file changed

+28
-21
lines changed

1 file changed

+28
-21
lines changed

src/diff/base.js

Lines changed: 28 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -34,11 +34,11 @@ Diff.prototype = {
3434
maxEditLength = Math.min(maxEditLength, options.maxEditLength);
3535
}
3636

37-
let bestPath = [{ newPos: -1, lastComponent: undefined }];
37+
let bestPath = [{ oldPos: -1, lastComponent: undefined }];
3838

3939
// Seed editLength = 0, i.e. the content starts with the same values
40-
let oldPos = this.extractCommon(bestPath[0], newString, oldString, 0);
41-
if (bestPath[0].newPos + 1 >= newLen && oldPos + 1 >= oldLen) {
40+
let newPos = this.extractCommon(bestPath[0], newString, oldString, 0);
41+
if (bestPath[0].oldPos + 1 >= oldLen && newPos + 1 >= newLen) {
4242
// Identity per the equality and tokenizer
4343
return done([{value: this.join(newString), count: newString.length}]);
4444
}
@@ -47,16 +47,21 @@ Diff.prototype = {
4747
function execEditLength() {
4848
for (let diagonalPath = -1 * editLength; diagonalPath <= editLength; diagonalPath += 2) {
4949
let basePath;
50-
let addPath = bestPath[diagonalPath - 1],
51-
removePath = bestPath[diagonalPath + 1],
52-
oldPos = (removePath ? removePath.newPos : 0) - diagonalPath;
53-
if (addPath) {
50+
let removePath = bestPath[diagonalPath - 1],
51+
addPath = bestPath[diagonalPath + 1];
52+
if (removePath) {
5453
// No one else is going to attempt to use this value, clear it
5554
bestPath[diagonalPath - 1] = undefined;
5655
}
5756

58-
let canAdd = addPath && addPath.newPos + 1 < newLen,
59-
canRemove = removePath && 0 <= oldPos && oldPos < oldLen;
57+
let canAdd = false;
58+
if (addPath) {
59+
// what newPos will be after we do an insertion:
60+
const addPathNewPos = addPath.oldPos - diagonalPath;
61+
canAdd = addPath && 0 <= addPathNewPos && addPathNewPos < newLen;
62+
}
63+
64+
let canRemove = removePath && removePath.oldPos + 1 < oldLen;
6065
if (!canAdd && !canRemove) {
6166
// If this path is a terminal then prune
6267
bestPath[diagonalPath] = undefined;
@@ -66,16 +71,18 @@ Diff.prototype = {
6671
// Select the diagonal that we want to branch from. We select the prior
6772
// path whose position in the new string is the farthest from the origin
6873
// and does not pass the bounds of the diff graph
69-
if (!canAdd || (canRemove && addPath.newPos < removePath.newPos)) {
70-
basePath = self.addToPath(removePath, undefined, true, 0);
74+
// TODO: Remove the `+ 1` here to make behavior match Myers algorithm
75+
// and prefer to order removals before insertions.
76+
if (!canRemove || (canAdd && removePath.oldPos + 1 < addPath.oldPos)) {
77+
basePath = self.addToPath(addPath, true, undefined, 0);
7178
} else {
72-
basePath = self.addToPath(addPath, true, undefined, 1);
79+
basePath = self.addToPath(removePath, undefined, true, 1);
7380
}
7481

75-
oldPos = self.extractCommon(basePath, newString, oldString, diagonalPath);
82+
newPos = self.extractCommon(basePath, newString, oldString, diagonalPath);
7683

7784
// If we have hit the end of both strings, then we are done
78-
if (basePath.newPos + 1 >= newLen && oldPos + 1 >= oldLen) {
85+
if (basePath.oldPos + 1 >= oldLen && newPos + 1 >= newLen) {
7986
return done(buildValues(self, basePath.lastComponent, newString, oldString, self.useLongestToken));
8087
} else {
8188
// Otherwise track this path as a potential candidate and continue.
@@ -112,25 +119,25 @@ Diff.prototype = {
112119
}
113120
},
114121

115-
addToPath(path, added, removed, newPosInc) {
122+
addToPath(path, added, removed, oldPosInc) {
116123
let last = path.lastComponent;
117124
if (last && last.added === added && last.removed === removed) {
118125
return {
119-
newPos: path.newPos + newPosInc,
126+
oldPos: path.oldPos + oldPosInc,
120127
lastComponent: {count: last.count + 1, added: added, removed: removed, previousComponent: last.previousComponent }
121128
};
122129
} else {
123130
return {
124-
newPos: path.newPos + newPosInc,
131+
oldPos: path.oldPos + oldPosInc,
125132
lastComponent: {count: 1, added: added, removed: removed, previousComponent: last }
126133
};
127134
}
128135
},
129136
extractCommon(basePath, newString, oldString, diagonalPath) {
130137
let newLen = newString.length,
131138
oldLen = oldString.length,
132-
newPos = basePath.newPos,
133-
oldPos = newPos - diagonalPath,
139+
oldPos = basePath.oldPos,
140+
newPos = oldPos - diagonalPath,
134141

135142
commonCount = 0;
136143
while (newPos + 1 < newLen && oldPos + 1 < oldLen && this.equals(newString[newPos + 1], oldString[oldPos + 1])) {
@@ -143,8 +150,8 @@ Diff.prototype = {
143150
basePath.lastComponent = {count: commonCount, previousComponent: basePath.lastComponent};
144151
}
145152

146-
basePath.newPos = newPos;
147-
return oldPos;
153+
basePath.oldPos = oldPos;
154+
return newPos;
148155
},
149156

150157
equals(left, right) {

0 commit comments

Comments
 (0)