From 1f7647f446c9659ec0a41e48433a711e95f0b153 Mon Sep 17 00:00:00 2001 From: Mark Chao Date: Sat, 3 Nov 2018 17:36:19 +0800 Subject: [PATCH 1/3] Update merge request's merge_commit for branch update Analyze new commits graph to determine each commit's merge commit. Fix "merged with [commit]" info for merge requests being merged automatically by other actions. Allow analyzing upto the relevant commit --- .../merge_requests/refresh_service.rb | 19 ++- .../48889-populate-merge_commit_sha.yml | 6 + .../branch_push_merge_commit_analyzer.rb | 121 ++++++++++++++++++ .../branch_push_merge_commit_analyzer_spec.rb | 43 +++++++ .../merge_requests/refresh_service_spec.rb | 49 +++++++ spec/support/helpers/test_env.rb | 3 + 6 files changed, 236 insertions(+), 5 deletions(-) create mode 100644 changelogs/unreleased/48889-populate-merge_commit_sha.yml create mode 100644 lib/gitlab/branch_push_merge_commit_analyzer.rb create mode 100644 spec/lib/gitlab/branch_push_merge_commit_analyzer_spec.rb diff --git a/app/services/merge_requests/refresh_service.rb b/app/services/merge_requests/refresh_service.rb index 667b5916f38..91de910796b 100644 --- a/app/services/merge_requests/refresh_service.rb +++ b/app/services/merge_requests/refresh_service.rb @@ -58,13 +58,22 @@ module MergeRequests .preload(:latest_merge_request_diff) .where(target_branch: @push.branch_name).to_a .select(&:diff_head_commit) + .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) - merge_requests = merge_requests.select do |merge_request| - commit_ids.include?(merge_request.diff_head_sha) && - merge_request.merge_request_diff.state != 'empty' - end + return if merge_requests.empty? + + analyzer = Gitlab::BranchPushMergeCommitAnalyzer.new( + @commits.reverse, + relevant_commit_ids: merge_requests.map(&:diff_head_sha) + ) + + merge_requests.each do |merge_request| + merge_request.merge_commit_sha = analyzer.get_merge_commit(merge_request.diff_head_sha) - filter_merge_requests(merge_requests).each do |merge_request| MergeRequests::PostMergeService .new(merge_request.target_project, @current_user) .execute(merge_request) diff --git a/changelogs/unreleased/48889-populate-merge_commit_sha.yml b/changelogs/unreleased/48889-populate-merge_commit_sha.yml new file mode 100644 index 00000000000..0e25d8ecfb0 --- /dev/null +++ b/changelogs/unreleased/48889-populate-merge_commit_sha.yml @@ -0,0 +1,6 @@ +--- +title: Fix "merged with [commit]" info for merge requests being merged automatically + by other actions +merge_request: 22794 +author: +type: fixed diff --git a/lib/gitlab/branch_push_merge_commit_analyzer.rb b/lib/gitlab/branch_push_merge_commit_analyzer.rb new file mode 100644 index 00000000000..046e83b0cf1 --- /dev/null +++ b/lib/gitlab/branch_push_merge_commit_analyzer.rb @@ -0,0 +1,121 @@ +# 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, 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 + 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 + + # 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.with_index do |parent_commit_id, i| + parent_commit = get_commit(parent_commit_id) + + next if parent_commit.nil? # parent commit may not be part of new commits + + parent_commit.set_merge_commit(commit, first_parent: i == 0) + end + end + + def get_commit(id) + @id_to_commit[id] + end + 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 new file mode 100644 index 00000000000..45e182358df --- /dev/null +++ b/spec/lib/gitlab/branch_push_merge_commit_analyzer_spec.rb @@ -0,0 +1,43 @@ +# 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 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 diff --git a/spec/services/merge_requests/refresh_service_spec.rb b/spec/services/merge_requests/refresh_service_spec.rb index d29a1091d95..493d3eb7392 100644 --- a/spec/services/merge_requests/refresh_service_spec.rb +++ b/spec/services/merge_requests/refresh_service_spec.rb @@ -621,4 +621,53 @@ 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') } + + 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 end diff --git a/spec/support/helpers/test_env.rb b/spec/support/helpers/test_env.rb index 1f00cdf7e92..d52c40ff4f1 100644 --- a/spec/support/helpers/test_env.rb +++ b/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', From c6c53d1c7418b2c83410a21bce068a6dfd7858b0 Mon Sep 17 00:00:00 2001 From: Mark Chao Date: Wed, 28 Nov 2018 23:10:48 +0800 Subject: [PATCH 2/3] 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. --- .../branch_push_merge_commit_analyzer.rb | 33 ++++++++++++------- .../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(commit, first_parent: i == 0) + 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 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]) } From 2f7563a6746519516c1464ac2a74b7e3c0eca63f Mon Sep 17 00:00:00 2001 From: Mark Chao Date: Fri, 7 Dec 2018 17:58:39 +0800 Subject: [PATCH 3/3] Guard with feature flag --- .../merge_requests/refresh_service.rb | 15 ++++--- .../merge_requests/refresh_service_spec.rb | 44 ++++++++++++++----- 2 files changed, 44 insertions(+), 15 deletions(-) diff --git a/app/services/merge_requests/refresh_service.rb b/app/services/merge_requests/refresh_service.rb index 91de910796b..f712b8863cd 100644 --- a/app/services/merge_requests/refresh_service.rb +++ b/app/services/merge_requests/refresh_service.rb @@ -66,13 +66,18 @@ module MergeRequests return if merge_requests.empty? - analyzer = Gitlab::BranchPushMergeCommitAnalyzer.new( - @commits.reverse, - relevant_commit_ids: merge_requests.map(&:diff_head_sha) - ) + 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| - merge_request.merge_commit_sha = analyzer.get_merge_commit(merge_request.diff_head_sha) + if commit_analyze_enabled + merge_request.merge_commit_sha = analyzer.get_merge_commit(merge_request.diff_head_sha) + end MergeRequests::PostMergeService .new(merge_request.target_project, @current_user) diff --git a/spec/services/merge_requests/refresh_service_spec.rb b/spec/services/merge_requests/refresh_service_spec.rb index 493d3eb7392..1d9c75dedce 100644 --- a/spec/services/merge_requests/refresh_service_spec.rb +++ b/spec/services/merge_requests/refresh_service_spec.rb @@ -654,20 +654,44 @@ describe MergeRequests::RefreshService do subject { service.execute(oldrev, newrev, 'refs/heads/merge-commit-analyze-before') } - 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) + context 'feature enabled' do + before do + stub_feature_flags(branch_push_merge_commit_analyze: true) end - subject + 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}) - merge_request.reload - merge_request_side_branch.reload + original_method.call(commits) + end - expect(merge_request.merge_commit.id).to eq('646ece5cfed840eca0a4feb21bcd6a81bb19bda3') - expect(merge_request_side_branch.merge_commit.id).to eq('29284d9bcc350bcae005872d0be6edd016e2efb5') + 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