Merge branch 'symlink-comments/process-symlink-files' into 'master'

Mark diff files if they're part of broken symlinks

See merge request gitlab-org/gitlab!37746

app/assets/javascripts/diffs/constants.js

@@ -36,6 +36,9 @@ export const LENGTH_OF_AVATAR_TOOLTIP = 17;
 export const LINES_TO_BE_RENDERED_DIRECTLY = 100;
 export const MAX_LINES_TO_BE_RENDERED = 2000;
 
+export const DIFF_FILE_SYMLINK_MODE = '120000';
+export const DIFF_FILE_DELETED_MODE = '0';
+
 export const MR_TREE_SHOW_KEY = 'mr_tree_show';
 
 export const TREE_TYPE = 'tree';

app/assets/javascripts/diffs/diff_file.js

+import { DIFF_FILE_SYMLINK_MODE, DIFF_FILE_DELETED_MODE } from './constants';
+
+function fileSymlinkInformation(file, fileList) {
+  const duplicates = fileList.filter(iteratedFile => iteratedFile.file_hash === file.file_hash);
+  const includesSymlink = duplicates.some(iteratedFile => {
+    return [iteratedFile.a_mode, iteratedFile.b_mode].includes(DIFF_FILE_SYMLINK_MODE);
+  });
+  const brokenSymlinkScenario = duplicates.length > 1 && includesSymlink;
+
+  return (
+    brokenSymlinkScenario && {
+      replaced: file.b_mode === DIFF_FILE_DELETED_MODE,
+      wasSymbolic: file.a_mode === DIFF_FILE_SYMLINK_MODE,
+      isSymbolic: file.b_mode === DIFF_FILE_SYMLINK_MODE,
+      wasReal: ![DIFF_FILE_SYMLINK_MODE, DIFF_FILE_DELETED_MODE].includes(file.a_mode),
+      isReal: ![DIFF_FILE_SYMLINK_MODE, DIFF_FILE_DELETED_MODE].includes(file.b_mode),
+    }
+  );
+}
+
+/* eslint-disable-next-line import/prefer-default-export */
+export function prepareRawDiffFile({ file, allFiles }) {
+  Object.assign(file, {
+    brokenSymlink: fileSymlinkInformation(file, allFiles),
+  });
+
+  return file;
+}

app/assets/javascripts/diffs/store/utils.js

@@ -18,6 +18,7 @@ import {
   SHOW_WHITESPACE,
   NO_SHOW_WHITESPACE,
 } from '../constants';
+import { prepareRawDiffFile } from '../diff_file';
 
 export function findDiffFile(files, match, matchKey = 'file_hash') {
   return files.find(file => file[matchKey] === match);
@@ -294,9 +295,10 @@ function cleanRichText(text) {
   return text ? text.replace(/^[+ -]/, '') : undefined;
 }
 
-function prepareLine(line) {
+function prepareLine(line, file) {
   if (!line.alreadyPrepared) {
     Object.assign(line, {
+      commentsDisabled: file.brokenSymlink,
       rich_text: cleanRichText(line.rich_text),
       discussionsExpanded: true,
       discussions: [],
@@ -330,7 +332,7 @@ export function prepareLineForRenamedFile({ line, diffViewType, diffFile, index
     old_line: lineNumber,
   };
 
-  prepareLine(cleanLine); // WARNING: In-Place Mutations!
+  prepareLine(cleanLine, diffFile); // WARNING: In-Place Mutations!
 
   if (diffViewType === PARALLEL_DIFF_VIEW_TYPE) {
     return {
@@ -348,19 +350,19 @@ function prepareDiffFileLines(file) {
   const parallelLines = file.parallel_diff_lines;
   let parallelLinesCount = 0;
 
-  inlineLines.forEach(prepareLine);
+  inlineLines.forEach(line => prepareLine(line, file)); // WARNING: In-Place Mutations!
 
   parallelLines.forEach((line, index) => {
     Object.assign(line, { line_code: getLineCode(line, index) });
 
     if (line.left) {
       parallelLinesCount += 1;
-      prepareLine(line.left);
+      prepareLine(line.left, file); // WARNING: In-Place Mutations!
     }
 
     if (line.right) {
       parallelLinesCount += 1;
-      prepareLine(line.right);
+      prepareLine(line.right, file); // WARNING: In-Place Mutations!
     }
   });
 
@@ -407,6 +409,7 @@ function deduplicateFilesList(files) {
 
 export function prepareDiffData(diff, priorFiles = []) {
   const cleanedFiles = (diff.diff_files || [])
+    .map((file, index, allFiles) => prepareRawDiffFile({ file, allFiles }))
     .map(ensureBasicDiffFileLines)
     .map(prepareDiffFileLines)
     .map(finalizeDiffFile);

spec/frontend/diffs/diff_file_spec.js

+import { prepareRawDiffFile } from '~/diffs/diff_file';
+
+const DIFF_FILES = [
+  {
+    file_hash: 'ABC', // This file is just a normal file
+  },
+  {
+    file_hash: 'DEF', // This file replaces a symlink
+    a_mode: '0',
+    b_mode: '0755',
+  },
+  {
+    file_hash: 'DEF', // This symlink is replaced by a file
+    a_mode: '120000',
+    b_mode: '0',
+  },
+  {
+    file_hash: 'GHI', // This symlink replaces a file
+    a_mode: '0',
+    b_mode: '120000',
+  },
+  {
+    file_hash: 'GHI', // This file is replaced by a symlink
+    a_mode: '0755',
+    b_mode: '0',
+  },
+];
+
+function makeBrokenSymlinkObject(replaced, wasSymbolic, isSymbolic, wasReal, isReal) {
+  return {
+    replaced,
+    wasSymbolic,
+    isSymbolic,
+    wasReal,
+    isReal,
+  };
+}
+
+describe('diff_file utilities', () => {
+  describe('prepareRawDiffFile', () => {
+    it.each`
+      fileIndex | description                               | brokenSymlink
+      ${0}      | ${'a file that is not symlink-adjacent'}  | ${false}
+      ${1}      | ${'a file that replaces a symlink'}       | ${makeBrokenSymlinkObject(false, false, false, false, true)}
+      ${2}      | ${'a symlink that is replaced by a file'} | ${makeBrokenSymlinkObject(true, true, false, false, false)}
+      ${3}      | ${'a symlink that replaces a file'}       | ${makeBrokenSymlinkObject(false, false, true, false, false)}
+      ${4}      | ${'a file that is replaced by a symlink'} | ${makeBrokenSymlinkObject(true, false, false, true, false)}
+    `(
+      'properly marks $description with the correct .brokenSymlink value',
+      ({ fileIndex, brokenSymlink }) => {
+        const preppedRaw = prepareRawDiffFile({
+          file: DIFF_FILES[fileIndex],
+          allFiles: DIFF_FILES,
+        });
+
+        expect(preppedRaw.brokenSymlink).toStrictEqual(brokenSymlink);
+      },
+    );
+  });
+});

spec/frontend/diffs/store/utils_spec.js

@@ -20,6 +20,14 @@ import { noteableDataMock } from '../../notes/mock_data';
 const getDiffFileMock = () => JSON.parse(JSON.stringify(diffFileMockData));
 const getDiffMetadataMock = () => JSON.parse(JSON.stringify(diffMetadata));
 
+function extractLinesFromFile(file) {
+  const unpackedParallel = file.parallel_diff_lines
+    .flatMap(({ left, right }) => [left, right])
+    .filter(Boolean);
+
+  return [...file.highlighted_diff_lines, ...unpackedParallel];
+}
+
 describe('DiffsStoreUtils', () => {
   describe('findDiffFile', () => {
     const files = [{ file_hash: 1, name: 'one' }];
@@ -429,6 +437,28 @@ describe('DiffsStoreUtils', () => {
       expect(preppedLine.right).toEqual(correctLine);
       expect(preppedLine.line_code).toEqual(correctLine.line_code);
     });
+
+    it.each`
+      brokenSymlink
+      ${false}
+      ${{}}
+      ${'anything except `false`'}
+    `(
+      "properly assigns each line's `commentsDisabled` as the same value as the parent file's `brokenSymlink` value (`$brokenSymlink`)",
+      ({ brokenSymlink }) => {
+        preppedLine = utils.prepareLineForRenamedFile({
+          diffViewType: INLINE_DIFF_VIEW_TYPE,
+          line: sourceLine,
+          index: lineIndex,
+          diffFile: {
+            ...diffFile,
+            brokenSymlink,
+          },
+        });
+
+        expect(preppedLine.commentsDisabled).toStrictEqual(brokenSymlink);
+      },
+    );
   });
 
   describe('prepareDiffData', () => {
@@ -541,6 +571,25 @@ describe('DiffsStoreUtils', () => {
           }),
         ]);
       });
+
+      it('adds the `.brokenSymlink` property to each diff file', () => {
+        preparedDiff.diff_files.forEach(file => {
+          expect(file).toEqual(expect.objectContaining({ brokenSymlink: false }));
+        });
+      });
+
+      it("copies the diff file's `.brokenSymlink` value to each of that file's child lines", () => {
+        const lines = [
+          ...preparedDiff.diff_files,
+          ...splitInlineDiff.diff_files,
+          ...splitParallelDiff.diff_files,
+          ...completedDiff.diff_files,
+        ].flatMap(file => extractLinesFromFile(file));
+
+        lines.forEach(line => {
+          expect(line.commentsDisabled).toBe(false);
+        });
+      });
     });
 
     describe('for diff metadata', () => {
@@ -603,6 +652,12 @@ describe('DiffsStoreUtils', () => {
           },
         ]);
       });
+
+      it('adds the `.brokenSymlink` property to each meta diff file', () => {
+        preparedDiffFiles.forEach(file => {
+          expect(file).toMatchObject({ brokenSymlink: false });
+        });
+      });
     });
   });