Merge branch 'ce-to-ee-2018-09-19' into 'master'

CE upstream - 2018-09-19 15:21 UTC

Closes gitlab-ce#48873

See merge request gitlab-org/gitlab-ee!7414

app/assets/javascripts/diffs/components/diff_line_note_form.vue

@@ -22,7 +22,7 @@ export default {
       type: Object,
       required: true,
     },
-    position: {
+    linePosition: {
       type: String,
       required: false,
       default: '',
@@ -81,7 +81,7 @@ export default {
         noteTargetLine: this.noteTargetLine,
         diffViewType: this.diffViewType,
         diffFile: selectedDiffFile,
-        linePosition: this.position,
+        linePosition: this.linePosition,
       });
 
       this.saveNote(postData)

app/assets/javascripts/diffs/components/parallel_diff_comment_row.vue

@@ -92,7 +92,7 @@ export default {
         :diff-file-hash="diffFileHash"
         :line="line.left"
         :note-target-line="line.left"
-        position="left"
+        line-position="left"
       />
     </td>
     <td class="notes_line new"></td>
@@ -111,7 +111,7 @@ export default {
         :diff-file-hash="diffFileHash"
         :line="line.right"
         :note-target-line="line.right"
-        position="right"
+        line-position="right"
       />
     </td>
   </tr>

app/assets/javascripts/diffs/constants.js

@@ -7,6 +7,7 @@ export const CONTEXT_LINE_TYPE = 'context';
 export const EMPTY_CELL_TYPE = 'empty-cell';
 export const COMMENT_FORM_TYPE = 'commentForm';
 export const DIFF_NOTE_TYPE = 'DiffNote';
+export const LEGACY_DIFF_NOTE_TYPE = 'LegacyDiffNote';
 export const NOTE_TYPE = 'Note';
 export const NEW_LINE_TYPE = 'new';
 export const OLD_LINE_TYPE = 'old';

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

@@ -90,16 +90,18 @@ export default {
     const firstDiscussion = discussions[0];
     const isDiffDiscussion = firstDiscussion.diff_discussion;
     const hasLineCode = firstDiscussion.line_code;
-    const isResolvable = firstDiscussion.resolvable;
     const diffPosition = diffPositionByLineCode[firstDiscussion.line_code];
 
     if (
       selectedFile &&
       isDiffDiscussion &&
       hasLineCode &&
-      isResolvable &&
       diffPosition &&
-      isDiscussionApplicableToLine(firstDiscussion, diffPosition)
+      isDiscussionApplicableToLine({
+        discussion: firstDiscussion,
+        diffPosition,
+        latestDiff: state.latestDiff,
+      })
     ) {
       const targetLine = selectedFile.parallelDiffLines.find(
         line =>

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

@@ -4,6 +4,7 @@ import {
   LINE_POSITION_LEFT,
   LINE_POSITION_RIGHT,
   TEXT_DIFF_POSITION_TYPE,
+  LEGACY_DIFF_NOTE_TYPE,
   DIFF_NOTE_TYPE,
   NEW_LINE_TYPE,
   OLD_LINE_TYPE,
@@ -60,7 +61,10 @@ export function getNoteFormData(params) {
       noteable_type: noteableType,
       noteable_id: noteableData.id,
       commit_id: '',
-      type: DIFF_NOTE_TYPE,
+      type:
+        diffFile.diffRefs.startSha && diffFile.diffRefs.headSha
+          ? DIFF_NOTE_TYPE
+          : LEGACY_DIFF_NOTE_TYPE,
       line_code: noteTargetLine.lineCode,
     },
   };
@@ -230,7 +234,16 @@ export function getDiffPositionByLineCode(diffFiles) {
         const { lineCode, oldLine, newLine } = line;
 
         if (lineCode) {
-          acc[lineCode] = { baseSha, headSha, startSha, newPath, oldPath, oldLine, newLine };
+          acc[lineCode] = {
+            baseSha,
+            headSha,
+            startSha,
+            newPath,
+            oldPath,
+            oldLine,
+            newLine,
+            lineCode,
+          };
         }
       });
     }
@@ -241,9 +254,15 @@ export function getDiffPositionByLineCode(diffFiles) {
 
 // This method will check whether the discussion is still applicable
 // to the diff line in question regarding different versions of the MR
-export function isDiscussionApplicableToLine(discussion, diffPosition) {
-  const originalRefs = convertObjectPropsToCamelCase(discussion.original_position.formatter);
-  const refs = convertObjectPropsToCamelCase(discussion.position.formatter);
+export function isDiscussionApplicableToLine({ discussion, diffPosition, latestDiff }) {
+  const { lineCode, ...diffPositionCopy } = diffPosition;
 
-  return _.isEqual(refs, diffPosition) || _.isEqual(originalRefs, diffPosition);
+  if (discussion.original_position && discussion.position) {
+    const originalRefs = convertObjectPropsToCamelCase(discussion.original_position.formatter);
+    const refs = convertObjectPropsToCamelCase(discussion.position.formatter);
+
+    return _.isEqual(refs, diffPositionCopy) || _.isEqual(originalRefs, diffPositionCopy);
+  }
+
+  return latestDiff && discussion.active && lineCode === discussion.line_code;
 }

app/assets/javascripts/notes/stores/utils.js

@@ -27,7 +27,7 @@ export const getQuickActionText = note => {
 
 export const reduceDiscussionsToLineCodes = selectedDiscussions =>
   selectedDiscussions.reduce((acc, note) => {
-    if (note.diff_discussion && note.line_code && note.resolvable) {
+    if (note.diff_discussion && note.line_code) {
       // For context about line notes: there might be multiple notes with the same line code
       const items = acc[note.line_code] || [];
       items.push(note);

app/models/diff_note.rb

@@ -131,7 +131,7 @@ class DiffNote < Note
       # As an extra benefit, the returned `diff_file` already
       # has `highlighted_diff_lines` data set from Redis on
       # `Diff::FileCollection::MergeRequestDiff`.
-      noteable.diffs(paths: original_position.paths, expanded: true).diff_files.first
+      noteable.diffs(original_position.diff_options).diff_files.first
     else
       original_position.diff_file(self.project.repository)
     end

app/services/merge_requests/reload_diffs_service.rb

@@ -31,7 +31,7 @@ module MergeRequests
     def clear_cache(new_diff)
       # Executing the iteration we cache highlighted diffs for each diff file of
       # MergeRequestDiff.
-      new_diff.diffs_collection.write_cache
+      cacheable_collection(new_diff).write_cache
 
       # Remove cache for all diffs on this MR. Do not use the association on the
       # model, as that will interfere with other actions happening when
@@ -39,9 +39,15 @@ module MergeRequests
       MergeRequestDiff.where(merge_request: merge_request).each do |merge_request_diff|
         next if merge_request_diff == new_diff
 
-        merge_request_diff.diffs_collection.clear_cache
+        cacheable_collection(merge_request_diff).clear_cache
       end
     end
     # rubocop: enable CodeReuse/ActiveRecord
+
+    def cacheable_collection(diff)
+      # There are scenarios where we don't need to request Diff Stats.
+      # Mainly when clearing / writing diff caches.
+      diff.diffs(include_stats: false)
+    end
   end
 end

app/workers/new_merge_request_worker.rb

@@ -10,7 +10,7 @@ class NewMergeRequestWorker
     EventCreateService.new.open_mr(issuable, user)
     NotificationService.new.new_merge_request(issuable, user)
 
-    issuable.diffs.write_cache
+    issuable.diffs(include_stats: false).write_cache
     issuable.create_cross_references!(user)
   end
 

changelogs/unreleased/force-post-migration-dir-schema-load.yml

 ---
 title: Ensure the schema is loaded with post_migrations included
-merge_request:
+merge_request: 21689
 author:
 type: changed

changelogs/unreleased/mr-legacy-diff-notes.yml

+---
+title: Correctly show legacy diff notes in the merge request changes tab
+merge_request: 21652
+author:
+type: fixed

changelogs/unreleased/osw-use-diff-stats-rpc-on-comparison-views.yml

+---
+title: Use stats RPC when comparing diffs
+merge_request: 21778
+author:
+type: fixed

lib/gitlab/diff/file.rb

@@ -20,8 +20,9 @@ module Gitlab
         DiffViewer::Image
       ].sort_by { |v| v.binary? ? 0 : 1 }.freeze
 
-      def initialize(diff, repository:, diff_refs: nil, fallback_diff_refs: nil)
+      def initialize(diff, repository:, diff_refs: nil, fallback_diff_refs: nil, stats: nil)
         @diff = diff
+        @stats = stats
         @repository = repository
         @diff_refs = diff_refs
         @fallback_diff_refs = fallback_diff_refs
@@ -165,11 +166,11 @@ module Gitlab
       end
 
       def added_lines
-        diff_lines.count(&:added?)
+        @stats&.additions || diff_lines.count(&:added?)
       end
 
       def removed_lines
-        diff_lines.count(&:removed?)
+        @stats&.deletions || diff_lines.count(&:removed?)
       end
 
       def file_identifier

lib/gitlab/diff/file_collection/base.rb

@@ -2,23 +2,27 @@ module Gitlab
   module Diff
     module FileCollection
       class Base
+        include Gitlab::Utils::StrongMemoize
+
         attr_reader :project, :diff_options, :diff_refs, :fallback_diff_refs, :diffable
 
         delegate :count, :size, :real_size, to: :diff_files
 
         def self.default_options
-          ::Commit.max_diff_options.merge(ignore_whitespace_change: false, expanded: false)
+          ::Commit.max_diff_options.merge(ignore_whitespace_change: false, expanded: false, include_stats: true)
         end
 
         def initialize(diffable, project:, diff_options: nil, diff_refs: nil, fallback_diff_refs: nil)
           diff_options = self.class.default_options.merge(diff_options || {})
 
           @diffable = diffable
+          @include_stats = diff_options.delete(:include_stats)
           @diffs = diffable.raw_diffs(diff_options)
           @project = project
           @diff_options = diff_options
           @diff_refs = diff_refs
           @fallback_diff_refs = fallback_diff_refs
+          @repository = project.repository
         end
 
         def diff_files
@@ -43,10 +47,27 @@ module Gitlab
 
         private
 
+        def diff_stats_collection
+          strong_memoize(:diff_stats) do
+            # There are scenarios where we don't need to request Diff Stats,
+            # when caching for instance.
+            next unless @include_stats
+            next unless diff_refs
+
+            @repository.diff_stats(diff_refs.base_sha, diff_refs.head_sha)
+          end
+        end
+
         def decorate_diff!(diff)
           return diff if diff.is_a?(File)
 
-          Gitlab::Diff::File.new(diff, repository: project.repository, diff_refs: diff_refs, fallback_diff_refs: fallback_diff_refs)
+          stats = diff_stats_collection&.find_by_path(diff.new_path)
+
+          Gitlab::Diff::File.new(diff,
+                                 repository: project.repository,
+                                 diff_refs: diff_refs,
+                                 fallback_diff_refs: fallback_diff_refs,
+                                 stats: stats)
         end
       end
     end

lib/gitlab/diff/position.rb

@@ -116,6 +116,10 @@ module Gitlab
         end
       end
 
+      def diff_options
+        { paths: paths, expanded: true, include_stats: false }
+      end
+
       def diff_line(repository)
         @diff_line ||= diff_file(repository)&.line_for_position(self)
       end
@@ -130,7 +134,7 @@ module Gitlab
         return unless diff_refs.complete?
         return unless comparison = diff_refs.compare_in(repository.project)
 
-        comparison.diffs(paths: paths, expanded: true).diff_files.first
+        comparison.diffs(diff_options).diff_files.first
       end
 
       def get_formatter_class(type)

lib/gitlab/git/diff_stats_collection.rb

@@ -3,6 +3,7 @@
 module Gitlab
   module Git
     class DiffStatsCollection
+      include Gitlab::Utils::StrongMemoize
       include Enumerable
 
       def initialize(diff_stats)
@@ -12,6 +13,18 @@ module Gitlab
       def each(&block)
         @collection.each(&block)
       end
+
+      def find_by_path(path)
+        indexed_by_path[path]
+      end
+
+      private
+
+      def indexed_by_path
+        strong_memoize(:indexed_by_path) do
+          index_by { |stats| stats.path }
+        end
+      end
     end
   end
 end

lib/gitlab/git/repository.rb

@@ -444,7 +444,7 @@ module Gitlab
         end
 
         Gitlab::Git::DiffStatsCollection.new(stats)
-      rescue CommandError
+      rescue CommandError, TypeError
         Gitlab::Git::DiffStatsCollection.new([])
       end
 

rubocop/cop/ruby_interpolation_in_translation.rb

@@ -6,7 +6,6 @@ module RuboCop
       MSG = "Don't use ruby interpolation \#{} inside translated strings, instead use \%{}"
 
       TRANSLATION_METHODS = ':_ :s_ :N_ :n_'
-      RUBY_INTERPOLATION_REGEX = /.*\#\{.*\}/
 
       def_node_matcher :translation_method?, <<~PATTERN
         (send nil? {#{TRANSLATION_METHODS}} $dstr ...)

spec/features/merge_request/user_posts_diff_notes_spec.rb

@@ -200,23 +200,20 @@ describe 'Merge request > User posts diff notes', :js do
     end
 
     context 'with a new line' do
-      # TODO: https://gitlab.com/gitlab-org/gitlab-ce/issues/48034
-      xit 'allows commenting' do
-        should_allow_commenting(find('[id="2f6fcd96b88b36ce98c38da085c795a27d92a3dd_10_9"]').find(:xpath, '..'))
+      it 'allows commenting' do
+        should_allow_commenting(find('[id="2f6fcd96b88b36ce98c38da085c795a27d92a3dd_10_9"]'))
       end
     end
 
     context 'with an old line' do
-      # TODO: https://gitlab.com/gitlab-org/gitlab-ce/issues/48034
-      xit 'allows commenting' do
-        should_allow_commenting(find('[id="6eb14e00385d2fb284765eb1cd8d420d33d63fc9_22_22"]').find(:xpath, '..'))
+      it 'allows commenting' do
+        should_allow_commenting(find('[id="6eb14e00385d2fb284765eb1cd8d420d33d63fc9_22_22"]'))
       end
     end
 
     context 'with an unchanged line' do
-      # TODO: https://gitlab.com/gitlab-org/gitlab-ce/issues/48034
-      xit 'allows commenting' do
-        should_allow_commenting(find('[id="2f6fcd96b88b36ce98c38da085c795a27d92a3dd_7_7"]').find(:xpath, '..'))
+      it 'allows commenting' do
+        should_allow_commenting(find('[id="2f6fcd96b88b36ce98c38da085c795a27d92a3dd_7_7"]'))
       end
     end
 

spec/javascripts/diffs/store/actions_spec.js

@@ -157,6 +157,7 @@ describe('DiffsStoreActions', () => {
                   newPath: 'file1',
                   oldLine: 5,
                   oldPath: 'file2',
+                  lineCode: 'ABC_1_1',
                 },
               },
             },

spec/javascripts/diffs/store/mutations_spec.js

@@ -162,6 +162,7 @@ describe('DiffsStoreMutations', () => {
       };
 
       const state = {
+        latestDiff: true,
         diffFiles: [
           {
             fileHash: 'ABC',
@@ -229,6 +230,76 @@ describe('DiffsStoreMutations', () => {
       expect(state.diffFiles[0].highlightedDiffLines[0].discussions.length).toEqual(2);
       expect(state.diffFiles[0].highlightedDiffLines[0].discussions[1].id).toEqual(2);
     });
+
+    it('should add legacy discussions to the given line', () => {
+      const diffPosition = {
+        baseSha: 'ed13df29948c41ba367caa757ab3ec4892509910',
+        headSha: 'b921914f9a834ac47e6fd9420f78db0f83559130',
+        newLine: null,
+        newPath: '500-lines-4.txt',
+        oldLine: 5,
+        oldPath: '500-lines-4.txt',
+        startSha: 'ed13df29948c41ba367caa757ab3ec4892509910',
+        lineCode: 'ABC_1',
+      };
+
+      const state = {
+        latestDiff: true,
+        diffFiles: [
+          {
+            fileHash: 'ABC',
+            parallelDiffLines: [
+              {
+                left: {
+                  lineCode: 'ABC_1',
+                  discussions: [],
+                },
+                right: {
+                  lineCode: 'ABC_1',
+                  discussions: [],
+                },
+              },
+            ],
+            highlightedDiffLines: [
+              {
+                lineCode: 'ABC_1',
+                discussions: [],
+              },
+            ],
+          },
+        ],
+      };
+      const discussions = [
+        {
+          id: 1,
+          line_code: 'ABC_1',
+          diff_discussion: true,
+          active: true,
+        },
+        {
+          id: 2,
+          line_code: 'ABC_1',
+          diff_discussion: true,
+          active: true,
+        },
+      ];
+
+      const diffPositionByLineCode = {
+        ABC_1: diffPosition,
+      };
+
+      mutations[types.SET_LINE_DISCUSSIONS_FOR_FILE](state, {
+        fileHash: 'ABC',
+        discussions,
+        diffPositionByLineCode,
+      });
+
+      expect(state.diffFiles[0].parallelDiffLines[0].left.discussions.length).toEqual(2);
+      expect(state.diffFiles[0].parallelDiffLines[0].left.discussions[1].id).toEqual(2);
+
+      expect(state.diffFiles[0].highlightedDiffLines[0].discussions.length).toEqual(2);
+      expect(state.diffFiles[0].highlightedDiffLines[0].discussions[1].id).toEqual(2);
+    });
   });
 
   describe('REMOVE_LINE_DISCUSSIONS', () => {

spec/javascripts/diffs/store/utils_spec.js

@@ -3,6 +3,7 @@ import {
   LINE_POSITION_LEFT,
   LINE_POSITION_RIGHT,
   TEXT_DIFF_POSITION_TYPE,
+  LEGACY_DIFF_NOTE_TYPE,
   DIFF_NOTE_TYPE,
   NEW_LINE_TYPE,
   OLD_LINE_TYPE,
@@ -151,6 +152,64 @@ describe('DiffsStoreUtils', () => {
         data: postData,
       });
     });
+
+    it('should create legacy note form data', () => {
+      const diffFile = getDiffFileMock();
+      delete diffFile.diffRefs.startSha;
+      delete diffFile.diffRefs.headSha;
+
+      noteableDataMock.targetType = MERGE_REQUEST_NOTEABLE_TYPE;
+
+      const options = {
+        note: 'Hello world!',
+        noteableData: noteableDataMock,
+        noteableType: MERGE_REQUEST_NOTEABLE_TYPE,
+        diffFile,
+        noteTargetLine: {
+          lineCode: '1c497fbb3a46b78edf04cc2a2fa33f67e3ffbe2a_1_3',
+          metaData: null,
+          newLine: 3,
+          oldLine: 1,
+        },
+        diffViewType: PARALLEL_DIFF_VIEW_TYPE,
+        linePosition: LINE_POSITION_LEFT,
+      };
+
+      const position = JSON.stringify({
+        base_sha: diffFile.diffRefs.baseSha,
+        start_sha: undefined,
+        head_sha: undefined,
+        old_path: diffFile.oldPath,
+        new_path: diffFile.newPath,
+        position_type: TEXT_DIFF_POSITION_TYPE,
+        old_line: options.noteTargetLine.oldLine,
+        new_line: options.noteTargetLine.newLine,
+      });
+
+      const postData = {
+        view: options.diffViewType,
+        line_type: options.linePosition === LINE_POSITION_RIGHT ? NEW_LINE_TYPE : OLD_LINE_TYPE,
+        merge_request_diff_head_sha: undefined,
+        in_reply_to_discussion_id: '',
+        note_project_id: '',
+        target_type: options.noteableType,
+        target_id: options.noteableData.id,
+        note: {
+          noteable_type: options.noteableType,
+          noteable_id: options.noteableData.id,
+          commit_id: '',
+          type: LEGACY_DIFF_NOTE_TYPE,
+          line_code: options.noteTargetLine.lineCode,
+          note: options.note,
+          position,
+        },
+      };
+
+      expect(utils.getNoteFormData(options)).toEqual({
+        endpoint: options.noteableData.create_note_path,
+        data: postData,
+      });
+    });
   });
 
   describe('addLineReferences', () => {
@@ -291,13 +350,72 @@ describe('DiffsStoreUtils', () => {
 
     it('returns true when the discussion is up to date', () => {
       expect(
-        utils.isDiscussionApplicableToLine(discussions.upToDateDiscussion1, diffPosition),
+        utils.isDiscussionApplicableToLine({
+          discussion: discussions.upToDateDiscussion1,
+          diffPosition,
+          latestDiff: true,
+        }),
       ).toBe(true);
     });
 
     it('returns false when the discussion is not up to date', () => {
       expect(
-        utils.isDiscussionApplicableToLine(discussions.outDatedDiscussion1, diffPosition),
+        utils.isDiscussionApplicableToLine({
+          discussion: discussions.outDatedDiscussion1,
+          diffPosition,
+          latestDiff: true,
+        }),
+      ).toBe(false);
+    });
+
+    it('returns true when line codes match and discussion does not contain position and is not active', () => {
+      const discussion = { ...discussions.outDatedDiscussion1, line_code: 'ABC_1', active: false };
+      delete discussion.original_position;
+      delete discussion.position;
+
+      expect(
+        utils.isDiscussionApplicableToLine({
+          discussion,
+          diffPosition: {
+            ...diffPosition,
+            lineCode: 'ABC_1',
+          },
+          latestDiff: true,
+        }),
+      ).toBe(false);
+    });
+
+    it('returns true when line codes match and discussion does not contain position and is active', () => {
+      const discussion = { ...discussions.outDatedDiscussion1, line_code: 'ABC_1', active: true };
+      delete discussion.original_position;
+      delete discussion.position;
+
+      expect(
+        utils.isDiscussionApplicableToLine({
+          discussion,
+          diffPosition: {
+            ...diffPosition,
+            lineCode: 'ABC_1',
+          },
+          latestDiff: true,
+        }),
+      ).toBe(true);
+    });
+
+    it('returns false when not latest diff', () => {
+      const discussion = { ...discussions.outDatedDiscussion1, line_code: 'ABC_1', active: true };
+      delete discussion.original_position;
+      delete discussion.position;
+
+      expect(
+        utils.isDiscussionApplicableToLine({
+          discussion,
+          diffPosition: {
+            ...diffPosition,
+            lineCode: 'ABC_1',
+          },
+          latestDiff: false,
+        }),
       ).toBe(false);
     });
   });

spec/lib/gitlab/diff/file_collection/commit_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Gitlab::Diff::FileCollection::Commit do
+  let(:project) { create(:project, :repository) }
+
+  it_behaves_like 'diff statistics' do
+    let(:collection_default_args) do
+      { diff_options: {} }
+    end
+    let(:diffable) { project.commit }
+    let(:stub_path) { 'bar/branch-test.txt' }
+  end
+end

spec/lib/gitlab/diff/file_collection/compare_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Gitlab::Diff::FileCollection::Compare do
+  include RepoHelpers
+
+  let(:project) { create(:project, :repository) }
+  let(:commit)  { project.commit }
+  let(:start_commit) { sample_image_commit }
+  let(:head_commit) { sample_commit }
+  let(:raw_compare) do
+    Gitlab::Git::Compare.new(project.repository.raw_repository,
+                             start_commit.id,
+                             head_commit.id)
+  end
+
+  it_behaves_like 'diff statistics' do
+    let(:collection_default_args) do
+      {
+        project: diffable.project,
+        diff_options: {},
+        diff_refs: diffable.diff_refs
+      }
+    end
+    let(:diffable) { Compare.new(raw_compare, project) }
+    let(:stub_path) { '.gitignore' }
+  end
+end

spec/lib/gitlab/diff/file_collection/merge_request_diff_spec.rb

@@ -29,6 +29,14 @@ describe Gitlab::Diff::FileCollection::MergeRequestDiff do
     expect(mr_diff.cache_key).not_to eq(key)
   end
 
+  it_behaves_like 'diff statistics' do
+    let(:collection_default_args) do
+      { diff_options: {} }
+    end
+    let(:diffable) { merge_request.merge_request_diff }
+    let(:stub_path) { '.gitignore' }
+  end
+
   shared_examples 'initializes a DiffCollection' do
     it 'returns a valid instance of a DiffCollection' do
       expect(diff_files).to be_a(Gitlab::Git::DiffCollection)

spec/lib/gitlab/diff/file_spec.rb

@@ -186,6 +186,70 @@ describe Gitlab::Diff::File do
     end
   end
 
+  context 'diff file stats' do
+    let(:diff_file) do
+      described_class.new(diff,
+                          diff_refs: commit.diff_refs,
+                          repository: project.repository,
+                          stats: stats)
+    end
+
+    let(:raw_diff) do
+      <<~EOS
+        --- a/files/ruby/popen.rb
+        +++ b/files/ruby/popen.rb
+        @@ -6,12 +6,18 @@ module Popen
+
+           def popen(cmd, path=nil)
+             unless cmd.is_a?(Array)
+        -      raise "System commands must be given as an array of strings"
+        +      raise RuntimeError, "System commands must be given as an array of strings"
+        +      # foobar
+             end
+      EOS
+    end
+
+    describe '#added_lines' do
+      context 'when stats argument given' do
+        let(:stats) { double(Gitaly::DiffStats, additions: 10, deletions: 15) }
+
+        it 'returns added lines from stats' do
+          expect(diff_file.added_lines).to eq(stats.additions)
+        end
+      end
+
+      context 'when stats argument not given' do
+        let(:stats) { nil }
+
+        it 'returns added lines by parsing raw diff' do
+          allow(diff_file).to receive(:raw_diff) { raw_diff }
+
+          expect(diff_file.added_lines).to eq(2)
+        end
+      end
+    end
+
+    describe '#removed_lines' do
+      context 'when stats argument given' do
+        let(:stats) { double(Gitaly::DiffStats, additions: 10, deletions: 15) }
+
+        it 'returns removed lines from stats' do
+          expect(diff_file.removed_lines).to eq(stats.deletions)
+        end
+      end
+
+      context 'when stats argument not given' do
+        let(:stats) { nil }
+
+        it 'returns removed lines by parsing raw diff' do
+          allow(diff_file).to receive(:raw_diff) { raw_diff }
+
+          expect(diff_file.removed_lines).to eq(1)
+        end
+      end
+    end
+  end
+
   describe '#simple_viewer' do
     context 'when the file is not diffable' do
       before do

spec/lib/gitlab/git/diff_stats_collection_spec.rb

+# frozen_string_literal: true
+
+require "spec_helper"
+
+describe Gitlab::Git::DiffStatsCollection do
+  let(:stats_a) do
+    double(Gitaly::DiffStats, additions: 10, deletions: 15, path: 'foo')
+  end
+
+  let(:stats_b) do
+    double(Gitaly::DiffStats, additions: 5, deletions: 1, path: 'bar')
+  end
+
+  let(:diff_stats) { [stats_a, stats_b] }
+  let(:collection) { described_class.new(diff_stats) }
+
+  describe '.find_by_path' do
+    it 'returns stats by path when found' do
+      expect(collection.find_by_path('foo')).to eq(stats_a)
+    end
+
+    it 'returns nil when stats is not found by path' do
+      expect(collection.find_by_path('no-file')).to be_nil
+    end
+  end
+end

spec/lib/gitlab/git/repository_spec.rb

@@ -1136,6 +1136,14 @@ describe Gitlab::Git::Repository, :seed_helper do
       expect(collection).to be_a(Enumerable)
       expect(collection.to_a).to be_empty
     end
+
+    it 'returns no Gitaly::DiffStats when there is a nil SHA' do
+      collection = repository.diff_stats(nil, 'master')
+
+      expect(collection).to be_a(Gitlab::Git::DiffStatsCollection)
+      expect(collection).to be_a(Enumerable)
+      expect(collection.to_a).to be_empty
+    end
   end
 
   describe "#ls_files" do

spec/support/shared_examples/diff_file_collections.rb

+# frozen_string_literal: true
+
+shared_examples 'diff statistics' do |test_include_stats_flag: true|
+  def stub_stats_find_by_path(path, stats_mock)
+    expect_next_instance_of(Gitlab::Git::DiffStatsCollection) do |collection|
+      allow(collection).to receive(:find_by_path).and_call_original
+      expect(collection).to receive(:find_by_path).with(path).and_return(stats_mock)
+    end
+  end
+
+  context 'when should request diff stats' do
+    it 'Repository#diff_stats is called' do
+      subject = described_class.new(diffable, collection_default_args)
+
+      expect(diffable.project.repository)
+        .to receive(:diff_stats)
+        .with(diffable.diff_refs.base_sha, diffable.diff_refs.head_sha)
+        .and_call_original
+
+      subject.diff_files
+    end
+
+    it 'Gitlab::Diff::File is initialized with diff stats' do
+      subject = described_class.new(diffable, collection_default_args)
+
+      stats_mock = double(Gitaly::DiffStats, path: '.gitignore', additions: 758, deletions: 120)
+      stub_stats_find_by_path(stub_path, stats_mock)
+
+      diff_file = subject.diff_files.find { |file| file.new_path == stub_path }
+
+      expect(diff_file.added_lines).to eq(stats_mock.additions)
+      expect(diff_file.removed_lines).to eq(stats_mock.deletions)
+    end
+  end
+
+  context 'when should not request diff stats' do
+    it 'Repository#diff_stats is not called' do
+      collection_default_args[:diff_options][:include_stats] = false
+
+      subject = described_class.new(diffable, collection_default_args)
+
+      expect(diffable.project.repository).not_to receive(:diff_stats)
+
+      subject.diff_files
+    end
+  end
+end