Merge branch 'feature/migrate-delete-refs-to-gitaly' into 'master'

Migrate Git::Repository#delete_refs to Gitaly

Closes gitaly#967

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

7 files changed, 59 insertions, 38 deletions

diff --git a/GITALY_SERVER_VERSION b/GITALY_SERVER_VERSION
index c52842c467c..62df9f538d8 100644
--- a/GITALY_SERVER_VERSION
+++ b/GITALY_SERVER_VERSION
@@ -1 +1 @@
-0.75.0
+0.76.0
diff --git a/Gemfile b/Gemfile
index d9110f9b5b2..7bffae06b51 100644
--- a/Gemfile
+++ b/Gemfile
@@ -406,7 +406,7 @@ group :ed25519 do
 end
 
 # Gitaly GRPC client
-gem 'gitaly-proto', '~> 0.82.0', require: 'gitaly'
+gem 'gitaly-proto', '~> 0.83.0', require: 'gitaly'
 
 gem 'toml-rb', '~> 0.3.15', require: false
 
diff --git a/Gemfile.lock b/Gemfile.lock
index a8531f8e436..4558b43af48 100644
--- a/Gemfile.lock
+++ b/Gemfile.lock
@@ -285,7 +285,7 @@ GEM
       po_to_json (>= 1.0.0)
       rails (>= 3.2.0)
     gherkin-ruby (0.3.2)
-    gitaly-proto (0.82.0)
+    gitaly-proto (0.83.0)
       google-protobuf (~> 3.1)
       grpc (~> 1.0)
     github-linguist (4.7.6)
@@ -1056,7 +1056,7 @@ DEPENDENCIES
   gettext (~> 3.2.2)
   gettext_i18n_rails (~> 1.8.0)
   gettext_i18n_rails_js (~> 1.2.0)
-  gitaly-proto (~> 0.82.0)
+  gitaly-proto (~> 0.83.0)
   github-linguist (~> 4.7.0)
   gitlab-flowdock-git-hook (~> 1.0.1)
   gitlab-markup (~> 1.6.2)
diff --git a/lib/gitlab/git/repository.rb b/lib/gitlab/git/repository.rb
index 8614c1bd417..7127f7858ee 100644
--- a/lib/gitlab/git/repository.rb
+++ b/lib/gitlab/git/repository.rb
@@ -888,16 +888,12 @@ module Gitlab
       end
 
       def delete_refs(*ref_names)
-        instructions = ref_names.map do |ref|
-          "delete #{ref}\x00\x00"
-        end
-
-        message, status = run_git(%w[update-ref --stdin -z]) do |stdin|
-          stdin.write(instructions.join)
-        end
-
-        unless status.zero?
-          raise GitError.new("Could not delete refs #{ref_names}: #{message}")
+        gitaly_migrate(:delete_refs) do |is_enabled|
+          if is_enabled
+            gitaly_delete_refs(*ref_names)
+          else
+            git_delete_refs(*ref_names)
+          end
         end
       end
 
@@ -2204,6 +2200,24 @@ module Gitlab
         remote_update(remote_name, url: url)
       end
 
+      def git_delete_refs(*ref_names)
+        instructions = ref_names.map do |ref|
+          "delete #{ref}\x00\x00"
+        end
+
+        message, status = run_git(%w[update-ref --stdin -z]) do |stdin|
+          stdin.write(instructions.join)
+        end
+
+        unless status.zero?
+          raise GitError.new("Could not delete refs #{ref_names}: #{message}")
+        end
+      end
+
+      def gitaly_delete_refs(*ref_names)
+        gitaly_ref_client.delete_refs(refs: ref_names)
+      end
+
       def rugged_remove_remote(remote_name)
         # When a remote is deleted all its remote refs are deleted too, but in
         # the case of mirrors we map its refs (that would usualy go under
diff --git a/lib/gitlab/gitaly_client/ref_service.rb b/lib/gitlab/gitaly_client/ref_service.rb
index f8e2a27f3fe..8b9a224b700 100644
--- a/lib/gitlab/gitaly_client/ref_service.rb
+++ b/lib/gitlab/gitaly_client/ref_service.rb
@@ -133,13 +133,16 @@ module Gitlab
         GitalyClient.call(@repository.storage, :ref_service, :delete_branch, request)
       end
 
-      def delete_refs(except_with_prefixes:)
+      def delete_refs(refs: [], except_with_prefixes: [])
         request = Gitaly::DeleteRefsRequest.new(
           repository: @gitaly_repo,
-          except_with_prefix: except_with_prefixes
+          refs: refs.map { |r| encode_binary(r) },
+          except_with_prefix: except_with_prefixes.map { |r| encode_binary(r) }
         )
 
-        GitalyClient.call(@repository.storage, :ref_service, :delete_refs, request)
+        response = GitalyClient.call(@repository.storage, :ref_service, :delete_refs, request)
+
+        raise Gitlab::Git::Repository::GitError, response.git_error if response.git_error.present?
       end
 
       private
diff --git a/spec/lib/gitlab/git/repository_spec.rb b/spec/lib/gitlab/git/repository_spec.rb
index ac7c0270916..bf01e6ef8e8 100644
--- a/spec/lib/gitlab/git/repository_spec.rb
+++ b/spec/lib/gitlab/git/repository_spec.rb
@@ -562,35 +562,39 @@ describe Gitlab::Git::Repository, seed_helper: true do
   end
 
   describe '#delete_refs' do
-    before(:all) do
-      @repo = Gitlab::Git::Repository.new('default', TEST_MUTABLE_REPO_PATH, '')
-    end
+    shared_examples 'deleting refs' do
+      let(:repo) { Gitlab::Git::Repository.new('default', TEST_MUTABLE_REPO_PATH, '') }
 
-    it 'deletes the ref' do
-      @repo.delete_refs('refs/heads/feature')
+      after do
+        ensure_seeds
+      end
 
-      expect(@repo.rugged.references['refs/heads/feature']).to be_nil
-    end
+      it 'deletes the ref' do
+        repo.delete_refs('refs/heads/feature')
 
-    it 'deletes all refs' do
-      refs = %w[refs/heads/wip refs/tags/v1.1.0]
-      @repo.delete_refs(*refs)
+        expect(repo.rugged.references['refs/heads/feature']).to be_nil
+      end
 
-      refs.each do |ref|
-        expect(@repo.rugged.references[ref]).to be_nil
+      it 'deletes all refs' do
+        refs = %w[refs/heads/wip refs/tags/v1.1.0]
+        repo.delete_refs(*refs)
+
+        refs.each do |ref|
+          expect(repo.rugged.references[ref]).to be_nil
+        end
       end
-    end
 
-    it 'raises an error if it failed' do
-      expect(@repo).to receive(:popen).and_return(['Error', 1])
+      it 'raises an error if it failed' do
+        expect { repo.delete_refs('refs\heads\fix') }.to raise_error(Gitlab::Git::Repository::GitError)
+      end
+    end
 
-      expect do
-        @repo.delete_refs('refs/heads/fix')
-      end.to raise_error(Gitlab::Git::Repository::GitError)
+    context 'when Gitaly delete_refs feature is enabled' do
+      it_behaves_like 'deleting refs'
     end
 
-    after(:all) do
-      ensure_seeds
+    context 'when Gitaly delete_refs feature is disabled', :disable_gitaly do
+      it_behaves_like 'deleting refs'
     end
   end
 
diff --git a/spec/lib/gitlab/gitaly_client/ref_service_spec.rb b/spec/lib/gitlab/gitaly_client/ref_service_spec.rb
index 951e146a30a..257e4c50f2d 100644
--- a/spec/lib/gitlab/gitaly_client/ref_service_spec.rb
+++ b/spec/lib/gitlab/gitaly_client/ref_service_spec.rb
@@ -112,7 +112,7 @@ describe Gitlab::GitalyClient::RefService do
       expect_any_instance_of(Gitaly::RefService::Stub)
         .to receive(:delete_refs)
         .with(gitaly_request_with_params(except_with_prefix: prefixes), kind_of(Hash))
-        .and_return(double('delete_refs_response'))
+        .and_return(double('delete_refs_response', git_error: ""))
 
       client.delete_refs(except_with_prefixes: prefixes)
     end