Merge branch '48889-populate-merge_commit_sha' into 'master'

Update merge request's merge_commit after branch update

Closes #48889

See merge request gitlab-org/gitlab-ce!22794

app/services/merge_requests/refresh_service.rb

@@ -58,13 +58,27 @@ module MergeRequests
         .preload(:latest_merge_request_diff)
         .where(target_branch: @push.branch_name).to_a
         .select(&:diff_head_commit)
-
-      merge_requests = merge_requests.select do |merge_request|
+        .select do |merge_request|
           commit_ids.include?(merge_request.diff_head_sha) &&
             merge_request.merge_request_diff.state != 'empty'
         end
+      merge_requests = filter_merge_requests(merge_requests)
+
+      return if merge_requests.empty?
+
+      commit_analyze_enabled = Feature.enabled?(:branch_push_merge_commit_analyze, @project, default_enabled: true)
+      if commit_analyze_enabled
+        analyzer = Gitlab::BranchPushMergeCommitAnalyzer.new(
+          @commits.reverse,
+          relevant_commit_ids: merge_requests.map(&:diff_head_sha)
+        )
+      end
+
+      merge_requests.each do |merge_request|
+        if commit_analyze_enabled
+          merge_request.merge_commit_sha = analyzer.get_merge_commit(merge_request.diff_head_sha)
+        end
 
-      filter_merge_requests(merge_requests).each do |merge_request|
         MergeRequests::PostMergeService
           .new(merge_request.target_project, @current_user)
           .execute(merge_request)

changelogs/unreleased/48889-populate-merge_commit_sha.yml

+---
+title: Fix "merged with [commit]" info for merge requests being merged automatically
+  by other actions
+merge_request: 22794
+author:
+type: fixed

lib/gitlab/branch_push_merge_commit_analyzer.rb

+# frozen_string_literal: true
+
+module Gitlab
+  # Analyse a graph of commits from a push to a branch,
+  # for each commit, analyze that if it is the head of a merge request,
+  # then what should its merge_commit be, relative to the branch.
+  #
+  # A----->B----->C----->D   target branch
+  # |             ^
+  # |             |
+  # +-->E----->F--+          merged branch
+  #     |     ^
+  #     |     |
+  #     +->G--+
+  #
+  # (See merge-commit-analyze-after branch in gitlab-test)
+  #
+  # Assuming
+  # - A is already in remote
+  # - B~D are all in its own branch with its own merge request, targeting the target branch
+  #
+  # When D is finally pushed to the target branch,
+  # what are the merge commits for all the other merge requests?
+  #
+  # We can walk backwards from the HEAD commit D,
+  # and find status of its parents.
+  # First we determine if commit belongs to the target branch (i.e. A, B, C, D),
+  # and then determine its merge commit.
+  #
+  # +--------+-----------------+--------------+
+  # | Commit | Direct ancestor | Merge commit |
+  # +--------+-----------------+--------------+
+  # | D      | Y               | D            |
+  # +--------+-----------------+--------------+
+  # | C      | Y               | C            |
+  # +--------+-----------------+--------------+
+  # | F      |                 | C            |
+  # +--------+-----------------+--------------+
+  # | B      | Y               | B            |
+  # +--------+-----------------+--------------+
+  # | E      |                 | C            |
+  # +--------+-----------------+--------------+
+  # | G      |                 | C            |
+  # +--------+-----------------+--------------+
+  #
+  # By examining the result, it can be said that
+  #
+  # - If commit is direct ancestor of HEAD, its merge commit is itself.
+  # - Otherwise, the merge commit is the same as its child's merge commit.
+  #
+  class BranchPushMergeCommitAnalyzer
+    class CommitDecorator < SimpleDelegator
+      attr_accessor :merge_commit
+      attr_writer :direct_ancestor # boolean
+
+      def direct_ancestor?
+        @direct_ancestor
+      end
+
+      # @param child_commit [CommitDecorator]
+      # @param first_parent [Boolean] whether `self` is the first parent of `child_commit`
+      def set_merge_commit(child_commit:)
+        @merge_commit ||= direct_ancestor? ? self : child_commit.merge_commit
+      end
+    end
+
+    # @param commits [Array] list of commits, must be ordered from the child (tip) of the graph back to the ancestors
+    def initialize(commits, relevant_commit_ids: nil)
+      @commits = commits
+      @id_to_commit = {}
+      @commits.each do |commit|
+        @id_to_commit[commit.id] = CommitDecorator.new(commit)
+
+        if relevant_commit_ids
+          relevant_commit_ids.delete(commit.id)
+          break if relevant_commit_ids.empty? # Only limit the analyze up to relevant_commit_ids
+        end
+      end
+
+      analyze
+    end
+
+    def get_merge_commit(id)
+      get_commit(id).merge_commit.id
+    end
+
+    private
+
+    def analyze
+      head_commit = get_commit(@commits.first.id)
+      head_commit.direct_ancestor = true
+      head_commit.merge_commit = head_commit
+
+      mark_all_direct_ancestors(head_commit)
+
+      # Analyzing a commit requires its child commit be analyzed first,
+      # which is the case here since commits are ordered from child to parent.
+      @id_to_commit.each_value do |commit|
+        analyze_parents(commit)
+      end
+    end
+
+    def analyze_parents(commit)
+      commit.parent_ids.each do |parent_commit_id|
+        parent_commit = get_commit(parent_commit_id)
+
+        next unless parent_commit # parent commit may not be part of new commits
+
+        parent_commit.set_merge_commit(child_commit: commit)
+      end
+    end
+
+    # Mark all direct ancestors.
+    # If child commit is a direct ancestor, its first parent is also a direct ancestor.
+    # We assume direct ancestors matches the trail of the target branch over time,
+    # This assumption is correct most of the time, especially for gitlab managed merges,
+    # but there are exception cases which can't be solved (https://stackoverflow.com/a/49754723/474597)
+    def mark_all_direct_ancestors(commit)
+      loop do
+        commit = get_commit(commit.parent_ids.first)
+
+        break unless commit
+
+        commit.direct_ancestor = true
+      end
+    end
+
+    def get_commit(id)
+      @id_to_commit[id]
+    end
+  end
+end

spec/lib/gitlab/branch_push_merge_commit_analyzer_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Gitlab::BranchPushMergeCommitAnalyzer do
+  let(:project) { create(:project, :repository) }
+  let(:oldrev) { 'merge-commit-analyze-before' }
+  let(:newrev) { 'merge-commit-analyze-after' }
+  let(:commits) { project.repository.commits_between(oldrev, newrev).reverse }
+
+  subject { described_class.new(commits) }
+
+  describe '#get_merge_commit' do
+    let(:expected_merge_commits) do
+      {
+        '646ece5cfed840eca0a4feb21bcd6a81bb19bda3' => '646ece5cfed840eca0a4feb21bcd6a81bb19bda3',
+        '29284d9bcc350bcae005872d0be6edd016e2efb5' => '29284d9bcc350bcae005872d0be6edd016e2efb5',
+        '5f82584f0a907f3b30cfce5bb8df371454a90051' => '29284d9bcc350bcae005872d0be6edd016e2efb5',
+        '8a994512e8c8f0dfcf22bb16df6e876be7a61036' => '29284d9bcc350bcae005872d0be6edd016e2efb5',
+        '689600b91aabec706e657e38ea706ece1ee8268f' => '29284d9bcc350bcae005872d0be6edd016e2efb5',
+        'db46a1c5a5e474aa169b6cdb7a522d891bc4c5f9' => 'db46a1c5a5e474aa169b6cdb7a522d891bc4c5f9'
+      }
+    end
+
+    it 'returns correct merge commit SHA for each commit' do
+      expected_merge_commits.each do |commit, merge_commit|
+        expect(subject.get_merge_commit(commit)).to eq(merge_commit)
+      end
+    end
+
+    context 'when one parent has two children' do
+      let(:oldrev) { '1adbdefe31288f3bbe4b614853de4908a0b6f792' }
+      let(:newrev) { '5f82584f0a907f3b30cfce5bb8df371454a90051' }
+
+      let(:expected_merge_commits) do
+        {
+          '5f82584f0a907f3b30cfce5bb8df371454a90051' => '5f82584f0a907f3b30cfce5bb8df371454a90051',
+          '8a994512e8c8f0dfcf22bb16df6e876be7a61036' => '5f82584f0a907f3b30cfce5bb8df371454a90051',
+          '689600b91aabec706e657e38ea706ece1ee8268f' => '689600b91aabec706e657e38ea706ece1ee8268f'
+        }
+      end
+
+      it 'returns correct merge commit SHA for each commit' do
+        expected_merge_commits.each do |commit, merge_commit|
+          expect(subject.get_merge_commit(commit)).to eq(merge_commit)
+        end
+      end
+    end
+
+    context 'when relevant_commit_ids is provided' do
+      let(:relevant_commit_id) { '8a994512e8c8f0dfcf22bb16df6e876be7a61036' }
+      subject { described_class.new(commits, relevant_commit_ids: [relevant_commit_id]) }
+
+      it 'returns correct merge commit' do
+        expected_merge_commits.each do |commit, merge_commit|
+          subject = described_class.new(commits, relevant_commit_ids: [commit])
+          expect(subject.get_merge_commit(commit)).to eq(merge_commit)
+        end
+      end
+    end
+  end
+end

spec/services/merge_requests/refresh_service_spec.rb

@@ -621,4 +621,77 @@ describe MergeRequests::RefreshService do
       @fork_build_failed_todo.reload
     end
   end
+
+  describe 'updating merge_commit' do
+    let(:service) { described_class.new(project, user) }
+    let(:user) { create(:user) }
+    let(:project) { create(:project, :repository) }
+
+    let(:oldrev) { TestEnv::BRANCH_SHA['merge-commit-analyze-before'] }
+    let(:newrev) { TestEnv::BRANCH_SHA['merge-commit-analyze-after'] } # Pretend branch is now updated
+
+    let!(:merge_request) do
+      create(
+        :merge_request,
+        source_project: project,
+        source_branch: 'merge-commit-analyze-after',
+        target_branch: 'merge-commit-analyze-before',
+        target_project: project,
+        merge_user: user
+      )
+    end
+
+    let!(:merge_request_side_branch) do
+      create(
+        :merge_request,
+        source_project: project,
+        source_branch: 'merge-commit-analyze-side-branch',
+        target_branch: 'merge-commit-analyze-before',
+        target_project: project,
+        merge_user: user
+      )
+    end
+
+    subject { service.execute(oldrev, newrev, 'refs/heads/merge-commit-analyze-before') }
+
+    context 'feature enabled' do
+      before do
+        stub_feature_flags(branch_push_merge_commit_analyze: true)
+      end
+
+      it "updates merge requests' merge_commits" do
+        expect(Gitlab::BranchPushMergeCommitAnalyzer).to receive(:new).and_wrap_original do |original_method, commits|
+          expect(commits.map(&:id)).to eq(%w{646ece5cfed840eca0a4feb21bcd6a81bb19bda3 29284d9bcc350bcae005872d0be6edd016e2efb5 5f82584f0a907f3b30cfce5bb8df371454a90051 8a994512e8c8f0dfcf22bb16df6e876be7a61036 689600b91aabec706e657e38ea706ece1ee8268f db46a1c5a5e474aa169b6cdb7a522d891bc4c5f9})
+
+          original_method.call(commits)
+        end
+
+        subject
+
+        merge_request.reload
+        merge_request_side_branch.reload
+
+        expect(merge_request.merge_commit.id).to eq('646ece5cfed840eca0a4feb21bcd6a81bb19bda3')
+        expect(merge_request_side_branch.merge_commit.id).to eq('29284d9bcc350bcae005872d0be6edd016e2efb5')
+      end
+    end
+
+    context 'when feature is disabled' do
+      before do
+        stub_feature_flags(branch_push_merge_commit_analyze: false)
+      end
+
+      it "does not trigger analysis" do
+        expect(Gitlab::BranchPushMergeCommitAnalyzer).not_to receive(:new)
+
+        subject
+
+        merge_request.reload
+        merge_request_side_branch.reload
+
+        expect(merge_request.merge_commit).to eq(nil)
+        expect(merge_request_side_branch.merge_commit).to eq(nil)
+      end
+    end
+  end
 end

spec/support/helpers/test_env.rb

@@ -54,6 +54,9 @@ module TestEnv
     'add_images_and_changes'             => '010d106',
     'update-gitlab-shell-v-6-0-1'        => '2f61d70',
     'update-gitlab-shell-v-6-0-3'        => 'de78448',
+    'merge-commit-analyze-before'        => '1adbdef',
+    'merge-commit-analyze-side-branch'   => '8a99451',
+    'merge-commit-analyze-after'         => '646ece5',
     '2-mb-file'                          => 'bf12d25',
     'before-create-delete-modify-move'   => '845009f',
     'between-create-delete-modify-move'  => '3f5f443',