Commit 5dce096c authored by Oswaldo Ferreira's avatar Oswaldo Ferreira Committed by Nick Thomas

Use standalone diff stats RPC on every comparison view

parent 3172de0d
...@@ -131,7 +131,7 @@ class DiffNote < Note ...@@ -131,7 +131,7 @@ class DiffNote < Note
# As an extra benefit, the returned `diff_file` already # As an extra benefit, the returned `diff_file` already
# has `highlighted_diff_lines` data set from Redis on # has `highlighted_diff_lines` data set from Redis on
# `Diff::FileCollection::MergeRequestDiff`. # `Diff::FileCollection::MergeRequestDiff`.
noteable.diffs(paths: original_position.paths, expanded: true).diff_files.first noteable.diffs(original_position.diff_options).diff_files.first
else else
original_position.diff_file(self.project.repository) original_position.diff_file(self.project.repository)
end end
......
...@@ -31,7 +31,7 @@ module MergeRequests ...@@ -31,7 +31,7 @@ module MergeRequests
def clear_cache(new_diff) def clear_cache(new_diff)
# Executing the iteration we cache highlighted diffs for each diff file of # Executing the iteration we cache highlighted diffs for each diff file of
# MergeRequestDiff. # MergeRequestDiff.
new_diff.diffs_collection.write_cache cacheable_collection(new_diff).write_cache
# Remove cache for all diffs on this MR. Do not use the association on the # Remove cache for all diffs on this MR. Do not use the association on the
# model, as that will interfere with other actions happening when # model, as that will interfere with other actions happening when
...@@ -39,9 +39,15 @@ module MergeRequests ...@@ -39,9 +39,15 @@ module MergeRequests
MergeRequestDiff.where(merge_request: merge_request).each do |merge_request_diff| MergeRequestDiff.where(merge_request: merge_request).each do |merge_request_diff|
next if merge_request_diff == new_diff next if merge_request_diff == new_diff
merge_request_diff.diffs_collection.clear_cache cacheable_collection(merge_request_diff).clear_cache
end end
end end
# rubocop: enable CodeReuse/ActiveRecord # rubocop: enable CodeReuse/ActiveRecord
def cacheable_collection(diff)
# There are scenarios where we don't need to request Diff Stats.
# Mainly when clearing / writing diff caches.
diff.diffs(include_stats: false)
end
end end
end end
...@@ -10,7 +10,7 @@ class NewMergeRequestWorker ...@@ -10,7 +10,7 @@ class NewMergeRequestWorker
EventCreateService.new.open_mr(issuable, user) EventCreateService.new.open_mr(issuable, user)
NotificationService.new.new_merge_request(issuable, user) NotificationService.new.new_merge_request(issuable, user)
issuable.diffs.write_cache issuable.diffs(include_stats: false).write_cache
issuable.create_cross_references!(user) issuable.create_cross_references!(user)
end end
......
---
title: Use stats RPC when comparing diffs
merge_request: 21778
author:
type: fixed
...@@ -20,8 +20,9 @@ module Gitlab ...@@ -20,8 +20,9 @@ module Gitlab
DiffViewer::Image DiffViewer::Image
].sort_by { |v| v.binary? ? 0 : 1 }.freeze ].sort_by { |v| v.binary? ? 0 : 1 }.freeze
def initialize(diff, repository:, diff_refs: nil, fallback_diff_refs: nil) def initialize(diff, repository:, diff_refs: nil, fallback_diff_refs: nil, stats: nil)
@diff = diff @diff = diff
@stats = stats
@repository = repository @repository = repository
@diff_refs = diff_refs @diff_refs = diff_refs
@fallback_diff_refs = fallback_diff_refs @fallback_diff_refs = fallback_diff_refs
...@@ -165,11 +166,11 @@ module Gitlab ...@@ -165,11 +166,11 @@ module Gitlab
end end
def added_lines def added_lines
diff_lines.count(&:added?) @stats&.additions || diff_lines.count(&:added?)
end end
def removed_lines def removed_lines
diff_lines.count(&:removed?) @stats&.deletions || diff_lines.count(&:removed?)
end end
def file_identifier def file_identifier
......
...@@ -2,23 +2,27 @@ module Gitlab ...@@ -2,23 +2,27 @@ module Gitlab
module Diff module Diff
module FileCollection module FileCollection
class Base class Base
include Gitlab::Utils::StrongMemoize
attr_reader :project, :diff_options, :diff_refs, :fallback_diff_refs, :diffable attr_reader :project, :diff_options, :diff_refs, :fallback_diff_refs, :diffable
delegate :count, :size, :real_size, to: :diff_files delegate :count, :size, :real_size, to: :diff_files
def self.default_options def self.default_options
::Commit.max_diff_options.merge(ignore_whitespace_change: false, expanded: false) ::Commit.max_diff_options.merge(ignore_whitespace_change: false, expanded: false, include_stats: true)
end end
def initialize(diffable, project:, diff_options: nil, diff_refs: nil, fallback_diff_refs: nil) def initialize(diffable, project:, diff_options: nil, diff_refs: nil, fallback_diff_refs: nil)
diff_options = self.class.default_options.merge(diff_options || {}) diff_options = self.class.default_options.merge(diff_options || {})
@diffable = diffable @diffable = diffable
@include_stats = diff_options.delete(:include_stats)
@diffs = diffable.raw_diffs(diff_options) @diffs = diffable.raw_diffs(diff_options)
@project = project @project = project
@diff_options = diff_options @diff_options = diff_options
@diff_refs = diff_refs @diff_refs = diff_refs
@fallback_diff_refs = fallback_diff_refs @fallback_diff_refs = fallback_diff_refs
@repository = project.repository
end end
def diff_files def diff_files
...@@ -43,10 +47,27 @@ module Gitlab ...@@ -43,10 +47,27 @@ module Gitlab
private private
def diff_stats_collection
strong_memoize(:diff_stats) do
# There are scenarios where we don't need to request Diff Stats,
# when caching for instance.
next unless @include_stats
next unless diff_refs
@repository.diff_stats(diff_refs.base_sha, diff_refs.head_sha)
end
end
def decorate_diff!(diff) def decorate_diff!(diff)
return diff if diff.is_a?(File) return diff if diff.is_a?(File)
Gitlab::Diff::File.new(diff, repository: project.repository, diff_refs: diff_refs, fallback_diff_refs: fallback_diff_refs) stats = diff_stats_collection&.find_by_path(diff.new_path)
Gitlab::Diff::File.new(diff,
repository: project.repository,
diff_refs: diff_refs,
fallback_diff_refs: fallback_diff_refs,
stats: stats)
end end
end end
end end
......
...@@ -116,6 +116,10 @@ module Gitlab ...@@ -116,6 +116,10 @@ module Gitlab
end end
end end
def diff_options
{ paths: paths, expanded: true, include_stats: false }
end
def diff_line(repository) def diff_line(repository)
@diff_line ||= diff_file(repository)&.line_for_position(self) @diff_line ||= diff_file(repository)&.line_for_position(self)
end end
...@@ -130,7 +134,7 @@ module Gitlab ...@@ -130,7 +134,7 @@ module Gitlab
return unless diff_refs.complete? return unless diff_refs.complete?
return unless comparison = diff_refs.compare_in(repository.project) return unless comparison = diff_refs.compare_in(repository.project)
comparison.diffs(paths: paths, expanded: true).diff_files.first comparison.diffs(diff_options).diff_files.first
end end
def get_formatter_class(type) def get_formatter_class(type)
......
...@@ -3,6 +3,7 @@ ...@@ -3,6 +3,7 @@
module Gitlab module Gitlab
module Git module Git
class DiffStatsCollection class DiffStatsCollection
include Gitlab::Utils::StrongMemoize
include Enumerable include Enumerable
def initialize(diff_stats) def initialize(diff_stats)
...@@ -12,6 +13,18 @@ module Gitlab ...@@ -12,6 +13,18 @@ module Gitlab
def each(&block) def each(&block)
@collection.each(&block) @collection.each(&block)
end end
def find_by_path(path)
indexed_by_path[path]
end
private
def indexed_by_path
strong_memoize(:indexed_by_path) do
index_by { |stats| stats.path }
end
end
end end
end end
end end
...@@ -444,7 +444,7 @@ module Gitlab ...@@ -444,7 +444,7 @@ module Gitlab
end end
Gitlab::Git::DiffStatsCollection.new(stats) Gitlab::Git::DiffStatsCollection.new(stats)
rescue CommandError rescue CommandError, TypeError
Gitlab::Git::DiffStatsCollection.new([]) Gitlab::Git::DiffStatsCollection.new([])
end end
......
# frozen_string_literal: true
require 'spec_helper'
describe Gitlab::Diff::FileCollection::Commit do
let(:project) { create(:project, :repository) }
it_behaves_like 'diff statistics' do
let(:collection_default_args) do
{ diff_options: {} }
end
let(:diffable) { project.commit }
let(:stub_path) { 'bar/branch-test.txt' }
end
end
# frozen_string_literal: true
require 'spec_helper'
describe Gitlab::Diff::FileCollection::Compare do
include RepoHelpers
let(:project) { create(:project, :repository) }
let(:commit) { project.commit }
let(:start_commit) { sample_image_commit }
let(:head_commit) { sample_commit }
let(:raw_compare) do
Gitlab::Git::Compare.new(project.repository.raw_repository,
start_commit.id,
head_commit.id)
end
it_behaves_like 'diff statistics' do
let(:collection_default_args) do
{
project: diffable.project,
diff_options: {},
diff_refs: diffable.diff_refs
}
end
let(:diffable) { Compare.new(raw_compare, project) }
let(:stub_path) { '.gitignore' }
end
end
...@@ -29,6 +29,14 @@ describe Gitlab::Diff::FileCollection::MergeRequestDiff do ...@@ -29,6 +29,14 @@ describe Gitlab::Diff::FileCollection::MergeRequestDiff do
expect(mr_diff.cache_key).not_to eq(key) expect(mr_diff.cache_key).not_to eq(key)
end end
it_behaves_like 'diff statistics' do
let(:collection_default_args) do
{ diff_options: {} }
end
let(:diffable) { merge_request.merge_request_diff }
let(:stub_path) { '.gitignore' }
end
shared_examples 'initializes a DiffCollection' do shared_examples 'initializes a DiffCollection' do
it 'returns a valid instance of a DiffCollection' do it 'returns a valid instance of a DiffCollection' do
expect(diff_files).to be_a(Gitlab::Git::DiffCollection) expect(diff_files).to be_a(Gitlab::Git::DiffCollection)
......
...@@ -186,6 +186,70 @@ describe Gitlab::Diff::File do ...@@ -186,6 +186,70 @@ describe Gitlab::Diff::File do
end end
end end
context 'diff file stats' do
let(:diff_file) do
described_class.new(diff,
diff_refs: commit.diff_refs,
repository: project.repository,
stats: stats)
end
let(:raw_diff) do
<<~EOS
--- a/files/ruby/popen.rb
+++ b/files/ruby/popen.rb
@@ -6,12 +6,18 @@ module Popen
def popen(cmd, path=nil)
unless cmd.is_a?(Array)
- raise "System commands must be given as an array of strings"
+ raise RuntimeError, "System commands must be given as an array of strings"
+ # foobar
end
EOS
end
describe '#added_lines' do
context 'when stats argument given' do
let(:stats) { double(Gitaly::DiffStats, additions: 10, deletions: 15) }
it 'returns added lines from stats' do
expect(diff_file.added_lines).to eq(stats.additions)
end
end
context 'when stats argument not given' do
let(:stats) { nil }
it 'returns added lines by parsing raw diff' do
allow(diff_file).to receive(:raw_diff) { raw_diff }
expect(diff_file.added_lines).to eq(2)
end
end
end
describe '#removed_lines' do
context 'when stats argument given' do
let(:stats) { double(Gitaly::DiffStats, additions: 10, deletions: 15) }
it 'returns removed lines from stats' do
expect(diff_file.removed_lines).to eq(stats.deletions)
end
end
context 'when stats argument not given' do
let(:stats) { nil }
it 'returns removed lines by parsing raw diff' do
allow(diff_file).to receive(:raw_diff) { raw_diff }
expect(diff_file.removed_lines).to eq(1)
end
end
end
end
describe '#simple_viewer' do describe '#simple_viewer' do
context 'when the file is not diffable' do context 'when the file is not diffable' do
before do before do
......
# frozen_string_literal: true
require "spec_helper"
describe Gitlab::Git::DiffStatsCollection do
let(:stats_a) do
double(Gitaly::DiffStats, additions: 10, deletions: 15, path: 'foo')
end
let(:stats_b) do
double(Gitaly::DiffStats, additions: 5, deletions: 1, path: 'bar')
end
let(:diff_stats) { [stats_a, stats_b] }
let(:collection) { described_class.new(diff_stats) }
describe '.find_by_path' do
it 'returns stats by path when found' do
expect(collection.find_by_path('foo')).to eq(stats_a)
end
it 'returns nil when stats is not found by path' do
expect(collection.find_by_path('no-file')).to be_nil
end
end
end
...@@ -1136,6 +1136,14 @@ describe Gitlab::Git::Repository, :seed_helper do ...@@ -1136,6 +1136,14 @@ describe Gitlab::Git::Repository, :seed_helper do
expect(collection).to be_a(Enumerable) expect(collection).to be_a(Enumerable)
expect(collection.to_a).to be_empty expect(collection.to_a).to be_empty
end end
it 'returns no Gitaly::DiffStats when there is a nil SHA' do
collection = repository.diff_stats(nil, 'master')
expect(collection).to be_a(Gitlab::Git::DiffStatsCollection)
expect(collection).to be_a(Enumerable)
expect(collection.to_a).to be_empty
end
end end
describe "#ls_files" do describe "#ls_files" do
......
# frozen_string_literal: true
shared_examples 'diff statistics' do |test_include_stats_flag: true|
def stub_stats_find_by_path(path, stats_mock)
expect_next_instance_of(Gitlab::Git::DiffStatsCollection) do |collection|
allow(collection).to receive(:find_by_path).and_call_original
expect(collection).to receive(:find_by_path).with(path).and_return(stats_mock)
end
end
context 'when should request diff stats' do
it 'Repository#diff_stats is called' do
subject = described_class.new(diffable, collection_default_args)
expect(diffable.project.repository)
.to receive(:diff_stats)
.with(diffable.diff_refs.base_sha, diffable.diff_refs.head_sha)
.and_call_original
subject.diff_files
end
it 'Gitlab::Diff::File is initialized with diff stats' do
subject = described_class.new(diffable, collection_default_args)
stats_mock = double(Gitaly::DiffStats, path: '.gitignore', additions: 758, deletions: 120)
stub_stats_find_by_path(stub_path, stats_mock)
diff_file = subject.diff_files.find { |file| file.new_path == stub_path }
expect(diff_file.added_lines).to eq(stats_mock.additions)
expect(diff_file.removed_lines).to eq(stats_mock.deletions)
end
end
context 'when should not request diff stats' do
it 'Repository#diff_stats is not called' do
collection_default_args[:diff_options][:include_stats] = false
subject = described_class.new(diffable, collection_default_args)
expect(diffable.project.repository).not_to receive(:diff_stats)
subject.diff_files
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