Merge branch 'sh-move-mr-diff-after-commit' into 'master'

Reduce idle in transaction time when updating a merge request

Closes #33650

See merge request gitlab-org/gitlab!18493

app/models/merge_request_diff.rb

@@ -136,6 +136,7 @@ class MergeRequestDiff < ApplicationRecord
   # All diff information is collected from repository after object is created.
   # It allows you to override variables like head_commit_sha before getting diff.
   after_create :save_git_content, unless: :importing?
+  after_create_commit :set_as_latest_diff
 
   after_save :update_external_diff_store, if: -> { !importing? && saved_change_to_external_diff? }
 
@@ -150,10 +151,6 @@ class MergeRequestDiff < ApplicationRecord
   # Collect information about commits and diff from repository
   # and save it to the database as serialized data
   def save_git_content
-    MergeRequest
-      .where('id = ? AND COALESCE(latest_merge_request_diff_id, 0) < ?', self.merge_request_id, self.id)
-      .update_all(latest_merge_request_diff_id: self.id)
-
     ensure_commit_shas
     save_commits
     save_diffs
@@ -168,6 +165,12 @@ class MergeRequestDiff < ApplicationRecord
     keep_around_commits
   end
 
+  def set_as_latest_diff
+    MergeRequest
+      .where('id = ? AND COALESCE(latest_merge_request_diff_id, 0) < ?', self.merge_request_id, self.id)
+      .update_all(latest_merge_request_diff_id: self.id)
+  end
+
   def ensure_commit_shas
     self.start_commit_sha ||= merge_request.target_branch_sha
     self.head_commit_sha  ||= merge_request.source_branch_sha
@@ -502,11 +505,6 @@ class MergeRequestDiff < ApplicationRecord
     merge_request.closed? && merge_request.metrics.latest_closed_at < EXTERNAL_DIFF_CUTOFF.ago
   end
 
-  # We can't rely on `merge_request.latest_merge_request_diff_id` because that
-  # may have been changed in `save_git_content` without being reflected in
-  # the association's instance. This query is always subject to races, but
-  # the worst case is that we *don't* make a diff external when we could. The
-  # background worker will make it external at a later date.
   def old_version?
     latest_id = MergeRequest
       .where(id: merge_request_id)
@@ -514,7 +512,7 @@ class MergeRequestDiff < ApplicationRecord
       .pluck(:latest_merge_request_diff_id)
       .first
 
-    self.id != latest_id
+    latest_id && self.id < latest_id
   end
 
   def load_diffs(options)

changelogs/unreleased/sh-move-mr-diff-after-commit.yml

+---
+title: Reduce idle in transaction time when updating a merge request
+merge_request: 18493
+author:
+type: performance

spec/lib/gitlab/github_import/importer/pull_request_importer_spec.rb

@@ -311,10 +311,11 @@ describe Gitlab::GithubImport::Importer::PullRequestImporter, :clean_gitlab_redi
       end
     end
 
-    it 'creates the merge request diffs' do
+    it 'creates a merge request diff and sets it as the latest' do
       mr = insert_git_data
 
       expect(mr.merge_request_diffs.exists?).to eq(true)
+      expect(mr.reload.latest_merge_request_diff_id).to eq(mr.merge_request_diffs.first.id)
     end
 
     it 'creates the merge request diff commits' do

spec/lib/gitlab/import/merge_request_creator_spec.rb

@@ -21,8 +21,11 @@ describe Gitlab::Import::MergeRequestCreator do
 
         subject.execute(attributes)
 
-        expect(merge_request.reload.merge_request_diffs.count).to eq(1)
-        expect(merge_request.reload.merge_request_diffs.first.commits.count).to eq(commits_count)
+        merge_request.reload
+
+        expect(merge_request.merge_request_diffs.count).to eq(1)
+        expect(merge_request.merge_request_diffs.first.commits.count).to eq(commits_count)
+        expect(merge_request.latest_merge_request_diff_id).to eq(merge_request.merge_request_diffs.first.id)
       end
     end