From ea86163b9b32bc7506463766862715ad2b5932d1 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Fri, 24 Jul 2020 18:59:39 -0600 Subject: [PATCH 1/2] Mark diff files if they're part of broken symlinks - Also begins moving Diff File-specific things to file just for Diff Files instead of dumping everything into the `utils.js` junk drawer. --- app/assets/javascripts/diffs/constants.js | 3 +++ app/assets/javascripts/diffs/diff_file.js | 28 +++++++++++++++++++++ app/assets/javascripts/diffs/store/utils.js | 13 ++++++---- 3 files changed, 39 insertions(+), 5 deletions(-) create mode 100644 app/assets/javascripts/diffs/diff_file.js diff --git a/app/assets/javascripts/diffs/constants.js b/app/assets/javascripts/diffs/constants.js index e3dd882f3dc480..447136036eeafb 100644 --- a/app/assets/javascripts/diffs/constants.js +++ b/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'; diff --git a/app/assets/javascripts/diffs/diff_file.js b/app/assets/javascripts/diffs/diff_file.js new file mode 100644 index 00000000000000..717c4a79ef9126 --- /dev/null +++ b/app/assets/javascripts/diffs/diff_file.js @@ -0,0 +1,28 @@ +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; +} diff --git a/app/assets/javascripts/diffs/store/utils.js b/app/assets/javascripts/diffs/store/utils.js index bc85dd0a1d4600..2d87008d3b1a1e 100644 --- a/app/assets/javascripts/diffs/store/utils.js +++ b/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); -- GitLab From 4ef6ed6ed2d64d1149b40c1a6f8b1baa2cbd1843 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Fri, 24 Jul 2020 20:29:23 -0600 Subject: [PATCH 2/2] Add and update tests for marking files as broken symlinks --- spec/frontend/diffs/diff_file_spec.js | 60 +++++++++++++++++++++++++ spec/frontend/diffs/store/utils_spec.js | 55 +++++++++++++++++++++++ 2 files changed, 115 insertions(+) create mode 100644 spec/frontend/diffs/diff_file_spec.js diff --git a/spec/frontend/diffs/diff_file_spec.js b/spec/frontend/diffs/diff_file_spec.js new file mode 100644 index 00000000000000..5d74760ef6615a --- /dev/null +++ b/spec/frontend/diffs/diff_file_spec.js @@ -0,0 +1,60 @@ +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); + }, + ); + }); +}); diff --git a/spec/frontend/diffs/store/utils_spec.js b/spec/frontend/diffs/store/utils_spec.js index d87619e1e3cc06..62c82468ea0e89 100644 --- a/spec/frontend/diffs/store/utils_spec.js +++ b/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 }); + }); + }); }); }); -- GitLab