diff options
| -rw-r--r-- | CHANGELOG | 1 | ||||
| -rw-r--r-- | app/assets/stylesheets/framework/files.scss | 26 | ||||
| -rw-r--r-- | app/helpers/diff_helper.rb | 11 | ||||
| -rw-r--r-- | app/views/notify/repository_push_email.html.haml | 2 | ||||
| -rw-r--r-- | app/views/projects/diffs/_file.html.haml | 22 | ||||
| -rw-r--r-- | lib/gitlab/diff/highlight.rb | 17 | ||||
| -rw-r--r-- | lib/gitlab/diff/inline_diff.rb | 57 | ||||
| -rw-r--r-- | lib/gitlab/diff/inline_diff_marker.rb | 14 | ||||
| -rw-r--r-- | spec/fixtures/parallel_diff_result.yml | 2 | ||||
| -rw-r--r-- | spec/lib/gitlab/diff/highlight_spec.rb | 74 | ||||
| -rw-r--r-- | spec/lib/gitlab/diff/inline_diff_marker_spec.rb | 26 | ||||
| -rw-r--r-- | spec/lib/gitlab/diff/inline_diff_spec.rb | 17 |
12 files changed, 194 insertions, 75 deletions
diff --git a/CHANGELOG b/CHANGELOG index 9dec6f9809e..d309ef318cf 100644 --- a/CHANGELOG +++ b/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.2 - Bump required gitlab-workhorse version to bring in a fix for missing diff --git a/app/assets/stylesheets/framework/files.scss b/app/assets/stylesheets/framework/files.scss index 00cb756b376..c7f3604850d 100644 --- a/app/assets/stylesheets/framework/files.scss +++ b/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; + } +} diff --git a/app/helpers/diff_helper.rb b/app/helpers/diff_helper.rb index 62971d1e14b..0b4fda12788 100644 --- a/app/helpers/diff_helper.rb +++ b/app/helpers/diff_helper.rb @@ -1,4 +1,13 @@ 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).html_safe + marked_new_line = Gitlab::Diff::InlineDiffMarker.new(new_line).mark(new_diffs).html_safe + + [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 diff --git a/app/views/notify/repository_push_email.html.haml b/app/views/notify/repository_push_email.html.haml index 3dd2595f1ad..f2e405b14fd 100644 --- a/app/views/notify/repository_push_email.html.haml +++ b/app/views/notify/repository_push_email.html.haml @@ -18,7 +18,7 @@ %div %span by #{commit.author_name} %i at #{commit.committed_date.to_s(:iso8601)} - %pre.commit-message + %pre.commit-message = commit.safe_message %h4 #{pluralize @message.diffs_count, "changed file"}: diff --git a/app/views/projects/diffs/_file.html.haml b/app/views/projects/diffs/_file.html.haml index fc0eaef2286..3ac058a3bf8 100644 --- a/app/views/projects/diffs/_file.html.haml +++ b/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 - %strong - = diff_file.new_path - - if diff_file.deleted_file - deleted - - elsif diff_file.renamed_file - renamed from - %strong - = diff_file.old_path + = 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 - if diff_file.mode_changed? %small diff --git a/lib/gitlab/diff/highlight.rb b/lib/gitlab/diff/highlight.rb index a7f925ce134..9429b3ff88d 100644 --- a/lib/gitlab/diff/highlight.rb +++ b/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 = { '&' => '&', '>' => '>', '<' => '<', '"' => '"', "'" => ''' } - str.gsub(/[&"'><]/, replacements) - end end end end diff --git a/lib/gitlab/diff/inline_diff.rb b/lib/gitlab/diff/inline_diff.rb index b8a61ad6115..789c14518b0 100644 --- a/lib/gitlab/diff/inline_diff.rb +++ b/lib/gitlab/diff/inline_diff.rb @@ -1,43 +1,58 @@ 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] - - # Skip inline diff if empty line was replaced with content - next if old_line[1..-1] == "" - - # Add one, because this is based on the prefixless version - lcp = longest_common_prefix(old_line[1..-1], new_line[1..-1]) + 1 - lcs = longest_common_suffix(old_line[lcp..-1], new_line[lcp..-1]) + old_line = lines[old_index] + new_line = lines[new_index] - old_diff_range = lcp..(old_line.length - lcs - 1) - new_diff_range = lcp..(new_line.length - lcs - 1) + old_diffs, new_diffs = new(old_line, new_line, offset: 1).inline_diffs - 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 + 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 + return if old_line == "" + + lcp = longest_common_prefix(old_line, new_line) + lcs = longest_common_suffix(old_line[lcp..-1], new_line[lcp..-1]) + + lcp += offset + old_length = old_line.length + offset + new_length = new_line.length + offset + + old_diff_range = lcp..(old_length - lcs - 1) + new_diff_range = lcp..(new_length - lcs - 1) + + 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 diff --git a/lib/gitlab/diff/inline_diff_marker.rb b/lib/gitlab/diff/inline_diff_marker.rb index 1d7fa1bce06..dccb717e95d 100644 --- a/lib/gitlab/diff/inline_diff_marker.rb +++ b/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 diff --git a/spec/fixtures/parallel_diff_result.yml b/spec/fixtures/parallel_diff_result.yml index a326b651aad..a8b7907d4ba 100644 --- a/spec/fixtures/parallel_diff_result.yml +++ b/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">"System commands must be given as an array of strings"</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">"System commands must be given as an array of strings"</span></span> :line_code: 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_10_9 - :left: :type: diff --git a/spec/lib/gitlab/diff/highlight_spec.rb b/spec/lib/gitlab/diff/highlight_spec.rb index b84a57f357a..deab16714e0 100644 --- a/spec/lib/gitlab/diff/highlight_spec.rb +++ b/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) - end + 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(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') - 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 'should highlight 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} + 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) - end + expect(subject[2].text).to eq(code) + end - it 'should highlight removed lines' do - code = %Q{-<span id="LC9" class="line"> <span class="k">raise</span> <span class="s2">"System commands must be given as an array of strings"</span></span>\n} + it 'highlights and marks removed lines' do + code = %Q{-<span id="LC9" class="line"> <span class="k">raise</span> <span class="s2">"System commands must be given as an array of strings"</span></span>\n} - expect(diff_lines[4].text).to eq(code) - end + 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">"System commands must be given as an array of strings"</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">"System commands must be given as an array of strings"</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>"System commands must be given as an array of strings"} + + expect(subject[5].text).to eq(code) + expect(subject[5].text).to be_html_safe + end + end end end diff --git a/spec/lib/gitlab/diff/inline_diff_marker_spec.rb b/spec/lib/gitlab/diff/inline_diff_marker_spec.rb index 6f3276a8b53..ea5c31011f0 100644 --- a/spec/lib/gitlab/diff/inline_diff_marker_spec.rb +++ b/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 - let(:raw) { "abc 'def'" } - let(:rich) { %{<span class="abc">abc</span><span class="space"> </span><span class="def">'def'</span>} } - let(:inline_diffs) { [2..5] } - let(:subject) { Gitlab::Diff::InlineDiffMarker.new(raw, rich).mark(inline_diffs) } + 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">'def'</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'>'d</span>ef'</span>}) + it 'marks the inline diffs' do + 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'>'d</span>ef'</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 'd</span>ef'}) + expect(subject).to be_html_safe + end end end end diff --git a/spec/lib/gitlab/diff/inline_diff_spec.rb b/spec/lib/gitlab/diff/inline_diff_spec.rb index 056917df893..95a993d26cf 100644 --- a/spec/lib/gitlab/diff/inline_diff_spec.rb +++ b/spec/lib/gitlab/diff/inline_diff_spec.rb @@ -1,7 +1,7 @@ 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 |