Merge branch 'rename-inline-diff' into 'master'

Mark inline difference between old and new paths when a file is renamed

See merge request !2652

CHANGELOG

@@ -18,6 +18,7 @@ v 8.5.0 (unreleased)
   - Revert "Add IP check against DNSBLs at account sign-up"
   - Deprecate API "merge_request/:merge_request_id/comments". Use "merge_requests/:merge_request_id/notes" instead
   - Deprecate API "merge_request/:merge_request_id/...". Use "merge_requests/:merge_request_id/..." instead
+  - Mark inline difference between old and new paths when a file is renamed
 
 v 8.4.3
   - Increase lfs_objects size column to 8-byte integer to allow files larger than 2.1GB

app/assets/stylesheets/framework/files.scss

@@ -36,6 +36,20 @@
       }
     }
 
+    .filename {
+      &.old {
+        span.idiff {
+          background-color: #f8cbcb;
+        }
+      }
+
+      &.new {
+        span.idiff {
+          background-color: #a6f3a6;
+        }
+      }
+    }
+
     .left-options {
       margin-top: -3px;
     }
@@ -158,3 +172,15 @@
     }
   }
 }
+
+span.idiff {
+  &.left {
+    border-top-left-radius: 2px;
+    border-bottom-left-radius: 2px;
+  }
+
+  &.right {
+    border-top-right-radius: 2px;
+    border-bottom-right-radius: 2px;
+  }
+}

app/helpers/diff_helper.rb

 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, marked_new_line]
+  end
+
   def diff_view
     params[:view] == 'parallel' ? 'parallel' : 'inline'
   end
@@ -55,7 +64,7 @@ module DiffHelper
     if line.blank?
       "  ".html_safe
     else
-      line.html_safe
+      line
     end
   end
 

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

@@ -7,16 +7,20 @@
           = submodule_link(blob, @commit.id, project.repository)
     - else
       = blob_icon blob.mode, blob.name
+
       = 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)
+          %strong.filename.old
+            = old_path
+          →
+          %strong.filename.new
+            = new_path
+        - else
           %strong
             = diff_file.new_path
-
           - if diff_file.deleted_file
             deleted
-      - elsif diff_file.renamed_file
-        renamed from
-        %strong
-          = diff_file.old_path
 
       - if diff_file.mode_changed?
         %small

lib/gitlab/diff/highlight.rb

@@ -21,13 +21,13 @@ module Gitlab
           # ignore highlighting for "match" lines
           next diff_line if diff_line.type == 'match' || diff_line.type == 'nonewline'
 
-          rich_line = highlight_line(diff_line, i)
+          rich_line = highlight_line(diff_line) || diff_line.text
 
           if line_inline_diffs = inline_diffs[i]
             rich_line = InlineDiffMarker.new(diff_line.text, rich_line).mark(line_inline_diffs)
           end
 
-          diff_line.text = rich_line.html_safe
+          diff_line.text = rich_line
 
           diff_line
         end
@@ -35,8 +35,8 @@ module Gitlab
 
       private
 
-      def highlight_line(diff_line, index)
-        return html_escape(diff_line.text) unless diff_file && diff_file.diff_refs
+      def highlight_line(diff_line)
+        return unless diff_file && diff_file.diff_refs
 
         line_prefix = diff_line.text.match(/\A(.)/) ? $1 : ' '
 
@@ -49,11 +49,11 @@ module Gitlab
 
         # Only update text if line is found. This will prevent
         # issues with submodules given the line only exists in diff content.
-        rich_line ? line_prefix + rich_line : html_escape(diff_line.text)
+        "#{line_prefix}#{rich_line}".html_safe if rich_line
       end
 
       def inline_diffs
-        @inline_diffs ||= InlineDiff.new(@raw_lines).inline_diffs
+        @inline_diffs ||= InlineDiff.for_lines(@raw_lines)
       end
 
       def old_lines
@@ -72,11 +72,6 @@ module Gitlab
 
         [ref.project.repository, ref.id, path]
       end
-
-      def html_escape(str)
-        replacements = { '&' => '&amp;', '>' => '&gt;', '<' => '&lt;', '"' => '&quot;', "'" => '&#39;' }
-        str.gsub(/[&"'><]/, replacements)
-      end
     end
   end
 end

lib/gitlab/diff/inline_diff.rb

 module Gitlab
   module Diff
     class InlineDiff
-      attr_accessor :lines
+      attr_accessor :old_line, :new_line, :offset
 
-      def initialize(lines)
-        @lines = lines
-      end
+      def self.for_lines(lines)
+        local_edit_indexes = self.find_local_edits(lines)
 
-      def inline_diffs
         inline_diffs = []
 
         local_edit_indexes.each do |index|
           old_index = index
           new_index = index + 1
-          old_line = @lines[old_index]
-          new_line = @lines[new_index]
+          old_line = lines[old_index]
+          new_line = lines[new_index]
+
+          old_diffs, new_diffs = new(old_line, new_line, offset: 1).inline_diffs
+
+          inline_diffs[old_index] = old_diffs
+          inline_diffs[new_index] = new_diffs
+        end
+
+        inline_diffs
+      end
+
+      def initialize(old_line, new_line, offset: 0)
+        @old_line = old_line[offset..-1]
+        @new_line = new_line[offset..-1]
+        @offset = offset
+      end
 
+      def inline_diffs
         # Skip inline diff if empty line was replaced with content
-          next if old_line[1..-1] == ""
+        return if old_line == ""
 
-          # Add one, because this is based on the prefixless version
-          lcp = longest_common_prefix(old_line[1..-1], new_line[1..-1]) + 1
+        lcp = longest_common_prefix(old_line, new_line)
         lcs = longest_common_suffix(old_line[lcp..-1], new_line[lcp..-1])
 
-          old_diff_range = lcp..(old_line.length - lcs - 1)
-          new_diff_range = lcp..(new_line.length - lcs - 1)
+        lcp += offset
+        old_length = old_line.length + offset
+        new_length = new_line.length + offset
 
-          inline_diffs[old_index] = [old_diff_range] if old_diff_range.begin <= old_diff_range.end
-          inline_diffs[new_index] = [new_diff_range] if new_diff_range.begin <= new_diff_range.end
-        end
+        old_diff_range = lcp..(old_length - lcs - 1)
+        new_diff_range = lcp..(new_length - lcs - 1)
 
-        inline_diffs
+        old_diffs = [old_diff_range] if old_diff_range.begin <= old_diff_range.end
+        new_diffs = [new_diff_range] if new_diff_range.begin <= new_diff_range.end
+
+        [old_diffs, new_diffs]
       end
 
       private
 
-      # Find runs of single line edits
-      def local_edit_indexes
-        line_prefixes = @lines.map { |line| line.match(/\A([+-])/) ? $1 : ' ' }
+      def self.find_local_edits(lines)
+        line_prefixes = lines.map { |line| line.match(/\A([+-])/) ? $1 : ' ' }
         joined_line_prefixes = " #{line_prefixes.join} "
 
         offset = 0

lib/gitlab/diff/inline_diff_marker.rb

@@ -5,10 +5,12 @@ module Gitlab
 
       def initialize(raw_line, rich_line = raw_line)
         @raw_line = raw_line
-        @rich_line = rich_line
+        @rich_line = ERB::Util.html_escape(rich_line)
       end
 
       def mark(line_inline_diffs)
+        return rich_line unless line_inline_diffs
+
         marker_ranges = []
         line_inline_diffs.each do |inline_diff_range|
           # Map the inline-diff range based on the raw line to character positions in the rich line
@@ -19,11 +21,15 @@ module Gitlab
 
         offset = 0
         # Mark each range
-        marker_ranges.each do |range|
-          offset = insert_around_range(rich_line, range, "<span class='idiff'>", "</span>", offset)
+        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)
         end
 
-        rich_line
+        rich_line.html_safe
       end
 
       private

spec/fixtures/parallel_diff_result.yml

@@ -55,7 +55,7 @@
     :type: new
     :number: 9
     :text: |
-      +<span id="LC9" class="line">      <span class="k">raise</span> <span class="no"><span class='idiff'>RuntimeError</span></span><span class="p"><span class='idiff'>,</span></span><span class='idiff'> </span><span class="s2">&quot;System commands must be given as an array of strings&quot;</span></span>
+      +<span id="LC9" class="line">      <span class="k">raise</span> <span class="no"><span class='idiff left'>RuntimeError</span></span><span class="p"><span class='idiff'>,</span></span><span class='idiff right'> </span><span class="s2">&quot;System commands must be given as an array of strings&quot;</span></span>
     :line_code: 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_10_9
 - :left:
     :type:

spec/helpers/diff_helper_spec.rb

@@ -104,8 +104,7 @@ describe DiffHelper do
     end
   end
 
-  describe 'diff_line_content' do
-
+  describe '#diff_line_content' do
     it 'should return non breaking space when line is empty' do
       expect(diff_line_content(nil)).to eq('  ')
     end
@@ -116,9 +115,19 @@ describe DiffHelper do
       expect(diff_line_content(diff_file.diff_lines.first.type)).to eq('match')
       expect(diff_file.diff_lines.first.new_pos).to eq(6)
     end
+  end
+
+  describe "#mark_inline_diffs" do
+    let(:old_line) { %{abc 'def'} }
+    let(:new_line) { %{abc "def"} }
+
+    it "returns strings with marked inline diffs" do
+      marked_old_line, marked_new_line = mark_inline_diffs(old_line, new_line)
 
-    it 'should return safe HTML' do
-      expect(diff_line_content(diff_file.diff_lines.first.text)).to be_html_safe
+      expect(marked_old_line).to eq("abc <span class='idiff left right'>&#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 be_html_safe
     end
   end
 end

spec/lib/gitlab/diff/highlight_spec.rb

@@ -9,33 +9,69 @@ describe Gitlab::Diff::Highlight, lib: true do
   let(:diff_file) { Gitlab::Diff::File.new(diff, [commit.parent, commit]) }
 
   describe '#highlight' do
-    let(:diff_lines) { Gitlab::Diff::Highlight.new(diff_file).highlight }
+    context "with a diff file" do
+      let(:subject) { Gitlab::Diff::Highlight.new(diff_file).highlight }
 
       it 'should return Gitlab::Diff::Line elements' do
-      expect(diff_lines.first).to be_an_instance_of(Gitlab::Diff::Line)
+        expect(subject.first).to be_an_instance_of(Gitlab::Diff::Line)
       end
 
       it 'should not modify "match" lines' do
-      expect(diff_lines[0].text).to eq('@@ -6,12 +6,18 @@ module Popen')
-      expect(diff_lines[22].text).to eq('@@ -19,6 +25,7 @@ module Popen')
+        expect(subject[0].text).to eq('@@ -6,12 +6,18 @@ module Popen')
+        expect(subject[22].text).to eq('@@ -19,6 +25,7 @@ module Popen')
       end
 
-    it 'should highlight unchanged lines' do
+      it 'highlights and marks unchanged lines' do
         code = %Q{ <span id="LC7" class="line">  <span class="k">def</span> <span class="nf">popen</span><span class="p">(</span><span class="n">cmd</span><span class="p">,</span> <span class="n">path</span><span class="o">=</span><span class="kp">nil</span><span class="p">)</span></span>\n}
 
-      expect(diff_lines[2].text).to eq(code)
+        expect(subject[2].text).to eq(code)
       end
 
-    it 'should highlight removed lines' do
+      it 'highlights and marks removed lines' do
         code = %Q{-<span id="LC9" class="line">      <span class="k">raise</span> <span class="s2">&quot;System commands must be given as an array of strings&quot;</span></span>\n}
 
-      expect(diff_lines[4].text).to eq(code)
+        expect(subject[4].text).to eq(code)
       end
 
-    it 'should highlight added lines' do
-      code = %Q{+<span id="LC9" class="line">      <span class="k">raise</span> <span class="no"><span class='idiff'>RuntimeError</span></span><span class="p"><span class='idiff'>,</span></span><span class='idiff'> </span><span class="s2">&quot;System commands must be given as an array of strings&quot;</span></span>\n}
+      it 'highlights and marks added lines' do
+        code = %Q{+<span id="LC9" class="line">      <span class="k">raise</span> <span class="no"><span class='idiff left'>RuntimeError</span></span><span class="p"><span class='idiff'>,</span></span><span class='idiff right'> </span><span class="s2">&quot;System commands must be given as an array of strings&quot;</span></span>\n}
 
-      expect(diff_lines[5].text).to eq(code)
+        expect(subject[5].text).to eq(code)
+      end
+    end
+
+    context "with diff lines" do
+      let(:subject) { Gitlab::Diff::Highlight.new(diff_file.diff_lines).highlight }
+
+      it 'should return Gitlab::Diff::Line elements' do
+        expect(subject.first).to be_an_instance_of(Gitlab::Diff::Line)
+      end
+
+      it 'should not modify "match" lines' do
+        expect(subject[0].text).to eq('@@ -6,12 +6,18 @@ module Popen')
+        expect(subject[22].text).to eq('@@ -19,6 +25,7 @@ module Popen')
+      end
+
+      it 'marks unchanged lines' do
+        code = %Q{   def popen(cmd, path=nil)}
+
+        expect(subject[2].text).to eq(code)
+        expect(subject[2].text).not_to be_html_safe
+      end
+
+      it 'marks removed lines' do
+        code = %Q{-      raise "System commands must be given as an array of strings"}
+
+        expect(subject[4].text).to eq(code)
+        expect(subject[4].text).not_to be_html_safe
+      end
+
+      it 'marks added lines' do
+        code = %Q{+      raise <span class='idiff left right'>RuntimeError, </span>&quot;System commands must be given as an array of strings&quot;}
+
+        expect(subject[5].text).to eq(code)
+        expect(subject[5].text).to be_html_safe
+      end
     end
   end
 end

spec/lib/gitlab/diff/inline_diff_marker_spec.rb

@@ -2,14 +2,28 @@ require 'spec_helper'
 
 describe Gitlab::Diff::InlineDiffMarker, lib: true do
   describe '#inline_diffs' do
+
+    context "when the rich text is html safe" do
       let(:raw)  { "abc 'def'" }
-    let(:rich) { %{<span class="abc">abc</span><span class="space"> </span><span class="def">&#39;def&#39;</span>} }
+      let(:rich) { %{<span class="abc">abc</span><span class="space"> </span><span class="def">&#39;def&#39;</span>}.html_safe }
       let(:inline_diffs) { [2..5] }
-
       let(:subject) { Gitlab::Diff::InlineDiffMarker.new(raw, rich).mark(inline_diffs) }
 
       it 'marks the inline diffs' do
-      expect(subject).to eq(%{<span class="abc">ab<span class='idiff'>c</span></span><span class="space"><span class='idiff'> </span></span><span class="def"><span class='idiff'>&#39;d</span>ef&#39;</span>})
+        expect(subject).to eq(%{<span class="abc">ab<span class='idiff left'>c</span></span><span class="space"><span class='idiff'> </span></span><span class="def"><span class='idiff right'>&#39;d</span>ef&#39;</span>})
+        expect(subject).to be_html_safe
+      end
+    end
+
+    context "when the text text is not html safe" do
+      let(:raw)  { "abc 'def'" }
+      let(:inline_diffs) { [2..5] }
+      let(:subject) { Gitlab::Diff::InlineDiffMarker.new(raw).mark(inline_diffs) }
+
+      it 'marks the inline diffs' do
+        expect(subject).to eq(%{ab<span class='idiff left right'>c &#39;d</span>ef&#39;})
+        expect(subject).to be_html_safe
+      end
     end
   end
 end

spec/lib/gitlab/diff/inline_diff_spec.rb

 require 'spec_helper'
 
 describe Gitlab::Diff::InlineDiff, lib: true do
-  describe '#inline_diffs' do
+  describe '.for_lines' do
     let(:diff) do
       <<eos
  class Test
@@ -13,7 +13,7 @@ describe Gitlab::Diff::InlineDiff, lib: true do
 eos
     end
 
-    let(:subject) { Gitlab::Diff::InlineDiff.new(diff.lines).inline_diffs }
+    let(:subject) { described_class.for_lines(diff.lines) }
 
     it 'finds all inline diffs' do
       expect(subject[0]).to be_nil
@@ -24,4 +24,17 @@ eos
       expect(subject[5]).to be_nil
     end
   end
+
+  describe "#inline_diffs" do
+    let(:old_line) { "XXX def initialize(test = true)" }
+    let(:new_line) { "YYY def initialize(test = false)" }
+    let(:subject) { described_class.new(old_line, new_line, offset: 3).inline_diffs }
+
+    it "finds the inline diff" do
+      old_diffs, new_diffs = subject
+
+      expect(old_diffs).to eq([26..28])
+      expect(new_diffs).to eq([26..29])
+    end
+  end
 end