Updates from last code review

* Use Rugged filters to work with local/remote branches.
* Rename Project#remote_mirror? to Project#has_remote_mirror?
* Use delegate pattern for Repository#push_remote_branches and Repository#delete_remote_branches
* Make Repository#remote_tags return Gitlab::Git::Tag objects.
* Refactor Repository#remote_branches
* Rename Repository#push_branches to Repository#push_remote_branches
* Made other small refactors to views and specs.
* Make encrypted_credentials column size bigger.
  This was giving problems with MySql for large passwords.
* Refactor for UpdateRemoteMirrorService spec so we no longer
  harcode the branch names and specs are not broken after `gitlab-test`
  project is updated.

app/models/project.rb

@@ -182,7 +182,7 @@ class Project < ActiveRecord::Base
 
   accepts_nested_attributes_for :variables, allow_destroy: true
   accepts_nested_attributes_for :remote_mirrors,
-    allow_destroy: true, reject_if: proc { |attrs| attrs[:id].blank? && attrs[:url].blank? }
+    allow_destroy: true, reject_if: ->(attrs) { attrs[:id].blank? && attrs[:url].blank? }
 
   delegate :name, to: :owner, allow_nil: true, prefix: true
   delegate :members, to: :team, prefix: true
@@ -538,7 +538,7 @@ class Project < ActiveRecord::Base
     update_column(:import_error, error_message)
   end
 
-  def remote_mirror?
+  def has_remote_mirror?
     remote_mirrors.enabled.exists?
   end
 
@@ -1213,10 +1213,6 @@ class Project < ActiveRecord::Base
 
   private
 
-  def remove_mirror_repository_reference
-    repository.remove_remote(Repository::MIRROR_REMOTE)
-  end
-
   def update_forks_visibility_level
     return unless visibility_level < visibility_level_was
 
@@ -1228,9 +1224,13 @@ class Project < ActiveRecord::Base
     end
   end
 
+  def remove_mirror_repository_reference
+    repository.remove_remote(Repository::MIRROR_REMOTE)
+  end
+
   def import_url_availability
     if remote_mirrors.find_by(url: import_url)
-      errors.add(:import_url, 'is already in use by the remote mirror')
+      errors.add(:import_url, 'is already in use by a remote mirror')
     end
   end
 

app/models/remote_mirror.rb

@@ -17,11 +17,15 @@
 class RemoteMirror < ActiveRecord::Base
   include AfterCommitQueue
 
-  attr_encrypted :credentials, key: Gitlab::Application.secrets.db_key_base, marshal: true, encode: true, mode: :per_attribute_iv_and_salt
+  attr_encrypted :credentials,
+                 key: Gitlab::Application.secrets.db_key_base,
+                 marshal: true,
+                 encode: true,
+                 mode: :per_attribute_iv_and_salt
 
   belongs_to :project
 
-  validates :url, presence: true, url: { protocols: %w(ssh git http https), allow_blank: true }, on: :create
+  validates :url, presence: true, url: { protocols: %w(ssh git http https), allow_blank: true }
   validate  :url_availability, if: :url_changed?
 
   after_save :refresh_remote, if: :url_changed?
@@ -30,6 +34,7 @@ class RemoteMirror < ActiveRecord::Base
 
   scope :enabled, -> { where(enabled: true) }
   scope :started, -> { with_update_status(:started) }
+  scope :stuck,   -> { started.where('last_update_at < ?', 1.day.ago) }
 
   state_machine :update_status, initial: :none do
     event :update_start do
@@ -55,14 +60,14 @@ class RemoteMirror < ActiveRecord::Base
     after_transition any => :started, do: :schedule_update_job
 
     after_transition started: :finished do |remote_mirror, transaction|
-      timestamp = DateTime.now
+      timestamp = Time.now
       remote_mirror.update_attributes!(
         last_update_at: timestamp, last_successful_update_at: timestamp, last_error: nil
       )
     end
 
     after_transition started: :failed do |remote_mirror, transaction|
-      remote_mirror.update(last_update_at: DateTime.now)
+      remote_mirror.update(last_update_at: Time.now)
     end
   end
 
@@ -81,7 +86,7 @@ class RemoteMirror < ActiveRecord::Base
   def sync
     return if !enabled || update_in_progress?
 
-    update_failed? ?  update_retry : update_start
+    update_failed? ? update_retry : update_start
   end
 
   def mark_for_delete_if_blank_url

app/models/repository.rb

 require 'securerandom'
+require 'forwardable'
 
 class Repository
   include Elastic::RepositoriesSearch
+  extend Forwardable
 
   class CommitError < StandardError; end
 
@@ -16,6 +18,8 @@ class Repository
 
   attr_accessor :path_with_namespace, :project
 
+  def_delegators :gitlab_shell, :push_remote_branches, :delete_remote_branches
+
   def self.clean_old_archives
     repository_downloads_path = Gitlab.config.gitlab.repository_downloads_path
 
@@ -183,14 +187,6 @@ class Repository
     gitlab_shell.rm_tag(path_with_namespace, tag_name)
   end
 
-  def push_branches(project_name, remote, branch_names)
-    gitlab_shell.push_branches(project_name, remote, branch_names)
-  end
-
-  def delete_remote_branches(project_name, remote, branch_names)
-    gitlab_shell.delete_remote_branches(project_name, remote, branch_names)
-  end
-
   def add_remote(name, url)
     raw_repository.remote_add(name, url)
   rescue Rugged::ConfigError
@@ -218,7 +214,9 @@ class Repository
   end
 
   def remote_tags(remote)
-    gitlab_shell.list_remote_tags(path_with_namespace, remote)
+    gitlab_shell.list_remote_tags(path_with_namespace, remote).map do |name, target|
+      Gitlab::Git::Tag.new(name, target)
+    end
   end
 
   def fetch_remote_forced!(remote)
@@ -687,7 +685,19 @@ class Repository
   alias_method :branches, :local_branches
 
   def remote_branches(remote_name)
-    branches_from_ref("remotes/#{remote_name}")
+    branches = []
+
+    rugged.references.each("refs/remotes/#{remote_name}/*").map do |ref|
+      name = ref.name.sub(/\Arefs\/remotes\/#{remote_name}\//, '')
+
+      begin
+        branches << Gitlab::Git::Branch.new(name, ref.target)
+      rescue Rugged::ReferenceError
+        # Omit invalid branch
+      end
+    end
+
+    branches
   end
 
   def tags
@@ -864,20 +874,8 @@ class Repository
     fetch_remote_forced!(Repository::MIRROR_GEO)
   end
 
-  def branches_from_ref(ref_name)
-    rugged.references.each("refs/#{ref_name}/*").map do |ref|
-      name = ref.name.sub(/\Arefs\/#{ref_name}\//, "")
-
-      begin
-        Gitlab::Git::Branch.new(name, ref.target)
-      rescue Rugged::ReferenceError
-        # Omit invalid branch
-      end
-    end.compact
-  end
-
   def upstream_branches
-    branches_from_ref("remotes/#{Repository::MIRROR_REMOTE}")
+    @upstream_branches ||= remote_branches(Repository::MIRROR_REMOTE)
   end
 
   def diverged_from_upstream?(branch_name)

app/services/projects/update_remote_mirror_service.rb

@@ -49,7 +49,7 @@ module Projects
       # Push the default branch first so it works fine when remote mirror is empty.
       branches.unshift(*default_branch)
 
-      repository.push_branches(project.path_with_namespace, mirror.ref_name, branches)
+      repository.push_remote_branches(project.path_with_namespace, mirror.ref_name, branches)
     end
 
     def delete_branches
@@ -81,7 +81,7 @@ module Projects
     end
 
     def divergent_branches
-      remote_branches.each_with_object([]) do |(name, branch), branches|
+      remote_branches.each_with_object([]) do |(name, _), branches|
         if local_branches[name] && repository.upstream_has_diverged?(name, mirror.ref_name)
           branches << name
         end
@@ -95,13 +95,13 @@ module Projects
     end
 
     def remote_tags
-      @remote_tags ||= repository.remote_tags(mirror.ref_name).each_with_object({}) do |(name, target), tags|
-        tags[name] = target
+      @remote_tags ||= repository.remote_tags(mirror.ref_name).each_with_object({}) do |tag, tags|
+        tags[tag.name] = tag
       end
     end
 
     def push_tags
-      repository.push_branches(project.path_with_namespace, mirror.ref_name, changed_tags)
+      repository.push_remote_branches(project.path_with_namespace, mirror.ref_name, changed_tags)
     end
 
     def delete_tags
@@ -110,16 +110,16 @@ module Projects
 
     def changed_tags
       @changed_tags ||= local_tags.each_with_object([]) do |(name, tag), tags|
-        remote_tag_target = remote_tags[name]
+        remote_tag = remote_tags[name]
 
-        if remote_tag_target.nil? || (tag.target != remote_tag_target)
+        if remote_tag.nil? || (tag.target != remote_tag.target)
           tags << name
         end
       end
     end
 
     def deleted_tags
-      @deleted_tags ||= remote_tags.each_with_object([]) do |(name, target), tags|
+      @deleted_tags ||= remote_tags.each_with_object([]) do |(name, _), tags|
         tags << name if local_tags[name].nil?
       end
     end

app/views/projects/buttons/_update_mirror.html.haml

 - if can?(current_user, :push_code, @project)
-  - if !@project.remote_mirror? && @project.mirror?
+  - if !@project.has_remote_mirror? && @project.mirror?
     - size = nil unless defined?(size) && size
     - if @project.updating_mirror?
-      %span.btn.disabled.update-mirror-button.has_tooltip{title: "Updating from upstream..."}
+      %span.btn.disabled.update-mirror-button.has-tooltip{title: "Updating from upstream..."}
         = icon('refresh')
     - else
-      = link_to update_now_namespace_project_mirror_path(@project.namespace, @project), method: :post, class: "btn update-mirror-button has_tooltip", title: "Update from upstream" do
+      = link_to update_now_namespace_project_mirror_path(@project.namespace, @project), method: :post, class: "btn update-mirror-button has-tooltip", title: "Update from upstream" do
         = icon('refresh')
-  - elsif @project.remote_mirror? && !@project.mirror?
+  - elsif @project.has_remote_mirror? && !@project.mirror?
     - if @project.updating_remote_mirror?
-      %span.btn.disabled.update-mirror-button.has_tooltip{title: "Updating remote repository..."}
+      %span.btn.disabled.update-mirror-button.has-tooltip{title: "Updating remote repository..."}
         = icon('refresh')
     - else
-      = link_to update_remote_now_namespace_project_mirror_path(@project.namespace, @project), method: :post, class: "btn update-mirror-button has_tooltip", title: "Update remote repository" do
+      = link_to update_remote_now_namespace_project_mirror_path(@project.namespace, @project), method: :post, class: "btn update-mirror-button has-tooltip", title: "Update remote repository" do
         = icon('refresh')
-  - elsif @project.remote_mirror? && @project.mirror?
+  - elsif @project.has_remote_mirror? && @project.mirror?
     .btn-group
       %a.btn.dropdown-toggle{href: '#', 'data-toggle' => 'dropdown'}
         = icon('refresh')
       %ul.dropdown-menu.dropdown-menu-right
         %li
           - if @project.updating_mirror?
-            %a.disabled-item Updating from upstream...
+            %span.prepend-left-10.disabled-item Updating from upstream...
           - else
             = link_to "Update this repository", update_now_namespace_project_mirror_path(@project.namespace, @project), method: :post
         %li
           - if @project.updating_remote_mirror?
-            %a.disabled-item Updating remote repository...
+            %span.prepend-left-10.disabled-item Updating remote repository...
           - else
             = link_to "Update remote repository", update_remote_now_namespace_project_mirror_path(@project.namespace, @project), method: :post

app/views/projects/mirrors/show.html.haml

 - page_title "Mirror Repository"
-%h3.page_title
+%h3.page-title
   Mirror Repository
 
 %p.light
   A repository can be setup as a mirror of another repository, and can also have a remote mirror associated.
 
 %p.light
-  When the repository is configured as a mirror, all of its content will automatically be updated from the repository configured in the <strong>Pull from a remote repository</strong> section.
+  When the repository is configured as a mirror, all of its branches, tags, and commits will automatically be updated from the repository configured in the
+  %strong Pull from a remote repository
+  section.
 
 %p.light
   When the repository has a remote mirror associated, it means that the remote repository configured in the <strong>Push to a remote repository</strong> section will automatically receive updates from the current repository.
@@ -24,7 +26,7 @@
     Set up your project to automatically have its branches, tags, and commits updated from an upstream repository every hour.
   = render "shared/mirror_update_button"
   - if @project.mirror_last_update_success?
-    %span &nbsp;Successfully updated #{time_ago_with_tooltip(@project.mirror_last_successful_update_at)}.
+    %span.prepend-left-default Successfully updated #{time_ago_with_tooltip(@project.mirror_last_successful_update_at)}.
   %hr.clearfix
 
   - if @project.mirror_last_update_failed?
@@ -83,7 +85,7 @@
     Set up the remote repository that you want to update with the content of the current repository every hour.
   = render "shared/remote_mirror_update_button", remote_mirror: @remote_mirror
   - if @remote_mirror.last_successful_update_at
-    %span &nbsp;Successfully updated #{time_ago_with_tooltip(@remote_mirror.last_successful_update_at)}.
+    %span.prepend-left-default Successfully updated #{time_ago_with_tooltip(@remote_mirror.last_successful_update_at)}.
 
   %hr.clearfix
 

app/views/shared/_remote_mirror_update_button.html.haml

-- if @project.remote_mirror?
+- if @project.has_remote_mirror?
   - if remote_mirror.update_in_progress?
     %span.btn.disabled
       = icon('refresh')
@@ -7,4 +7,3 @@
     = link_to update_remote_now_namespace_project_mirror_path(@project.namespace, @project), method: :post, class: "btn" do
       = icon('refresh')
       Update Now
-

app/workers/update_all_remote_mirrors_worker.rb

@@ -8,10 +8,7 @@ class UpdateAllRemoteMirrorsWorker
   end
 
   def fail_stuck_mirrors!
-    stuck = RemoteMirror.started.
-      where("last_update_at < ?", 1.day.ago)
-
-    stuck.find_each(batch_size: 50) do |remote_mirror|
+    RemoteMirror.stuck.find_each(batch_size: 50) do |remote_mirror|
       remote_mirror.mark_as_failed('The mirror update took too long to complete.')
     end
   end

config/gitlab.yml.example

@@ -190,6 +190,10 @@ production: &base
     update_all_mirrors_worker:
       cron: "0 * * * *"
 
+    # Update remote mirrors
+    update_all_remote_mirrors_worker:
+      cron: "30 * * * *"
+
     # In addition to refreshing users when they log in,
     # periodically refresh LDAP users membership.
     # NOTE: This will only take effect if LDAP is enabled

db/migrate/20160321161032_create_remote_mirrors.rb

@@ -8,6 +8,9 @@ class CreateRemoteMirrors < ActiveRecord::Migration
       t.datetime :last_update_at
       t.datetime :last_successful_update_at
       t.string :last_error
+      t.text :encrypted_credentials
+      t.string :encrypted_credentials_iv
+      t.string :encrypted_credentials_salt
 
       t.timestamps null: false
     end

db/migrate/20160330031024_add_mirror_credentials_to_remote_mirrors.rb

-class AddMirrorCredentialsToRemoteMirrors < ActiveRecord::Migration
-  def change
-    add_column :remote_mirrors, :encrypted_credentials, :text
-    add_column :remote_mirrors, :encrypted_credentials_iv, :text
-    add_column :remote_mirrors, :encrypted_credentials_salt, :text
-  end
-end

db/schema.rb

@@ -891,11 +891,11 @@ ActiveRecord::Schema.define(version: 20160331223143) do
     t.datetime "last_update_at"
     t.datetime "last_successful_update_at"
     t.string   "last_error"
-    t.datetime "created_at",                               null: false
-    t.datetime "updated_at",                               null: false
     t.text     "encrypted_credentials"
-    t.text     "encrypted_credentials_iv"
-    t.text     "encrypted_credentials_salt"
+    t.string   "encrypted_credentials_iv"
+    t.string   "encrypted_credentials_salt"
+    t.datetime "created_at",                                null: false
+    t.datetime "updated_at",                                null: false
   end
 
   add_index "remote_mirrors", ["project_id"], name: "index_remote_mirrors_on_project_id", using: :btree

lib/gitlab/backend/shell.rb

@@ -43,12 +43,13 @@ module Gitlab
 
     def list_remote_tags(name, remote)
       output, status = Popen::popen([gitlab_shell_projects_path, 'list-remote-tags', "#{name}.git", remote])
+      tags_with_targets = []
 
       raise Error, output unless status.zero?
 
       # Each line has this format: "dc872e9fa6963f8f03da6c8f6f264d0845d6b092\trefs/tags/v1.10.0\n"
       # We want to convert it to: [{ 'v1.10.0' => 'dc872e9fa6963f8f03da6c8f6f264d0845d6b092' }, ...]
-      tags_with_targets = output.lines.flat_map do |line|
+      output.lines.each do |line|
         target, path = line.strip!.split("\t")
 
         # When the remote repo is empty we don't have tags.
@@ -59,8 +60,8 @@ module Gitlab
         # See: http://stackoverflow.com/questions/15472107/when-listing-git-ls-remote-why-theres-after-the-tag-name
         next if name =~ /\^\{\}\Z/
 
-        [name, target]
-      end.compact
+        tags_with_targets.concat([name, target])
+      end
 
       Hash[*tags_with_targets]
     end
@@ -304,9 +305,9 @@ module Gitlab
     # branch_name - remote branch name
     #
     # Ex.
-    #   push_branches('upstream', 'feature')
+    #   push_remote_branches('upstream', 'feature')
     #
-    def push_branches(project_name, remote_name, branch_names)
+    def push_remote_branches(project_name, remote_name, branch_names)
       args = [gitlab_shell_projects_path, 'push-branches', "#{project_name}.git", remote_name, *branch_names]
       output, status = Popen::popen(args)
       raise Error, output unless status.zero?

spec/models/remote_mirror_spec.rb

 require 'rails_helper'
 
-describe RemoteMirror, focus: true do
+describe RemoteMirror do
   describe 'encrypting credentials' do
     context 'when setting URL for a first time' do
       it 'should store the URL without credentials' do
@@ -14,6 +14,15 @@ describe RemoteMirror, focus: true do
 
         expect(mirror.credentials).to eq({ user: 'foo', password: 'bar' })
       end
+
+      it 'should handle credentials with large content' do
+        mirror = create_mirror_with_url('http://bxnhm8dote33ct932r3xavslj81wxmr7o8yux8do10oozckkif:9ne7fuvjn40qjt35dgt8v86q9m9g9essryxj76sumg2ccl2fg26c0krtz2gzfpyq4hf22h328uhq6npuiq6h53tpagtsj7vsrz75@test.com')
+
+        expect(mirror.credentials).to eq({
+          user: 'bxnhm8dote33ct932r3xavslj81wxmr7o8yux8do10oozckkif',
+          password: '9ne7fuvjn40qjt35dgt8v86q9m9g9essryxj76sumg2ccl2fg26c0krtz2gzfpyq4hf22h328uhq6npuiq6h53tpagtsj7vsrz75'
+        })
+      end
     end
 
     context 'when updating the URL' do

spec/models/repository_spec.rb

@@ -939,12 +939,12 @@ describe Repository, models: true do
     end
   end
 
-  describe '#push_branches' do
+  describe '#push_remote_branches' do
     it 'push branches to the remote repo' do
-      expect_any_instance_of(Gitlab::Shell).to receive(:push_branches).
+      expect_any_instance_of(Gitlab::Shell).to receive(:push_remote_branches).
         with('project_name', 'remote_name', ['branch'])
 
-      repository.push_branches('project_name', 'remote_name', ['branch'])
+      repository.push_remote_branches('project_name', 'remote_name', ['branch'])
     end
   end
 
@@ -967,10 +967,17 @@ describe Repository, models: true do
 
   describe '#remote_tags' do
     it 'gets the remote tags' do
+      masterrev = repository.find_branch('master').target
+
       expect_any_instance_of(Gitlab::Shell).to receive(:list_remote_tags).
-        with(repository.path_with_namespace, 'upstream')
+        with(repository.path_with_namespace, 'upstream').
+        and_return({ 'v0.0.1' => masterrev })
+
+      tags = repository.remote_tags('upstream')
 
-      repository.remote_tags('upstream')
+      expect(tags.first).to be_an_instance_of(Gitlab::Git::Tag)
+      expect(tags.first.name).to eq('v0.0.1')
+      expect(tags.first.target).to eq(masterrev)
     end
   end
 
@@ -1009,7 +1016,7 @@ describe Repository, models: true do
 
   def create_remote_branch(remote_name, branch_name, target)
     rugged = repository.rugged
-    ref = rugged.references.create("refs/remotes/#{remote_name}/#{branch_name}", target)
+    rugged.references.create("refs/remotes/#{remote_name}/#{branch_name}", target)
   end
 
 end

spec/services/projects/update_remote_mirror_service_spec.rb

@@ -4,9 +4,7 @@ describe Projects::UpdateRemoteMirrorService do
   let(:project) { create(:project) }
   let(:remote_project) { create(:forked_project_with_submodules) }
   let(:repository) { project.repository }
-  let(:remote_repository) { remote_project.repository }
   let(:remote_mirror) { project.remote_mirrors.create!(url: remote_project.http_url_to_repo) }
-  let(:all_branches) { ["master", 'existing-branch', "'test'", "empty-branch", "feature", "feature_conflict", "fix", "flatten-dir", "improve/awesome", "lfs", "markdown"] }
 
   subject { described_class.new(project, project.creator) }
 
@@ -18,14 +16,14 @@ describe Projects::UpdateRemoteMirrorService do
 
     it "fetches the remote repository" do
       expect(repository).to receive(:fetch_remote).with(remote_mirror.ref_name, no_tags: true) do
-        sync_remote(repository, remote_mirror.ref_name, all_branches)
+        sync_remote(repository, remote_mirror.ref_name, local_branch_names)
       end
 
       subject.execute(remote_mirror)
     end
 
     it "succeeds" do
-      allow(repository).to receive(:fetch_remote) { sync_remote(repository, remote_mirror.ref_name, all_branches) }
+      allow(repository).to receive(:fetch_remote) { sync_remote(repository, remote_mirror.ref_name, local_branch_names) }
 
       result = subject.execute(remote_mirror)
 
@@ -36,42 +34,44 @@ describe Projects::UpdateRemoteMirrorService do
       it "push all the branches the first time" do
         allow(repository).to receive(:fetch_remote)
 
-        expect(repository).to receive(:push_branches).with(project.path_with_namespace, remote_mirror.ref_name, all_branches)
+        expect(repository).to receive(:push_remote_branches).with(project.path_with_namespace, remote_mirror.ref_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.ref_name, all_branches) }
+        allow(repository).to receive(:fetch_remote) { sync_remote(repository, remote_mirror.ref_name, local_branch_names) }
 
-        expect(repository).not_to receive(:push_branches)
+        expect(repository).not_to receive(:push_remote_branches)
 
         subject.execute(remote_mirror)
       end
 
       it "sync new branches" do
-        allow(repository).to receive(:fetch_remote) { sync_remote(repository, remote_mirror.ref_name, all_branches) }
+        # 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.ref_name, current_branches) }
         create_branch(repository, 'my-new-branch')
 
-        expect(repository).to receive(:push_branches).with(project.path_with_namespace, remote_mirror.ref_name, ['my-new-branch'])
+        expect(repository).to receive(:push_remote_branches).with(project.path_with_namespace, remote_mirror.ref_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.ref_name, all_branches)
+          sync_remote(repository, remote_mirror.ref_name, local_branch_names)
           update_branch(repository, 'existing-branch')
         end
 
-        expect(repository).to receive(:push_branches).with(project.path_with_namespace, remote_mirror.ref_name, ['existing-branch'])
+        expect(repository).to receive(:push_remote_branches).with(project.path_with_namespace, remote_mirror.ref_name, ['existing-branch'])
 
         subject.execute(remote_mirror)
       end
 
       it "sync deleted branches" do
         allow(repository).to receive(:fetch_remote) do
-          sync_remote(repository, remote_mirror.ref_name, all_branches)
+          sync_remote(repository, remote_mirror.ref_name, local_branch_names)
           delete_branch(repository, 'existing-branch')
         end
 
@@ -83,7 +83,7 @@ describe Projects::UpdateRemoteMirrorService do
 
     describe 'Syncing tags' do
       before do
-        allow(repository).to receive(:fetch_remote) { sync_remote(repository, remote_mirror.ref_name, all_branches) }
+        allow(repository).to receive(:fetch_remote) { sync_remote(repository, remote_mirror.ref_name, local_branch_names) }
       end
 
       context 'when there are not tags to push' do
@@ -101,7 +101,7 @@ describe Projects::UpdateRemoteMirrorService do
         it 'should push tags to remote' do
           allow(repository).to receive(:remote_tags) { {} }
 
-          expect(repository).to receive(:push_branches).with(
+          expect(repository).to receive(:push_remote_branches).with(
             project.path_with_namespace, remote_mirror.ref_name, ['v1.0.0', 'v1.1.0']
           )
 
@@ -135,10 +135,10 @@ describe Projects::UpdateRemoteMirrorService do
     repository.expire_branches_cache
   end
 
-  def sync_remote(repository, remote_name, all_branches)
+  def sync_remote(repository, remote_name, local_branch_names)
     rugged = repository.rugged
 
-    all_branches.each do |branch|
+    local_branch_names.each do |branch|
       target = repository.find_branch(branch).try(:target)
       rugged.references.create("refs/remotes/#{remote_name}/#{branch}", target) if target
     end
@@ -148,7 +148,7 @@ describe Projects::UpdateRemoteMirrorService do
     rugged = repository.rugged
     masterrev = repository.find_branch('master').target
 
-    # # Updated existing branch
+    # Updated existing branch
     rugged.references.create("refs/heads/#{branch}", masterrev, force: true)
     repository.expire_branches_cache
   end
@@ -161,8 +161,15 @@ describe Projects::UpdateRemoteMirrorService do
   end
 
   def generate_tags(repository, *tag_names)
-    tag_names.each_with_object({}) do |name, tags|
-      tags[name] = repository.find_tag(name).try(:target)
+    tag_names.each_with_object([]) do |name, tags|
+      tag_rev = repository.find_tag(name).try(:target)
+      tags << Gitlab::Git::Tag.new(name, tag_rev)
     end
   end
+
+  def local_branch_names
+    branch_names = repository.branches.map(&:name)
+    # we want the protected branch to be pushed first
+    branch_names.unshift(branch_names.delete('master'))
+  end
 end