Merge branch '342-fix-diff-algorithm' into 'main'

refactor: fix diff algorithm

See merge request gitlab-org/gitlab-vscode-extension!268

src/git/diff_line_count.test.ts

 import { diffFile, mrVersion } from '../test_utils/entities';
-import {
-  getAddedLinesForFile,
-  getNewLineForOldUnchangedLine,
-  getUnchangedLines,
-} from './diff_line_count';
+import { getAddedLinesForFile, getNewLineForOldUnchangedLine } from './diff_line_count';
 
 describe('diff_line_count', () => {
   const sevenNewLinesHunk = [
@@ -104,6 +100,11 @@ describe('diff_line_count', () => {
     ` 18`,
   ].join('\n');
 
+  const testVersion = (diff: string): RestMrVersion => ({
+    ...mrVersion,
+    diffs: [{ ...diffFile, diff }],
+  });
+
   describe('getAddedLinesForFile', () => {
     it.each`
       hunkName                          | hunk                            | newLines
@@ -112,12 +113,7 @@ describe('diff_line_count', () => {
       ${'multiHunk'}                    | ${multiHunk}                    | ${[15, 39, 40, 97, 98]}
       ${'hunkWithHunkHeaders'}          | ${hunkWithHunkHeaders}          | ${[15]}
     `('$hunkName gets correctly parsed', ({ hunk, newLines }) => {
-      const testMrVersion = {
-        ...mrVersion,
-        diffs: [{ ...diffFile, diff: hunk }],
-      };
-
-      const ranges = getAddedLinesForFile(testMrVersion, diffFile.new_path);
+      const ranges = getAddedLinesForFile(testVersion(hunk), diffFile.new_path);
 
       expect(ranges).toEqual(newLines);
     });
@@ -129,43 +125,8 @@ describe('diff_line_count', () => {
     });
   });
 
-  describe('getUnchangedLines', () => {
-    const range = (start: number, end: number) =>
-      [...Array(end - start).keys()].map(n => n + start);
-    const shiftBy = (shift: number, oldLines: number[]) => oldLines.map(x => [x, x + shift]);
-
-    it.each`
-      hunkName                          | hunk                            | unchangedLines
-      ${'sevenNewLinesHunk'}            | ${sevenNewLinesHunk}            | ${[]}
-      ${'hunkWithAddedAndRemovedLines'} | ${hunkWithAddedAndRemovedLines} | ${[...shiftBy(0, [10, 11, 12, 15, 16, 17, 19, 20, 21, 22, 23, 25, 26])]}
-      ${'multiHunk'}                    | ${multiHunk}                    | ${[...shiftBy(0, range(12, 15)), ...shiftBy(-2, range(18, 41)), ...shiftBy(-3, range(44, 78)), ...shiftBy(-4, range(79, 101))]}
-      ${'hunkWithHunkHeaders'}          | ${hunkWithHunkHeaders}          | ${[...shiftBy(0, range(2, 5)), ...shiftBy(-1, range(6, 16)), ...shiftBy(0, range(16, 19))]}
-    `('$hunkName gets correctly parsed', ({ hunk, unchangedLines }) => {
-      const testMrVersion = {
-        ...mrVersion,
-        diffs: [{ ...diffFile, diff: hunk }],
-      };
-
-      const unchangedLineNumbers = getUnchangedLines(testMrVersion, diffFile.old_path).map(ul => [
-        ul.oldLine,
-        ul.newLine,
-      ]);
-
-      expect(unchangedLineNumbers).toEqual(unchangedLines);
-    });
-
-    it('returns empty array if invoked with invalid file name', () => {
-      const ranges = getUnchangedLines(mrVersion, '/invalid/path');
-
-      expect(ranges).toEqual([]);
-    });
-  });
-
   describe('getNewLineForOldUnchangedLine', () => {
-    const testMrVersion = {
-      ...mrVersion,
-      diffs: [{ ...diffFile, diff: multiHunk }],
-    };
+    const testMrVersion = testVersion(multiHunk);
 
     it('returns undefined when diff file is not found', () => {
       expect(getNewLineForOldUnchangedLine(testMrVersion, 'non/existent/path.js', 1)).toBe(
@@ -174,25 +135,63 @@ describe('diff_line_count', () => {
     });
 
     it('returns undefined when the old line has been removed', () => {
-      expect(getNewLineForOldUnchangedLine(testMrVersion, diffFile.old_path, 15)).toBe(undefined);
+      const hunk = [
+        '@@ -6,3 +6,2 @@',
+        ' unchanged line 6',
+        '-removed line 7',
+        ' unchanged line 8',
+      ].join('\n');
+      expect(getNewLineForOldUnchangedLine(testVersion(hunk), diffFile.old_path, 7)).toBe(
+        undefined,
+      );
     });
 
     it('returns new line index that has been parsed from the diff hunks', () => {
-      expect(getNewLineForOldUnchangedLine(testMrVersion, diffFile.old_path, 18)).toBe(16);
+      const hunk = [
+        '@@ -6,2 +6,3 @@',
+        ' unchanged line 6',
+        '+addded line 6.1',
+        ' unchanged line 7 (new index 8)',
+      ].join('\n');
+      expect(getNewLineForOldUnchangedLine(testVersion(hunk), diffFile.old_path, 7)).toBe(8);
     });
 
     it('returns the same line number when the old line precedes all changes (all hunks)', () => {
-      expect(getNewLineForOldUnchangedLine(testMrVersion, diffFile.old_path, 1)).toBe(1);
+      const hunk = ['@@ -6,1 +6,1 @@', ' unchanged line 6'].join('\n');
+      expect(getNewLineForOldUnchangedLine(testVersion(hunk), diffFile.old_path, 1)).toBe(1);
     });
 
     it('extrapolates the last unchanged line indexes', () => {
+      const hunk = [
+        '@@ -6,3 +6,2 @@',
+        ' unchanged line 6',
+        '-removed line 7',
+        ' unchanged line 8',
+      ].join('\n');
       // the only information we get is from the hunks
       // for a file that's 1000 lines long we might have only a diff hunk for a change from the beginning
       // of that file. But that change might have shifted the unchanged line old and new index difference
       // for all lines following that change (e.g. removed line will decrease the new line index by 1 for
       // all following lines)
-      expect(getNewLineForOldUnchangedLine(testMrVersion, diffFile.old_path, 100)).toBe(96); // this is the last unchanged line available from the diff hunk
-      expect(getNewLineForOldUnchangedLine(testMrVersion, diffFile.old_path, 1000)).toBe(996);
+      expect(getNewLineForOldUnchangedLine(testVersion(hunk), diffFile.old_path, 8)).toBe(7); // this is the last unchanged line available from the diff hunk
+      expect(getNewLineForOldUnchangedLine(testVersion(hunk), diffFile.old_path, 1000)).toBe(999);
+    });
+
+    it('returns undefined if the last line in the  diff is not unchanged', () => {
+      expect(getNewLineForOldUnchangedLine(testMrVersion, diffFile.old_path, 101)).toBe(undefined); // this is the last line available from the diff hunk
+    });
+
+    it('returns undefined if the first line in the  diff is not unchanged', () => {
+      const hunk = [
+        '@@ -1,3 +1,2 @@',
+        '-removed line 1',
+        ' unchanged line 2',
+        ' unchanged line 3',
+      ].join('\n');
+
+      expect(getNewLineForOldUnchangedLine(testVersion(hunk), diffFile.old_path, 1)).toBe(
+        undefined,
+      );
     });
   });
 });

src/git/diff_line_count.ts

@@ -36,8 +36,8 @@ const REMOVED = 'REMOVED';
 const ADDED = 'ADDED';
 const UNCHANGED = 'UNCHANGED';
 
-type RemovedLine = { type: typeof REMOVED; oldLine: number };
-type AddedLine = { type: typeof ADDED; newLine: number };
+type RemovedLine = { type: typeof REMOVED; oldLine: number; newLine?: never };
+type AddedLine = { type: typeof ADDED; newLine: number; oldLine?: never };
 export type UnchangedLine = { type: typeof UNCHANGED; oldLine: number; newLine: number };
 type HunkLine = RemovedLine | AddedLine | UnchangedLine;
 
@@ -118,21 +118,28 @@ const connectHunks = (parsedHunks: HunkLine[][]): HunkLine[] =>
         ...hunk,
       ]);
 
-export const getUnchangedLines = (mrVersion: RestMrVersion, oldPath: string): UnchangedLine[] =>
-  connectHunks(getHunksForFile(mrVersion, { oldPath })).filter(
-    (l): l is UnchangedLine => l.type === UNCHANGED,
-  );
+const addUnchangedLinesToBeginning = (lines: HunkLine[]): HunkLine[] => {
+  if (isEmpty(lines) || first(lines)!.oldLine === 1) return lines;
+  return connectHunks([[{ type: UNCHANGED, oldLine: 1, newLine: 1 }], lines]);
+};
+
+const ensureOldLineIsPresent = (lines: HunkLine[], oldLine: number): HunkLine[] => {
+  const lastLine = last(lines);
+  if (!lastLine?.oldLine || lastLine.oldLine >= oldLine) return lines;
+  assert(lastLine.type === UNCHANGED);
+  return connectHunks([
+    lines,
+    [{ type: UNCHANGED, oldLine, newLine: oldLine + newLineOffset(lastLine) }],
+  ]);
+};
 
 export const getNewLineForOldUnchangedLine = (
   mrVersion: RestMrVersion,
   oldPath: string,
   oldLine: number,
 ): number | undefined => {
-  const unchangedLines = getUnchangedLines(mrVersion, oldPath);
-  if (isEmpty(unchangedLines)) return undefined;
-  if (oldLine < first(unchangedLines)!.oldLine)
-    return oldLine + newLineOffset(first(unchangedLines)!);
-  if (oldLine > last(unchangedLines)!.oldLine)
-    return oldLine + newLineOffset(last(unchangedLines)!);
-  return unchangedLines.find(ul => ul.oldLine === oldLine)?.newLine;
+  const connectedHunks = connectHunks(getHunksForFile(mrVersion, { oldPath }));
+  const linesFromBeginning = addUnchangedLinesToBeginning(connectedHunks);
+  const allDiffLines = ensureOldLineIsPresent(linesFromBeginning, oldLine);
+  return allDiffLines.find(l => l.oldLine === oldLine)?.newLine;
 };