From 11fd2f80b0500736a9df0befbd371bfec9549138 Mon Sep 17 00:00:00 2001 From: Stan Hu Date: Sun, 16 Apr 2017 20:51:16 -0700 Subject: Don't delete a branch involved in an open merge request in "Delete all merged branches" service Customers were surprised by the previous behavior, which destroyed branches even though an open merge request existed for it. Closes #29427 --- app/services/delete_merged_branches_service.rb | 11 +++++++++++ changelogs/unreleased/sh-issue-29247-fix.yml | 5 +++++ spec/factories/merge_requests.rb | 4 ++++ spec/services/delete_merged_branches_service_spec.rb | 13 +++++++++++++ 4 files changed, 33 insertions(+) create mode 100644 changelogs/unreleased/sh-issue-29247-fix.yml diff --git a/app/services/delete_merged_branches_service.rb b/app/services/delete_merged_branches_service.rb index 1b5623baebe..3b611588466 100644 --- a/app/services/delete_merged_branches_service.rb +++ b/app/services/delete_merged_branches_service.rb @@ -8,9 +8,20 @@ class DeleteMergedBranchesService < BaseService branches = project.repository.branch_names branches = branches.select { |branch| project.repository.merged_to_root_ref?(branch) } + # Prevent deletion of branches relevant to open merge requests + branches -= merge_request_branch_names branches.each do |branch| DeleteBranchService.new(project, current_user).execute(branch) end end + + private + + def merge_request_branch_names + # reorder(nil) is necessary for SELECT DISTINCT because default scope adds an ORDER BY + source_names = project.origin_merge_requests.opened.reorder(nil).uniq.pluck(:source_branch) + target_names = project.merge_requests.opened.reorder(nil).uniq.pluck(:target_branch) + (source_names + target_names).uniq + end end diff --git a/changelogs/unreleased/sh-issue-29247-fix.yml b/changelogs/unreleased/sh-issue-29247-fix.yml new file mode 100644 index 00000000000..b530e5da55b --- /dev/null +++ b/changelogs/unreleased/sh-issue-29247-fix.yml @@ -0,0 +1,5 @@ +--- +title: Don't delete a branch involved in an open merge request in "Delete all merged + branches" service +merge_request: +author: diff --git a/spec/factories/merge_requests.rb b/spec/factories/merge_requests.rb index 361f9dac191..253a025af48 100644 --- a/spec/factories/merge_requests.rb +++ b/spec/factories/merge_requests.rb @@ -40,6 +40,10 @@ FactoryGirl.define do state :closed end + trait :opened do + state :opened + end + trait :reopened do state :reopened end diff --git a/spec/services/delete_merged_branches_service_spec.rb b/spec/services/delete_merged_branches_service_spec.rb index a41a421fa6e..7b921f606f8 100644 --- a/spec/services/delete_merged_branches_service_spec.rb +++ b/spec/services/delete_merged_branches_service_spec.rb @@ -42,6 +42,19 @@ describe DeleteMergedBranchesService, services: true do expect { described_class.new(project, user).execute }.to raise_error(Gitlab::Access::AccessDeniedError) end end + + context 'open merge requests' do + it 'does not delete branches from open merge requests' do + fork_link = create(:forked_project_link, forked_from_project: project) + create(:merge_request, :reopened, source_project: project, target_project: project, source_branch: 'branch-merged', target_branch: 'master') + create(:merge_request, :opened, source_project: fork_link.forked_to_project, target_project: project, target_branch: 'improve/awesome', source_branch: 'master') + + service.execute + + expect(project.repository.branch_names).to include('branch-merged') + expect(project.repository.branch_names).to include('improve/awesome') + end + end end context '#async_execute' do -- cgit v1.2.1