Merge branch 'fix-mrs-merged-immediately' into 'master'

Don't mark empty MRs as merged on push to the target branch

Closes #32800

See merge request !12711

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|

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:

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
+      context 'when all MRs to the target branch had diffs' do
-        service.new(@project, @user).execute(@oldrev, @newrev, 'refs/heads/feature')
+        before do
-        reload_mrs
+          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
+      context 'when an MR to be closed was empty already' do
-        expect(@merge_request.notes.last.note).to include('merged')
+        let!(:empty_fork_merge_request) do
-        expect(@merge_request).to be_merged
+          create(:merge_request,
-        expect(@fork_merge_request).to be_merged
+                 source_project: @fork_project,
-        expect(@fork_merge_request.notes.last.note).to include('merged')
+                 source_branch: 'master',
-        expect(@build_failed_todo).to be_done
+                 target_branch: 'master',
-        expect(@fork_build_failed_todo).to be_done
+                 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