Update remote happens through Gitaly only

This change makes closes another migration! And remotes tests around it,
mostly due to stubbing this was needed. Also, these are tested on the
Gitaly side too:
- https://gitlab.com/gitlab-org/gitaly/issues/791
-
https://gitlab.com/gitlab-org/gitaly/blob/6dd74543f6af8ebcf22aa10e39da004f388fdda5/internal/service/repository/fetch_remote_test.go

Closes https://gitlab.com/gitlab-org/gitaly/issues/789

2 files changed, 29 insertions, 353 deletions

diff --git a/lib/gitlab/git/remote_mirror.rb b/lib/gitlab/git/remote_mirror.rb
index ebe46722890..e4743b4db0a 100644
--- a/lib/gitlab/git/remote_mirror.rb
+++ b/lib/gitlab/git/remote_mirror.rb
@@ -7,81 +7,8 @@ module Gitlab
       end
 
       def update(only_branches_matching: [])
-        @repository.gitaly_migrate(:remote_update_remote_mirror) do |is_enabled|
-          if is_enabled
-            gitaly_update(only_branches_matching)
-          else
-            rugged_update(only_branches_matching)
-          end
-        end
-      end
-
-      private
-
-      def gitaly_update(only_branches_matching)
-        @repository.gitaly_remote_client.update_remote_mirror(@ref_name, only_branches_matching)
-      end
-
-      def rugged_update(only_branches_matching)
-        local_branches = refs_obj(@repository.local_branches, only_refs_matching: only_branches_matching)
-        remote_branches = refs_obj(@repository.remote_branches(@ref_name), only_refs_matching: only_branches_matching)
-
-        updated_branches = changed_refs(local_branches, remote_branches)
-        push_branches(updated_branches.keys) if updated_branches.present?
-
-        delete_refs(local_branches, remote_branches)
-
-        local_tags = refs_obj(@repository.tags)
-        remote_tags = refs_obj(@repository.remote_tags(@ref_name))
-
-        updated_tags = changed_refs(local_tags, remote_tags)
-        @repository.push_remote_branches(@ref_name, updated_tags.keys) if updated_tags.present?
-
-        delete_refs(local_tags, remote_tags)
-      end
-
-      def refs_obj(refs, only_refs_matching: [])
-        refs.each_with_object({}) do |ref, refs|
-          next if only_refs_matching.present? && !only_refs_matching.include?(ref.name)
-
-          refs[ref.name] = ref
-        end
-      end
-
-      def changed_refs(local_refs, remote_refs)
-        local_refs.select do |ref_name, ref|
-          remote_ref = remote_refs[ref_name]
-
-          remote_ref.nil? || ref.dereferenced_target != remote_ref.dereferenced_target
-        end
-      end
-
-      def push_branches(branches)
-        default_branch, branches = branches.partition do |branch|
-          @repository.root_ref == branch
-        end
-
-        # Push the default branch first so it works fine when remote mirror is empty.
-        branches.unshift(*default_branch)
-
-        @repository.push_remote_branches(@ref_name, branches)
-      end
-
-      def delete_refs(local_refs, remote_refs)
-        refs = refs_to_delete(local_refs, remote_refs)
-
-        @repository.delete_remote_branches(@ref_name, refs.keys) if refs.present?
-      end
-
-      def refs_to_delete(local_refs, remote_refs)
-        default_branch_id = @repository.commit.id
-
-        remote_refs.select do |remote_ref_name, remote_ref|
-          next false if local_refs[remote_ref_name] # skip if branch or tag exist in local repo
-
-          remote_ref_id = remote_ref.dereferenced_target.try(:id)
-
-          remote_ref_id && @repository.rugged_is_ancestor?(remote_ref_id, default_branch_id)
+        @repository.wrapped_gitaly_errors do
+          @repository.gitaly_remote_client.update_remote_mirror(@ref_name, only_branches_matching)
         end
       end
     end
diff --git a/spec/services/projects/update_remote_mirror_service_spec.rb b/spec/services/projects/update_remote_mirror_service_spec.rb
index 723cb374c37..5c2e79ff9af 100644
--- a/spec/services/projects/update_remote_mirror_service_spec.rb
+++ b/spec/services/projects/update_remote_mirror_service_spec.rb
@@ -1,7 +1,8 @@
 require 'spec_helper'
 
 describe Projects::UpdateRemoteMirrorService do
-  let(:project) { create(:project, :repository) }
+  set(:project) { create(:project, :repository) }
+  let(:owner) { project.owner }
   let(:remote_project) { create(:forked_project_with_submodules) }
   let(:repository) { project.repository }
   let(:raw_repository) { repository.raw }
@@ -9,13 +10,11 @@ describe Projects::UpdateRemoteMirrorService do
 
   subject { described_class.new(project, project.creator) }
 
-  describe "#execute", :skip_gitaly_mock do
+  describe "#execute" do
     before do
-      create_branch(repository, 'existing-branch')
-      allow(raw_repository).to receive(:remote_tags) do
-        generate_tags(repository, 'v1.0.0', 'v1.1.0')
-      end
-      allow(raw_repository).to receive(:push_remote_branches).and_return(true)
+      repository.add_branch(owner, 'existing-branch', 'master')
+
+      allow(remote_mirror).to receive(:update_repository).and_return(true)
     end
 
     it "fetches the remote repository" do
@@ -34,307 +33,57 @@ describe Projects::UpdateRemoteMirrorService do
       expect(result[:status]).to eq(:success)
     end
 
-    describe 'Syncing branches' do
+    context 'when syncing all branches' do
       it "push all the branches the first time" do
         allow(repository).to receive(:fetch_remote)
 
-        expect(raw_repository).to receive(:push_remote_branches).with(remote_mirror.remote_name, local_branch_names)
-
-        subject.execute(remote_mirror)
-      end
-
-      it "does not push anything is remote is up to date" do
-        allow(repository).to receive(:fetch_remote) { sync_remote(repository, remote_mirror.remote_name, local_branch_names) }
-
-        expect(raw_repository).not_to receive(:push_remote_branches)
-
-        subject.execute(remote_mirror)
-      end
-
-      it "sync new branches" do
-        # call local_branch_names early so it is not called after the new branch has been created
-        current_branches = local_branch_names
-        allow(repository).to receive(:fetch_remote) { sync_remote(repository, remote_mirror.remote_name, current_branches) }
-        create_branch(repository, 'my-new-branch')
-
-        expect(raw_repository).to receive(:push_remote_branches).with(remote_mirror.remote_name, ['my-new-branch'])
-
-        subject.execute(remote_mirror)
-      end
-
-      it "sync updated branches" do
-        allow(repository).to receive(:fetch_remote) do
-          sync_remote(repository, remote_mirror.remote_name, local_branch_names)
-          update_branch(repository, 'existing-branch')
-        end
-
-        expect(raw_repository).to receive(:push_remote_branches).with(remote_mirror.remote_name, ['existing-branch'])
+        expect(remote_mirror).to receive(:update_repository).with({})
 
         subject.execute(remote_mirror)
       end
-
-      context 'when push only protected branches option is set' do
-        let(:unprotected_branch_name) { 'existing-branch' }
-        let(:protected_branch_name) do
-          project.repository.branch_names.find { |n| n != unprotected_branch_name }
-        end
-        let!(:protected_branch) do
-          create(:protected_branch, project: project, name: protected_branch_name)
-        end
-
-        before do
-          project.reload
-          remote_mirror.only_protected_branches = true
-        end
-
-        it "sync updated protected branches" do
-          allow(repository).to receive(:fetch_remote) do
-            sync_remote(repository, remote_mirror.remote_name, local_branch_names)
-            update_branch(repository, protected_branch_name)
-          end
-
-          expect(raw_repository).to receive(:push_remote_branches).with(remote_mirror.remote_name, [protected_branch_name])
-
-          subject.execute(remote_mirror)
-        end
-
-        it 'does not sync unprotected branches' do
-          allow(repository).to receive(:fetch_remote) do
-            sync_remote(repository, remote_mirror.remote_name, local_branch_names)
-            update_branch(repository, unprotected_branch_name)
-          end
-
-          expect(raw_repository).not_to receive(:push_remote_branches).with(remote_mirror.remote_name, [unprotected_branch_name])
-
-          subject.execute(remote_mirror)
-        end
-      end
-
-      context 'when branch exists in local and remote repo' do
-        context 'when it has diverged' do
-          it 'syncs branches' do
-            allow(repository).to receive(:fetch_remote) do
-              sync_remote(repository, remote_mirror.remote_name, local_branch_names)
-              update_remote_branch(repository, remote_mirror.remote_name, 'markdown')
-            end
-
-            expect(raw_repository).to receive(:push_remote_branches).with(remote_mirror.remote_name, ['markdown'])
-
-            subject.execute(remote_mirror)
-          end
-        end
-      end
-
-      describe 'for delete' do
-        context 'when branch exists in local and remote repo' do
-          it 'deletes the branch from remote repo' do
-            allow(repository).to receive(:fetch_remote) do
-              sync_remote(repository, remote_mirror.remote_name, local_branch_names)
-              delete_branch(repository, 'existing-branch')
-            end
-
-            expect(raw_repository).to receive(:delete_remote_branches).with(remote_mirror.remote_name, ['existing-branch'])
-
-            subject.execute(remote_mirror)
-          end
-        end
-
-        context 'when push only protected branches option is set' do
-          before do
-            remote_mirror.only_protected_branches = true
-          end
-
-          context 'when branch exists in local and remote repo' do
-            let!(:protected_branch_name) { local_branch_names.first }
-
-            before do
-              create(:protected_branch, project: project, name: protected_branch_name)
-              project.reload
-            end
-
-            it 'deletes the protected branch from remote repo' do
-              allow(repository).to receive(:fetch_remote) do
-                sync_remote(repository, remote_mirror.remote_name, local_branch_names)
-                delete_branch(repository, protected_branch_name)
-              end
-
-              expect(raw_repository).not_to receive(:delete_remote_branches).with(remote_mirror.remote_name, [protected_branch_name])
-
-              subject.execute(remote_mirror)
-            end
-
-            it 'does not delete the unprotected branch from remote repo' do
-              allow(repository).to receive(:fetch_remote) do
-                sync_remote(repository, remote_mirror.remote_name, local_branch_names)
-                delete_branch(repository, 'existing-branch')
-              end
-
-              expect(raw_repository).not_to receive(:delete_remote_branches).with(remote_mirror.remote_name, ['existing-branch'])
-
-              subject.execute(remote_mirror)
-            end
-          end
-
-          context 'when branch only exists on remote repo' do
-            let!(:protected_branch_name) { 'remote-branch' }
-
-            before do
-              create(:protected_branch, project: project, name: protected_branch_name)
-            end
-
-            context 'when it has diverged' do
-              it 'does not delete the remote branch' do
-                allow(repository).to receive(:fetch_remote) do
-                  sync_remote(repository, remote_mirror.remote_name, local_branch_names)
-
-                  rev = repository.find_branch('markdown').dereferenced_target
-                  create_remote_branch(repository, remote_mirror.remote_name, 'remote-branch', rev.id)
-                end
-
-                expect(raw_repository).not_to receive(:delete_remote_branches)
-
-                subject.execute(remote_mirror)
-              end
-            end
-
-            context 'when it has not diverged' do
-              it 'deletes the remote branch' do
-                allow(repository).to receive(:fetch_remote) do
-                  sync_remote(repository, remote_mirror.remote_name, local_branch_names)
-
-                  masterrev = repository.find_branch('master').dereferenced_target
-                  create_remote_branch(repository, remote_mirror.remote_name, protected_branch_name, masterrev.id)
-                end
-
-                expect(raw_repository).to receive(:delete_remote_branches).with(remote_mirror.remote_name, [protected_branch_name])
-
-                subject.execute(remote_mirror)
-              end
-            end
-          end
-        end
-
-        context 'when branch only exists on remote repo' do
-          context 'when it has diverged' do
-            it 'does not delete the remote branch' do
-              allow(repository).to receive(:fetch_remote) do
-                sync_remote(repository, remote_mirror.remote_name, local_branch_names)
-
-                rev = repository.find_branch('markdown').dereferenced_target
-                create_remote_branch(repository, remote_mirror.remote_name, 'remote-branch', rev.id)
-              end
-
-              expect(raw_repository).not_to receive(:delete_remote_branches)
-
-              subject.execute(remote_mirror)
-            end
-          end
-
-          context 'when it has not diverged' do
-            it 'deletes the remote branch' do
-              allow(repository).to receive(:fetch_remote) do
-                sync_remote(repository, remote_mirror.remote_name, local_branch_names)
-
-                masterrev = repository.find_branch('master').dereferenced_target
-                create_remote_branch(repository, remote_mirror.remote_name, 'remote-branch', masterrev.id)
-              end
-
-              expect(raw_repository).to receive(:delete_remote_branches).with(remote_mirror.remote_name, ['remote-branch'])
-
-              subject.execute(remote_mirror)
-            end
-          end
-        end
-      end
     end
 
-    describe 'Syncing tags' do
-      before do
-        allow(repository).to receive(:fetch_remote) { sync_remote(repository, remote_mirror.remote_name, local_branch_names) }
+    context 'when only syncing protected branches' do
+      let(:unprotected_branch_name) { 'existing-branch' }
+      let(:protected_branch_name) do
+        project.repository.branch_names.find { |n| n != unprotected_branch_name }
       end
-
-      context 'when there are not tags to push' do
-        it 'does not try to push tags' do
-          allow(repository).to receive(:remote_tags) { {} }
-          allow(repository).to receive(:tags) { [] }
-
-          expect(repository).not_to receive(:push_tags)
-
-          subject.execute(remote_mirror)
-        end
+      let!(:protected_branch) do
+        create(:protected_branch, project: project, name: protected_branch_name)
       end
 
-      context 'when there are some tags to push' do
-        it 'pushes tags to remote' do
-          allow(raw_repository).to receive(:remote_tags) { {} }
-
-          expect(raw_repository).to receive(:push_remote_branches).with(remote_mirror.remote_name, ['v1.0.0', 'v1.1.0'])
-
-          subject.execute(remote_mirror)
-        end
+      before do
+        project.reload
+        remote_mirror.only_protected_branches = true
       end
 
-      context 'when there are some tags to delete' do
-        it 'deletes tags from remote' do
-          remote_tags = generate_tags(repository, 'v1.0.0', 'v1.1.0')
-          allow(raw_repository).to receive(:remote_tags) { remote_tags }
-
-          repository.rm_tag(create(:user), 'v1.0.0')
-
-          expect(raw_repository).to receive(:delete_remote_branches).with(remote_mirror.remote_name, ['v1.0.0'])
+      it "sync updated protected branches" do
+        allow(repository).to receive(:fetch_remote)
+        expect(remote_mirror).to receive(:update_repository).with(only_branches_matching: [protected_branch_name])
 
-          subject.execute(remote_mirror)
-        end
+        subject.execute(remote_mirror)
       end
     end
   end
 
-  def create_branch(repository, branch_name)
-    rugged = repository.rugged
-    masterrev = repository.find_branch('master').dereferenced_target
-    parentrev = repository.commit(masterrev).parent_id
-
-    rugged.references.create("refs/heads/#{branch_name}", parentrev)
-
-    repository.expire_branches_cache
-  end
-
-  def create_remote_branch(repository, remote_name, branch_name, source_id)
-    rugged = repository.rugged
-
-    rugged.references.create("refs/remotes/#{remote_name}/#{branch_name}", source_id)
-  end
-
   def sync_remote(repository, remote_name, local_branch_names)
-    rugged = repository.rugged
-
     local_branch_names.each do |branch|
-      target = repository.find_branch(branch).try(:dereferenced_target)
-      rugged.references.create("refs/remotes/#{remote_name}/#{branch}", target.id) if target
+      commit = repository.commit(branch)
+      repository.write_ref("refs/remotes/#{remote_name}/#{branch}", commit.id) if commit
     end
   end
 
   def update_remote_branch(repository, remote_name, branch)
-    rugged = repository.rugged
-    masterrev = repository.find_branch('master').dereferenced_target.id
+    masterrev = repository.commit('master').id
 
-    rugged.references.create("refs/remotes/#{remote_name}/#{branch}", masterrev, force: true)
+    repository.write_ref("refs/remotes/#{remote_name}/#{branch}", masterrev, force: true)
     repository.expire_branches_cache
   end
 
   def update_branch(repository, branch)
-    rugged = repository.rugged
-    masterrev = repository.find_branch('master').dereferenced_target.id
-
-    # Updated existing branch
-    rugged.references.create("refs/heads/#{branch}", masterrev, force: true)
-    repository.expire_branches_cache
-  end
-
-  def delete_branch(repository, branch)
-    rugged = repository.rugged
+    masterrev = repository.commit('master').id
 
-    rugged.references.delete("refs/heads/#{branch}")
+    repository.write_ref("refs/heads/#{branch}", masterrev, force: true)
     repository.expire_branches_cache
   end