Merge branch 'fix-emails-on-push-diff-limit' into 'master'

Fix emails on push diff limit

The diff limits were not respected in the emails-on-push since the refactoring in gitlab-org/gitlab-ce!2705. This was one possible cause for customer issue described in gitlab-org/gitlab-ce#15137.

This MR also brings in the changes in !337 since they are required for this to work.


See merge request !338

CHANGELOG-EE

 Please view this file on the master branch, on stable branches it's out of date.
 
 v 8.7.0 (unreleased)
+  - Don't attempt to include too large diffs in e-mail-on-push messages (Stan Hu)
   - Update GitLab Pages to 0.2.1: support user-defined 404 pages
   - Refactor group sync to pull access level logic to its own class. !306
   - [Elastic] Stabilize database indexer if database is inconsistent

app/views/notify/repository_push_email.html.haml

@@ -46,35 +46,36 @@
             = diff.new_path
 
   - unless @message.disable_diffs?
-    - diff_files = safe_diff_files(@message.diffs, @message.diff_refs)
+    - diff_files = @message.diffs
 
-    %h4 Changes:
-    - diff_files.each_with_index do |diff_file, i|
-      %li{id: "diff-#{i}"}
-        %a{href: @message.target_url + "#diff-#{i}"}
-          - if diff_file.deleted_file
-            %strong
-              = diff_file.old_path
-            deleted
-          - elsif diff_file.renamed_file
-            %strong
-              = diff_file.old_path
-            →
-            %strong
-              = diff_file.new_path
+    - if @message.compare_timeout
+      %h5 The diff was not included because it is too large.
+    - else
+      %h4 Changes:
+      - diff_files.each_with_index do |diff_file, i|
+        %li{id: "diff-#{i}"}
+          %a{href: @message.target_url + "#diff-#{i}"}
+            - if diff_file.deleted_file
+              %strong
+                = diff_file.old_path
+              deleted
+            - elsif diff_file.renamed_file
+              %strong
+                = diff_file.old_path
+              →
+              %strong
+                = diff_file.new_path
+            - else
+              %strong
+                = diff_file.new_path
+          %hr
+          - diff_commit = diff_file.deleted_file ? @message.diff_refs.first : @message.diff_refs.last
+          - blob = @message.project.repository.blob_for_diff(diff_commit, diff_file)
+          - if blob && blob.respond_to?(:text?) && blob_text_viewable?(blob)
+            %table.code.white
+              - diff_file.highlighted_diff_lines.each do |line|
+                = render "projects/diffs/line", {line: line, diff_file: diff_file, line_code: nil, plain: true}
           - else
-            %strong
-              = diff_file.new_path
-        %hr
-        - diff_commit = diff_file.deleted_file ? @message.diff_refs.first : @message.diff_refs.last
-        - blob = @message.project.repository.blob_for_diff(diff_commit, diff_file)
-        - if blob && blob.respond_to?(:text?) && blob_text_viewable?(blob)
-          %table.code.white
-            - diff_file.highlighted_diff_lines.each do |line|
-              = render "projects/diffs/line", {line: line, diff_file: diff_file, line_code: nil, plain: true}
-        - else
-          No preview for this file type
-        %br
+            No preview for this file type
+          %br
 
-  - if @message.compare_timeout
-    %h5 Huge diff. To prevent performance issues changes are hidden

app/views/notify/repository_push_email.text.haml

@@ -25,24 +25,26 @@
     - else
       \- #{diff.new_path}
   - unless @message.disable_diffs?
-    \
-    \
-    Changes:
-    - @message.diffs.each do |diff|
+    - if @message.compare_timeout
       \
-      \=====================================
-      - if diff.deleted_file
-        #{diff.old_path} deleted
-      - elsif diff.renamed_file
-        #{diff.old_path} → #{diff.new_path}
-      - else
-        = diff.new_path
-      \=====================================
-      != diff.diff
-  - if @message.compare_timeout
-    \
-    \
-    Huge diff. To prevent performance issues it was hidden
+      \
+      The diff was not included because it is too large.
+    - else
+      \
+      \
+      Changes:
+      - @message.diffs.each do |diff_file|
+        \
+        \=====================================
+        - if diff_file.deleted_file
+          #{diff_file.old_path} deleted
+        - elsif diff_file.renamed_file
+          #{diff_file.old_path} → #{diff_file.new_path}
+        - else
+          = diff_file.new_path
+        \=====================================
+        != diff_file.diff.diff
+
   - if @message.target_url
     \
     \

lib/gitlab/email/message/repository_push.rb

@@ -6,6 +6,7 @@ module Gitlab
         attr_reader :author_id, :ref, :action
 
         include Gitlab::Routing.url_helpers
+        include DiffHelper
 
         delegate :namespace, :name_with_namespace, to: :project, prefix: :project
         delegate :name, to: :author, prefix: :author
@@ -38,7 +39,7 @@ module Gitlab
         end
 
         def diffs
-          @diffs ||= (compare.diffs if compare)
+          @diffs ||= (safe_diff_files(compare.diffs, diff_refs) if compare)
         end
 
         def diffs_count

spec/lib/gitlab/email/message/repository_push_spec.rb

@@ -57,7 +57,7 @@ describe Gitlab::Email::Message::RepositoryPush do
 
     describe '#diffs' do
       subject { message.diffs }
-      it { is_expected.to all(be_an_instance_of Gitlab::Git::Diff) }
+      it { is_expected.to all(be_an_instance_of Gitlab::Diff::File) }
     end
 
     describe '#diffs_count' do