BigW Consortium Gitlab

Commit f3cf8cc8 by Sean McGivern

Only search for MR revert commits on notes after MR was merged

If we search for notes before the MR was merged, we have to load every commit that was ever part of the MR, or mentioned in a push. In extreme cases, this can be tens of thousands of commits to load, but we know they can't revert the merge commit, because they are from before the MR was merged. In the (rare) case that we don't have a `merged_at` value for the MR, we can still search all notes.
parent 678a00d6
......@@ -342,10 +342,11 @@ class Commit
@merged_merge_request_hash[current_user]
end
def has_been_reverted?(current_user, noteable = self)
def has_been_reverted?(current_user, notes_association = nil)
ext = all_references(current_user)
notes_association ||= notes_with_associations
noteable.notes_with_associations.system.each do |note|
notes_association.system.each do |note|
note.all_references(current_user, extractor: ext)
end
......
......@@ -982,7 +982,16 @@ class MergeRequest < ActiveRecord::Base
end
def can_be_reverted?(current_user)
merge_commit && !merge_commit.has_been_reverted?(current_user, self)
return false unless merge_commit
merged_at = metrics&.merged_at
notes_association = notes_with_associations
if merged_at
notes_association = notes_association.where('created_at > ?', merged_at)
end
!merge_commit.has_been_reverted?(current_user, notes_association)
end
def can_be_cherry_picked?
......
---
title: Speed up loading merged merge requests when they contained a lot of commits
before merging
merge_request: 16320
author:
type: performance
......@@ -151,11 +151,11 @@ describe CommitRange do
.with(commit1, user)
.and_return(true)
expect(commit1.has_been_reverted?(user, issue)).to eq(true)
expect(commit1.has_been_reverted?(user, issue.notes_with_associations)).to eq(true)
end
it 'returns false a commit has not been reverted' do
expect(commit1.has_been_reverted?(user, issue)).to eq(false)
it 'returns false if the commit has not been reverted' do
expect(commit1.has_been_reverted?(user, issue.notes_with_associations)).to eq(false)
end
end
end
......@@ -1035,6 +1035,83 @@ describe MergeRequest do
end
end
describe '#can_be_reverted?' do
context 'when there is no merged_at for the MR' do
before do
subject.metrics.update!(merged_at: nil)
end
it 'returns false' do
expect(subject.can_be_reverted?(nil)).to be_falsey
end
end
context 'when there is no merge_commit for the MR' do
before do
subject.metrics.update!(merged_at: Time.now.utc)
end
it 'returns false' do
expect(subject.can_be_reverted?(nil)).to be_falsey
end
end
context 'when the MR has been merged' do
before do
MergeRequests::MergeService
.new(subject.target_project, subject.author)
.execute(subject)
end
context 'when there is no revert commit' do
it 'returns true' do
expect(subject.can_be_reverted?(nil)).to be_truthy
end
end
context 'when there is a revert commit' do
let(:current_user) { subject.author }
let(:branch) { subject.target_branch }
let(:project) { subject.target_project }
let(:revert_commit_id) do
params = {
commit: subject.merge_commit,
branch_name: branch,
start_branch: branch
}
Commits::RevertService.new(project, current_user, params).execute[:result]
end
before do
project.add_master(current_user)
ProcessCommitWorker.new.perform(project.id,
current_user.id,
project.commit(revert_commit_id).to_hash,
project.default_branch == branch)
end
context 'when the revert commit is mentioned in a note after the MR was merged' do
it 'returns false' do
expect(subject.can_be_reverted?(current_user)).to be_falsey
end
end
context 'when the revert commit is mentioned in a note before the MR was merged' do
before do
subject.notes.last.update!(created_at: subject.metrics.merged_at - 1.second)
end
it 'returns true' do
expect(subject.can_be_reverted?(current_user)).to be_truthy
end
end
end
end
end
describe '#participants' do
let(:project) { create(:project, :public) }
......
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