Merge branch 'mk/fix-non-project-git-verification-head-ref' into 'master'

Geo: Fix snippet verification by replicating the HEAD ref

See merge request gitlab-org/gitlab!65783

app/models/concerns/has_repository.rb

@@ -14,6 +14,7 @@ module HasRepository
   include Gitlab::Utils::StrongMemoize
 
   delegate :base_dir, :disk_path, to: :storage
+  delegate :change_head, to: :repository
 
   def valid_repo?
     repository.exists?
@@ -117,4 +118,8 @@ module HasRepository
   def repository_size_checker
     raise NotImplementedError
   end
+
+  def after_repository_change_head
+    reload_default_branch
+  end
 end

app/models/project.rb

@@ -1649,18 +1649,11 @@ class Project < ApplicationRecord
     :visibility_level
   end
 
-  def change_head(branch)
-    if repository.branch_exists?(branch)
-      repository.before_change_head
-      repository.raw_repository.write_ref('HEAD', "refs/heads/#{branch}")
-      repository.copy_gitattributes(branch)
-      repository.after_change_head
+  override :after_repository_change_head
+  def after_repository_change_head
     ProjectCacheWorker.perform_async(self.id, [], [:commit_count])
-      reload_default_branch
-    else
-      errors.add(:base, _("Could not change HEAD: branch '%{branch}' does not exist") % { branch: branch })
-      false
-    end
+
+    super
   end
 
   def forked_from?(other_project)

app/models/repository.rb

@@ -466,6 +466,7 @@ class Repository
   # Runs code after the HEAD of a repository is changed.
   def after_change_head
     expire_all_method_caches
+    container.after_repository_change_head
   end
 
   # Runs code after a new commit has been pushed.
@@ -1142,6 +1143,18 @@ class Repository
     Gitlab::CurrentSettings.pick_repository_storage
   end
 
+  def change_head(branch)
+    if branch_exists?(branch)
+      before_change_head
+      raw_repository.write_ref('HEAD', "refs/heads/#{branch}")
+      copy_gitattributes(branch)
+      after_change_head
+    else
+      container.errors.add(:base, _("Could not change HEAD: branch '%{branch}' does not exist") % { branch: branch })
+      false
+    end
+  end
+
   private
 
   # TODO Genericize finder, later split this on finders by Ref or Oid

ee/app/models/ee/project.rb

@@ -684,15 +684,6 @@ module EE
         ::Gitlab::CurrentSettings.custom_project_templates_enabled?
     end
 
-    # Update the default branch querying the remote to determine its HEAD
-    def update_root_ref(remote, remote_url, authorization)
-      root_ref = repository.find_remote_root_ref(remote, remote_url, authorization)
-      change_head(root_ref) if root_ref.present?
-    rescue ::Gitlab::Git::Repository::NoRepository => e
-      ::Gitlab::AppLogger.error("Error updating root ref for project #{full_path} (#{id}): #{e.message}.")
-      nil
-    end
-
     override :lfs_http_url_to_repo
     def lfs_http_url_to_repo(operation = nil)
       return super unless ::Gitlab::Geo.secondary_with_primary?

ee/app/models/ee/repository.rb

@@ -101,6 +101,15 @@ module EE
       blob_data_at(sha, ::Gitlab::Insights::CONFIG_FILE_PATH)
     end
 
+    # Update the default branch querying the remote to determine its HEAD
+    def update_root_ref(remote, remote_url, authorization)
+      root_ref = find_remote_root_ref(remote, remote_url, authorization)
+      change_head(root_ref) if root_ref.present?
+    rescue ::Gitlab::Git::Repository::NoRepository => e
+      ::Gitlab::AppLogger.error("Error updating root ref for repository #{full_path} (#{container.id}): #{e.message}.")
+      nil
+    end
+
     private
 
     def diverged?(branch_name, remote_ref)

ee/app/services/geo/framework_repository_sync_service.rb

@@ -38,6 +38,7 @@ module Geo
     def sync_repository
       start_registry_sync!
       fetch_repository
+      update_root_ref
       mark_sync_as_successful
     rescue Gitlab::Git::Repository::NoRepository => e
       log_info('Marking the repository for a forced re-download')
@@ -258,5 +259,13 @@ module Geo
     def replicable_name
       replicator.replicable_name
     end
+
+    def update_root_ref
+      authorization = ::Gitlab::Geo::RepoSyncRequest.new(
+        scope: repository.full_path
+      ).authorization
+
+      repository.update_root_ref(GEO_REMOTE_NAME, replicator.remote_url, authorization)
+    end
   end
 end

ee/app/services/geo/repository_sync_service.rb

@@ -54,7 +54,7 @@ module Geo
         scope: repository.full_path
       ).authorization
 
-      project.update_root_ref(GEO_REMOTE_NAME, remote_url, authorization)
+      repository.update_root_ref(GEO_REMOTE_NAME, remote_url, authorization)
     end
 
     def execute_housekeeping

ee/spec/models/project_spec.rb

@@ -2029,55 +2029,6 @@ RSpec.describe Project do
     end
   end
 
-  describe '#update_root_ref' do
-    let(:project) { create(:project, :repository) }
-    let(:url) { 'http://git.example.com/remote-repo.git' }
-    let(:auth) { 'Basic secret' }
-
-    it 'updates the default branch when HEAD has changed' do
-      stub_find_remote_root_ref(project, ref: 'feature')
-
-      expect { project.update_root_ref('origin', url, auth) }
-        .to change { project.default_branch }
-        .from('master')
-        .to('feature')
-    end
-
-    it 'always updates the default branch even when HEAD does not change' do
-      stub_find_remote_root_ref(project, ref: 'master')
-
-      expect(project).to receive(:change_head).with('master').and_call_original
-
-      project.update_root_ref('origin', url, auth)
-
-      # For good measure, expunge the root ref cache and reload.
-      project.repository.expire_all_method_caches
-      expect(project.reload.default_branch).to eq('master')
-    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', url, auth) }
-        .not_to change { project.default_branch }
-    end
-
-    it 'does not raise error when repository does not exist' do
-      allow(project.repository).to receive(:find_remote_root_ref)
-        .with('origin', url, auth)
-        .and_raise(Gitlab::Git::Repository::NoRepository)
-
-      expect { project.update_root_ref('origin', url, auth) }.not_to raise_error
-    end
-
-    def stub_find_remote_root_ref(project, ref:)
-      allow(project.repository)
-        .to receive(:find_remote_root_ref)
-        .with('origin', url, auth)
-        .and_return(ref)
-    end
-  end
-
   describe '#feature_flags_client_token' do
     let(:project) { create(:project) }
 

ee/spec/models/repository_spec.rb

@@ -264,4 +264,50 @@ RSpec.describe Repository do
       end
     end
   end
+
+  describe '#update_root_ref' do
+    let(:url) { 'http://git.example.com/remote-repo.git' }
+    let(:auth) { 'Basic secret' }
+
+    it 'updates the default branch when HEAD has changed' do
+      stub_find_remote_root_ref(repository, ref: 'feature')
+
+      expect { repository.update_root_ref('origin', url, auth) }
+        .to change { project.default_branch }
+        .from('master')
+        .to('feature')
+    end
+
+    it 'always updates the default branch even when HEAD does not change' do
+      stub_find_remote_root_ref(repository, ref: 'master')
+
+      expect(repository).to receive(:change_head).with('master').and_call_original
+
+      repository.update_root_ref('origin', url, auth)
+
+      expect(project.default_branch).to eq('master')
+    end
+
+    it 'does not update the default branch when HEAD does not exist' do
+      stub_find_remote_root_ref(repository, ref: 'foo')
+
+      expect { repository.update_root_ref('origin', url, auth) }
+        .not_to change { project.default_branch }
+    end
+
+    it 'does not raise error when repository does not exist' do
+      allow(repository).to receive(:find_remote_root_ref)
+        .with('origin', url, auth)
+        .and_raise(Gitlab::Git::Repository::NoRepository)
+
+      expect { repository.update_root_ref('origin', url, auth) }.not_to raise_error
+    end
+
+    def stub_find_remote_root_ref(repository, ref:)
+      allow(repository)
+        .to receive(:find_remote_root_ref)
+        .with('origin', url, auth)
+        .and_return(ref)
+    end
+  end
 end

ee/spec/services/geo/framework_repository_sync_service_spec.rb

@@ -53,7 +53,7 @@ RSpec.describe Geo::FrameworkRepositorySyncService, :geo do
 
       allow_any_instance_of(Repository)
         .to receive(:find_remote_root_ref)
-        .with('geo')
+        .with('geo', url_to_repo, anything)
         .and_return('master')
     end
 
@@ -72,8 +72,8 @@ RSpec.describe Geo::FrameworkRepositorySyncService, :geo do
     end
 
     it 'expires repository caches' do
-      expect_any_instance_of(Repository).to receive(:expire_all_method_caches).once
-      expect_any_instance_of(Repository).to receive(:expire_branch_cache).once
+      expect_any_instance_of(Repository).to receive(:expire_all_method_caches).twice
+      expect_any_instance_of(Repository).to receive(:expire_branch_cache).twice
       expect_any_instance_of(Repository).to receive(:expire_content_cache).once
 
       subject.execute
@@ -172,6 +172,53 @@ RSpec.describe Geo::FrameworkRepositorySyncService, :geo do
           expect(registry.reload.retry_count).to be_zero
           expect(registry.retry_at).to be_nil
         end
+
+        context 'with non empty repositories' do
+          context 'when HEAD change' do
+            before do
+              allow(repository)
+                .to receive(:find_remote_root_ref)
+                .with('geo', url_to_repo, anything)
+                .and_return('feature')
+            end
+
+            it 'syncs gitattributes to info/attributes' do
+              expect(repository).to receive(:copy_gitattributes)
+
+              subject.execute
+            end
+
+            it 'updates the default branch' do
+              expect(repository).to receive(:with_config)
+                .with("http.#{url_to_repo}.extraHeader" => anything)
+                .and_call_original
+                .once
+
+              expect(repository).to receive(:change_head).with('feature').once
+
+              subject.execute
+            end
+          end
+
+          context 'when HEAD does not change' do
+            it 'syncs gitattributes to info/attributes' do
+              expect(repository).to receive(:copy_gitattributes)
+
+              subject.execute
+            end
+
+            it 'updates the default branch' do
+              expect(repository).to receive(:with_config)
+                .with("http.#{url_to_repo}.extraHeader" => anything)
+                .and_call_original
+                .once
+
+              expect(repository).to receive(:change_head).with('master').once
+
+              subject.execute
+            end
+          end
+        end
       end
 
       context 'when repository sync fail' do

ee/spec/services/geo/repository_sync_service_spec.rb

@@ -253,19 +253,13 @@ RSpec.describe Geo::RepositorySyncService, :geo do
                 .and_call_original
                 .once
 
-              expect(project).to receive(:change_head).with('feature').once
+              expect(repository).to receive(:change_head).with('feature').once
 
               subject.execute
             end
-
-            context 'when HEAD does not change' do
-              before do
-                allow(project.repository)
-                  .to receive(:find_remote_root_ref)
-                  .with('geo', url_to_repo, anything)
-                  .and_return(project.default_branch)
           end
 
+          context 'when HEAD does not change' do
             it 'syncs gitattributes to info/attributes' do
               expect(repository).to receive(:copy_gitattributes)
 
@@ -278,14 +272,13 @@ RSpec.describe Geo::RepositorySyncService, :geo do
                 .and_call_original
                 .once
 
-                expect(project).to receive(:change_head).with('master').once
+              expect(repository).to receive(:change_head).with('master').once
 
               subject.execute
             end
           end
         end
       end
-      end
 
       context 'when repository sync fail' do
         let(:registry) { Geo::ProjectRegistry.find_by(project_id: project.id) }

spec/models/project_spec.rb

@@ -3156,35 +3156,19 @@ RSpec.describe Project, factory_default: :keep do
     end
   end
 
-  describe '#change_head' do
-    let_it_be(:project) { create(:project, :repository) }
-
-    it 'returns error if branch does not exist' do
-      expect(project.change_head('unexisted-branch')).to be false
-      expect(project.errors.size).to eq(1)
-    end
-
-    it 'calls the before_change_head and after_change_head methods' do
-      expect(project.repository).to receive(:before_change_head)
-      expect(project.repository).to receive(:after_change_head)
-
-      project.change_head(project.default_branch)
-    end
+  describe '#after_repository_change_head' do
+    let_it_be(:project) { create(:project) }
 
     it 'updates commit count' do
       expect(ProjectCacheWorker).to receive(:perform_async).with(project.id, [], [:commit_count])
 
-      project.change_head(project.default_branch)
-    end
-
-    it 'copies the gitattributes' do
-      expect(project.repository).to receive(:copy_gitattributes).with(project.default_branch)
-      project.change_head(project.default_branch)
+      project.after_repository_change_head
     end
 
     it 'reloads the default branch' do
       expect(project).to receive(:reload_default_branch)
-      project.change_head(project.default_branch)
+
+      project.after_repository_change_head
     end
   end
 

spec/models/repository_spec.rb

@@ -2141,6 +2141,12 @@ RSpec.describe Repository do
 
       repository.after_change_head
     end
+
+    it 'calls after_repository_change_head on container' do
+      expect(repository.container).to receive(:after_repository_change_head)
+
+      repository.after_change_head
+    end
   end
 
   describe '#expires_caches_for_tags' do
@@ -3266,4 +3272,30 @@ RSpec.describe Repository do
       settings.save!
     end
   end
+
+  describe '#change_head' do
+    let(:branch) { repository.container.default_branch }
+
+    it 'adds an error to container if branch does not exist' do
+      expect(repository.change_head('unexisted-branch')).to be false
+      expect(repository.container.errors.size).to eq(1)
+    end
+
+    it 'calls the before_change_head and after_change_head methods' do
+      expect(repository).to receive(:before_change_head)
+      expect(repository).to receive(:after_change_head)
+
+      repository.change_head(branch)
+    end
+
+    it 'copies the gitattributes' do
+      expect(repository).to receive(:copy_gitattributes).with(branch)
+      repository.change_head(branch)
+    end
+
+    it 'reloads the default branch' do
+      expect(repository.container).to receive(:reload_default_branch)
+      repository.change_head(branch)
+    end
+  end
 end

spec/support/shared_examples/models/concerns/has_repository_shared_examples.rb

@@ -152,4 +152,20 @@ RSpec.shared_examples 'model with repository' do
     it { is_expected.to respond_to(:disk_path) }
     it { is_expected.to respond_to(:gitlab_shell) }
   end
+
+  describe '#change_head' do
+    it 'delegates #change_head to repository' do
+      expect(stubbed_container.repository).to receive(:change_head).with('foo')
+
+      stubbed_container.change_head('foo')
+    end
+  end
+
+  describe '#after_repository_change_head' do
+    it 'calls #reload_default_branch' do
+      expect(stubbed_container).to receive(:reload_default_branch)
+
+      stubbed_container.after_repository_change_head
+    end
+  end
 end