diff --git a/app/services/merge_requests/post_merge_service.rb b/app/services/merge_requests/post_merge_service.rb index f04ec3c3e80c83ecde1582ef428717ed06c70391..aafba9bfcefa4da1f58dec0ccea715bfd0eb653b 100644 --- a/app/services/merge_requests/post_merge_service.rb +++ b/app/services/merge_requests/post_merge_service.rb @@ -9,6 +9,8 @@ module MergeRequests class PostMergeService < MergeRequests::BaseService include RemovesRefs + MAX_RETARGET_MERGE_REQUESTS = 4 + def execute(merge_request) merge_request.mark_as_merged close_issues(merge_request) @@ -18,6 +20,7 @@ def execute(merge_request) merge_request_activity_counter.track_merge_mr_action(user: current_user) notification_service.merge_mr(merge_request, current_user) execute_hooks(merge_request, 'merge') + retarget_chain_merge_requests(merge_request) invalidate_cache_counts(merge_request, users: merge_request.assignees | merge_request.reviewers) merge_request.update_project_counter_caches delete_non_latest_diffs(merge_request) @@ -28,6 +31,34 @@ def execute(merge_request) private + def retarget_chain_merge_requests(merge_request) + return unless Feature.enabled?(:retarget_merge_requests, merge_request.target_project) + + # we can only retarget MRs that are targeting the same project + # and have a remove source branch set + return unless merge_request.for_same_project? && merge_request.remove_source_branch? + + # find another merge requests that + # - as a target have a current source project and branch + other_merge_requests = merge_request.source_project + .merge_requests + .opened + .by_target_branch(merge_request.source_branch) + .preload_source_project + .at_most(MAX_RETARGET_MERGE_REQUESTS) + + other_merge_requests.find_each do |other_merge_request| + # Update only MRs on projects that we have access to + next unless can?(current_user, :update_merge_request, other_merge_request.source_project) + + ::MergeRequests::UpdateService + .new(other_merge_request.source_project, current_user, + target_branch: merge_request.target_branch, + target_branch_was_deleted: true) + .execute(other_merge_request) + end + end + def close_issues(merge_request) return unless merge_request.target_branch == project.default_branch diff --git a/app/services/merge_requests/update_service.rb b/app/services/merge_requests/update_service.rb index 8cf84e32e85c3201fd17266a0c3385a11950f395..1707daff734a98d84f4f4ba032d7419d066ccd08 100644 --- a/app/services/merge_requests/update_service.rb +++ b/app/services/merge_requests/update_service.rb @@ -4,6 +4,12 @@ module MergeRequests class UpdateService < MergeRequests::BaseService extend ::Gitlab::Utils::Override + def initialize(project, user = nil, params = {}) + super + + @target_branch_was_deleted = @params.delete(:target_branch_was_deleted) + end + def execute(merge_request) # We don't allow change of source/target projects and source branch # after merge request was created @@ -36,7 +42,9 @@ def handle_changes(merge_request, options) end if merge_request.previous_changes.include?('target_branch') - create_branch_change_note(merge_request, 'target', + create_branch_change_note(merge_request, + 'target', + target_branch_was_deleted ? 'delete' : 'update', merge_request.previous_changes['target_branch'].first, merge_request.target_branch) @@ -130,6 +138,8 @@ def after_update(issuable) private + attr_reader :target_branch_was_deleted + def handle_milestone_change(merge_request) return if skip_milestone_email @@ -162,9 +172,9 @@ def handle_reviewers_change(merge_request, old_reviewers) merge_request_activity_counter.track_users_review_requested(users: new_reviewers) end - def create_branch_change_note(issuable, branch_type, old_branch, new_branch) + def create_branch_change_note(issuable, branch_type, event_type, old_branch, new_branch) SystemNoteService.change_branch( - issuable, issuable.project, current_user, branch_type, + issuable, issuable.project, current_user, branch_type, event_type, old_branch, new_branch) end diff --git a/app/services/system_note_service.rb b/app/services/system_note_service.rb index 58f72e9badc09f3ce29a86a29372a746a2fe93a6..7d654ca7f5bf605184cb46e4c557064eda36078f 100644 --- a/app/services/system_note_service.rb +++ b/app/services/system_note_service.rb @@ -168,16 +168,19 @@ def change_issue_confidentiality(issue, project, author) # project - Project owning noteable # author - User performing the change # branch_type - 'source' or 'target' + # event_type - the source of event: 'update' or 'delete' # old_branch - old branch name # new_branch - new branch name # - # Example Note text: + # Example Note text is based on event_type: # - # "changed target branch from `Old` to `New`" + # update: "changed target branch from `Old` to `New`" + # delete: "changed automatically target branch to `New` because `Old` was deleted" # # Returns the created Note object - def change_branch(noteable, project, author, branch_type, old_branch, new_branch) - ::SystemNotes::MergeRequestsService.new(noteable: noteable, project: project, author: author).change_branch(branch_type, old_branch, new_branch) + def change_branch(noteable, project, author, branch_type, event_type, old_branch, new_branch) + ::SystemNotes::MergeRequestsService.new(noteable: noteable, project: project, author: author) + .change_branch(branch_type, event_type, old_branch, new_branch) end # Called when a branch in Noteable is added or deleted diff --git a/app/services/system_notes/merge_requests_service.rb b/app/services/system_notes/merge_requests_service.rb index a51e2053394be89c7e17c67ba8ddf20701f54439..99e03e67bf1655396af6ad7cd6e0672c335af353 100644 --- a/app/services/system_notes/merge_requests_service.rb +++ b/app/services/system_notes/merge_requests_service.rb @@ -83,16 +83,26 @@ def diff_discussion_outdated(discussion, change_position) # Called when a branch in Noteable is changed # # branch_type - 'source' or 'target' + # event_type - the source of event: 'update' or 'delete' # old_branch - old branch name # new_branch - new branch name + + # Example Note text is based on event_type: # - # Example Note text: - # - # "changed target branch from `Old` to `New`" + # update: "changed target branch from `Old` to `New`" + # delete: "changed automatically target branch to `New` because `Old` was deleted" # # Returns the created Note object - def change_branch(branch_type, old_branch, new_branch) - body = "changed #{branch_type} branch from `#{old_branch}` to `#{new_branch}`" + def change_branch(branch_type, event_type, old_branch, new_branch) + body = + case event_type.to_s + when 'delete' + "changed automatically #{branch_type} branch to `#{new_branch}` because `#{old_branch}` was deleted" + when 'update' + "changed #{branch_type} branch from `#{old_branch}` to `#{new_branch}`" + else + raise ArgumentError, "invalid value for event_type: #{event_type}" + end create_note(NoteSummary.new(noteable, project, author, body, action: 'branch')) end diff --git a/changelogs/unreleased/retarget-branch.yml b/changelogs/unreleased/retarget-branch.yml new file mode 100644 index 0000000000000000000000000000000000000000..a879f7fdbeceaebf35dc1c42caacdbc7f6ab5591 --- /dev/null +++ b/changelogs/unreleased/retarget-branch.yml @@ -0,0 +1,5 @@ +--- +title: Automatically retarget merge requests +merge_request: 53710 +author: +type: added diff --git a/config/feature_flags/development/retarget_merge_requests.yml b/config/feature_flags/development/retarget_merge_requests.yml new file mode 100644 index 0000000000000000000000000000000000000000..39ac4be021998df5612e27fd8bcdbaebc454cb11 --- /dev/null +++ b/config/feature_flags/development/retarget_merge_requests.yml @@ -0,0 +1,8 @@ +--- +name: retarget_merge_requests +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/53710 +rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/320895 +milestone: '13.9' +type: development +group: group::memory +default_enabled: false diff --git a/doc/.vale/gitlab/spelling-exceptions.txt b/doc/.vale/gitlab/spelling-exceptions.txt index ad9eae469c7e80e2d24900b907eda8163fe9f95c..161f9b7131a996d267fffad4118192d0f3426a0f 100644 --- a/doc/.vale/gitlab/spelling-exceptions.txt +++ b/doc/.vale/gitlab/spelling-exceptions.txt @@ -488,6 +488,10 @@ resync resynced resyncing resyncs +retarget +retargeted +retargeting +retargets reusability reverified reverifies diff --git a/doc/administration/instance_limits.md b/doc/administration/instance_limits.md index ab0880df3030927cea822bdf505352fd05c7aa9d..27e8b13812fcd06108f3ddc06091b6d81a27b975 100644 --- a/doc/administration/instance_limits.md +++ b/doc/administration/instance_limits.md @@ -626,3 +626,11 @@ Set the limit to `0` to allow any file size. ### Package versions returned When asking for versions of a given NuGet package name, the GitLab Package Registry returns a maximum of 300 versions. + +## Branch retargeting on merge **(FREE SELF)** + +> [Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/320902) in GitLab 13.9. + +If a branch is merged while open merge requests still point to it, GitLab can +retarget merge requests pointing to the now-merged branch. To learn more, read +[Branch retargeting on merge](../user/project/merge_requests/getting_started.md#branch-retargeting-on-merge). diff --git a/doc/user/project/merge_requests/getting_started.md b/doc/user/project/merge_requests/getting_started.md index 92db8bb2618db5582184c3461dcbcb0006f8a2d3..b1a57d9c3e63ed04999c87eefa49e5cfe65680d4 100644 --- a/doc/user/project/merge_requests/getting_started.md +++ b/doc/user/project/merge_requests/getting_started.md @@ -194,6 +194,33 @@ is set for deletion, the merge request widget displays the ![Delete source branch status](img/remove_source_branch_status.png) +### Branch retargeting on merge **(FREE SELF)** + +> - [Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/320902) in GitLab 13.9. +> - It's [deployed behind a feature flag](../../feature_flags.md), disabled by default. +> - It's disabled on GitLab.com. +> - It's not recommended for production use. +> - To use it in GitLab self-managed instances, ask a GitLab administrator to [enable it](#enable-or-disable-branch-retargeting-on-merge). + +In specific circumstances, GitLab can retarget the destination branch of +open merge request, if the destination branch merges while the merge request is +open. Merge requests are often chained in this manner, with one merge request +depending on another: + +- **Merge request 1**: merge `feature-alpha` into `master`. +- **Merge request 2**: merge `feature-beta` into `feature-alpha`. + +These merge requests are usually handled in one of these ways: + +- Merge request 1 is merged into `master` first. Merge request 2 is then + retargeted to `master`. +- Merge request 2 is merged into `feature-alpha`. The updated merge request 1, which + now contains the contents of `feature-alpha` and `feature-beta`, is merged into `master`. + +GitLab retargets up to four merge requests when their target branch is merged into +`master`, so you don't need to perform this operation manually. Merge requests from +forks are not retargeted. + ## Recommendations and best practices for Merge Requests - When working locally in your branch, add multiple commits and only push when @@ -230,3 +257,22 @@ Feature.disable(:reviewer_approval_rules) # For a single project Feature.disable(:reviewer_approval_rules, Project.find()) ``` + +### Enable or disable branch retargeting on merge **(FREE SELF)** + +Automatically retargeting merge requests is under development but ready for production use. +It is deployed behind a feature flag that is **enabled by default**. +[GitLab administrators with access to the GitLab Rails console](../../../administration/feature_flags.md) +can opt to disable it. + +To enable it: + +```ruby +Feature.enable(:retarget_merge_requests) +``` + +To disable it: + +```ruby +Feature.disable(:retarget_merge_requests) +``` diff --git a/ee/app/services/ee/merge_requests/update_service.rb b/ee/app/services/ee/merge_requests/update_service.rb index 6635c899768ec9925dd06e4c5f4beb335798f800..4029bb8c9947d402009eca7e55ee2ace4cb03c3b 100644 --- a/ee/app/services/ee/merge_requests/update_service.rb +++ b/ee/app/services/ee/merge_requests/update_service.rb @@ -50,7 +50,7 @@ def after_update(merge_request) end override :create_branch_change_note - def create_branch_change_note(merge_request, branch_type, old_branch, new_branch) + def create_branch_change_note(merge_request, branch_type, event_type, old_branch, new_branch) super reset_approvals(merge_request) diff --git a/spec/services/merge_requests/post_merge_service_spec.rb b/spec/services/merge_requests/post_merge_service_spec.rb index 6523b5a158c73812d17bf250abf911330985cb60..71329905558bf5906a5c211be20f90f899049773 100644 --- a/spec/services/merge_requests/post_merge_service_spec.rb +++ b/spec/services/merge_requests/post_merge_service_spec.rb @@ -3,9 +3,11 @@ require 'spec_helper' RSpec.describe MergeRequests::PostMergeService do - let(:user) { create(:user) } - let(:merge_request) { create(:merge_request, assignees: [user]) } - let(:project) { merge_request.project } + include ProjectForksHelper + + let_it_be(:user) { create(:user) } + let_it_be(:merge_request, reload: true) { create(:merge_request, assignees: [user]) } + let_it_be(:project) { merge_request.project } subject { described_class.new(project, user).execute(merge_request) } @@ -128,5 +130,139 @@ expect(deploy_job.reload.canceled?).to be false end end + + context 'for a merge request chain' do + before do + ::MergeRequests::UpdateService + .new(project, user, force_remove_source_branch: '1') + .execute(merge_request) + end + + context 'when there is another MR' do + let!(:another_merge_request) do + create(:merge_request, + source_project: source_project, + source_branch: 'my-awesome-feature', + target_project: merge_request.source_project, + target_branch: merge_request.source_branch + ) + end + + shared_examples 'does not retarget merge request' do + it 'another merge request is unchanged' do + expect { subject }.not_to change { another_merge_request.reload.target_branch } + .from(merge_request.source_branch) + end + end + + shared_examples 'retargets merge request' do + it 'another merge request is retargeted' do + expect(SystemNoteService) + .to receive(:change_branch).once + .with(another_merge_request, another_merge_request.project, user, + 'target', 'delete', + merge_request.source_branch, merge_request.target_branch) + + expect { subject }.to change { another_merge_request.reload.target_branch } + .from(merge_request.source_branch) + .to(merge_request.target_branch) + end + + context 'when FF retarget_merge_requests is disabled' do + before do + stub_feature_flags(retarget_merge_requests: false) + end + + include_examples 'does not retarget merge request' + end + + context 'when source branch is to be kept' do + before do + ::MergeRequests::UpdateService + .new(project, user, force_remove_source_branch: false) + .execute(merge_request) + end + + include_examples 'does not retarget merge request' + end + end + + context 'in the same project' do + let(:source_project) { project } + + it_behaves_like 'retargets merge request' + + context 'and is closed' do + before do + another_merge_request.close + end + + it_behaves_like 'does not retarget merge request' + end + + context 'and is merged' do + before do + another_merge_request.mark_as_merged + end + + it_behaves_like 'does not retarget merge request' + end + end + + context 'in forked project' do + let!(:source_project) { fork_project(project) } + + context 'when user has access to source project' do + before do + source_project.add_developer(user) + end + + it_behaves_like 'retargets merge request' + end + + context 'when user does not have access to source project' do + it_behaves_like 'does not retarget merge request' + end + end + + context 'and current and another MR is from a fork' do + let(:project) { create(:project) } + let(:source_project) { fork_project(project) } + + let(:merge_request) do + create(:merge_request, + source_project: source_project, + target_project: project + ) + end + + before do + source_project.add_developer(user) + end + + it_behaves_like 'does not retarget merge request' + end + end + + context 'when many merge requests are to be retargeted' do + let!(:many_merge_requests) do + create_list(:merge_request, 10, :unique_branches, + source_project: merge_request.source_project, + target_project: merge_request.source_project, + target_branch: merge_request.source_branch + ) + end + + it 'retargets only 4 of them' do + subject + + expect(many_merge_requests.each(&:reload).pluck(:target_branch).tally) + .to eq( + merge_request.source_branch => 6, + merge_request.target_branch => 4 + ) + 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 3ccf02fcdfba9b77c3c10a3db6d3d91335f70170..747ecbf4fa43f12337e0ae483abc161abe0602e6 100644 --- a/spec/services/merge_requests/refresh_service_spec.rb +++ b/spec/services/merge_requests/refresh_service_spec.rb @@ -633,31 +633,37 @@ def refresh end context 'merge request metrics' do - let(:issue) { create :issue, project: @project } - let(:commit_author) { create :user } + let(:user) { create(:user) } + let(:project) { create(:project, :repository) } + let(:issue) { create(:issue, project: project) } let(:commit) { project.commit } before do - project.add_developer(commit_author) project.add_developer(user) allow(commit).to receive_messages( safe_message: "Closes #{issue.to_reference}", references: [issue], - author_name: commit_author.name, - author_email: commit_author.email, + author_name: user.name, + author_email: user.email, committed_date: Time.current ) - - allow_any_instance_of(MergeRequest).to receive(:commits).and_return(CommitCollection.new(@project, [commit], 'feature')) end context 'when the merge request is sourced from the same project' do it 'creates a `MergeRequestsClosingIssues` record for each issue closed by a commit' do - merge_request = create(:merge_request, target_branch: 'master', source_branch: 'feature', source_project: @project) - refresh_service = service.new(@project, @user) + allow_any_instance_of(MergeRequest).to receive(:commits).and_return( + CommitCollection.new(project, [commit], 'close-by-commit') + ) + + merge_request = create(:merge_request, + target_branch: 'master', + source_branch: 'close-by-commit', + source_project: project) + + refresh_service = service.new(project, user) allow(refresh_service).to receive(:execute_hooks) - refresh_service.execute(@oldrev, @newrev, 'refs/heads/feature') + refresh_service.execute(@oldrev, @newrev, 'refs/heads/close-by-commit') issue_ids = MergeRequestsClosingIssues.where(merge_request: merge_request).pluck(:issue_id) expect(issue_ids).to eq([issue.id]) @@ -666,16 +672,21 @@ def refresh context 'when the merge request is sourced from a different project' do it 'creates a `MergeRequestsClosingIssues` record for each issue closed by a commit' do - forked_project = fork_project(@project, @user, repository: true) + forked_project = fork_project(project, user, repository: true) + + allow_any_instance_of(MergeRequest).to receive(:commits).and_return( + CommitCollection.new(forked_project, [commit], 'close-by-commit') + ) merge_request = create(:merge_request, target_branch: 'master', - source_branch: 'feature', - target_project: @project, + target_project: project, + source_branch: 'close-by-commit', source_project: forked_project) - refresh_service = service.new(@project, @user) + + refresh_service = service.new(forked_project, user) allow(refresh_service).to receive(:execute_hooks) - refresh_service.execute(@oldrev, @newrev, 'refs/heads/feature') + refresh_service.execute(@oldrev, @newrev, 'refs/heads/close-by-commit') issue_ids = MergeRequestsClosingIssues.where(merge_request: merge_request).pluck(:issue_id) expect(issue_ids).to eq([issue.id]) diff --git a/spec/services/merge_requests/update_service_spec.rb b/spec/services/merge_requests/update_service_spec.rb index dff0d3297b36e984afc9e5cbe235cb6132f2f5fd..edb958406044cc9dc580175d2919ddc737a71884 100644 --- a/spec/services/merge_requests/update_service_spec.rb +++ b/spec/services/merge_requests/update_service_spec.rb @@ -913,6 +913,33 @@ def update_merge_request(opts) end end + context 'updating `target_branch`' do + let(:merge_request) do + create(:merge_request, + source_project: project, + source_branch: 'mr-b', + target_branch: 'mr-a') + end + + it 'updates to master' do + expect(SystemNoteService).to receive(:change_branch).with( + merge_request, project, user, 'target', 'update', 'mr-a', 'master' + ) + + expect { update_merge_request(target_branch: 'master') } + .to change { merge_request.reload.target_branch }.from('mr-a').to('master') + end + + it 'updates to master because of branch deletion' do + expect(SystemNoteService).to receive(:change_branch).with( + merge_request, project, user, 'target', 'delete', 'mr-a', 'master' + ) + + expect { update_merge_request(target_branch: 'master', target_branch_was_deleted: true) } + .to change { merge_request.reload.target_branch }.from('mr-a').to('master') + end + end + it_behaves_like 'issuable record that supports quick actions' do let(:existing_merge_request) { create(:merge_request, source_project: project) } let(:issuable) { described_class.new(project, user, params).execute(existing_merge_request) } diff --git a/spec/services/system_note_service_spec.rb b/spec/services/system_note_service_spec.rb index 9c35f9e3817023b6facfbc5ff16702f8f494a319..df4880dfa1321f5e52eb317c0113773004f3c02a 100644 --- a/spec/services/system_note_service_spec.rb +++ b/spec/services/system_note_service_spec.rb @@ -213,15 +213,16 @@ describe '.change_branch' do it 'calls MergeRequestsService' do - old_branch = double - new_branch = double - branch_type = double + old_branch = double('old_branch') + new_branch = double('new_branch') + branch_type = double('branch_type') + event_type = double('event_type') expect_next_instance_of(::SystemNotes::MergeRequestsService) do |service| - expect(service).to receive(:change_branch).with(branch_type, old_branch, new_branch) + expect(service).to receive(:change_branch).with(branch_type, event_type, old_branch, new_branch) end - described_class.change_branch(noteable, project, author, branch_type, old_branch, new_branch) + described_class.change_branch(noteable, project, author, branch_type, event_type, old_branch, new_branch) end end diff --git a/spec/services/system_notes/merge_requests_service_spec.rb b/spec/services/system_notes/merge_requests_service_spec.rb index 50d16231e8f7746404f20b0db137ff458ead4212..2131f3d3bdffffe4773214271fc120ef03c10609 100644 --- a/spec/services/system_notes/merge_requests_service_spec.rb +++ b/spec/services/system_notes/merge_requests_service_spec.rb @@ -167,18 +167,38 @@ def reloaded_merge_request end describe '.change_branch' do - subject { service.change_branch('target', old_branch, new_branch) } - let(:old_branch) { 'old_branch'} let(:new_branch) { 'new_branch'} it_behaves_like 'a system note' do let(:action) { 'branch' } + + subject { service.change_branch('target', 'update', old_branch, new_branch) } end context 'when target branch name changed' do - it 'sets the note text' do - expect(subject.note).to eq "changed target branch from `#{old_branch}` to `#{new_branch}`" + context 'on update' do + subject { service.change_branch('target', 'update', old_branch, new_branch) } + + it 'sets the note text' do + expect(subject.note).to eq "changed target branch from `#{old_branch}` to `#{new_branch}`" + end + end + + context 'on delete' do + subject { service.change_branch('target', 'delete', old_branch, new_branch) } + + it 'sets the note text' do + expect(subject.note).to eq "changed automatically target branch to `#{new_branch}` because `#{old_branch}` was deleted" + end + end + + context 'for invalid event_type' do + subject { service.change_branch('target', 'invalid', old_branch, new_branch) } + + it 'raises exception' do + expect { subject }.to raise_error /invalid value for event_type/ + end end end end