Add changelog and tests

5 files changed, 88 insertions, 3 deletions

diff --git a/app/services/projects/import_export/cleanup_service.rb b/app/services/projects/import_export/cleanup_service.rb
index 54d7fb88a91..75eaee0cb7b 100644
--- a/app/services/projects/import_export/cleanup_service.rb
+++ b/app/services/projects/import_export/cleanup_service.rb
@@ -1,8 +1,11 @@
 module Projects
   module ImportExport
     class CleanupService
+      RESERVED_REFS_NAMES =
+        %w[heads tags merge-requests keep-around environments]
       RESERVED_REFS_REGEXP =
-        %r{\Arefs/(?:heads|tags|merge\-requests|keep\-around|environments)/}
+        %r{\Arefs/(?:#{
+          RESERVED_REFS_NAMES.map(&Regexp.method(:escape)).join('|')})/}x
 
       attr_reader :project
 
diff --git a/changelogs/unreleased/36807-gc-unwanted-refs-after-import.yml b/changelogs/unreleased/36807-gc-unwanted-refs-after-import.yml
new file mode 100644
index 00000000000..a37de4325bb
--- /dev/null
+++ b/changelogs/unreleased/36807-gc-unwanted-refs-after-import.yml
@@ -0,0 +1,5 @@
+---
+title: Remove unwanted refs after importing a project
+merge_request: 13766
+author:
+type: other
diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb
index 2e613c44357..130c0739033 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(:cleanup_service) { spy(:cleanup_service) }
+      let(:housekeeping_service) { spy(:housekeeping_service) }
 
       before do
-        allow(Projects::HousekeepingService).to receive(:new) { housekeeping_service }
+        allow(Projects::ImportExport::CleanupService)
+          .to receive(:new) { cleanup_service }
+
+        allow(cleanup_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(cleanup_service).to have_received(:execute)
         expect(housekeeping_service).to have_received(:execute)
       end
 
diff --git a/spec/services/projects/housekeeping_service_spec.rb b/spec/services/projects/housekeeping_service_spec.rb
index 385f56e447f..6e916a523fe 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 |b|
+        subject.execute(&b)
+      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 |b|
+          expect { subject.execute(&b) }
+            .to raise_error(Projects::HousekeepingService::LeaseTaken)
+        end.not_to yield_with_no_args
+      end
     end
   end
 
diff --git a/spec/services/projects/import_export/cleanup_service_spec.rb b/spec/services/projects/import_export/cleanup_service_spec.rb
new file mode 100644
index 00000000000..b46efc40a2f
--- /dev/null
+++ b/spec/services/projects/import_export/cleanup_service_spec.rb
@@ -0,0 +1,55 @@
+require 'spec_helper'
+
+describe Projects::ImportExport::CleanupService 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
+
+    described_class::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