Merge branch 'repo-remove' into fix-group-remove

Signed-off-by: Dmitriy Zaporozhets <dmitriy.zaporozhets@gmail.com>

Conflicts:
	spec/features/projects_spec.rb

7 files changed, 106 insertions, 41 deletions

diff --git a/CHANGELOG b/CHANGELOG
index 85fa933fa6b..455b4dcf960 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -41,6 +41,7 @@ v 7.12.0 (unreleased)
   - Add an option to automatically sign-in with an Omniauth provider
   - Better performance for web editor (switched from satellites to rugged)
   - GitLab CI service sends .gitlab-ci.yaml in each push call
+  - When remove project - move repository and schedule it removal
 
 v 7.11.4
   - Fix missing bullets when creating lists
diff --git a/app/controllers/projects_controller.rb b/app/controllers/projects_controller.rb
index dc430351551..4ca5fc65459 100644
--- a/app/controllers/projects_controller.rb
+++ b/app/controllers/projects_controller.rb
@@ -97,18 +97,15 @@ class ProjectsController < ApplicationController
     return access_denied! unless can?(current_user, :remove_project, @project)
 
     ::Projects::DestroyService.new(@project, current_user, {}).execute
+    flash[:alert] = 'Project deleted.'
 
-    respond_to do |format|
-      format.html do
-        flash[:alert] = 'Project deleted.'
-
-        if request.referer.include?('/admin')
-          redirect_to admin_namespaces_projects_path
-        else
-          redirect_to dashboard_path
-        end
-      end
+    if request.referer.include?('/admin')
+      redirect_to admin_namespaces_projects_path
+    else
+      redirect_to dashboard_path
     end
+  rescue Projects::DestroyService::DestroyError => ex
+    redirect_to edit_project_path(@project), alert: ex.message
   end
 
   def autocomplete_sources
diff --git a/app/services/projects/destroy_service.rb b/app/services/projects/destroy_service.rb
index 7e1d753b021..29e8ba347d4 100644
--- a/app/services/projects/destroy_service.rb
+++ b/app/services/projects/destroy_service.rb
@@ -1,28 +1,67 @@
 module Projects
   class DestroyService < BaseService
+    include Gitlab::ShellAdapter
+
+    class DestroyError < StandardError; end
+
+    DELETED_FLAG = '+deleted'
+
     def execute
       return false unless can?(current_user, :remove_project, project)
 
       project.team.truncate
       project.repository.expire_cache unless project.empty_repo?
 
-      if project.destroy
-        GitlabShellWorker.perform_async(
-          :remove_repository,
-          project.path_with_namespace
-        )
+      repo_path = project.path_with_namespace
+      wiki_path = repo_path + '.wiki'
+
+      Project.transaction do
+        project.destroy!
 
-        GitlabShellWorker.perform_async(
-          :remove_repository,
-          project.path_with_namespace + ".wiki"
-        )
+        unless remove_repository(repo_path)
+          raise_error('Failed to remove project repository. Please try again or contact administrator')
+        end
+
+        unless remove_repository(wiki_path)
+          raise_error('Failed to remove wiki repository. Please try again or contact administrator')
+        end
+      end
+
+      project.satellite.destroy
+      log_info("Project \"#{project.name}\" was removed")
+      system_hook_service.execute_hooks_for(project, :destroy)
+      true
+    end
 
-        project.satellite.destroy
+    private
 
-        log_info("Project \"#{project.name}\" was removed")
-        system_hook_service.execute_hooks_for(project, :destroy)
-        true
+    def remove_repository(path)
+      unless gitlab_shell.exists?(path + '.git')
+        return true
       end
+
+      new_path = removal_path(path)
+
+      if gitlab_shell.mv_repository(path, new_path)
+        log_info("Repository \"#{path}\" moved to \"#{new_path}\"")
+        GitlabShellWorker.perform_in(5.minutes, :remove_repository, new_path)
+      else
+        false
+      end
+    end
+
+    def raise_error(message)
+      raise DestroyError.new(message)
+    end
+
+    # Build a path for removing repositories
+    # We use `+` because its not allowed by GitLab so user can not create
+    # project with name cookies+119+deleted and capture someone stalled repository
+    #
+    # gitlab/cookies.git -> gitlab/cookies+119+deleted.git
+    #
+    def removal_path(path)
+      "#{path}+#{project.id}#{DELETED_FLAG}"
     end
   end
 end
diff --git a/lib/gitlab/backend/shell.rb b/lib/gitlab/backend/shell.rb
index 530f9d93de4..172d4902add 100644
--- a/lib/gitlab/backend/shell.rb
+++ b/lib/gitlab/backend/shell.rb
@@ -244,6 +244,16 @@ module Gitlab
       end
     end
 
+    # Check if such directory exists in repositories.
+    #
+    # Usage:
+    #   exists?('gitlab')
+    #   exists?('gitlab/cookies.git')
+    #
+    def exists?(dir_name)
+      File.exists?(full_path(dir_name))
+    end
+
     protected
 
     def gitlab_shell_path
@@ -264,10 +274,6 @@ module Gitlab
       File.join(repos_path, dir_name)
     end
 
-    def exists?(dir_name)
-      File.exists?(full_path(dir_name))
-    end
-
     def gitlab_shell_projects_path
       File.join(gitlab_shell_path, 'bin', 'gitlab-projects')
     end
diff --git a/spec/features/projects_spec.rb b/spec/features/projects_spec.rb
index 56523f6e1a8..f8eea70ec4a 100644
--- a/spec/features/projects_spec.rb
+++ b/spec/features/projects_spec.rb
@@ -47,13 +47,6 @@ feature 'Project' do
     it 'should remove project' do
       expect { remove_project }.to change {Project.count}.by(-1)
     end
-
-    it 'should delete the project from disk' do
-      expect(GitlabShellWorker).to receive(:perform_async).
-        with(:remove_repository, /#{project.path_with_namespace}/).twice
-
-      remove_project
-    end
   end
 
   def remove_project
diff --git a/spec/requests/api/projects_spec.rb b/spec/requests/api/projects_spec.rb
index 46cd26eb927..dbfd72e5f19 100644
--- a/spec/requests/api/projects_spec.rb
+++ b/spec/requests/api/projects_spec.rb
@@ -57,14 +57,14 @@ describe API::API, api: true  do
         expect(json_response.first['name']).to eq(project.name)
         expect(json_response.first['owner']['username']).to eq(user.username)
       end
-      
+
       it 'should include the project labels as the tag_list' do
         get api('/projects', user)
         response.status.should == 200
         json_response.should be_an Array
         json_response.first.keys.should include('tag_list')
       end
-      
+
       context 'and using search' do
         it 'should return searched project' do
           get api('/projects', user), { search: project.name }
@@ -792,11 +792,6 @@ describe API::API, api: true  do
   describe 'DELETE /projects/:id' do
     context 'when authenticated as user' do
       it 'should remove project' do
-        expect(GitlabShellWorker).to(
-          receive(:perform_async).with(:remove_repository,
-                                       /#{project.path_with_namespace}/)
-        ).twice
-
         delete api("/projects/#{project.id}", user)
         expect(response.status).to eq(200)
       end
diff --git a/spec/services/projects/destroy_service_spec.rb b/spec/services/projects/destroy_service_spec.rb
new file mode 100644
index 00000000000..cdf576cc0c1
--- /dev/null
+++ b/spec/services/projects/destroy_service_spec.rb
@@ -0,0 +1,34 @@
+require 'spec_helper'
+
+describe Projects::DestroyService do
+  let!(:user) { create(:user) }
+  let!(:project) { create(:project, namespace: user.namespace) }
+  let!(:path) { project.repository.path_to_repo }
+  let!(:remove_path) { path.sub(/\.git\Z/, "+#{project.id}+deleted.git") }
+
+  context 'Sidekiq inline' do
+    before do
+      # Run sidekiq immediatly to check that renamed repository will be removed
+      Sidekiq::Testing.inline! { destroy_project(project, user, {}) }
+    end
+
+    it { Project.all.should_not include(project) }
+    it { Dir.exists?(path).should be_falsey }
+    it { Dir.exists?(remove_path).should be_falsey }
+  end
+
+  context 'Sidekiq fake' do
+    before do
+      # Dont run sidekiq to check if renamed repository exists
+      Sidekiq::Testing.fake! { destroy_project(project, user, {}) }
+    end
+
+    it { Project.all.should_not include(project) }
+    it { Dir.exists?(path).should be_falsey }
+    it { Dir.exists?(remove_path).should be_truthy }
+  end
+
+  def destroy_project(project, user, params)
+    Projects::DestroyService.new(project, user, params).execute
+  end
+end