Commit c86c1905 authored by Paco Guzman's avatar Paco Guzman

switch from diff_file_collection to diffs

So we have raw_diffs too
parent 1d0c7b74
......@@ -28,7 +28,7 @@ class Projects::CommitController < Projects::ApplicationController
end
def diff_for_path
render_diff_for_path(@commit.diff_file_collection(diff_options))
render_diff_for_path(@commit.diffs(diff_options))
end
def builds
......@@ -110,7 +110,7 @@ class Projects::CommitController < Projects::ApplicationController
opts = diff_options
opts[:ignore_whitespace_change] = true if params[:format] == 'diff'
@diffs = commit.diff_file_collection(opts)
@diffs = commit.diffs(opts)
@notes_count = commit.notes.count
end
......
......@@ -21,7 +21,7 @@ class Projects::CompareController < Projects::ApplicationController
def diff_for_path
return render_404 unless @compare
render_diff_for_path(@compare.diff_file_collection(diff_options: diff_options))
render_diff_for_path(@compare.diffs(diff_options: diff_options))
end
def create
......@@ -45,7 +45,7 @@ class Projects::CompareController < Projects::ApplicationController
@commit = @compare.commit
@base_commit = @compare.base_commit
@diffs = @compare.diff_file_collection(diff_options: diff_options)
@diffs = @compare.diffs(diff_options: diff_options)
@diff_notes_disabled = true
@grouped_diff_discussions = {}
......
......@@ -86,7 +86,7 @@ class Projects::MergeRequestsController < Projects::ApplicationController
respond_to do |format|
format.html { define_discussion_vars }
format.json do
@diffs = @merge_request.diff_file_collection(diff_options)
@diffs = @merge_request.diffs(diff_options)
render json: { html: view_to_html_string("projects/merge_requests/show/_diffs") }
end
......@@ -108,7 +108,7 @@ class Projects::MergeRequestsController < Projects::ApplicationController
define_commit_vars
render_diff_for_path(@merge_request.diff_file_collection(diff_options))
render_diff_for_path(@merge_request.diffs(diff_options))
end
def commits
......@@ -156,9 +156,7 @@ class Projects::MergeRequestsController < Projects::ApplicationController
@commits = @merge_request.compare_commits.reverse
@commit = @merge_request.diff_head_commit
@base_commit = @merge_request.diff_base_commit
if @merge_request.compare
@diffs = @merge_request.diff_file_collection(diff_options)
end
@diffs = @merge_request.diffs(diff_options) if @merge_request.compare
@diff_notes_disabled = true
@pipeline = @merge_request.pipeline
......
......@@ -23,10 +23,7 @@ module DiffHelper
end
def diff_options
options = Gitlab::Diff::FileCollection.default_options.merge(
ignore_whitespace_change: hide_whitespace?,
no_collapse: expand_all_diffs?
)
options = { ignore_whitespace_change: hide_whitespace?, no_collapse: expand_all_diffs? }
if action_name == 'diff_for_path'
options[:no_collapse] = true
......
......@@ -104,7 +104,7 @@ class Commit
end
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
end
......@@ -317,7 +317,11 @@ class Commit
nil
end
def diff_file_collection(diff_options = nil)
def raw_diffs(*args)
raw.diffs(*args)
end
def diffs(diff_options = nil)
Gitlab::Diff::FileCollection::Commit.new(self, diff_options: diff_options)
end
......@@ -330,7 +334,7 @@ class Commit
def repo_changes
changes = { added: [], modified: [], removed: [] }
diffs.each do |diff|
raw_diffs.each do |diff|
if diff.deleted_file
changes[:removed] << diff.old_path
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
......@@ -15,49 +17,47 @@ class Compare
end
def commits
@commits ||= Commit.decorate(@compare.commits, @project)
@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
@start_commit = commit ? ::Commit.new(commit, project) : nil
end
def commit
return @commit if defined?(@commit)
def head_commit
return @head_commit if defined?(@head_commit)
commit = @compare.head
@commit = commit ? ::Commit.new(commit, @project) : nil
end
alias_method :head_commit, :commit
# Used only on emails_on_push_worker.rb
def base_commit=(commit)
@base_commit = commit
@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 && commit
@project.merge_base_commit(start_commit.id, commit.id)
@base_commit = if start_commit && head_commit
project.merge_base_commit(start_commit.id, head_commit.id)
else
nil
end
end
# keyword args until we get ride of diff_refs as argument
def diff_file_collection(diff_options:, diff_refs: self.diff_refs)
Gitlab::Diff::FileCollection::Compare.new(@compare,
project: @project,
def raw_diffs(*args)
@compare.diffs(*args)
end
def diffs(diff_options:)
Gitlab::Diff::FileCollection::Compare.new(self,
project: project,
diff_options: diff_options,
diff_refs: diff_refs)
end
def diff_refs
@diff_refs ||= Gitlab::Diff::DiffRefs.new(
Gitlab::Diff::DiffRefs.new(
base_sha: base_commit.try(:sha),
start_sha: start_commit.try(:sha),
head_sha: commit.try(:sha)
......
......@@ -85,7 +85,7 @@ class LegacyDiffNote < Note
return nil unless noteable
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
end
end
......@@ -116,7 +116,7 @@ class LegacyDiffNote < Note
# Find the diff on noteable that matches our own
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 }
end
end
......@@ -164,13 +164,13 @@ class MergeRequest < ActiveRecord::Base
merge_request_diff ? merge_request_diff.first_commit : compare_commits.first
end
def diffs(*args)
merge_request_diff ? merge_request_diff.diffs(*args) : compare.diffs(*args)
def raw_diffs(*args)
merge_request_diff ? merge_request_diff.diffs(*args) : compare.raw_diffs(*args)
end
def diff_file_collection(diff_options = nil)
def diffs(diff_options = nil)
if self.compare
self.compare.diff_file_collection(diff_options: diff_options, diff_refs: diff_refs)
self.compare.diffs(diff_options: diff_options)
else
Gitlab::Diff::FileCollection::MergeRequest.new(self, diff_options: diff_options)
end
......
......@@ -372,7 +372,7 @@ class Repository
# We don't want to flush the cache if the commit didn't actually make any
# changes to any of the possible avatar files.
if revision && commit = self.commit(revision)
return unless commit.diffs.
return unless commit.raw_diffs.
any? { |diff| AVATAR_FILES.include?(diff.new_path) }
end
......
......@@ -26,7 +26,6 @@ class CompareService
source_sha
)
# REVIEW be sure if it's target_project or source_project
Compare.new(raw_compare, target_project)
end
end
......@@ -2,7 +2,7 @@ module MergeRequests
class MergeRequestDiffCacheService
def execute(merge_request)
# Executing the iteration we cache all the highlighted diff information
merge_request.diff_file_collection.diff_files.to_a
merge_request.diffs.diff_files.to_a
end
end
end
......@@ -9,11 +9,11 @@
= link_to 'Expand all', url_for(params.merge(expand_all_diffs: 1, format: nil)), class: 'btn btn-default'
- if show_whitespace_toggle
- 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)
= 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)
= 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
= inline_diff_btn
= parallel_diff_btn
......@@ -22,12 +22,12 @@
- if diff_files.overflow?
= 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_commit = commit_for_diff(diff_file)
- blob = diff_file.blob(diff_commit)
- 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,
diff_file: diff_file, diff_commit: diff_commit, blob: blob, diff_refs: diffs.diff_refs
= render 'projects/diffs/file', i: index, project: diffs.project,
diff_file: diff_file, diff_commit: diff_commit, blob: blob
- if @merge_request_diff.collected?
= render "projects/diffs/diffs", diffs: @diffs,
diff_refs: @diffs.diff_refs, project: @diffs.project
= render "projects/diffs/diffs", diffs: @diffs
- elsif @merge_request_diff.empty?
.nothing-here-block Nothing to merge from #{@merge_request.source_branch} into #{@merge_request.target_branch}
- else
......
......@@ -33,18 +33,13 @@ class EmailsOnPushWorker
reverse_compare = false
if action == :push
base_commit = project.merge_base_commit(before_sha, after_sha)
compare = Gitlab::Git::Compare.new(project.repository.raw_repository, before_sha, after_sha)
compare = Compare.decorate(compare, project)
compare.base_commit = base_commit
compare = CompareService.new.execute(project, before_sha, project, after_sha)
diff_refs = compare.diff_refs
return false if compare.same
if compare.commits.empty?
compare = Gitlab::Git::Compare.new(project.repository.raw_repository, after_sha, before_sha)
compare = Compare.decorate(compare, project)
compare.base_commit = base_commit
compare = CompareService.new.execute(project, after_sha, project, before_sha)
diff_refs = compare.diff_refs
reverse_compare = true
......
......@@ -141,8 +141,10 @@ class IrkerWorker
end
def files_count(commit)
files = "#{commit.diffs.real_size} file"
files += 's' if commit.diffs.size > 1
diffs = commit.raw_diffs
files = "#{diffs.real_size} file"
files += 's' if diffs.size > 1
files
end
......
......@@ -54,7 +54,7 @@ module API
sha = params[:sha]
commit = user_project.commit(sha)
not_found! "Commit" unless commit
commit.diffs.to_a
commit.raw_diffs.to_a
end
# Get a commit's comments
......@@ -96,7 +96,7 @@ module API
}
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]
lines = Gitlab::Diff::Parser.new.parse(diff.diff.each_line)
......
......@@ -224,7 +224,7 @@ module API
class MergeRequestChanges < MergeRequest
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
......
module Gitlab
module Diff
module FileCollection
def self.default_options
::Commit.max_diff_options.merge(ignore_whitespace_change: false, no_collapse: false)
end
end
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 initialize(diffs, project:, diff_options: nil, diff_refs: nil)
@diffs = diffs
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
@diffs.decorate! { |diff| decorate_diff!(diff) }
@diff_files ||= @diffs.decorate! { |diff| decorate_diff!(diff) }
end
private
def decorate_diff!(diff)
return diff if diff.is_a?(Gitlab::Diff::File)
Gitlab::Diff::File.new(diff, diff_refs: @diff_refs, repository: @project.repository)
Gitlab::Diff::File.new(diff, repository: project.repository, diff_refs: diff_refs)
end
end
end
......
......@@ -3,10 +3,7 @@ module Gitlab
module FileCollection
class Commit < Base
def initialize(commit, diff_options:)
# Not merge just set defaults
diff_options = diff_options || Gitlab::Diff::FileCollection.default_options
super(commit.diffs(diff_options),
super(commit,
project: commit.project,
diff_options: diff_options,
diff_refs: commit.diff_refs)
......
......@@ -3,10 +3,7 @@ module Gitlab
module FileCollection
class Compare < Base
def initialize(compare, project:, diff_options:, diff_refs: nil)
# Not merge just set defaults
diff_options = diff_options || Gitlab::Diff::FileCollection.default_options
super(compare.diffs(diff_options),
super(compare,
project: project,
diff_options: diff_options,
diff_refs: diff_refs)
......
......@@ -4,10 +4,8 @@ module Gitlab
class MergeRequest < Base
def initialize(merge_request, diff_options:)
@merge_request = merge_request
# Not merge just set defaults
diff_options = diff_options || Gitlab::Diff::FileCollection.default_options
super(merge_request.diffs(diff_options),
super(merge_request,
project: merge_request.project,
diff_options: diff_options,
diff_refs: merge_request.diff_refs)
......@@ -19,18 +17,11 @@ module Gitlab
private
# Extracted method to highlight in the same iteration to the diff_collection. Iteration in the DiffCollections
# seems particularly slow on big diffs (event when already populated).
# Extracted method to highlight in the same iteration to the diff_collection.
def decorate_diff!(diff)
highlight! super
end
def highlight!(diff_file)
if cacheable?
cache_highlight!(diff_file)
else
diff_file # Don't need to eager load highlighted diff lines
end
diff_file = super
cache_highlight!(diff_file) if cacheable?
diff_file
end
def highlight_diff_file_from_cache!(diff_file, cache_diff_lines)
......@@ -55,15 +46,13 @@ module Gitlab
else
highlight_cache[file_path] = diff_file.highlighted_diff_lines.map(&:to_hash)
end
diff_file
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_was_empty = @highlight_cache.empty?
@highlight_cache
end
......
......@@ -41,7 +41,8 @@ module Gitlab
def diffs
return unless compare
@diffs ||= compare.diff_file_collection(diff_options: { max_files: 30 }).diff_files
# This diff is more moderated in number of files and lines
@diffs ||= compare.diffs(diff_options: { max_files: 30, max_lines: 5000, no_collapse: true }).diff_files
end
def diffs_count
......
......@@ -6,7 +6,7 @@ describe DiffHelper do
let(:project) { create(:project) }
let(:repository) { project.repository }
let(:commit) { project.commit(sample_commit.id) }
let(:diffs) { commit.diffs }
let(:diffs) { commit.raw_diffs }
let(:diff) { diffs.first }
let(:diff_refs) { [commit.parent, commit] }
let(:diff_file) { Gitlab::Diff::File.new(diff, diff_refs: diff_refs, repository: repository) }
......@@ -32,16 +32,6 @@ describe DiffHelper do
end
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
expect(diff_options).to include(no_collapse: false)
end
......@@ -55,6 +45,18 @@ describe DiffHelper do
allow(controller).to receive(:action_name) { 'diff_for_path' }
expect(diff_options).to include(no_collapse: true)
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
describe 'unfold_bottom_class' do
......
......@@ -5,7 +5,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) { commit.raw_diffs.first }
let(:diff_file) { Gitlab::Diff::File.new(diff, diff_refs: commit.diff_refs, repository: project.repository) }
describe '#diff_lines' do
......
......@@ -5,7 +5,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) { commit.raw_diffs.first }
let(:diff_file) { Gitlab::Diff::File.new(diff, diff_refs: commit.diff_refs, repository: project.repository) }
describe '#highlight' do
......
......@@ -6,7 +6,7 @@ describe Gitlab::Diff::LineMapper, lib: true do
let(:project) { create(:project) }
let(:repository) { project.repository }
let(:commit) { project.commit(sample_commit.id) }
let(:diffs) { commit.diffs }
let(:diffs) { commit.raw_diffs }
let(:diff) { diffs.first }
let(:diff_file) { Gitlab::Diff::File.new(diff, diff_refs: commit.diff_refs, repository: repository) }
subject { described_class.new(diff_file) }
......
......@@ -6,7 +6,7 @@ describe Gitlab::Diff::ParallelDiff, lib: true do
let(:project) { create(:project) }
let(:repository) { project.repository }
let(:commit) { project.commit(sample_commit.id) }
let(:diffs) { commit.diffs }
let(:diffs) { commit.raw_diffs }
let(:diff) { diffs.first }
let(:diff_file) { Gitlab::Diff::File.new(diff, diff_refs: commit.diff_refs, repository: repository) }
subject { described_class.new(diff_file) }
......
......@@ -5,7 +5,7 @@ describe Gitlab::Diff::Parser, lib: true do
let(:project) { create(:project) }
let(:commit) { project.commit(sample_commit.id) }
let(:diff) { commit.diffs.first }
let(:diff) { commit.raw_diffs.first }
let(:parser) { Gitlab::Diff::Parser.new }
describe '#parse' do
......
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
# Generate a real line_code value so we know it will match. We use a
# 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
code = Gitlab::Diff::LineCode.generate(diff.new_path, line.new_pos, line.old_pos)
......
......@@ -128,7 +128,7 @@ describe MergeRequest, models: true do
end
end
describe '#diffs' do
describe '#raw_diffs' do
let(:merge_request) { build(:merge_request) }
let(:options) { { paths: ['a/b', 'b/a', 'c/*'] } }
......@@ -138,6 +138,31 @@ describe MergeRequest, models: true do
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)