Synchronize the default branch when updating a pull mirror

ee/app/models/ee/repository.rb

@@ -10,7 +10,7 @@ module EE
     MIRROR_REMOTE = "upstream".freeze
 
     included do
-      delegate :checksum, :find_remote_root_ref, to: :raw_repository
+      delegate :checksum, to: :raw_repository
     end
 
     # Transiently sets a configuration variable

ee/app/services/geo/repository_sync_service.rb

@@ -45,10 +45,8 @@ module Geo
       project.ensure_repository
     end
 
-    # Update the default branch querying the remote to determine its HEAD
     def update_root_ref
-      root_ref = repository.find_remote_root_ref(GEO_REMOTE_NAME)
-      project.change_head(root_ref) if root_ref.present? && root_ref != project.default_branch
+      project.update_root_ref(GEO_REMOTE_NAME)
     end
 
     def schedule_repack

ee/app/services/projects/update_mirror_service.rb

@@ -17,6 +17,7 @@ module Projects
       end
 
       update_branches
+      update_root_ref
 
       success
     rescue Gitlab::Shell::Error, UpdateError => e
@@ -60,6 +61,11 @@ module Projects
       end
     end
 
+    # Update the default branch querying the remote to determine its HEAD
+    def update_root_ref
+      project.update_root_ref(::Repository::MIRROR_REMOTE)
+    end
+
     def update_tags(&block)
       old_tags = repository_tags_with_target.each_with_object({}) { |tag, tags| tags[tag.name] = tag }
 

ee/spec/models/project_spec.rb

@@ -1763,4 +1763,38 @@ describe Project do
 
     it { is_expected.to be true }
   end
+
+  describe '#update_root_ref' do
+    let(:project) { create(:project, :repository) }
+
+    it 'updates the default branch when HEAD has changed' do
+      stub_find_remote_root_ref(project, ref: 'feature')
+
+      expect { project.update_root_ref('origin') }
+        .to change { project.default_branch }
+        .from('master')
+        .to('feature')
+    end
+
+    it 'does not update the default branch when HEAD does not change' do
+      stub_find_remote_root_ref(project, ref: 'master')
+
+      expect { project.update_root_ref('origin') }
+        .not_to change { project.default_branch }
+    end
+
+    it 'does not update the default branch when HEAD does not exist' do
+      stub_find_remote_root_ref(project, ref: 'foo')
+
+      expect { project.update_root_ref('origin') }
+        .not_to change { project.default_branch }
+    end
+
+    def stub_find_remote_root_ref(project, ref:)
+      allow(project.repository)
+        .to receive(:find_remote_root_ref)
+        .with('origin')
+        .and_return(ref)
+    end
+  end
 end

ee/spec/services/projects/update_mirror_service_spec.rb

@@ -5,6 +5,8 @@ describe Projects::UpdateMirrorService do
     create(:project, :repository, :mirror, import_url: Project::UNKNOWN_IMPORT_URL, only_mirror_protected_branches: false)
   end
 
+  subject(:service) { described_class.new(project, project.owner) }
+
   describe "#execute" do
     context 'unlicensed' do
       before do
@@ -14,55 +16,61 @@ describe Projects::UpdateMirrorService do
       it 'does nothing' do
         expect(project).not_to receive(:fetch_mirror)
 
-        result = described_class.new(project, project.owner).execute
+        result = service.execute
+
         expect(result[:status]).to eq(:success)
       end
     end
 
     it "fetches the upstream repository" do
+      stub_find_remote_root_ref(project)
+
       expect(project).to receive(:fetch_mirror)
 
-      described_class.new(project, project.owner).execute
+      service.execute
     end
 
     it 'rescues exceptions from Repository#ff_merge' do
       stub_fetch_mirror(project)
+      stub_find_remote_root_ref(project)
 
       expect(project.repository).to receive(:ff_merge).and_raise(Gitlab::Git::PreReceiveError)
 
-      expect { described_class.new(project, project.owner).execute }.not_to raise_error
+      expect { service.execute }.not_to raise_error
     end
 
-    it "succeeds" do
+    it "returns success when updated succeeds" do
       stub_fetch_mirror(project)
+      stub_find_remote_root_ref(project)
 
-      result = described_class.new(project, project.owner).execute
+      result = service.execute
 
       expect(result[:status]).to eq(:success)
     end
 
-    describe "updating tags" do
+    context "updating tags" do
       it "creates new tags" do
         stub_fetch_mirror(project)
+        stub_find_remote_root_ref(project)
 
-        described_class.new(project, project.owner).execute
+        service.execute
 
         expect(project.repository.tag_names).to include('new-tag')
       end
 
       it "only invokes GitTagPushService for tags pointing to commits" do
         stub_fetch_mirror(project)
+        stub_find_remote_root_ref(project)
 
         expect(GitTagPushService).to receive(:new)
-          .with(project, project.owner, hash_including(ref: 'refs/tags/new-tag')).and_return(double(execute: true))
+          .with(project, project.owner, hash_including(ref: 'refs/tags/new-tag'))
+          .and_return(double(execute: true))
 
-        described_class.new(project, project.owner).execute
+        service.execute
       end
     end
 
-    describe "updating branches" do
-      subject { described_class.new(project, project.owner) }
-
+    context "updating branches" do
       context 'when mirror only protected branches option is set' do
         let(:new_protected_branch_name) { 'new-branch' }
         let(:protected_branch_name) { 'existing-branch' }
@@ -76,16 +84,18 @@ describe Projects::UpdateMirrorService do
           project.reload
 
           stub_fetch_mirror(project)
+          stub_find_remote_root_ref(project)
 
-          subject.execute
+          service.execute
 
           expect(project.repository.branch_names).to include(new_protected_branch_name)
         end
 
         it 'does not create an unprotected branch' do
           stub_fetch_mirror(project)
+          stub_find_remote_root_ref(project)
 
-          described_class.new(project, project.owner).execute
+          service.execute
 
           expect(project.repository.branch_names).not_to include(new_protected_branch_name)
         end
@@ -95,8 +105,9 @@ describe Projects::UpdateMirrorService do
           project.reload
 
           stub_fetch_mirror(project)
+          stub_find_remote_root_ref(project)
 
-          subject.execute
+          service.execute
 
           expect(project.repository.find_branch(protected_branch_name).dereferenced_target)
             .to eq(project.repository.find_branch('master').dereferenced_target)
@@ -104,8 +115,9 @@ describe Projects::UpdateMirrorService do
 
         it "does not update unprotected branches" do
           stub_fetch_mirror(project)
+          stub_find_remote_root_ref(project)
 
-          subject.execute
+          service.execute
 
           expect(project.repository.find_branch(protected_branch_name).dereferenced_target)
             .not_to eq(project.repository.find_branch('master').dereferenced_target)
@@ -114,16 +126,18 @@ describe Projects::UpdateMirrorService do
 
       it "creates new branches" do
         stub_fetch_mirror(project)
+        stub_find_remote_root_ref(project)
 
-        subject.execute
+        service.execute
 
         expect(project.repository.branch_names).to include('new-branch')
       end
 
       it "updates existing branches" do
         stub_fetch_mirror(project)
+        stub_find_remote_root_ref(project)
 
-        subject.execute
+        service.execute
 
         expect(project.repository.find_branch('existing-branch').dereferenced_target)
           .to eq(project.repository.find_branch('master').dereferenced_target)
@@ -132,13 +146,14 @@ describe Projects::UpdateMirrorService do
       context 'with diverged branches' do
         before do
           stub_fetch_mirror(project)
+          stub_find_remote_root_ref(project)
         end
 
         context 'when mirror_overwrites_diverged_branches is true' do
           it 'update diverged branches' do
             project.mirror_overwrites_diverged_branches = true
 
-            subject.execute
+            service.execute
 
             expect(project.repository.find_branch('markdown').dereferenced_target)
                 .to eq(project.repository.find_branch('master').dereferenced_target)
@@ -149,7 +164,7 @@ describe Projects::UpdateMirrorService do
           it "doesn't update diverged branches" do
             project.mirror_overwrites_diverged_branches = false
 
-            subject.execute
+            service.execute
 
             expect(project.repository.find_branch('markdown').dereferenced_target)
                 .not_to eq(project.repository.find_branch('master').dereferenced_target)
@@ -160,7 +175,7 @@ describe Projects::UpdateMirrorService do
           it "doesn't update diverged branches" do
             project.mirror_overwrites_diverged_branches = nil
 
-            subject.execute
+            service.execute
 
             expect(project.repository.find_branch('markdown').dereferenced_target)
                 .not_to eq(project.repository.find_branch('master').dereferenced_target)
@@ -168,16 +183,16 @@ describe Projects::UpdateMirrorService do
         end
       end
 
-      describe 'when project is empty' do
-        let(:project) { create(:project_empty_repo, :mirror, import_url: Project::UNKNOWN_IMPORT_URL) }
-
+      context 'when project is empty' do
         it 'does not add a default master branch' do
+          project    = create(:project_empty_repo, :mirror, import_url: Project::UNKNOWN_IMPORT_URL)
           repository = project.repository
+          stub_find_remote_root_ref(project)
 
           allow(project).to receive(:fetch_mirror) { create_file(repository) }
           expect(CreateBranchService).not_to receive(:create_master_branch)
 
-          subject.execute
+          service.execute
 
           expect(repository.branch_names).not_to include('master')
         end
@@ -194,37 +209,52 @@ describe Projects::UpdateMirrorService do
       end
     end
 
-    describe "when the mirror user doesn't have access" do
-      it "fails" do
-        stub_fetch_mirror(project)
+    it 'updates the default branch when HEAD has changed' do
+      stub_fetch_mirror(project)
+      stub_find_remote_root_ref(project, ref: 'new-branch')
 
-        result = described_class.new(project, create(:user)).execute
+      expect { service.execute }.to change { project.default_branch }.from('master').to('new-branch')
+    end
 
-        expect(result[:status]).to eq(:error)
-      end
+    it 'does not update the default branch when HEAD does not change' do
+      stub_fetch_mirror(project)
+      stub_find_remote_root_ref(project, ref: 'master')
+
+      expect { service.execute }.not_to change { project.default_branch }
     end
 
-    describe "when no user is present" do
-      it "fails" do
-        result = described_class.new(project, nil).execute
+    it "fails when the mirror user doesn't have access" do
+      stub_fetch_mirror(project)
+      stub_find_remote_root_ref(project)
 
-        expect(result[:status]).to eq(:error)
-      end
+      result = described_class.new(project, create(:user)).execute
+
+      expect(result[:status]).to eq(:error)
     end
 
-    describe "when is no mirror" do
-      let(:project) { build_stubbed(:project) }
+    it "fails when no user is present" do
+      result = described_class.new(project, nil).execute
 
-      it "success" do
-        expect(project.mirror?).to eq(false)
+      expect(result[:status]).to eq(:error)
+    end
 
-        result = described_class.new(project, create(:user)).execute
+    it "returns success when there is no mirror" do
+      project = build_stubbed(:project)
+      user    = create(:user)
 
-        expect(result[:status]).to eq(:success)
-      end
+      result = described_class.new(project, user).execute
+
+      expect(result[:status]).to eq(:success)
     end
   end
 
+  def stub_find_remote_root_ref(project, remote: 'upstream', ref: 'master')
+    allow(project.repository)
+      .to receive(:find_remote_root_ref)
+      .with(remote)
+      .and_return(ref)
+  end
+
   def stub_fetch_mirror(project, repository: project.repository)
     allow(project).to receive(:fetch_mirror) { fetch_mirror(repository) }
   end