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

6 files changed, 73 insertions, 5 deletions

diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb
index 7eef08aa6a3..735d9fba966 100644
--- a/app/models/merge_request.rb
+++ b/app/models/merge_request.rb
@@ -353,6 +353,15 @@ class MergeRequest < ActiveRecord::Base
     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
   # updates `merge_jid` with the MergeWorker#jid.
   # This helps tracking enqueued and ongoing merge jobs.
diff --git a/app/models/merge_request_diff.rb b/app/models/merge_request_diff.rb
index 02c6b650f33..bb6ff8921df 100644
--- a/app/models/merge_request_diff.rb
+++ b/app/models/merge_request_diff.rb
@@ -140,6 +140,12 @@ class MergeRequestDiff < ActiveRecord::Base
     merge_request_diff_commits.map(&:sha)
   end
 
+  def commits_by_shas(shas)
+    return [] unless shas.present?
+
+    merge_request_diff_commits.where(sha: shas)
+  end
+
   def diff_refs=(new_diff_refs)
     self.base_commit_sha = new_diff_refs&.base_sha
     self.start_commit_sha = new_diff_refs&.start_sha
diff --git a/app/services/merge_requests/refresh_service.rb b/app/services/merge_requests/refresh_service.rb
index f01872b205e..53768ff2cbe 100644
--- a/app/services/merge_requests/refresh_service.rb
+++ b/app/services/merge_requests/refresh_service.rb
@@ -87,11 +87,8 @@ module MergeRequests
       filter_merge_requests(merge_requests).each do |merge_request|
         if branch_and_project_match?(merge_request) || @push.force_push?
           merge_request.reload_diff(current_user)
-        else
-          mr_commit_ids = merge_request.commit_shas
-          push_commit_ids = @commits.map(&:id)
-          matches = mr_commit_ids & push_commit_ids
-          merge_request.reload_diff(current_user) if matches.any?
+        elsif merge_request.includes_any_commits?(push_commit_ids)
+          merge_request.reload_diff(current_user)
         end
 
         merge_request.mark_as_unchecked
@@ -104,6 +101,10 @@ module MergeRequests
     end
     # rubocop: enable CodeReuse/ActiveRecord
 
+    def push_commit_ids
+      @push_commit_ids ||= @commits.map(&:id)
+    end
+
     def branch_and_project_match?(merge_request)
       merge_request.source_project == @project &&
         merge_request.source_branch == @push.branch_name
diff --git a/changelogs/unreleased/sh-optimize-mr-commit-sha-lookup.yml b/changelogs/unreleased/sh-optimize-mr-commit-sha-lookup.yml
new file mode 100644
index 00000000000..bea73f8d329
--- /dev/null
+++ b/changelogs/unreleased/sh-optimize-mr-commit-sha-lookup.yml
@@ -0,0 +1,5 @@
+---
+title: Optimize merge request refresh by using the database to check commit SHAs
+merge_request: 22731
+author:
+type: performance
diff --git a/spec/models/merge_request_diff_spec.rb b/spec/models/merge_request_diff_spec.rb
index 562ccaf6c0b..47e8f04e728 100644
--- a/spec/models/merge_request_diff_spec.rb
+++ b/spec/models/merge_request_diff_spec.rb
@@ -211,4 +211,25 @@ describe MergeRequestDiff do
       expect(diff_with_commits.commits_count).to eq(29)
     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
diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb
index 7d500f9e579..2eb5e39ccfd 100644
--- a/spec/models/merge_request_spec.rb
+++ b/spec/models/merge_request_spec.rb
@@ -2611,6 +2611,32 @@ describe MergeRequest do
     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
     let(:target_project) { create(:project, :public) }
     let(:source_project) { fork_project(target_project) }