diff options
author | Oswaldo Ferreira <oswaldo@gitlab.com> | 2018-09-19 15:26:28 +0300 |
---|---|---|
committer | Nick Thomas <nick@gitlab.com> | 2018-09-19 15:26:28 +0300 |
commit | 5dce096cf8b645d430bfdce8add8a577b595dc23 (patch) | |
tree | 69ac2bf5373bffd1e869d9b1c9f54e2c912cb767 | |
parent | 3172de0df3801125c23d1811a53d1467dec45645 (diff) |
Use standalone diff stats RPC on every comparison view
-rw-r--r-- | app/models/diff_note.rb | 2 | ||||
-rw-r--r-- | app/services/merge_requests/reload_diffs_service.rb | 10 | ||||
-rw-r--r-- | app/workers/new_merge_request_worker.rb | 2 | ||||
-rw-r--r-- | changelogs/unreleased/osw-use-diff-stats-rpc-on-comparison-views.yml | 5 | ||||
-rw-r--r-- | lib/gitlab/diff/file.rb | 7 | ||||
-rw-r--r-- | lib/gitlab/diff/file_collection/base.rb | 25 | ||||
-rw-r--r-- | lib/gitlab/diff/position.rb | 6 | ||||
-rw-r--r-- | lib/gitlab/git/diff_stats_collection.rb | 13 | ||||
-rw-r--r-- | lib/gitlab/git/repository.rb | 2 | ||||
-rw-r--r-- | spec/lib/gitlab/diff/file_collection/commit_spec.rb | 15 | ||||
-rw-r--r-- | spec/lib/gitlab/diff/file_collection/compare_spec.rb | 29 | ||||
-rw-r--r-- | spec/lib/gitlab/diff/file_collection/merge_request_diff_spec.rb | 8 | ||||
-rw-r--r-- | spec/lib/gitlab/diff/file_spec.rb | 64 | ||||
-rw-r--r-- | spec/lib/gitlab/git/diff_stats_collection_spec.rb | 26 | ||||
-rw-r--r-- | spec/lib/gitlab/git/repository_spec.rb | 8 | ||||
-rw-r--r-- | spec/support/shared_examples/diff_file_collections.rb | 47 |
16 files changed, 258 insertions, 11 deletions
diff --git a/app/models/diff_note.rb b/app/models/diff_note.rb index 716cf6574d3..047d353b4b5 100644 --- a/app/models/diff_note.rb +++ b/app/models/diff_note.rb @@ -131,7 +131,7 @@ class DiffNote < Note # As an extra benefit, the returned `diff_file` already # has `highlighted_diff_lines` data set from Redis on # `Diff::FileCollection::MergeRequestDiff`. - noteable.diffs(paths: original_position.paths, expanded: true).diff_files.first + noteable.diffs(original_position.diff_options).diff_files.first else original_position.diff_file(self.project.repository) end diff --git a/app/services/merge_requests/reload_diffs_service.rb b/app/services/merge_requests/reload_diffs_service.rb index c350b14d12b..b4d48fe92ad 100644 --- a/app/services/merge_requests/reload_diffs_service.rb +++ b/app/services/merge_requests/reload_diffs_service.rb @@ -31,7 +31,7 @@ module MergeRequests def clear_cache(new_diff) # Executing the iteration we cache highlighted diffs for each diff file of # 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 # model, as that will interfere with other actions happening when @@ -39,9 +39,15 @@ module MergeRequests MergeRequestDiff.where(merge_request: merge_request).each do |merge_request_diff| next if merge_request_diff == new_diff - merge_request_diff.diffs_collection.clear_cache + cacheable_collection(merge_request_diff).clear_cache end end # 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 diff --git a/app/workers/new_merge_request_worker.rb b/app/workers/new_merge_request_worker.rb index 62f9d9b6f57..fa48c1b29a8 100644 --- a/app/workers/new_merge_request_worker.rb +++ b/app/workers/new_merge_request_worker.rb @@ -10,7 +10,7 @@ class NewMergeRequestWorker EventCreateService.new.open_mr(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) end diff --git a/changelogs/unreleased/osw-use-diff-stats-rpc-on-comparison-views.yml b/changelogs/unreleased/osw-use-diff-stats-rpc-on-comparison-views.yml new file mode 100644 index 00000000000..c71d4e58d6f --- /dev/null +++ b/changelogs/unreleased/osw-use-diff-stats-rpc-on-comparison-views.yml @@ -0,0 +1,5 @@ +--- +title: Use stats RPC when comparing diffs +merge_request: 21778 +author: +type: fixed diff --git a/lib/gitlab/diff/file.rb b/lib/gitlab/diff/file.rb index d2ca7a070fa..fb117baca9e 100644 --- a/lib/gitlab/diff/file.rb +++ b/lib/gitlab/diff/file.rb @@ -20,8 +20,9 @@ module Gitlab DiffViewer::Image ].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 + @stats = stats @repository = repository @diff_refs = diff_refs @fallback_diff_refs = fallback_diff_refs @@ -165,11 +166,11 @@ module Gitlab end def added_lines - diff_lines.count(&:added?) + @stats&.additions || diff_lines.count(&:added?) end def removed_lines - diff_lines.count(&:removed?) + @stats&.deletions || diff_lines.count(&:removed?) end def file_identifier diff --git a/lib/gitlab/diff/file_collection/base.rb b/lib/gitlab/diff/file_collection/base.rb index 2acb0e43b69..b79ff771a2b 100644 --- a/lib/gitlab/diff/file_collection/base.rb +++ b/lib/gitlab/diff/file_collection/base.rb @@ -2,23 +2,27 @@ module Gitlab module Diff module FileCollection class Base + include Gitlab::Utils::StrongMemoize + attr_reader :project, :diff_options, :diff_refs, :fallback_diff_refs, :diffable delegate :count, :size, :real_size, to: :diff_files 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 def initialize(diffable, project:, diff_options: nil, diff_refs: nil, fallback_diff_refs: nil) diff_options = self.class.default_options.merge(diff_options || {}) @diffable = diffable + @include_stats = diff_options.delete(:include_stats) @diffs = diffable.raw_diffs(diff_options) @project = project @diff_options = diff_options @diff_refs = diff_refs @fallback_diff_refs = fallback_diff_refs + @repository = project.repository end def diff_files @@ -43,10 +47,27 @@ module Gitlab 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) 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 diff --git a/lib/gitlab/diff/position.rb b/lib/gitlab/diff/position.rb index 978962ab2eb..4b6016e00bc 100644 --- a/lib/gitlab/diff/position.rb +++ b/lib/gitlab/diff/position.rb @@ -116,6 +116,10 @@ module Gitlab end end + def diff_options + { paths: paths, expanded: true, include_stats: false } + end + def diff_line(repository) @diff_line ||= diff_file(repository)&.line_for_position(self) end @@ -130,7 +134,7 @@ module Gitlab return unless diff_refs.complete? 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 def get_formatter_class(type) diff --git a/lib/gitlab/git/diff_stats_collection.rb b/lib/gitlab/git/diff_stats_collection.rb index 84d9e46f98e..d4033f56387 100644 --- a/lib/gitlab/git/diff_stats_collection.rb +++ b/lib/gitlab/git/diff_stats_collection.rb @@ -3,6 +3,7 @@ module Gitlab module Git class DiffStatsCollection + include Gitlab::Utils::StrongMemoize include Enumerable def initialize(diff_stats) @@ -12,6 +13,18 @@ module Gitlab def each(&block) @collection.each(&block) 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 diff --git a/lib/gitlab/git/repository.rb b/lib/gitlab/git/repository.rb index fa22294ac51..3d5a63bdbac 100644 --- a/lib/gitlab/git/repository.rb +++ b/lib/gitlab/git/repository.rb @@ -444,7 +444,7 @@ module Gitlab end Gitlab::Git::DiffStatsCollection.new(stats) - rescue CommandError + rescue CommandError, TypeError Gitlab::Git::DiffStatsCollection.new([]) end diff --git a/spec/lib/gitlab/diff/file_collection/commit_spec.rb b/spec/lib/gitlab/diff/file_collection/commit_spec.rb new file mode 100644 index 00000000000..6d1b66deb6a --- /dev/null +++ b/spec/lib/gitlab/diff/file_collection/commit_spec.rb @@ -0,0 +1,15 @@ +# 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 diff --git a/spec/lib/gitlab/diff/file_collection/compare_spec.rb b/spec/lib/gitlab/diff/file_collection/compare_spec.rb new file mode 100644 index 00000000000..f330f299ac1 --- /dev/null +++ b/spec/lib/gitlab/diff/file_collection/compare_spec.rb @@ -0,0 +1,29 @@ +# 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 diff --git a/spec/lib/gitlab/diff/file_collection/merge_request_diff_spec.rb b/spec/lib/gitlab/diff/file_collection/merge_request_diff_spec.rb index 79287021981..4578da70bfc 100644 --- a/spec/lib/gitlab/diff/file_collection/merge_request_diff_spec.rb +++ b/spec/lib/gitlab/diff/file_collection/merge_request_diff_spec.rb @@ -29,6 +29,14 @@ describe Gitlab::Diff::FileCollection::MergeRequestDiff do expect(mr_diff.cache_key).not_to eq(key) 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 it 'returns a valid instance of a DiffCollection' do expect(diff_files).to be_a(Gitlab::Git::DiffCollection) diff --git a/spec/lib/gitlab/diff/file_spec.rb b/spec/lib/gitlab/diff/file_spec.rb index ebeb05d6e02..2f51642b58e 100644 --- a/spec/lib/gitlab/diff/file_spec.rb +++ b/spec/lib/gitlab/diff/file_spec.rb @@ -186,6 +186,70 @@ describe Gitlab::Diff::File do 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 context 'when the file is not diffable' do before do diff --git a/spec/lib/gitlab/git/diff_stats_collection_spec.rb b/spec/lib/gitlab/git/diff_stats_collection_spec.rb new file mode 100644 index 00000000000..89927cbb3a6 --- /dev/null +++ b/spec/lib/gitlab/git/diff_stats_collection_spec.rb @@ -0,0 +1,26 @@ +# 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 diff --git a/spec/lib/gitlab/git/repository_spec.rb b/spec/lib/gitlab/git/repository_spec.rb index 58c260ee1f0..d02536a2fb4 100644 --- a/spec/lib/gitlab/git/repository_spec.rb +++ b/spec/lib/gitlab/git/repository_spec.rb @@ -1136,6 +1136,14 @@ describe Gitlab::Git::Repository, :seed_helper do expect(collection).to be_a(Enumerable) expect(collection.to_a).to be_empty 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 describe "#ls_files" do diff --git a/spec/support/shared_examples/diff_file_collections.rb b/spec/support/shared_examples/diff_file_collections.rb new file mode 100644 index 00000000000..55ce160add0 --- /dev/null +++ b/spec/support/shared_examples/diff_file_collections.rb @@ -0,0 +1,47 @@ +# 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 |