Commit f4282412 authored by Douwe Maan's avatar Douwe Maan

Merge branch '20034-safe-diffs' into 'master'

Cache diff syntax highlighted output

## What does this MR do?

Cache highlighted diffs for merge requests when they are requested for existing merge requests but after each change on the merge request diff recalculate again the cache.

I've introduced the concept of SafeDiffs which are the diffs that we're going to show on the UI and will be highlighted so maybe we can extend cache capabilities to more diffs.

The more problematic part is what and how we cache the highlighted lines, for the moment what I did was store in Redis as Gitlab::Diff::Line serialized as an array of json objects, similar of what we do for commits and diffs on merge request diffs.

The next step can be add a new field in the merge_request_diff object if we find it worth it. But let's first confirm this is the way to go

## What are the relevant issue numbers?

Closes #20034

## Screenshots (if relevant)

## Does this MR meet the acceptance criteria?

- [x] [CHANGELOG](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/CHANGELOG) entry added
- ~~[ ] [Documentation created/updated](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/doc/development/doc_styleguide.md)~~
- ~~[ ] API support added~~
- Tests
  - [x] Added for this feature/bug
  - [x] All builds are passing
- [x] Conform by the [style guides](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/CONTRIBUTING.md#style-guides)
- [x] Branch has no merge conflicts with `master` (if you do - rebase it please)
- [x] [Squashed related commits together](https://git-scm.com/book/en/Git-Tools-Rewriting-History#Squashing-Commits)


See merge request !5401
parents 1ee11136 c008a1a9
...@@ -10,6 +10,7 @@ v 8.11.0 (unreleased) ...@@ -10,6 +10,7 @@ v 8.11.0 (unreleased)
- The Repository class is now instrumented - The Repository class is now instrumented
- Cache the commit author in RequestStore to avoid extra lookups in PostReceive - Cache the commit author in RequestStore to avoid extra lookups in PostReceive
- Expand commit message width in repo view (ClemMakesApps) - Expand commit message width in repo view (ClemMakesApps)
- Cache highlighted diff lines for merge requests
- Fix of 'Commits being passed to custom hooks are already reachable when using the UI' - Fix of 'Commits being passed to custom hooks are already reachable when using the UI'
- Add support for using RequestStore within Sidekiq tasks via SIDEKIQ_REQUEST_STORE env variable - Add support for using RequestStore within Sidekiq tasks via SIDEKIQ_REQUEST_STORE env variable
- Optimize maximum user access level lookup in loading of notes - Optimize maximum user access level lookup in loading of notes
......
module DiffForPath module DiffForPath
extend ActiveSupport::Concern extend ActiveSupport::Concern
def render_diff_for_path(diffs, diff_refs, project) def render_diff_for_path(diffs)
diff_file = safe_diff_files(diffs, diff_refs: diff_refs, repository: project.repository).find do |diff| diff_file = diffs.diff_files.find do |diff|
diff.old_path == params[:old_path] && diff.new_path == params[:new_path] diff.old_path == params[:old_path] && diff.new_path == params[:new_path]
end end
...@@ -14,7 +14,7 @@ module DiffForPath ...@@ -14,7 +14,7 @@ module DiffForPath
locals = { locals = {
diff_file: diff_file, diff_file: diff_file,
diff_commit: diff_commit, diff_commit: diff_commit,
diff_refs: diff_refs, diff_refs: diffs.diff_refs,
blob: blob, blob: blob,
project: project project: project
} }
......
...@@ -28,7 +28,7 @@ class Projects::CommitController < Projects::ApplicationController ...@@ -28,7 +28,7 @@ class Projects::CommitController < Projects::ApplicationController
end end
def diff_for_path def diff_for_path
render_diff_for_path(@diffs, @commit.diff_refs, @project) render_diff_for_path(@commit.diffs(diff_options))
end end
def builds def builds
......
...@@ -21,7 +21,7 @@ class Projects::CompareController < Projects::ApplicationController ...@@ -21,7 +21,7 @@ class Projects::CompareController < Projects::ApplicationController
def diff_for_path def diff_for_path
return render_404 unless @compare return render_404 unless @compare
render_diff_for_path(@diffs, @diff_refs, @project) render_diff_for_path(@compare.diffs(diff_options))
end end
def create def create
...@@ -40,18 +40,12 @@ class Projects::CompareController < Projects::ApplicationController ...@@ -40,18 +40,12 @@ class Projects::CompareController < Projects::ApplicationController
@compare = CompareService.new.execute(@project, @head_ref, @project, @start_ref) @compare = CompareService.new.execute(@project, @head_ref, @project, @start_ref)
if @compare if @compare
@commits = Commit.decorate(@compare.commits, @project) @commits = @compare.commits
@start_commit = @compare.start_commit
@start_commit = @project.commit(@start_ref) @commit = @compare.commit
@commit = @project.commit(@head_ref) @base_commit = @compare.base_commit
@base_commit = @project.merge_base_commit(@start_ref, @head_ref)
@diffs = @compare.diffs(diff_options) @diffs = @compare.diffs(diff_options)
@diff_refs = Gitlab::Diff::DiffRefs.new(
base_sha: @base_commit.try(:sha),
start_sha: @start_commit.try(:sha),
head_sha: @commit.try(:sha)
)
@diff_notes_disabled = true @diff_notes_disabled = true
@grouped_diff_discussions = {} @grouped_diff_discussions = {}
......
...@@ -85,7 +85,11 @@ class Projects::MergeRequestsController < Projects::ApplicationController ...@@ -85,7 +85,11 @@ class Projects::MergeRequestsController < Projects::ApplicationController
respond_to do |format| respond_to do |format|
format.html { define_discussion_vars } format.html { define_discussion_vars }
format.json { render json: { html: view_to_html_string("projects/merge_requests/show/_diffs") } } format.json do
@diffs = @merge_request.diffs(diff_options)
render json: { html: view_to_html_string("projects/merge_requests/show/_diffs") }
end
end end
end end
...@@ -103,9 +107,8 @@ class Projects::MergeRequestsController < Projects::ApplicationController ...@@ -103,9 +107,8 @@ class Projects::MergeRequestsController < Projects::ApplicationController
end end
define_commit_vars define_commit_vars
diffs = @merge_request.diffs(diff_options)
render_diff_for_path(diffs, @merge_request.diff_refs, @merge_request.project) render_diff_for_path(@merge_request.diffs(diff_options))
end end
def commits def commits
...@@ -153,7 +156,7 @@ class Projects::MergeRequestsController < Projects::ApplicationController ...@@ -153,7 +156,7 @@ class Projects::MergeRequestsController < Projects::ApplicationController
@commits = @merge_request.compare_commits.reverse @commits = @merge_request.compare_commits.reverse
@commit = @merge_request.diff_head_commit @commit = @merge_request.diff_head_commit
@base_commit = @merge_request.diff_base_commit @base_commit = @merge_request.diff_base_commit
@diffs = @merge_request.compare.diffs(diff_options) if @merge_request.compare @diffs = @merge_request.diffs(diff_options) if @merge_request.compare
@diff_notes_disabled = true @diff_notes_disabled = true
@pipeline = @merge_request.pipeline @pipeline = @merge_request.pipeline
......
...@@ -206,10 +206,10 @@ module CommitsHelper ...@@ -206,10 +206,10 @@ module CommitsHelper
end end
end end
def view_file_btn(commit_sha, diff, project) def view_file_btn(commit_sha, diff_new_path, project)
link_to( link_to(
namespace_project_blob_path(project.namespace, project, namespace_project_blob_path(project.namespace, project,
tree_join(commit_sha, diff.new_path)), tree_join(commit_sha, diff_new_path)),
class: 'btn view-file js-view-file btn-file-option' class: 'btn view-file js-view-file btn-file-option'
) do ) do
raw('View file @') + content_tag(:span, commit_sha[0..6], raw('View file @') + content_tag(:span, commit_sha[0..6],
......
...@@ -30,11 +30,7 @@ module DiffHelper ...@@ -30,11 +30,7 @@ module DiffHelper
options[:paths] = params.values_at(:old_path, :new_path) options[:paths] = params.values_at(:old_path, :new_path)
end end
Commit.max_diff_options.merge(options) options
end
def safe_diff_files(diffs, diff_refs: nil, repository: nil)
diffs.decorate! { |diff| Gitlab::Diff::File.new(diff, diff_refs: diff_refs, repository: repository) }
end end
def unfold_bottom_class(bottom) def unfold_bottom_class(bottom)
......
...@@ -104,7 +104,7 @@ class Commit ...@@ -104,7 +104,7 @@ class Commit
end end
def diff_line_count def diff_line_count
@diff_line_count ||= Commit::diff_line_count(self.diffs) @diff_line_count ||= Commit::diff_line_count(raw_diffs)
@diff_line_count @diff_line_count
end end
...@@ -317,6 +317,14 @@ class Commit ...@@ -317,6 +317,14 @@ class Commit
nil nil
end end
def raw_diffs(*args)
raw.diffs(*args)
end
def diffs(diff_options = nil)
Gitlab::Diff::FileCollection::Commit.new(self, diff_options: diff_options)
end
private private
def find_author_by_any_email def find_author_by_any_email
...@@ -326,7 +334,7 @@ class Commit ...@@ -326,7 +334,7 @@ class Commit
def repo_changes def repo_changes
changes = { added: [], modified: [], removed: [] } changes = { added: [], modified: [], removed: [] }
diffs.each do |diff| raw_diffs.each do |diff|
if diff.deleted_file if diff.deleted_file
changes[:removed] << diff.old_path changes[:removed] << diff.old_path
elsif diff.renamed_file || diff.new_file elsif diff.renamed_file || diff.new_file
......
class Compare
delegate :same, :head, :base, to: :@compare
attr_reader :project
def self.decorate(compare, project)
if compare.is_a?(Compare)
compare
else
self.new(compare, project)
end
end
def initialize(compare, project)
@compare = compare
@project = project
end
def commits
@commits ||= Commit.decorate(@compare.commits, project)
end
def start_commit
return @start_commit if defined?(@start_commit)
commit = @compare.base
@start_commit = commit ? ::Commit.new(commit, project) : nil
end
def head_commit
return @head_commit if defined?(@head_commit)
commit = @compare.head
@head_commit = commit ? ::Commit.new(commit, project) : nil
end
alias_method :commit, :head_commit
def base_commit
return @base_commit if defined?(@base_commit)
@base_commit = if start_commit && head_commit
project.merge_base_commit(start_commit.id, head_commit.id)
else
nil
end
end
def raw_diffs(*args)
@compare.diffs(*args)
end
def diffs(diff_options = nil)
Gitlab::Diff::FileCollection::Compare.new(self,
project: project,
diff_options: diff_options,
diff_refs: diff_refs)
end
def diff_refs
Gitlab::Diff::DiffRefs.new(
base_sha: base_commit.try(:sha),
start_sha: start_commit.try(:sha),
head_sha: commit.try(:sha)
)
end
end
...@@ -85,7 +85,7 @@ class LegacyDiffNote < Note ...@@ -85,7 +85,7 @@ class LegacyDiffNote < Note
return nil unless noteable return nil unless noteable
return @diff if defined?(@diff) return @diff if defined?(@diff)
@diff = noteable.diffs(Commit.max_diff_options).find do |d| @diff = noteable.raw_diffs(Commit.max_diff_options).find do |d|
d.new_path && Digest::SHA1.hexdigest(d.new_path) == diff_file_hash d.new_path && Digest::SHA1.hexdigest(d.new_path) == diff_file_hash
end end
end end
...@@ -116,7 +116,7 @@ class LegacyDiffNote < Note ...@@ -116,7 +116,7 @@ class LegacyDiffNote < Note
# Find the diff on noteable that matches our own # Find the diff on noteable that matches our own
def find_noteable_diff def find_noteable_diff
diffs = noteable.diffs(Commit.max_diff_options) diffs = noteable.raw_diffs(Commit.max_diff_options)
diffs.find { |d| d.new_path == self.diff.new_path } diffs.find { |d| d.new_path == self.diff.new_path }
end end
end end
...@@ -164,8 +164,16 @@ class MergeRequest < ActiveRecord::Base ...@@ -164,8 +164,16 @@ class MergeRequest < ActiveRecord::Base
merge_request_diff ? merge_request_diff.first_commit : compare_commits.first merge_request_diff ? merge_request_diff.first_commit : compare_commits.first
end end
def diffs(*args) def raw_diffs(*args)
merge_request_diff ? merge_request_diff.diffs(*args) : compare.diffs(*args) merge_request_diff ? merge_request_diff.diffs(*args) : compare.raw_diffs(*args)
end
def diffs(diff_options = nil)
if self.compare
self.compare.diffs(diff_options)
else
Gitlab::Diff::FileCollection::MergeRequest.new(self, diff_options: diff_options)
end
end end
def diff_size def diff_size
...@@ -313,6 +321,8 @@ class MergeRequest < ActiveRecord::Base ...@@ -313,6 +321,8 @@ class MergeRequest < ActiveRecord::Base
merge_request_diff.reload_content merge_request_diff.reload_content
MergeRequests::MergeRequestDiffCacheService.new.execute(self)
new_diff_refs = self.diff_refs new_diff_refs = self.diff_refs
update_diff_notes_positions( update_diff_notes_positions(
......
...@@ -372,7 +372,7 @@ class Repository ...@@ -372,7 +372,7 @@ class Repository
# We don't want to flush the cache if the commit didn't actually make any # We don't want to flush the cache if the commit didn't actually make any
# changes to any of the possible avatar files. # changes to any of the possible avatar files.
if revision && commit = self.commit(revision) if revision && commit = self.commit(revision)
return unless commit.diffs. return unless commit.raw_diffs.
any? { |diff| AVATAR_FILES.include?(diff.new_path) } any? { |diff| AVATAR_FILES.include?(diff.new_path) }
end end
......
...@@ -20,10 +20,12 @@ class CompareService ...@@ -20,10 +20,12 @@ class CompareService
) )
end end
Gitlab::Git::Compare.new( raw_compare = Gitlab::Git::Compare.new(
target_project.repository.raw_repository, target_project.repository.raw_repository,
target_branch, target_branch,
source_sha, source_sha
) )
Compare.new(raw_compare, target_project)
end end
end end
...@@ -34,7 +34,7 @@ module MergeRequests ...@@ -34,7 +34,7 @@ module MergeRequests
# At this point we decide if merge request can be created # At this point we decide if merge request can be created
# If we have at least one commit to merge -> creation allowed # If we have at least one commit to merge -> creation allowed
if commits.present? if commits.present?
merge_request.compare_commits = Commit.decorate(commits, merge_request.source_project) merge_request.compare_commits = commits
merge_request.can_be_created = true merge_request.can_be_created = true
merge_request.compare = compare merge_request.compare = compare
else else
......
module MergeRequests
class MergeRequestDiffCacheService
def execute(merge_request)
# Executing the iteration we cache all the highlighted diff information
merge_request.diffs.diff_files.to_a
end
end
end
...@@ -2,7 +2,7 @@ ...@@ -2,7 +2,7 @@
= nav_link(path: 'commit#show') do = nav_link(path: 'commit#show') do
= link_to namespace_project_commit_path(@project.namespace, @project, @commit.id) do = link_to namespace_project_commit_path(@project.namespace, @project, @commit.id) do
Changes Changes
%span.badge= @diffs.count %span.badge= @diffs.size
= nav_link(path: 'commit#builds') do = nav_link(path: 'commit#builds') do
= link_to builds_namespace_project_commit_path(@project.namespace, @project, @commit.id) do = link_to builds_namespace_project_commit_path(@project.namespace, @project, @commit.id) do
Builds Builds
......
...@@ -7,7 +7,7 @@ ...@@ -7,7 +7,7 @@
= render "ci_menu" = render "ci_menu"
- else - else
%div.block-connector %div.block-connector
= render "projects/diffs/diffs", diffs: @diffs, project: @project, diff_refs: @commit.diff_refs = render "projects/diffs/diffs", diffs: @diffs
= render "projects/notes/notes_with_form" = render "projects/notes/notes_with_form"
- if can_collaborate_with_project? - if can_collaborate_with_project?
- %w(revert cherry-pick).each do |type| - %w(revert cherry-pick).each do |type|
......
...@@ -8,7 +8,7 @@ ...@@ -8,7 +8,7 @@
- if @commits.present? - if @commits.present?
= render "projects/commits/commit_list" = render "projects/commits/commit_list"
= render "projects/diffs/diffs", diffs: @diffs, project: @project, diff_refs: @diff_refs = render "projects/diffs/diffs", diffs: @diffs
- else - else
.light-well .light-well
.center .center
......
- show_whitespace_toggle = local_assigns.fetch(:show_whitespace_toggle, true) - show_whitespace_toggle = local_assigns.fetch(:show_whitespace_toggle, true)
- diff_files = diffs.diff_files
- if diff_view == 'parallel' - if diff_view == 'parallel'
- fluid_layout true - fluid_layout true
- diff_files = safe_diff_files(diffs, diff_refs: diff_refs, repository: project.repository)
.content-block.oneline-block.files-changed .content-block.oneline-block.files-changed
.inline-parallel-buttons .inline-parallel-buttons
- if !expand_all_diffs? && diff_files.any? { |diff_file| diff_file.collapsed? } - if !expand_all_diffs? && diff_files.any? { |diff_file| diff_file.collapsed? }
= link_to 'Expand all', url_for(params.merge(expand_all_diffs: 1, format: nil)), class: 'btn btn-default' = link_to 'Expand all', url_for(params.merge(expand_all_diffs: 1, format: nil)), class: 'btn btn-default'
- if show_whitespace_toggle - if show_whitespace_toggle
- if current_controller?(:commit) - if current_controller?(:commit)
= commit_diff_whitespace_link(@project, @commit, class: 'hidden-xs') = commit_diff_whitespace_link(diffs.project, @commit, class: 'hidden-xs')
- elsif current_controller?(:merge_requests) - elsif current_controller?(:merge_requests)
= diff_merge_request_whitespace_link(@project, @merge_request, class: 'hidden-xs') = diff_merge_request_whitespace_link(diffs.project, @merge_request, class: 'hidden-xs')
- elsif current_controller?(:compare) - elsif current_controller?(:compare)
= diff_compare_whitespace_link(@project, params[:from], params[:to], class: 'hidden-xs') = diff_compare_whitespace_link(diffs.project, params[:from], params[:to], class: 'hidden-xs')
.btn-group .btn-group
= inline_diff_btn = inline_diff_btn
= parallel_diff_btn = parallel_diff_btn
...@@ -23,12 +22,12 @@ ...@@ -23,12 +22,12 @@
- if diff_files.overflow? - if diff_files.overflow?
= render 'projects/diffs/warning', diff_files: diff_files = render 'projects/diffs/warning', diff_files: diff_files
.files{data: {can_create_note: (!@diff_notes_disabled && can?(current_user, :create_note, @project))}} .files{data: {can_create_note: (!@diff_notes_disabled && can?(current_user, :create_note, diffs.project))}}
- diff_files.each_with_index do |diff_file, index| - diff_files.each_with_index do |diff_file, index|
- diff_commit = commit_for_diff(diff_file) - diff_commit = commit_for_diff(diff_file)
- blob = diff_file.blob(diff_commit) - blob = diff_file.blob(diff_commit)
- next unless blob - next unless blob
- blob.load_all_data!(project.repository) unless blob.only_display_raw? - blob.load_all_data!(diffs.project.repository) unless blob.only_display_raw?
= render 'projects/diffs/file', i: index, project: project, = render 'projects/diffs/file', i: index, project: diffs.project,
diff_file: diff_file, diff_commit: diff_commit, blob: blob, diff_refs: diff_refs diff_file: diff_file, diff_commit: diff_commit, blob: blob
...@@ -15,6 +15,6 @@ ...@@ -15,6 +15,6 @@
from_merge_request_id: @merge_request.id, from_merge_request_id: @merge_request.id,
skip_visible_check: true) skip_visible_check: true)
= view_file_btn(diff_commit.id, diff_file, project) = view_file_btn(diff_commit.id, diff_file.new_path, project)
= render 'projects/diffs/content', diff_file: diff_file, diff_commit: diff_commit, diff_refs: diff_refs, blob: blob, project: project = render 'projects/diffs/content', diff_file: diff_file, diff_commit: diff_commit, blob: blob, project: project
...@@ -42,7 +42,7 @@ ...@@ -42,7 +42,7 @@
%h4 This comparison includes more than #{MergeRequestDiff::COMMITS_SAFE_SIZE} commits. %h4 This comparison includes more than #{MergeRequestDiff::COMMITS_SAFE_SIZE} commits.
%p To preserve performance the line changes are not shown. %p To preserve performance the line changes are not shown.
- else - else
= render "projects/diffs/diffs", diffs: @diffs, project: @project, diff_refs: @merge_request.diff_refs, show_whitespace_toggle: false = render "projects/diffs/diffs", diffs: @diffs, show_whitespace_toggle: false
- if @pipeline - if @pipeline
#builds.builds.tab-pane #builds.builds.tab-pane
= render "projects/merge_requests/show/builds" = render "projects/merge_requests/show/builds"
......
- if @merge_request_diff.collected? - if @merge_request_diff.collected?
= render "projects/diffs/diffs", diffs: @merge_request.diffs(diff_options), = render "projects/diffs/diffs", diffs: @diffs
project: @merge_request.project, diff_refs: @merge_request.diff_refs
- elsif @merge_request_diff.empty? - elsif @merge_request_diff.empty?
.nothing-here-block Nothing to merge from #{@merge_request.source_branch} into #{@merge_request.target_branch} .nothing-here-block Nothing to merge from #{@merge_request.source_branch} into #{@merge_request.target_branch}
- else - else
......
...@@ -33,25 +33,14 @@ class EmailsOnPushWorker ...@@ -33,25 +33,14 @@ class EmailsOnPushWorker
reverse_compare = false reverse_compare = false
if action == :push if action == :push
merge_base_sha = project.merge_base_commit(before_sha, after_sha).try(:sha) compare = CompareService.new.execute(project, before_sha, project, after_sha)
compare = Gitlab::Git::Compare.new(project.repository.raw_repository, before_sha, after_sha) diff_refs = compare.diff_refs
diff_refs = Gitlab::Diff::DiffRefs.new(
base_sha: merge_base_sha,
start_sha: before_sha,
head_sha: after_sha
)
return false if compare.same return false if compare.same
if compare.commits.empty? if compare.commits.empty?
compare = Gitlab::Git::Compare.new(project.repository.raw_repository, after_sha, before_sha) compare = CompareService.new.execute(project, after_sha, project, before_sha)
diff_refs = compare.diff_refs
diff_refs = Gitlab::Diff::DiffRefs.new(
base_sha: merge_base_sha,
start_sha: after_sha,
head_sha: before_sha
)
reverse_compare = true reverse_compare = true
......
...@@ -141,8 +141,10 @@ class IrkerWorker ...@@ -141,8 +141,10 @@ class IrkerWorker
end end
def files_count(commit) def files_count(commit)
files = "#{commit.diffs.real_size} file" diffs = commit.raw_diffs
files += 's' if commit.diffs.count > 1
files = "#{diffs.real_size} file"
files += 's' if diffs.size > 1
files files
end end
......
...@@ -54,7 +54,7 @@ module API ...@@ -54,7 +54,7 @@ module API
sha = params[:sha] sha = params[:sha]
commit = user_project.commit(sha) commit = user_project.commit(sha)
not_found! "Commit" unless commit not_found! "Commit" unless commit
commit.diffs.to_a commit.raw_diffs.to_a
end end
# Get a commit's comments # Get a commit's comments
...@@ -96,7 +96,7 @@ module API ...@@ -96,7 +96,7 @@ module API
} }
if params[:path] && params[:line] && params[:line_type] if params[:path] && params[:line] && params[:line_type]
commit.diffs(all_diffs: true).each do |diff| commit.raw_diffs(all_diffs: true).each do |diff|
next unless diff.new_path == params[:path] next unless diff.new_path == params[:path]
lines = Gitlab::Diff::Parser.new.parse(diff.diff.each_line) lines = Gitlab::Diff::Parser.new.parse(diff.diff.each_line)
......
...@@ -224,7 +224,7 @@ module API ...@@ -224,7 +224,7 @@ module API
class MergeRequestChanges < MergeRequest class MergeRequestChanges < MergeRequest
expose :diffs, as: :changes, using: Entities::RepoDiff do |compare, _| expose :diffs, as: :changes, using: Entities::RepoDiff do |compare, _|
compare.diffs(all_diffs: true).to_a compare.raw_diffs(all_diffs: true).to_a
end end
end end
......
...@@ -63,15 +63,18 @@ module Gitlab ...@@ -63,15 +63,18 @@ module Gitlab
diff_refs.try(:head_sha) diff_refs.try(:head_sha)
end end
attr_writer :highlighted_diff_lines
# Array of Gitlab::Diff::Line objects # Array of Gitlab::Diff::Line objects
def diff_lines def diff_lines
@lines ||= Gitlab::Diff::Parser.new.parse(raw_diff.each_line).to_a @diff_lines ||= Gitlab::Diff::Parser.new.parse(raw_diff.each_line).to_a
end end
def highlighted_diff_lines def highlighted_diff_lines
@highlighted_diff_lines ||= Gitlab::Diff::Highlight.new(self, repository: self.repository).highlight @highlighted_diff_lines ||= Gitlab::Diff::Highlight.new(self, repository: self.repository).highlight
end end
# Array[<Hash>] with right/left keys that contains Gitlab::Diff::Line objects which text is hightlighted
def parallel_diff_lines def parallel_diff_lines
@parallel_diff_lines ||= Gitlab::Diff::ParallelDiff.new(self).parallelize @parallel_diff_lines ||= Gitlab::Diff::ParallelDiff.new(self).parallelize
end end
......
module Gitlab
module Diff
module FileCollection
class Base
attr_reader :project, :diff_options, :diff_view, :diff_refs
delegate :count, :size, :real_size, to: :diff_files
def self.default_options
::Commit.max_diff_options.merge(ignore_whitespace_change: false, no_collapse: false)
end
def initialize(diffable, project:, diff_options: nil, diff_refs: nil)
diff_options = self.class.default_options.merge(diff_options || {})
@diffable = diffable
@diffs = diffable.raw_diffs(diff_options)
@project = project
@diff_options = diff_options
@diff_refs = diff_refs
end
def diff_files
@diff_files ||= @diffs.decorate! { |diff| decorate_diff!(diff) }
end
private
def decorate_diff!(diff)
Gitlab::Diff::File.new(diff, repository: project.repository, diff_refs: diff_refs)
end
end
end
end
end
module Gitlab
module Diff
module FileCollection
class Commit < Base
def initialize(commit, diff_options:)
super(commit,
project: commit.project,
diff_options: diff_options,
diff_refs: commit.diff_refs)
end
end
end
end
end
module Gitlab
module Diff
module FileCollection
class Compare < Base
def initialize(compare, project:, diff_options:, diff_refs: nil)
super(compare,
project: project,
diff_options: diff_options,
diff_refs: diff_refs)
end
end
end
end
end
module Gitlab
module Diff
module FileCollection
class MergeRequest < Base
def initialize(merge_request, diff_options:)
@merge_request = merge_request
super(merge_request,
project: merge_request.project,
diff_options: diff_options,
diff_refs: merge_request.diff_refs)
end
def diff_files
super.tap { |_| store_highlight_cache }
end
private
# Extracted method to highlight in the same iteration to the diff_collection.
def decorate_diff!(diff)
diff_file = super
cache_highlight!(diff_file) if cacheable?
diff_file
end
def highlight_diff_file_from_cache!(diff_file, cache_diff_lines)
diff_file.highlighted_diff_lines = cache_diff_lines.map do |line|
Gitlab::Diff::Line.init_from_hash(line)
end
end
#
# If we find the highlighted diff files lines on the cache we replace existing diff_files lines (no highlighted)
# for the highlighted ones, so we just skip their execution.
# If the highlighted diff files lines are not cached we calculate and cache them.
#
# The content of the cache is a Hash where the key correspond to the file_path and the values are Arrays of
# hashes that represent serialized diff lines.
#
def cache_highlight!(diff_file)
file_path = diff_file.file_path
if highlight_cache[file_path]
highlight_diff_file_from_cache!(diff_file, highlight_cache[file_path])
else
highlight_cache[file_path] = diff_file.highlighted_diff_lines.map(&:to_hash)
end
end
def highlight_cache
return @highlight_cache if defined?(@highlight_cache)
@highlight_cache = Rails.cache.read(cache_key) || {}
@highlight_cache_was_empty = @highlight_cache.empty?
@highlight_cache
end
def store_highlight_cache
Rails.cache.write(cache_key, highlight_cache) if @highlight_cache_was_empty
end
def cacheable?
@merge_request.merge_request_diff.present?
end
def cache_key
[@merge_request.merge_request_diff, 'highlighted-diff-files', diff_options]
end
end
end
end
end
...@@ -40,8 +40,6 @@ module Gitlab ...@@ -40,8 +40,6 @@ module Gitlab
def highlight_line(diff_line) def highlight_line(diff_line)
return unless diff_file && diff_file.diff_refs return unless diff_file && diff_file.diff_refs
line_prefix = diff_line.text.match(/\A(.)/) ? $1 : ' '
rich_line = rich_line =
if diff_line.unchanged? || diff_line.added? if diff_line.unchanged? || diff_line.added?
new_lines[diff_line.new_pos - 1] new_lines[diff_line.new_pos - 1]
...@@ -51,7 +49,10 @@ module Gitlab ...@@ -51,7 +49,10 @@ module Gitlab
# Only update text if line is found. This will prevent # Only update text if line is found. This will prevent
# issues with submodules given the line only exists in diff content. # issues with submodules given the line only exists in diff content.
"#{line_prefix}#{rich_line}".html_safe if rich_line if rich_line
line_prefix = diff_line.text.match(/\A(.)/) ? $1 : ' '
"#{line_prefix}#{rich_line}".html_safe
end
end end
def inline_diffs def inline_diffs
......
...@@ -9,6 +9,20 @@ module Gitlab ...@@ -9,6 +9,20 @@ module Gitlab
@old_pos, @new_pos = old_pos, new_pos @old_pos, @new_pos = old_pos, new_pos
end end
def self.init_from_hash(hash)
new(hash[:text], hash[:type], hash[:index], hash[:old_pos], hash[:new_pos])
end
def serialize_keys
@serialize_keys ||= %i(text type index old_pos new_pos)
end
def to_hash
hash = {}
serialize_keys.each { |key| hash[key] = send(key) }
hash
end
def old_line def old_line
old_pos unless added? || meta? old_pos unless added? || meta?
end end
......
...@@ -35,21 +35,22 @@ module Gitlab ...@@ -35,21 +35,22 @@ module Gitlab
def commits def commits
return unless compare return unless compare
@commits ||= Commit.decorate(compare.commits, project) @commits ||= compare.commits
end end
def diffs def diffs
return unless compare return unless compare
@diffs ||= safe_diff_files(compare.diffs(max_files: 30), diff_refs: diff_refs, repository: project.repository) # This diff is more moderated in number of files and lines
@diffs ||= compare.diffs(max_files: 30, max_lines: 5000, no_collapse: true).diff_files
end end
def diffs_count def diffs_count
diffs.count if diffs diffs.size if diffs
end end
def compare def compare
@opts[:compare] @opts[:compare] if @opts[:compare]
end end
def diff_refs def diff_refs
...@@ -97,16 +98,18 @@ module Gitlab ...@@ -97,16 +98,18 @@ module Gitlab
if commits.length > 1 if commits.length > 1
namespace_project_compare_url(project_namespace, namespace_project_compare_url(project_namespace,
project, project,
from: Commit.new(compare.base, project), from: compare.start_commit,
to: Commit.new(compare.head, project)) to: compare.head_commit)
else else
namespace_project_commit_url(project_namespace, namespace_project_commit_url(project_namespace,
project, commits.first) project,
commits.first)
end end
else else
unless @action == :delete unless @action == :delete
namespace_project_tree_url(project_namespace, namespace_project_tree_url(project_namespace,
project, ref_name) project,
ref_name)
end end
end end
end end
......
...@@ -83,7 +83,7 @@ describe Projects::CommitController do ...@@ -83,7 +83,7 @@ describe Projects::CommitController do
let(:format) { :diff } let(:format) { :diff }
it "should really only be a git diff" do it "should really only be a git diff" do
go(id: commit.id, format: format) go(id: '66eceea0db202bb39c4e445e8ca28689645366c5', format: format)
expect(response.body).to start_with("diff --git") expect(response.body).to start_with("diff --git")
end end
...@@ -92,8 +92,9 @@ describe Projects::CommitController do ...@@ -92,8 +92,9 @@ describe Projects::CommitController do
go(id: '66eceea0db202bb39c4e445e8ca28689645366c5', format: format, w: 1) go(id: '66eceea0db202bb39c4e445e8ca28689645366c5', format: format, w: 1)
expect(response.body).to start_with("diff --git") expect(response.body).to start_with("diff --git")
# without whitespace option, there are more than 2 diff_splits
diff_splits = assigns(:diffs).first.diff.split("\n") # without whitespace option, there are more than 2 diff_splits for other formats
diff_splits = assigns(:diffs).diff_files.first.diff.diff.split("\n")
expect(diff_splits.length).to be <= 2 expect(diff_splits.length).to be <= 2
end end
end end
...@@ -266,9 +267,9 @@ describe Projects::CommitController do ...@@ -266,9 +267,9 @@ describe Projects::CommitController do
end end
it 'only renders the diffs for the path given' do it 'only renders the diffs for the path given' do
expect(controller).to receive(:render_diff_for_path).and_wrap_original do |meth, diffs, diff_refs, project| expect(controller).to receive(:render_diff_for_path).and_wrap_original do |meth, diffs|
expect(diffs.map(&:new_path)).to contain_exactly(existing_path) expect(diffs.diff_files.map(&:new_path)).to contain_exactly(existing_path)
meth.call(diffs, diff_refs, project) meth.call(diffs)
end end
diff_for_path(id: commit.id, old_path: existing_path, new_path: existing_path) diff_for_path(id: commit.id, old_path: existing_path, new_path: existing_path)
......
...@@ -19,7 +19,7 @@ describe Projects::CompareController do ...@@ -19,7 +19,7 @@ describe Projects::CompareController do
to: ref_to) to: ref_to)
expect(response).to be_success expect(response).to be_success
expect(assigns(:diffs).first).not_to be_nil expect(assigns(:diffs).diff_files.first).not_to be_nil
expect(assigns(:commits).length).to be >= 1 expect(assigns(:commits).length).to be >= 1
end end
...@@ -32,10 +32,11 @@ describe Projects::CompareController do ...@@ -32,10 +32,11 @@ describe Projects::CompareController do
w: 1) w: 1)
expect(response).to be_success expect(response).to be_success
expect(assigns(:diffs).first).not_to be_nil diff_file = assigns(:diffs).diff_files.first
expect(diff_file).not_to be_nil
expect(assigns(:commits).length).to be >= 1 expect(assigns(:commits).length).to be >= 1
# without whitespace option, there are more than 2 diff_splits # without whitespace option, there are more than 2 diff_splits
diff_splits = assigns(:diffs).first.diff.split("\n") diff_splits = diff_file.diff.diff.split("\n")
expect(diff_splits.length).to be <= 2 expect(diff_splits.length).to be <= 2
end end
...@@ -48,7 +49,7 @@ describe Projects::CompareController do ...@@ -48,7 +49,7 @@ describe Projects::CompareController do
to: ref_to) to: ref_to)
expect(response).to be_success expect(response).to be_success
expect(assigns(:diffs).to_a).to eq([]) expect(assigns(:diffs).diff_files.to_a).to eq([])
expect(assigns(:commits)).to eq([]) expect(assigns(:commits)).to eq([])
end end
...@@ -87,9 +88,9 @@ describe Projects::CompareController do ...@@ -87,9 +88,9 @@ describe Projects::CompareController do
end end
it 'only renders the diffs for the path given' do it 'only renders the diffs for the path given' do
expect(controller).to receive(:render_diff_for_path).and_wrap_original do |meth, diffs, diff_refs, project| expect(controller).to receive(:render_diff_for_path).and_wrap_original do |meth, diffs|
expect(diffs.map(&:new_path)).to contain_exactly(existing_path) expect(diffs.diff_files.map(&:new_path)).to contain_exactly(existing_path)
meth.call(diffs, diff_refs, project) meth.call(diffs)
end end
diff_for_path(from: ref_from, to: ref_to, old_path: existing_path, new_path: existing_path) diff_for_path(from: ref_from, to: ref_to, old_path: existing_path, new_path: existing_path)
......
...@@ -392,9 +392,9 @@ describe Projects::MergeRequestsController do ...@@ -392,9 +392,9 @@ describe Projects::MergeRequestsController do
end end
it 'only renders the diffs for the path given' do it 'only renders the diffs for the path given' do
expect(controller).to receive(:render_diff_for_path).and_wrap_original do |meth, diffs, diff_refs, project| expect(controller).to receive(:render_diff_for_path).and_wrap_original do |meth, diffs|
expect(diffs.map(&:new_path)).to contain_exactly(existing_path) expect(diffs.diff_files.map(&:new_path)).to contain_exactly(existing_path)
meth.call(diffs, diff_refs, project) meth.call(diffs)
end end
diff_for_path(id: merge_request.iid, old_path: existing_path, new_path: existing_path) diff_for_path(id: merge_request.iid, old_path: existing_path, new_path: existing_path)
...@@ -455,9 +455,9 @@ describe Projects::MergeRequestsController do ...@@ -455,9 +455,9 @@ describe Projects::MergeRequestsController do
end end
it 'only renders the diffs for the path given' do it 'only renders the diffs for the path given' do
expect(controller).to receive(:render_diff_for_path).and_wrap_original do |meth, diffs, diff_refs, project| expect(controller).to receive(:render_diff_for_path).and_wrap_original do |meth, diffs|
expect(diffs.map(&:new_path)).to contain_exactly(existing_path) expect(diffs.diff_files.map(&:new_path)).to contain_exactly(existing_path)
meth.call(diffs, diff_refs, project) meth.call(diffs)
end end
diff_for_path(old_path: existing_path, new_path: existing_path, merge_request: { source_branch: 'feature', target_branch: 'master' }) diff_for_path(old_path: existing_path, new_path: existing_path, merge_request: { source_branch: 'feature', target_branch: 'master' })
...@@ -477,9 +477,9 @@ describe Projects::MergeRequestsController do ...@@ -477,9 +477,9 @@ describe Projects::MergeRequestsController do
end end
it 'only renders the diffs for the path given' do it 'only renders the diffs for the path given' do
expect(controller).to receive(:render_diff_for_path).and_wrap_original do |meth, diffs, diff_refs, project| expect(controller).to receive(:render_diff_for_path).and_wrap_original do |meth, diffs|
expect(diffs.map(&:new_path)).to contain_exactly(existing_path) expect(diffs.diff_files.map(&:new_path)).to contain_exactly(existing_path)
meth.call(diffs, diff_refs, project) meth.call(diffs)
end end
diff_for_path(old_path: existing_path, new_path: existing_path, merge_request: { source_project: other_project, source_branch: 'feature', target_branch: 'master' }) diff_for_path(old_path: existing_path, new_path: existing_path, merge_request: { source_project: other_project, source_branch: 'feature', target_branch: 'master' })
......
...@@ -6,7 +6,7 @@ describe DiffHelper do ...@@ -6,7 +6,7 @@ describe DiffHelper do
let(:project) { create(:project) } let(:project) { create(:project) }
let(:repository) { project.repository } let(:repository) { project.repository }
let(:commit) { project.commit(sample_commit.id) } let(:commit) { project.commit(sample_commit.id) }
let(:diffs) { commit.diffs } let(:diffs) { commit.raw_diffs }
let(:diff) { diffs.first } let(:diff) { diffs.first }
let(:diff_refs) { [commit.parent, commit] } let(:diff_refs) { [commit.parent, commit] }
let(:diff_file) { Gitlab::Diff::File.new(diff, diff_refs: diff_refs, repository: repository) } let(:diff_file) { Gitlab::Diff::File.new(diff, diff_refs: diff_refs, repository: repository) }
...@@ -32,16 +32,6 @@ describe DiffHelper do ...@@ -32,16 +32,6 @@ describe DiffHelper do
end end
describe 'diff_options' do describe 'diff_options' do
it 'should return hard limit for a diff if force diff is true' do
allow(controller).to receive(:params) { { force_show_diff: true } }
expect(diff_options).to include(Commit.max_diff_options)
end
it 'should return hard limit for a diff if expand_all_diffs is true' do
allow(controller).to receive(:params) { { expand_all_diffs: true } }
expect(diff_options).to include(Commit.max_diff_options)
end
it 'should return no collapse false' do it 'should return no collapse false' do
expect(diff_options).to include(no_collapse: false) expect(diff_options).to include(no_collapse: false)
end end
...@@ -55,6 +45,18 @@ describe DiffHelper do ...@@ -55,6 +45,18 @@ describe DiffHelper do
allow(controller).to receive(:action_name) { 'diff_for_path' } allow(controller).to receive(:action_name) { 'diff_for_path' }
expect(diff_options).to include(no_collapse: true) expect(diff_options).to include(no_collapse: true)
end end
it 'should return paths if action name diff_for_path and param old path' do
allow(controller).to receive(:params) { { old_path: 'lib/wadus.rb' } }
allow(controller).to receive(:action_name) { 'diff_for_path' }
expect(diff_options[:paths]).to include('lib/wadus.rb')
end
it 'should return paths if action name diff_for_path and param new path' do
allow(controller).to receive(:params) { { new_path: 'lib/wadus.rb' } }
allow(controller).to receive(:action_name) { 'diff_for_path' }
expect(diff_options[:paths]).to include('lib/wadus.rb')
end
end end
describe 'unfold_bottom_class' do describe 'unfold_bottom_class' do
......
...@@ -5,7 +5,7 @@ describe Gitlab::Diff::File, lib: true do ...@@ -5,7 +5,7 @@ describe Gitlab::Diff::File, lib: true do
let(:project) { create(:project) } let(:project) { create(:project) }
let(:commit) { project.commit(sample_commit.id) } let(:commit) { project.commit(sample_commit.id) }
let(:diff) { commit.diffs.first } let(:diff) { commit.raw_diffs.first }
let(:diff_file) { Gitlab::Diff::File.new(diff, diff_refs: commit.diff_refs, repository: project.repository) } let(:diff_file) { Gitlab::Diff::File.new(diff, diff_refs: commit.diff_refs, repository: project.repository) }
describe '#diff_lines' do describe '#diff_lines' do
......
...@@ -5,7 +5,7 @@ describe Gitlab::Diff::Highlight, lib: true do ...@@ -5,7 +5,7 @@ describe Gitlab::Diff::Highlight, lib: true do
let(:project) { create(:project) } let(:project) { create(:project) }
let(:commit) { project.commit(sample_commit.id) } let(:commit) { project.commit(sample_commit.id) }
let(:diff) { commit.diffs.first } let(:diff) { commit.raw_diffs.first }
let(:diff_file) { Gitlab::Diff::File.new(diff, diff_refs: commit.diff_refs, repository: project.repository) } let(:diff_file) { Gitlab::Diff::File.new(diff, diff_refs: commit.diff_refs, repository: project.repository) }
describe '#highlight' do describe '#highlight' do
......
...@@ -6,7 +6,7 @@ describe Gitlab::Diff::LineMapper, lib: true do ...@@ -6,7 +6,7 @@ describe Gitlab::Diff::LineMapper, lib: true do
let(:project) { create(:project) } let(:project) { create(:project) }
let(:repository) { project.repository } let(:repository) { project.repository }
let(:commit) { project.commit(sample_commit.id) } let(:commit) { project.commit(sample_commit.id) }
let(:diffs) { commit.diffs } let(:diffs) { commit.raw_diffs }
let(:diff) { diffs.first } let(:diff) { diffs.first }
let(:diff_file) { Gitlab::Diff::File.new(diff, diff_refs: commit.diff_refs, repository: repository) } let(:diff_file) { Gitlab::Diff::File.new(diff, diff_refs: commit.diff_refs, repository: repository) }
subject { described_class.new(diff_file) } subject { described_class.new(diff_file) }
......
...@@ -6,7 +6,7 @@ describe Gitlab::Diff::ParallelDiff, lib: true do ...@@ -6,7 +6,7 @@ describe Gitlab::Diff::ParallelDiff, lib: true do
let(:project) { create(:project) } let(:project) { create(:project) }
let(:repository) { project.repository } let(:repository) { project.repository }
let(:commit) { project.commit(sample_commit.id) } let(:commit) { project.commit(sample_commit.id) }
let(:diffs) { commit.diffs } let(:diffs) { commit.raw_diffs }
let(:diff) { diffs.first } let(:diff) { diffs.first }
let(:diff_file) { Gitlab::Diff::File.new(diff, diff_refs: commit.diff_refs, repository: repository) } let(:diff_file) { Gitlab::Diff::File.new(diff, diff_refs: commit.diff_refs, repository: repository) }
subject { described_class.new(diff_file) } subject { described_class.new(diff_file) }
......
...@@ -5,7 +5,7 @@ describe Gitlab::Diff::Parser, lib: true do ...@@ -5,7 +5,7 @@ describe Gitlab::Diff::Parser, lib: true do
let(:project) { create(:project) } let(:project) { create(:project) }
let(:commit) { project.commit(sample_commit.id) } let(:commit) { project.commit(sample_commit.id) }
let(:diff) { commit.diffs.first } let(:diff) { commit.raw_diffs.first }
let(:parser) { Gitlab::Diff::Parser.new } let(:parser) { Gitlab::Diff::Parser.new }
describe '#parse' do describe '#parse' do
......
...@@ -16,9 +16,12 @@ describe Gitlab::Email::Message::RepositoryPush do ...@@ -16,9 +16,12 @@ describe Gitlab::Email::Message::RepositoryPush do
{ author_id: author.id, ref: 'master', action: :push, compare: compare, { author_id: author.id, ref: 'master', action: :push, compare: compare,
send_from_committer_email: true } send_from_committer_email: true }
end end
let(:compare) do let(:raw_compare) do
Gitlab::Git::Compare.new(project.repository.raw_repository, Gitlab::Git::Compare.new(project.repository.raw_repository,
sample_image_commit.id, sample_commit.id) sample_image_commit.id, sample_commit.id)
end
let(:compare) do
Compare.decorate(raw_compare, project)
end end
describe '#project' do describe '#project' do
...@@ -62,17 +65,17 @@ describe Gitlab::Email::Message::RepositoryPush do ...@@ -62,17 +65,17 @@ describe Gitlab::Email::Message::RepositoryPush do
describe '#diffs_count' do describe '#diffs_count' do
subject { message.diffs_count } subject { message.diffs_count }
it { is_expected.to eq compare.diffs.count } it { is_expected.to eq raw_compare.diffs.size }
end end
describe '#compare' do describe '#compare' do
subject { message.compare } subject { message.compare }
it { is_expected.to be_an_instance_of Gitlab::Git::Compare } it { is_expected.to be_an_instance_of Compare }
end end
describe '#compare_timeout' do describe '#compare_timeout' do
subject { message.compare_timeout } subject { message.compare_timeout }
it { is_expected.to eq compare.diffs.overflow? } it { is_expected.to eq raw_compare.diffs.overflow? }
end end
describe '#reverse_compare?' do describe '#reverse_compare?' do
......
...@@ -944,8 +944,9 @@ describe Notify do ...@@ -944,8 +944,9 @@ describe Notify do
describe 'email on push with multiple commits' do describe 'email on push with multiple commits' do
let(:example_site_path) { root_path } let(:example_site_path) { root_path }
let(:user) { create(:user) } let(:user) { create(:user) }
let(:compare) { Gitlab::Git::Compare.new(project.repository.raw_repository, sample_image_commit.id, sample_commit.id) } let(:raw_compare) { Gitlab::Git::Compare.new(project.repository.raw_repository, sample_image_commit.id, sample_commit.id) }
let(:commits) { Commit.decorate(compare.commits, nil) } let(:compare) { Compare.decorate(raw_compare, project) }
let(:commits) { compare.commits }
let(:diff_path) { namespace_project_compare_path(project.namespace, project, from: Commit.new(compare.base, project), to: Commit.new(compare.head, project)) } let(:diff_path) { namespace_project_compare_path(project.namespace, project, from: Commit.new(compare.base, project), to: Commit.new(compare.head, project)) }
let(:send_from_committer_email) { false } let(:send_from_committer_email) { false }
let(:diff_refs) { Gitlab::Diff::DiffRefs.new(base_sha: project.merge_base_commit(sample_image_commit.id, sample_commit.id).id, head_sha: sample_commit.id) } let(:diff_refs) { Gitlab::Diff::DiffRefs.new(base_sha: project.merge_base_commit(sample_image_commit.id, sample_commit.id).id, head_sha: sample_commit.id) }
...@@ -1046,8 +1047,9 @@ describe Notify do ...@@ -1046,8 +1047,9 @@ describe Notify do
describe 'email on push with a single commit' do describe 'email on push with a single commit' do
let(:example_site_path) { root_path } let(:example_site_path) { root_path }
let(:user) { create(:user) } let(:user) { create(:user) }
let(:compare) { Gitlab::Git::Compare.new(project.repository.raw_repository, sample_commit.parent_id, sample_commit.id) } let(:raw_compare) { Gitlab::Git::Compare.new(project.repository.raw_repository, sample_commit.parent_id, sample_commit.id) }
let(:commits) { Commit.decorate(compare.commits, nil) } let(:compare) { Compare.decorate(raw_compare, project) }
let(:commits) { compare.commits }
let(:diff_path) { namespace_project_commit_path(project.namespace, project, commits.first) } let(:diff_path) { namespace_project_commit_path(project.namespace, project, commits.first) }
let(:diff_refs) { Gitlab::Diff::DiffRefs.new(base_sha: project.merge_base_commit(sample_image_commit.id, sample_commit.id).id, head_sha: sample_commit.id) } let(:diff_refs) { Gitlab::Diff::DiffRefs.new(base_sha: project.merge_base_commit(sample_image_commit.id, sample_commit.id).id, head_sha: sample_commit.id) }
......
require 'spec_helper'
describe Compare, models: true do
include RepoHelpers
let(:project) { create(:project, :public) }
let(:commit) { project.commit }
let(:start_commit) { sample_image_commit }
let(:head_commit) { sample_commit }
let(:raw_compare) { Gitlab::Git::Compare.new(project.repository.raw_repository, start_commit.id, head_commit.id) }
subject { described_class.new(raw_compare, project) }
describe '#start_commit' do
it 'returns raw compare base commit' do
expect(subject.start_commit.id).to eq(start_commit.id)
end
it 'returns nil if compare base commit is nil' do
expect(raw_compare).to receive(:base).and_return(nil)
expect(subject.start_commit).to eq(nil)
end
end
describe '#commit' do
it 'returns raw compare head commit' do
expect(subject.commit.id).to eq(head_commit.id)
end
it 'returns nil if compare head commit is nil' do
expect(raw_compare).to receive(:head).and_return(nil)
expect(subject.commit).to eq(nil)
end
end
describe '#base_commit' do
let(:base_commit) { Commit.new(another_sample_commit, project) }
it 'returns project merge base commit' do
expect(project).to receive(:merge_base_commit).with(start_commit.id, head_commit.id).and_return(base_commit)
expect(subject.base_commit).to eq(base_commit)
end
it 'returns nil if there is no start_commit' do
expect(subject).to receive(:start_commit).and_return(nil)
expect(subject.base_commit).to eq(nil)
end
it 'returns nil if there is no head commit' do
expect(subject).to receive(:head_commit).and_return(nil)
expect(subject.base_commit).to eq(nil)
end
end
describe '#diff_refs' do
it 'uses base_commit sha as base_sha' do
expect(subject).to receive(:base_commit).at_least(:once).and_call_original
expect(subject.diff_refs.base_sha).to eq(subject.base_commit.id)
end
it 'uses start_commit sha as start_sha' do
expect(subject.diff_refs.start_sha).to eq(start_commit.id)
end
it 'uses commit sha as head sha' do
expect(subject.diff_refs.head_sha).to eq(head_commit.id)
end
end
end
...@@ -58,7 +58,7 @@ describe LegacyDiffNote, models: true do ...@@ -58,7 +58,7 @@ describe LegacyDiffNote, models: true do
# Generate a real line_code value so we know it will match. We use a # Generate a real line_code value so we know it will match. We use a
# random line from a random diff just for funsies. # random line from a random diff just for funsies.
diff = merge.diffs.to_a.sample diff = merge.raw_diffs.to_a.sample
line = Gitlab::Diff::Parser.new.parse(diff.diff.each_line).to_a.sample line = Gitlab::Diff::Parser.new.parse(diff.diff.each_line).to_a.sample
code = Gitlab::Diff::LineCode.generate(diff.new_path, line.new_pos, line.old_pos) code = Gitlab::Diff::LineCode.generate(diff.new_path, line.new_pos, line.old_pos)
......
...@@ -128,7 +128,7 @@ describe MergeRequest, models: true do ...@@ -128,7 +128,7 @@ describe MergeRequest, models: true do
end end
end end
describe '#diffs' do describe '#raw_diffs' do
let(:merge_request) { build(:merge_request) } let(:merge_request) { build(:merge_request) }
let(:options) { { paths: ['a/b', 'b/a', 'c/*'] } } let(:options) { { paths: ['a/b', 'b/a', 'c/*'] } }
...@@ -138,6 +138,31 @@ describe MergeRequest, models: true do ...@@ -138,6 +138,31 @@ describe MergeRequest, models: true do
expect(merge_request.merge_request_diff).to receive(:diffs).with(options) expect(merge_request.merge_request_diff).to receive(:diffs).with(options)
merge_request.raw_diffs(options)
end
end
context 'when there are no MR diffs' do
it 'delegates to the compare object' do
merge_request.compare = double(:compare)
expect(merge_request.compare).to receive(:raw_diffs).with(options)
merge_request.raw_diffs(options)
end
end
end
describe '#diffs' do
let(:merge_request) { build(:merge_request) }
let(:options) { { paths: ['a/b', 'b/a', 'c/*'] } }
context 'when there are MR diffs' do
it 'delegates to the MR diffs' do
merge_request.merge_request_diff = MergeRequestDiff.new
expect(merge_request.merge_request_diff).to receive(:diffs).with(hash_including(options))
merge_request.diffs(options) merge_request.diffs(options)
end end
end end
...@@ -660,6 +685,12 @@ describe MergeRequest, models: true do ...@@ -660,6 +685,12 @@ describe MergeRequest, models: true do
subject.reload_diff subject.reload_diff
end end
it "executs diff cache service" do
expect_any_instance_of(MergeRequests::MergeRequestDiffCacheService).to receive(:execute).with(subject)
subject.reload_diff
end
it "updates diff note positions" do it "updates diff note positions" do
old_diff_refs = subject.diff_refs old_diff_refs = subject.diff_refs
......
...@@ -1154,7 +1154,7 @@ describe Repository, models: true do ...@@ -1154,7 +1154,7 @@ describe Repository, models: true do
it 'does not flush the cache if the commit does not change any logos' do it 'does not flush the cache if the commit does not change any logos' do
diff = double(:diff, new_path: 'test.txt') diff = double(:diff, new_path: 'test.txt')
expect(commit).to receive(:diffs).and_return([diff]) expect(commit).to receive(:raw_diffs).and_return([diff])
expect(cache).not_to receive(:expire) expect(cache).not_to receive(:expire)
repository.expire_avatar_cache(repository.root_ref, '123') repository.expire_avatar_cache(repository.root_ref, '123')
...@@ -1163,7 +1163,7 @@ describe Repository, models: true do ...@@ -1163,7 +1163,7 @@ describe Repository, models: true do
it 'flushes the cache if the commit changes any of the logos' do it 'flushes the cache if the commit changes any of the logos' do
diff = double(:diff, new_path: Repository::AVATAR_FILES[0]) diff = double(:diff, new_path: Repository::AVATAR_FILES[0])
expect(commit).to receive(:diffs).and_return([diff]) expect(commit).to receive(:raw_diffs).and_return([diff])
expect(cache).to receive(:expire).with(:avatar) expect(cache).to receive(:expire).with(:avatar)
repository.expire_avatar_cache(repository.root_ref, '123') repository.expire_avatar_cache(repository.root_ref, '123')
......
...@@ -173,10 +173,10 @@ describe API::API, api: true do ...@@ -173,10 +173,10 @@ describe API::API, api: true do
end end
it 'should return the inline comment' do it 'should return the inline comment' do
post api("/projects/#{project.id}/repository/commits/#{project.repository.commit.id}/comments", user), note: 'My comment', path: project.repository.commit.diffs.first.new_path, line: 7, line_type: 'new' post api("/projects/#{project.id}/repository/commits/#{project.repository.commit.id}/comments", user), note: 'My comment', path: project.repository.commit.raw_diffs.first.new_path, line: 7, line_type: 'new'
expect(response).to have_http_status(201) expect(response).to have_http_status(201)
expect(json_response['note']).to eq('My comment') expect(json_response['note']).to eq('My comment')
expect(json_response['path']).to eq(project.repository.commit.diffs.first.new_path) expect(json_response['path']).to eq(project.repository.commit.raw_diffs.first.new_path)
expect(json_response['line']).to eq(7) expect(json_response['line']).to eq(7)
expect(json_response['line_type']).to eq('new') expect(json_response['line_type']).to eq('new')
end end
......
...@@ -61,7 +61,7 @@ describe MergeRequests::BuildService, services: true do ...@@ -61,7 +61,7 @@ describe MergeRequests::BuildService, services: true do
end end
context 'one commit in the diff' do context 'one commit in the diff' do
let(:commits) { [commit_1] } let(:commits) { Commit.decorate([commit_1], project) }
it 'allows the merge request to be created' do it 'allows the merge request to be created' do
expect(merge_request.can_be_created).to eq(true) expect(merge_request.can_be_created).to eq(true)
...@@ -84,7 +84,7 @@ describe MergeRequests::BuildService, services: true do ...@@ -84,7 +84,7 @@ describe MergeRequests::BuildService, services: true do
end end
context 'commit has no description' do context 'commit has no description' do
let(:commits) { [commit_2] } let(:commits) { Commit.decorate([commit_2], project) }
it 'uses the title of the commit as the title of the merge request' do it 'uses the title of the commit as the title of the merge request' do
expect(merge_request.title).to eq(commit_2.safe_message) expect(merge_request.title).to eq(commit_2.safe_message)
...@@ -111,7 +111,7 @@ describe MergeRequests::BuildService, services: true do ...@@ -111,7 +111,7 @@ describe MergeRequests::BuildService, services: true do
end end
context 'commit has no description' do context 'commit has no description' do
let(:commits) { [commit_2] } let(:commits) { Commit.decorate([commit_2], project) }
it 'sets the description to "Closes #$issue-iid"' do it 'sets the description to "Closes #$issue-iid"' do
expect(merge_request.description).to eq("Closes ##{issue.iid}") expect(merge_request.description).to eq("Closes ##{issue.iid}")
...@@ -121,7 +121,7 @@ describe MergeRequests::BuildService, services: true do ...@@ -121,7 +121,7 @@ describe MergeRequests::BuildService, services: true do
end end
context 'more than one commit in the diff' do context 'more than one commit in the diff' do
let(:commits) { [commit_1, commit_2] } let(:commits) { Commit.decorate([commit_1, commit_2], project) }
it 'allows the merge request to be created' do it 'allows the merge request to be created' do
expect(merge_request.can_be_created).to eq(true) expect(merge_request.can_be_created).to eq(true)
......
require 'spec_helper'
describe MergeRequests::MergeRequestDiffCacheService do
let(:subject) { MergeRequests::MergeRequestDiffCacheService.new }
describe '#execute' do
it 'retrieves the diff files to cache the highlighted result' do
merge_request = create(:merge_request)
cache_key = [merge_request.merge_request_diff, 'highlighted-diff-files', Gitlab::Diff::FileCollection::MergeRequest.default_options]
expect(Rails.cache).to receive(:read).with(cache_key).and_return({})
expect(Rails.cache).to receive(:write).with(cache_key, anything)
subject.execute(merge_request)
end
end
end
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment