Merge branch 'changelog-include-merge-commits' into 'master'

Include MRs for merge commits for changelogs

See merge request gitlab-org/gitlab!55371

app/finders/merge_requests/oldest_per_commit_finder.rb

@@ -15,19 +15,45 @@ module MergeRequests
     # Returns a Hash that maps a commit ID to the oldest merge request that
     # introduced that commit.
     def execute(commits)
+      mapping = {}
+      shas = commits.map(&:id)
+
+      # To include merge requests by the commit SHA, we don't need to go through
+      # any diff rows.
+      #
+      # We can't squeeze all this into a single query, as the diff based data
+      # relies on a GROUP BY. On the other hand, retrieving MRs by their merge
+      # SHAs separately is much easier, and plenty fast.
+      @project
+        .merge_requests
+        .preload_target_project
+        .by_merge_commit_sha(shas)
+        .each do |mr|
+          # Merge SHAs can't be in the merge request itself. It _is_ possible a
+          # newer merge request includes the merge commit, but in that case we
+          # still want the oldest merge request.
+          mapping[mr.merge_commit_sha] = mr
+        end
+
+      remaining = shas - mapping.keys
+
+      return mapping if remaining.empty?
+
       id_rows = MergeRequestDiffCommit
-        .oldest_merge_request_id_per_commit(@project.id, commits.map(&:id))
+        .oldest_merge_request_id_per_commit(@project.id, remaining)
 
       mrs = MergeRequest
         .preload_target_project
         .id_in(id_rows.map { |r| r[:merge_request_id] })
         .index_by(&:id)
 
-      id_rows.each_with_object({}) do |row, hash|
+      id_rows.each do |row|
         if (mr = mrs[row[:merge_request_id]])
-          hash[row[:sha]] = mr
+          mapping[row[:sha]] = mr
         end
       end
+
+      mapping
     end
   end
 end

changelogs/unreleased/changelog-include-merge-commits.yml

+---
+title: Include MRs for merge commits for changelogs
+merge_request: 55371
+author:
+type: fixed

spec/finders/merge_requests/oldest_per_commit_finder_spec.rb

@@ -6,12 +6,20 @@ RSpec.describe MergeRequests::OldestPerCommitFinder do
   describe '#execute' do
     it 'returns a Hash mapping commit SHAs to their oldest merge requests' do
       project = create(:project)
+      sha1 = Digest::SHA1.hexdigest('foo')
+      sha2 = Digest::SHA1.hexdigest('bar')
+      sha3 = Digest::SHA1.hexdigest('baz')
       mr1 = create(:merge_request, :merged, target_project: project)
       mr2 = create(:merge_request, :merged, target_project: project)
+      mr3 = create(
+        :merge_request,
+        :merged,
+        target_project: project,
+        merge_commit_sha: sha3
+      )
+
       mr1_diff = create(:merge_request_diff, merge_request: mr1)
       mr2_diff = create(:merge_request_diff, merge_request: mr2)
-      sha1 = Digest::SHA1.hexdigest('foo')
-      sha2 = Digest::SHA1.hexdigest('bar')
 
       create(:merge_request_diff_commit, merge_request_diff: mr1_diff, sha: sha1)
       create(:merge_request_diff_commit, merge_request_diff: mr2_diff, sha: sha1)
@@ -22,11 +30,16 @@ RSpec.describe MergeRequests::OldestPerCommitFinder do
         relative_order: 1
       )
 
-      commits = [double(:commit, id: sha1), double(:commit, id: sha2)]
+      commits = [
+        double(:commit, id: sha1),
+        double(:commit, id: sha2),
+        double(:commit, id: sha3)
+      ]
 
       expect(described_class.new(project).execute(commits)).to eq(
         sha1 => mr1,
-        sha2 => mr2
+        sha2 => mr2,
+        sha3 => mr3
       )
     end
 
@@ -42,5 +55,45 @@ RSpec.describe MergeRequests::OldestPerCommitFinder do
       expect(described_class.new(mr.target_project).execute(commits))
         .to be_empty
     end
+
+    it 'includes the merge request for a merge commit' do
+      project = create(:project)
+      sha = Digest::SHA1.hexdigest('foo')
+      mr = create(
+        :merge_request,
+        :merged,
+        target_project: project,
+        merge_commit_sha: sha
+      )
+
+      commits = [double(:commit, id: sha)]
+
+      # This expectation is set so we're certain that the merge commit SHAs (if
+      # a matching merge request is found) aren't also used for finding MRs
+      # according to diffs.
+      expect(MergeRequestDiffCommit)
+        .not_to receive(:oldest_merge_request_id_per_commit)
+
+      expect(described_class.new(project).execute(commits)).to eq(sha => mr)
+    end
+
+    it 'includes the oldest merge request when a merge commit is present in a newer merge request' do
+      project = create(:project)
+      sha = Digest::SHA1.hexdigest('foo')
+      mr1 = create(
+        :merge_request,
+        :merged,
+        target_project: project, merge_commit_sha: sha
+      )
+
+      mr2 = create(:merge_request, :merged, target_project: project)
+      mr_diff = create(:merge_request_diff, merge_request: mr2)
+
+      create(:merge_request_diff_commit, merge_request_diff: mr_diff, sha: sha)
+
+      commits = [double(:commit, id: sha)]
+
+      expect(described_class.new(project).execute(commits)).to eq(sha => mr1)
+    end
   end
 end

spec/services/repositories/changelog_service_spec.rb

@@ -76,7 +76,7 @@ RSpec.describe Repositories::ChangelogService do
       recorder = ActiveRecord::QueryRecorder.new { service.execute }
       changelog = project.repository.blob_at('master', 'CHANGELOG.md')&.data
 
-      expect(recorder.count).to eq(10)
+      expect(recorder.count).to eq(11)
       expect(changelog).to include('Title 1', 'Title 2')
     end