Syntax-highlight diffs in push emails

Based on:
https://gitlab.com/gitlab-org/gitlab-ee/merge_requests/151

app/assets/stylesheets/mailers/repository_push_email.scss

+@import "framework/variables";
+
+table.code {
+  width: 100%;
+  font-family: monospace;
+  border: none;
+  border-collapse: separate;
+  margin: 0;
+  padding: 0;
+  -premailer-cellpadding: 0;
+  -premailer-cellspacing: 0;
+  -premailer-width: 100%;
+
+  td {
+    line-height: $code_line_height;
+    font-family: monospace;
+    font-size: $code_font_size;
+  }
+
+  td.diff-line-num {
+    margin: 0;
+    padding: 0;
+    border: none;
+    background: $background-color;
+    color: rgba(0, 0, 0, 0.3);
+    padding: 0 5px;
+    border-right: 1px solid $border-color;
+    text-align: right;
+    min-width: 35px;
+    max-width: 50px;
+    width: 35px;
+  }
+
+  td.line_content {
+    display: block;
+    margin: 0;
+    padding: 0 0.5em;
+    border: none;
+    white-space: pre;
+  }
+}
+
+@import "highlight/white";

app/helpers/emails_helper.rb

@@ -32,12 +32,6 @@ module EmailsHelper
     nil
   end
 
-  def color_email_diff(diffcontent)
-    formatter = Rouge::Formatters::HTML.new(css_class: 'highlight', inline_theme: 'github')
-    lexer = Rouge::Lexers::Diff
-    raw formatter.format(lexer.lex(diffcontent))
-  end
-
   def password_reset_token_valid_time
     valid_hours = Devise.reset_password_within / 60 / 60
     if valid_hours >= 24

app/mailers/emails/projects.rb

@@ -65,7 +65,8 @@ module Emails
 
       # used in notify layout
       @target_url = @message.target_url
-      @project = Project.find project_id
+      @project = Project.find(project_id)
+      @diff_notes_disabled = true
 
       add_project_headers
       headers['X-GitLab-Author'] = @message.author_username

app/mailers/notify.rb

@@ -10,6 +10,8 @@ class Notify < BaseMailer
   include Emails::Builds
 
   add_template_helper MergeRequestsHelper
+  add_template_helper DiffHelper
+  add_template_helper BlobHelper
   add_template_helper EmailsHelper
 
   def test_email(recipient_email, subject, body)

app/views/layouts/notify.html.haml

@@ -4,6 +4,7 @@
     %title
       GitLab
     = stylesheet_link_tag 'notify'
+    = yield :head
   %body
     %div.content
       = yield

app/views/notify/repository_push_email.html.haml

+= content_for :head do
+  = stylesheet_link_tag 'mailers/repository_push_email'
+
 %h3
   #{@message.author_name} #{@message.action_name} #{@message.ref_type} #{@message.ref_name}
   at #{link_to(@message.project_name_with_namespace, namespace_project_url(@message.project_namespace, @message.project))}
@@ -43,26 +46,38 @@
             = diff.new_path
 
   - unless @message.disable_diffs?
-    %h4 Changes:
-    - @message.diffs.each_with_index do |diff, i|
-      %li{id: "diff-#{i}"}
-        %a{href: @message.target_url + "#diff-#{i}"}
-          - if diff.deleted_file
-            %strong
-              = diff.old_path
-            deleted
-          - elsif diff.renamed_file
-            %strong
-              = diff.old_path
-            →
-            %strong
-              = diff.new_path
-          - else
-            %strong
-              = diff.new_path
-        %hr
-        = color_email_diff(diff.diff)
-        %br
+    - diff_files = @message.diffs
 
-  - if @message.compare_timeout
-    %h5 Huge diff. To prevent performance issues changes are hidden
+    - 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
+              &rarr;
+              %strong<
+                = diff_file.new_path
+            - else
+              %strong<
+                = diff_file.new_path
+          - if diff_file.too_large?
+            The diff for this file was not included because it is too large.
+          - else
+            %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

app/views/notify/repository_push_email.text.haml

@@ -25,24 +25,28 @@
     - 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
+        \=====================================
+        - if diff_file.too_large?
+          The diff for this file was not included because it is too large.
+        - else
+          != diff_file.diff.diff
   - if @message.target_url
     \
     \

app/views/projects/diffs/_file.html.haml

@@ -41,7 +41,7 @@
 
   .diff-content.diff-wrap-lines
     - # Skip all non non-supported blobs
-    - return unless blob.respond_to?('text?')
+    - return unless blob.respond_to?(:text?)
     - if diff_file.too_large?
       .nothing-here-block This diff could not be displayed because it is too large.
     - elsif blob_text_viewable?(blob) && !project.repository.diffable?(blob)

app/workers/emails_on_push_worker.rb

@@ -27,15 +27,18 @@ class EmailsOnPushWorker
         :push
       end
 
+    diff_refs = nil
     compare = nil
     reverse_compare = false
     if action == :push
       compare = Gitlab::Git::Compare.new(project.repository.raw_repository, before_sha, after_sha)
+      diff_refs = [project.merge_base_commit(before_sha, after_sha), project.commit(after_sha)]
 
       return false if compare.same
 
       if compare.commits.empty?
         compare = Gitlab::Git::Compare.new(project.repository.raw_repository, after_sha, before_sha)
+        diff_refs = [project.merge_base_commit(after_sha, before_sha), project.commit(before_sha)]
 
         reverse_compare = true
 
@@ -48,13 +51,14 @@ class EmailsOnPushWorker
         send_email(
           recipient,
           project_id,
-          author_id:                  author_id,
-          ref:                        ref,
-          action:                     action,
-          compare:                    compare,
-          reverse_compare:            reverse_compare,
-          send_from_committer_email:  send_from_committer_email,
-          disable_diffs:              disable_diffs
+          author_id:                 author_id,
+          ref:                       ref,
+          action:                    action,
+          compare:                   compare,
+          reverse_compare:           reverse_compare,
+          diff_refs:                 diff_refs,
+          send_from_committer_email: send_from_committer_email,
+          disable_diffs:             disable_diffs
         )
 
       # These are input errors and won't be corrected even if Sidekiq retries

config/application.rb

@@ -78,6 +78,7 @@ module Gitlab
     config.assets.precompile << "*.png"
     config.assets.precompile << "print.css"
     config.assets.precompile << "notify.css"
+    config.assets.precompile << "mailers/repository_push_email.css"
 
     # Version of your assets, change this if you want to expire all your assets
     config.assets.version = '1.0'

lib/gitlab/email/message/repository_push.rb

@@ -5,6 +5,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
@@ -36,7 +37,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
@@ -47,6 +48,10 @@ module Gitlab
           @opts[:compare]
         end
 
+        def diff_refs
+          @opts[:diff_refs]
+        end
+
         def compare_timeout
           diffs.overflow? if diffs
         end

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

spec/mailers/notify_spec.rb

@@ -693,8 +693,9 @@ describe Notify do
     let(:commits) { Commit.decorate(compare.commits, nil) }
     let(:diff_path) { namespace_project_compare_path(project.namespace, project, from: Commit.new(compare.base, project), to: Commit.new(compare.head, project)) }
     let(:send_from_committer_email) { false }
+    let(:diff_refs) { [project.merge_base_commit(sample_image_commit.id, sample_commit.id), project.commit(sample_commit.id)] }
 
-    subject { Notify.repository_push_email(project.id, author_id: user.id, ref: 'refs/heads/master', action: :push, compare: compare, reverse_compare: false, send_from_committer_email: send_from_committer_email) }
+    subject { Notify.repository_push_email(project.id, author_id: user.id, ref: 'refs/heads/master', action: :push, compare: compare, reverse_compare: false, diff_refs: diff_refs, send_from_committer_email: send_from_committer_email) }
 
     it_behaves_like 'it should not have Gmail Actions links'
     it_behaves_like "a user cannot unsubscribe through footer link"
@@ -715,15 +716,15 @@ describe Notify do
       is_expected.to have_body_text /Change some files/
     end
 
-    it 'includes diffs' do
-      is_expected.to have_body_text /def archive_formats_regex/
+    it 'includes diffs with character-level highlighting' do
+      is_expected.to have_body_text /def<\/span> <span class=\"nf\">archive_formats_regex/
     end
 
     it 'contains a link to the diff' do
       is_expected.to have_body_text /#{diff_path}/
     end
 
-    it 'doesn not contain the misleading footer' do
+    it 'does not contain the misleading footer' do
       is_expected.not_to have_body_text /you are a member of/
     end
 
@@ -797,8 +798,9 @@ describe Notify do
     let(:compare) { Gitlab::Git::Compare.new(project.repository.raw_repository, sample_commit.parent_id, sample_commit.id) }
     let(:commits) { Commit.decorate(compare.commits, nil) }
     let(:diff_path) { namespace_project_commit_path(project.namespace, project, commits.first) }
+    let(:diff_refs) { [project.merge_base_commit(sample_commit.parent_id, sample_commit.id), project.commit(sample_commit.id)] }
 
-    subject { Notify.repository_push_email(project.id, author_id: user.id, ref: 'refs/heads/master', action: :push, compare: compare) }
+    subject { Notify.repository_push_email(project.id, author_id: user.id, ref: 'refs/heads/master', action: :push, compare: compare, diff_refs: diff_refs) }
 
     it_behaves_like 'it should show Gmail Actions View Commit link'
     it_behaves_like "a user cannot unsubscribe through footer link"
@@ -819,8 +821,8 @@ describe Notify do
       is_expected.to have_body_text /Change some files/
     end
 
-    it 'includes diffs' do
-      is_expected.to have_body_text /def archive_formats_regex/
+    it 'includes diffs with character-level highlighting' do
+      is_expected.to have_body_text /def<\/span> <span class=\"nf\">archive_formats_regex/
     end
 
     it 'contains a link to the diff' do