Skip to content

Commit d4ddda4

Browse files
alexdimaalexdima
committed
Fixes microsoft#44422: Improve diff hunks merging strategy
1 parent caa5334 commit d4ddda4

2 files changed

Lines changed: 141 additions & 36 deletions

File tree

‎src/vs/base/common/diff/diff.ts‎

Lines changed: 23 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -263,7 +263,7 @@ export class LcsDiff {
263263
// We have to clean up the computed diff to be more intuitive
264264
// but it turns out this cannot be done correctly until the entire set
265265
// of diffs have been computed
266-
return this.ShiftChanges(changes);
266+
return this.PrettifyChanges(changes);
267267
}
268268

269269
return changes;
@@ -746,45 +746,32 @@ export class LcsDiff {
746746
* @param changes The list of changes to shift
747747
* @returns The shifted changes
748748
*/
749-
private ShiftChanges(changes: DiffChange[]): DiffChange[] {
750-
let mergedDiffs: boolean;
751-
do {
752-
mergedDiffs = false;
753-
754-
// Shift all the changes down first
755-
for (let i = 0; i < changes.length; i++) {
756-
const change = changes[i];
757-
const originalStop = (i < changes.length - 1) ? changes[i + 1].originalStart : this.OriginalSequence.getLength();
758-
const modifiedStop = (i < changes.length - 1) ? changes[i + 1].modifiedStart : this.ModifiedSequence.getLength();
759-
const checkOriginal = change.originalLength > 0;
760-
const checkModified = change.modifiedLength > 0;
761-
762-
while (change.originalStart + change.originalLength < originalStop &&
763-
change.modifiedStart + change.modifiedLength < modifiedStop &&
764-
(!checkOriginal || this.OriginalElementsAreEqual(change.originalStart, change.originalStart + change.originalLength)) &&
765-
(!checkModified || this.ModifiedElementsAreEqual(change.modifiedStart, change.modifiedStart + change.modifiedLength))) {
766-
change.originalStart++;
767-
change.modifiedStart++;
768-
}
749+
private PrettifyChanges(changes: DiffChange[]): DiffChange[] {
750+
751+
// Shift all the changes down first
752+
for (let i = 0; i < changes.length; i++) {
753+
const change = changes[i];
754+
const originalStop = (i < changes.length - 1) ? changes[i + 1].originalStart : this.OriginalSequence.getLength();
755+
const modifiedStop = (i < changes.length - 1) ? changes[i + 1].modifiedStart : this.ModifiedSequence.getLength();
756+
const checkOriginal = change.originalLength > 0;
757+
const checkModified = change.modifiedLength > 0;
758+
759+
while (change.originalStart + change.originalLength < originalStop &&
760+
change.modifiedStart + change.modifiedLength < modifiedStop &&
761+
(!checkOriginal || this.OriginalElementsAreEqual(change.originalStart, change.originalStart + change.originalLength)) &&
762+
(!checkModified || this.ModifiedElementsAreEqual(change.modifiedStart, change.modifiedStart + change.modifiedLength))) {
763+
change.originalStart++;
764+
change.modifiedStart++;
769765
}
770766

771-
// Build up the new list (we have to build a new list because we
772-
// might have changes we can merge together now)
773-
let result = new Array<DiffChange>();
774767
let mergedChangeArr: DiffChange[] = [null];
775-
for (let i = 0; i < changes.length; i++) {
776-
if (i < changes.length - 1 && this.ChangesOverlap(changes[i], changes[i + 1], mergedChangeArr)) {
777-
mergedDiffs = true;
778-
result.push(mergedChangeArr[0]);
779-
i++;
780-
}
781-
else {
782-
result.push(changes[i]);
783-
}
768+
if (i < changes.length - 1 && this.ChangesOverlap(changes[i], changes[i + 1], mergedChangeArr)) {
769+
changes[i] = mergedChangeArr[0];
770+
changes.splice(i + 1, 1);
771+
i--;
772+
continue;
784773
}
785-
786-
changes = result;
787-
} while (mergedDiffs);
774+
}
788775

789776
// Shift changes back up until we hit empty or whitespace-only lines
790777
for (let i = changes.length - 1; i >= 0; i--) {

‎src/vs/editor/test/common/diff/diffComputer.test.ts‎

Lines changed: 118 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -729,4 +729,122 @@ suite('Editor Diff - DiffComputer', () => {
729729
];
730730
assertDiff(original, modified, expected, false, false, false);
731731
});
732+
733+
test('issue #44422: Less than ideal diff results', () => {
734+
let original = [
735+
'export class C {',
736+
'',
737+
' public m1(): void {',
738+
' {',
739+
' //2',
740+
' //3',
741+
' //4',
742+
' //5',
743+
' //6',
744+
' //7',
745+
' //8',
746+
' //9',
747+
' //10',
748+
' //11',
749+
' //12',
750+
' //13',
751+
' //14',
752+
' //15',
753+
' //16',
754+
' //17',
755+
' //18',
756+
' }',
757+
' }',
758+
'',
759+
' public m2(): void {',
760+
' if (a) {',
761+
' if (b) {',
762+
' //A1',
763+
' //A2',
764+
' //A3',
765+
' //A4',
766+
' //A5',
767+
' //A6',
768+
' //A7',
769+
' //A8',
770+
' }',
771+
' }',
772+
'',
773+
' //A9',
774+
' //A10',
775+
' //A11',
776+
' //A12',
777+
' //A13',
778+
' //A14',
779+
' //A15',
780+
' }',
781+
'',
782+
' public m3(): void {',
783+
' if (a) {',
784+
' //B1',
785+
' }',
786+
' //B2',
787+
' //B3',
788+
' }',
789+
'',
790+
' public m4(): boolean {',
791+
' //1',
792+
' //2',
793+
' //3',
794+
' //4',
795+
' }',
796+
'',
797+
'}',
798+
];
799+
let modified = [
800+
'export class C {',
801+
'',
802+
' constructor() {',
803+
'',
804+
'',
805+
'',
806+
'',
807+
' }',
808+
'',
809+
' public m1(): void {',
810+
' {',
811+
' //2',
812+
' //3',
813+
' //4',
814+
' //5',
815+
' //6',
816+
' //7',
817+
' //8',
818+
' //9',
819+
' //10',
820+
' //11',
821+
' //12',
822+
' //13',
823+
' //14',
824+
' //15',
825+
' //16',
826+
' //17',
827+
' //18',
828+
' }',
829+
' }',
830+
'',
831+
' public m4(): boolean {',
832+
' //1',
833+
' //2',
834+
' //3',
835+
' //4',
836+
' }',
837+
'',
838+
'}',
839+
];
840+
let expected = [
841+
createLineChange(
842+
2, 0, 3, 9
843+
),
844+
createLineChange(
845+
25, 55, 31, 0
846+
)
847+
];
848+
assertDiff(original, modified, expected, false, false, false);
849+
});
732850
});

0 commit comments

Comments
 (0)