Merge branch 'gitaly-delete-branch' into 'master'

Implement OperationService.UserDeleteBranch Gitaly RPC

See merge request gitlab-org/gitlab-ce!14603

.flayignore

@@ -5,3 +5,4 @@ app/policies/project_policy.rb
 app/models/concerns/relative_positioning.rb
 app/workers/stuck_merge_jobs_worker.rb
 lib/gitlab/redis/*.rb
+lib/gitlab/gitaly_client/operation_service.rb

lib/gitlab/git/repository.rb

@@ -676,7 +676,13 @@ module Gitlab
       end
 
       def rm_branch(branch_name, user:)
-        OperationService.new(user, self).rm_branch(find_branch(branch_name))
+        gitaly_migrate(:operation_user_delete_branch) do |is_enabled|
+          if is_enabled
+            gitaly_operations_client.user_delete_branch(branch_name, user)
+          else
+            OperationService.new(user, self).rm_branch(find_branch(branch_name))
+          end
+        end
       end
 
       def rm_tag(tag_name, user:)

lib/gitlab/gitaly_client/operation_service.rb

@@ -60,6 +60,20 @@ module Gitlab
         target_commit = Gitlab::Git::Commit.decorate(@repository, branch.target_commit)
         Gitlab::Git::Branch.new(@repository, branch.name, target_commit.id, target_commit)
       end
+
+      def user_delete_branch(branch_name, user)
+        request = Gitaly::UserDeleteBranchRequest.new(
+          repository: @gitaly_repo,
+          branch_name: GitalyClient.encode(branch_name),
+          user: Util.gitaly_user(user)
+        )
+
+        response = GitalyClient.call(@repository.storage, :operation_service, :user_delete_branch, request)
+
+        if pre_receive_error = response.pre_receive_error.presence
+          raise Gitlab::Git::HooksService::PreReceiveError, pre_receive_error
+        end
+      end
     end
   end
 end

spec/lib/gitlab/git/repository_spec.rb

@@ -1444,6 +1444,34 @@ describe Gitlab::Git::Repository, seed_helper: true do
     end
   end
 
+  describe '#rm_branch' do
+    shared_examples "user deleting a branch" do
+      let(:project) { create(:project, :repository) }
+      let(:repository) { project.repository.raw }
+      let(:user) { create(:user) }
+      let(:branch_name) { "to-be-deleted-soon" }
+
+      before do
+        project.team << [user, :developer]
+        repository.create_branch(branch_name)
+      end
+
+      it "removes the branch from the repo" do
+        repository.rm_branch(branch_name, user: user)
+
+        expect(repository.rugged.branches[branch_name]).to be_nil
+      end
+    end
+
+    context "when Gitaly user_delete_branch is enabled" do
+      it_behaves_like "user deleting a branch"
+    end
+
+    context "when Gitaly user_delete_branch is disabled", skip_gitaly_mock: true do
+      it_behaves_like "user deleting a branch"
+    end
+  end
+
   def create_remote_branch(repository, remote_name, branch_name, source_branch_name)
     source_branch = repository.branches.find { |branch| branch.name == source_branch_name }
     rugged = repository.rugged

spec/lib/gitlab/gitaly_client/operation_service_spec.rb

@@ -4,10 +4,10 @@ describe Gitlab::GitalyClient::OperationService do
   let(:project) { create(:project) }
   let(:repository) { project.repository.raw }
   let(:client) { described_class.new(repository) }
+  let(:user) { create(:user) }
+  let(:gitaly_user) { Gitlab::GitalyClient::Util.gitaly_user(user) }
 
   describe '#user_create_branch' do
-    let(:user) { create(:user) }
-    let(:gitaly_user) { Gitlab::GitalyClient::Util.gitaly_user(user) }
     let(:branch_name) { 'new' }
     let(:start_point) { 'master' }
     let(:request) do
@@ -52,4 +52,41 @@ describe Gitlab::GitalyClient::OperationService do
       end
     end
   end
+
+  describe '#user_delete_branch' do
+    let(:branch_name) { 'my-branch' }
+    let(:request) do
+      Gitaly::UserDeleteBranchRequest.new(
+        repository: repository.gitaly_repository,
+        branch_name: branch_name,
+        user: gitaly_user
+      )
+    end
+    let(:response) { Gitaly::UserDeleteBranchResponse.new }
+
+    subject { client.user_delete_branch(branch_name, user) }
+
+    it 'sends a user_delete_branch message' do
+      expect_any_instance_of(Gitaly::OperationService::Stub)
+        .to receive(:user_delete_branch).with(request, kind_of(Hash))
+        .and_return(response)
+
+      subject
+    end
+
+    context "when pre_receive_error is present" do
+      let(:response) do
+        Gitaly::UserDeleteBranchResponse.new(pre_receive_error: "something failed")
+      end
+
+      it "throws a PreReceive exception" do
+        expect_any_instance_of(Gitaly::OperationService::Stub)
+          .to receive(:user_delete_branch).with(request, kind_of(Hash))
+          .and_return(response)
+
+        expect { subject }.to raise_error(
+          Gitlab::Git::HooksService::PreReceiveError, "something failed")
+      end
+    end
+  end
 end

spec/models/repository_spec.rb

@@ -901,47 +901,6 @@ describe Repository do
     end
   end
 
-  describe '#rm_branch' do
-    let(:old_rev) { '0b4bc9a49b562e85de7cc9e834518ea6828729b9' } # git rev-parse feature
-    let(:blank_sha) { '0000000000000000000000000000000000000000' }
-
-    context 'when pre hooks were successful' do
-      it 'runs without errors' do
-        expect_any_instance_of(Gitlab::Git::HooksService).to receive(:execute)
-          .with(git_user, repository.raw_repository, old_rev, blank_sha, 'refs/heads/feature')
-
-        expect { repository.rm_branch(user, 'feature') }.not_to raise_error
-      end
-
-      it 'deletes the branch' do
-        allow_any_instance_of(Gitlab::Git::Hook).to receive(:trigger).and_return([true, nil])
-
-        expect { repository.rm_branch(user, 'feature') }.not_to raise_error
-
-        expect(repository.find_branch('feature')).to be_nil
-      end
-    end
-
-    context 'when pre hooks failed' do
-      it 'gets an error' do
-        allow_any_instance_of(Gitlab::Git::Hook).to receive(:trigger).and_return([false, ''])
-
-        expect do
-          repository.rm_branch(user, 'feature')
-        end.to raise_error(Gitlab::Git::HooksService::PreReceiveError)
-      end
-
-      it 'does not delete the branch' do
-        allow_any_instance_of(Gitlab::Git::Hook).to receive(:trigger).and_return([false, ''])
-
-        expect do
-          repository.rm_branch(user, 'feature')
-        end.to raise_error(Gitlab::Git::HooksService::PreReceiveError)
-        expect(repository.find_branch('feature')).not_to be_nil
-      end
-    end
-  end
-
   describe '#update_branch_with_hooks' do
     let(:old_rev) { '0b4bc9a49b562e85de7cc9e834518ea6828729b9' } # git rev-parse feature
     let(:new_rev) { 'a74ae73c1ccde9b974a70e82b901588071dc142a' } # commit whose parent is old_rev
@@ -1744,13 +1703,75 @@ describe Repository do
   end
 
   describe '#rm_branch' do
-    let(:user) { create(:user) }
+    shared_examples "user deleting a branch" do
+      it 'removes a branch' do
+        expect(repository).to receive(:before_remove_branch)
+        expect(repository).to receive(:after_remove_branch)
+
+        repository.rm_branch(user, 'feature')
+      end
+    end
 
-    it 'removes a branch' do
-      expect(repository).to receive(:before_remove_branch)
-      expect(repository).to receive(:after_remove_branch)
+    context 'with gitaly enabled' do
+      it_behaves_like "user deleting a branch"
 
-      repository.rm_branch(user, 'feature')
+      context 'when pre hooks failed' do
+        before do
+          allow_any_instance_of(Gitlab::GitalyClient::OperationService)
+            .to receive(:user_delete_branch).and_raise(Gitlab::Git::HooksService::PreReceiveError)
+        end
+
+        it 'gets an error and does not delete the branch' do
+          expect do
+            repository.rm_branch(user, 'feature')
+          end.to raise_error(Gitlab::Git::HooksService::PreReceiveError)
+
+          expect(repository.find_branch('feature')).not_to be_nil
+        end
+      end
+    end
+
+    context 'with gitaly disabled', skip_gitaly_mock: true do
+      it_behaves_like "user deleting a branch"
+
+      let(:old_rev) { '0b4bc9a49b562e85de7cc9e834518ea6828729b9' } # git rev-parse feature
+      let(:blank_sha) { '0000000000000000000000000000000000000000' }
+
+      context 'when pre hooks were successful' do
+        it 'runs without errors' do
+          expect_any_instance_of(Gitlab::Git::HooksService).to receive(:execute)
+            .with(git_user, repository.raw_repository, old_rev, blank_sha, 'refs/heads/feature')
+
+          expect { repository.rm_branch(user, 'feature') }.not_to raise_error
+        end
+
+        it 'deletes the branch' do
+          allow_any_instance_of(Gitlab::Git::Hook).to receive(:trigger).and_return([true, nil])
+
+          expect { repository.rm_branch(user, 'feature') }.not_to raise_error
+
+          expect(repository.find_branch('feature')).to be_nil
+        end
+      end
+
+      context 'when pre hooks failed' do
+        it 'gets an error' do
+          allow_any_instance_of(Gitlab::Git::Hook).to receive(:trigger).and_return([false, ''])
+
+          expect do
+            repository.rm_branch(user, 'feature')
+          end.to raise_error(Gitlab::Git::HooksService::PreReceiveError)
+        end
+
+        it 'does not delete the branch' do
+          allow_any_instance_of(Gitlab::Git::Hook).to receive(:trigger).and_return([false, ''])
+
+          expect do
+            repository.rm_branch(user, 'feature')
+          end.to raise_error(Gitlab::Git::HooksService::PreReceiveError)
+          expect(repository.find_branch('feature')).not_to be_nil
+        end
+      end
     end
   end