Fixes #44422: Improve diff hunks merging strategy

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

@@ -263,7 +263,7 @@ export class LcsDiff {
 			// We have to clean up the computed diff to be more intuitive
 			// but it turns out this cannot be done correctly until the entire set
 			// of diffs have been computed
-			return this.ShiftChanges(changes);
+			return this.PrettifyChanges(changes);
 		}
 
 		return changes;
@@ -746,45 +746,32 @@ export class LcsDiff {
 	 * @param changes The list of changes to shift
 	 * @returns The shifted changes
 	 */
-	private ShiftChanges(changes: DiffChange[]): DiffChange[] {
-		let mergedDiffs: boolean;
-		do {
-			mergedDiffs = false;
-
-			// Shift all the changes down first
-			for (let i = 0; i < changes.length; i++) {
-				const change = changes[i];
-				const originalStop = (i < changes.length - 1) ? changes[i + 1].originalStart : this.OriginalSequence.getLength();
-				const modifiedStop = (i < changes.length - 1) ? changes[i + 1].modifiedStart : this.ModifiedSequence.getLength();
-				const checkOriginal = change.originalLength > 0;
-				const checkModified = change.modifiedLength > 0;
-
-				while (change.originalStart + change.originalLength < originalStop &&
-					change.modifiedStart + change.modifiedLength < modifiedStop &&
-					(!checkOriginal || this.OriginalElementsAreEqual(change.originalStart, change.originalStart + change.originalLength)) &&
-					(!checkModified || this.ModifiedElementsAreEqual(change.modifiedStart, change.modifiedStart + change.modifiedLength))) {
-					change.originalStart++;
-					change.modifiedStart++;
-				}
+	private PrettifyChanges(changes: DiffChange[]): DiffChange[] {
+
+		// Shift all the changes down first
+		for (let i = 0; i < changes.length; i++) {
+			const change = changes[i];
+			const originalStop = (i < changes.length - 1) ? changes[i + 1].originalStart : this.OriginalSequence.getLength();
+			const modifiedStop = (i < changes.length - 1) ? changes[i + 1].modifiedStart : this.ModifiedSequence.getLength();
+			const checkOriginal = change.originalLength > 0;
+			const checkModified = change.modifiedLength > 0;
+
+			while (change.originalStart + change.originalLength < originalStop &&
+				change.modifiedStart + change.modifiedLength < modifiedStop &&
+				(!checkOriginal || this.OriginalElementsAreEqual(change.originalStart, change.originalStart + change.originalLength)) &&
+				(!checkModified || this.ModifiedElementsAreEqual(change.modifiedStart, change.modifiedStart + change.modifiedLength))) {
+				change.originalStart++;
+				change.modifiedStart++;
 			}
 
-			// Build up the new list (we have to build a new list because we
-			// might have changes we can merge together now)
-			let result = new Array<DiffChange>();
 			let mergedChangeArr: DiffChange[] = [null];
-			for (let i = 0; i < changes.length; i++) {
-				if (i < changes.length - 1 && this.ChangesOverlap(changes[i], changes[i + 1], mergedChangeArr)) {
-					mergedDiffs = true;
-					result.push(mergedChangeArr[0]);
-					i++;
-				}
-				else {
-					result.push(changes[i]);
-				}
+			if (i < changes.length - 1 && this.ChangesOverlap(changes[i], changes[i + 1], mergedChangeArr)) {
+				changes[i] = mergedChangeArr[0];
+				changes.splice(i + 1, 1);
+				i--;
+				continue;
 			}
-
-			changes = result;
-		} while (mergedDiffs);
+		}
 
 		// Shift changes back up until we hit empty or whitespace-only lines
 		for (let i = changes.length - 1; i >= 0; i--) {

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

@@ -729,4 +729,122 @@ suite('Editor Diff - DiffComputer', () => {
 		];
 		assertDiff(original, modified, expected, false, false, false);
 	});
+
+	test('issue #44422: Less than ideal diff results', () => {
+		let original = [
+			'export class C {',
+			'',
+			'	public m1(): void {',
+			'		{',
+			'		//2',
+			'		//3',
+			'		//4',
+			'		//5',
+			'		//6',
+			'		//7',
+			'		//8',
+			'		//9',
+			'		//10',
+			'		//11',
+			'		//12',
+			'		//13',
+			'		//14',
+			'		//15',
+			'		//16',
+			'		//17',
+			'		//18',
+			'		}',
+			'	}',
+			'',
+			'	public m2(): void {',
+			'		if (a) {',
+			'			if (b) {',
+			'				//A1',
+			'				//A2',
+			'				//A3',
+			'				//A4',
+			'				//A5',
+			'				//A6',
+			'				//A7',
+			'				//A8',
+			'			}',
+			'		}',
+			'',
+			'		//A9',
+			'		//A10',
+			'		//A11',
+			'		//A12',
+			'		//A13',
+			'		//A14',
+			'		//A15',
+			'	}',
+			'',
+			'	public m3(): void {',
+			'		if (a) {',
+			'			//B1',
+			'		}',
+			'		//B2',
+			'		//B3',
+			'	}',
+			'',
+			'	public m4(): boolean {',
+			'		//1',
+			'		//2',
+			'		//3',
+			'		//4',
+			'	}',
+			'',
+			'}',
+		];
+		let modified = [
+			'export class C {',
+			'',
+			'	constructor() {',
+			'',
+			'',
+			'',
+			'',
+			'	}',
+			'',
+			'	public m1(): void {',
+			'		{',
+			'		//2',
+			'		//3',
+			'		//4',
+			'		//5',
+			'		//6',
+			'		//7',
+			'		//8',
+			'		//9',
+			'		//10',
+			'		//11',
+			'		//12',
+			'		//13',
+			'		//14',
+			'		//15',
+			'		//16',
+			'		//17',
+			'		//18',
+			'		}',
+			'	}',
+			'',
+			'	public m4(): boolean {',
+			'		//1',
+			'		//2',
+			'		//3',
+			'		//4',
+			'	}',
+			'',
+			'}',
+		];
+		let expected = [
+			createLineChange(
+				2, 0, 3, 9
+			),
+			createLineChange(
+				25, 55, 31, 0
+			)
+		];
+		assertDiff(original, modified, expected, false, false, false);
+	});
 });