Commit 4d3ff28a authored by Douwe Maan's avatar Douwe Maan

Merge branch 'sh-optimize-mr-commit-sha-lookup' into 'master'

Optimize merge request refresh by using the database to check commit SHAs

See merge request gitlab-org/gitlab-ce!22731
parents 3cdf7c7e dbc03ce3
...@@ -353,6 +353,15 @@ class MergeRequest < ActiveRecord::Base ...@@ -353,6 +353,15 @@ class MergeRequest < ActiveRecord::Base
end end
end end
# Returns true if there are commits that match at least one commit SHA.
def includes_any_commits?(shas)
if persisted?
merge_request_diff.commits_by_shas(shas).exists?
else
(commit_shas & shas).present?
end
end
# Calls `MergeWorker` to proceed with the merge process and # Calls `MergeWorker` to proceed with the merge process and
# updates `merge_jid` with the MergeWorker#jid. # updates `merge_jid` with the MergeWorker#jid.
# This helps tracking enqueued and ongoing merge jobs. # This helps tracking enqueued and ongoing merge jobs.
......
...@@ -140,6 +140,12 @@ class MergeRequestDiff < ActiveRecord::Base ...@@ -140,6 +140,12 @@ class MergeRequestDiff < ActiveRecord::Base
merge_request_diff_commits.map(&:sha) merge_request_diff_commits.map(&:sha)
end end
def commits_by_shas(shas)
return [] unless shas.present?
merge_request_diff_commits.where(sha: shas)
end
def diff_refs=(new_diff_refs) def diff_refs=(new_diff_refs)
self.base_commit_sha = new_diff_refs&.base_sha self.base_commit_sha = new_diff_refs&.base_sha
self.start_commit_sha = new_diff_refs&.start_sha self.start_commit_sha = new_diff_refs&.start_sha
......
...@@ -87,11 +87,8 @@ module MergeRequests ...@@ -87,11 +87,8 @@ module MergeRequests
filter_merge_requests(merge_requests).each do |merge_request| filter_merge_requests(merge_requests).each do |merge_request|
if branch_and_project_match?(merge_request) || @push.force_push? if branch_and_project_match?(merge_request) || @push.force_push?
merge_request.reload_diff(current_user) merge_request.reload_diff(current_user)
else elsif merge_request.includes_any_commits?(push_commit_ids)
mr_commit_ids = merge_request.commit_shas merge_request.reload_diff(current_user)
push_commit_ids = @commits.map(&:id)
matches = mr_commit_ids & push_commit_ids
merge_request.reload_diff(current_user) if matches.any?
end end
merge_request.mark_as_unchecked merge_request.mark_as_unchecked
...@@ -104,6 +101,10 @@ module MergeRequests ...@@ -104,6 +101,10 @@ module MergeRequests
end end
# rubocop: enable CodeReuse/ActiveRecord # rubocop: enable CodeReuse/ActiveRecord
def push_commit_ids
@push_commit_ids ||= @commits.map(&:id)
end
def branch_and_project_match?(merge_request) def branch_and_project_match?(merge_request)
merge_request.source_project == @project && merge_request.source_project == @project &&
merge_request.source_branch == @push.branch_name merge_request.source_branch == @push.branch_name
......
---
title: Optimize merge request refresh by using the database to check commit SHAs
merge_request: 22731
author:
type: performance
...@@ -211,4 +211,25 @@ describe MergeRequestDiff do ...@@ -211,4 +211,25 @@ describe MergeRequestDiff do
expect(diff_with_commits.commits_count).to eq(29) expect(diff_with_commits.commits_count).to eq(29)
end end
end end
describe '#commits_by_shas' do
let(:commit_shas) { diff_with_commits.commit_shas }
it 'returns empty if no SHAs were provided' do
expect(diff_with_commits.commits_by_shas([])).to be_empty
end
it 'returns one SHA' do
commits = diff_with_commits.commits_by_shas([commit_shas.first, Gitlab::Git::BLANK_SHA])
expect(commits.count).to eq(1)
end
it 'returns all matching SHAs' do
commits = diff_with_commits.commits_by_shas(commit_shas)
expect(commits.count).to eq(commit_shas.count)
expect(commits.map(&:sha)).to match_array(commit_shas)
end
end
end end
...@@ -2611,6 +2611,32 @@ describe MergeRequest do ...@@ -2611,6 +2611,32 @@ describe MergeRequest do
end end
end end
describe '#includes_any_commits?' do
it 'returns false' do
expect(subject.includes_any_commits?([Gitlab::Git::BLANK_SHA])).to be_falsey
end
it 'returns true' do
expect(subject.includes_any_commits?([subject.merge_request_diff.head_commit_sha])).to be_truthy
end
it 'returns true even when there is a non-existent comit' do
expect(subject.includes_any_commits?([Gitlab::Git::BLANK_SHA, subject.merge_request_diff.head_commit_sha])).to be_truthy
end
context 'unpersisted merge request' do
let(:new_mr) { build(:merge_request) }
it 'returns false' do
expect(new_mr.includes_any_commits?([Gitlab::Git::BLANK_SHA])).to be_falsey
end
it 'returns true' do
expect(new_mr.includes_any_commits?([subject.merge_request_diff.head_commit_sha])).to be_truthy
end
end
end
describe '#can_allow_collaboration?' do describe '#can_allow_collaboration?' do
let(:target_project) { create(:project, :public) } let(:target_project) { create(:project, :public) }
let(:source_project) { fork_project(target_project) } let(:source_project) { fork_project(target_project) }
......
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