diff --git a/app/services/merge_requests/refresh_service.rb b/app/services/merge_requests/refresh_service.rb index c5f959a1874..bc4a13cf4bc 100644 --- a/app/services/merge_requests/refresh_service.rb +++ b/app/services/merge_requests/refresh_service.rb @@ -35,11 +35,12 @@ module MergeRequests # target branch manually def close_merge_requests commit_ids = @commits.map(&:id) - merge_requests = @project.merge_requests.opened.where(target_branch: @branch_name).to_a + merge_requests = @project.merge_requests.preload(:merge_request_diff).opened.where(target_branch: @branch_name).to_a merge_requests = merge_requests.select(&:diff_head_commit) merge_requests = merge_requests.select do |merge_request| - commit_ids.include?(merge_request.diff_head_sha) + commit_ids.include?(merge_request.diff_head_sha) && + merge_request.merge_request_diff.state != 'empty' end filter_merge_requests(merge_requests).each do |merge_request| diff --git a/changelogs/unreleased/fix-mrs-merged-immediately.yml b/changelogs/unreleased/fix-mrs-merged-immediately.yml new file mode 100644 index 00000000000..41c06614e6d --- /dev/null +++ b/changelogs/unreleased/fix-mrs-merged-immediately.yml @@ -0,0 +1,4 @@ +--- +title: Don't mark empty MRs as merged on push to the target branch +merge_request: +author: diff --git a/spec/services/merge_requests/refresh_service_spec.rb b/spec/services/merge_requests/refresh_service_spec.rb index 671a932441e..74dcf152cb8 100644 --- a/spec/services/merge_requests/refresh_service_spec.rb +++ b/spec/services/merge_requests/refresh_service_spec.rb @@ -98,18 +98,52 @@ describe MergeRequests::RefreshService, services: true do end context 'push to origin repo target branch' do - before do - service.new(@project, @user).execute(@oldrev, @newrev, 'refs/heads/feature') - reload_mrs + context 'when all MRs to the target branch had diffs' do + before do + service.new(@project, @user).execute(@oldrev, @newrev, 'refs/heads/feature') + reload_mrs + end + + it 'updates the merge state' do + expect(@merge_request.notes.last.note).to include('merged') + expect(@merge_request).to be_merged + expect(@fork_merge_request).to be_merged + expect(@fork_merge_request.notes.last.note).to include('merged') + expect(@build_failed_todo).to be_done + expect(@fork_build_failed_todo).to be_done + end end - it 'updates the merge state' do - expect(@merge_request.notes.last.note).to include('merged') - expect(@merge_request).to be_merged - expect(@fork_merge_request).to be_merged - expect(@fork_merge_request.notes.last.note).to include('merged') - expect(@build_failed_todo).to be_done - expect(@fork_build_failed_todo).to be_done + context 'when an MR to be closed was empty already' do + let!(:empty_fork_merge_request) do + create(:merge_request, + source_project: @fork_project, + source_branch: 'master', + target_branch: 'master', + target_project: @project) + end + + before do + # This spec already has a fake push, so pretend that we were targeting + # feature all along. + empty_fork_merge_request.update_columns(target_branch: 'feature') + + service.new(@project, @user).execute(@oldrev, @newrev, 'refs/heads/feature') + reload_mrs + empty_fork_merge_request.reload + end + + it 'only updates the non-empty MRs' do + expect(@merge_request).to be_merged + expect(@merge_request.notes.last.note).to include('merged') + + expect(@fork_merge_request).to be_merged + expect(@fork_merge_request.notes.last.note).to include('merged') + + expect(empty_fork_merge_request).to be_open + expect(empty_fork_merge_request.merge_request_diff.state).to eq('empty') + expect(empty_fork_merge_request.notes).to be_empty + end end end