Merge branch 'osw-fix-lost-diffs-when-source-branch-deleted' into 'master'

app/services/merge_requests/refresh_service.rb

@@ -9,7 +9,8 @@ module MergeRequests
       Gitlab::GitalyClient.allow_n_plus_1_calls(&method(:find_new_commits))
       # Be sure to close outstanding MRs before reloading them to avoid generating an
       # empty diff during a manual merge
-      close_merge_requests
+      close_upon_missing_source_branch_ref
+      post_merge_manually_merged
       reload_merge_requests
       reset_merge_when_pipeline_succeeds
       mark_pending_todos_done
@@ -29,11 +30,22 @@ module MergeRequests
 
     private
 
+    def close_upon_missing_source_branch_ref
+      # MergeRequest#reload_diff ignores not opened MRs. This means it won't
+      # create an `empty` diff for `closed` MRs without a source branch, keeping
+      # the latest diff state as the last _valid_ one.
+      merge_requests_for_source_branch.reject(&:source_branch_exists?).each do |mr|
+        MergeRequests::CloseService
+          .new(mr.target_project, @current_user)
+          .execute(mr)
+      end
+    end
+
     # Collect open merge requests that target same branch we push into
     # and close if push to master include last commit from merge request
     # We need this to close(as merged) merge requests that were merged into
     # target branch manually
-    def close_merge_requests
+    def post_merge_manually_merged
       commit_ids = @commits.map(&:id)
       merge_requests = @project.merge_requests.preload(:latest_merge_request_diff).opened.where(target_branch: @branch_name).to_a
       merge_requests = merge_requests.select(&:diff_head_commit)

changelogs/unreleased/osw-fix-lost-diffs-when-source-branch-deleted.yml

+---
+title: Close and do not reload MR diffs when source branch is deleted
+merge_request:
+author:
+type: fixed

spec/models/merge_request_spec.rb

@@ -1539,7 +1539,7 @@ describe MergeRequest do
       expect { subject.reload_diff }.to change { subject.merge_request_diffs.count }.by(1)
     end
 
-    it "executs diff cache service" do
+    it "executes diff cache service" do
       expect_any_instance_of(MergeRequests::MergeRequestDiffCacheService).to receive(:execute).with(subject)
 
       subject.reload_diff

spec/services/merge_requests/refresh_service_spec.rb

@@ -55,11 +55,12 @@ describe MergeRequests::RefreshService do
 
       before do
         allow(refresh_service).to receive(:execute_hooks)
-        refresh_service.execute(@oldrev, @newrev, 'refs/heads/master')
-        reload_mrs
       end
 
       it 'executes hooks with update action' do
+        refresh_service.execute(@oldrev, @newrev, 'refs/heads/master')
+        reload_mrs
+
         expect(refresh_service).to have_received(:execute_hooks)
           .with(@merge_request, 'update', old_rev: @oldrev)
 
@@ -72,6 +73,26 @@ describe MergeRequests::RefreshService do
         expect(@build_failed_todo).to be_done
         expect(@fork_build_failed_todo).to be_done
       end
+
+      context 'when source branch ref does not exists' do
+        before do
+          DeleteBranchService.new(@project, @user).execute(@merge_request.source_branch)
+        end
+
+        it 'closes MRs without source branch ref' do
+          expect { refresh_service.execute(@oldrev, @newrev, 'refs/heads/master') }
+            .to change { @merge_request.reload.state }
+            .from('opened')
+            .to('closed')
+
+          expect(@fork_merge_request.reload).to be_open
+        end
+
+        it 'does not change the merge request diff' do
+          expect { refresh_service.execute(@oldrev, @newrev, 'refs/heads/master') }
+            .not_to change { @merge_request.reload.merge_request_diff }
+        end
+      end
     end
 
     context 'when pipeline exists for the source branch' do