Merge branch 'gitaly-remotes' into 'master'

Incorporate Gitaly's RemoteService RPCs

Closes gitaly#852

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

8 files changed, 162 insertions, 44 deletions

diff --git a/GITALY_SERVER_VERSION b/GITALY_SERVER_VERSION
index 7e750b4ebf3..d4f16f06004 100644
--- a/GITALY_SERVER_VERSION
+++ b/GITALY_SERVER_VERSION
@@ -1 +1 @@
-0.60.0
+0.64.0
diff --git a/Gemfile b/Gemfile
index db86c86428c..4cd5919e135 100644
--- a/Gemfile
+++ b/Gemfile
@@ -402,7 +402,7 @@ group :ed25519 do
 end
 
 # Gitaly GRPC client
-gem 'gitaly-proto', '~> 0.61.0', require: 'gitaly'
+gem 'gitaly-proto', '~> 0.62.0', require: 'gitaly'
 
 gem 'toml-rb', '~> 0.3.15', require: false
 
diff --git a/Gemfile.lock b/Gemfile.lock
index b11b438a29c..0b714b6fe5d 100644
--- a/Gemfile.lock
+++ b/Gemfile.lock
@@ -284,7 +284,7 @@ GEM
       po_to_json (>= 1.0.0)
       rails (>= 3.2.0)
     gherkin-ruby (0.3.2)
-    gitaly-proto (0.61.0)
+    gitaly-proto (0.62.0)
       google-protobuf (~> 3.1)
       grpc (~> 1.0)
     github-linguist (4.7.6)
@@ -1046,7 +1046,7 @@ DEPENDENCIES
   gettext (~> 3.2.2)
   gettext_i18n_rails (~> 1.8.0)
   gettext_i18n_rails_js (~> 1.2.0)
-  gitaly-proto (~> 0.61.0)
+  gitaly-proto (~> 0.62.0)
   github-linguist (~> 4.7.0)
   gitlab-flowdock-git-hook (~> 1.0.1)
   gitlab-markup (~> 1.6.2)
diff --git a/app/models/repository.rb b/app/models/repository.rb
index a34f5e5439b..b1fd981965c 100644
--- a/app/models/repository.rb
+++ b/app/models/repository.rb
@@ -1010,10 +1010,6 @@ class Repository
     raw_repository.fetch_source_branch!(source_repository.raw_repository, source_branch, local_ref)
   end
 
-  def remote_exists?(name)
-    raw_repository.remote_exists?(name)
-  end
-
   def compare_source_branch(target_branch_name, source_repository, source_branch_name, straight:)
     raw_repository.compare_source_branch(target_branch_name, source_repository.raw_repository, source_branch_name, straight: straight)
   end
diff --git a/lib/gitlab/git/repository.rb b/lib/gitlab/git/repository.rb
index 603323d0452..36dc6b820ce 100644
--- a/lib/gitlab/git/repository.rb
+++ b/lib/gitlab/git/repository.rb
@@ -919,31 +919,23 @@ module Gitlab
 
       # If `mirror_refmap` is present the remote is set as mirror with that mapping
       def add_remote(remote_name, url, mirror_refmap: nil)
-        rugged.remotes.create(remote_name, url)
-
-        set_remote_as_mirror(remote_name, refmap: mirror_refmap) if mirror_refmap
-      rescue Rugged::ConfigError
-        remote_update(remote_name, url: url)
+        gitaly_migrate(:operation_user_add_tag) do |is_enabled|
+          if is_enabled
+            gitaly_remote_client.add_remote(remote_name, url, mirror_refmap)
+          else
+            rugged_add_remote(remote_name, url, mirror_refmap)
+          end
+        end
       end
 
       def 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
-        # [remote_name]/) to the top level namespace. We clean the mapping so
-        # those don't get deleted.
-        if rugged.config["remote.#{remote_name}.mirror"]
-          rugged.config.delete("remote.#{remote_name}.fetch")
+        gitaly_migrate(:operation_user_add_tag) do |is_enabled|
+          if is_enabled
+            gitaly_remote_client.remove_remote(remote_name)
+          else
+            rugged_remove_remote(remote_name)
+          end
         end
-
-        rugged.remotes.delete(remote_name)
-        true
-      rescue Rugged::ConfigError
-        false
-      end
-
-      # Returns true if a remote exists.
-      def remote_exists?(name)
-        rugged.remotes[name].present?
       end
 
       # Update the specified remote using the values in the +options+ hash
@@ -1298,6 +1290,10 @@ module Gitlab
         @gitaly_operation_client ||= Gitlab::GitalyClient::OperationService.new(self)
       end
 
+      def gitaly_remote_client
+        @gitaly_remote_client ||= Gitlab::GitalyClient::RemoteService.new(self)
+      end
+
       def gitaly_migrate(method, status: Gitlab::GitalyClient::MigrationStatus::OPT_IN, &block)
         Gitlab::GitalyClient.migrate(method, status: status, &block)
       rescue GRPC::NotFound => e
@@ -1917,6 +1913,29 @@ module Gitlab
         raise ArgumentError, 'Invalid merge source'
       end
 
+      def rugged_add_remote(remote_name, url, mirror_refmap)
+        rugged.remotes.create(remote_name, url)
+
+        set_remote_as_mirror(remote_name, refmap: mirror_refmap) if mirror_refmap
+      rescue Rugged::ConfigError
+        remote_update(remote_name, url: url)
+      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
+        # [remote_name]/) to the top level namespace. We clean the mapping so
+        # those don't get deleted.
+        if rugged.config["remote.#{remote_name}.mirror"]
+          rugged.config.delete("remote.#{remote_name}.fetch")
+        end
+
+        rugged.remotes.delete(remote_name)
+        true
+      rescue Rugged::ConfigError
+        false
+      end
+
       def fetch_remote(remote_name = 'origin', env: nil)
         run_git(['fetch', remote_name], env: env).last.zero?
       end
diff --git a/lib/gitlab/gitaly_client/remote_service.rb b/lib/gitlab/gitaly_client/remote_service.rb
new file mode 100644
index 00000000000..9218f6cfd68
--- /dev/null
+++ b/lib/gitlab/gitaly_client/remote_service.rb
@@ -0,0 +1,28 @@
+module Gitlab
+  module GitalyClient
+    class RemoteService
+      def initialize(repository)
+        @repository = repository
+        @gitaly_repo = repository.gitaly_repository
+        @storage = repository.storage
+      end
+
+      def add_remote(name, url, mirror_refmap)
+        request = Gitaly::AddRemoteRequest.new(
+          repository: @gitaly_repo, name: name, url: url,
+          mirror_refmap: mirror_refmap.to_s
+        )
+
+        GitalyClient.call(@storage, :remote_service, :add_remote, request)
+      end
+
+      def remove_remote(name)
+        request = Gitaly::RemoveRemoteRequest.new(repository: @gitaly_repo, name: name)
+
+        response = GitalyClient.call(@storage, :remote_service, :remove_remote, request)
+
+        response.result
+      end
+    end
+  end
+end
diff --git a/spec/lib/gitlab/git/repository_spec.rb b/spec/lib/gitlab/git/repository_spec.rb
index d20a2ca935f..0e4292026df 100644
--- a/spec/lib/gitlab/git/repository_spec.rb
+++ b/spec/lib/gitlab/git/repository_spec.rb
@@ -701,21 +701,6 @@ describe Gitlab::Git::Repository, seed_helper: true do
     end
   end
 
-  describe '#remote_exists?' do
-    before(:all) do
-      @repo = Gitlab::Git::Repository.new('default', TEST_MUTABLE_REPO_PATH, '')
-      @repo.add_remote("new_remote", SeedHelper::GITLAB_GIT_TEST_REPO_URL)
-    end
-
-    it 'returns true for an existing remote' do
-      expect(@repo.remote_exists?('new_remote')).to eq(true)
-    end
-
-    it 'returns false for a non-existing remote' do
-      expect(@repo.remote_exists?('foo')).to eq(false)
-    end
-  end
-
   describe "#log" do
     let(:commit_with_old_name) do
       Gitlab::Git::Commit.decorate(repository, @commit_with_old_name_id)
@@ -1843,6 +1828,62 @@ describe Gitlab::Git::Repository, seed_helper: true do
     end
   end
 
+  describe 'remotes' do
+    let(:repository) do
+      Gitlab::Git::Repository.new('default', TEST_MUTABLE_REPO_PATH, '')
+    end
+    let(:remote_name) { 'my-remote' }
+
+    after do
+      ensure_seeds
+    end
+
+    describe '#add_remote' do
+      let(:url) { 'http://my-repo.git' }
+      let(:mirror_refmap) { '+refs/*:refs/*' }
+
+      it 'creates a new remote via Gitaly' do
+        expect_any_instance_of(Gitlab::GitalyClient::RemoteService)
+          .to receive(:add_remote).with(remote_name, url, mirror_refmap)
+
+        repository.add_remote(remote_name, url, mirror_refmap: mirror_refmap)
+      end
+
+      context 'with Gitaly disabled', :skip_gitaly_mock do
+        it 'creates a new remote via Rugged' do
+          expect_any_instance_of(Rugged::RemoteCollection).to receive(:create)
+            .with(remote_name, url)
+          expect_any_instance_of(Rugged::Config).to receive(:[]=)
+          .with("remote.#{remote_name}.mirror", true)
+          expect_any_instance_of(Rugged::Config).to receive(:[]=)
+          .with("remote.#{remote_name}.prune", true)
+          expect_any_instance_of(Rugged::Config).to receive(:[]=)
+            .with("remote.#{remote_name}.fetch", mirror_refmap)
+
+          repository.add_remote(remote_name, url, mirror_refmap: mirror_refmap)
+        end
+      end
+    end
+
+    describe '#remove_remote' do
+      it 'removes the remote via Gitaly' do
+        expect_any_instance_of(Gitlab::GitalyClient::RemoteService)
+          .to receive(:remove_remote).with(remote_name)
+
+        repository.remove_remote(remote_name)
+      end
+
+      context 'with Gitaly disabled', :skip_gitaly_mock do
+        it 'removes the remote via Rugged' do
+          expect_any_instance_of(Rugged::RemoteCollection).to receive(:delete)
+            .with(remote_name)
+
+          repository.remove_remote(remote_name)
+        end
+      end
+    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
diff --git a/spec/lib/gitlab/gitaly_client/remote_service_spec.rb b/spec/lib/gitlab/gitaly_client/remote_service_spec.rb
new file mode 100644
index 00000000000..69c6f054016
--- /dev/null
+++ b/spec/lib/gitlab/gitaly_client/remote_service_spec.rb
@@ -0,0 +1,34 @@
+require 'spec_helper'
+
+describe Gitlab::GitalyClient::RemoteService do
+  let(:project) { create(:project) }
+  let(:storage_name) { project.repository_storage }
+  let(:relative_path) { project.disk_path + '.git' }
+  let(:remote_name) { 'my-remote' }
+  let(:client) { described_class.new(project.repository) }
+
+  describe '#add_remote' do
+    let(:url) { 'http://my-repo.git' }
+    let(:mirror_refmap) { :all_refs }
+
+    it 'sends an add_remote message' do
+      expect_any_instance_of(Gitaly::RemoteService::Stub)
+        .to receive(:add_remote)
+        .with(gitaly_request_with_path(storage_name, relative_path), kind_of(Hash))
+        .and_return(double(:add_remote_response))
+
+      client.add_remote(remote_name, url, mirror_refmap)
+    end
+  end
+
+  describe '#remove_remote' do
+    it 'sends an remove_remote message and returns the result value' do
+      expect_any_instance_of(Gitaly::RemoteService::Stub)
+        .to receive(:remove_remote)
+        .with(gitaly_request_with_path(storage_name, relative_path), kind_of(Hash))
+        .and_return(double(result: true))
+
+      expect(client.remove_remote(remote_name)).to be(true)
+    end
+  end
+end