Commit 677b4db9 authored by Douwe Maan's avatar Douwe Maan

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

parent e221990e
...@@ -18,6 +18,7 @@ v 8.5.0 (unreleased) ...@@ -18,6 +18,7 @@ v 8.5.0 (unreleased)
- Revert "Add IP check against DNSBLs at account sign-up" - 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/comments". Use "merge_requests/:merge_request_id/notes" instead
- Deprecate API "merge_request/:merge_request_id/...". Use "merge_requests/:merge_request_id/..." 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 v 8.4.2
- Bump required gitlab-workhorse version to bring in a fix for missing - Bump required gitlab-workhorse version to bring in a fix for missing
......
...@@ -36,6 +36,20 @@ ...@@ -36,6 +36,20 @@
} }
} }
.filename {
&.old {
span.idiff {
background-color: #f8cbcb;
}
}
&.new {
span.idiff {
background-color: #a6f3a6;
}
}
}
.left-options { .left-options {
margin-top: -3px; margin-top: -3px;
} }
...@@ -158,3 +172,15 @@ ...@@ -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;
}
}
module DiffHelper 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 def diff_view
params[:view] == 'parallel' ? 'parallel' : 'inline' params[:view] == 'parallel' ? 'parallel' : 'inline'
end end
...@@ -55,7 +64,7 @@ module DiffHelper ...@@ -55,7 +64,7 @@ module DiffHelper
if line.blank? if line.blank?
"  ".html_safe "  ".html_safe
else else
line.html_safe line
end end
end end
......
...@@ -18,7 +18,7 @@ ...@@ -18,7 +18,7 @@
%div %div
%span by #{commit.author_name} %span by #{commit.author_name}
%i at #{commit.committed_date.to_s(:iso8601)} %i at #{commit.committed_date.to_s(:iso8601)}
%pre.commit-message %pre.commit-message
= commit.safe_message = commit.safe_message
%h4 #{pluralize @message.diffs_count, "changed file"}: %h4 #{pluralize @message.diffs_count, "changed file"}:
......
...@@ -7,16 +7,20 @@ ...@@ -7,16 +7,20 @@
= submodule_link(blob, @commit.id, project.repository) = submodule_link(blob, @commit.id, project.repository)
- else - else
= blob_icon blob.mode, blob.name = blob_icon blob.mode, blob.name
= link_to "#diff-#{i}" do
%strong
= diff_file.new_path
- if diff_file.deleted_file = link_to "#diff-#{i}" do
deleted - if diff_file.renamed_file
- elsif diff_file.renamed_file - old_path, new_path = mark_inline_diffs(diff_file.old_path, diff_file.new_path)
renamed from %strong.filename.old
%strong = old_path
= diff_file.old_path →
%strong.filename.new
= new_path
- else
%strong
= diff_file.new_path
- if diff_file.deleted_file
deleted
- if diff_file.mode_changed? - if diff_file.mode_changed?
%small %small
......
...@@ -21,13 +21,13 @@ module Gitlab ...@@ -21,13 +21,13 @@ module Gitlab
# ignore highlighting for "match" lines # ignore highlighting for "match" lines
next diff_line if diff_line.type == 'match' || diff_line.type == 'nonewline' 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] if line_inline_diffs = inline_diffs[i]
rich_line = InlineDiffMarker.new(diff_line.text, rich_line).mark(line_inline_diffs) rich_line = InlineDiffMarker.new(diff_line.text, rich_line).mark(line_inline_diffs)
end end
diff_line.text = rich_line.html_safe diff_line.text = rich_line
diff_line diff_line
end end
...@@ -35,8 +35,8 @@ module Gitlab ...@@ -35,8 +35,8 @@ module Gitlab
private private
def highlight_line(diff_line, index) def highlight_line(diff_line)
return html_escape(diff_line.text) unless diff_file && diff_file.diff_refs return unless diff_file && diff_file.diff_refs
line_prefix = diff_line.text.match(/\A(.)/) ? $1 : ' ' line_prefix = diff_line.text.match(/\A(.)/) ? $1 : ' '
...@@ -49,11 +49,11 @@ module Gitlab ...@@ -49,11 +49,11 @@ module Gitlab
# Only update text if line is found. This will prevent # Only update text if line is found. This will prevent
# issues with submodules given the line only exists in diff content. # 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 end
def inline_diffs def inline_diffs
@inline_diffs ||= InlineDiff.new(@raw_lines).inline_diffs @inline_diffs ||= InlineDiff.for_lines(@raw_lines)
end end
def old_lines def old_lines
...@@ -72,11 +72,6 @@ module Gitlab ...@@ -72,11 +72,6 @@ module Gitlab
[ref.project.repository, ref.id, path] [ref.project.repository, ref.id, path]
end end
def html_escape(str)
replacements = { '&' => '&amp;', '>' => '&gt;', '<' => '&lt;', '"' => '&quot;', "'" => '&#39;' }
str.gsub(/[&"'><]/, replacements)
end
end end
end end
end end
module Gitlab module Gitlab
module Diff module Diff
class InlineDiff class InlineDiff
attr_accessor :lines attr_accessor :old_line, :new_line, :offset
def initialize(lines) def self.for_lines(lines)
@lines = lines local_edit_indexes = self.find_local_edits(lines)
end
def inline_diffs
inline_diffs = [] inline_diffs = []
local_edit_indexes.each do |index| local_edit_indexes.each do |index|
old_index = index old_index = index
new_index = index + 1 new_index = index + 1
old_line = @lines[old_index] old_line = lines[old_index]
new_line = @lines[new_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_diff_range = lcp..(old_line.length - lcs - 1) old_diffs, new_diffs = new(old_line, new_line, offset: 1).inline_diffs
new_diff_range = lcp..(new_line.length - lcs - 1)
inline_diffs[old_index] = [old_diff_range] if old_diff_range.begin <= old_diff_range.end inline_diffs[old_index] = old_diffs
inline_diffs[new_index] = [new_diff_range] if new_diff_range.begin <= new_diff_range.end inline_diffs[new_index] = new_diffs
end end
inline_diffs inline_diffs
end 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 private
# Find runs of single line edits def self.find_local_edits(lines)
def local_edit_indexes line_prefixes = lines.map { |line| line.match(/\A([+-])/) ? $1 : ' ' }
line_prefixes = @lines.map { |line| line.match(/\A([+-])/) ? $1 : ' ' }
joined_line_prefixes = " #{line_prefixes.join} " joined_line_prefixes = " #{line_prefixes.join} "
offset = 0 offset = 0
......
...@@ -5,10 +5,12 @@ module Gitlab ...@@ -5,10 +5,12 @@ module Gitlab
def initialize(raw_line, rich_line = raw_line) def initialize(raw_line, rich_line = raw_line)
@raw_line = raw_line @raw_line = raw_line
@rich_line = rich_line @rich_line = ERB::Util.html_escape(rich_line)
end end
def mark(line_inline_diffs) def mark(line_inline_diffs)
return rich_line unless line_inline_diffs
marker_ranges = [] marker_ranges = []
line_inline_diffs.each do |inline_diff_range| 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 # Map the inline-diff range based on the raw line to character positions in the rich line
...@@ -19,11 +21,15 @@ module Gitlab ...@@ -19,11 +21,15 @@ module Gitlab
offset = 0 offset = 0
# Mark each range # Mark each range
marker_ranges.each do |range| marker_ranges.each_with_index do |range, i|
offset = insert_around_range(rich_line, range, "<span class='idiff'>", "</span>", offset) 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 end
rich_line rich_line.html_safe
end end
private private
......
...@@ -55,7 +55,7 @@ ...@@ -55,7 +55,7 @@
:type: new :type: new
:number: 9 :number: 9
:text: | :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 :line_code: 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_10_9
- :left: - :left:
:type: :type:
......
...@@ -9,33 +9,69 @@ describe Gitlab::Diff::Highlight, lib: true do ...@@ -9,33 +9,69 @@ describe Gitlab::Diff::Highlight, lib: true do
let(:diff_file) { Gitlab::Diff::File.new(diff, [commit.parent, commit]) } let(:diff_file) { Gitlab::Diff::File.new(diff, [commit.parent, commit]) }
describe '#highlight' do 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 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 end
it 'should not modify "match" lines' do it 'should not modify "match" lines' do
expect(diff_lines[0].text).to eq('@@ -6,12 +6,18 @@ module Popen') expect(subject[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[22].text).to eq('@@ -19,6 +25,7 @@ module Popen')
end 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} 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 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} 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 end
it 'should highlight added lines' do 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'>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} 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 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
end end
...@@ -2,14 +2,28 @@ require 'spec_helper' ...@@ -2,14 +2,28 @@ require 'spec_helper'
describe Gitlab::Diff::InlineDiffMarker, lib: true do describe Gitlab::Diff::InlineDiffMarker, lib: true do
describe '#inline_diffs' do describe '#inline_diffs' do
let(:raw) { "abc 'def'" }
let(:rich) { %{<span class="abc">abc</span><span class="space"> </span><span class="def">&#39;def&#39;</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">&#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 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 end
end end
require 'spec_helper' require 'spec_helper'
describe Gitlab::Diff::InlineDiff, lib: true do describe Gitlab::Diff::InlineDiff, lib: true do
describe '#inline_diffs' do describe '.for_lines' do
let(:diff) do let(:diff) do
<<eos <<eos
class Test class Test
...@@ -13,7 +13,7 @@ describe Gitlab::Diff::InlineDiff, lib: true do ...@@ -13,7 +13,7 @@ describe Gitlab::Diff::InlineDiff, lib: true do
eos eos
end 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 it 'finds all inline diffs' do
expect(subject[0]).to be_nil expect(subject[0]).to be_nil
...@@ -24,4 +24,17 @@ eos ...@@ -24,4 +24,17 @@ eos
expect(subject[5]).to be_nil expect(subject[5]).to be_nil
end end
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 end
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment