Commit c6c53d1c authored by Mark Chao's avatar Mark Chao

Fix commit with two parents is set with wrong direct_ancestor

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.
parent 1f7647f4
...@@ -59,14 +59,8 @@ module Gitlab ...@@ -59,14 +59,8 @@ module Gitlab
# @param child_commit [CommitDecorator] # @param child_commit [CommitDecorator]
# @param first_parent [Boolean] whether `self` is the first parent of `child_commit` # @param first_parent [Boolean] whether `self` is the first parent of `child_commit`
def set_merge_commit(child_commit, first_parent:) def set_merge_commit(child_commit:)
# If child commit is a direct ancestor, its first parent is also the direct ancestor. @merge_commit ||= direct_ancestor? ? self : child_commit.merge_commit
# 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
end end
end end
...@@ -97,6 +91,8 @@ module Gitlab ...@@ -97,6 +91,8 @@ module Gitlab
head_commit.direct_ancestor = true head_commit.direct_ancestor = true
head_commit.merge_commit = head_commit head_commit.merge_commit = head_commit
mark_all_direct_ancestors(head_commit)
# Analyzing a commit requires its child commit be analyzed first, # Analyzing a commit requires its child commit be analyzed first,
# which is the case here since commits are ordered from child to parent. # which is the case here since commits are ordered from child to parent.
@id_to_commit.each_value do |commit| @id_to_commit.each_value do |commit|
...@@ -105,12 +101,27 @@ module Gitlab ...@@ -105,12 +101,27 @@ module Gitlab
end end
def analyze_parents(commit) 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) 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
end end
......
...@@ -28,6 +28,25 @@ describe Gitlab::BranchPushMergeCommitAnalyzer do ...@@ -28,6 +28,25 @@ describe Gitlab::BranchPushMergeCommitAnalyzer do
end end
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 context 'when relevant_commit_ids is provided' do
let(:relevant_commit_id) { '8a994512e8c8f0dfcf22bb16df6e876be7a61036' } let(:relevant_commit_id) { '8a994512e8c8f0dfcf22bb16df6e876be7a61036' }
subject { described_class.new(commits, relevant_commit_ids: [relevant_commit_id]) } subject { described_class.new(commits, relevant_commit_ids: [relevant_commit_id]) }
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment