Resolve feedback on the MR:

https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/13766

* Rename AfterImportService
* Use constants
* Move Git operations to Gitlab::Git::Repository
* Use Regexp.union

5 files changed, 46 insertions, 47 deletions

diff --git a/app/models/project.rb b/app/models/project.rb
index c0060504d74..f86f75fbbdc 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -371,11 +371,7 @@ class Project < ActiveRecord::Base
 
       if Gitlab::ImportSources.importer_names.include?(project.import_type) && project.repo_exists?
         project.run_after_commit do
-          begin
-            Projects::HousecleaningService.new(project).execute
-          rescue Projects::HousekeepingService::LeaseTaken => e
-            Rails.logger.info("Could not perform housekeeping for project #{project.full_path} (#{project.id}): #{e}")
-          end
+          Projects::AfterImportService.new(project).execute
         end
       end
     end
diff --git a/app/services/projects/after_import_service.rb b/app/services/projects/after_import_service.rb
new file mode 100644
index 00000000000..bbada7e2b1c
--- /dev/null
+++ b/app/services/projects/after_import_service.rb
@@ -0,0 +1,32 @@
+module Projects
+  class AfterImportService
+    RESERVED_REFS_NAMES =
+      %w[heads tags merge-requests keep-around environments].freeze
+
+    RESERVED_REFS_REGEXP =
+      %r{\Arefs/(?:#{Regexp.union(*RESERVED_REFS_NAMES)})/}
+
+    def initialize(project)
+      @project = project
+    end
+
+    def execute
+      Projects::HousekeepingService.new(@project).execute do
+        repository.delete_refs(garbage_refs)
+      end
+    rescue Projects::HousekeepingService::LeaseTaken => e
+      Rails.logger.info(
+        "Could not perform housekeeping for project #{@project.full_path} (#{@project.id}): #{e}")
+    end
+
+    private
+
+    def garbage_refs
+      @garbage_refs ||= repository.all_ref_names_except(RESERVED_REFS_REGEXP)
+    end
+
+    def repository
+      @repository ||= @project.repository
+    end
+  end
+end
diff --git a/app/services/projects/housecleaning_service.rb b/app/services/projects/housecleaning_service.rb
deleted file mode 100644
index d5cf8478e13..00000000000
--- a/app/services/projects/housecleaning_service.rb
+++ /dev/null
@@ -1,40 +0,0 @@
-module Projects
-  class HousecleaningService
-    def self.reserved_refs_names
-      %w[heads tags merge-requests keep-around environments]
-    end
-
-    def self.reserved_refs_regexp
-      names = reserved_refs_names.map(&Regexp.method(:escape)).join('|')
-
-      %r{\Arefs/(?:#{names})/}
-    end
-
-    def initialize(project)
-      @project = project
-    end
-
-    # This could raise Projects::HousekeepingService::LeaseTaken
-    def execute
-      Projects::HousekeepingService.new(@project).execute do
-        garbage_refs.each(&rugged.references.method(:delete))
-      end
-    end
-
-    private
-
-    def garbage_refs
-      @garbage_refs ||= begin
-        reserved_refs_regexp = self.class.reserved_refs_regexp
-
-        rugged.references.reject do |ref|
-          ref.name =~ reserved_refs_regexp
-        end
-      end
-    end
-
-    def rugged
-      @rugged ||= @project.repository.rugged
-    end
-  end
-end
diff --git a/lib/gitlab/git/repository.rb b/lib/gitlab/git/repository.rb
index eb3731ba35a..f6d30ad7534 100644
--- a/lib/gitlab/git/repository.rb
+++ b/lib/gitlab/git/repository.rb
@@ -232,6 +232,13 @@ module Gitlab
         branch_names + tag_names
       end
 
+      # Returns an Array of all ref names, except when it's matching pattern
+      #
+      # regexp - The pattern for ref names we don't want
+      def all_ref_names_except(regexp)
+        rugged.references.reject { |ref| ref.name =~ regexp }.map(&:name)
+      end
+
       # Discovers the default branch based on the repository's available branches
       #
       # - If no branches are present, returns nil
@@ -577,6 +584,10 @@ module Gitlab
         rugged.branches.delete(branch_name)
       end
 
+      def delete_refs(ref_names)
+        ref_names.each(&rugged.references.method(:delete))
+      end
+
       # Create a new branch named **ref+ based on **stat_point+, HEAD by default
       #
       # Examples:
diff --git a/spec/services/projects/housecleaning_service_spec.rb b/spec/services/projects/after_import_service_spec.rb
index 3dd7906dc9a..540d2191b2d 100644
--- a/spec/services/projects/housecleaning_service_spec.rb
+++ b/spec/services/projects/after_import_service_spec.rb
@@ -1,6 +1,6 @@
 require 'spec_helper'
 
-describe Projects::HousecleaningService do
+describe Projects::AfterImportService do
   subject { described_class.new(project) }
 
   let(:project) { create(:project, :repository) }
@@ -37,7 +37,7 @@ describe Projects::HousecleaningService do
       end
     end
 
-    described_class.reserved_refs_names.each do |name|
+    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)