Create DiffFilter and change SystemNoteService#change_title to use Gitlab::Diff::InlineDiff

18 files changed, 172 insertions, 38 deletions

diff --git a/CHANGELOG b/CHANGELOG
index 2dd80779541..71964051bb1 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -100,6 +100,10 @@ v 8.7.1
   - Prevent users from deleting Webhooks via API they do not own
   - Fix Error 500 due to stale cache when projects are renamed or transferred
   - Update width of search box to fix Safari bug. !3900 (Jedidiah)
+  - Added inline diff styling for `change_title` system notes. !3576 (Adam Butler)
+  - Added `InlineDiffFilter` to the markdown parser. !3576 (Adam Butler)
+
+v 8.7.1 (unreleased)
   - Use the `can?` helper instead of `current_user.can?`
 
 v 8.7.0
diff --git a/app/assets/stylesheets/framework/files.scss b/app/assets/stylesheets/framework/files.scss
index 61d9954c6c8..8c96c7a9c31 100644
--- a/app/assets/stylesheets/framework/files.scss
+++ b/app/assets/stylesheets/framework/files.scss
@@ -36,22 +36,6 @@
       }
     }
 
-    .filename {
-      &.old {
-        display: inline-block;
-        span.idiff {
-          background-color: #f8cbcb;
-        }
-      }
-
-      &.new {
-        display: inline-block;
-        span.idiff {
-          background-color: #a6f3a6;
-        }
-      }
-    }
-
     a:not(.btn) {
       color: $gl-dark-link-color;
     }
diff --git a/app/assets/stylesheets/framework/typography.scss b/app/assets/stylesheets/framework/typography.scss
index 2779cd56788..487081c3c7c 100644
--- a/app/assets/stylesheets/framework/typography.scss
+++ b/app/assets/stylesheets/framework/typography.scss
@@ -269,3 +269,11 @@ h1, h2, h3, h4 {
     text-align: right;
   }
 }
+
+.idiff.deletion {
+  background: $gl-idiff-deletion;
+}
+
+.idiff.addition {
+  background: $gl-idiff-addition;
+}
diff --git a/app/assets/stylesheets/framework/variables.scss b/app/assets/stylesheets/framework/variables.scss
index 5fa4c266607..b7d8f611621 100644
--- a/app/assets/stylesheets/framework/variables.scss
+++ b/app/assets/stylesheets/framework/variables.scss
@@ -178,6 +178,9 @@ $table-border-gray: #f0f0f0;
 $line-target-blue: #eaf3fc;
 $line-select-yellow: #fcf8e7;
 $line-select-yellow-dark: #f0e2bd;
+$gl-idiff-deletion: #f8cbcb;
+$gl-idiff-addition: #a6f3a6;
+
 /*
  * Fonts
  */
diff --git a/app/helpers/diff_helper.rb b/app/helpers/diff_helper.rb
index 5f311f3780a..ea383f9b0f6 100644
--- a/app/helpers/diff_helper.rb
+++ b/app/helpers/diff_helper.rb
@@ -2,8 +2,8 @@ module DiffHelper
   def mark_inline_diffs(old_line, new_line)
     old_diffs, new_diffs = Gitlab::Diff::InlineDiff.new(old_line, new_line).inline_diffs
 
-    marked_old_line = Gitlab::Diff::InlineDiffMarker.new(old_line).mark(old_diffs)
-    marked_new_line = Gitlab::Diff::InlineDiffMarker.new(new_line).mark(new_diffs)
+    marked_old_line = Gitlab::Diff::InlineDiffMarker.new(old_line).mark(old_diffs, mode: :deletion)
+    marked_new_line = Gitlab::Diff::InlineDiffMarker.new(new_line).mark(new_diffs, mode: :addition)
 
     [marked_old_line, marked_new_line]
   end
diff --git a/app/services/system_note_service.rb b/app/services/system_note_service.rb
index 4bdb1b0c074..758abd7f547 100644
--- a/app/services/system_note_service.rb
+++ b/app/services/system_note_service.rb
@@ -171,7 +171,14 @@ class SystemNoteService
   def self.change_title(noteable, project, author, old_title)
     return unless noteable.respond_to?(:title)
 
-    body = "Title changed from **#{old_title}** to **#{noteable.title}**"
+    new_title = noteable.title.dup
+
+    old_diffs, new_diffs = Gitlab::Diff::InlineDiff.new(old_title, new_title).inline_diffs
+
+    marked_old_title = Gitlab::Diff::InlineDiffMarker.new(old_title).mark(old_diffs, mode: :deletion, markdown: true)
+    marked_new_title = Gitlab::Diff::InlineDiffMarker.new(new_title).mark(new_diffs, mode: :addition, markdown: true)
+
+    body = "Changed title: **#{marked_old_title}** → **#{marked_new_title}**"
     create_note(noteable: noteable, project: project, author: author, note: body)
   end
 
diff --git a/app/views/projects/diffs/_file.html.haml b/app/views/projects/diffs/_file.html.haml
index 0f04fc5d33c..7194d9c3ebb 100644
--- a/app/views/projects/diffs/_file.html.haml
+++ b/app/views/projects/diffs/_file.html.haml
@@ -11,11 +11,9 @@
       = link_to "#diff-#{i}" do
         - if diff_file.renamed_file
           - old_path, new_path = mark_inline_diffs(diff_file.old_path, diff_file.new_path)
-          .filename.old
-            = old_path
+          = old_path
           →
-          .filename.new
-            = new_path
+          = new_path
         - else
           %span
             = diff_file.new_path
diff --git a/lib/banzai/filter/inline_diff_filter.rb b/lib/banzai/filter/inline_diff_filter.rb
new file mode 100644
index 00000000000..9e75edd4d4c
--- /dev/null
+++ b/lib/banzai/filter/inline_diff_filter.rb
@@ -0,0 +1,22 @@
+module Banzai
+  module Filter
+    class InlineDiffFilter < HTML::Pipeline::Filter
+      IGNORED_ANCESTOR_TAGS = %w(pre code tt).to_set
+
+      def call
+        search_text_nodes(doc).each do |node|
+          next if has_ancestor?(node, IGNORED_ANCESTOR_TAGS)
+
+          content = node.to_html
+          content = content.gsub(/(?:\[\-(.*?)\-\]|\{\-(.*?)\-\})/, '<span class="idiff left right deletion">\1\2</span>')
+          content = content.gsub(/(?:\[\+(.*?)\+\]|\{\+(.*?)\+\})/, '<span class="idiff left right addition">\1\2</span>')
+
+          next if html == content
+
+          node.replace(content)
+        end
+        doc
+      end
+    end
+  end
+end
diff --git a/lib/banzai/pipeline/gfm_pipeline.rb b/lib/banzai/pipeline/gfm_pipeline.rb
index ed3cfd6b023..b27ecf3c923 100644
--- a/lib/banzai/pipeline/gfm_pipeline.rb
+++ b/lib/banzai/pipeline/gfm_pipeline.rb
@@ -23,7 +23,8 @@ module Banzai
           Filter::LabelReferenceFilter,
           Filter::MilestoneReferenceFilter,
 
-          Filter::TaskListFilter
+          Filter::TaskListFilter,
+          Filter::InlineDiffFilter
         ]
       end
 
diff --git a/lib/gitlab/diff/inline_diff_marker.rb b/lib/gitlab/diff/inline_diff_marker.rb
index dccb717e95d..c9601d42d50 100644
--- a/lib/gitlab/diff/inline_diff_marker.rb
+++ b/lib/gitlab/diff/inline_diff_marker.rb
@@ -1,6 +1,11 @@
 module Gitlab
   module Diff
     class InlineDiffMarker
+      MARKDOWN_SYMBOLS = {
+        addition: "+",
+        deletion: "-"
+      }
+
       attr_accessor :raw_line, :rich_line
 
       def initialize(raw_line, rich_line = raw_line)
@@ -8,7 +13,7 @@ module Gitlab
         @rich_line = ERB::Util.html_escape(rich_line)
       end
 
-      def mark(line_inline_diffs)
+      def mark(line_inline_diffs, mode: nil, markdown: false)
         return rich_line unless line_inline_diffs
 
         marker_ranges = []
@@ -20,13 +25,12 @@ module Gitlab
         end
 
         offset = 0
-        # Mark each range
-        marker_ranges.each_with_index do |range, i|
-          class_names = ["idiff"]
-          class_names << "left"   if i == 0
-          class_names << "right"  if i == marker_ranges.length - 1
 
-          offset = insert_around_range(rich_line, range, "<span class='#{class_names.join(" ")}'>", "</span>", offset)
+        # Mark each range
+        marker_ranges.each_with_index do |range, index|
+          before_content = markdown ? "{#{MARKDOWN_SYMBOLS[mode]}" : "<span class='#{html_class_names(marker_ranges, mode, index)}'>"
+          after_content = markdown ? "#{MARKDOWN_SYMBOLS[mode]}}" : "</span>"
+          offset = insert_around_range(rich_line, range, before_content, after_content, offset)
         end
 
         rich_line.html_safe
@@ -34,6 +38,14 @@ module Gitlab
 
       private
 
+      def html_class_names(marker_ranges, mode, index)
+        class_names = ["idiff"]
+        class_names << "left"  if index == 0
+        class_names << "right" if index == marker_ranges.length - 1
+        class_names << mode if mode
+        class_names.join(" ")
+      end
+
       # Mapping of character positions in the raw line, to the rich (highlighted) line
       def position_mapping
         @position_mapping ||= begin
diff --git a/spec/features/markdown_spec.rb b/spec/features/markdown_spec.rb
index 0148c87084a..1d892fe1a55 100644
--- a/spec/features/markdown_spec.rb
+++ b/spec/features/markdown_spec.rb
@@ -278,6 +278,10 @@ describe 'GitLab Markdown', feature: true do
     it 'includes GollumTagsFilter' do
       expect(doc).to parse_gollum_tags
     end
+
+    it 'includes InlineDiffFilter' do
+      expect(doc).to parse_inline_diffs
+    end
   end
 
   # Fake a `current_user` helper
diff --git a/spec/fixtures/markdown.md.erb b/spec/fixtures/markdown.md.erb
index 1772cc3f6a4..92c7fdf27bd 100644
--- a/spec/fixtures/markdown.md.erb
+++ b/spec/fixtures/markdown.md.erb
@@ -239,3 +239,16 @@ References should be parseable even inside _<%= merge_request.to_reference %>_ e
 - [[link-text|http://example.com/pdfs/gollum.pdf]]
 - [[images/example.jpg]]
 - [[http://example.com/images/example.jpg]]
+
+### Inline Diffs
+
+With inline diffs tags you can display {+ additions +} or [- deletions -].
+
+The wrapping tags can be either curly braces or square brackets [+ additions +] or {- deletions -}.
+
+However the wrapping tags can not be mixed as such -
+
+- {+ additions +]
+- [+ additions +}
+- {- delletions -]
+- [- delletions -}
diff --git a/spec/helpers/diff_helper_spec.rb b/spec/helpers/diff_helper_spec.rb
index b7810185d16..52764f41e0d 100644
--- a/spec/helpers/diff_helper_spec.rb
+++ b/spec/helpers/diff_helper_spec.rb
@@ -93,9 +93,9 @@ describe DiffHelper do
     it "returns strings with marked inline diffs" do
       marked_old_line, marked_new_line = mark_inline_diffs(old_line, new_line)
 
-      expect(marked_old_line).to eq("abc <span class='idiff left right'>&#39;def&#39;</span>")
+      expect(marked_old_line).to eq("abc <span class='idiff left right deletion'>&#39;def&#39;</span>")
       expect(marked_old_line).to be_html_safe
-      expect(marked_new_line).to eq("abc <span class='idiff left right'>&quot;def&quot;</span>")
+      expect(marked_new_line).to eq("abc <span class='idiff left right addition'>&quot;def&quot;</span>")
       expect(marked_new_line).to be_html_safe
     end
   end
diff --git a/spec/lib/banzai/filter/inline_diff_filter_spec.rb b/spec/lib/banzai/filter/inline_diff_filter_spec.rb
new file mode 100644
index 00000000000..9e526371294
--- /dev/null
+++ b/spec/lib/banzai/filter/inline_diff_filter_spec.rb
@@ -0,0 +1,68 @@
+require 'spec_helper'
+
+describe Banzai::Filter::InlineDiffFilter, lib: true do
+  include FilterSpecHelper
+
+  it 'adds inline diff span tags for deletions when using square brackets' do
+    doc = "START [-something deleted-] END"
+    expect(filter(doc).to_html).to eq('START <span class="idiff left right deletion">something deleted</span> END')
+  end
+
+  it 'adds inline diff span tags for deletions when using curley braces' do
+    doc = "START {-something deleted-} END"
+    expect(filter(doc).to_html).to eq('START <span class="idiff left right deletion">something deleted</span> END')
+  end
+
+  it 'does not add inline diff span tags when a closing tag is not provided' do
+    doc = "START [- END"
+    expect(filter(doc).to_html).to eq(doc)
+  end
+
+  it 'adds inline span tags for additions when using square brackets' do
+    doc = "START [+something added+] END"
+    expect(filter(doc).to_html).to eq('START <span class="idiff left right addition">something added</span> END')
+  end
+
+  it 'adds inline span tags for additions  when using curley braces' do
+    doc = "START {+something added+} END"
+    expect(filter(doc).to_html).to eq('START <span class="idiff left right addition">something added</span> END')
+  end
+
+  it 'does not add inline diff span tags when a closing addition tag is not provided' do
+    doc = "START {+ END"
+    expect(filter(doc).to_html).to eq(doc)
+  end
+
+  it 'does not add inline diff span tags when the tags do not match' do
+    examples = [
+      "{+ additions +]",
+      "[+ additions +}",
+      "{- delletions -]",
+      "[- delletions -}"
+    ]
+
+    examples.each do |doc|
+      expect(filter(doc).to_html).to eq(doc)
+    end
+  end
+
+  it 'prevents user-land html being injected' do
+    doc = "START {+&lt;script&gt;alert('I steal cookies')&lt;/script&gt;+} END"
+    expect(filter(doc).to_html).to eq("START <span class=\"idiff left right addition\">&lt;script&gt;alert('I steal cookies')&lt;/script&gt;</span> END")
+  end
+
+  it 'preserves content inside pre tags' do
+    doc = "<pre>START {+something added+} END</pre>"
+    expect(filter(doc).to_html).to eq(doc)
+  end
+
+  it 'preserves content inside code tags' do
+    doc = "<code>START {+something added+} END</code>"
+    expect(filter(doc).to_html).to eq(doc)
+  end
+
+  it 'preserves content inside tt tags' do
+    doc = "<tt>START {+something added+} END</tt>"
+    expect(filter(doc).to_html).to eq(doc)
+  end
+end
diff --git a/spec/services/issues/update_service_spec.rb b/spec/services/issues/update_service_spec.rb
index 52f69306994..ff63f2ce8de 100644
--- a/spec/services/issues/update_service_spec.rb
+++ b/spec/services/issues/update_service_spec.rb
@@ -79,10 +79,10 @@ describe Issues::UpdateService, services: true do
       end
 
       it 'creates system note about title change' do
-        note = find_note('Title changed')
+        note = find_note('Changed title:')
 
         expect(note).not_to be_nil
-        expect(note.note).to eq 'Title changed from **Old title** to **New title**'
+        expect(note.note).to eq 'Changed title: **{-Old-} title** → **{+New+} title**'
       end
     end
 
diff --git a/spec/services/merge_requests/update_service_spec.rb b/spec/services/merge_requests/update_service_spec.rb
index 213e8c2eb3a..56ee3e43056 100644
--- a/spec/services/merge_requests/update_service_spec.rb
+++ b/spec/services/merge_requests/update_service_spec.rb
@@ -90,10 +90,10 @@ describe MergeRequests::UpdateService, services: true do
       end
 
       it 'creates system note about title change' do
-        note = find_note('Title changed')
+        note = find_note('Changed title:')
 
         expect(note).not_to be_nil
-        expect(note.note).to eq 'Title changed from **Old title** to **New title**'
+        expect(note.note).to eq 'Changed title: **{-Old-} title** → **{+New+} title**'
       end
 
       it 'creates system note about branch change' do
diff --git a/spec/services/system_note_service_spec.rb b/spec/services/system_note_service_spec.rb
index 5fbf2ae5247..3957546b18d 100644
--- a/spec/services/system_note_service_spec.rb
+++ b/spec/services/system_note_service_spec.rb
@@ -241,7 +241,7 @@ describe SystemNoteService, services: true do
 
       it 'sets the note text' do
         expect(subject.note).
-          to eq "Title changed from **Old title** to **#{noteable.title}**"
+          to eq "Changed title: **{-Old title-}** → **{+#{noteable.title}+}**"
       end
     end
 
diff --git a/spec/support/matchers/markdown_matchers.rb b/spec/support/matchers/markdown_matchers.rb
index 43cb6ef43f2..259aa469a18 100644
--- a/spec/support/matchers/markdown_matchers.rb
+++ b/spec/support/matchers/markdown_matchers.rb
@@ -168,6 +168,16 @@ module MarkdownMatchers
       expect(actual).to have_selector('input[checked]', count: 3)
     end
   end
+
+  # InlineDiffFilter
+  matcher :parse_inline_diffs do
+    set_default_markdown_messages
+
+    match do |actual|
+      expect(actual).to have_selector('span.idiff.addition', count: 2)
+      expect(actual).to have_selector('span.idiff.deletion', count: 2)
+    end
+  end
 end
 
 # Monkeypatch the matcher DSL so that we can reduce some noisy duplication for