Merge branch '36807-gc-unwanted-refs-after-import' into 'master'

Remove unwanted refs after importing a project

Closes #36807

See merge request !13766

4 files changed, 113 insertions, 2 deletions

diff --git a/spec/lib/gitlab/git/repository_spec.rb b/spec/lib/gitlab/git/repository_spec.rb
index 6b9773c9b63..4cfb4b7d357 100644
--- a/spec/lib/gitlab/git/repository_spec.rb
+++ b/spec/lib/gitlab/git/repository_spec.rb
@@ -433,6 +433,40 @@ describe Gitlab::Git::Repository, seed_helper: true do
     end
   end
 
+  describe '#delete_refs' do
+    before(:all) do
+      @repo = Gitlab::Git::Repository.new('default', TEST_MUTABLE_REPO_PATH, '')
+    end
+
+    it 'deletes the ref' do
+      @repo.delete_refs('refs/heads/feature')
+
+      expect(@repo.rugged.references['refs/heads/feature']).to be_nil
+    end
+
+    it 'deletes all refs' do
+      refs = %w[refs/heads/wip refs/tags/v1.1.0]
+      @repo.delete_refs(*refs)
+
+      refs.each do |ref|
+        expect(@repo.rugged.references[ref]).to be_nil
+      end
+    end
+
+    it 'raises an error if it failed' do
+      expect(Gitlab::Popen).to receive(:popen).and_return(['Error', 1])
+
+      expect do
+        @repo.delete_refs('refs/heads/fix')
+      end.to raise_error(Gitlab::Git::Repository::GitError)
+    end
+
+    after(:all) do
+      FileUtils.rm_rf(TEST_MUTABLE_REPO_PATH)
+      ensure_seeds
+    end
+  end
+
   describe "#refs_hash" do
     let(:refs) { repository.refs_hash }
 
diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb
index 2e613c44357..11717ba39e8 100644
--- a/spec/models/project_spec.rb
+++ b/spec/models/project_spec.rb
@@ -1563,10 +1563,18 @@ describe Project do
 
   describe 'project import state transitions' do
     context 'state transition: [:started] => [:finished]' do
-      let(:housekeeping_service) { spy }
+      let(:after_import_service) { spy(:after_import_service) }
+      let(:housekeeping_service) { spy(:housekeeping_service) }
 
       before do
-        allow(Projects::HousekeepingService).to receive(:new) { housekeeping_service }
+        allow(Projects::AfterImportService)
+          .to receive(:new) { after_import_service }
+
+        allow(after_import_service)
+          .to receive(:execute) { housekeeping_service.execute }
+
+        allow(Projects::HousekeepingService)
+          .to receive(:new) { housekeeping_service }
       end
 
       it 'resets project import_error' do
@@ -1581,6 +1589,7 @@ describe Project do
 
         project.import_finish
 
+        expect(after_import_service).to have_received(:execute)
         expect(housekeeping_service).to have_received(:execute)
       end
 
diff --git a/spec/services/projects/after_import_service_spec.rb b/spec/services/projects/after_import_service_spec.rb
new file mode 100644
index 00000000000..c6678fc1f5c
--- /dev/null
+++ b/spec/services/projects/after_import_service_spec.rb
@@ -0,0 +1,55 @@
+require 'spec_helper'
+
+describe Projects::AfterImportService do
+  subject { described_class.new(project) }
+
+  let(:project) { create(:project, :repository) }
+  let(:repository) { project.repository }
+  let(:sha) { project.commit.sha }
+  let(:housekeeping_service) { double(:housekeeping_service) }
+
+  describe '#execute' do
+    before do
+      allow(Projects::HousekeepingService)
+        .to receive(:new).with(project).and_return(housekeeping_service)
+
+      allow(housekeeping_service)
+        .to receive(:execute).and_yield
+    end
+
+    it 'performs housekeeping' do
+      subject.execute
+
+      expect(housekeeping_service).to have_received(:execute)
+    end
+
+    context 'with some refs in refs/pull/**/*' do
+      before do
+        repository.write_ref('refs/pull/1/head', sha)
+        repository.write_ref('refs/pull/1/merge', sha)
+
+        subject.execute
+      end
+
+      it 'removes refs/pull/**/*' do
+        expect(repository.rugged.references.map(&:name))
+          .not_to include(%r{\Arefs/pull/})
+      end
+    end
+
+    Repository::RESERVED_REFS_NAMES.each do |name|
+      context "with a ref in refs/#{name}/tmp" do
+        before do
+          repository.write_ref("refs/#{name}/tmp", sha)
+
+          subject.execute
+        end
+
+        it "does not remove refs/#{name}/tmp" do
+          expect(repository.rugged.references.map(&:name))
+            .to include("refs/#{name}/tmp")
+        end
+      end
+    end
+  end
+end
diff --git a/spec/services/projects/housekeeping_service_spec.rb b/spec/services/projects/housekeeping_service_spec.rb
index 385f56e447f..9386c110385 100644
--- a/spec/services/projects/housekeeping_service_spec.rb
+++ b/spec/services/projects/housekeeping_service_spec.rb
@@ -23,6 +23,12 @@ describe Projects::HousekeepingService do
       expect(project.reload.pushes_since_gc).to eq(0)
     end
 
+    it 'yields the block if given' do
+      expect do |block|
+        subject.execute(&block)
+      end.to yield_with_no_args
+    end
+
     context 'when no lease can be obtained' do
       before do
         expect(subject).to receive(:try_obtain_lease).and_return(false)
@@ -39,6 +45,13 @@ describe Projects::HousekeepingService do
           expect { subject.execute }.to raise_error(Projects::HousekeepingService::LeaseTaken)
         end.not_to change { project.pushes_since_gc }
       end
+
+      it 'does not yield' do
+        expect do |block|
+          expect { subject.execute(&block) }
+            .to raise_error(Projects::HousekeepingService::LeaseTaken)
+        end.not_to yield_with_no_args
+      end
     end
   end