Commit 345aa034 authored by Igor Drozdov's avatar Igor Drozdov

Use diff-stats for calculating raw diffs modified paths

That is more performance than fetching diff files for this
parent da6b9e51
...@@ -555,22 +555,28 @@ class MergeRequest < ApplicationRecord ...@@ -555,22 +555,28 @@ class MergeRequest < ApplicationRecord
end end
end end
def diff_stats
return unless diff_refs
strong_memoize(:diff_stats) do
project.repository.diff_stats(diff_refs.base_sha, diff_refs.head_sha)
end
end
def diff_size def diff_size
# Calling `merge_request_diff.diffs.real_size` will also perform # Calling `merge_request_diff.diffs.real_size` will also perform
# highlighting, which we don't need here. # highlighting, which we don't need here.
merge_request_diff&.real_size || diffs.real_size merge_request_diff&.real_size || diff_stats&.real_size || diffs.real_size
end end
def modified_paths(past_merge_request_diff: nil) def modified_paths(past_merge_request_diff: nil)
diffs = if past_merge_request_diff if past_merge_request_diff
past_merge_request_diff past_merge_request_diff.modified_paths
elsif compare elsif compare
compare diff_stats&.paths || compare.modified_paths
else else
self.merge_request_diff merge_request_diff.modified_paths
end end
diffs.modified_paths
end end
def new_paths def new_paths
......
---
title: Use diff-stats for calculating raw diffs modified paths
merge_request: 29134
author:
type: performance
...@@ -22,6 +22,15 @@ module Gitlab ...@@ -22,6 +22,15 @@ module Gitlab
@collection.map(&:path) @collection.map(&:path)
end end
def real_size
max_files = ::Commit.max_diff_options[:max_files]
if paths.size > max_files
"#{max_files}+"
else
paths.size.to_s
end
end
private private
def indexed_by_path def indexed_by_path
......
...@@ -29,4 +29,16 @@ describe Gitlab::Git::DiffStatsCollection do ...@@ -29,4 +29,16 @@ describe Gitlab::Git::DiffStatsCollection do
expect(collection.paths).to eq %w[foo bar] expect(collection.paths).to eq %w[foo bar]
end end
end end
describe '#real_size' do
it 'returns the number of modified files' do
expect(collection.real_size).to eq('2')
end
it 'returns capped number when it is bigger than max_files' do
allow(::Commit).to receive(:max_diff_options).and_return(max_files: 1)
expect(collection.real_size).to eq('1+')
end
end
end end
...@@ -874,7 +874,7 @@ describe MergeRequest do ...@@ -874,7 +874,7 @@ describe MergeRequest do
subject(:merge_request) { build(:merge_request) } subject(:merge_request) { build(:merge_request) }
before do before do
expect(diff).to receive(:modified_paths).and_return(paths) allow(diff).to receive(:modified_paths).and_return(paths)
end end
context 'when past_merge_request_diff is specified' do context 'when past_merge_request_diff is specified' do
...@@ -890,10 +890,29 @@ describe MergeRequest do ...@@ -890,10 +890,29 @@ describe MergeRequest do
let(:compare) { double(:compare) } let(:compare) { double(:compare) }
let(:diff) { compare } let(:diff) { compare }
it 'returns affected file paths from compare' do before do
merge_request.compare = compare merge_request.compare = compare
expect(merge_request.modified_paths).to eq(paths) expect(merge_request).to receive(:diff_stats).and_return(diff_stats)
end
context 'and diff_stats are not present' do
let(:diff_stats) { nil }
it 'returns affected file paths from compare' do
expect(merge_request.modified_paths).to eq(paths)
end
end
context 'and diff_stats are present' do
let(:diff_stats) { double(:diff_stats) }
it 'returns affected file paths from compare' do
diff_stats_path = double(:diff_stats_paths)
expect(diff_stats).to receive(:paths).and_return(diff_stats_path)
expect(merge_request.modified_paths).to eq(diff_stats_path)
end
end 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