Fix commit with two parents is set with wrong direct_ancestor48889-populate-merge_commit_sha

If a commit has two parents, one is direct ancestor, and one is not,
and the order of `commits` is in such fashion that the non-ancestor
side is visited first, the commit would be determined as non-ancestor,
when in fact it can be.

Therefore we should first determine all direct ancestors
prior to analyzing.

2 files changed, 41 insertions, 11 deletions

diff --git a/lib/gitlab/branch_push_merge_commit_analyzer.rb b/lib/gitlab/branch_push_merge_commit_analyzer.rb
index 046e83b0cf1..a8f601f2451 100644
--- a/lib/gitlab/branch_push_merge_commit_analyzer.rb
+++ b/lib/gitlab/branch_push_merge_commit_analyzer.rb
@@ -59,14 +59,8 @@ module Gitlab
 
       # @param child_commit [CommitDecorator]
       # @param first_parent [Boolean] whether `self` is the first parent of `child_commit`
-      def set_merge_commit(child_commit, first_parent:)
-        # If child commit is a direct ancestor, its first parent is also the 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)
-        @direct_ancestor = first_parent && child_commit.direct_ancestor?
-
-        @merge_commit = direct_ancestor? ? self : child_commit.merge_commit
+      def set_merge_commit(child_commit:)
+        @merge_commit ||= direct_ancestor? ? self : child_commit.merge_commit
       end
     end
 
@@ -97,6 +91,8 @@ module Gitlab
       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|
@@ -105,12 +101,27 @@ module Gitlab
     end
 
     def analyze_parents(commit)
-      commit.parent_ids.each.with_index do |parent_commit_id, i|
+      commit.parent_ids.each do |parent_commit_id|
         parent_commit = get_commit(parent_commit_id)
 
-        next if parent_commit.nil? # parent commit may not be part of new commits
+        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
 
-        parent_commit.set_merge_commit(commit, first_parent: i == 0)
+        commit.direct_ancestor = true
       end
     end
 
diff --git a/spec/lib/gitlab/branch_push_merge_commit_analyzer_spec.rb b/spec/lib/gitlab/branch_push_merge_commit_analyzer_spec.rb
index 45e182358df..dff4c258c45 100644
--- a/spec/lib/gitlab/branch_push_merge_commit_analyzer_spec.rb
+++ b/spec/lib/gitlab/branch_push_merge_commit_analyzer_spec.rb
@@ -28,6 +28,25 @@ describe Gitlab::BranchPushMergeCommitAnalyzer do
       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]) }