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

* 36807-gc-unwanted-refs-after-import:
  Use `git update-ref --stdin -z` to delete refs
  Just use a block
  Fix tests
  Try to make reserved ref names more obvious
  Resolve feedback on the MR:
  Move to Projects::HousecleaningService
  Use block rather than just b
  Just use @project directly
  Just use methods over constants, so that we could
  Add changelog and tests
  Remove unwanted refs after importing a project

app/models/environment.rb

@@ -114,7 +114,7 @@ class Environment < ActiveRecord::Base
   end
 
   def ref_path
-    "refs/environments/#{Shellwords.shellescape(name)}"
+    "refs/#{Repository::REF_ENVIRONMENTS}/#{Shellwords.shellescape(name)}"
   end
 
   def formatted_external_url

app/models/merge_request.rb

@@ -822,7 +822,7 @@ class MergeRequest < ActiveRecord::Base
   end
 
   def ref_path
-    "refs/merge-requests/#{iid}/head"
+    "refs/#{Repository::REF_MERGE_REQUEST}/#{iid}/head"
   end
 
   def ref_fetched?

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::HousekeepingService.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

app/models/repository.rb

@@ -3,6 +3,18 @@ require 'securerandom'
 require 'forwardable'
 
 class Repository
+  REF_MERGE_REQUEST = 'merge-requests'.freeze
+  REF_KEEP_AROUND = 'keep-around'.freeze
+  REF_ENVIRONMENTS = 'environments'.freeze
+
+  RESERVED_REFS_NAMES = %W[
+    heads
+    tags
+    #{REF_ENVIRONMENTS}
+    #{REF_KEEP_AROUND}
+    #{REF_ENVIRONMENTS}
+  ].freeze
+
   include Gitlab::ShellAdapter
   include Elastic::RepositoriesSearch
   include RepositoryMirroring
@@ -241,10 +253,10 @@ class Repository
     begin
       write_ref(keep_around_ref_name(sha), sha)
     rescue Rugged::ReferenceError => ex
-      Rails.logger.error "Unable to create keep-around reference for repository #{path}: #{ex}"
+      Rails.logger.error "Unable to create #{REF_KEEP_AROUND} reference for repository #{path}: #{ex}"
     rescue Rugged::OSError => ex
       raise unless ex.message =~ /Failed to create locked file/ && ex.message =~ /File exists/
-      Rails.logger.error "Unable to create keep-around reference for repository #{path}: #{ex}"
+      Rails.logger.error "Unable to create #{REF_KEEP_AROUND} reference for repository #{path}: #{ex}"
     end
   end
 
@@ -1239,7 +1251,7 @@ class Repository
   end
 
   def keep_around_ref_name(sha)
-    "refs/keep-around/#{sha}"
+    "refs/#{REF_KEEP_AROUND}/#{sha}"
   end
 
   def repository_event(event, tags = {})
@@ -1299,4 +1311,16 @@ class Repository
       .commits_by_message(query, revision: ref, path: path, limit: limit, offset: offset)
       .map { |c| commit(c) }
   end
+
+  def with_repo_tmp_commit(start_repository, start_branch_name, sha)
+    tmp_ref = fetch_ref(
+      start_repository.path_to_repo,
+      "#{Gitlab::Git::BRANCH_REF_PREFIX}#{start_branch_name}",
+      "refs/tmp/#{SecureRandom.hex}/head"
+    )
+
+    yield commit(sha)
+  ensure
+    delete_refs(tmp_ref) if tmp_ref
+  end
 end

app/services/projects/after_import_service.rb

+module Projects
+  class AfterImportService
+    RESERVED_REFS_REGEXP =
+      %r{\Arefs/(?:#{Regexp.union(*Repository::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

app/services/projects/housekeeping_service.rb

@@ -26,6 +26,8 @@ module Projects
       lease_uuid = try_obtain_lease
       raise LeaseTaken unless lease_uuid.present?
 
+      yield if block_given?
+
       execute_gitlab_shell_gc(lease_uuid)
     end
 

changelogs/unreleased/36807-gc-unwanted-refs-after-import.yml

+---
+title: Remove unwanted refs after importing a project
+merge_request: 13766
+author:
+type: other

lib/gitlab/git/repository.rb

@@ -17,6 +17,7 @@ module Gitlab
       NoRepository = Class.new(StandardError)
       InvalidBlobName = Class.new(StandardError)
       InvalidRef = Class.new(StandardError)
+      GitError = Class.new(StandardError)
 
       class << self
         # Unlike `new`, `create` takes the storage path, not the storage name
@@ -246,6 +247,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
@@ -591,6 +599,23 @@ module Gitlab
         rugged.branches.delete(branch_name)
       end
 
+      def delete_refs(*ref_names)
+        instructions = ref_names.map do |ref|
+          "delete #{ref}\x00\x00"
+        end
+
+        command = %W[#{Gitlab.config.git.bin_path} update-ref --stdin -z]
+        message, status = Gitlab::Popen.popen(
+          command,
+          path) do |stdin|
+          stdin.write(instructions.join)
+        end
+
+        unless status.zero?
+          raise GitError.new("Could not delete refs #{ref_names}: #{message}")
+        end
+      end
+
       # Create a new branch named **ref+ based on **stat_point+, HEAD by default
       #
       # Examples:

lib/tasks/gitlab/cleanup.rake

@@ -140,7 +140,7 @@ namespace :gitlab do
           next unless id > max_iid
 
           project.deployments.find(id).create_ref
-          rugged.references.delete(ref)
+          project.repository.delete_refs(ref)
         end
       end
     end

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 }
 

spec/models/project_spec.rb

@@ -1778,10 +1778,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
@@ -1796,6 +1804,7 @@ describe Project do
 
         project.import_finish
 
+        expect(after_import_service).to have_received(:execute)
         expect(housekeeping_service).to have_received(:execute)
       end
 

spec/services/projects/after_import_service_spec.rb

+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

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