From 34657b821ae597de76ffd5a70d2b0b298dc270ed Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rub=C3=A9n=20D=C3=A1vila?= Date: Tue, 15 Dec 2015 18:09:09 -0500 Subject: [PATCH 01/57] Add syntax highlighting to diff view. #3945 --- app/helpers/application_helper.rb | 6 ++++ app/helpers/blob_helper.rb | 29 ++++++++++++++++--- .../projects/diffs/_parallel_view.html.haml | 8 +++-- app/views/projects/diffs/_text_file.html.haml | 6 ++-- lib/rouge/lexers/gitlab_diff.rb | 20 +++++++++++++ spec/helpers/blob_helper_spec.rb | 11 +++++++ 6 files changed, 71 insertions(+), 9 deletions(-) create mode 100644 lib/rouge/lexers/gitlab_diff.rb diff --git a/app/helpers/application_helper.rb b/app/helpers/application_helper.rb index 0b00b9a0702..bc4b6ec0327 100644 --- a/app/helpers/application_helper.rb +++ b/app/helpers/application_helper.rb @@ -326,4 +326,10 @@ module ApplicationHelper def truncate_first_line(message, length = 50) truncate(message.each_line.first.chomp, length: length) if message end + + def unescape_html(content) + text = CGI.unescapeHTML(content) + text.gsub!(' ', ' ') + text + end end diff --git a/app/helpers/blob_helper.rb b/app/helpers/blob_helper.rb index d31d4cde08f..bf18673972c 100644 --- a/app/helpers/blob_helper.rb +++ b/app/helpers/blob_helper.rb @@ -1,11 +1,17 @@ module BlobHelper - def highlight(blob_name, blob_content, nowrap: false, continue: false) - @formatter ||= Rouge::Formatters::HTMLGitlab.new( - nowrap: nowrap, + def rouge_formatter(options = {}) + default_options = { + nowrap: false, cssclass: 'code highlight', lineanchors: true, lineanchorsid: 'LC' - ) + } + + Rouge::Formatters::HTMLGitlab.new(default_options.merge!(options)) + end + + def highlight(blob_name, blob_content, nowrap: false, continue: false) + @formatter ||= rouge_formatter(nowrap: nowrap) begin @lexer ||= Rouge::Lexer.guess(filename: blob_name, source: blob_content).new @@ -18,6 +24,21 @@ module BlobHelper result end + def highlight_line(blob_name, content, continue: false) + if @previous_blob_name != blob_name + @parent = Rouge::Lexer.guess(filename: blob_name, source: content).new rescue Rouge::Lexers::PlainText.new + @lexer = Rouge::Lexers::GitlabDiff.new(parent_lexer: @parent) + @options = Rouge::Lexers::PlainText === @parent ? {} : { continue: continue } + end + + @previous_blob_name = blob_name + @formatter ||= rouge_formatter(nowrap: true) + + content.sub!(/\A((?:\+|-)\s*)/, '') # Don't format '+' or '-' indicators. + + "#{$1}#{@formatter.format(@lexer.lex(content, @options))}".html_safe + end + def no_highlight_files %w(credits changelog news copying copyright license authors) end diff --git a/app/views/projects/diffs/_parallel_view.html.haml b/app/views/projects/diffs/_parallel_view.html.haml index 37fd1b1ec8a..c6a9d71e789 100644 --- a/app/views/projects/diffs/_parallel_view.html.haml +++ b/app/views/projects/diffs/_parallel_view.html.haml @@ -1,5 +1,5 @@ / Side-by-side diff view -%div.text-file.diff-wrap-lines +%div.text-file.diff-wrap-lines.code.file-content.js-syntax-highlight{ class: user_color_scheme } %table - parallel_diff(diff_file, index).each do |line| - type_left = line[0] @@ -20,7 +20,8 @@ = link_to raw(line_number_left), "##{line_code_left}", id: line_code_left - if @comments_allowed && can?(current_user, :create_note, @project) = link_to_new_diff_note(line_code_left, 'old') - %td.line_content{class: "parallel noteable_line #{type_left} #{line_code_left}", "line_code" => line_code_left }= raw line_content_left + %td.line_content{class: "parallel noteable_line #{type_left} #{line_code_left}", "line_code" => line_code_left }< + = highlight_line(diff_file.new_path, unescape_html(line_content_left)) - if type_right == 'new' - new_line_class = 'new' @@ -33,7 +34,8 @@ = link_to raw(line_number_right), "##{new_line_code}", id: new_line_code - if @comments_allowed && can?(current_user, :create_note, @project) = link_to_new_diff_note(line_code_right, 'new') - %td.line_content.parallel{class: "noteable_line #{new_line_class} #{new_line_code}", "line_code" => new_line_code}= raw line_content_right + %td.line_content.parallel{class: "noteable_line #{new_line_class} #{new_line_code}", "line_code" => new_line_code}< + = highlight_line(diff_file.new_path, unescape_html(line_content_right)) - if @reply_allowed - comments_left, comments_right = organize_comments(type_left, type_right, line_code_left, line_code_right) diff --git a/app/views/projects/diffs/_text_file.html.haml b/app/views/projects/diffs/_text_file.html.haml index 977ca423f75..78c66a6291e 100644 --- a/app/views/projects/diffs/_text_file.html.haml +++ b/app/views/projects/diffs/_text_file.html.haml @@ -3,7 +3,8 @@ .suppressed-container %a.show-suppressed-diff.js-show-suppressed-diff Changes suppressed. Click to show. -%table.text-file{class: "#{'hide' if too_big}"} +%table.text-file.code.js-syntax-highlight{ class: [user_color_scheme, too_big ? 'hide' : ''] } + - last_line = 0 - diff_file.diff_lines.each_with_index do |line, index| - type = line.type @@ -21,7 +22,8 @@ = link_to_new_diff_note(line_code) %td.new_line{data: {linenumber: line.new_pos}} = link_to raw(type == "old" ? " " : line.new_pos) , "##{line_code}", id: line_code - %td.line_content{class: "noteable_line #{type} #{line_code}", "line_code" => line_code}= raw diff_line_content(line.text) + %td.line_content{class: "noteable_line #{type} #{line_code}", "line_code" => line_code}< + = highlight_line(diff_file.new_path, unescape_html(diff_line_content(line.text))) - if @reply_allowed - comments = @line_notes.select { |n| n.line_code == line_code && n.active? }.sort_by(&:created_at) diff --git a/lib/rouge/lexers/gitlab_diff.rb b/lib/rouge/lexers/gitlab_diff.rb new file mode 100644 index 00000000000..e136d47df00 --- /dev/null +++ b/lib/rouge/lexers/gitlab_diff.rb @@ -0,0 +1,20 @@ +Rouge::Token::Tokens.token(:InlineDiff, 'idiff') + +module Rouge + module Lexers + class GitlabDiff < RegexLexer + title "GitLab Diff" + tag 'gitlab_diff' + + state :root do + rule %r{(.*?)} do |match| + token InlineDiff, match[1] + end + + rule /(?:(?!puts 'Hello' world) + end + + it 'should respect the inline diff markup' do + result = highlight_line('demo.rb', "puts 'Hello' world") + expect(result).to eq(expected) + end + end end From c031b9d9cd1ea41ab68f46eb5e630efaf901933a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rub=C3=A9n=20D=C3=A1vila?= Date: Wed, 23 Dec 2015 19:10:13 -0500 Subject: [PATCH 02/57] Set initial state on parent Lexer. #3945 --- lib/rouge/lexers/gitlab_diff.rb | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/lib/rouge/lexers/gitlab_diff.rb b/lib/rouge/lexers/gitlab_diff.rb index e136d47df00..c7aaeb92608 100644 --- a/lib/rouge/lexers/gitlab_diff.rb +++ b/lib/rouge/lexers/gitlab_diff.rb @@ -15,6 +15,10 @@ module Rouge delegate option(:parent_lexer) end end + + start do + option(:parent_lexer).reset! + end end end end From bb96d631537d3d8181f0d3b762603a012219c3e6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rub=C3=A9n=20D=C3=A1vila?= Date: Wed, 30 Dec 2015 00:52:50 -0500 Subject: [PATCH 03/57] New implementation for highlighting diff files. #3945 * It is more performant given now we process all the diff file instead of processing line by line. * Multiline comments are highlighted correctly. --- app/helpers/application_helper.rb | 6 -- app/helpers/blob_helper.rb | 15 ----- app/helpers/diff_helper.rb | 6 +- .../projects/diffs/_parallel_view.html.haml | 6 +- app/views/projects/diffs/_text_file.html.haml | 5 +- lib/gitlab/diff/file.rb | 4 ++ lib/gitlab/diff/highlight.rb | 55 +++++++++++++++++++ lib/gitlab/diff/line.rb | 1 + lib/rouge/lexers/gitlab_diff.rb | 2 +- spec/helpers/blob_helper_spec.rb | 11 ---- 10 files changed, 68 insertions(+), 43 deletions(-) create mode 100644 lib/gitlab/diff/highlight.rb diff --git a/app/helpers/application_helper.rb b/app/helpers/application_helper.rb index bc4b6ec0327..0b00b9a0702 100644 --- a/app/helpers/application_helper.rb +++ b/app/helpers/application_helper.rb @@ -326,10 +326,4 @@ module ApplicationHelper def truncate_first_line(message, length = 50) truncate(message.each_line.first.chomp, length: length) if message end - - def unescape_html(content) - text = CGI.unescapeHTML(content) - text.gsub!(' ', ' ') - text - end end diff --git a/app/helpers/blob_helper.rb b/app/helpers/blob_helper.rb index bf18673972c..1230002e69c 100644 --- a/app/helpers/blob_helper.rb +++ b/app/helpers/blob_helper.rb @@ -24,21 +24,6 @@ module BlobHelper result end - def highlight_line(blob_name, content, continue: false) - if @previous_blob_name != blob_name - @parent = Rouge::Lexer.guess(filename: blob_name, source: content).new rescue Rouge::Lexers::PlainText.new - @lexer = Rouge::Lexers::GitlabDiff.new(parent_lexer: @parent) - @options = Rouge::Lexers::PlainText === @parent ? {} : { continue: continue } - end - - @previous_blob_name = blob_name - @formatter ||= rouge_formatter(nowrap: true) - - content.sub!(/\A((?:\+|-)\s*)/, '') # Don't format '+' or '-' indicators. - - "#{$1}#{@formatter.format(@lexer.lex(content, @options))}".html_safe - end - def no_highlight_files %w(credits changelog news copying copyright license authors) end diff --git a/app/helpers/diff_helper.rb b/app/helpers/diff_helper.rb index 24134310fc5..2ff8c65e5ca 100644 --- a/app/helpers/diff_helper.rb +++ b/app/helpers/diff_helper.rb @@ -54,9 +54,9 @@ module DiffHelper # right_line_type, right_line_number, right_line_content, right_line_code # ] # - diff_file.diff_lines.each do |line| + diff_file.highlighted_diff_lines.each do |line| - full_line = line.text + full_line = line.highlighted_text type = line.type line_code = generate_line_code(diff_file.file_path, line) line_new = line.new_pos @@ -67,7 +67,7 @@ module DiffHelper if next_line next_line_code = generate_line_code(diff_file.file_path, next_line) next_type = next_line.type - next_line = next_line.text + next_line = next_line.highlighted_text end if type == 'match' || type.nil? diff --git a/app/views/projects/diffs/_parallel_view.html.haml b/app/views/projects/diffs/_parallel_view.html.haml index c6a9d71e789..4ded4d2daad 100644 --- a/app/views/projects/diffs/_parallel_view.html.haml +++ b/app/views/projects/diffs/_parallel_view.html.haml @@ -20,8 +20,7 @@ = link_to raw(line_number_left), "##{line_code_left}", id: line_code_left - if @comments_allowed && can?(current_user, :create_note, @project) = link_to_new_diff_note(line_code_left, 'old') - %td.line_content{class: "parallel noteable_line #{type_left} #{line_code_left}", "line_code" => line_code_left }< - = highlight_line(diff_file.new_path, unescape_html(line_content_left)) + %td.line_content{class: "parallel noteable_line #{type_left} #{line_code_left}", "line_code" => line_code_left }= raw(line_content_left) - if type_right == 'new' - new_line_class = 'new' @@ -34,8 +33,7 @@ = link_to raw(line_number_right), "##{new_line_code}", id: new_line_code - if @comments_allowed && can?(current_user, :create_note, @project) = link_to_new_diff_note(line_code_right, 'new') - %td.line_content.parallel{class: "noteable_line #{new_line_class} #{new_line_code}", "line_code" => new_line_code}< - = highlight_line(diff_file.new_path, unescape_html(line_content_right)) + %td.line_content.parallel{class: "noteable_line #{new_line_class} #{new_line_code}", "line_code" => new_line_code}= raw(line_content_right) - if @reply_allowed - comments_left, comments_right = organize_comments(type_left, type_right, line_code_left, line_code_right) diff --git a/app/views/projects/diffs/_text_file.html.haml b/app/views/projects/diffs/_text_file.html.haml index 78c66a6291e..641e9e5501a 100644 --- a/app/views/projects/diffs/_text_file.html.haml +++ b/app/views/projects/diffs/_text_file.html.haml @@ -6,7 +6,7 @@ %table.text-file.code.js-syntax-highlight{ class: [user_color_scheme, too_big ? 'hide' : ''] } - last_line = 0 - - diff_file.diff_lines.each_with_index do |line, index| + - diff_file.highlighted_diff_lines.each_with_index do |line, index| - type = line.type - last_line = line.new_pos - line_code = generate_line_code(diff_file.file_path, line) @@ -22,8 +22,7 @@ = link_to_new_diff_note(line_code) %td.new_line{data: {linenumber: line.new_pos}} = link_to raw(type == "old" ? " " : line.new_pos) , "##{line_code}", id: line_code - %td.line_content{class: "noteable_line #{type} #{line_code}", "line_code" => line_code}< - = highlight_line(diff_file.new_path, unescape_html(diff_line_content(line.text))) + %td.line_content{class: "noteable_line #{type} #{line_code}", "line_code" => line_code}= raw(diff_line_content(line.highlighted_text)) - if @reply_allowed - comments = @line_notes.select { |n| n.line_code == line_code && n.active? }.sort_by(&:created_at) diff --git a/lib/gitlab/diff/file.rb b/lib/gitlab/diff/file.rb index 79061cd0141..ff8765b8e26 100644 --- a/lib/gitlab/diff/file.rb +++ b/lib/gitlab/diff/file.rb @@ -15,6 +15,10 @@ module Gitlab @lines ||= parser.parse(raw_diff.lines) end + def highlighted_diff_lines + Gitlab::Diff::Highlight.process_diff_lines(self) + end + def mode_changed? !!(diff.a_mode && diff.b_mode && diff.a_mode != diff.b_mode) end diff --git a/lib/gitlab/diff/highlight.rb b/lib/gitlab/diff/highlight.rb new file mode 100644 index 00000000000..f10b55eb00b --- /dev/null +++ b/lib/gitlab/diff/highlight.rb @@ -0,0 +1,55 @@ +module Gitlab + module Diff + class Highlight + def self.process_diff_lines(diff_file) + processor = new(diff_file) + processor.highlight + end + + def initialize(diff_file) + text_lines = diff_file.diff_lines.map(&:text) + @diff_file = diff_file + @diff_lines = diff_file.diff_lines + @diff_line_prefixes = text_lines.map { |line| line.sub!(/\A((\+|\-)\s*)/, '');$1 } + @raw_lines = text_lines.join("\n") + end + + def highlight + @code = unescape_html(@raw_lines) + @highlighted_code = formatter.format(lexer.lex(@code)) + + update_diff_lines + end + + private + + def update_diff_lines + @highlighted_code.lines.each_with_index do |line, i| + @diff_lines[i].highlighted_text = "#{@diff_line_prefixes[i]}#{line}" + end + + @diff_lines + end + + def lexer + parent = Rouge::Lexer.guess(filename: @diff_file.new_path, source: @code).new rescue Rouge::Lexers::PlainText.new + Rouge::Lexers::GitlabDiff.new(parent_lexer: parent) + end + + def unescape_html(content) + text = CGI.unescapeHTML(content) + text.gsub!(' ', ' ') + text + end + + def formatter + @formatter ||= Rouge::Formatters::HTMLGitlab.new( + nowrap: true, + cssclass: 'code highlight', + lineanchors: true, + lineanchorsid: 'LC' + ) + end + end + end +end diff --git a/lib/gitlab/diff/line.rb b/lib/gitlab/diff/line.rb index 0072194606e..c48c69fb344 100644 --- a/lib/gitlab/diff/line.rb +++ b/lib/gitlab/diff/line.rb @@ -2,6 +2,7 @@ module Gitlab module Diff class Line attr_reader :type, :text, :index, :old_pos, :new_pos + attr_accessor :highlighted_text def initialize(text, type, index, old_pos, new_pos) @text, @type, @index = text, type, index diff --git a/lib/rouge/lexers/gitlab_diff.rb b/lib/rouge/lexers/gitlab_diff.rb index c7aaeb92608..d91dd6c4245 100644 --- a/lib/rouge/lexers/gitlab_diff.rb +++ b/lib/rouge/lexers/gitlab_diff.rb @@ -11,7 +11,7 @@ module Rouge token InlineDiff, match[1] end - rule /(?:(?!puts 'Hello' world) - end - - it 'should respect the inline diff markup' do - result = highlight_line('demo.rb', "puts 'Hello' world") - expect(result).to eq(expected) - end - end end From b74f36c9caae38a1d62c18281d8240ec5905c5d8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rub=C3=A9n=20D=C3=A1vila?= Date: Wed, 30 Dec 2015 13:10:28 -0500 Subject: [PATCH 04/57] Fix Rubocop complain. #3945 --- lib/gitlab/diff/highlight.rb | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/lib/gitlab/diff/highlight.rb b/lib/gitlab/diff/highlight.rb index f10b55eb00b..adb437abed2 100644 --- a/lib/gitlab/diff/highlight.rb +++ b/lib/gitlab/diff/highlight.rb @@ -44,11 +44,11 @@ module Gitlab def formatter @formatter ||= Rouge::Formatters::HTMLGitlab.new( - nowrap: true, - cssclass: 'code highlight', - lineanchors: true, - lineanchorsid: 'LC' - ) + nowrap: true, + cssclass: 'code highlight', + lineanchors: true, + lineanchorsid: 'LC' + ) end end end From 7de90f4b53f865dc417d022a9133372e57274549 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rub=C3=A9n=20D=C3=A1vila?= Date: Wed, 30 Dec 2015 18:42:11 -0500 Subject: [PATCH 05/57] Fix broken spec and small refactor. #3945 --- app/helpers/diff_helper.rb | 4 +- app/views/projects/diffs/_text_file.html.haml | 2 +- lib/gitlab/diff/highlight.rb | 2 +- lib/gitlab/diff/line.rb | 4 +- spec/fixtures/parallel_diff_result.yml | 274 ++++++++++++++++++ spec/helpers/diff_helper_spec.rb | 30 +- 6 files changed, 281 insertions(+), 35 deletions(-) create mode 100644 spec/fixtures/parallel_diff_result.yml diff --git a/app/helpers/diff_helper.rb b/app/helpers/diff_helper.rb index 2ff8c65e5ca..22deb69cec5 100644 --- a/app/helpers/diff_helper.rb +++ b/app/helpers/diff_helper.rb @@ -56,7 +56,7 @@ module DiffHelper # diff_file.highlighted_diff_lines.each do |line| - full_line = line.highlighted_text + full_line = line.text type = line.type line_code = generate_line_code(diff_file.file_path, line) line_new = line.new_pos @@ -67,7 +67,7 @@ module DiffHelper if next_line next_line_code = generate_line_code(diff_file.file_path, next_line) next_type = next_line.type - next_line = next_line.highlighted_text + next_line = next_line.text end if type == 'match' || type.nil? diff --git a/app/views/projects/diffs/_text_file.html.haml b/app/views/projects/diffs/_text_file.html.haml index 641e9e5501a..059d0baf45e 100644 --- a/app/views/projects/diffs/_text_file.html.haml +++ b/app/views/projects/diffs/_text_file.html.haml @@ -22,7 +22,7 @@ = link_to_new_diff_note(line_code) %td.new_line{data: {linenumber: line.new_pos}} = link_to raw(type == "old" ? " " : line.new_pos) , "##{line_code}", id: line_code - %td.line_content{class: "noteable_line #{type} #{line_code}", "line_code" => line_code}= raw(diff_line_content(line.highlighted_text)) + %td.line_content{class: "noteable_line #{type} #{line_code}", "line_code" => line_code}= raw(diff_line_content(line.text)) - if @reply_allowed - comments = @line_notes.select { |n| n.line_code == line_code && n.active? }.sort_by(&:created_at) diff --git a/lib/gitlab/diff/highlight.rb b/lib/gitlab/diff/highlight.rb index adb437abed2..d0c2e3670c6 100644 --- a/lib/gitlab/diff/highlight.rb +++ b/lib/gitlab/diff/highlight.rb @@ -25,7 +25,7 @@ module Gitlab def update_diff_lines @highlighted_code.lines.each_with_index do |line, i| - @diff_lines[i].highlighted_text = "#{@diff_line_prefixes[i]}#{line}" + @diff_lines[i].text = "#{@diff_line_prefixes[i]}#{line}" end @diff_lines diff --git a/lib/gitlab/diff/line.rb b/lib/gitlab/diff/line.rb index c48c69fb344..03730b435ad 100644 --- a/lib/gitlab/diff/line.rb +++ b/lib/gitlab/diff/line.rb @@ -1,8 +1,8 @@ module Gitlab module Diff class Line - attr_reader :type, :text, :index, :old_pos, :new_pos - attr_accessor :highlighted_text + attr_reader :type, :index, :old_pos, :new_pos + attr_accessor :text def initialize(text, type, index, old_pos, new_pos) @text, @type, @index = text, type, index diff --git a/spec/fixtures/parallel_diff_result.yml b/spec/fixtures/parallel_diff_result.yml new file mode 100644 index 00000000000..abbb364f61f --- /dev/null +++ b/spec/fixtures/parallel_diff_result.yml @@ -0,0 +1,274 @@ +--- +- - match + - 6 + - | + @@ -6,12 +6,18 @@ module Popen + - 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_6_6 + - match + - 6 + - | + @@ -6,12 +6,18 @@ module Popen + - 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_6_6 +- - + - 6 + - | + + - 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_6_6 + - + - 6 + - | + + - 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_6_6 +- - + - 7 + - | + def popen(cmd, path=nil) + - 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_7_7 + - + - 7 + - | + def popen(cmd, path=nil) + - 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_7_7 +- - + - 8 + - | + unless cmd.is_a?(Array) + - 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_8_8 + - + - 8 + - | + unless cmd.is_a?(Array) + - 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_8_8 +- - old + - 9 + - | + - raise "System commands must be given as an array of strings" + - 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_9_9 + - new + - 9 + - | + + raise RuntimeError, "System commands must be given as an array of strings" + - 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_10_9 +- - + - 10 + - | + end + - 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_10_10 + - + - 10 + - | + end + - 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_10_10 +- - + - 11 + - | + + - 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_11_11 + - + - 11 + - | + + - 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_11_11 +- - + - 12 + - | + path ||= Dir.pwd + - 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_12_12 + - + - 12 + - | + path ||= Dir.pwd + - 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_12_12 +- - old + - 13 + - | + - vars = { "PWD" => path } + - 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_13_13 + - old + - + - " " + - +- - old + - 14 + - | + - options = { chdir: path } + - 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_14_13 + - new + - 13 + - | + + + - 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_15_13 +- - + - + - " " + - 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_15_14 + - new + - 14 + - | + + vars = { + - 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_15_14 +- - + - + - " " + - 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_15_15 + - new + - 15 + - | + + "PWD" => path + - 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_15_15 +- - + - + - " " + - 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_15_16 + - new + - 16 + - | + + } + - 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_15_16 +- - + - + - " " + - 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_15_17 + - new + - 17 + - | + + + - 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_15_17 +- - + - + - " " + - 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_15_18 + - new + - 18 + - | + + options = { + - 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_15_18 +- - + - + - " " + - 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_15_19 + - new + - 19 + - | + + chdir: path + - 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_15_19 +- - + - + - " " + - 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_15_20 + - new + - 20 + - | + + } + - 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_15_20 +- - + - 15 + - | + + - 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_15_21 + - + - 21 + - | + + - 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_15_21 +- - + - 16 + - | + unless File.directory?(path) + - 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_16_22 + - + - 22 + - | + unless File.directory?(path) + - 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_16_22 +- - + - 17 + - | + FileUtils.mkdir_p(path) + - 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_17_23 + - + - 23 + - | + FileUtils.mkdir_p(path) + - 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_17_23 +- - match + - 19 + - | + @@ -19,6 +25,7 @@ module Popen + - 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_19_25 + - match + - 25 + - | + @@ -19,6 +25,7 @@ module Popen + - 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_19_25 +- - + - 19 + - | + + - 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_19_25 + - + - 25 + - | + + - 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_19_25 +- - + - 20 + - | + @cmd_output = "" + - 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_20_26 + - + - 26 + - | + @cmd_output = "" + - 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_20_26 +- - + - 21 + - | + @cmd_status = 0 + - 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_21_27 + - + - 27 + - | + @cmd_status = 0 + - 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_21_27 +- - + - + - " " + - 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_22_28 + - new + - 28 + - | + + + - 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_22_28 +- - + - 22 + - | + Open3.popen3(vars, *cmd, options) do |stdin, stdout, stderr, wait_thr| + - 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_22_29 + - + - 29 + - | + Open3.popen3(vars, *cmd, options) do |stdin, stdout, stderr, wait_thr| + - 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_22_29 +- - + - 23 + - | + @cmd_output << stdout.read + - 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_23_30 + - + - 30 + - | + @cmd_output << stdout.read + - 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_23_30 +- - + - 24 + - @cmd_output << stderr.read + - 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_24_31 + - + - 31 + - @cmd_output << stderr.read + - 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_24_31 diff --git a/spec/helpers/diff_helper_spec.rb b/spec/helpers/diff_helper_spec.rb index 7c96a74e581..0501c2e8c29 100644 --- a/spec/helpers/diff_helper_spec.rb +++ b/spec/helpers/diff_helper_spec.rb @@ -131,34 +131,6 @@ describe DiffHelper do end def parallel_diff_result_array - [ - ["match", 6, "@@ -6,12 +6,18 @@ module Popen", "2f6fcd96b88b36ce98c38da085c795a27d92a3dd_6_6", "match", 6, "@@ -6,12 +6,18 @@ module Popen", "2f6fcd96b88b36ce98c38da085c795a27d92a3dd_6_6"], - [nil, 6, " ", "2f6fcd96b88b36ce98c38da085c795a27d92a3dd_6_6", nil, 6, " ", "2f6fcd96b88b36ce98c38da085c795a27d92a3dd_6_6"], [nil, 7, " def popen(cmd, path=nil)", "2f6fcd96b88b36ce98c38da085c795a27d92a3dd_7_7", nil, 7, " def popen(cmd, path=nil)", "2f6fcd96b88b36ce98c38da085c795a27d92a3dd_7_7"], - [nil, 8, " unless cmd.is_a?(Array)", "2f6fcd96b88b36ce98c38da085c795a27d92a3dd_8_8", nil, 8, " unless cmd.is_a?(Array)", "2f6fcd96b88b36ce98c38da085c795a27d92a3dd_8_8"], - ["old", 9, "- raise "System commands must be given as an array of strings"", "2f6fcd96b88b36ce98c38da085c795a27d92a3dd_9_9", "new", 9, "+ raise RuntimeError, "System commands must be given as an array of strings"", "2f6fcd96b88b36ce98c38da085c795a27d92a3dd_10_9"], - [nil, 10, " end", "2f6fcd96b88b36ce98c38da085c795a27d92a3dd_10_10", nil, 10, " end", "2f6fcd96b88b36ce98c38da085c795a27d92a3dd_10_10"], - [nil, 11, " ", "2f6fcd96b88b36ce98c38da085c795a27d92a3dd_11_11", nil, 11, " ", "2f6fcd96b88b36ce98c38da085c795a27d92a3dd_11_11"], - [nil, 12, " path ||= Dir.pwd", "2f6fcd96b88b36ce98c38da085c795a27d92a3dd_12_12", nil, 12, " path ||= Dir.pwd", "2f6fcd96b88b36ce98c38da085c795a27d92a3dd_12_12"], - ["old", 13, "- vars = { "PWD" => path }", "2f6fcd96b88b36ce98c38da085c795a27d92a3dd_13_13", "old", nil, " ", nil], - ["old", 14, "- options = { chdir: path }", "2f6fcd96b88b36ce98c38da085c795a27d92a3dd_14_13", "new", 13, "+", "2f6fcd96b88b36ce98c38da085c795a27d92a3dd_15_13"], - [nil, nil, " ", "2f6fcd96b88b36ce98c38da085c795a27d92a3dd_15_14", "new", 14, "+ vars = {", "2f6fcd96b88b36ce98c38da085c795a27d92a3dd_15_14"], - [nil, nil, " ", "2f6fcd96b88b36ce98c38da085c795a27d92a3dd_15_15", "new", 15, "+ "PWD" => path", "2f6fcd96b88b36ce98c38da085c795a27d92a3dd_15_15"], - [nil, nil, " ", "2f6fcd96b88b36ce98c38da085c795a27d92a3dd_15_16", "new", 16, "+ }", "2f6fcd96b88b36ce98c38da085c795a27d92a3dd_15_16"], - [nil, nil, " ", "2f6fcd96b88b36ce98c38da085c795a27d92a3dd_15_17", "new", 17, "+", "2f6fcd96b88b36ce98c38da085c795a27d92a3dd_15_17"], - [nil, nil, " ", "2f6fcd96b88b36ce98c38da085c795a27d92a3dd_15_18", "new", 18, "+ options = {", "2f6fcd96b88b36ce98c38da085c795a27d92a3dd_15_18"], - [nil, nil, " ", "2f6fcd96b88b36ce98c38da085c795a27d92a3dd_15_19", "new", 19, "+ chdir: path", "2f6fcd96b88b36ce98c38da085c795a27d92a3dd_15_19"], - [nil, nil, " ", "2f6fcd96b88b36ce98c38da085c795a27d92a3dd_15_20", "new", 20, "+ }", "2f6fcd96b88b36ce98c38da085c795a27d92a3dd_15_20"], - [nil, 15, " ", "2f6fcd96b88b36ce98c38da085c795a27d92a3dd_15_21", nil, 21, " ", "2f6fcd96b88b36ce98c38da085c795a27d92a3dd_15_21"], - [nil, 16, " unless File.directory?(path)", "2f6fcd96b88b36ce98c38da085c795a27d92a3dd_16_22", nil, 22, " unless File.directory?(path)", "2f6fcd96b88b36ce98c38da085c795a27d92a3dd_16_22"], - [nil, 17, " FileUtils.mkdir_p(path)", "2f6fcd96b88b36ce98c38da085c795a27d92a3dd_17_23", nil, 23, " FileUtils.mkdir_p(path)", "2f6fcd96b88b36ce98c38da085c795a27d92a3dd_17_23"], - ["match", 19, "@@ -19,6 +25,7 @@ module Popen", "2f6fcd96b88b36ce98c38da085c795a27d92a3dd_19_25", "match", 25, "@@ -19,6 +25,7 @@ module Popen", "2f6fcd96b88b36ce98c38da085c795a27d92a3dd_19_25"], - [nil, 19, " ", "2f6fcd96b88b36ce98c38da085c795a27d92a3dd_19_25", nil, 25, " ", "2f6fcd96b88b36ce98c38da085c795a27d92a3dd_19_25"], - [nil, 20, " @cmd_output = """, "2f6fcd96b88b36ce98c38da085c795a27d92a3dd_20_26", nil, 26, " @cmd_output = """, "2f6fcd96b88b36ce98c38da085c795a27d92a3dd_20_26"], - [nil, 21, " @cmd_status = 0", "2f6fcd96b88b36ce98c38da085c795a27d92a3dd_21_27", nil, 27, " @cmd_status = 0", "2f6fcd96b88b36ce98c38da085c795a27d92a3dd_21_27"], - [nil, nil, " ", "2f6fcd96b88b36ce98c38da085c795a27d92a3dd_22_28", "new", 28, "+", "2f6fcd96b88b36ce98c38da085c795a27d92a3dd_22_28"], - [nil, 22, " Open3.popen3(vars, *cmd, options) do |stdin, stdout, stderr, wait_thr|", "2f6fcd96b88b36ce98c38da085c795a27d92a3dd_22_29", nil, 29, " Open3.popen3(vars, *cmd, options) do |stdin, stdout, stderr, wait_thr|", "2f6fcd96b88b36ce98c38da085c795a27d92a3dd_22_29"], - [nil, 23, " @cmd_output << stdout.read", "2f6fcd96b88b36ce98c38da085c795a27d92a3dd_23_30", nil, 30, " @cmd_output << stdout.read", "2f6fcd96b88b36ce98c38da085c795a27d92a3dd_23_30"], - [nil, 24, " @cmd_output << stderr.read", "2f6fcd96b88b36ce98c38da085c795a27d92a3dd_24_31", nil, 31, " @cmd_output << stderr.read", "2f6fcd96b88b36ce98c38da085c795a27d92a3dd_24_31"] - ] + YAML.load_file("#{Rails.root}/spec/fixtures/parallel_diff_result.yml") end end From d83275620a0eeaf17a2958e59e2b4d6f217c6464 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rub=C3=A9n=20D=C3=A1vila?= Date: Wed, 30 Dec 2015 20:18:40 -0500 Subject: [PATCH 06/57] Add specs for Gitlab::Diff::Highlight. #3945 --- spec/lib/gitlab/diff/highlight_spec.rb | 28 ++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) create mode 100644 spec/lib/gitlab/diff/highlight_spec.rb diff --git a/spec/lib/gitlab/diff/highlight_spec.rb b/spec/lib/gitlab/diff/highlight_spec.rb new file mode 100644 index 00000000000..2a827a08dba --- /dev/null +++ b/spec/lib/gitlab/diff/highlight_spec.rb @@ -0,0 +1,28 @@ +require 'spec_helper' + +describe Gitlab::Diff::Highlight, lib: true do + include RepoHelpers + + let(:project) { create(:project) } + let(:commit) { project.commit(sample_commit.id) } + let(:diff) { commit.diffs.first } + let(:diff_file) { Gitlab::Diff::File.new(diff) } + + describe '.process_diff_lines' do + let(:diff_lines) { Gitlab::Diff::Highlight.process_diff_lines(diff_file) } + + it 'should return Gitlab::Diff::Line elements' do + expect(diff_lines.first).to be_an_instance_of(Gitlab::Diff::Line) + end + + it 'should highlight the code' do + code = %Q{ def popen(cmd, path=nil)\n} + + expect(diff_lines[2].text).to eq(code) + end + + it 'should keep the inline diff markup' do + expect(diff_lines[5].text).to match(Regexp.new(Regexp.escape('RuntimeError, '))) + end + end +end From 8b079315d98a8ccf852592148632c6f052d9cb55 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rub=C3=A9n=20D=C3=A1vila?= Date: Wed, 30 Dec 2015 21:23:50 -0500 Subject: [PATCH 07/57] A bit of refactoring. #3945 --- lib/gitlab/diff/file.rb | 2 +- lib/gitlab/diff/highlight.rb | 16 ++++++++-------- lib/rouge/lexers/gitlab_diff.rb | 2 ++ spec/lib/gitlab/diff/highlight_spec.rb | 2 +- 4 files changed, 12 insertions(+), 10 deletions(-) diff --git a/lib/gitlab/diff/file.rb b/lib/gitlab/diff/file.rb index ff8765b8e26..69b38a32eeb 100644 --- a/lib/gitlab/diff/file.rb +++ b/lib/gitlab/diff/file.rb @@ -16,7 +16,7 @@ module Gitlab end def highlighted_diff_lines - Gitlab::Diff::Highlight.process_diff_lines(self) + Gitlab::Diff::Highlight.process_diff_lines(new_path, diff_lines) end def mode_changed? diff --git a/lib/gitlab/diff/highlight.rb b/lib/gitlab/diff/highlight.rb index d0c2e3670c6..40a54ede2bb 100644 --- a/lib/gitlab/diff/highlight.rb +++ b/lib/gitlab/diff/highlight.rb @@ -1,15 +1,15 @@ module Gitlab module Diff class Highlight - def self.process_diff_lines(diff_file) - processor = new(diff_file) + def self.process_diff_lines(file_name, diff_lines) + processor = new(file_name, diff_lines) processor.highlight end - def initialize(diff_file) - text_lines = diff_file.diff_lines.map(&:text) - @diff_file = diff_file - @diff_lines = diff_file.diff_lines + def initialize(file_name, diff_lines) + text_lines = diff_lines.map(&:text) + @file_name = file_name + @diff_lines = diff_lines @diff_line_prefixes = text_lines.map { |line| line.sub!(/\A((\+|\-)\s*)/, '');$1 } @raw_lines = text_lines.join("\n") end @@ -32,7 +32,7 @@ module Gitlab end def lexer - parent = Rouge::Lexer.guess(filename: @diff_file.new_path, source: @code).new rescue Rouge::Lexers::PlainText.new + parent = Rouge::Lexer.guess(filename: @file_name, source: @code).new rescue Rouge::Lexers::PlainText.new Rouge::Lexers::GitlabDiff.new(parent_lexer: parent) end @@ -43,7 +43,7 @@ module Gitlab end def formatter - @formatter ||= Rouge::Formatters::HTMLGitlab.new( + Rouge::Formatters::HTMLGitlab.new( nowrap: true, cssclass: 'code highlight', lineanchors: true, diff --git a/lib/rouge/lexers/gitlab_diff.rb b/lib/rouge/lexers/gitlab_diff.rb index d91dd6c4245..cbf272ee1de 100644 --- a/lib/rouge/lexers/gitlab_diff.rb +++ b/lib/rouge/lexers/gitlab_diff.rb @@ -2,6 +2,8 @@ Rouge::Token::Tokens.token(:InlineDiff, 'idiff') module Rouge module Lexers + # This new Lexer is required in order to avoid the inline diff markup + # to be tokenized, it will be rendered as raw HTML code if that happens. class GitlabDiff < RegexLexer title "GitLab Diff" tag 'gitlab_diff' diff --git a/spec/lib/gitlab/diff/highlight_spec.rb b/spec/lib/gitlab/diff/highlight_spec.rb index 2a827a08dba..80083c15cff 100644 --- a/spec/lib/gitlab/diff/highlight_spec.rb +++ b/spec/lib/gitlab/diff/highlight_spec.rb @@ -9,7 +9,7 @@ describe Gitlab::Diff::Highlight, lib: true do let(:diff_file) { Gitlab::Diff::File.new(diff) } describe '.process_diff_lines' do - let(:diff_lines) { Gitlab::Diff::Highlight.process_diff_lines(diff_file) } + let(:diff_lines) { Gitlab::Diff::Highlight.process_diff_lines(diff_file.new_path, diff_file.diff_lines) } it 'should return Gitlab::Diff::Line elements' do expect(diff_lines.first).to be_an_instance_of(Gitlab::Diff::Line) From fd100e1ef1726418c81ab8833cf8bcf86fab6eef Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rub=C3=A9n=20D=C3=A1vila?= Date: Wed, 30 Dec 2015 21:44:12 -0500 Subject: [PATCH 08/57] Don't modify "match" diff lines. #3945 --- lib/gitlab/diff/highlight.rb | 7 ++++++- spec/lib/gitlab/diff/highlight_spec.rb | 5 +++++ 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/lib/gitlab/diff/highlight.rb b/lib/gitlab/diff/highlight.rb index 40a54ede2bb..c780ea21775 100644 --- a/lib/gitlab/diff/highlight.rb +++ b/lib/gitlab/diff/highlight.rb @@ -25,7 +25,12 @@ module Gitlab def update_diff_lines @highlighted_code.lines.each_with_index do |line, i| - @diff_lines[i].text = "#{@diff_line_prefixes[i]}#{line}" + diff_line = @diff_lines[i] + + # ignore highlighting for "match" lines + next if diff_line.type == 'match' + + diff_line.text = "#{@diff_line_prefixes[i]}#{line}" end @diff_lines diff --git a/spec/lib/gitlab/diff/highlight_spec.rb b/spec/lib/gitlab/diff/highlight_spec.rb index 80083c15cff..54621f773d7 100644 --- a/spec/lib/gitlab/diff/highlight_spec.rb +++ b/spec/lib/gitlab/diff/highlight_spec.rb @@ -24,5 +24,10 @@ describe Gitlab::Diff::Highlight, lib: true do it 'should keep the inline diff markup' do expect(diff_lines[5].text).to match(Regexp.new(Regexp.escape('RuntimeError, '))) 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 end end From 3fbcf52ec8decc3a4e331d52b2f47d7b85d399cf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rub=C3=A9n=20D=C3=A1vila?= Date: Thu, 31 Dec 2015 01:05:52 -0500 Subject: [PATCH 09/57] Apply syntax highlighting when expanding diff plus some refactor. #3945 --- app/controllers/projects/blob_controller.rb | 2 +- app/views/projects/blob/diff.html.haml | 2 +- lib/gitlab/diff/highlight.rb | 55 ++++++++++++++++----- spec/lib/gitlab/diff/highlight_spec.rb | 44 +++++++++++------ 4 files changed, 75 insertions(+), 28 deletions(-) diff --git a/app/controllers/projects/blob_controller.rb b/app/controllers/projects/blob_controller.rb index c56a3497bb2..d22c7b550b0 100644 --- a/app/controllers/projects/blob_controller.rb +++ b/app/controllers/projects/blob_controller.rb @@ -66,7 +66,7 @@ class Projects::BlobController < Projects::ApplicationController def diff @form = UnfoldForm.new(params) - @lines = @blob.data.lines[@form.since - 1..@form.to - 1] + @lines = Gitlab::Diff::Highlight.process_diff_lines(@blob.name, @blob.data.lines[@form.since - 1..@form.to - 1]) if @form.bottom? @match_line = '' diff --git a/app/views/projects/blob/diff.html.haml b/app/views/projects/blob/diff.html.haml index f3b01ff3288..9d8f6ecb3ac 100644 --- a/app/views/projects/blob/diff.html.haml +++ b/app/views/projects/blob/diff.html.haml @@ -11,7 +11,7 @@ %td.old_line.diff-line-num{data: {linenumber: line_old}} = link_to raw(line_old), "#" %td.new_line= link_to raw(line_new) , "#" - %td.line_content.noteable_line= ' ' * @form.indent + line + %td.line_content.noteable_line= raw("#{' ' * @form.indent}#{line}") - if @form.unfold? && @form.bottom? && @form.to < @blob.loc %tr.line_holder{ id: @form.to } diff --git a/lib/gitlab/diff/highlight.rb b/lib/gitlab/diff/highlight.rb index c780ea21775..7f340de65cc 100644 --- a/lib/gitlab/diff/highlight.rb +++ b/lib/gitlab/diff/highlight.rb @@ -1,31 +1,62 @@ module Gitlab module Diff class Highlight - def self.process_diff_lines(file_name, diff_lines) - processor = new(file_name, diff_lines) + # Apply syntax highlight to provided source code + # + # file_name - The file name related to the code. + # lines - It can be an Array of Gitlab::Diff::Line objects or simple Strings. + # When passing Strings you need to provide the required 'end of lines' + # chars ("\n") for each String given that we don't append them automatically. + # + # Returns an Array with the processed items. + def self.process_diff_lines(file_name, lines) + processor = new(file_name, lines) processor.highlight end - def initialize(file_name, diff_lines) - text_lines = diff_lines.map(&:text) - @file_name = file_name - @diff_lines = diff_lines - @diff_line_prefixes = text_lines.map { |line| line.sub!(/\A((\+|\-)\s*)/, '');$1 } - @raw_lines = text_lines.join("\n") + def initialize(file_name, lines) + @file_name = file_name + @lines = lines end def highlight - @code = unescape_html(@raw_lines) + return [] if @lines.empty? + + extract_line_prefixes + + @code = unescape_html(raw_content) @highlighted_code = formatter.format(lexer.lex(@code)) - update_diff_lines + is_diff_line? ? update_diff_lines : @highlighted_code.lines end private + def is_diff_line? + @lines.first.is_a?(Gitlab::Diff::Line) + end + + def text_lines + @text_lines ||= (is_diff_line? ? @lines.map(&:text) : @lines) + end + + def raw_content + @raw_content ||= text_lines.join(is_diff_line? ? "\n" : nil) + end + + def extract_line_prefixes + @diff_line_prefixes ||= begin + if is_diff_line? + text_lines.map { |line| line.sub!(/\A((\+|\-)\s*)/, '');$1 } + else + [] + end + end + end + def update_diff_lines @highlighted_code.lines.each_with_index do |line, i| - diff_line = @diff_lines[i] + diff_line = @lines[i] # ignore highlighting for "match" lines next if diff_line.type == 'match' @@ -33,7 +64,7 @@ module Gitlab diff_line.text = "#{@diff_line_prefixes[i]}#{line}" end - @diff_lines + @lines end def lexer diff --git a/spec/lib/gitlab/diff/highlight_spec.rb b/spec/lib/gitlab/diff/highlight_spec.rb index 54621f773d7..fc5cb894d2a 100644 --- a/spec/lib/gitlab/diff/highlight_spec.rb +++ b/spec/lib/gitlab/diff/highlight_spec.rb @@ -9,25 +9,41 @@ describe Gitlab::Diff::Highlight, lib: true do let(:diff_file) { Gitlab::Diff::File.new(diff) } describe '.process_diff_lines' do - let(:diff_lines) { Gitlab::Diff::Highlight.process_diff_lines(diff_file.new_path, diff_file.diff_lines) } + context 'when processing Gitlab::Diff::Line objects' do + let(:diff_lines) { Gitlab::Diff::Highlight.process_diff_lines(diff_file.new_path, diff_file.diff_lines) } - it 'should return Gitlab::Diff::Line elements' do - expect(diff_lines.first).to be_an_instance_of(Gitlab::Diff::Line) + it 'should return Gitlab::Diff::Line elements' do + expect(diff_lines.first).to be_an_instance_of(Gitlab::Diff::Line) + end + + it 'should highlight the code' do + code = %Q{ def popen(cmd, path=nil)\n} + + expect(diff_lines[2].text).to eq(code) + end + + it 'should keep the inline diff markup' do + expect(diff_lines[5].text).to match(Regexp.new(Regexp.escape('RuntimeError, '))) + 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 end - it 'should highlight the code' do - code = %Q{ def popen(cmd, path=nil)\n} + context 'when processing raw lines' do + let(:lines) { ["puts 'Hello'\n", "# A comment"] } + let(:highlighted_lines) { Gitlab::Diff::Highlight.process_diff_lines('demo.rb', lines) } - expect(diff_lines[2].text).to eq(code) + it 'should highlight the code' do + line_1 = %Q{puts 'Hello'\n} + line_2 = %Q{# A comment} + + expect(highlighted_lines[0]).to eq(line_1) + expect(highlighted_lines[1]).to eq(line_2) + end end - it 'should keep the inline diff markup' do - expect(diff_lines[5].text).to match(Regexp.new(Regexp.escape('RuntimeError, '))) - 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 end end From 795ecb498c74d7fbc2db461b3d37ff10b350a3c6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rub=C3=A9n=20D=C3=A1vila?= Date: Thu, 31 Dec 2015 01:46:52 -0500 Subject: [PATCH 10/57] Fix broken spec. #3945 --- spec/fixtures/parallel_diff_result.yml | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/spec/fixtures/parallel_diff_result.yml b/spec/fixtures/parallel_diff_result.yml index abbb364f61f..16c4bac96df 100644 --- a/spec/fixtures/parallel_diff_result.yml +++ b/spec/fixtures/parallel_diff_result.yml @@ -1,13 +1,11 @@ --- - - match - 6 - - | - @@ -6,12 +6,18 @@ module Popen + - '@@ -6,12 +6,18 @@ module Popen' - 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_6_6 - match - 6 - - | - @@ -6,12 +6,18 @@ module Popen + - '@@ -6,12 +6,18 @@ module Popen' - 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_6_6 - - - 6 @@ -193,13 +191,11 @@ - 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_17_23 - - match - 19 - - | - @@ -19,6 +25,7 @@ module Popen + - '@@ -19,6 +25,7 @@ module Popen' - 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_19_25 - match - 25 - - | - @@ -19,6 +25,7 @@ module Popen + - '@@ -19,6 +25,7 @@ module Popen' - 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_19_25 - - - 19 From 776d70d11b822e5991d518e91b138853275ef42c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rub=C3=A9n=20D=C3=A1vila?= Date: Wed, 6 Jan 2016 19:54:36 -0500 Subject: [PATCH 11/57] Use #html_safe instead of #raw in some diff views. #3945 --- app/views/projects/blob/diff.html.haml | 2 +- app/views/projects/diffs/_parallel_view.html.haml | 4 ++-- app/views/projects/diffs/_text_file.html.haml | 4 ++-- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/app/views/projects/blob/diff.html.haml b/app/views/projects/blob/diff.html.haml index 9d8f6ecb3ac..ef23876677a 100644 --- a/app/views/projects/blob/diff.html.haml +++ b/app/views/projects/blob/diff.html.haml @@ -11,7 +11,7 @@ %td.old_line.diff-line-num{data: {linenumber: line_old}} = link_to raw(line_old), "#" %td.new_line= link_to raw(line_new) , "#" - %td.line_content.noteable_line= raw("#{' ' * @form.indent}#{line}") + %td.line_content.noteable_line= "#{' ' * @form.indent}#{line}".html_safe - if @form.unfold? && @form.bottom? && @form.to < @blob.loc %tr.line_holder{ id: @form.to } diff --git a/app/views/projects/diffs/_parallel_view.html.haml b/app/views/projects/diffs/_parallel_view.html.haml index 4ded4d2daad..6a7d4f2db20 100644 --- a/app/views/projects/diffs/_parallel_view.html.haml +++ b/app/views/projects/diffs/_parallel_view.html.haml @@ -20,7 +20,7 @@ = link_to raw(line_number_left), "##{line_code_left}", id: line_code_left - if @comments_allowed && can?(current_user, :create_note, @project) = link_to_new_diff_note(line_code_left, 'old') - %td.line_content{class: "parallel noteable_line #{type_left} #{line_code_left}", "line_code" => line_code_left }= raw(line_content_left) + %td.line_content{class: "parallel noteable_line #{type_left} #{line_code_left}", "line_code" => line_code_left }= line_content_left.html_safe - if type_right == 'new' - new_line_class = 'new' @@ -33,7 +33,7 @@ = link_to raw(line_number_right), "##{new_line_code}", id: new_line_code - if @comments_allowed && can?(current_user, :create_note, @project) = link_to_new_diff_note(line_code_right, 'new') - %td.line_content.parallel{class: "noteable_line #{new_line_class} #{new_line_code}", "line_code" => new_line_code}= raw(line_content_right) + %td.line_content.parallel{class: "noteable_line #{new_line_class} #{new_line_code}", "line_code" => new_line_code}= line_content_right.html_safe - if @reply_allowed - comments_left, comments_right = organize_comments(type_left, type_right, line_code_left, line_code_right) diff --git a/app/views/projects/diffs/_text_file.html.haml b/app/views/projects/diffs/_text_file.html.haml index 059d0baf45e..5509d5403b0 100644 --- a/app/views/projects/diffs/_text_file.html.haml +++ b/app/views/projects/diffs/_text_file.html.haml @@ -21,8 +21,8 @@ - if @comments_allowed && can?(current_user, :create_note, @project) = link_to_new_diff_note(line_code) %td.new_line{data: {linenumber: line.new_pos}} - = link_to raw(type == "old" ? " " : line.new_pos) , "##{line_code}", id: line_code - %td.line_content{class: "noteable_line #{type} #{line_code}", "line_code" => line_code}= raw(diff_line_content(line.text)) + = link_to raw(type == "old" ? " " : line.new_pos), "##{line_code}", id: line_code + %td.line_content{class: "noteable_line #{type} #{line_code}", "line_code" => line_code}= diff_line_content(line.text).html_safe - if @reply_allowed - comments = @line_notes.select { |n| n.line_code == line_code && n.active? }.sort_by(&:created_at) From 1494bb3f253b2e37065080c27af22ec531966ab7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rub=C3=A9n=20D=C3=A1vila?= Date: Wed, 6 Jan 2016 21:01:44 -0500 Subject: [PATCH 12/57] Force white theme when viewing diffs. #3945 --- app/views/projects/diffs/_parallel_view.html.haml | 2 +- app/views/projects/diffs/_text_file.html.haml | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/app/views/projects/diffs/_parallel_view.html.haml b/app/views/projects/diffs/_parallel_view.html.haml index 6a7d4f2db20..2a43bbd11f2 100644 --- a/app/views/projects/diffs/_parallel_view.html.haml +++ b/app/views/projects/diffs/_parallel_view.html.haml @@ -1,5 +1,5 @@ / Side-by-side diff view -%div.text-file.diff-wrap-lines.code.file-content.js-syntax-highlight{ class: user_color_scheme } +%div.text-file.diff-wrap-lines.code.file-content.js-syntax-highlight.white %table - parallel_diff(diff_file, index).each do |line| - type_left = line[0] diff --git a/app/views/projects/diffs/_text_file.html.haml b/app/views/projects/diffs/_text_file.html.haml index 5509d5403b0..8e86219155f 100644 --- a/app/views/projects/diffs/_text_file.html.haml +++ b/app/views/projects/diffs/_text_file.html.haml @@ -3,7 +3,7 @@ .suppressed-container %a.show-suppressed-diff.js-show-suppressed-diff Changes suppressed. Click to show. -%table.text-file.code.js-syntax-highlight{ class: [user_color_scheme, too_big ? 'hide' : ''] } +%table.text-file.code.js-syntax-highlight.white{ class: too_big ? 'hide' : '' } - last_line = 0 - diff_file.highlighted_diff_lines.each_with_index do |line, index| From 21958a393955037318141d39fbe14b1c2e842cab Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rub=C3=A9n=20D=C3=A1vila?= Date: Thu, 7 Jan 2016 13:45:19 -0500 Subject: [PATCH 13/57] Add some styling for syntax highlighting themes. #3945 --- app/assets/stylesheets/highlight/dark.scss | 14 ++++++++++++++ app/assets/stylesheets/highlight/monokai.scss | 18 ++++++++++++++++++ .../stylesheets/highlight/solarized_dark.scss | 14 ++++++++++++++ .../stylesheets/highlight/solarized_light.scss | 15 +++++++++++++++ app/assets/stylesheets/pages/diff.scss | 15 +++++++++++++++ .../projects/diffs/_parallel_view.html.haml | 2 +- app/views/projects/diffs/_text_file.html.haml | 2 +- 7 files changed, 78 insertions(+), 2 deletions(-) diff --git a/app/assets/stylesheets/highlight/dark.scss b/app/assets/stylesheets/highlight/dark.scss index 6a2b25ddc67..f7d1334705f 100644 --- a/app/assets/stylesheets/highlight/dark.scss +++ b/app/assets/stylesheets/highlight/dark.scss @@ -90,4 +90,18 @@ .vg { color: #cc6666 } /* Name.Variable.Global */ .vi { color: #cc6666 } /* Name.Variable.Instance */ .il { color: #de935f } /* Literal.Number.Integer.Long */ + + .line_holder { + &.new .old_line, + &.new .new_line, + &.new .line_content { + @include diff_background(255, 255, 255, #808080); + } + + &.old .old_line, + &.old .new_line, + &.old .line_content { + @include diff_background(255, 51, 51, #808080); + } + } } diff --git a/app/assets/stylesheets/highlight/monokai.scss b/app/assets/stylesheets/highlight/monokai.scss index 8560c3c490f..cc03ed6ae45 100644 --- a/app/assets/stylesheets/highlight/monokai.scss +++ b/app/assets/stylesheets/highlight/monokai.scss @@ -90,4 +90,22 @@ .gu { color: #75715e; } /* Generic.Subheading & Diff Unified/Comment? */ .gd { color: #f92672; } /* Generic.Deleted & Diff Deleted */ .gi { color: #a6e22e; } /* Generic.Inserted & Diff Inserted */ + + .line_holder { + &.parallel .new.new_line, + &.parallel .new.line_content, + &.new .old_line, + &.new .new_line, + &.new .line_content { + @include diff_background(156, 175, 183, #808080); + } + + &.parallel .old.old_line, + &.parallel .old.line_content, + &.old .old_line, + &.old .new_line, + &.old .line_content { + @include diff_background(254, 147, 140, #808080); + } + } } diff --git a/app/assets/stylesheets/highlight/solarized_dark.scss b/app/assets/stylesheets/highlight/solarized_dark.scss index 7d489a9666b..2c3648274cf 100644 --- a/app/assets/stylesheets/highlight/solarized_dark.scss +++ b/app/assets/stylesheets/highlight/solarized_dark.scss @@ -111,4 +111,18 @@ .vg { color: #268bd2 } /* Name.Variable.Global */ .vi { color: #268bd2 } /* Name.Variable.Instance */ .il { color: #2aa198 } /* Literal.Number.Integer.Long */ + + .line_holder { + &.new .old_line, + &.new .new_line, + &.new .line_content { + @include diff_background(255, 255, 255, #808080); + } + + &.old .old_line, + &.old .new_line, + &.old .line_content { + @include diff_background(255, 51, 51, #808080); + } + } } diff --git a/app/assets/stylesheets/highlight/solarized_light.scss b/app/assets/stylesheets/highlight/solarized_light.scss index 200ed346446..c16473ffe66 100644 --- a/app/assets/stylesheets/highlight/solarized_light.scss +++ b/app/assets/stylesheets/highlight/solarized_light.scss @@ -111,4 +111,19 @@ .vg { color: #268bd2 } /* Name.Variable.Global */ .vi { color: #268bd2 } /* Name.Variable.Instance */ .il { color: #2aa198 } /* Literal.Number.Integer.Long */ + + + .line_holder { + &.new .old_line, + &.new .new_line, + &.new .line_content { + @include diff_background(92, 164, 169, #FAF3DD); + } + + &.old .old_line, + &.old .new_line, + &.old .line_content { + @include diff_background(237, 106, 90, #FAF3DD); + } + } } diff --git a/app/assets/stylesheets/pages/diff.scss b/app/assets/stylesheets/pages/diff.scss index afd6fb73675..caaad1e31d3 100644 --- a/app/assets/stylesheets/pages/diff.scss +++ b/app/assets/stylesheets/pages/diff.scss @@ -402,3 +402,18 @@ right: 15px; } } + +@mixin diff_background($r, $g, $b, $custom-border) { + /* Fallback for web browsers that doesn't support RGBa */ + background: rgb($r, $g, $b); + /* RGBa with 0.3 opacity */ + background: rgba($r, $g, $b, 0.3); + + &.new_line, &.old_line { + border-right-color: $custom-border !important; + } + + &.line_content span.idiff { + background: rgb($r, $g, $b); + } +} diff --git a/app/views/projects/diffs/_parallel_view.html.haml b/app/views/projects/diffs/_parallel_view.html.haml index 2a43bbd11f2..1ad54d1848b 100644 --- a/app/views/projects/diffs/_parallel_view.html.haml +++ b/app/views/projects/diffs/_parallel_view.html.haml @@ -1,5 +1,5 @@ / Side-by-side diff view -%div.text-file.diff-wrap-lines.code.file-content.js-syntax-highlight.white +%div.text-file.diff-wrap-lines.code.file-content.js-syntax-highlight %table - parallel_diff(diff_file, index).each do |line| - type_left = line[0] diff --git a/app/views/projects/diffs/_text_file.html.haml b/app/views/projects/diffs/_text_file.html.haml index 8e86219155f..f1b8cba5e55 100644 --- a/app/views/projects/diffs/_text_file.html.haml +++ b/app/views/projects/diffs/_text_file.html.haml @@ -3,7 +3,7 @@ .suppressed-container %a.show-suppressed-diff.js-show-suppressed-diff Changes suppressed. Click to show. -%table.text-file.code.js-syntax-highlight.white{ class: too_big ? 'hide' : '' } +%table.text-file.code.js-syntax-highlight{ class: too_big ? 'hide' : '' } - last_line = 0 - diff_file.highlighted_diff_lines.each_with_index do |line, index| From f1f4fdf778245cab74ff9cda2a421315c21a99aa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rub=C3=A9n=20D=C3=A1vila?= Date: Thu, 7 Jan 2016 21:08:57 -0500 Subject: [PATCH 14/57] Don't process inline diffs on backend. #3945 --- lib/gitlab/diff/parser.rb | 5 +- lib/gitlab/inline_diff.rb | 104 ---------------------------- spec/lib/gitlab/inline_diff_spec.rb | 39 ----------- 3 files changed, 1 insertion(+), 147 deletions(-) delete mode 100644 lib/gitlab/inline_diff.rb delete mode 100644 spec/lib/gitlab/inline_diff_spec.rb diff --git a/lib/gitlab/diff/parser.rb b/lib/gitlab/diff/parser.rb index 7015fe36c3d..177bad4b1cf 100644 --- a/lib/gitlab/diff/parser.rb +++ b/lib/gitlab/diff/parser.rb @@ -11,13 +11,10 @@ module Gitlab line_new = 1 type = nil - lines_arr = ::Gitlab::InlineDiff.processing lines - - lines_arr.each do |line| + @lines.each do |line| next if filename?(line) full_line = html_escape(line.gsub(/\n/, '')) - full_line = ::Gitlab::InlineDiff.replace_markers full_line if line.match(/^@@ -/) type = "match" diff --git a/lib/gitlab/inline_diff.rb b/lib/gitlab/inline_diff.rb deleted file mode 100644 index 44507bde25d..00000000000 --- a/lib/gitlab/inline_diff.rb +++ /dev/null @@ -1,104 +0,0 @@ -module Gitlab - class InlineDiff - class << self - - START = "#!idiff-start!#" - FINISH = "#!idiff-finish!#" - - def processing(diff_arr) - indexes = _indexes_of_changed_lines diff_arr - - indexes.each do |index| - first_line = diff_arr[index+1] - second_line = diff_arr[index+2] - - # Skip inline diff if empty line was replaced with content - next if first_line == "-\n" - - first_token = find_first_token(first_line, second_line) - apply_first_token(diff_arr, index, first_token) - - last_token = find_last_token(first_line, second_line, first_token) - apply_last_token(diff_arr, index, last_token) - end - - diff_arr - end - - def apply_first_token(diff_arr, index, first_token) - start = first_token + START - - if first_token.empty? - # In case if we remove string of spaces in commit - diff_arr[index+1].sub!("-", "-" => "-#{START}") - diff_arr[index+2].sub!("+", "+" => "+#{START}") - else - diff_arr[index+1].sub!(first_token, first_token => start) - diff_arr[index+2].sub!(first_token, first_token => start) - end - end - - def apply_last_token(diff_arr, index, last_token) - # This is tricky: escape backslashes so that `sub` doesn't interpret them - # as backreferences. Regexp.escape does NOT do the right thing. - replace_token = FINISH + last_token.gsub(/\\/, '\&\&') - diff_arr[index+1].sub!(/#{Regexp.escape(last_token)}$/, replace_token) - diff_arr[index+2].sub!(/#{Regexp.escape(last_token)}$/, replace_token) - end - - def find_first_token(first_line, second_line) - max_length = [first_line.size, second_line.size].max - first_the_same_symbols = 0 - - (0..max_length + 1).each do |i| - first_the_same_symbols = i - 1 - - if first_line[i] != second_line[i] && i > 0 - break - end - end - - first_line[0..first_the_same_symbols][1..-1] - end - - def find_last_token(first_line, second_line, first_token) - max_length = [first_line.size, second_line.size].max - last_the_same_symbols = 0 - - (1..max_length + 1).each do |i| - last_the_same_symbols = -i - shortest_line = second_line.size > first_line.size ? first_line : second_line - - if (first_line[-i] != second_line[-i]) || "#{first_token}#{START}".size == shortest_line[1..-i].size - break - end - end - - last_the_same_symbols += 1 - first_line[last_the_same_symbols..-1] - end - - def _indexes_of_changed_lines(diff_arr) - chain_of_first_symbols = "" - diff_arr.each_with_index do |line, i| - chain_of_first_symbols += line[0] - end - chain_of_first_symbols.gsub!(/[^\-\+]/, "#") - - offset = 0 - indexes = [] - while index = chain_of_first_symbols.index("#-+#", offset) - indexes << index - offset = index + 1 - end - indexes - end - - def replace_markers(line) - line.gsub!(START, "") - line.gsub!(FINISH, "") - line - end - end - end -end diff --git a/spec/lib/gitlab/inline_diff_spec.rb b/spec/lib/gitlab/inline_diff_spec.rb deleted file mode 100644 index c690c195112..00000000000 --- a/spec/lib/gitlab/inline_diff_spec.rb +++ /dev/null @@ -1,39 +0,0 @@ -require 'spec_helper' - -describe Gitlab::InlineDiff, lib: true do - describe '#processing' do - let(:diff) do - < Date: Thu, 7 Jan 2016 22:37:01 -0500 Subject: [PATCH 15/57] Change strategy to highlight diffs. #3945 Now we apply syntax highlighting to the whole old and new files. This basically help us to highlight adequately multiline content. --- app/controllers/application_controller.rb | 1 + app/controllers/projects/blob_controller.rb | 7 +- app/controllers/projects/commit_controller.rb | 1 + .../projects/compare_controller.rb | 1 + .../projects/merge_requests_controller.rb | 2 + app/helpers/diff_helper.rb | 4 +- app/views/projects/commit/show.html.haml | 3 +- app/views/projects/compare/show.html.haml | 2 +- app/views/projects/diffs/_diffs.html.haml | 2 +- .../merge_requests/_new_submit.html.haml | 2 +- .../merge_requests/show/_diffs.html.haml | 2 +- lib/gitlab/diff/file.rb | 8 ++- lib/gitlab/diff/highlight.rb | 70 ++++++++++++++----- 13 files changed, 76 insertions(+), 29 deletions(-) diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index d9a37a4d45f..d3c1ff035f5 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -24,6 +24,7 @@ class ApplicationController < ActionController::Base helper_method :abilities, :can?, :current_application_settings helper_method :import_sources_enabled?, :github_import_enabled?, :github_import_configured?, :gitlab_import_enabled?, :gitlab_import_configured?, :bitbucket_import_enabled?, :bitbucket_import_configured?, :gitorious_import_enabled?, :google_code_import_enabled?, :fogbugz_import_enabled?, :git_import_enabled? + helper_method :repository rescue_from Encoding::CompatibilityError do |exception| log_exception(exception) diff --git a/app/controllers/projects/blob_controller.rb b/app/controllers/projects/blob_controller.rb index d22c7b550b0..6aa602321f4 100644 --- a/app/controllers/projects/blob_controller.rb +++ b/app/controllers/projects/blob_controller.rb @@ -65,8 +65,11 @@ class Projects::BlobController < Projects::ApplicationController end def diff - @form = UnfoldForm.new(params) - @lines = Gitlab::Diff::Highlight.process_diff_lines(@blob.name, @blob.data.lines[@form.since - 1..@form.to - 1]) + ref, file_name = params[:id].split('/', 2) + + @form = UnfoldForm.new(params) + @lines = Gitlab::Diff::Highlight.process_file(repository, ref, file_name) + @lines = @lines[@form.since - 1..@form.to - 1] if @form.bottom? @match_line = '' diff --git a/app/controllers/projects/commit_controller.rb b/app/controllers/projects/commit_controller.rb index 0aaba3792bf..9bbf4581057 100644 --- a/app/controllers/projects/commit_controller.rb +++ b/app/controllers/projects/commit_controller.rb @@ -72,6 +72,7 @@ class Projects::CommitController < Projects::ApplicationController @diffs = commit.diffs end + @diff_refs = [commit.parent.id, commit.id] @notes_count = commit.notes.count @statuses = ci_commit.statuses if ci_commit diff --git a/app/controllers/projects/compare_controller.rb b/app/controllers/projects/compare_controller.rb index 5200d609cc9..5b88aed7a64 100644 --- a/app/controllers/projects/compare_controller.rb +++ b/app/controllers/projects/compare_controller.rb @@ -20,6 +20,7 @@ class Projects::CompareController < Projects::ApplicationController if compare_result @commits = Commit.decorate(compare_result.commits, @project) @diffs = compare_result.diffs + @diff_refs = [base_ref, head_ref] @commit = @project.commit(head_ref) @first_commit = @project.commit(base_ref) @line_notes = [] diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb index ab5c953189c..d25adbe952d 100644 --- a/app/controllers/projects/merge_requests_controller.rb +++ b/app/controllers/projects/merge_requests_controller.rb @@ -59,6 +59,7 @@ class Projects::MergeRequestsController < Projects::ApplicationController def diffs @commit = @merge_request.last_commit @first_commit = @merge_request.first_commit + @diff_refs = [@merge_request.target_sha, @merge_request.source_sha] @comments_allowed = @reply_allowed = true @comments_target = { @@ -103,6 +104,7 @@ class Projects::MergeRequestsController < Projects::ApplicationController @commit = @merge_request.last_commit @first_commit = @merge_request.first_commit @diffs = @merge_request.compare_diffs + @diff_refs = [@merge_request.target_sha, @merge_request.source_branch] @ci_commit = @merge_request.ci_commit @statuses = @ci_commit.statuses if @ci_commit diff --git a/app/helpers/diff_helper.rb b/app/helpers/diff_helper.rb index 22deb69cec5..668610364c5 100644 --- a/app/helpers/diff_helper.rb +++ b/app/helpers/diff_helper.rb @@ -19,13 +19,13 @@ module DiffHelper end end - def safe_diff_files(diffs) + def safe_diff_files(diffs, diff_refs, repository) lines = 0 safe_files = [] diffs.first(allowed_diff_size).each do |diff| lines += diff.diff.lines.count break if lines > allowed_diff_lines - safe_files << Gitlab::Diff::File.new(diff) + safe_files << Gitlab::Diff::File.new(diff, diff_refs, repository) end safe_files end diff --git a/app/views/projects/commit/show.html.haml b/app/views/projects/commit/show.html.haml index 069b8b1f169..16ebce2d771 100644 --- a/app/views/projects/commit/show.html.haml +++ b/app/views/projects/commit/show.html.haml @@ -5,5 +5,6 @@ = render "ci_menu" - else %div.block-connector -= render "projects/diffs/diffs", diffs: @diffs, project: @project += render "projects/diffs/diffs", diffs: @diffs, project: @project, + diff_refs: @diff_refs = render "projects/notes/notes_with_form" diff --git a/app/views/projects/compare/show.html.haml b/app/views/projects/compare/show.html.haml index 51088a7dea8..da731f28bb6 100644 --- a/app/views/projects/compare/show.html.haml +++ b/app/views/projects/compare/show.html.haml @@ -9,7 +9,7 @@ - if @commits.present? .prepend-top-default = render "projects/commits/commit_list" - = render "projects/diffs/diffs", diffs: @diffs, project: @project + = render "projects/diffs/diffs", diffs: @diffs, project: @project, diff_refs: @diff_refs - else .light-well.prepend-top-default .center diff --git a/app/views/projects/diffs/_diffs.html.haml b/app/views/projects/diffs/_diffs.html.haml index f9d661d59d2..58ad7d79af9 100644 --- a/app/views/projects/diffs/_diffs.html.haml +++ b/app/views/projects/diffs/_diffs.html.haml @@ -1,7 +1,7 @@ - if diff_view == 'parallel' - fluid_layout true -- diff_files = safe_diff_files(diffs) +- diff_files = safe_diff_files(diffs, diff_refs, repository) .gray-content-block.middle-block.oneline-block .inline-parallel-buttons diff --git a/app/views/projects/merge_requests/_new_submit.html.haml b/app/views/projects/merge_requests/_new_submit.html.haml index a14943b15d3..0cb8b6daedb 100644 --- a/app/views/projects/merge_requests/_new_submit.html.haml +++ b/app/views/projects/merge_requests/_new_submit.html.haml @@ -38,7 +38,7 @@ = render "projects/merge_requests/show/commits" #diffs.diffs.tab-pane.active - if @diffs.present? - = render "projects/diffs/diffs", diffs: @diffs, project: @project + = render "projects/diffs/diffs", diffs: @diffs, project: @project, diff_refs: @diff_refs - elsif @commits.size > MergeRequestDiff::COMMITS_SAFE_SIZE .alert.alert-danger %h4 This comparison includes more than #{MergeRequestDiff::COMMITS_SAFE_SIZE} commits. diff --git a/app/views/projects/merge_requests/show/_diffs.html.haml b/app/views/projects/merge_requests/show/_diffs.html.haml index d9cfc3d7ae9..1d9af1e4160 100644 --- a/app/views/projects/merge_requests/show/_diffs.html.haml +++ b/app/views/projects/merge_requests/show/_diffs.html.haml @@ -1,5 +1,5 @@ - if @merge_request_diff.collected? - = render "projects/diffs/diffs", diffs: params[:w] == '1' ? @merge_request.diffs_no_whitespace : @merge_request.diffs, project: @merge_request.project + = render "projects/diffs/diffs", diffs: params[:w] == '1' ? @merge_request.diffs_no_whitespace : @merge_request.diffs, project: @merge_request.project, diff_refs: @diff_refs - elsif @merge_request_diff.empty? .nothing-here-block Nothing to merge from #{@merge_request.source_branch} into #{@merge_request.target_branch} - else diff --git a/lib/gitlab/diff/file.rb b/lib/gitlab/diff/file.rb index 69b38a32eeb..cb93c6a574a 100644 --- a/lib/gitlab/diff/file.rb +++ b/lib/gitlab/diff/file.rb @@ -1,13 +1,15 @@ module Gitlab module Diff class File - attr_reader :diff + attr_reader :diff, :repository, :new_ref, :old_ref delegate :new_file, :deleted_file, :renamed_file, :old_path, :new_path, to: :diff, prefix: false - def initialize(diff) + def initialize(diff, diff_refs, repository) @diff = diff + @repository = repository + @old_ref, @new_ref = diff_refs end # Array of Gitlab::DIff::Line objects @@ -16,7 +18,7 @@ module Gitlab end def highlighted_diff_lines - Gitlab::Diff::Highlight.process_diff_lines(new_path, diff_lines) + Gitlab::Diff::Highlight.process_diff_lines(self) end def mode_changed? diff --git a/lib/gitlab/diff/highlight.rb b/lib/gitlab/diff/highlight.rb index 7f340de65cc..0d0a3268107 100644 --- a/lib/gitlab/diff/highlight.rb +++ b/lib/gitlab/diff/highlight.rb @@ -1,22 +1,43 @@ module Gitlab module Diff class Highlight + attr_reader :diff_file + + delegate :repository, :old_path, :new_path, :old_ref, :new_ref, + to: :diff_file, prefix: :diff + # Apply syntax highlight to provided source code # - # file_name - The file name related to the code. - # lines - It can be an Array of Gitlab::Diff::Line objects or simple Strings. - # When passing Strings you need to provide the required 'end of lines' - # chars ("\n") for each String given that we don't append them automatically. + # diff_file - an instance of Gitlab::Diff::File # # Returns an Array with the processed items. - def self.process_diff_lines(file_name, lines) - processor = new(file_name, lines) + def self.process_diff_lines(diff_file) + processor = new(diff_file) processor.highlight end - def initialize(file_name, lines) - @file_name = file_name - @lines = lines + def self.process_file(repository, ref, file_name) + blob = repository.blob_at(ref, file_name) + return [] unless blob + + content = blob.data + lexer = Rouge::Lexer.guess(filename: file_name, source: content).new rescue Rouge::Lexers::PlainText.new + formatter.format(lexer.lex(content)).lines + end + + def self.formatter + @formatter ||= Rouge::Formatters::HTMLGitlab.new( + nowrap: true, + cssclass: 'code highlight', + lineanchors: true, + lineanchorsid: 'LC' + ) + end + + def initialize(diff_file) + @diff_file = diff_file + @file_name = diff_file.new_path + @lines = diff_file.diff_lines end def highlight @@ -47,7 +68,7 @@ module Gitlab def extract_line_prefixes @diff_line_prefixes ||= begin if is_diff_line? - text_lines.map { |line| line.sub!(/\A((\+|\-)\s*)/, '');$1 } + text_lines.map { |line| line.sub!(/\A((\+|\-))/, '');$1 } else [] end @@ -57,11 +78,17 @@ module Gitlab def update_diff_lines @highlighted_code.lines.each_with_index do |line, i| diff_line = @lines[i] + line_prefix = @diff_line_prefixes[i] || ' ' # ignore highlighting for "match" lines next if diff_line.type == 'match' - diff_line.text = "#{@diff_line_prefixes[i]}#{line}" + case diff_line.type + when 'new', nil + diff_line.text = new_lines[diff_line.new_pos - 1].try(:gsub!, /\A\s/, line_prefix) + when 'old' + diff_line.text = old_lines[diff_line.old_pos - 1].try(:gsub!, /\A\s/, line_prefix) + end end @lines @@ -79,12 +106,21 @@ module Gitlab end def formatter - Rouge::Formatters::HTMLGitlab.new( - nowrap: true, - cssclass: 'code highlight', - lineanchors: true, - lineanchorsid: 'LC' - ) + self.class.formatter + end + + def old_lines + @old_lines ||= begin + lines = self.class.process_file(diff_repository, diff_old_ref, diff_old_path) + lines.map! { |line| " #{line}" } + end + end + + def new_lines + @new_lines ||= begin + lines = self.class.process_file(diff_repository, diff_new_ref, diff_new_path) + lines.map! { |line| " #{line}" } + end end end end From 6282202ee84f80f2197698b6f132abdf588e94d4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rub=C3=A9n=20D=C3=A1vila?= Date: Fri, 8 Jan 2016 15:17:45 -0500 Subject: [PATCH 16/57] Remove custom Lexer. #3945 [ci skip] Inline diff is going to be generated client side now. #3945 --- lib/gitlab/diff/highlight.rb | 3 +-- lib/rouge/lexers/gitlab_diff.rb | 26 -------------------------- 2 files changed, 1 insertion(+), 28 deletions(-) delete mode 100644 lib/rouge/lexers/gitlab_diff.rb diff --git a/lib/gitlab/diff/highlight.rb b/lib/gitlab/diff/highlight.rb index 0d0a3268107..7dd44b6004a 100644 --- a/lib/gitlab/diff/highlight.rb +++ b/lib/gitlab/diff/highlight.rb @@ -95,8 +95,7 @@ module Gitlab end def lexer - parent = Rouge::Lexer.guess(filename: @file_name, source: @code).new rescue Rouge::Lexers::PlainText.new - Rouge::Lexers::GitlabDiff.new(parent_lexer: parent) + Rouge::Lexer.guess(filename: @file_name, source: @code).new rescue Rouge::Lexers::PlainText.new end def unescape_html(content) diff --git a/lib/rouge/lexers/gitlab_diff.rb b/lib/rouge/lexers/gitlab_diff.rb deleted file mode 100644 index cbf272ee1de..00000000000 --- a/lib/rouge/lexers/gitlab_diff.rb +++ /dev/null @@ -1,26 +0,0 @@ -Rouge::Token::Tokens.token(:InlineDiff, 'idiff') - -module Rouge - module Lexers - # This new Lexer is required in order to avoid the inline diff markup - # to be tokenized, it will be rendered as raw HTML code if that happens. - class GitlabDiff < RegexLexer - title "GitLab Diff" - tag 'gitlab_diff' - - state :root do - rule %r{(.*?)} do |match| - token InlineDiff, match[1] - end - - rule /(?:(?! Date: Fri, 8 Jan 2016 18:40:05 -0500 Subject: [PATCH 17/57] Update specs. #3945 --- spec/lib/gitlab/diff/highlight_spec.rb | 31 +++++++++++++------------- 1 file changed, 15 insertions(+), 16 deletions(-) diff --git a/spec/lib/gitlab/diff/highlight_spec.rb b/spec/lib/gitlab/diff/highlight_spec.rb index fc5cb894d2a..4ab509f47b9 100644 --- a/spec/lib/gitlab/diff/highlight_spec.rb +++ b/spec/lib/gitlab/diff/highlight_spec.rb @@ -6,24 +6,24 @@ describe Gitlab::Diff::Highlight, lib: true do let(:project) { create(:project) } let(:commit) { project.commit(sample_commit.id) } let(:diff) { commit.diffs.first } - let(:diff_file) { Gitlab::Diff::File.new(diff) } + let(:diff_file) { Gitlab::Diff::File.new(diff, [commit.parent.id, commit.id], project.repository) } describe '.process_diff_lines' do context 'when processing Gitlab::Diff::Line objects' do - let(:diff_lines) { Gitlab::Diff::Highlight.process_diff_lines(diff_file.new_path, diff_file.diff_lines) } + let(:diff_lines) { Gitlab::Diff::Highlight.process_diff_lines(diff_file) } it 'should return Gitlab::Diff::Line elements' do expect(diff_lines.first).to be_an_instance_of(Gitlab::Diff::Line) end it 'should highlight the code' do - code = %Q{ def popen(cmd, path=nil)\n} + code = %Q{ def popen(cmd, path=nil)\n} expect(diff_lines[2].text).to eq(code) end - it 'should keep the inline diff markup' do - expect(diff_lines[5].text).to match(Regexp.new(Regexp.escape('RuntimeError, '))) + it 'should not generate the inline diff markup' do + expect(diff_lines[5].text).not_to match(Regexp.new(Regexp.escape(''))) end it 'should not modify "match" lines' do @@ -31,19 +31,18 @@ describe Gitlab::Diff::Highlight, lib: true do expect(diff_lines[22].text).to eq('@@ -19,6 +25,7 @@ module Popen') end end + end - context 'when processing raw lines' do - let(:lines) { ["puts 'Hello'\n", "# A comment"] } - let(:highlighted_lines) { Gitlab::Diff::Highlight.process_diff_lines('demo.rb', lines) } - - it 'should highlight the code' do - line_1 = %Q{puts 'Hello'\n} - line_2 = %Q{# A comment} - - expect(highlighted_lines[0]).to eq(line_1) - expect(highlighted_lines[1]).to eq(line_2) - end + describe '.process_file' do + let(:lines) do + Gitlab::Diff::Highlight.process_file(project.repository, commit.id, 'files/ruby/popen.rb') end + it 'should properly highlight all the lines' do + expect(lines[4]).to eq(%Q{ extend self\n}) + expect(lines[21]).to eq(%Q{ unless File.directory?(path)\n}) + expect(lines[26]).to eq(%Q{ @cmd_status = 0\n}) + end end + end From 78d7c0e0d81c40e0d9541e4ef0dd15df30d457e3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rub=C3=A9n=20D=C3=A1vila?= Date: Fri, 8 Jan 2016 19:05:55 -0500 Subject: [PATCH 18/57] Fix broken specs. #3945 --- app/views/projects/diffs/_parallel_view.html.haml | 4 ++-- lib/gitlab/diff/highlight.rb | 10 +++++----- spec/helpers/diff_helper_spec.rb | 3 ++- spec/lib/gitlab/diff/file_spec.rb | 2 +- 4 files changed, 10 insertions(+), 9 deletions(-) diff --git a/app/views/projects/diffs/_parallel_view.html.haml b/app/views/projects/diffs/_parallel_view.html.haml index 1ad54d1848b..77069501d1c 100644 --- a/app/views/projects/diffs/_parallel_view.html.haml +++ b/app/views/projects/diffs/_parallel_view.html.haml @@ -20,7 +20,7 @@ = link_to raw(line_number_left), "##{line_code_left}", id: line_code_left - if @comments_allowed && can?(current_user, :create_note, @project) = link_to_new_diff_note(line_code_left, 'old') - %td.line_content{class: "parallel noteable_line #{type_left} #{line_code_left}", "line_code" => line_code_left }= line_content_left.html_safe + %td.line_content{class: "parallel noteable_line #{type_left} #{line_code_left}", "line_code" => line_code_left }= raw(line_content_left) - if type_right == 'new' - new_line_class = 'new' @@ -33,7 +33,7 @@ = link_to raw(line_number_right), "##{new_line_code}", id: new_line_code - if @comments_allowed && can?(current_user, :create_note, @project) = link_to_new_diff_note(line_code_right, 'new') - %td.line_content.parallel{class: "noteable_line #{new_line_class} #{new_line_code}", "line_code" => new_line_code}= line_content_right.html_safe + %td.line_content.parallel{class: "noteable_line #{new_line_class} #{new_line_code}", "line_code" => new_line_code}= raw(line_content_right) - if @reply_allowed - comments_left, comments_right = organize_comments(type_left, type_right, line_code_left, line_code_right) diff --git a/lib/gitlab/diff/highlight.rb b/lib/gitlab/diff/highlight.rb index 7dd44b6004a..caeefe5bbdd 100644 --- a/lib/gitlab/diff/highlight.rb +++ b/lib/gitlab/diff/highlight.rb @@ -27,11 +27,11 @@ module Gitlab def self.formatter @formatter ||= Rouge::Formatters::HTMLGitlab.new( - nowrap: true, - cssclass: 'code highlight', - lineanchors: true, - lineanchorsid: 'LC' - ) + nowrap: true, + cssclass: 'code highlight', + lineanchors: true, + lineanchorsid: 'LC' + ) end def initialize(diff_file) diff --git a/spec/helpers/diff_helper_spec.rb b/spec/helpers/diff_helper_spec.rb index 0501c2e8c29..84b8f4d1679 100644 --- a/spec/helpers/diff_helper_spec.rb +++ b/spec/helpers/diff_helper_spec.rb @@ -7,7 +7,8 @@ describe DiffHelper do let(:commit) { project.commit(sample_commit.id) } let(:diffs) { commit.diffs } let(:diff) { diffs.first } - let(:diff_file) { Gitlab::Diff::File.new(diff) } + let(:diff_refs) { [commit.parent.id, commit.id] } + let(:diff_file) { Gitlab::Diff::File.new(diff, diff_refs, project.repository) } describe 'diff_hard_limit_enabled?' do it 'should return true if param is provided' do diff --git a/spec/lib/gitlab/diff/file_spec.rb b/spec/lib/gitlab/diff/file_spec.rb index c7cdf8691d6..97ba7f14eb1 100644 --- a/spec/lib/gitlab/diff/file_spec.rb +++ b/spec/lib/gitlab/diff/file_spec.rb @@ -6,7 +6,7 @@ describe Gitlab::Diff::File, lib: true do let(:project) { create(:project) } let(:commit) { project.commit(sample_commit.id) } let(:diff) { commit.diffs.first } - let(:diff_file) { Gitlab::Diff::File.new(diff) } + let(:diff_file) { Gitlab::Diff::File.new(diff, [commit.parent.id, commit.id], project.repository) } describe :diff_lines do let(:diff_lines) { diff_file.diff_lines } From 164c6374a708754086a1bca1033c46c01503056d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rub=C3=A9n=20D=C3=A1vila?= Date: Fri, 8 Jan 2016 19:57:51 -0500 Subject: [PATCH 19/57] Fix broken specs. #3945 --- spec/fixtures/parallel_diff_result.yml | 156 ++++++++++++------------- spec/helpers/diff_helper_spec.rb | 17 +-- 2 files changed, 86 insertions(+), 87 deletions(-) diff --git a/spec/fixtures/parallel_diff_result.yml b/spec/fixtures/parallel_diff_result.yml index 16c4bac96df..020c1817226 100644 --- a/spec/fixtures/parallel_diff_result.yml +++ b/spec/fixtures/parallel_diff_result.yml @@ -1,86 +1,86 @@ --- - - match - 6 - - '@@ -6,12 +6,18 @@ module Popen' + - "@@ -6,12 +6,18 @@ module Popen" - 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_6_6 - match - 6 - - '@@ -6,12 +6,18 @@ module Popen' + - "@@ -6,12 +6,18 @@ module Popen" - 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_6_6 - - - 6 - - | - + - |2 + - 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_6_6 - - 6 - - | - + - |2 + - 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_6_6 - - - 7 - - | - def popen(cmd, path=nil) + - |2 + def popen(cmd, path=nil) - 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_7_7 - - 7 - - | - def popen(cmd, path=nil) + - |2 + def popen(cmd, path=nil) - 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_7_7 - - - 8 - - | - unless cmd.is_a?(Array) + - |2 + unless cmd.is_a?(Array) - 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_8_8 - - 8 - - | - unless cmd.is_a?(Array) + - |2 + unless cmd.is_a?(Array) - 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_8_8 - - old - 9 - | - - raise "System commands must be given as an array of strings" + - raise "System commands must be given as an array of strings" - 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_9_9 - new - 9 - | - + raise RuntimeError, "System commands must be given as an array of strings" + + raise RuntimeError, "System commands must be given as an array of strings" - 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_10_9 - - - 10 - - | - end + - |2 + end - 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_10_10 - - 10 - - | - end + - |2 + end - 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_10_10 - - - 11 - - | - + - |2 + - 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_11_11 - - 11 - - | - + - |2 + - 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_11_11 - - - 12 - - | - path ||= Dir.pwd + - |2 + path ||= Dir.pwd - 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_12_12 - - 12 - - | - path ||= Dir.pwd + - |2 + path ||= Dir.pwd - 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_12_12 - - old - 13 - | - - vars = { "PWD" => path } + - vars = { "PWD" => path } - 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_13_13 - old - @@ -89,12 +89,12 @@ - - old - 14 - | - - options = { chdir: path } + - options = { chdir: path } - 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_14_13 - new - 13 - | - + + + - 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_15_13 - - - @@ -103,7 +103,7 @@ - new - 14 - | - + vars = { + + vars = { - 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_15_14 - - - @@ -112,7 +112,7 @@ - new - 15 - | - + "PWD" => path + + "PWD" => path - 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_15_15 - - - @@ -121,7 +121,7 @@ - new - 16 - | - + } + + } - 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_15_16 - - - @@ -130,7 +130,7 @@ - new - 17 - | - + + + - 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_15_17 - - - @@ -139,7 +139,7 @@ - new - 18 - | - + options = { + + options = { - 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_15_18 - - - @@ -148,7 +148,7 @@ - new - 19 - | - + chdir: path + + chdir: path - 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_15_19 - - - @@ -157,75 +157,75 @@ - new - 20 - | - + } + + } - 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_15_20 - - - 15 - - | - + - |2 + - 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_15_21 - - 21 - - | - + - |2 + - 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_15_21 - - - 16 - - | - unless File.directory?(path) + - |2 + unless File.directory?(path) - 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_16_22 - - 22 - - | - unless File.directory?(path) + - |2 + unless File.directory?(path) - 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_16_22 - - - 17 - - | - FileUtils.mkdir_p(path) + - |2 + FileUtils.mkdir_p(path) - 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_17_23 - - 23 - - | - FileUtils.mkdir_p(path) + - |2 + FileUtils.mkdir_p(path) - 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_17_23 - - match - 19 - - '@@ -19,6 +25,7 @@ module Popen' + - "@@ -19,6 +25,7 @@ module Popen" - 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_19_25 - match - 25 - - '@@ -19,6 +25,7 @@ module Popen' + - "@@ -19,6 +25,7 @@ module Popen" - 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_19_25 - - - 19 - - | - + - |2 + - 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_19_25 - - 25 - - | - + - |2 + - 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_19_25 - - - 20 - - | - @cmd_output = "" + - |2 + @cmd_output = "" - 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_20_26 - - 26 - - | - @cmd_output = "" + - |2 + @cmd_output = "" - 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_20_26 - - - 21 - - | - @cmd_status = 0 + - |2 + @cmd_status = 0 - 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_21_27 - - 27 - - | - @cmd_status = 0 + - |2 + @cmd_status = 0 - 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_21_27 - - - @@ -234,37 +234,35 @@ - new - 28 - | - + + + - 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_22_28 - - - 22 - - | - Open3.popen3(vars, *cmd, options) do |stdin, stdout, stderr, wait_thr| + - |2 + Open3.popen3(vars, *cmd, options) do |stdin, stdout, stderr, wait_thr| - 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_22_29 - - 29 - - | - Open3.popen3(vars, *cmd, options) do |stdin, stdout, stderr, wait_thr| + - |2 + Open3.popen3(vars, *cmd, options) do |stdin, stdout, stderr, wait_thr| - 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_22_29 - - - 23 - - | - @cmd_output << stdout.read + - |2 + @cmd_output << stdout.read - 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_23_30 - - 30 - - | - @cmd_output << stdout.read + - |2 + @cmd_output << stdout.read - 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_23_30 - - - 24 - - @cmd_output << stderr.read + - |2 + @cmd_output << stderr.read - 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_24_31 - - 31 - - @cmd_output << stderr.read + - |2 + @cmd_output << stderr.read - 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_24_31 diff --git a/spec/helpers/diff_helper_spec.rb b/spec/helpers/diff_helper_spec.rb index 84b8f4d1679..4b10ee3d33c 100644 --- a/spec/helpers/diff_helper_spec.rb +++ b/spec/helpers/diff_helper_spec.rb @@ -4,11 +4,12 @@ describe DiffHelper do include RepoHelpers let(:project) { create(:project) } + let(:repository) { project.repository } let(:commit) { project.commit(sample_commit.id) } let(:diffs) { commit.diffs } let(:diff) { diffs.first } let(:diff_refs) { [commit.parent.id, commit.id] } - let(:diff_file) { Gitlab::Diff::File.new(diff, diff_refs, project.repository) } + let(:diff_file) { Gitlab::Diff::File.new(diff, diff_refs, repository) } describe 'diff_hard_limit_enabled?' do it 'should return true if param is provided' do @@ -45,41 +46,41 @@ describe DiffHelper do describe 'safe_diff_files' do it 'should return all files from a commit that is smaller than safe limits' do - expect(safe_diff_files(diffs).length).to eq(2) + expect(safe_diff_files(diffs, diff_refs, repository).length).to eq(2) end it 'should return only the first file if the diff line count in the 2nd file takes the total beyond safe limits' do allow(diffs[1].diff).to receive(:lines).and_return([""] * 4999) #simulate 4999 lines - expect(safe_diff_files(diffs).length).to eq(1) + expect(safe_diff_files(diffs, diff_refs, repository).length).to eq(1) end it 'should return all files from a commit that is beyond safe limit for numbers of lines if force diff is true' do allow(controller).to receive(:params) { { force_show_diff: true } } allow(diffs[1].diff).to receive(:lines).and_return([""] * 4999) #simulate 4999 lines - expect(safe_diff_files(diffs).length).to eq(2) + expect(safe_diff_files(diffs, diff_refs, repository).length).to eq(2) end it 'should return only the first file if the diff line count in the 2nd file takes the total beyond hard limits' do allow(controller).to receive(:params) { { force_show_diff: true } } allow(diffs[1].diff).to receive(:lines).and_return([""] * 49999) #simulate 49999 lines - expect(safe_diff_files(diffs).length).to eq(1) + expect(safe_diff_files(diffs, diff_refs, repository).length).to eq(1) end it 'should return only a safe number of file diffs if a commit touches more files than the safe limits' do large_diffs = diffs * 100 #simulate 200 diffs - expect(safe_diff_files(large_diffs).length).to eq(100) + expect(safe_diff_files(large_diffs, diff_refs, repository).length).to eq(100) end it 'should return all file diffs if a commit touches more files than the safe limits but force diff is true' do allow(controller).to receive(:params) { { force_show_diff: true } } large_diffs = diffs * 100 #simulate 200 diffs - expect(safe_diff_files(large_diffs).length).to eq(200) + expect(safe_diff_files(large_diffs, diff_refs, repository).length).to eq(200) end it 'should return a limited file diffs if a commit touches more files than the hard limits and force diff is true' do allow(controller).to receive(:params) { { force_show_diff: true } } very_large_diffs = diffs * 1000 #simulate 2000 diffs - expect(safe_diff_files(very_large_diffs).length).to eq(1000) + expect(safe_diff_files(very_large_diffs, diff_refs, repository).length).to eq(1000) end end From fed10766e533fcef2a6840deeb8d7ea1747f0c72 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rub=C3=A9n=20D=C3=A1vila?= Date: Sat, 9 Jan 2016 01:55:31 -0500 Subject: [PATCH 20/57] Fix broken spec for submodule commit. #3945 --- lib/gitlab/diff/highlight.rb | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/lib/gitlab/diff/highlight.rb b/lib/gitlab/diff/highlight.rb index caeefe5bbdd..3c44abff3fb 100644 --- a/lib/gitlab/diff/highlight.rb +++ b/lib/gitlab/diff/highlight.rb @@ -85,10 +85,14 @@ module Gitlab case diff_line.type when 'new', nil - diff_line.text = new_lines[diff_line.new_pos - 1].try(:gsub!, /\A\s/, line_prefix) + line = new_lines[diff_line.new_pos - 1] when 'old' - diff_line.text = old_lines[diff_line.old_pos - 1].try(:gsub!, /\A\s/, line_prefix) + line = old_lines[diff_line.old_pos - 1] end + + # Only update text if line is found. This will prevent + # issues with submodules given the line only exists in diff content. + diff_line.text = line.gsub!(/\A\s/, line_prefix) if line end @lines @@ -121,6 +125,10 @@ module Gitlab lines.map! { |line| " #{line}" } end end + + def submodules + @submodules ||= diff_repository.raw_repository.submodules(diff_new_ref).keys + end end end end From ee2230c3291354035f52cc4d87e3340982367fa1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rub=C3=A9n=20D=C3=A1vila?= Date: Sat, 9 Jan 2016 02:24:30 -0500 Subject: [PATCH 21/57] Fix css for other highlighting themes. #3945 --- app/assets/stylesheets/highlight/dark.scss | 4 ++++ app/assets/stylesheets/highlight/solarized_dark.scss | 4 ++++ app/assets/stylesheets/highlight/solarized_light.scss | 4 ++++ 3 files changed, 12 insertions(+) diff --git a/app/assets/stylesheets/highlight/dark.scss b/app/assets/stylesheets/highlight/dark.scss index f7d1334705f..8201735beb5 100644 --- a/app/assets/stylesheets/highlight/dark.scss +++ b/app/assets/stylesheets/highlight/dark.scss @@ -92,12 +92,16 @@ .il { color: #de935f } /* Literal.Number.Integer.Long */ .line_holder { + &.parallel .new.new_line, + &.parallel .new.line_content, &.new .old_line, &.new .new_line, &.new .line_content { @include diff_background(255, 255, 255, #808080); } + &.parallel .old.old_line, + &.parallel .old.line_content, &.old .old_line, &.old .new_line, &.old .line_content { diff --git a/app/assets/stylesheets/highlight/solarized_dark.scss b/app/assets/stylesheets/highlight/solarized_dark.scss index 2c3648274cf..fdfac6cd249 100644 --- a/app/assets/stylesheets/highlight/solarized_dark.scss +++ b/app/assets/stylesheets/highlight/solarized_dark.scss @@ -113,12 +113,16 @@ .il { color: #2aa198 } /* Literal.Number.Integer.Long */ .line_holder { + &.parallel .new.new_line, + &.parallel .new.line_content, &.new .old_line, &.new .new_line, &.new .line_content { @include diff_background(255, 255, 255, #808080); } + &.parallel .old.old_line, + &.parallel .old.line_content, &.old .old_line, &.old .new_line, &.old .line_content { diff --git a/app/assets/stylesheets/highlight/solarized_light.scss b/app/assets/stylesheets/highlight/solarized_light.scss index c16473ffe66..f9788951aa8 100644 --- a/app/assets/stylesheets/highlight/solarized_light.scss +++ b/app/assets/stylesheets/highlight/solarized_light.scss @@ -114,12 +114,16 @@ .line_holder { + &.parallel .new.new_line, + &.parallel .new.line_content, &.new .old_line, &.new .new_line, &.new .line_content { @include diff_background(92, 164, 169, #FAF3DD); } + &.parallel .old.old_line, + &.parallel .old.line_content, &.old .old_line, &.old .new_line, &.old .line_content { From 80a4c808b1e5d331833f7b2ed531cb4fc81c7ef3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rub=C3=A9n=20D=C3=A1vila?= Date: Tue, 12 Jan 2016 11:02:16 -0500 Subject: [PATCH 22/57] Make diff_line_content helper return a safe String. #3945 --- app/helpers/diff_helper.rb | 4 ++-- app/views/projects/blob/preview.html.haml | 2 +- app/views/projects/diffs/_text_file.html.haml | 2 +- app/views/projects/notes/discussions/_diff.html.haml | 2 +- 4 files changed, 5 insertions(+), 5 deletions(-) diff --git a/app/helpers/diff_helper.rb b/app/helpers/diff_helper.rb index 668610364c5..28c8e58b2ad 100644 --- a/app/helpers/diff_helper.rb +++ b/app/helpers/diff_helper.rb @@ -111,9 +111,9 @@ module DiffHelper def diff_line_content(line) if line.blank? - "  " + "  ".html_safe else - line + line.try(:html_safe) end end diff --git a/app/views/projects/blob/preview.html.haml b/app/views/projects/blob/preview.html.haml index e7c3460ad78..fed483d6788 100644 --- a/app/views/projects/blob/preview.html.haml +++ b/app/views/projects/blob/preview.html.haml @@ -20,6 +20,6 @@ - else %td.old_line %td.new_line - %td.line_content{class: "#{line.type}"}= raw diff_line_content(line.text) + %td.line_content{class: "#{line.type}"}= diff_line_content(line.text) - else .nothing-here-block No changes. diff --git a/app/views/projects/diffs/_text_file.html.haml b/app/views/projects/diffs/_text_file.html.haml index f1b8cba5e55..521f2ac1e8d 100644 --- a/app/views/projects/diffs/_text_file.html.haml +++ b/app/views/projects/diffs/_text_file.html.haml @@ -22,7 +22,7 @@ = link_to_new_diff_note(line_code) %td.new_line{data: {linenumber: line.new_pos}} = link_to raw(type == "old" ? " " : line.new_pos), "##{line_code}", id: line_code - %td.line_content{class: "noteable_line #{type} #{line_code}", "line_code" => line_code}= diff_line_content(line.text).html_safe + %td.line_content{class: "noteable_line #{type} #{line_code}", "line_code" => line_code}= diff_line_content(line.text) - if @reply_allowed - comments = @line_notes.select { |n| n.line_code == line_code && n.active? }.sort_by(&:created_at) diff --git a/app/views/projects/notes/discussions/_diff.html.haml b/app/views/projects/notes/discussions/_diff.html.haml index 0301445b5b2..97347a9f67f 100644 --- a/app/views/projects/notes/discussions/_diff.html.haml +++ b/app/views/projects/notes/discussions/_diff.html.haml @@ -24,7 +24,7 @@ = raw(type == "new" ? " " : line.old_pos) %td.new_line = raw(type == "old" ? " " : line.new_pos) - %td.line_content{class: "noteable_line #{type} #{line_code}", "line_code" => line_code}= raw diff_line_content(line.text) + %td.line_content{class: "noteable_line #{type} #{line_code}", "line_code" => line_code}= diff_line_content(line.text) - if line_code == note.line_code = render "projects/notes/diff_notes_with_reply", notes: discussion_notes From c476395b4d8b78cfc7431153a144ffccbd414c61 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rub=C3=A9n=20D=C3=A1vila?= Date: Tue, 12 Jan 2016 11:14:41 -0500 Subject: [PATCH 23/57] Reuse existent vars with ref and path. #3945 --- app/controllers/projects/blob_controller.rb | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/app/controllers/projects/blob_controller.rb b/app/controllers/projects/blob_controller.rb index 6aa602321f4..6ca3a636359 100644 --- a/app/controllers/projects/blob_controller.rb +++ b/app/controllers/projects/blob_controller.rb @@ -65,10 +65,8 @@ class Projects::BlobController < Projects::ApplicationController end def diff - ref, file_name = params[:id].split('/', 2) - @form = UnfoldForm.new(params) - @lines = Gitlab::Diff::Highlight.process_file(repository, ref, file_name) + @lines = Gitlab::Diff::Highlight.process_file(repository, @ref, @path) @lines = @lines[@form.since - 1..@form.to - 1] if @form.bottom? From f1f9b5f7d388c6d7a0938229c9211beddb2fd6a2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rub=C3=A9n=20D=C3=A1vila?= Date: Tue, 12 Jan 2016 12:53:54 -0500 Subject: [PATCH 24/57] Small fixes from code review. #3945 --- app/controllers/projects/merge_requests_controller.rb | 2 +- app/helpers/blob_helper.rb | 6 +++--- lib/gitlab/diff/highlight.rb | 4 ---- 3 files changed, 4 insertions(+), 8 deletions(-) diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb index d25adbe952d..b70533ef7be 100644 --- a/app/controllers/projects/merge_requests_controller.rb +++ b/app/controllers/projects/merge_requests_controller.rb @@ -104,7 +104,7 @@ class Projects::MergeRequestsController < Projects::ApplicationController @commit = @merge_request.last_commit @first_commit = @merge_request.first_commit @diffs = @merge_request.compare_diffs - @diff_refs = [@merge_request.target_sha, @merge_request.source_branch] + @diff_refs = [@merge_request.target_sha, @merge_request.source_sha] @ci_commit = @merge_request.ci_commit @statuses = @ci_commit.statuses if @ci_commit diff --git a/app/helpers/blob_helper.rb b/app/helpers/blob_helper.rb index 1230002e69c..a80162a3e33 100644 --- a/app/helpers/blob_helper.rb +++ b/app/helpers/blob_helper.rb @@ -11,14 +11,14 @@ module BlobHelper end def highlight(blob_name, blob_content, nowrap: false, continue: false) - @formatter ||= rouge_formatter(nowrap: nowrap) + formatter = rouge_formatter(nowrap: nowrap) begin @lexer ||= Rouge::Lexer.guess(filename: blob_name, source: blob_content).new - result = @formatter.format(@lexer.lex(blob_content, continue: continue)).html_safe + result = formatter.format(@lexer.lex(blob_content, continue: continue)).html_safe rescue @lexer = Rouge::Lexers::PlainText - result = @formatter.format(@lexer.lex(blob_content)).html_safe + result = formatter.format(@lexer.lex(blob_content)).html_safe end result diff --git a/lib/gitlab/diff/highlight.rb b/lib/gitlab/diff/highlight.rb index 3c44abff3fb..d0137ab5f08 100644 --- a/lib/gitlab/diff/highlight.rb +++ b/lib/gitlab/diff/highlight.rb @@ -125,10 +125,6 @@ module Gitlab lines.map! { |line| " #{line}" } end end - - def submodules - @submodules ||= diff_repository.raw_repository.submodules(diff_new_ref).keys - end end end end From c0385488fbfaf1285122793cea417e607c8e771e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rub=C3=A9n=20D=C3=A1vila?= Date: Tue, 12 Jan 2016 15:12:30 -0500 Subject: [PATCH 25/57] Fix broken spec. #3945 --- app/helpers/diff_helper.rb | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/app/helpers/diff_helper.rb b/app/helpers/diff_helper.rb index 28c8e58b2ad..0ec532a9a90 100644 --- a/app/helpers/diff_helper.rb +++ b/app/helpers/diff_helper.rb @@ -113,7 +113,8 @@ module DiffHelper if line.blank? "  ".html_safe else - line.try(:html_safe) + # Return line if it isn't a String, it helps when it's Numeric + line.is_a?(String) ? line.html_safe : line end end From 6e3358a5077f5c6052e733722cd6baa63e43c081 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rub=C3=A9n=20D=C3=A1vila?= Date: Tue, 12 Jan 2016 17:36:08 -0500 Subject: [PATCH 26/57] Remove no longer required code. #3945 --- lib/gitlab/diff/highlight.rb | 51 +++++++----------------------------- 1 file changed, 9 insertions(+), 42 deletions(-) diff --git a/lib/gitlab/diff/highlight.rb b/lib/gitlab/diff/highlight.rb index d0137ab5f08..14cf98e24a2 100644 --- a/lib/gitlab/diff/highlight.rb +++ b/lib/gitlab/diff/highlight.rb @@ -44,74 +44,41 @@ module Gitlab return [] if @lines.empty? extract_line_prefixes - - @code = unescape_html(raw_content) - @highlighted_code = formatter.format(lexer.lex(@code)) - - is_diff_line? ? update_diff_lines : @highlighted_code.lines + update_diff_lines end private - def is_diff_line? - @lines.first.is_a?(Gitlab::Diff::Line) - end - def text_lines - @text_lines ||= (is_diff_line? ? @lines.map(&:text) : @lines) - end - - def raw_content - @raw_content ||= text_lines.join(is_diff_line? ? "\n" : nil) + @text_lines ||= @lines.map(&:text) end def extract_line_prefixes - @diff_line_prefixes ||= begin - if is_diff_line? - text_lines.map { |line| line.sub!(/\A((\+|\-))/, '');$1 } - else - [] - end - end + @diff_line_prefixes ||= text_lines.map { |line| line.sub!(/\A((\+|\-))/, '');$1 } end def update_diff_lines - @highlighted_code.lines.each_with_index do |line, i| - diff_line = @lines[i] + @lines.each_with_index do |line, i| line_prefix = @diff_line_prefixes[i] || ' ' # ignore highlighting for "match" lines - next if diff_line.type == 'match' + next if line.type == 'match' - case diff_line.type + case line.type when 'new', nil - line = new_lines[diff_line.new_pos - 1] + highlighted_line = new_lines[line.new_pos - 1] when 'old' - line = old_lines[diff_line.old_pos - 1] + highlighted_line = old_lines[line.old_pos - 1] end # Only update text if line is found. This will prevent # issues with submodules given the line only exists in diff content. - diff_line.text = line.gsub!(/\A\s/, line_prefix) if line + line.text = highlighted_line.gsub!(/\A\s/, line_prefix) if line end @lines end - def lexer - Rouge::Lexer.guess(filename: @file_name, source: @code).new rescue Rouge::Lexers::PlainText.new - end - - def unescape_html(content) - text = CGI.unescapeHTML(content) - text.gsub!(' ', ' ') - text - end - - def formatter - self.class.formatter - end - def old_lines @old_lines ||= begin lines = self.class.process_file(diff_repository, diff_old_ref, diff_old_path) From f547e733d1f8acf2c8ae82835b91ae166cf95b16 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rub=C3=A9n=20D=C3=A1vila?= Date: Tue, 12 Jan 2016 17:49:11 -0500 Subject: [PATCH 27/57] Add more specs. #3945 --- spec/lib/gitlab/diff/highlight_spec.rb | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/spec/lib/gitlab/diff/highlight_spec.rb b/spec/lib/gitlab/diff/highlight_spec.rb index 4ab509f47b9..cdeed603e23 100644 --- a/spec/lib/gitlab/diff/highlight_spec.rb +++ b/spec/lib/gitlab/diff/highlight_spec.rb @@ -30,6 +30,24 @@ describe Gitlab::Diff::Highlight, lib: true 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 highlight unchanged lines' do + code = %Q{ def popen(cmd, path=nil)\n} + + expect(diff_lines[2].text).to eq(code) + end + + it 'should highlight added lines' do + code = %Q{+ raise RuntimeError, "System commands must be given as an array of strings"\n} + + expect(diff_lines[5].text).to eq(code) + end + + it 'should highlight removed lines' do + code = %Q{- raise "System commands must be given as an array of strings"\n} + + expect(diff_lines[4].text).to eq(code) + end end end From 7307fa48e70a9fb42902c8a84479d4cf669640aa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rub=C3=A9n=20D=C3=A1vila?= Date: Tue, 12 Jan 2016 19:28:31 -0500 Subject: [PATCH 28/57] Fix broken specs. #3945 --- app/controllers/projects/merge_requests_controller.rb | 3 ++- lib/gitlab/diff/highlight.rb | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb index b70533ef7be..95fd62b8ace 100644 --- a/app/controllers/projects/merge_requests_controller.rb +++ b/app/controllers/projects/merge_requests_controller.rb @@ -104,7 +104,8 @@ class Projects::MergeRequestsController < Projects::ApplicationController @commit = @merge_request.last_commit @first_commit = @merge_request.first_commit @diffs = @merge_request.compare_diffs - @diff_refs = [@merge_request.target_sha, @merge_request.source_sha] + # We need to use #source_branch because #source_sha requires an existent MergeRequestDiff object. + @diff_refs = [@merge_request.target_sha, @merge_request.source_branch] @ci_commit = @merge_request.ci_commit @statuses = @ci_commit.statuses if @ci_commit diff --git a/lib/gitlab/diff/highlight.rb b/lib/gitlab/diff/highlight.rb index 14cf98e24a2..0b6a348acbc 100644 --- a/lib/gitlab/diff/highlight.rb +++ b/lib/gitlab/diff/highlight.rb @@ -73,7 +73,7 @@ module Gitlab # Only update text if line is found. This will prevent # issues with submodules given the line only exists in diff content. - line.text = highlighted_line.gsub!(/\A\s/, line_prefix) if line + line.text = highlighted_line.gsub!(/\A\s/, line_prefix) if highlighted_line end @lines From 48c45ba9a8a9a5536a3d501e40536cc5b73062a1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rub=C3=A9n=20D=C3=A1vila?= Date: Wed, 13 Jan 2016 10:58:19 -0500 Subject: [PATCH 29/57] Use html_safe instead of raw. #3945 --- app/views/projects/diffs/_parallel_view.html.haml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/views/projects/diffs/_parallel_view.html.haml b/app/views/projects/diffs/_parallel_view.html.haml index 77069501d1c..1ad54d1848b 100644 --- a/app/views/projects/diffs/_parallel_view.html.haml +++ b/app/views/projects/diffs/_parallel_view.html.haml @@ -20,7 +20,7 @@ = link_to raw(line_number_left), "##{line_code_left}", id: line_code_left - if @comments_allowed && can?(current_user, :create_note, @project) = link_to_new_diff_note(line_code_left, 'old') - %td.line_content{class: "parallel noteable_line #{type_left} #{line_code_left}", "line_code" => line_code_left }= raw(line_content_left) + %td.line_content{class: "parallel noteable_line #{type_left} #{line_code_left}", "line_code" => line_code_left }= line_content_left.html_safe - if type_right == 'new' - new_line_class = 'new' @@ -33,7 +33,7 @@ = link_to raw(line_number_right), "##{new_line_code}", id: new_line_code - if @comments_allowed && can?(current_user, :create_note, @project) = link_to_new_diff_note(line_code_right, 'new') - %td.line_content.parallel{class: "noteable_line #{new_line_class} #{new_line_code}", "line_code" => new_line_code}= raw(line_content_right) + %td.line_content.parallel{class: "noteable_line #{new_line_class} #{new_line_code}", "line_code" => new_line_code}= line_content_right.html_safe - if @reply_allowed - comments_left, comments_right = organize_comments(type_left, type_right, line_code_left, line_code_right) From 0f0af19139db71255934e9a7a5b5cd86420b7186 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rub=C3=A9n=20D=C3=A1vila?= Date: Wed, 13 Jan 2016 11:39:15 -0500 Subject: [PATCH 30/57] Little refactor for usage of html_safe. #3945 --- app/helpers/diff_helper.rb | 9 +++++---- app/views/projects/blob/diff.html.haml | 2 +- app/views/projects/diffs/_parallel_view.html.haml | 4 ++-- lib/gitlab/diff/highlight.rb | 4 ++-- 4 files changed, 10 insertions(+), 9 deletions(-) diff --git a/app/helpers/diff_helper.rb b/app/helpers/diff_helper.rb index 0ec532a9a90..d49e22e8c84 100644 --- a/app/helpers/diff_helper.rb +++ b/app/helpers/diff_helper.rb @@ -1,4 +1,6 @@ module DiffHelper + BLANK_SPACE = " ".html_safe + def diff_view params[:view] == 'parallel' ? 'parallel' : 'inline' end @@ -83,7 +85,7 @@ module DiffHelper elsif next_type == 'old' || next_type.nil? # Left side has text removed, right side doesn't have any change # No next line code, no new line number, no new line text - line = [type, line_old, full_line, line_code, next_type, nil, " ", nil] + line = [type, line_old, full_line, line_code, next_type, nil, BLANK_SPACE, nil] lines.push(line) end elsif type == 'new' @@ -93,7 +95,7 @@ module DiffHelper next else # Change is only on the right side, left side has no change - line = [nil, nil, " ", line_code, type, line_new, full_line, line_code] + line = [nil, nil, BLANK_SPACE, line_code, type, line_new, full_line, line_code] lines.push(line) end end @@ -113,8 +115,7 @@ module DiffHelper if line.blank? "  ".html_safe else - # Return line if it isn't a String, it helps when it's Numeric - line.is_a?(String) ? line.html_safe : line + line.html_safe end end diff --git a/app/views/projects/blob/diff.html.haml b/app/views/projects/blob/diff.html.haml index ef23876677a..2e913802be1 100644 --- a/app/views/projects/blob/diff.html.haml +++ b/app/views/projects/blob/diff.html.haml @@ -11,7 +11,7 @@ %td.old_line.diff-line-num{data: {linenumber: line_old}} = link_to raw(line_old), "#" %td.new_line= link_to raw(line_new) , "#" - %td.line_content.noteable_line= "#{' ' * @form.indent}#{line}".html_safe + %td.line_content.noteable_line==#{' ' * @form.indent}#{line} - if @form.unfold? && @form.bottom? && @form.to < @blob.loc %tr.line_holder{ id: @form.to } diff --git a/app/views/projects/diffs/_parallel_view.html.haml b/app/views/projects/diffs/_parallel_view.html.haml index 1ad54d1848b..e9108c04cef 100644 --- a/app/views/projects/diffs/_parallel_view.html.haml +++ b/app/views/projects/diffs/_parallel_view.html.haml @@ -20,7 +20,7 @@ = link_to raw(line_number_left), "##{line_code_left}", id: line_code_left - if @comments_allowed && can?(current_user, :create_note, @project) = link_to_new_diff_note(line_code_left, 'old') - %td.line_content{class: "parallel noteable_line #{type_left} #{line_code_left}", "line_code" => line_code_left }= line_content_left.html_safe + %td.line_content{class: "parallel noteable_line #{type_left} #{line_code_left}", "line_code" => line_code_left }= line_content_left - if type_right == 'new' - new_line_class = 'new' @@ -33,7 +33,7 @@ = link_to raw(line_number_right), "##{new_line_code}", id: new_line_code - if @comments_allowed && can?(current_user, :create_note, @project) = link_to_new_diff_note(line_code_right, 'new') - %td.line_content.parallel{class: "noteable_line #{new_line_class} #{new_line_code}", "line_code" => new_line_code}= line_content_right.html_safe + %td.line_content.parallel{class: "noteable_line #{new_line_class} #{new_line_code}", "line_code" => new_line_code}= line_content_right - if @reply_allowed - comments_left, comments_right = organize_comments(type_left, type_right, line_code_left, line_code_right) diff --git a/lib/gitlab/diff/highlight.rb b/lib/gitlab/diff/highlight.rb index 0b6a348acbc..f940b57d596 100644 --- a/lib/gitlab/diff/highlight.rb +++ b/lib/gitlab/diff/highlight.rb @@ -22,7 +22,7 @@ module Gitlab content = blob.data lexer = Rouge::Lexer.guess(filename: file_name, source: content).new rescue Rouge::Lexers::PlainText.new - formatter.format(lexer.lex(content)).lines + formatter.format(lexer.lex(content)).lines.map!(&:html_safe) end def self.formatter @@ -73,7 +73,7 @@ module Gitlab # Only update text if line is found. This will prevent # issues with submodules given the line only exists in diff content. - line.text = highlighted_line.gsub!(/\A\s/, line_prefix) if highlighted_line + line.text = highlighted_line.gsub!(/\A\s/, line_prefix).html_safe if highlighted_line end @lines From 1161cf2ec610cb0ceba61c92180566c9786ab059 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rub=C3=A9n=20D=C3=A1vila?= Date: Wed, 13 Jan 2016 18:01:40 -0500 Subject: [PATCH 31/57] Use current commit id if it doesn't have a parent. #3945 --- app/controllers/projects/commit_controller.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/controllers/projects/commit_controller.rb b/app/controllers/projects/commit_controller.rb index 9bbf4581057..c8f143dd6b4 100644 --- a/app/controllers/projects/commit_controller.rb +++ b/app/controllers/projects/commit_controller.rb @@ -72,7 +72,7 @@ class Projects::CommitController < Projects::ApplicationController @diffs = commit.diffs end - @diff_refs = [commit.parent.id, commit.id] + @diff_refs = [commit.parent_id || commit.id, commit.id] @notes_count = commit.notes.count @statuses = ci_commit.statuses if ci_commit From c179b48c97be22ef55ef9f5874984f7359fb12f1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rub=C3=A9n=20D=C3=A1vila?= Date: Wed, 13 Jan 2016 21:04:53 -0500 Subject: [PATCH 32/57] Use #sub instead of #gsub!. #3945 * This is because is not a good idea to modify the original lines * Also I run into this issue https://gitlab.com/gitlab-org/gitlab_git/issues/14 which is returning Diff Lines with the same @new_pos value. --- lib/gitlab/diff/highlight.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/gitlab/diff/highlight.rb b/lib/gitlab/diff/highlight.rb index f940b57d596..e76a6f27856 100644 --- a/lib/gitlab/diff/highlight.rb +++ b/lib/gitlab/diff/highlight.rb @@ -73,7 +73,7 @@ module Gitlab # Only update text if line is found. This will prevent # issues with submodules given the line only exists in diff content. - line.text = highlighted_line.gsub!(/\A\s/, line_prefix).html_safe if highlighted_line + line.text = highlighted_line.sub(/\A\s/, line_prefix).html_safe if highlighted_line end @lines From c881627d114eb9c050d605e93673ef65a9da9a58 Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Thu, 14 Jan 2016 14:52:08 +0100 Subject: [PATCH 33/57] Refactor parallel_diff generation a bit. --- app/helpers/blob_helper.rb | 11 +-- app/helpers/diff_helper.rb | 86 ++++++++++++++----- .../projects/diffs/_parallel_view.html.haml | 43 ++++------ 3 files changed, 83 insertions(+), 57 deletions(-) diff --git a/app/helpers/blob_helper.rb b/app/helpers/blob_helper.rb index a80162a3e33..84e3cbb380b 100644 --- a/app/helpers/blob_helper.rb +++ b/app/helpers/blob_helper.rb @@ -13,15 +13,8 @@ module BlobHelper def highlight(blob_name, blob_content, nowrap: false, continue: false) formatter = rouge_formatter(nowrap: nowrap) - begin - @lexer ||= Rouge::Lexer.guess(filename: blob_name, source: blob_content).new - result = formatter.format(@lexer.lex(blob_content, continue: continue)).html_safe - rescue - @lexer = Rouge::Lexers::PlainText - result = formatter.format(@lexer.lex(blob_content)).html_safe - end - - result + @lexer ||= Rouge::Lexer.guess(filename: blob_name, source: blob_content).new rescue Rouge::Lexers::PlainText + formatter.format(@lexer.lex(blob_content, continue: continue)).html_safe end def no_highlight_files diff --git a/app/helpers/diff_helper.rb b/app/helpers/diff_helper.rb index d49e22e8c84..1596f9e7d19 100644 --- a/app/helpers/diff_helper.rb +++ b/app/helpers/diff_helper.rb @@ -1,6 +1,4 @@ module DiffHelper - BLANK_SPACE = " ".html_safe - def diff_view params[:view] == 'parallel' ? 'parallel' : 'inline' end @@ -49,15 +47,7 @@ module DiffHelper lines = [] skip_next = false - # Building array of lines - # - # [ - # left_type, left_line_number, left_line_content, left_line_code, - # right_line_type, right_line_number, right_line_content, right_line_code - # ] - # diff_file.highlighted_diff_lines.each do |line| - full_line = line.text type = line.type line_code = generate_line_code(diff_file.file_path, line) @@ -72,31 +62,81 @@ module DiffHelper next_line = next_line.text end - if type == 'match' || type.nil? + case type + when 'match', nil # line in the right panel is the same as in the left one - line = [type, line_old, full_line, line_code, type, line_new, full_line, line_code] - lines.push(line) - elsif type == 'old' - if next_type == 'new' + lines << { + left: { + type: type, + number: line_old, + text: full_line, + line_code: line_code, + }, + right: { + type: type, + number: line_new, + text: full_line, + line_code: line_code + } + } + when 'old' + case next_type + when 'new' # Left side has text removed, right side has text added - line = [type, line_old, full_line, line_code, next_type, line_new, next_line, next_line_code] - lines.push(line) + lines << { + left: { + type: type, + number: line_old, + text: full_line, + line_code: line_code, + }, + right: { + type: next_type, + number: line_new, + text: next_line, + line_code: next_line_code + } + } skip_next = true - elsif next_type == 'old' || next_type.nil? + when 'old', nil # Left side has text removed, right side doesn't have any change # No next line code, no new line number, no new line text - line = [type, line_old, full_line, line_code, next_type, nil, BLANK_SPACE, nil] - lines.push(line) + lines << { + left: { + type: type, + number: line_old, + text: full_line, + line_code: line_code, + }, + right: { + type: next_type, + number: nil, + text: "", + line_code: nil + } + } end - elsif type == 'new' + when 'new' if skip_next # Change has been already included in previous line so no need to do it again skip_next = false next else # Change is only on the right side, left side has no change - line = [nil, nil, BLANK_SPACE, line_code, type, line_new, full_line, line_code] - lines.push(line) + lines << { + left: { + type: nil, + number: nil, + text: "", + line_code: line_code, + }, + right: { + type: type, + number: line_new, + text: full_line, + line_code: line_code + } + } end end end diff --git a/app/views/projects/diffs/_parallel_view.html.haml b/app/views/projects/diffs/_parallel_view.html.haml index e9108c04cef..a2958286ada 100644 --- a/app/views/projects/diffs/_parallel_view.html.haml +++ b/app/views/projects/diffs/_parallel_view.html.haml @@ -2,41 +2,34 @@ %div.text-file.diff-wrap-lines.code.file-content.js-syntax-highlight %table - parallel_diff(diff_file, index).each do |line| - - type_left = line[0] - - line_number_left = line[1] - - line_content_left = line[2] - - line_code_left = line[3] - - type_right = line[4] - - line_number_right = line[5] - - line_content_right = line[6] - - line_code_right = line[7] - + - left = line[:left] + - right = line[:right] %tr.line_holder.parallel - - if type_left == 'match' - = render "projects/diffs/match_line_parallel", { line: line_content_left, - line_old: line_number_left, line_new: line_number_right } - - elsif type_left == 'old' || type_left.nil? - %td.old_line{id: line_code_left, class: "#{type_left}"} - = link_to raw(line_number_left), "##{line_code_left}", id: line_code_left + - if left[:type] == 'match' + = render "projects/diffs/match_line_parallel", { line: left[:text], + line_old: left[:number], line_new: right[:number] } + - else + %td.old_line{id: left[:line_code], class: "#{left[:type]}"} + = link_to raw(left[:number]), "##{left[:line_code]}", id: left[:line_code] - if @comments_allowed && can?(current_user, :create_note, @project) - = link_to_new_diff_note(line_code_left, 'old') - %td.line_content{class: "parallel noteable_line #{type_left} #{line_code_left}", "line_code" => line_code_left }= line_content_left + = link_to_new_diff_note(left[:line_code], 'old') + %td.line_content{class: "parallel noteable_line #{left[:type]} #{left[:line_code]}", "line_code" => left[:line_code] }= diff_line_content(left[:text]) - - if type_right == 'new' + - if right[:type] == 'new' - new_line_class = 'new' - - new_line_code = line_code_right + - new_line_code = right[:line_code] - else - new_line_class = nil - - new_line_code = line_code_left + - new_line_code = left[:line_code] - %td.new_line{id: new_line_code, class: "#{new_line_class}", data: { linenumber: line_number_right }} - = link_to raw(line_number_right), "##{new_line_code}", id: new_line_code + %td.new_line{id: new_line_code, class: "#{new_line_class}", data: { linenumber: right[:number] }} + = link_to raw(right[:number]), "##{new_line_code}", id: new_line_code - if @comments_allowed && can?(current_user, :create_note, @project) - = link_to_new_diff_note(line_code_right, 'new') - %td.line_content.parallel{class: "noteable_line #{new_line_class} #{new_line_code}", "line_code" => new_line_code}= line_content_right + = link_to_new_diff_note(right[:line_code], 'new') + %td.line_content.parallel{class: "noteable_line #{new_line_class} #{new_line_code}", "line_code" => new_line_code}= diff_line_content(right[:text]) - if @reply_allowed - - comments_left, comments_right = organize_comments(type_left, type_right, line_code_left, line_code_right) + - comments_left, comments_right = organize_comments(left[:type], right[:type], left[:line_code], right[:line_code]) - if comments_left.present? || comments_right.present? = render "projects/notes/diff_notes_with_reply_parallel", notes_left: comments_left, notes_right: comments_right From 3a1d0535992594bc77320f081d1f20b760b1c1f7 Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Thu, 14 Jan 2016 15:11:50 +0100 Subject: [PATCH 34/57] Remove duplication around highlighting. --- app/helpers/blob_helper.rb | 16 +--------------- lib/gitlab/diff/highlight.rb | 13 +------------ lib/gitlab/highlight.rb | 23 +++++++++++++++++++++++ 3 files changed, 25 insertions(+), 27 deletions(-) create mode 100644 lib/gitlab/highlight.rb diff --git a/app/helpers/blob_helper.rb b/app/helpers/blob_helper.rb index 84e3cbb380b..be856242c43 100644 --- a/app/helpers/blob_helper.rb +++ b/app/helpers/blob_helper.rb @@ -1,20 +1,6 @@ module BlobHelper - def rouge_formatter(options = {}) - default_options = { - nowrap: false, - cssclass: 'code highlight', - lineanchors: true, - lineanchorsid: 'LC' - } - - Rouge::Formatters::HTMLGitlab.new(default_options.merge!(options)) - end - def highlight(blob_name, blob_content, nowrap: false, continue: false) - formatter = rouge_formatter(nowrap: nowrap) - - @lexer ||= Rouge::Lexer.guess(filename: blob_name, source: blob_content).new rescue Rouge::Lexers::PlainText - formatter.format(@lexer.lex(blob_content, continue: continue)).html_safe + Gitlab::Highlight.highlight(blob_name, blob_content, nowrap: nowrap, continue: continue) end def no_highlight_files diff --git a/lib/gitlab/diff/highlight.rb b/lib/gitlab/diff/highlight.rb index e76a6f27856..f34eff62d79 100644 --- a/lib/gitlab/diff/highlight.rb +++ b/lib/gitlab/diff/highlight.rb @@ -20,18 +20,7 @@ module Gitlab blob = repository.blob_at(ref, file_name) return [] unless blob - content = blob.data - lexer = Rouge::Lexer.guess(filename: file_name, source: content).new rescue Rouge::Lexers::PlainText.new - formatter.format(lexer.lex(content)).lines.map!(&:html_safe) - end - - def self.formatter - @formatter ||= Rouge::Formatters::HTMLGitlab.new( - nowrap: true, - cssclass: 'code highlight', - lineanchors: true, - lineanchorsid: 'LC' - ) + Gitlab::Highlight.highlight(file_name, blob.data).lines.map!(&:html_safe) end def initialize(diff_file) diff --git a/lib/gitlab/highlight.rb b/lib/gitlab/highlight.rb new file mode 100644 index 00000000000..02e097eca3d --- /dev/null +++ b/lib/gitlab/highlight.rb @@ -0,0 +1,23 @@ +module Gitlab + class Highlight + def self.highlight(blob_name, blob_content, nowrap: true, continue: false) + formatter = rouge_formatter(nowrap: nowrap) + + lexer = Rouge::Lexer.guess(filename: blob_name, source: blob_content).new rescue Rouge::Lexers::PlainText + formatter.format(lexer.lex(blob_content, continue: continue)).html_safe + end + + private + + def self.rouge_formatter(options = {}) + options = options.reverse_merge( + nowrap: true, + cssclass: 'code highlight', + lineanchors: true, + lineanchorsid: 'LC' + ) + + Rouge::Formatters::HTMLGitlab.new(options) + end + end +end From 83e4fc188b22731d89106b4da28f11bf5509c116 Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Thu, 14 Jan 2016 16:13:35 +0100 Subject: [PATCH 35/57] Refactor highlighting lines --- lib/gitlab/diff/highlight.rb | 29 ++++------------------------- 1 file changed, 4 insertions(+), 25 deletions(-) diff --git a/lib/gitlab/diff/highlight.rb b/lib/gitlab/diff/highlight.rb index f34eff62d79..ba2f12db147 100644 --- a/lib/gitlab/diff/highlight.rb +++ b/lib/gitlab/diff/highlight.rb @@ -32,23 +32,8 @@ module Gitlab def highlight return [] if @lines.empty? - extract_line_prefixes - update_diff_lines - end - - private - - def text_lines - @text_lines ||= @lines.map(&:text) - end - - def extract_line_prefixes - @diff_line_prefixes ||= text_lines.map { |line| line.sub!(/\A((\+|\-))/, '');$1 } - end - - def update_diff_lines @lines.each_with_index do |line, i| - line_prefix = @diff_line_prefixes[i] || ' ' + line_prefix = line.text.match(/\A([+-])/) ? $1 : ' ' # ignore highlighting for "match" lines next if line.type == 'match' @@ -62,24 +47,18 @@ module Gitlab # Only update text if line is found. This will prevent # issues with submodules given the line only exists in diff content. - line.text = highlighted_line.sub(/\A\s/, line_prefix).html_safe if highlighted_line + line.text = highlighted_line.insert(0, line_prefix).html_safe if highlighted_line end @lines end def old_lines - @old_lines ||= begin - lines = self.class.process_file(diff_repository, diff_old_ref, diff_old_path) - lines.map! { |line| " #{line}" } - end + @old_lines ||= self.class.process_file(diff_repository, diff_old_ref, diff_old_path) end def new_lines - @new_lines ||= begin - lines = self.class.process_file(diff_repository, diff_new_ref, diff_new_path) - lines.map! { |line| " #{line}" } - end + @new_lines ||= self.class.process_file(diff_repository, diff_new_ref, diff_new_path) end end end From 8dfad143d44af4896ff6c71e8a42ad32b69ad593 Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Thu, 14 Jan 2016 22:28:07 +0100 Subject: [PATCH 36/57] Add inline diff markers in highlighted diffs. --- app/controllers/projects/blob_controller.rb | 2 +- .../projects/diffs/_parallel_view.html.haml | 4 +- app/views/projects/diffs/_text_file.html.haml | 2 +- lib/gitlab/diff/file.rb | 2 +- lib/gitlab/diff/highlight.rb | 210 +++++++++++++++--- lib/gitlab/highlight.rb | 7 + spec/lib/gitlab/diff/highlight_spec.rb | 4 +- 7 files changed, 189 insertions(+), 42 deletions(-) diff --git a/app/controllers/projects/blob_controller.rb b/app/controllers/projects/blob_controller.rb index 6ca3a636359..8133de90a41 100644 --- a/app/controllers/projects/blob_controller.rb +++ b/app/controllers/projects/blob_controller.rb @@ -66,7 +66,7 @@ class Projects::BlobController < Projects::ApplicationController def diff @form = UnfoldForm.new(params) - @lines = Gitlab::Diff::Highlight.process_file(repository, @ref, @path) + @lines = Gitlab::Highlight.highlight_lines(repository, @ref, @path) @lines = @lines[@form.since - 1..@form.to - 1] if @form.bottom? diff --git a/app/views/projects/diffs/_parallel_view.html.haml b/app/views/projects/diffs/_parallel_view.html.haml index a2958286ada..7b9cecbc3da 100644 --- a/app/views/projects/diffs/_parallel_view.html.haml +++ b/app/views/projects/diffs/_parallel_view.html.haml @@ -13,7 +13,7 @@ = link_to raw(left[:number]), "##{left[:line_code]}", id: left[:line_code] - if @comments_allowed && can?(current_user, :create_note, @project) = link_to_new_diff_note(left[:line_code], 'old') - %td.line_content{class: "parallel noteable_line #{left[:type]} #{left[:line_code]}", "line_code" => left[:line_code] }= diff_line_content(left[:text]) + %td.line_content{class: "parallel noteable_line #{left[:type]} #{left[:line_code]}", data: { "line_code" => left[:line_code] }}= diff_line_content(left[:text]) - if right[:type] == 'new' - new_line_class = 'new' @@ -26,7 +26,7 @@ = link_to raw(right[:number]), "##{new_line_code}", id: new_line_code - if @comments_allowed && can?(current_user, :create_note, @project) = link_to_new_diff_note(right[:line_code], 'new') - %td.line_content.parallel{class: "noteable_line #{new_line_class} #{new_line_code}", "line_code" => new_line_code}= diff_line_content(right[:text]) + %td.line_content.parallel{class: "noteable_line #{new_line_class} #{new_line_code}", data: { "line_code" => new_line_code }}= diff_line_content(right[:text]) - if @reply_allowed - comments_left, comments_right = organize_comments(left[:type], right[:type], left[:line_code], right[:line_code]) diff --git a/app/views/projects/diffs/_text_file.html.haml b/app/views/projects/diffs/_text_file.html.haml index 521f2ac1e8d..6761155dcf9 100644 --- a/app/views/projects/diffs/_text_file.html.haml +++ b/app/views/projects/diffs/_text_file.html.haml @@ -22,7 +22,7 @@ = link_to_new_diff_note(line_code) %td.new_line{data: {linenumber: line.new_pos}} = link_to raw(type == "old" ? " " : line.new_pos), "##{line_code}", id: line_code - %td.line_content{class: "noteable_line #{type} #{line_code}", "line_code" => line_code}= diff_line_content(line.text) + %td.line_content{class: "noteable_line #{type} #{line_code}", data: { "line_code" => line_code }}= diff_line_content(line.text) - if @reply_allowed - comments = @line_notes.select { |n| n.line_code == line_code && n.active? }.sort_by(&:created_at) diff --git a/lib/gitlab/diff/file.rb b/lib/gitlab/diff/file.rb index cb93c6a574a..c1a6e16da5a 100644 --- a/lib/gitlab/diff/file.rb +++ b/lib/gitlab/diff/file.rb @@ -18,7 +18,7 @@ module Gitlab end def highlighted_diff_lines - Gitlab::Diff::Highlight.process_diff_lines(self) + Gitlab::Diff::Highlight.new(self).highlight end def mode_changed? diff --git a/lib/gitlab/diff/highlight.rb b/lib/gitlab/diff/highlight.rb index ba2f12db147..e21f496102d 100644 --- a/lib/gitlab/diff/highlight.rb +++ b/lib/gitlab/diff/highlight.rb @@ -6,59 +6,199 @@ module Gitlab delegate :repository, :old_path, :new_path, :old_ref, :new_ref, to: :diff_file, prefix: :diff - # Apply syntax highlight to provided source code - # - # diff_file - an instance of Gitlab::Diff::File - # - # Returns an Array with the processed items. - def self.process_diff_lines(diff_file) - processor = new(diff_file) - processor.highlight - end - - def self.process_file(repository, ref, file_name) - blob = repository.blob_at(ref, file_name) - return [] unless blob - - Gitlab::Highlight.highlight(file_name, blob.data).lines.map!(&:html_safe) - end - def initialize(diff_file) @diff_file = diff_file - @file_name = diff_file.new_path - @lines = diff_file.diff_lines + @diff_lines = diff_file.diff_lines + @raw_lines = @diff_lines.map(&:text) end def highlight - return [] if @lines.empty? + return [] if @diff_lines.empty? - @lines.each_with_index do |line, i| - line_prefix = line.text.match(/\A([+-])/) ? $1 : ' ' + find_inline_diffs + process_lines + + @diff_lines + end + + private + + def find_inline_diffs + @inline_diffs = [] + + local_edit_indexes.each do |index| + old_index = index + new_index = index + 1 + old_line = @raw_lines[old_index][1..-1] + new_line = @raw_lines[new_index][1..-1] + + # Skip inline diff if empty line was replaced with content + next if old_line == "" + + lcp = longest_common_prefix(old_line, new_line) + lcs = longest_common_suffix(old_line, new_line) + + old_diff_range = lcp..(old_line.length - lcs - 1) + 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[new_index] = new_diff_range if new_diff_range.begin <= new_diff_range.end + end + end + + def process_lines + @diff_lines.each_with_index do |diff_line, i| # ignore highlighting for "match" lines - next if line.type == 'match' + next if diff_line.type == 'match' - case line.type - when 'new', nil - highlighted_line = new_lines[line.new_pos - 1] - when 'old' - highlighted_line = old_lines[line.old_pos - 1] - end + rich_line = highlight_line(diff_line, i) + rich_line = mark_inline_diffs(rich_line, diff_line, i) + diff_line.text = rich_line.html_safe + end + end - # Only update text if line is found. This will prevent - # issues with submodules given the line only exists in diff content. - line.text = highlighted_line.insert(0, line_prefix).html_safe if highlighted_line + def highlight_line(diff_line, index) + line_prefix = line_prefixes[index] + + case diff_line.type + when 'new', nil + rich_line = new_lines[diff_line.new_pos - 1] + when 'old' + rich_line = old_lines[diff_line.old_pos - 1] end - @lines + # 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 : diff_line.text + end + + def mark_inline_diffs(rich_line, diff_line, index) + inline_diff = @inline_diffs[index] + return rich_line unless inline_diff + + raw_line = diff_line.text + + # Based on the prefixless versions + from = inline_diff.begin + 1 + to = inline_diff.end + 1 + + position_mapping = map_character_positions(raw_line, rich_line) + inline_diff_positions = position_mapping[from..to] + marker_ranges = collapse_ranges(inline_diff_positions) + + offset = 0 + marker_ranges.each do |range| + offset = insert_around_range(rich_line, range, "", "", offset) + end + + rich_line + end + + def line_prefixes + @line_prefixes ||= @raw_lines.map { |line| line.match(/\A([+-])/) ? $1 : ' ' } + end + + def local_edit_indexes + @local_edit_indexes ||= begin + joined_line_prefixes = " #{line_prefixes.join} " + + offset = 0 + local_edit_indexes = [] + while index = joined_line_prefixes.index(" -+ ", offset) + local_edit_indexes << index + offset = index + 1 + end + + local_edit_indexes + end + end + + def map_character_positions(raw_line, rich_line) + mapping = [] + raw_pos = 0 + rich_pos = 0 + (0..raw_line.length).each do |raw_pos| + raw_char = raw_line[raw_pos] + rich_char = rich_line[rich_pos] + + while rich_char == '<' + until rich_char == '>' + rich_pos += 1 + rich_char = rich_line[rich_pos] + end + + rich_pos += 1 + rich_char = rich_line[rich_pos] + end + + mapping[raw_pos] = rich_pos + + rich_pos += 1 + end + + mapping end def old_lines - @old_lines ||= self.class.process_file(diff_repository, diff_old_ref, diff_old_path) + @old_lines ||= Gitlab::Highlight.highlight_lines(diff_repository, diff_old_ref, diff_old_path) end def new_lines - @new_lines ||= self.class.process_file(diff_repository, diff_new_ref, diff_new_path) + @new_lines ||= Gitlab::Highlight.highlight_lines(diff_repository, diff_new_ref, diff_new_path) + end + + def longest_common_suffix(a, b) + longest_common_prefix(a.reverse, b.reverse) + end + + def longest_common_prefix(a, b) + max_length = [a.length, b.length].max + + length = 0 + (0..max_length - 1).each do |pos| + old_char = a[pos] + new_char = b[pos] + + break if old_char != new_char + length += 1 + end + + length + end + + def collapse_ranges(positions) + return [] if positions.empty? + ranges = [] + + start = prev = positions[0] + range = start..prev + positions[1..-1].each do |pos| + if pos == prev + 1 + range = start..pos + prev = pos + else + ranges << range + start = prev = pos + range = start..prev + end + end + ranges << range + + ranges + end + + def insert_around_range(text, range, before, after, offset = 0) + from = range.begin + to = range.end + + text.insert(offset + from, before) + offset += before.length + + text.insert(offset + to + 1, after) + offset += after.length + + offset end end end diff --git a/lib/gitlab/highlight.rb b/lib/gitlab/highlight.rb index 02e097eca3d..a5b041687e3 100644 --- a/lib/gitlab/highlight.rb +++ b/lib/gitlab/highlight.rb @@ -7,6 +7,13 @@ module Gitlab formatter.format(lexer.lex(blob_content, continue: continue)).html_safe end + def self.highlight_lines(repository, ref, file_name) + blob = repository.blob_at(ref, file_name) + return [] unless blob + + highlight(file_name, blob.data).lines.map!(&:html_safe) + end + private def self.rouge_formatter(options = {}) diff --git a/spec/lib/gitlab/diff/highlight_spec.rb b/spec/lib/gitlab/diff/highlight_spec.rb index cdeed603e23..3c66c9889ba 100644 --- a/spec/lib/gitlab/diff/highlight_spec.rb +++ b/spec/lib/gitlab/diff/highlight_spec.rb @@ -51,9 +51,9 @@ describe Gitlab::Diff::Highlight, lib: true do end end - describe '.process_file' do + describe '.highlight_lines' do let(:lines) do - Gitlab::Diff::Highlight.process_file(project.repository, commit.id, 'files/ruby/popen.rb') + Gitlab::Diff::Highlight.highlight_lines(project.repository, commit.id, 'files/ruby/popen.rb') end it 'should properly highlight all the lines' do From 70bc322415b33a4789067b81387d30f1411c9091 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rub=C3=A9n=20D=C3=A1vila?= Date: Thu, 14 Jan 2016 12:56:44 -0500 Subject: [PATCH 37/57] Use the adequate reference for the old rev. #3945 --- app/controllers/projects/merge_requests_controller.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb index 95fd62b8ace..3d792af0a48 100644 --- a/app/controllers/projects/merge_requests_controller.rb +++ b/app/controllers/projects/merge_requests_controller.rb @@ -59,7 +59,7 @@ class Projects::MergeRequestsController < Projects::ApplicationController def diffs @commit = @merge_request.last_commit @first_commit = @merge_request.first_commit - @diff_refs = [@merge_request.target_sha, @merge_request.source_sha] + @diff_refs = [@merge_request.last_commit.parent_id, @merge_request.source_sha] @comments_allowed = @reply_allowed = true @comments_target = { From 6b9c730e91962a6d6343bcb7fc4dc75c99b41bde Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rub=C3=A9n=20D=C3=A1vila?= Date: Thu, 14 Jan 2016 16:38:37 -0500 Subject: [PATCH 38/57] More refactoring from last code review. #3945 * Use commit objects instead of IDs when generating diffs * Use proper references when generating MR's source and target * Update broken specs --- app/controllers/projects/commit_controller.rb | 2 +- .../projects/compare_controller.rb | 2 +- .../projects/merge_requests_controller.rb | 3 - app/helpers/diff_helper.rb | 4 +- app/models/merge_request.rb | 4 + app/views/projects/diffs/_diffs.html.haml | 2 +- .../merge_requests/_new_submit.html.haml | 2 +- .../merge_requests/show/_diffs.html.haml | 3 +- lib/gitlab/diff/file.rb | 5 +- lib/gitlab/diff/highlight.rb | 17 +- spec/fixtures/parallel_diff_result.yml | 590 ++++++++++-------- spec/helpers/diff_helper_spec.rb | 24 +- spec/lib/gitlab/diff/file_spec.rb | 2 +- spec/lib/gitlab/diff/highlight_spec.rb | 2 +- 14 files changed, 366 insertions(+), 296 deletions(-) diff --git a/app/controllers/projects/commit_controller.rb b/app/controllers/projects/commit_controller.rb index c8f143dd6b4..ba924ccc5a5 100644 --- a/app/controllers/projects/commit_controller.rb +++ b/app/controllers/projects/commit_controller.rb @@ -72,7 +72,7 @@ class Projects::CommitController < Projects::ApplicationController @diffs = commit.diffs end - @diff_refs = [commit.parent_id || commit.id, commit.id] + @diff_refs = [commit.parent || commit, commit] @notes_count = commit.notes.count @statuses = ci_commit.statuses if ci_commit diff --git a/app/controllers/projects/compare_controller.rb b/app/controllers/projects/compare_controller.rb index 5b88aed7a64..60360c8e5c8 100644 --- a/app/controllers/projects/compare_controller.rb +++ b/app/controllers/projects/compare_controller.rb @@ -20,9 +20,9 @@ class Projects::CompareController < Projects::ApplicationController if compare_result @commits = Commit.decorate(compare_result.commits, @project) @diffs = compare_result.diffs - @diff_refs = [base_ref, head_ref] @commit = @project.commit(head_ref) @first_commit = @project.commit(base_ref) + @diff_refs = [@first_commit, @commit] @line_notes = [] end end diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb index 3d792af0a48..ab5c953189c 100644 --- a/app/controllers/projects/merge_requests_controller.rb +++ b/app/controllers/projects/merge_requests_controller.rb @@ -59,7 +59,6 @@ class Projects::MergeRequestsController < Projects::ApplicationController def diffs @commit = @merge_request.last_commit @first_commit = @merge_request.first_commit - @diff_refs = [@merge_request.last_commit.parent_id, @merge_request.source_sha] @comments_allowed = @reply_allowed = true @comments_target = { @@ -104,8 +103,6 @@ class Projects::MergeRequestsController < Projects::ApplicationController @commit = @merge_request.last_commit @first_commit = @merge_request.first_commit @diffs = @merge_request.compare_diffs - # We need to use #source_branch because #source_sha requires an existent MergeRequestDiff object. - @diff_refs = [@merge_request.target_sha, @merge_request.source_branch] @ci_commit = @merge_request.ci_commit @statuses = @ci_commit.statuses if @ci_commit diff --git a/app/helpers/diff_helper.rb b/app/helpers/diff_helper.rb index 1596f9e7d19..425a8ced549 100644 --- a/app/helpers/diff_helper.rb +++ b/app/helpers/diff_helper.rb @@ -19,13 +19,13 @@ module DiffHelper end end - def safe_diff_files(diffs, diff_refs, repository) + def safe_diff_files(diffs, diff_refs) lines = 0 safe_files = [] diffs.first(allowed_diff_size).each do |diff| lines += diff.diff.lines.count break if lines > allowed_diff_lines - safe_files << Gitlab::Diff::File.new(diff, diff_refs, repository) + safe_files << Gitlab::Diff::File.new(diff, diff_refs) end safe_files end diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index ac25d38eb63..fe87b820e98 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -511,4 +511,8 @@ class MergeRequest < ActiveRecord::Base def broken? self.commits.blank? || branch_missing? || cannot_be_merged? end + + def diff_range + [last_commit.parent, first_commit] + end end diff --git a/app/views/projects/diffs/_diffs.html.haml b/app/views/projects/diffs/_diffs.html.haml index 58ad7d79af9..b2f9c14da88 100644 --- a/app/views/projects/diffs/_diffs.html.haml +++ b/app/views/projects/diffs/_diffs.html.haml @@ -1,7 +1,7 @@ - if diff_view == 'parallel' - fluid_layout true -- diff_files = safe_diff_files(diffs, diff_refs, repository) +- diff_files = safe_diff_files(diffs, diff_refs) .gray-content-block.middle-block.oneline-block .inline-parallel-buttons diff --git a/app/views/projects/merge_requests/_new_submit.html.haml b/app/views/projects/merge_requests/_new_submit.html.haml index 0cb8b6daedb..f2a12099b26 100644 --- a/app/views/projects/merge_requests/_new_submit.html.haml +++ b/app/views/projects/merge_requests/_new_submit.html.haml @@ -38,7 +38,7 @@ = render "projects/merge_requests/show/commits" #diffs.diffs.tab-pane.active - if @diffs.present? - = render "projects/diffs/diffs", diffs: @diffs, project: @project, diff_refs: @diff_refs + = render "projects/diffs/diffs", diffs: @diffs, project: @project, diff_refs: @merge_request.diff_range - elsif @commits.size > MergeRequestDiff::COMMITS_SAFE_SIZE .alert.alert-danger %h4 This comparison includes more than #{MergeRequestDiff::COMMITS_SAFE_SIZE} commits. diff --git a/app/views/projects/merge_requests/show/_diffs.html.haml b/app/views/projects/merge_requests/show/_diffs.html.haml index 1d9af1e4160..46c6f79937b 100644 --- a/app/views/projects/merge_requests/show/_diffs.html.haml +++ b/app/views/projects/merge_requests/show/_diffs.html.haml @@ -1,5 +1,6 @@ - if @merge_request_diff.collected? - = render "projects/diffs/diffs", diffs: params[:w] == '1' ? @merge_request.diffs_no_whitespace : @merge_request.diffs, project: @merge_request.project, diff_refs: @diff_refs + = render "projects/diffs/diffs", diffs: params[:w] == '1' ? @merge_request.diffs_no_whitespace : @merge_request.diffs, + project: @merge_request.project, diff_refs: @merge_request.diff_range - elsif @merge_request_diff.empty? .nothing-here-block Nothing to merge from #{@merge_request.source_branch} into #{@merge_request.target_branch} - else diff --git a/lib/gitlab/diff/file.rb b/lib/gitlab/diff/file.rb index c1a6e16da5a..a6a7fc8ff4c 100644 --- a/lib/gitlab/diff/file.rb +++ b/lib/gitlab/diff/file.rb @@ -1,14 +1,13 @@ module Gitlab module Diff class File - attr_reader :diff, :repository, :new_ref, :old_ref + attr_reader :diff, :new_ref, :old_ref delegate :new_file, :deleted_file, :renamed_file, :old_path, :new_path, to: :diff, prefix: false - def initialize(diff, diff_refs, repository) + def initialize(diff, diff_refs) @diff = diff - @repository = repository @old_ref, @new_ref = diff_refs end diff --git a/lib/gitlab/diff/highlight.rb b/lib/gitlab/diff/highlight.rb index e21f496102d..fb79a2a69de 100644 --- a/lib/gitlab/diff/highlight.rb +++ b/lib/gitlab/diff/highlight.rb @@ -3,8 +3,7 @@ module Gitlab class Highlight attr_reader :diff_file - delegate :repository, :old_path, :new_path, :old_ref, :new_ref, - to: :diff_file, prefix: :diff + delegate :old_path, :new_path, :old_ref, :new_ref, to: :diff_file, prefix: :diff def initialize(diff_file) @diff_file = diff_file @@ -141,11 +140,11 @@ module Gitlab end def old_lines - @old_lines ||= Gitlab::Highlight.highlight_lines(diff_repository, diff_old_ref, diff_old_path) + @old_lines ||= self.class.process_file(*processing_args(:old)) end def new_lines - @new_lines ||= Gitlab::Highlight.highlight_lines(diff_repository, diff_new_ref, diff_new_path) + @new_lines ||= self.class.process_file(*processing_args(:new)) end def longest_common_suffix(a, b) @@ -200,6 +199,16 @@ module Gitlab offset end + + private + + def processing_args(version) + ref = send("diff_#{version}_ref") + path = send("diff_#{version}_path") + + [ref.project.repository, ref.id, path] + end + end end end diff --git a/spec/fixtures/parallel_diff_result.yml b/spec/fixtures/parallel_diff_result.yml index 020c1817226..4583de1231e 100644 --- a/spec/fixtures/parallel_diff_result.yml +++ b/spec/fixtures/parallel_diff_result.yml @@ -1,268 +1,324 @@ --- -- - match - - 6 - - "@@ -6,12 +6,18 @@ module Popen" - - 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_6_6 - - match - - 6 - - "@@ -6,12 +6,18 @@ module Popen" - - 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_6_6 -- - - - 6 - - |2 - - - 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_6_6 - - - - 6 - - |2 - - - 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_6_6 -- - - - 7 - - |2 - def popen(cmd, path=nil) - - 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_7_7 - - - - 7 - - |2 - def popen(cmd, path=nil) - - 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_7_7 -- - - - 8 - - |2 - unless cmd.is_a?(Array) - - 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_8_8 - - - - 8 - - |2 - unless cmd.is_a?(Array) - - 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_8_8 -- - old - - 9 - - | - - raise "System commands must be given as an array of strings" - - 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_9_9 - - new - - 9 - - | - + raise RuntimeError, "System commands must be given as an array of strings" - - 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_10_9 -- - - - 10 - - |2 - end - - 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_10_10 - - - - 10 - - |2 - end - - 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_10_10 -- - - - 11 - - |2 - - - 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_11_11 - - - - 11 - - |2 - - - 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_11_11 -- - - - 12 - - |2 - path ||= Dir.pwd - - 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_12_12 - - - - 12 - - |2 - path ||= Dir.pwd - - 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_12_12 -- - old - - 13 - - | - - vars = { "PWD" => path } - - 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_13_13 - - old - - - - " " - - -- - old - - 14 - - | - - options = { chdir: path } - - 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_14_13 - - new - - 13 - - | - + - - 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_15_13 -- - - - - - " " - - 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_15_14 - - new - - 14 - - | - + vars = { - - 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_15_14 -- - - - - - " " - - 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_15_15 - - new - - 15 - - | - + "PWD" => path - - 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_15_15 -- - - - - - " " - - 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_15_16 - - new - - 16 - - | - + } - - 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_15_16 -- - - - - - " " - - 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_15_17 - - new - - 17 - - | - + - - 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_15_17 -- - - - - - " " - - 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_15_18 - - new - - 18 - - | - + options = { - - 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_15_18 -- - - - - - " " - - 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_15_19 - - new - - 19 - - | - + chdir: path - - 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_15_19 -- - - - - - " " - - 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_15_20 - - new - - 20 - - | - + } - - 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_15_20 -- - - - 15 - - |2 - - - 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_15_21 - - - - 21 - - |2 - - - 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_15_21 -- - - - 16 - - |2 - unless File.directory?(path) - - 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_16_22 - - - - 22 - - |2 - unless File.directory?(path) - - 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_16_22 -- - - - 17 - - |2 - FileUtils.mkdir_p(path) - - 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_17_23 - - - - 23 - - |2 - FileUtils.mkdir_p(path) - - 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_17_23 -- - match - - 19 - - "@@ -19,6 +25,7 @@ module Popen" - - 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_19_25 - - match - - 25 - - "@@ -19,6 +25,7 @@ module Popen" - - 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_19_25 -- - - - 19 - - |2 - - - 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_19_25 - - - - 25 - - |2 - - - 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_19_25 -- - - - 20 - - |2 - @cmd_output = "" - - 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_20_26 - - - - 26 - - |2 - @cmd_output = "" - - 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_20_26 -- - - - 21 - - |2 - @cmd_status = 0 - - 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_21_27 - - - - 27 - - |2 - @cmd_status = 0 - - 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_21_27 -- - - - - - " " - - 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_22_28 - - new - - 28 - - | - + - - 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_22_28 -- - - - 22 - - |2 - Open3.popen3(vars, *cmd, options) do |stdin, stdout, stderr, wait_thr| - - 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_22_29 - - - - 29 - - |2 - Open3.popen3(vars, *cmd, options) do |stdin, stdout, stderr, wait_thr| - - 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_22_29 -- - - - 23 - - |2 - @cmd_output << stdout.read - - 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_23_30 - - - - 30 - - |2 - @cmd_output << stdout.read - - 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_23_30 -- - - - 24 - - |2 - @cmd_output << stderr.read - - 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_24_31 - - - - 31 - - |2 - @cmd_output << stderr.read - - 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_24_31 +- :left: + :type: match + :number: 6 + :text: "@@ -6,12 +6,18 @@ module Popen" + :line_code: 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_6_6 + :right: + :type: match + :number: 6 + :text: "@@ -6,12 +6,18 @@ module Popen" + :line_code: 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_6_6 +- :left: + :type: + :number: 6 + :text: |2 + + :line_code: 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_6_6 + :right: + :type: + :number: 6 + :text: |2 + + :line_code: 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_6_6 +- :left: + :type: + :number: 7 + :text: |2 + def popen(cmd, path=nil) + :line_code: 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_7_7 + :right: + :type: + :number: 7 + :text: |2 + def popen(cmd, path=nil) + :line_code: 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_7_7 +- :left: + :type: + :number: 8 + :text: |2 + unless cmd.is_a?(Array) + :line_code: 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_8_8 + :right: + :type: + :number: 8 + :text: |2 + unless cmd.is_a?(Array) + :line_code: 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_8_8 +- :left: + :type: old + :number: 9 + :text: | + - raise "System commands must be given as an array of strings" + :line_code: 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_9_9 + :right: + :type: new + :number: 9 + :text: | + + raise RuntimeError, "System commands must be given as an array of strings" + :line_code: 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_10_9 +- :left: + :type: + :number: 10 + :text: |2 + end + :line_code: 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_10_10 + :right: + :type: + :number: 10 + :text: |2 + end + :line_code: 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_10_10 +- :left: + :type: + :number: 11 + :text: |2 + + :line_code: 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_11_11 + :right: + :type: + :number: 11 + :text: |2 + + :line_code: 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_11_11 +- :left: + :type: + :number: 12 + :text: |2 + path ||= Dir.pwd + :line_code: 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_12_12 + :right: + :type: + :number: 12 + :text: |2 + path ||= Dir.pwd + :line_code: 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_12_12 +- :left: + :type: old + :number: 13 + :text: | + - vars = { "PWD" => path } + :line_code: 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_13_13 + :right: + :type: old + :number: + :text: '' + :line_code: +- :left: + :type: old + :number: 14 + :text: | + - options = { chdir: path } + :line_code: 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_14_13 + :right: + :type: new + :number: 13 + :text: | + + + :line_code: 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_15_13 +- :left: + :type: + :number: + :text: '' + :line_code: 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_15_14 + :right: + :type: new + :number: 14 + :text: | + + vars = { + :line_code: 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_15_14 +- :left: + :type: + :number: + :text: '' + :line_code: 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_15_15 + :right: + :type: new + :number: 15 + :text: | + + "PWD" => path + :line_code: 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_15_15 +- :left: + :type: + :number: + :text: '' + :line_code: 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_15_16 + :right: + :type: new + :number: 16 + :text: | + + } + :line_code: 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_15_16 +- :left: + :type: + :number: + :text: '' + :line_code: 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_15_17 + :right: + :type: new + :number: 17 + :text: | + + + :line_code: 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_15_17 +- :left: + :type: + :number: + :text: '' + :line_code: 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_15_18 + :right: + :type: new + :number: 18 + :text: | + + options = { + :line_code: 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_15_18 +- :left: + :type: + :number: + :text: '' + :line_code: 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_15_19 + :right: + :type: new + :number: 19 + :text: | + + chdir: path + :line_code: 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_15_19 +- :left: + :type: + :number: + :text: '' + :line_code: 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_15_20 + :right: + :type: new + :number: 20 + :text: | + + } + :line_code: 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_15_20 +- :left: + :type: + :number: 15 + :text: |2 + + :line_code: 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_15_21 + :right: + :type: + :number: 21 + :text: |2 + + :line_code: 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_15_21 +- :left: + :type: + :number: 16 + :text: |2 + unless File.directory?(path) + :line_code: 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_16_22 + :right: + :type: + :number: 22 + :text: |2 + unless File.directory?(path) + :line_code: 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_16_22 +- :left: + :type: + :number: 17 + :text: |2 + FileUtils.mkdir_p(path) + :line_code: 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_17_23 + :right: + :type: + :number: 23 + :text: |2 + FileUtils.mkdir_p(path) + :line_code: 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_17_23 +- :left: + :type: match + :number: 19 + :text: "@@ -19,6 +25,7 @@ module Popen" + :line_code: 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_19_25 + :right: + :type: match + :number: 25 + :text: "@@ -19,6 +25,7 @@ module Popen" + :line_code: 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_19_25 +- :left: + :type: + :number: 19 + :text: |2 + + :line_code: 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_19_25 + :right: + :type: + :number: 25 + :text: |2 + + :line_code: 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_19_25 +- :left: + :type: + :number: 20 + :text: |2 + @cmd_output = "" + :line_code: 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_20_26 + :right: + :type: + :number: 26 + :text: |2 + @cmd_output = "" + :line_code: 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_20_26 +- :left: + :type: + :number: 21 + :text: |2 + @cmd_status = 0 + :line_code: 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_21_27 + :right: + :type: + :number: 27 + :text: |2 + @cmd_status = 0 + :line_code: 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_21_27 +- :left: + :type: + :number: + :text: '' + :line_code: 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_22_28 + :right: + :type: new + :number: 28 + :text: | + + + :line_code: 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_22_28 +- :left: + :type: + :number: 22 + :text: |2 + Open3.popen3(vars, *cmd, options) do |stdin, stdout, stderr, wait_thr| + :line_code: 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_22_29 + :right: + :type: + :number: 29 + :text: |2 + Open3.popen3(vars, *cmd, options) do |stdin, stdout, stderr, wait_thr| + :line_code: 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_22_29 +- :left: + :type: + :number: 23 + :text: |2 + @cmd_output << stdout.read + :line_code: 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_23_30 + :right: + :type: + :number: 30 + :text: |2 + @cmd_output << stdout.read + :line_code: 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_23_30 +- :left: + :type: + :number: 24 + :text: |2 + @cmd_output << stderr.read + :line_code: 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_24_31 + :right: + :type: + :number: 31 + :text: |2 + @cmd_output << stderr.read + :line_code: 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_24_31 diff --git a/spec/helpers/diff_helper_spec.rb b/spec/helpers/diff_helper_spec.rb index 4b10ee3d33c..435ecd04fbe 100644 --- a/spec/helpers/diff_helper_spec.rb +++ b/spec/helpers/diff_helper_spec.rb @@ -8,8 +8,8 @@ describe DiffHelper do let(:commit) { project.commit(sample_commit.id) } let(:diffs) { commit.diffs } let(:diff) { diffs.first } - let(:diff_refs) { [commit.parent.id, commit.id] } - let(:diff_file) { Gitlab::Diff::File.new(diff, diff_refs, repository) } + let(:diff_refs) { [commit.parent, commit] } + let(:diff_file) { Gitlab::Diff::File.new(diff, diff_refs) } describe 'diff_hard_limit_enabled?' do it 'should return true if param is provided' do @@ -46,41 +46,41 @@ describe DiffHelper do describe 'safe_diff_files' do it 'should return all files from a commit that is smaller than safe limits' do - expect(safe_diff_files(diffs, diff_refs, repository).length).to eq(2) + expect(safe_diff_files(diffs, diff_refs).length).to eq(2) end it 'should return only the first file if the diff line count in the 2nd file takes the total beyond safe limits' do allow(diffs[1].diff).to receive(:lines).and_return([""] * 4999) #simulate 4999 lines - expect(safe_diff_files(diffs, diff_refs, repository).length).to eq(1) + expect(safe_diff_files(diffs, diff_refs).length).to eq(1) end it 'should return all files from a commit that is beyond safe limit for numbers of lines if force diff is true' do allow(controller).to receive(:params) { { force_show_diff: true } } allow(diffs[1].diff).to receive(:lines).and_return([""] * 4999) #simulate 4999 lines - expect(safe_diff_files(diffs, diff_refs, repository).length).to eq(2) + expect(safe_diff_files(diffs, diff_refs).length).to eq(2) end it 'should return only the first file if the diff line count in the 2nd file takes the total beyond hard limits' do allow(controller).to receive(:params) { { force_show_diff: true } } allow(diffs[1].diff).to receive(:lines).and_return([""] * 49999) #simulate 49999 lines - expect(safe_diff_files(diffs, diff_refs, repository).length).to eq(1) + expect(safe_diff_files(diffs, diff_refs).length).to eq(1) end it 'should return only a safe number of file diffs if a commit touches more files than the safe limits' do large_diffs = diffs * 100 #simulate 200 diffs - expect(safe_diff_files(large_diffs, diff_refs, repository).length).to eq(100) + expect(safe_diff_files(large_diffs, diff_refs).length).to eq(100) end it 'should return all file diffs if a commit touches more files than the safe limits but force diff is true' do allow(controller).to receive(:params) { { force_show_diff: true } } large_diffs = diffs * 100 #simulate 200 diffs - expect(safe_diff_files(large_diffs, diff_refs, repository).length).to eq(200) + expect(safe_diff_files(large_diffs, diff_refs).length).to eq(200) end it 'should return a limited file diffs if a commit touches more files than the hard limits and force diff is true' do allow(controller).to receive(:params) { { force_show_diff: true } } very_large_diffs = diffs * 1000 #simulate 2000 diffs - expect(safe_diff_files(very_large_diffs, diff_refs, repository).length).to eq(1000) + expect(safe_diff_files(very_large_diffs, diff_refs).length).to eq(1000) end end @@ -128,7 +128,11 @@ describe DiffHelper do expect(diff_line_content(diff_file.diff_lines.first.text)). to eq('@@ -6,12 +6,18 @@ module Popen') expect(diff_line_content(diff_file.diff_lines.first.type)).to eq('match') - expect(diff_line_content(diff_file.diff_lines.first.new_pos)).to eq(6) + expect(diff_file.diff_lines.first.new_pos).to eq(6) + end + + it 'should return safe HTML' do + expect(diff_line_content(diff_file.diff_lines.first.text)).to be_html_safe end end diff --git a/spec/lib/gitlab/diff/file_spec.rb b/spec/lib/gitlab/diff/file_spec.rb index 97ba7f14eb1..0d9694f2c13 100644 --- a/spec/lib/gitlab/diff/file_spec.rb +++ b/spec/lib/gitlab/diff/file_spec.rb @@ -6,7 +6,7 @@ describe Gitlab::Diff::File, lib: true do let(:project) { create(:project) } let(:commit) { project.commit(sample_commit.id) } let(:diff) { commit.diffs.first } - let(:diff_file) { Gitlab::Diff::File.new(diff, [commit.parent.id, commit.id], project.repository) } + let(:diff_file) { Gitlab::Diff::File.new(diff, [commit.parent, commit]) } describe :diff_lines do let(:diff_lines) { diff_file.diff_lines } diff --git a/spec/lib/gitlab/diff/highlight_spec.rb b/spec/lib/gitlab/diff/highlight_spec.rb index 3c66c9889ba..f307dcaae44 100644 --- a/spec/lib/gitlab/diff/highlight_spec.rb +++ b/spec/lib/gitlab/diff/highlight_spec.rb @@ -6,7 +6,7 @@ describe Gitlab::Diff::Highlight, lib: true do let(:project) { create(:project) } let(:commit) { project.commit(sample_commit.id) } let(:diff) { commit.diffs.first } - let(:diff_file) { Gitlab::Diff::File.new(diff, [commit.parent.id, commit.id], project.repository) } + let(:diff_file) { Gitlab::Diff::File.new(diff, [commit.parent, commit]) } describe '.process_diff_lines' do context 'when processing Gitlab::Diff::Line objects' do From 71b4341a37dc274ce94d0b967e5b69fd49834cb8 Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Fri, 15 Jan 2016 14:11:36 +0100 Subject: [PATCH 39/57] Method was moved --- lib/gitlab/diff/highlight.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/gitlab/diff/highlight.rb b/lib/gitlab/diff/highlight.rb index fb79a2a69de..65deea9d335 100644 --- a/lib/gitlab/diff/highlight.rb +++ b/lib/gitlab/diff/highlight.rb @@ -140,11 +140,11 @@ module Gitlab end def old_lines - @old_lines ||= self.class.process_file(*processing_args(:old)) + @old_lines ||= Gitlab::Highlight.highlight_lines(*processing_args(:old)) end def new_lines - @new_lines ||= self.class.process_file(*processing_args(:new)) + @new_lines ||= Gitlab::Highlight.highlight_lines(*processing_args(:new)) end def longest_common_suffix(a, b) From 7d31f372191c38e5676cd6ec746721eb64b4a634 Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Fri, 15 Jan 2016 14:25:29 +0100 Subject: [PATCH 40/57] Move inline diff logic to its own class --- lib/gitlab/diff/highlight.rb | 84 +++++------------------------------- 1 file changed, 11 insertions(+), 73 deletions(-) diff --git a/lib/gitlab/diff/highlight.rb b/lib/gitlab/diff/highlight.rb index 65deea9d335..f89121e184b 100644 --- a/lib/gitlab/diff/highlight.rb +++ b/lib/gitlab/diff/highlight.rb @@ -24,26 +24,7 @@ module Gitlab private def find_inline_diffs - @inline_diffs = [] - - local_edit_indexes.each do |index| - old_index = index - new_index = index + 1 - old_line = @raw_lines[old_index][1..-1] - new_line = @raw_lines[new_index][1..-1] - - # Skip inline diff if empty line was replaced with content - next if old_line == "" - - lcp = longest_common_prefix(old_line, new_line) - lcs = longest_common_suffix(old_line, new_line) - - old_diff_range = lcp..(old_line.length - lcs - 1) - 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[new_index] = new_diff_range if new_diff_range.begin <= new_diff_range.end - end + @inline_diffs = InlineDiff.new(@raw_lines).inline_diffs end def process_lines @@ -58,7 +39,7 @@ module Gitlab end def highlight_line(diff_line, index) - line_prefix = line_prefixes[index] + line_prefix = diff_line.text.match(/\A([+-])/) ? $1 : ' ' case diff_line.type when 'new', nil @@ -73,46 +54,25 @@ module Gitlab end def mark_inline_diffs(rich_line, diff_line, index) - inline_diff = @inline_diffs[index] - return rich_line unless inline_diff + line_inline_diffs = @inline_diffs[index] + return rich_line unless line_inline_diffs raw_line = diff_line.text - - # Based on the prefixless versions - from = inline_diff.begin + 1 - to = inline_diff.end + 1 - position_mapping = map_character_positions(raw_line, rich_line) - inline_diff_positions = position_mapping[from..to] - marker_ranges = collapse_ranges(inline_diff_positions) offset = 0 - marker_ranges.each do |range| - offset = insert_around_range(rich_line, range, "", "", offset) + line_inline_diffs.each do |inline_diff_range| + inline_diff_positions = position_mapping[inline_diff_range] + marker_ranges = collapse_ranges(inline_diff_positions) + + marker_ranges.each do |range| + offset = insert_around_range(rich_line, range, "", "", offset) + end end rich_line end - def line_prefixes - @line_prefixes ||= @raw_lines.map { |line| line.match(/\A([+-])/) ? $1 : ' ' } - end - - def local_edit_indexes - @local_edit_indexes ||= begin - joined_line_prefixes = " #{line_prefixes.join} " - - offset = 0 - local_edit_indexes = [] - while index = joined_line_prefixes.index(" -+ ", offset) - local_edit_indexes << index - offset = index + 1 - end - - local_edit_indexes - end - end - def map_character_positions(raw_line, rich_line) mapping = [] raw_pos = 0 @@ -147,25 +107,6 @@ module Gitlab @new_lines ||= Gitlab::Highlight.highlight_lines(*processing_args(:new)) end - def longest_common_suffix(a, b) - longest_common_prefix(a.reverse, b.reverse) - end - - def longest_common_prefix(a, b) - max_length = [a.length, b.length].max - - length = 0 - (0..max_length - 1).each do |pos| - old_char = a[pos] - new_char = b[pos] - - break if old_char != new_char - length += 1 - end - - length - end - def collapse_ranges(positions) return [] if positions.empty? ranges = [] @@ -200,15 +141,12 @@ module Gitlab offset end - private - def processing_args(version) ref = send("diff_#{version}_ref") path = send("diff_#{version}_path") [ref.project.repository, ref.id, path] end - end end end From 13f10efcf120f2d5244bf6abe934dc7c026834ef Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Fri, 15 Jan 2016 14:39:43 +0100 Subject: [PATCH 41/57] Move inline diff marker logic to its own class --- lib/gitlab/diff/highlight.rb | 106 ++++------------------------------- 1 file changed, 11 insertions(+), 95 deletions(-) diff --git a/lib/gitlab/diff/highlight.rb b/lib/gitlab/diff/highlight.rb index f89121e184b..b6875f07279 100644 --- a/lib/gitlab/diff/highlight.rb +++ b/lib/gitlab/diff/highlight.rb @@ -12,32 +12,24 @@ module Gitlab end def highlight - return [] if @diff_lines.empty? - - find_inline_diffs - - process_lines - - @diff_lines - end - - private - - def find_inline_diffs - @inline_diffs = InlineDiff.new(@raw_lines).inline_diffs - end - - def process_lines @diff_lines.each_with_index do |diff_line, i| # ignore highlighting for "match" lines next if diff_line.type == 'match' rich_line = highlight_line(diff_line, i) - rich_line = mark_inline_diffs(rich_line, diff_line, i) + + 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 end + + @diff_lines end + private + def highlight_line(diff_line, index) line_prefix = diff_line.text.match(/\A([+-])/) ? $1 : ' ' @@ -53,50 +45,8 @@ module Gitlab rich_line ? line_prefix + rich_line : diff_line.text end - def mark_inline_diffs(rich_line, diff_line, index) - line_inline_diffs = @inline_diffs[index] - return rich_line unless line_inline_diffs - - raw_line = diff_line.text - position_mapping = map_character_positions(raw_line, rich_line) - - offset = 0 - line_inline_diffs.each do |inline_diff_range| - inline_diff_positions = position_mapping[inline_diff_range] - marker_ranges = collapse_ranges(inline_diff_positions) - - marker_ranges.each do |range| - offset = insert_around_range(rich_line, range, "", "", offset) - end - end - - rich_line - end - - def map_character_positions(raw_line, rich_line) - mapping = [] - raw_pos = 0 - rich_pos = 0 - (0..raw_line.length).each do |raw_pos| - raw_char = raw_line[raw_pos] - rich_char = rich_line[rich_pos] - - while rich_char == '<' - until rich_char == '>' - rich_pos += 1 - rich_char = rich_line[rich_pos] - end - - rich_pos += 1 - rich_char = rich_line[rich_pos] - end - - mapping[raw_pos] = rich_pos - - rich_pos += 1 - end - - mapping + def inline_diffs + @inline_diffs ||= InlineDiff.new(@raw_lines).inline_diffs end def old_lines @@ -107,40 +57,6 @@ module Gitlab @new_lines ||= Gitlab::Highlight.highlight_lines(*processing_args(:new)) end - def collapse_ranges(positions) - return [] if positions.empty? - ranges = [] - - start = prev = positions[0] - range = start..prev - positions[1..-1].each do |pos| - if pos == prev + 1 - range = start..pos - prev = pos - else - ranges << range - start = prev = pos - range = start..prev - end - end - ranges << range - - ranges - end - - def insert_around_range(text, range, before, after, offset = 0) - from = range.begin - to = range.end - - text.insert(offset + from, before) - offset += before.length - - text.insert(offset + to + 1, after) - offset += after.length - - offset - end - def processing_args(version) ref = send("diff_#{version}_ref") path = send("diff_#{version}_path") From 5de8971ddfa10cbc6a082316eee7377038670f75 Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Fri, 15 Jan 2016 16:02:48 +0100 Subject: [PATCH 42/57] Whoops, forgot to add files --- lib/gitlab/diff/inline_diff.rb | 75 +++++++++++++++++++++++ lib/gitlab/diff/inline_diff_marker.rb | 85 +++++++++++++++++++++++++++ 2 files changed, 160 insertions(+) create mode 100644 lib/gitlab/diff/inline_diff.rb create mode 100644 lib/gitlab/diff/inline_diff_marker.rb diff --git a/lib/gitlab/diff/inline_diff.rb b/lib/gitlab/diff/inline_diff.rb new file mode 100644 index 00000000000..bae297ab00f --- /dev/null +++ b/lib/gitlab/diff/inline_diff.rb @@ -0,0 +1,75 @@ +module Gitlab + module Diff + class InlineDiff + attr_accessor :lines + + def initialize(lines) + @lines = lines + end + + 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] + + suffixless_old_line = old_line[1..-1] + suffixless_new_line = new_line[1..-1] + + # Skip inline diff if empty line was replaced with content + next if suffixless_old_line == "" + + # Add one, because this is based on the suffixless version + lcp = longest_common_prefix(suffixless_old_line, suffixless_new_line) + 1 + lcs = longest_common_suffix(suffixless_old_line, suffixless_new_line) + + old_diff_range = lcp..(old_line.length - lcs - 1) + 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[new_index] = [new_diff_range] if new_diff_range.begin <= new_diff_range.end + end + + inline_diffs + end + + private + + def local_edit_indexes + line_prefixes = @lines.map { |line| line.match(/\A([+-])/) ? $1 : ' ' } + joined_line_prefixes = " #{line_prefixes.join} " + + offset = 0 + local_edit_indexes = [] + while index = joined_line_prefixes.index(" -+ ", offset) + local_edit_indexes << index + offset = index + 1 + end + + local_edit_indexes + end + + def longest_common_prefix(a, b) + max_length = [a.length, b.length].max + + length = 0 + (0..max_length - 1).each do |pos| + old_char = a[pos] + new_char = b[pos] + + break if old_char != new_char + length += 1 + end + + length + end + + def longest_common_suffix(a, b) + longest_common_prefix(a.reverse, b.reverse) + end + end + end +end diff --git a/lib/gitlab/diff/inline_diff_marker.rb b/lib/gitlab/diff/inline_diff_marker.rb new file mode 100644 index 00000000000..4bb755e3d3d --- /dev/null +++ b/lib/gitlab/diff/inline_diff_marker.rb @@ -0,0 +1,85 @@ +module Gitlab + module Diff + class InlineDiffMarker + attr_accessor :raw_line, :rich_line + + def initialize(raw_line, rich_line = raw_line) + @raw_line = raw_line + @rich_line = rich_line + end + + def mark(line_inline_diffs) + offset = 0 + line_inline_diffs.each do |inline_diff_range| + inline_diff_positions = position_mapping[inline_diff_range] + marker_ranges = collapse_ranges(inline_diff_positions) + + marker_ranges.each do |range| + offset = insert_around_range(rich_line, range, "", "", offset) + end + end + + rich_line + end + + def position_mapping + @position_mapping ||= begin + mapping = [] + raw_pos = 0 + rich_pos = 0 + (0..raw_line.length).each do |raw_pos| + raw_char = raw_line[raw_pos] + rich_char = rich_line[rich_pos] + + while rich_char == '<' + until rich_char == '>' + rich_pos += 1 + rich_char = rich_line[rich_pos] + end + + rich_pos += 1 + rich_char = rich_line[rich_pos] + end + + mapping[raw_pos] = rich_pos + + rich_pos += 1 + end + + mapping + end + end + + def collapse_ranges(positions) + return [] if positions.empty? + ranges = [] + + start = prev = positions[0] + range = start..prev + positions[1..-1].each do |pos| + if pos == prev + 1 + range = start..pos + prev = pos + else + ranges << range + start = prev = pos + range = start..prev + end + end + ranges << range + + ranges + end + + def insert_around_range(text, range, before, after, offset = 0) + text.insert(offset + range.begin, before) + offset += before.length + + text.insert(offset + range.end + 1, after) + offset += after.length + + offset + end + end + end +end From 12546f895f86929ffe69299a02c93692370ee55a Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Fri, 15 Jan 2016 16:29:26 +0100 Subject: [PATCH 43/57] Add tests --- lib/gitlab/diff/inline_diff.rb | 1 + lib/gitlab/diff/inline_diff_marker.rb | 10 +++++++ .../gitlab/diff/inline_diff_marker_spec.rb | 15 +++++++++++ spec/lib/gitlab/diff/inline_diff_spec.rb | 27 +++++++++++++++++++ 4 files changed, 53 insertions(+) create mode 100644 spec/lib/gitlab/diff/inline_diff_marker_spec.rb create mode 100644 spec/lib/gitlab/diff/inline_diff_spec.rb diff --git a/lib/gitlab/diff/inline_diff.rb b/lib/gitlab/diff/inline_diff.rb index bae297ab00f..e5986fd69e2 100644 --- a/lib/gitlab/diff/inline_diff.rb +++ b/lib/gitlab/diff/inline_diff.rb @@ -38,6 +38,7 @@ module Gitlab private + # Find runs of single line edits def local_edit_indexes line_prefixes = @lines.map { |line| line.match(/\A([+-])/) ? $1 : ' ' } joined_line_prefixes = " #{line_prefixes.join} " diff --git a/lib/gitlab/diff/inline_diff_marker.rb b/lib/gitlab/diff/inline_diff_marker.rb index 4bb755e3d3d..405465a641f 100644 --- a/lib/gitlab/diff/inline_diff_marker.rb +++ b/lib/gitlab/diff/inline_diff_marker.rb @@ -11,9 +11,12 @@ module Gitlab def mark(line_inline_diffs) offset = 0 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 inline_diff_positions = position_mapping[inline_diff_range] + # Turn the array of character positions into ranges marker_ranges = collapse_ranges(inline_diff_positions) + # Mark each range marker_ranges.each do |range| offset = insert_around_range(rich_line, range, "", "", offset) end @@ -22,6 +25,9 @@ module Gitlab rich_line end + private + + # Mapping of character positions in the raw line, to the rich (highlighted) line def position_mapping @position_mapping ||= begin mapping = [] @@ -31,6 +37,8 @@ module Gitlab raw_char = raw_line[raw_pos] rich_char = rich_line[rich_pos] + # The raw and rich lines are the same except for HTML tags, + # so skip over any `<...>` segment while rich_char == '<' until rich_char == '>' rich_pos += 1 @@ -50,6 +58,7 @@ module Gitlab end end + # Takes an array of integers, and returns an array of ranges covering the same integers def collapse_ranges(positions) return [] if positions.empty? ranges = [] @@ -71,6 +80,7 @@ module Gitlab ranges end + # Inserts tags around the characters identified by the given range def insert_around_range(text, range, before, after, offset = 0) text.insert(offset + range.begin, before) offset += before.length diff --git a/spec/lib/gitlab/diff/inline_diff_marker_spec.rb b/spec/lib/gitlab/diff/inline_diff_marker_spec.rb new file mode 100644 index 00000000000..5d6aea17509 --- /dev/null +++ b/spec/lib/gitlab/diff/inline_diff_marker_spec.rb @@ -0,0 +1,15 @@ +require 'spec_helper' + +describe Gitlab::Diff::InlineDiffMarker, lib: true do + describe '#inline_diffs' do + let(:raw) { "abc def" } + let(:rich) { %{abc def} } + let(:inline_diffs) { [2..4] } + + let(:subject) { Gitlab::Diff::InlineDiffMarker.new(raw, rich).mark(inline_diffs) } + + it 'marks the inline diffs' do + expect(subject).to eq(%{abc def}) + end + end +end diff --git a/spec/lib/gitlab/diff/inline_diff_spec.rb b/spec/lib/gitlab/diff/inline_diff_spec.rb new file mode 100644 index 00000000000..056917df893 --- /dev/null +++ b/spec/lib/gitlab/diff/inline_diff_spec.rb @@ -0,0 +1,27 @@ +require 'spec_helper' + +describe Gitlab::Diff::InlineDiff, lib: true do + describe '#inline_diffs' do + let(:diff) do + < Date: Tue, 19 Jan 2016 13:56:36 +0100 Subject: [PATCH 44/57] Remove useless assignments --- lib/gitlab/diff/inline_diff_marker.rb | 2 -- 1 file changed, 2 deletions(-) diff --git a/lib/gitlab/diff/inline_diff_marker.rb b/lib/gitlab/diff/inline_diff_marker.rb index 405465a641f..8998ccba5ce 100644 --- a/lib/gitlab/diff/inline_diff_marker.rb +++ b/lib/gitlab/diff/inline_diff_marker.rb @@ -31,10 +31,8 @@ module Gitlab def position_mapping @position_mapping ||= begin mapping = [] - raw_pos = 0 rich_pos = 0 (0..raw_line.length).each do |raw_pos| - raw_char = raw_line[raw_pos] rich_char = rich_line[rich_pos] # The raw and rich lines are the same except for HTML tags, From 512bebe21d7f57b691a1c8355581feb64b9b6292 Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Tue, 19 Jan 2016 14:52:41 +0100 Subject: [PATCH 45/57] Refactor Gitlab::Highlight and fix tests --- app/helpers/blob_helper.rb | 8 +- app/views/projects/blame/show.html.haml | 5 +- app/views/shared/_file_highlight.html.haml | 3 +- lib/gitlab/highlight.rb | 26 ++++-- spec/fixtures/parallel_diff_result.yml | 98 +++++++++++----------- spec/helpers/blob_helper_spec.rb | 63 +++++++------- spec/lib/gitlab/diff/highlight_spec.rb | 65 +++++--------- 7 files changed, 132 insertions(+), 136 deletions(-) diff --git a/app/helpers/blob_helper.rb b/app/helpers/blob_helper.rb index be856242c43..7c55934edda 100644 --- a/app/helpers/blob_helper.rb +++ b/app/helpers/blob_helper.rb @@ -1,6 +1,10 @@ module BlobHelper - def highlight(blob_name, blob_content, nowrap: false, continue: false) - Gitlab::Highlight.highlight(blob_name, blob_content, nowrap: nowrap, continue: continue) + def highlighter(blob_name, blob_content, nowrap: false) + Gitlab::Highlight.new(blob_name, blob_content, nowrap: nowrap) + end + + def highlight(blob_name, blob_content, nowrap: false) + Gitlab::Highlight.highlight(blob_name, blob_content, nowrap: nowrap) end def no_highlight_files diff --git a/app/views/projects/blame/show.html.haml b/app/views/projects/blame/show.html.haml index 8d9ec068a43..d5d04954490 100644 --- a/app/views/projects/blame/show.html.haml +++ b/app/views/projects/blame/show.html.haml @@ -15,6 +15,7 @@ .file-content.blame.highlight %table - current_line = 1 + - blame_highlighter = highlighter(@blob.name, @blob.data, nowrap: true) - @blame.each do |blame_group| %tr %td.blame-commit @@ -41,5 +42,5 @@ %pre{class: 'code highlight white'} %code - blame_group[:lines].each do |line| - :erb - <%= highlight(@blob.name, line, nowrap: true, continue: true).html_safe %> + :preserve + #{blame_highlighter.highlight(line)} diff --git a/app/views/shared/_file_highlight.html.haml b/app/views/shared/_file_highlight.html.haml index 2bc98983d67..1bef1de433a 100644 --- a/app/views/shared/_file_highlight.html.haml +++ b/app/views/shared/_file_highlight.html.haml @@ -9,5 +9,4 @@ %i.fa.fa-link = i .blob-content{data: {blob_id: blob.id}} - :preserve - #{highlight(blob.name, blob.data)} + = highlight(blob.name, blob.data) diff --git a/lib/gitlab/highlight.rb b/lib/gitlab/highlight.rb index a5b041687e3..28cfebef968 100644 --- a/lib/gitlab/highlight.rb +++ b/lib/gitlab/highlight.rb @@ -1,10 +1,7 @@ module Gitlab class Highlight - def self.highlight(blob_name, blob_content, nowrap: true, continue: false) - formatter = rouge_formatter(nowrap: nowrap) - - lexer = Rouge::Lexer.guess(filename: blob_name, source: blob_content).new rescue Rouge::Lexers::PlainText - formatter.format(lexer.lex(blob_content, continue: continue)).html_safe + def self.highlight(blob_name, blob_content, nowrap: true) + new(blob_name, blob_content, nowrap: nowrap).highlight(blob_content, continue: false) end def self.highlight_lines(repository, ref, file_name) @@ -14,9 +11,26 @@ module Gitlab highlight(file_name, blob.data).lines.map!(&:html_safe) end + def initialize(blob_name, blob_content, nowrap: true) + @formatter = rouge_formatter(nowrap: nowrap) + @lexer = Rouge::Lexer.guess(filename: blob_name, source: blob_content).new rescue Rouge::Lexers::PlainText + end + + def highlight(text, continue: true) + @formatter.format(lex(text, continue: continue)).html_safe + end + private - def self.rouge_formatter(options = {}) + def lex(text, continue: true) + if @lexer == Rouge::Lexers::PlainText + @lexer.lex(text) + else + @lexer.lex(text, continue: continue) + end + end + + def rouge_formatter(options = {}) options = options.reverse_merge( nowrap: true, cssclass: 'code highlight', diff --git a/spec/fixtures/parallel_diff_result.yml b/spec/fixtures/parallel_diff_result.yml index 4583de1231e..0fb9bf6c5d8 100644 --- a/spec/fixtures/parallel_diff_result.yml +++ b/spec/fixtures/parallel_diff_result.yml @@ -10,37 +10,37 @@ :text: "@@ -6,12 +6,18 @@ module Popen" :line_code: 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_6_6 - :left: - :type: + :type: :number: 6 :text: |2 :line_code: 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_6_6 :right: - :type: + :type: :number: 6 :text: |2 :line_code: 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_6_6 - :left: - :type: + :type: :number: 7 :text: |2 def popen(cmd, path=nil) :line_code: 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_7_7 :right: - :type: + :type: :number: 7 :text: |2 def popen(cmd, path=nil) :line_code: 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_7_7 - :left: - :type: + :type: :number: 8 :text: |2 unless cmd.is_a?(Array) :line_code: 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_8_8 :right: - :type: + :type: :number: 8 :text: |2 unless cmd.is_a?(Array) @@ -55,40 +55,40 @@ :type: new :number: 9 :text: | - + raise RuntimeError, "System commands must be given as an array of strings" + + raise RuntimeError, "System commands must be given as an array of strings" :line_code: 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_10_9 - :left: - :type: + :type: :number: 10 :text: |2 end :line_code: 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_10_10 :right: - :type: + :type: :number: 10 :text: |2 end :line_code: 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_10_10 - :left: - :type: + :type: :number: 11 :text: |2 :line_code: 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_11_11 :right: - :type: + :type: :number: 11 :text: |2 :line_code: 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_11_11 - :left: - :type: + :type: :number: 12 :text: |2 path ||= Dir.pwd :line_code: 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_12_12 :right: - :type: + :type: :number: 12 :text: |2 path ||= Dir.pwd @@ -101,9 +101,9 @@ :line_code: 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_13_13 :right: :type: old - :number: + :number: :text: '' - :line_code: + :line_code: - :left: :type: old :number: 14 @@ -117,8 +117,8 @@ + :line_code: 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_15_13 - :left: - :type: - :number: + :type: + :number: :text: '' :line_code: 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_15_14 :right: @@ -128,8 +128,8 @@ + vars = { :line_code: 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_15_14 - :left: - :type: - :number: + :type: + :number: :text: '' :line_code: 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_15_15 :right: @@ -139,8 +139,8 @@ + "PWD" => path :line_code: 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_15_15 - :left: - :type: - :number: + :type: + :number: :text: '' :line_code: 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_15_16 :right: @@ -150,8 +150,8 @@ + } :line_code: 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_15_16 - :left: - :type: - :number: + :type: + :number: :text: '' :line_code: 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_15_17 :right: @@ -161,8 +161,8 @@ + :line_code: 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_15_17 - :left: - :type: - :number: + :type: + :number: :text: '' :line_code: 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_15_18 :right: @@ -172,8 +172,8 @@ + options = { :line_code: 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_15_18 - :left: - :type: - :number: + :type: + :number: :text: '' :line_code: 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_15_19 :right: @@ -183,8 +183,8 @@ + chdir: path :line_code: 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_15_19 - :left: - :type: - :number: + :type: + :number: :text: '' :line_code: 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_15_20 :right: @@ -194,37 +194,37 @@ + } :line_code: 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_15_20 - :left: - :type: + :type: :number: 15 :text: |2 :line_code: 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_15_21 :right: - :type: + :type: :number: 21 :text: |2 :line_code: 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_15_21 - :left: - :type: + :type: :number: 16 :text: |2 unless File.directory?(path) :line_code: 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_16_22 :right: - :type: + :type: :number: 22 :text: |2 unless File.directory?(path) :line_code: 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_16_22 - :left: - :type: + :type: :number: 17 :text: |2 FileUtils.mkdir_p(path) :line_code: 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_17_23 :right: - :type: + :type: :number: 23 :text: |2 FileUtils.mkdir_p(path) @@ -240,44 +240,44 @@ :text: "@@ -19,6 +25,7 @@ module Popen" :line_code: 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_19_25 - :left: - :type: + :type: :number: 19 :text: |2 :line_code: 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_19_25 :right: - :type: + :type: :number: 25 :text: |2 :line_code: 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_19_25 - :left: - :type: + :type: :number: 20 :text: |2 @cmd_output = "" :line_code: 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_20_26 :right: - :type: + :type: :number: 26 :text: |2 @cmd_output = "" :line_code: 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_20_26 - :left: - :type: + :type: :number: 21 :text: |2 @cmd_status = 0 :line_code: 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_21_27 :right: - :type: + :type: :number: 27 :text: |2 @cmd_status = 0 :line_code: 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_21_27 - :left: - :type: - :number: + :type: + :number: :text: '' :line_code: 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_22_28 :right: @@ -287,37 +287,37 @@ + :line_code: 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_22_28 - :left: - :type: + :type: :number: 22 :text: |2 Open3.popen3(vars, *cmd, options) do |stdin, stdout, stderr, wait_thr| :line_code: 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_22_29 :right: - :type: + :type: :number: 29 :text: |2 Open3.popen3(vars, *cmd, options) do |stdin, stdout, stderr, wait_thr| :line_code: 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_22_29 - :left: - :type: + :type: :number: 23 :text: |2 @cmd_output << stdout.read :line_code: 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_23_30 :right: - :type: + :type: :number: 30 :text: |2 @cmd_output << stdout.read :line_code: 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_23_30 - :left: - :type: + :type: :number: 24 :text: |2 @cmd_output << stderr.read :line_code: 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_24_31 :right: - :type: + :type: :number: 31 :text: |2 @cmd_output << stderr.read diff --git a/spec/helpers/blob_helper_spec.rb b/spec/helpers/blob_helper_spec.rb index b8bba36439a..87849230dbe 100644 --- a/spec/helpers/blob_helper_spec.rb +++ b/spec/helpers/blob_helper_spec.rb @@ -1,22 +1,22 @@ require 'spec_helper' describe BlobHelper do - describe 'highlight' do - let(:blob_name) { 'test.lisp' } - let(:no_context_content) { ":type \"assem\"))" } - let(:blob_content) { "(make-pathname :defaults name\n#{no_context_content}" } - let(:split_content) { blob_content.split("\n") } - let(:multiline_content) do - %q( - def test(input): - """This is line 1 of a multi-line comment. - This is line 2. - """ - ) - end + let(:blob_name) { 'test.lisp' } + let(:no_context_content) { ":type \"assem\"))" } + let(:blob_content) { "(make-pathname :defaults name\n#{no_context_content}" } + let(:split_content) { blob_content.split("\n") } + let(:multiline_content) do + %q( + def test(input): + """This is line 1 of a multi-line comment. + This is line 2. + """ + ) + end + describe '#highlight' do it 'should return plaintext for unknown lexer context' do - result = highlight(blob_name, no_context_content, nowrap: true, continue: false) + result = helper.highlight(blob_name, no_context_content, nowrap: true) expect(result).to eq(':type "assem"))') end @@ -24,28 +24,17 @@ describe BlobHelper do expected = %Q[(make-pathname :defaults name :type "assem"))] - expect(highlight(blob_name, blob_content, nowrap: true, continue: false)).to eq(expected) - end - - it 'should highlight continued blocks' do - # Both lines have LC1 as ID since formatter doesn't support continue at the moment - expected = [ - '(make-pathname :defaults name', - ':type "assem"))' - ] - - result = split_content.map{ |content| highlight(blob_name, content, nowrap: true, continue: true) } - expect(result).to eq(expected) + expect(helper.highlight(blob_name, blob_content, nowrap: true)).to eq(expected) end it 'should highlight multi-line comments' do - result = highlight(blob_name, multiline_content, nowrap: true, continue: false) + result = helper.highlight(blob_name, multiline_content, nowrap: true) html = Nokogiri::HTML(result) lines = html.search('.s') expect(lines.count).to eq(3) expect(lines[0].text).to eq('"""This is line 1 of a multi-line comment.') - expect(lines[1].text).to eq(' This is line 2.') - expect(lines[2].text).to eq(' """') + expect(lines[1].text).to eq(' This is line 2.') + expect(lines[2].text).to eq(' """') end context 'diff highlighting' do @@ -59,9 +48,23 @@ describe BlobHelper do end it 'should highlight each line properly' do - result = highlight(blob_name, blob_content, nowrap: true, continue: false) + result = helper.highlight(blob_name, blob_content, nowrap: true) expect(result).to eq(expected) end end end + + describe "#highlighter" do + it 'should highlight continued blocks' do + # Both lines have LC1 as ID since formatter doesn't support continue at the moment + expected = [ + '(make-pathname :defaults name', + ':type "assem"))' + ] + + highlighter = helper.highlighter(blob_name, blob_content, nowrap: true) + result = split_content.map{ |content| highlighter.highlight(content) } + expect(result).to eq(expected) + end + end end diff --git a/spec/lib/gitlab/diff/highlight_spec.rb b/spec/lib/gitlab/diff/highlight_spec.rb index f307dcaae44..b54e95483d3 100644 --- a/spec/lib/gitlab/diff/highlight_spec.rb +++ b/spec/lib/gitlab/diff/highlight_spec.rb @@ -8,59 +8,34 @@ describe Gitlab::Diff::Highlight, lib: true do let(:diff) { commit.diffs.first } let(:diff_file) { Gitlab::Diff::File.new(diff, [commit.parent, commit]) } - describe '.process_diff_lines' do - context 'when processing Gitlab::Diff::Line objects' do - let(:diff_lines) { Gitlab::Diff::Highlight.process_diff_lines(diff_file) } + describe '#highlight' do + let(:diff_lines) { 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(diff_lines.first).to be_an_instance_of(Gitlab::Diff::Line) + end - it 'should highlight the code' do - code = %Q{ def popen(cmd, path=nil)\n} + 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 - expect(diff_lines[2].text).to eq(code) - end + it 'should highlight unchanged lines' do + code = %Q{ def popen(cmd, path=nil)\n} - it 'should not generate the inline diff markup' do - expect(diff_lines[5].text).not_to match(Regexp.new(Regexp.escape(''))) - end + expect(diff_lines[2].text).to eq(code) + 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 highlight removed lines' do + code = %Q{- raise "System commands must be given as an array of strings"\n} - it 'should highlight unchanged lines' do - code = %Q{ def popen(cmd, path=nil)\n} + expect(diff_lines[4].text).to eq(code) + end - expect(diff_lines[2].text).to eq(code) - end + it 'should highlight added lines' do + code = %Q{+ raise RuntimeError, "System commands must be given as an array of strings"\n} - it 'should highlight added lines' do - code = %Q{+ raise RuntimeError, "System commands must be given as an array of strings"\n} - - expect(diff_lines[5].text).to eq(code) - end - - it 'should highlight removed lines' do - code = %Q{- raise "System commands must be given as an array of strings"\n} - - expect(diff_lines[4].text).to eq(code) - end + expect(diff_lines[5].text).to eq(code) end end - - describe '.highlight_lines' do - let(:lines) do - Gitlab::Diff::Highlight.highlight_lines(project.repository, commit.id, 'files/ruby/popen.rb') - end - - it 'should properly highlight all the lines' do - expect(lines[4]).to eq(%Q{ extend self\n}) - expect(lines[21]).to eq(%Q{ unless File.directory?(path)\n}) - expect(lines[26]).to eq(%Q{ @cmd_status = 0\n}) - end - end - end From 5c7259c7c30228bf85a4efbbddb673046e8d733f Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Tue, 19 Jan 2016 15:13:37 +0100 Subject: [PATCH 46/57] Don't crash when file can't be highlighted --- lib/gitlab/highlight.rb | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) diff --git a/lib/gitlab/highlight.rb b/lib/gitlab/highlight.rb index 28cfebef968..4ddb4fea977 100644 --- a/lib/gitlab/highlight.rb +++ b/lib/gitlab/highlight.rb @@ -17,19 +17,13 @@ module Gitlab end def highlight(text, continue: true) - @formatter.format(lex(text, continue: continue)).html_safe + @formatter.format(@lexer.lex(text, continue: continue)).html_safe + rescue + @formatter.format(Rouge::Lexers::PlainText.lex(text)).html_safe end private - def lex(text, continue: true) - if @lexer == Rouge::Lexers::PlainText - @lexer.lex(text) - else - @lexer.lex(text, continue: continue) - end - end - def rouge_formatter(options = {}) options = options.reverse_merge( nowrap: true, From d1938fae3d9a6f4d31576461fae3efb4f09686c4 Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Tue, 19 Jan 2016 16:26:44 +0100 Subject: [PATCH 47/57] Add missing specs --- spec/lib/gitlab/highlight_spec.rb | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) create mode 100644 spec/lib/gitlab/highlight_spec.rb diff --git a/spec/lib/gitlab/highlight_spec.rb b/spec/lib/gitlab/highlight_spec.rb new file mode 100644 index 00000000000..1620eb6c60a --- /dev/null +++ b/spec/lib/gitlab/highlight_spec.rb @@ -0,0 +1,21 @@ +require 'spec_helper' + +describe Gitlab::Highlight, lib: true do + include RepoHelpers + + let(:project) { create(:project) } + let(:commit) { project.commit(sample_commit.id) } + + describe '.highlight_lines' do + let(:lines) do + Gitlab::Highlight.highlight_lines(project.repository, commit.id, 'files/ruby/popen.rb') + end + + it 'should properly highlight all the lines' do + expect(lines[4]).to eq(%Q{ extend self\n}) + expect(lines[21]).to eq(%Q{ unless File.directory?(path)\n}) + expect(lines[26]).to eq(%Q{ @cmd_status = 0\n}) + end + end + +end From c7264d2a76abdc9d173e2160e27974d41380e93f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rub=C3=A9n=20D=C3=A1vila?= Date: Tue, 19 Jan 2016 11:40:43 -0500 Subject: [PATCH 48/57] Check if MR is not broken. --- app/models/merge_request.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index a9fc6bc167a..9511521879b 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -254,7 +254,7 @@ class MergeRequest < ActiveRecord::Base end def mergeable? - return false unless open? && !work_in_progress? + return false unless open? && !work_in_progress? && !broken? check_if_can_be_merged From 701513dcc7afb403372bc738642a9a52e9be5983 Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Wed, 20 Jan 2016 14:51:56 +0100 Subject: [PATCH 49/57] Move parallel diff logic to separate class --- app/helpers/diff_helper.rb | 104 ---------------- .../projects/diffs/_parallel_view.html.haml | 6 +- app/views/projects/diffs/_text_file.html.haml | 2 +- .../notes/discussions/_diff.html.haml | 2 +- lib/gitlab/diff/file.rb | 4 + lib/gitlab/diff/parallel_diff.rb | 117 ++++++++++++++++++ spec/helpers/diff_helper_spec.rb | 18 --- spec/lib/gitlab/diff/parallel_diff_spec.rb | 22 ++++ 8 files changed, 148 insertions(+), 127 deletions(-) create mode 100644 lib/gitlab/diff/parallel_diff.rb create mode 100644 spec/lib/gitlab/diff/parallel_diff_spec.rb diff --git a/app/helpers/diff_helper.rb b/app/helpers/diff_helper.rb index 425a8ced549..e524f6da9c8 100644 --- a/app/helpers/diff_helper.rb +++ b/app/helpers/diff_helper.rb @@ -39,110 +39,6 @@ module DiffHelper end end - def generate_line_code(file_path, line) - Gitlab::Diff::LineCode.generate(file_path, line.new_pos, line.old_pos) - end - - def parallel_diff(diff_file, index) - lines = [] - skip_next = false - - diff_file.highlighted_diff_lines.each do |line| - full_line = line.text - type = line.type - line_code = generate_line_code(diff_file.file_path, line) - line_new = line.new_pos - line_old = line.old_pos - - next_line = diff_file.next_line(line.index) - - if next_line - next_line_code = generate_line_code(diff_file.file_path, next_line) - next_type = next_line.type - next_line = next_line.text - end - - case type - when 'match', nil - # line in the right panel is the same as in the left one - lines << { - left: { - type: type, - number: line_old, - text: full_line, - line_code: line_code, - }, - right: { - type: type, - number: line_new, - text: full_line, - line_code: line_code - } - } - when 'old' - case next_type - when 'new' - # Left side has text removed, right side has text added - lines << { - left: { - type: type, - number: line_old, - text: full_line, - line_code: line_code, - }, - right: { - type: next_type, - number: line_new, - text: next_line, - line_code: next_line_code - } - } - skip_next = true - when 'old', nil - # Left side has text removed, right side doesn't have any change - # No next line code, no new line number, no new line text - lines << { - left: { - type: type, - number: line_old, - text: full_line, - line_code: line_code, - }, - right: { - type: next_type, - number: nil, - text: "", - line_code: nil - } - } - end - when 'new' - if skip_next - # Change has been already included in previous line so no need to do it again - skip_next = false - next - else - # Change is only on the right side, left side has no change - lines << { - left: { - type: nil, - number: nil, - text: "", - line_code: line_code, - }, - right: { - type: type, - number: line_new, - text: full_line, - line_code: line_code - } - } - end - end - end - lines - end - def unfold_bottom_class(bottom) (bottom) ? 'js-unfold-bottom' : '' end diff --git a/app/views/projects/diffs/_parallel_view.html.haml b/app/views/projects/diffs/_parallel_view.html.haml index 7b9cecbc3da..986d30d36a8 100644 --- a/app/views/projects/diffs/_parallel_view.html.haml +++ b/app/views/projects/diffs/_parallel_view.html.haml @@ -1,7 +1,7 @@ / Side-by-side diff view %div.text-file.diff-wrap-lines.code.file-content.js-syntax-highlight %table - - parallel_diff(diff_file, index).each do |line| + - diff_file.parallel_diff_lines.each do |line| - left = line[:left] - right = line[:right] %tr.line_holder.parallel @@ -13,7 +13,7 @@ = link_to raw(left[:number]), "##{left[:line_code]}", id: left[:line_code] - if @comments_allowed && can?(current_user, :create_note, @project) = link_to_new_diff_note(left[:line_code], 'old') - %td.line_content{class: "parallel noteable_line #{left[:type]} #{left[:line_code]}", data: { "line_code" => left[:line_code] }}= diff_line_content(left[:text]) + %td.line_content{class: "parallel noteable_line #{left[:type]} #{left[:line_code]}", data: { line_code: left[:line_code] }}= diff_line_content(left[:text]) - if right[:type] == 'new' - new_line_class = 'new' @@ -26,7 +26,7 @@ = link_to raw(right[:number]), "##{new_line_code}", id: new_line_code - if @comments_allowed && can?(current_user, :create_note, @project) = link_to_new_diff_note(right[:line_code], 'new') - %td.line_content.parallel{class: "noteable_line #{new_line_class} #{new_line_code}", data: { "line_code" => new_line_code }}= diff_line_content(right[:text]) + %td.line_content.parallel{class: "noteable_line #{new_line_class} #{new_line_code}", data: { line_code: new_line_code }}= diff_line_content(right[:text]) - if @reply_allowed - comments_left, comments_right = organize_comments(left[:type], right[:type], left[:line_code], right[:line_code]) diff --git a/app/views/projects/diffs/_text_file.html.haml b/app/views/projects/diffs/_text_file.html.haml index 6761155dcf9..310bbd4efea 100644 --- a/app/views/projects/diffs/_text_file.html.haml +++ b/app/views/projects/diffs/_text_file.html.haml @@ -22,7 +22,7 @@ = link_to_new_diff_note(line_code) %td.new_line{data: {linenumber: line.new_pos}} = link_to raw(type == "old" ? " " : line.new_pos), "##{line_code}", id: line_code - %td.line_content{class: "noteable_line #{type} #{line_code}", data: { "line_code" => line_code }}= diff_line_content(line.text) + %td.line_content{class: "noteable_line #{type} #{line_code}", data: { line_code: line_code }}= diff_line_content(line.text) - if @reply_allowed - comments = @line_notes.select { |n| n.line_code == line_code && n.active? }.sort_by(&:created_at) diff --git a/app/views/projects/notes/discussions/_diff.html.haml b/app/views/projects/notes/discussions/_diff.html.haml index 97347a9f67f..46962b184ce 100644 --- a/app/views/projects/notes/discussions/_diff.html.haml +++ b/app/views/projects/notes/discussions/_diff.html.haml @@ -24,7 +24,7 @@ = raw(type == "new" ? " " : line.old_pos) %td.new_line = raw(type == "old" ? " " : line.new_pos) - %td.line_content{class: "noteable_line #{type} #{line_code}", "line_code" => line_code}= diff_line_content(line.text) + %td.line_content{class: "noteable_line #{type} #{line_code}", line_code: line_code}= diff_line_content(line.text) - if line_code == note.line_code = render "projects/notes/diff_notes_with_reply", notes: discussion_notes diff --git a/lib/gitlab/diff/file.rb b/lib/gitlab/diff/file.rb index a6a7fc8ff4c..74b1c117129 100644 --- a/lib/gitlab/diff/file.rb +++ b/lib/gitlab/diff/file.rb @@ -20,6 +20,10 @@ module Gitlab Gitlab::Diff::Highlight.new(self).highlight end + def parallel_diff_lines + Gitlab::Diff::ParallelDiff.new(self).parallelize + end + def mode_changed? !!(diff.a_mode && diff.b_mode && diff.a_mode != diff.b_mode) end diff --git a/lib/gitlab/diff/parallel_diff.rb b/lib/gitlab/diff/parallel_diff.rb new file mode 100644 index 00000000000..a0dc3da875d --- /dev/null +++ b/lib/gitlab/diff/parallel_diff.rb @@ -0,0 +1,117 @@ +module Gitlab + module Diff + class ParallelDiff + attr_accessor :diff_file + + def initialize(diff_file) + @diff_file = diff_file + end + + def parallelize + lines = [] + skip_next = false + + diff_file.highlighted_diff_lines.each do |line| + full_line = line.text + type = line.type + line_code = generate_line_code(diff_file.file_path, line) + line_new = line.new_pos + line_old = line.old_pos + + next_line = diff_file.next_line(line.index) + + if next_line + next_line_code = generate_line_code(diff_file.file_path, next_line) + next_type = next_line.type + next_line = next_line.text + end + + case type + when 'match', nil + # line in the right panel is the same as in the left one + lines << { + left: { + type: type, + number: line_old, + text: full_line, + line_code: line_code, + }, + right: { + type: type, + number: line_new, + text: full_line, + line_code: line_code + } + } + when 'old' + case next_type + when 'new' + # Left side has text removed, right side has text added + lines << { + left: { + type: type, + number: line_old, + text: full_line, + line_code: line_code, + }, + right: { + type: next_type, + number: line_new, + text: next_line, + line_code: next_line_code + } + } + skip_next = true + when 'old', nil + # Left side has text removed, right side doesn't have any change + # No next line code, no new line number, no new line text + lines << { + left: { + type: type, + number: line_old, + text: full_line, + line_code: line_code, + }, + right: { + type: next_type, + number: nil, + text: "", + line_code: nil + } + } + end + when 'new' + if skip_next + # Change has been already included in previous line so no need to do it again + skip_next = false + next + else + # Change is only on the right side, left side has no change + lines << { + left: { + type: nil, + number: nil, + text: "", + line_code: line_code, + }, + right: { + type: type, + number: line_new, + text: full_line, + line_code: line_code + } + } + end + end + end + lines + end + + private + + def generate_line_code(file_path, line) + Gitlab::Diff::LineCode.generate(file_path, line.new_pos, line.old_pos) + end + end + end +end diff --git a/spec/helpers/diff_helper_spec.rb b/spec/helpers/diff_helper_spec.rb index 435ecd04fbe..955d2852cfd 100644 --- a/spec/helpers/diff_helper_spec.rb +++ b/spec/helpers/diff_helper_spec.rb @@ -84,20 +84,6 @@ describe DiffHelper do end end - describe 'parallel_diff' do - it 'should return an array of arrays containing the parsed diff' do - expect(parallel_diff(diff_file, 0)). - to match_array(parallel_diff_result_array) - end - end - - describe 'generate_line_code' do - it 'should generate correct line code' do - expect(generate_line_code(diff_file.file_path, diff_file.diff_lines.first)). - to eq('2f6fcd96b88b36ce98c38da085c795a27d92a3dd_6_6') - end - end - describe 'unfold_bottom_class' do it 'should return empty string when bottom line shouldnt be unfolded' do expect(unfold_bottom_class(false)).to eq('') @@ -135,8 +121,4 @@ describe DiffHelper do expect(diff_line_content(diff_file.diff_lines.first.text)).to be_html_safe end end - - def parallel_diff_result_array - YAML.load_file("#{Rails.root}/spec/fixtures/parallel_diff_result.yml") - end end diff --git a/spec/lib/gitlab/diff/parallel_diff_spec.rb b/spec/lib/gitlab/diff/parallel_diff_spec.rb new file mode 100644 index 00000000000..852218b4e62 --- /dev/null +++ b/spec/lib/gitlab/diff/parallel_diff_spec.rb @@ -0,0 +1,22 @@ +require 'spec_helper' + +describe Gitlab::Diff::ParallelDiff, lib: true do + include RepoHelpers + + let(:project) { create(:project) } + let(:repository) { project.repository } + let(:commit) { project.commit(sample_commit.id) } + let(:diffs) { commit.diffs } + let(:diff) { diffs.first } + let(:diff_refs) { [commit.parent, commit] } + let(:diff_file) { Gitlab::Diff::File.new(diff, diff_refs) } + subject { described_class.new(diff_file) } + + let(:parallel_diff_result_array) { YAML.load_file("#{Rails.root}/spec/fixtures/parallel_diff_result.yml") } + + describe '#parallelize' do + it 'should return an array of arrays containing the parsed diff' do + expect(subject.parallelize).to match_array(parallel_diff_result_array) + end + end +end From 88bd13851300b98c9d0290cfac5ad1f14673b34d Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Wed, 20 Jan 2016 14:59:14 +0100 Subject: [PATCH 50/57] Restore helper --- app/helpers/diff_helper.rb | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/app/helpers/diff_helper.rb b/app/helpers/diff_helper.rb index e524f6da9c8..f500d3572e0 100644 --- a/app/helpers/diff_helper.rb +++ b/app/helpers/diff_helper.rb @@ -39,6 +39,10 @@ module DiffHelper end end + def generate_line_code(file_path, line) + Gitlab::Diff::LineCode.generate(file_path, line.new_pos, line.old_pos) + end + def unfold_bottom_class(bottom) (bottom) ? 'js-unfold-bottom' : '' end From a010db5db243a532cb8d1c2d5ac787e90da0044f Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Wed, 20 Jan 2016 15:26:44 +0100 Subject: [PATCH 51/57] Properly handle HTML entities with inline diffs --- lib/gitlab/diff/inline_diff.rb | 11 ++++------- lib/gitlab/diff/inline_diff_marker.rb | 16 ++++++++++++++-- lib/gitlab/diff/parser.rb | 7 +------ spec/lib/gitlab/diff/inline_diff_marker_spec.rb | 8 ++++---- spec/lib/gitlab/diff/parser_spec.rb | 2 +- 5 files changed, 24 insertions(+), 20 deletions(-) diff --git a/lib/gitlab/diff/inline_diff.rb b/lib/gitlab/diff/inline_diff.rb index e5986fd69e2..b8a61ad6115 100644 --- a/lib/gitlab/diff/inline_diff.rb +++ b/lib/gitlab/diff/inline_diff.rb @@ -16,15 +16,12 @@ module Gitlab old_line = @lines[old_index] new_line = @lines[new_index] - suffixless_old_line = old_line[1..-1] - suffixless_new_line = new_line[1..-1] - # Skip inline diff if empty line was replaced with content - next if suffixless_old_line == "" + next if old_line[1..-1] == "" - # Add one, because this is based on the suffixless version - lcp = longest_common_prefix(suffixless_old_line, suffixless_new_line) + 1 - lcs = longest_common_suffix(suffixless_old_line, suffixless_new_line) + # 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) new_diff_range = lcp..(new_line.length - lcs - 1) diff --git a/lib/gitlab/diff/inline_diff_marker.rb b/lib/gitlab/diff/inline_diff_marker.rb index 8998ccba5ce..c31149d374e 100644 --- a/lib/gitlab/diff/inline_diff_marker.rb +++ b/lib/gitlab/diff/inline_diff_marker.rb @@ -12,7 +12,7 @@ module Gitlab offset = 0 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 - inline_diff_positions = position_mapping[inline_diff_range] + inline_diff_positions = position_mapping[inline_diff_range].flatten # Turn the array of character positions into ranges marker_ranges = collapse_ranges(inline_diff_positions) @@ -47,7 +47,19 @@ module Gitlab rich_char = rich_line[rich_pos] end - mapping[raw_pos] = rich_pos + # multi-char HTML entities in the rich line correspond to a single character in the raw line + if rich_char == '&' + multichar_mapping = [rich_pos] + until rich_char == ';' + rich_pos += 1 + multichar_mapping << rich_pos + rich_char = rich_line[rich_pos] + end + + mapping[raw_pos] = multichar_mapping + else + mapping[raw_pos] = rich_pos + end rich_pos += 1 end diff --git a/lib/gitlab/diff/parser.rb b/lib/gitlab/diff/parser.rb index 2c0a866e8ba..6c8a1fc6d6f 100644 --- a/lib/gitlab/diff/parser.rb +++ b/lib/gitlab/diff/parser.rb @@ -14,7 +14,7 @@ module Gitlab @lines.each do |line| next if filename?(line) - full_line = html_escape(line.gsub(/\n/, '')) + full_line = line.gsub(/\n/, '') if line.match(/^@@ -/) type = "match" @@ -67,11 +67,6 @@ module Gitlab nil end end - - def html_escape(str) - replacements = { '&' => '&', '>' => '>', '<' => '<', '"' => '"', "'" => ''' } - str.gsub(/[&"'><]/, replacements) - 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 5d6aea17509..6f3276a8b53 100644 --- a/spec/lib/gitlab/diff/inline_diff_marker_spec.rb +++ b/spec/lib/gitlab/diff/inline_diff_marker_spec.rb @@ -2,14 +2,14 @@ require 'spec_helper' describe Gitlab::Diff::InlineDiffMarker, lib: true do describe '#inline_diffs' do - let(:raw) { "abc def" } - let(:rich) { %{abc def} } - let(:inline_diffs) { [2..4] } + let(:raw) { "abc 'def'" } + let(:rich) { %{abc 'def'} } + 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(%{abc def}) + expect(subject).to eq(%{abc 'def'}) end end end diff --git a/spec/lib/gitlab/diff/parser_spec.rb b/spec/lib/gitlab/diff/parser_spec.rb index ba577bd28e5..fe0dea77909 100644 --- a/spec/lib/gitlab/diff/parser_spec.rb +++ b/spec/lib/gitlab/diff/parser_spec.rb @@ -86,7 +86,7 @@ eos it { expect(line.type).to eq(nil) } it { expect(line.old_pos).to eq(24) } it { expect(line.new_pos).to eq(31) } - it { expect(line.text).to eq(' @cmd_output << stderr.read') } + it { expect(line.text).to eq(' @cmd_output << stderr.read') } end end end From 987e3d94b017cb94f55cf47cdbb417c45adf0681 Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Wed, 20 Jan 2016 15:40:49 +0100 Subject: [PATCH 52/57] Fix MR diff_refs --- app/models/merge_request.rb | 4 ++-- app/views/projects/merge_requests/_new_submit.html.haml | 2 +- app/views/projects/merge_requests/show/_diffs.html.haml | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index 9511521879b..0a890bbf6db 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -518,7 +518,7 @@ class MergeRequest < ActiveRecord::Base @ci_commit ||= source_project.ci_commit(last_commit.id) if last_commit && source_project end - def diff_range - [last_commit.parent, first_commit] + def diff_refs + [first_commit.parent || first_commit, last_commit] end end diff --git a/app/views/projects/merge_requests/_new_submit.html.haml b/app/views/projects/merge_requests/_new_submit.html.haml index 8b75976abd1..4c5a9818e3e 100644 --- a/app/views/projects/merge_requests/_new_submit.html.haml +++ b/app/views/projects/merge_requests/_new_submit.html.haml @@ -38,7 +38,7 @@ = render "projects/merge_requests/show/commits" #diffs.diffs.tab-pane.active - if @diffs.present? - = render "projects/diffs/diffs", diffs: @diffs, project: @project, diff_refs: @merge_request.diff_range + = render "projects/diffs/diffs", diffs: @diffs, project: @project, diff_refs: @merge_request.diff_refs - elsif @commits.size > MergeRequestDiff::COMMITS_SAFE_SIZE .alert.alert-danger %h4 This comparison includes more than #{MergeRequestDiff::COMMITS_SAFE_SIZE} commits. diff --git a/app/views/projects/merge_requests/show/_diffs.html.haml b/app/views/projects/merge_requests/show/_diffs.html.haml index 46c6f79937b..64cd484193e 100644 --- a/app/views/projects/merge_requests/show/_diffs.html.haml +++ b/app/views/projects/merge_requests/show/_diffs.html.haml @@ -1,6 +1,6 @@ - if @merge_request_diff.collected? = render "projects/diffs/diffs", diffs: params[:w] == '1' ? @merge_request.diffs_no_whitespace : @merge_request.diffs, - project: @merge_request.project, diff_refs: @merge_request.diff_range + project: @merge_request.project, diff_refs: @merge_request.diff_refs - elsif @merge_request_diff.empty? .nothing-here-block Nothing to merge from #{@merge_request.source_branch} into #{@merge_request.target_branch} - else From 26f7d023e6d15f4deab39f74509bd6dddebf6974 Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Wed, 20 Jan 2016 17:14:26 +0100 Subject: [PATCH 53/57] Update tests --- spec/fixtures/parallel_diff_result.yml | 2 +- spec/lib/gitlab/diff/highlight_spec.rb | 2 +- spec/lib/gitlab/diff/parallel_diff_spec.rb | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/spec/fixtures/parallel_diff_result.yml b/spec/fixtures/parallel_diff_result.yml index 0fb9bf6c5d8..a326b651aad 100644 --- a/spec/fixtures/parallel_diff_result.yml +++ b/spec/fixtures/parallel_diff_result.yml @@ -55,7 +55,7 @@ :type: new :number: 9 :text: | - + raise RuntimeError, "System commands must be given as an array of strings" + + raise RuntimeError, "System commands must be given as an array of strings" :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 b54e95483d3..b84a57f357a 100644 --- a/spec/lib/gitlab/diff/highlight_spec.rb +++ b/spec/lib/gitlab/diff/highlight_spec.rb @@ -33,7 +33,7 @@ describe Gitlab::Diff::Highlight, lib: true do end it 'should highlight added lines' do - code = %Q{+ raise RuntimeError, "System commands must be given as an array of strings"\n} + code = %Q{+ raise RuntimeError, "System commands must be given as an array of strings"\n} expect(diff_lines[5].text).to eq(code) end diff --git a/spec/lib/gitlab/diff/parallel_diff_spec.rb b/spec/lib/gitlab/diff/parallel_diff_spec.rb index 852218b4e62..1c5bbc47120 100644 --- a/spec/lib/gitlab/diff/parallel_diff_spec.rb +++ b/spec/lib/gitlab/diff/parallel_diff_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' describe Gitlab::Diff::ParallelDiff, lib: true do include RepoHelpers - + let(:project) { create(:project) } let(:repository) { project.repository } let(:commit) { project.commit(sample_commit.id) } From 577f2fb47a7ef57415b78b49d5c746d4e99f6a98 Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Wed, 20 Jan 2016 18:44:27 +0100 Subject: [PATCH 54/57] Save and use actual diff base commit for MR diff highlighting --- .../projects/compare_controller.rb | 4 +-- .../projects/merge_requests_controller.rb | 8 +++-- app/helpers/diff_helper.rb | 3 +- app/models/merge_request.rb | 15 ++++++++-- app/models/merge_request_diff.rb | 9 ++++++ ..._base_commit_sha_to_merge_request_diffs.rb | 5 ++++ db/schema.rb | 29 ++++++++++--------- lib/gitlab/diff/file.rb | 12 ++++++-- lib/gitlab/diff/highlight.rb | 9 +++++- lib/gitlab/diff/inline_diff_marker.rb | 13 +++++---- 10 files changed, 75 insertions(+), 32 deletions(-) create mode 100644 db/migrate/20160120172143_add_base_commit_sha_to_merge_request_diffs.rb diff --git a/app/controllers/projects/compare_controller.rb b/app/controllers/projects/compare_controller.rb index 60360c8e5c8..f8ec76cd4e5 100644 --- a/app/controllers/projects/compare_controller.rb +++ b/app/controllers/projects/compare_controller.rb @@ -21,8 +21,8 @@ class Projects::CompareController < Projects::ApplicationController @commits = Commit.decorate(compare_result.commits, @project) @diffs = compare_result.diffs @commit = @project.commit(head_ref) - @first_commit = @project.commit(base_ref) - @diff_refs = [@first_commit, @commit] + @base_commit = @project.commit(base_ref) + @diff_refs = [@base_commit, @commit] @line_notes = [] end end diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb index a6284a24223..ed3050d59aa 100644 --- a/app/controllers/projects/merge_requests_controller.rb +++ b/app/controllers/projects/merge_requests_controller.rb @@ -58,7 +58,11 @@ class Projects::MergeRequestsController < Projects::ApplicationController def diffs @commit = @merge_request.last_commit - @first_commit = @merge_request.first_commit + @base_commit = @merge_request.diff_base_commit + + # MRs created before 8.4 don't have a diff_base_commit, + # but we need it for the "View file @ ..." link by deleted files + @base_commit ||= @merge_request.first_commit.parent || @merge_request.first_commit @comments_allowed = @reply_allowed = true @comments_target = { @@ -102,7 +106,7 @@ class Projects::MergeRequestsController < Projects::ApplicationController @source_project = merge_request.source_project @commits = @merge_request.compare_commits.reverse @commit = @merge_request.last_commit - @first_commit = @merge_request.first_commit + @base_commit = @merge_request.diff_base_commit @diffs = @merge_request.compare_diffs @ci_commit = @merge_request.ci_commit diff --git a/app/helpers/diff_helper.rb b/app/helpers/diff_helper.rb index f500d3572e0..62971d1e14b 100644 --- a/app/helpers/diff_helper.rb +++ b/app/helpers/diff_helper.rb @@ -102,8 +102,7 @@ module DiffHelper def commit_for_diff(diff) if diff.deleted_file - first_commit = @first_commit || @commit - first_commit.parent || @first_commit + @base_commit || @commit.parent || @commit else @commit end diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index 0a890bbf6db..41dd248d80a 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -180,6 +180,14 @@ class MergeRequest < ActiveRecord::Base merge_request_diff ? merge_request_diff.first_commit : compare_commits.first end + def diff_base_commit + if merge_request_diff + merge_request_diff.base_commit + else + self.target_project.commit(self.target_branch) + end + end + def last_commit_short_sha last_commit.short_id end @@ -477,8 +485,7 @@ class MergeRequest < ActiveRecord::Base end def target_sha - @target_sha ||= target_project. - repository.commit(target_branch).sha + @target_sha ||= target_project.repository.commit(target_branch).sha end def source_sha @@ -519,6 +526,8 @@ class MergeRequest < ActiveRecord::Base end def diff_refs - [first_commit.parent || first_commit, last_commit] + return nil unless diff_base_commit + + [diff_base_commit, last_commit] end end diff --git a/app/models/merge_request_diff.rb b/app/models/merge_request_diff.rb index c499a4b5b4c..ba0194cd0a6 100644 --- a/app/models/merge_request_diff.rb +++ b/app/models/merge_request_diff.rb @@ -73,6 +73,12 @@ class MergeRequestDiff < ActiveRecord::Base commits.last end + def base_commit + return nil unless self.base_commit_sha + + merge_request.target_project.commit(self.base_commit_sha) + end + def last_commit_short_sha @last_commit_short_sha ||= last_commit.short_id end @@ -156,6 +162,9 @@ class MergeRequestDiff < ActiveRecord::Base end self.st_diffs = new_diffs + + self.base_commit_sha = merge_request.target_project.commit(target_branch).try(:sha) + self.save end diff --git a/db/migrate/20160120172143_add_base_commit_sha_to_merge_request_diffs.rb b/db/migrate/20160120172143_add_base_commit_sha_to_merge_request_diffs.rb new file mode 100644 index 00000000000..d6c6aa4a4e8 --- /dev/null +++ b/db/migrate/20160120172143_add_base_commit_sha_to_merge_request_diffs.rb @@ -0,0 +1,5 @@ +class AddBaseCommitShaToMergeRequestDiffs < ActiveRecord::Migration + def change + add_column :merge_request_diffs, :base_commit_sha, :string + end +end diff --git a/db/schema.rb b/db/schema.rb index dc7cb9f667f..7eb99a47c71 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -11,7 +11,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 20160119145451) do +ActiveRecord::Schema.define(version: 20160120172143) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -490,6 +490,7 @@ ActiveRecord::Schema.define(version: 20160119145451) do t.integer "merge_request_id", null: false t.datetime "created_at" t.datetime "updated_at" + t.string "base_commit_sha" end add_index "merge_request_diffs", ["merge_request_id"], name: "index_merge_request_diffs_on_merge_request_id", unique: true, using: :btree @@ -725,19 +726,19 @@ ActiveRecord::Schema.define(version: 20160119145451) do t.string "type" t.string "title" t.integer "project_id" - t.datetime "created_at", null: false - t.datetime "updated_at", null: false - t.boolean "active", null: false + t.datetime "created_at", null: false + t.datetime "updated_at", null: false + t.boolean "active", null: false t.text "properties" - t.boolean "template", default: false - t.boolean "push_events", default: true - t.boolean "issues_events", default: true - t.boolean "merge_requests_events", default: true - t.boolean "tag_push_events", default: true - t.boolean "note_events", default: true, null: false - t.boolean "build_events", default: false, null: false - t.string "category", default: "common", null: false - t.boolean "default", default: false + t.boolean "template", default: false + t.boolean "push_events", default: true + t.boolean "issues_events", default: true + t.boolean "merge_requests_events", default: true + t.boolean "tag_push_events", default: true + t.boolean "note_events", default: true, null: false + t.boolean "build_events", default: false, null: false + t.string "category", default: "common", null: false + t.boolean "default", default: false end add_index "services", ["category"], name: "index_services_on_category", using: :btree @@ -854,7 +855,7 @@ ActiveRecord::Schema.define(version: 20160119145451) do t.boolean "hide_project_limit", default: false t.string "unlock_token" t.datetime "otp_grace_period_started_at" - t.boolean "ldap_email", default: false, null: false + t.boolean "ldap_email", default: false, null: false end add_index "users", ["admin"], name: "index_users_on_admin", using: :btree diff --git a/lib/gitlab/diff/file.rb b/lib/gitlab/diff/file.rb index 74b1c117129..a484177ae33 100644 --- a/lib/gitlab/diff/file.rb +++ b/lib/gitlab/diff/file.rb @@ -1,14 +1,22 @@ module Gitlab module Diff class File - attr_reader :diff, :new_ref, :old_ref + attr_reader :diff, :diff_refs delegate :new_file, :deleted_file, :renamed_file, :old_path, :new_path, to: :diff, prefix: false def initialize(diff, diff_refs) @diff = diff - @old_ref, @new_ref = diff_refs + @diff_refs = diff_refs + end + + def old_ref + diff_refs[0] if diff_refs + end + + def new_ref + diff_refs[1] if diff_refs end # Array of Gitlab::DIff::Line objects diff --git a/lib/gitlab/diff/highlight.rb b/lib/gitlab/diff/highlight.rb index b6875f07279..fe084a7399a 100644 --- a/lib/gitlab/diff/highlight.rb +++ b/lib/gitlab/diff/highlight.rb @@ -31,6 +31,8 @@ module Gitlab private def highlight_line(diff_line, index) + return html_escape(diff_line.text) unless diff_file.diff_refs + line_prefix = diff_line.text.match(/\A([+-])/) ? $1 : ' ' case diff_line.type @@ -42,7 +44,7 @@ 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 : diff_line.text + rich_line ? line_prefix + rich_line : html_escape(diff_line.text) end def inline_diffs @@ -63,6 +65,11 @@ 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_marker.rb b/lib/gitlab/diff/inline_diff_marker.rb index c31149d374e..7ca198eeb86 100644 --- a/lib/gitlab/diff/inline_diff_marker.rb +++ b/lib/gitlab/diff/inline_diff_marker.rb @@ -9,17 +9,18 @@ module Gitlab end def mark(line_inline_diffs) - offset = 0 + 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 inline_diff_positions = position_mapping[inline_diff_range].flatten # Turn the array of character positions into ranges - marker_ranges = collapse_ranges(inline_diff_positions) + marker_ranges.concat(collapse_ranges(inline_diff_positions)) + end - # Mark each range - marker_ranges.each do |range| - offset = insert_around_range(rich_line, range, "", "", offset) - end + offset = 0 + # Mark each range + marker_ranges.each do |range| + offset = insert_around_range(rich_line, range, "", "", offset) end rich_line From 0e992a3b4e710f8486a37bfa73ad6981365fceb2 Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Wed, 20 Jan 2016 19:20:13 +0100 Subject: [PATCH 55/57] Properly highlight lines around '\ No newline at end of file' --- .../projects/diffs/_parallel_view.html.haml | 5 +++++ app/views/projects/diffs/_text_file.html.haml | 4 ++++ lib/gitlab/diff/highlight.rb | 4 ++-- lib/gitlab/diff/parallel_diff.rb | 2 +- lib/gitlab/diff/parser.rb | 16 ++++++++++++---- 5 files changed, 24 insertions(+), 7 deletions(-) diff --git a/app/views/projects/diffs/_parallel_view.html.haml b/app/views/projects/diffs/_parallel_view.html.haml index 986d30d36a8..a15d147ab05 100644 --- a/app/views/projects/diffs/_parallel_view.html.haml +++ b/app/views/projects/diffs/_parallel_view.html.haml @@ -8,6 +8,11 @@ - if left[:type] == 'match' = render "projects/diffs/match_line_parallel", { line: left[:text], line_old: left[:number], line_new: right[:number] } + - elsif left[:type] == 'nonewline' + %td.old_line + %td.line_content.parallel.matched= left[:text] + %td.new_line + %td.line_content.parallel.matched= left[:text] - else %td.old_line{id: left[:line_code], class: "#{left[:type]}"} = link_to raw(left[:number]), "##{left[:line_code]}", id: left[:line_code] diff --git a/app/views/projects/diffs/_text_file.html.haml b/app/views/projects/diffs/_text_file.html.haml index 310bbd4efea..f4fc6caba0f 100644 --- a/app/views/projects/diffs/_text_file.html.haml +++ b/app/views/projects/diffs/_text_file.html.haml @@ -15,6 +15,10 @@ - if type == "match" = render "projects/diffs/match_line", {line: line.text, line_old: line_old, line_new: line.new_pos, bottom: false, new_file: diff_file.new_file} + - elsif type == 'nonewline' + %td.old_line.diff-line-num + %td.new_line.diff-line-num + %td.line_content.matched= line.text - else %td.old_line = link_to raw(type == "new" ? " " : line_old), "##{line_code}", id: line_code diff --git a/lib/gitlab/diff/highlight.rb b/lib/gitlab/diff/highlight.rb index fe084a7399a..179f8164c84 100644 --- a/lib/gitlab/diff/highlight.rb +++ b/lib/gitlab/diff/highlight.rb @@ -14,7 +14,7 @@ module Gitlab def highlight @diff_lines.each_with_index do |diff_line, i| # ignore highlighting for "match" lines - next if diff_line.type == 'match' + next if diff_line.type == 'match' || diff_line.type == 'nonewline' rich_line = highlight_line(diff_line, i) @@ -33,7 +33,7 @@ module Gitlab def highlight_line(diff_line, index) return html_escape(diff_line.text) unless diff_file.diff_refs - line_prefix = diff_line.text.match(/\A([+-])/) ? $1 : ' ' + line_prefix = diff_line.text.match(/\A(.)/) ? $1 : ' ' case diff_line.type when 'new', nil diff --git a/lib/gitlab/diff/parallel_diff.rb b/lib/gitlab/diff/parallel_diff.rb index a0dc3da875d..c0db3559e3a 100644 --- a/lib/gitlab/diff/parallel_diff.rb +++ b/lib/gitlab/diff/parallel_diff.rb @@ -62,7 +62,7 @@ module Gitlab } } skip_next = true - when 'old', nil + when 'old', 'nonewline', nil # Left side has text removed, right side doesn't have any change # No next line code, no new line number, no new line text lines << { diff --git a/lib/gitlab/diff/parser.rb b/lib/gitlab/diff/parser.rb index 6c8a1fc6d6f..3666063bf8b 100644 --- a/lib/gitlab/diff/parser.rb +++ b/lib/gitlab/diff/parser.rb @@ -26,6 +26,10 @@ module Gitlab lines_obj << Gitlab::Diff::Line.new(full_line, type, line_obj_index, line_old, line_new) line_obj_index += 1 next + elsif line[0] == '\\' + type = 'nonewline' + lines_obj << Gitlab::Diff::Line.new(full_line, type, line_obj_index, line_old, line_new) + line_obj_index += 1 else type = identification_type(line) lines_obj << Gitlab::Diff::Line.new(full_line, type, line_obj_index, line_old, line_new) @@ -33,10 +37,13 @@ module Gitlab end - if line[0] == "+" + case line[0] + when "+" line_new += 1 - elsif line[0] == "-" + when "-" line_old += 1 + when "\\" + # No increment else line_new += 1 line_old += 1 @@ -59,9 +66,10 @@ module Gitlab end def identification_type(line) - if line[0] == "+" + case line[0] + when "+" "new" - elsif line[0] == "-" + when "-" "old" else nil From 6000f8545f43b449035cb50382901ce40fb807b0 Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Wed, 20 Jan 2016 19:33:34 +0100 Subject: [PATCH 56/57] Validate bounds just to be sure --- lib/gitlab/diff/inline_diff_marker.rb | 3 +++ 1 file changed, 3 insertions(+) diff --git a/lib/gitlab/diff/inline_diff_marker.rb b/lib/gitlab/diff/inline_diff_marker.rb index 7ca198eeb86..1d7fa1bce06 100644 --- a/lib/gitlab/diff/inline_diff_marker.rb +++ b/lib/gitlab/diff/inline_diff_marker.rb @@ -93,6 +93,9 @@ module Gitlab # Inserts tags around the characters identified by the given range def insert_around_range(text, range, before, after, offset = 0) + # Just to be sure + return offset if offset + range.end + 1 > text.length + text.insert(offset + range.begin, before) offset += before.length From 86b75f4713fa6cb14cbf443c1e29ca6384204b65 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rub=C3=A9n=20D=C3=A1vila?= Date: Wed, 20 Jan 2016 14:38:34 -0500 Subject: [PATCH 57/57] Use a MR with commits. --- .../merge_requests/merge_when_build_succeeds_service_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/services/merge_requests/merge_when_build_succeeds_service_spec.rb b/spec/services/merge_requests/merge_when_build_succeeds_service_spec.rb index 449cecaa789..de9fed2b7dd 100644 --- a/spec/services/merge_requests/merge_when_build_succeeds_service_spec.rb +++ b/spec/services/merge_requests/merge_when_build_succeeds_service_spec.rb @@ -6,7 +6,7 @@ describe MergeRequests::MergeWhenBuildSucceedsService do let(:mr_merge_if_green_enabled) do create(:merge_request, merge_when_build_succeeds: true, merge_user: user, - source_branch: "source_branch", target_branch: project.default_branch, + source_branch: "master", target_branch: 'feature', source_project: project, target_project: project, state: "opened") end