Commit 0509c3cf authored by Stan Hu's avatar Stan Hu

Fix extra merge request versions created from forked merge requests

When a forked merge request was created with the same branch name as the
target name, MergeRequests::RefreshService would always create a new
diff even though nothing had changed. For example, on GitLab.com:

1. There were a number of merge requests in the gitlab-ce and www-gitlab-com
projects that had old merge requests from the community.
2. These merge requests originated from forked projects and used the
source branch master.
3. When someone pushed to master in the main repository, MergeRequests::RefreshService
would see that master matched the merge requests in question and generated a new
diff.
4. This led to an explosion of merge request diffs and slowed down the "Changes"
tab considerably.

This change alters MergeRequests::RefreshService so that it will only
refresh the diff if the merge request's source project and branch
match. Otherwise, the refresh will only happen if a pushed commit
contains a commit relevant to the existing merge request.

Closes https://gitlab.com/gitlab-org/gitlab-ce/issues/53153
parent 44a6b8d4
...@@ -85,7 +85,7 @@ module MergeRequests ...@@ -85,7 +85,7 @@ module MergeRequests
.where.not(target_project: @project).to_a .where.not(target_project: @project).to_a
filter_merge_requests(merge_requests).each do |merge_request| filter_merge_requests(merge_requests).each do |merge_request|
if merge_request.source_branch == @push.branch_name || @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 else
mr_commit_ids = merge_request.commit_shas mr_commit_ids = merge_request.commit_shas
...@@ -104,6 +104,11 @@ module MergeRequests ...@@ -104,6 +104,11 @@ module MergeRequests
end end
# rubocop: enable CodeReuse/ActiveRecord # rubocop: enable CodeReuse/ActiveRecord
def branch_and_project_match?(merge_request)
merge_request.source_project == @project &&
merge_request.source_branch == @push.branch_name
end
def reset_merge_when_pipeline_succeeds def reset_merge_when_pipeline_succeeds
merge_requests_for_source_branch.each(&:reset_merge_when_pipeline_succeeds) merge_requests_for_source_branch.each(&:reset_merge_when_pipeline_succeeds)
end end
......
---
title: Fix extra merge request versions created from forked merge requests
merge_request: 22611
author:
type: fixed
...@@ -306,6 +306,66 @@ describe MergeRequests::RefreshService do ...@@ -306,6 +306,66 @@ describe MergeRequests::RefreshService do
end end
end end
context 'forked projects with the same source branch name as target branch' do
let!(:first_commit) do
@fork_project.repository.create_file(@user, 'test1.txt', 'Test data',
message: 'Test commit',
branch_name: 'master')
end
let!(:second_commit) do
@fork_project.repository.create_file(@user, 'test2.txt', 'More test data',
message: 'Second test commit',
branch_name: 'master')
end
let!(:forked_master_mr) do
create(:merge_request,
source_project: @fork_project,
source_branch: 'master',
target_branch: 'master',
target_project: @project)
end
let(:force_push_commit) { @project.commit('feature').id }
it 'should reload a new diff for a push to the forked project' do
expect do
service.new(@fork_project, @user).execute(@oldrev, first_commit, 'refs/heads/master')
reload_mrs
end.to change { forked_master_mr.merge_request_diffs.count }.by(1)
end
it 'should reload a new diff for a force push to the source branch' do
expect do
service.new(@fork_project, @user).execute(@oldrev, force_push_commit, 'refs/heads/master')
reload_mrs
end.to change { forked_master_mr.merge_request_diffs.count }.by(1)
end
it 'should reload a new diff for a force push to the target branch' do
expect do
service.new(@project, @user).execute(@oldrev, force_push_commit, 'refs/heads/master')
reload_mrs
end.to change { forked_master_mr.merge_request_diffs.count }.by(1)
end
it 'should reload a new diff for a push to the target project that contains a commit in the MR' do
expect do
service.new(@project, @user).execute(@oldrev, first_commit, 'refs/heads/master')
reload_mrs
end.to change { forked_master_mr.merge_request_diffs.count }.by(1)
end
it 'should not increase the diff count for a new push to target branch' do
new_commit = @project.repository.create_file(@user, 'new-file.txt', 'A new file',
message: 'This is a test',
branch_name: 'master')
expect do
service.new(@project, @user).execute(@newrev, new_commit, 'refs/heads/master')
reload_mrs
end.not_to change { forked_master_mr.merge_request_diffs.count }
end
end
context 'push to origin repo target branch after fork project was removed' do context 'push to origin repo target branch after fork project was removed' do
before do before do
@fork_project.destroy @fork_project.destroy
......
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