diff options
author | Mark Chao <mchao@gitlab.com> | 2018-11-28 18:10:48 +0300 |
---|---|---|
committer | Mark Chao <mchao@gitlab.com> | 2018-12-07 16:00:47 +0300 |
commit | c6c53d1c7418b2c83410a21bce068a6dfd7858b0 (patch) | |
tree | 72585e4cbc29c639bc809d33f1b12429a8eb69fb | |
parent | 1f7647f446c9659ec0a41e48433a711e95f0b153 (diff) |
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.
-rw-r--r-- | lib/gitlab/branch_push_merge_commit_analyzer.rb | 33 | ||||
-rw-r--r-- | spec/lib/gitlab/branch_push_merge_commit_analyzer_spec.rb | 19 |
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..1e969542975 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]) } |