Merge branch '235192-fj-fix-repository-check-when-deleting' into 'master'

Move repo existence checking

See merge request gitlab-org/gitlab!81799

app/services/repositories/base_service.rb

@@ -18,8 +18,6 @@ class Repositories::BaseService < BaseService
   end
 
   def mv_repository(from_path, to_path)
-    return true unless repo_exists?(from_path)
-
     gitlab_shell.mv_repository(repository.shard, from_path, to_path)
   end
 

app/services/repositories/destroy_rollback_service.rb

@@ -12,8 +12,14 @@ class Repositories::DestroyRollbackService < Repositories::BaseService
       log_info(%Q{Repository "#{removal_path}" moved to "#{disk_path}" for repository "#{full_path}"})
 
       success
-    else
+    elsif repo_exists?(removal_path)
+      # If the repo does not exist, there is no need to return an
+      # error because there was nothing to do.
       move_error(removal_path)
+    else
+      success
     end
+  rescue Gitlab::Git::Repository::NoRepository
+    success
   end
 end

app/services/repositories/destroy_service.rb

@@ -30,8 +30,12 @@ class Repositories::DestroyService < Repositories::BaseService
       log_info("Repository \"#{full_path}\" was removed")
 
       success
-    else
+    elsif repo_exists?(disk_path)
       move_error(disk_path)
+    else
+      success
     end
+  rescue Gitlab::Git::Repository::NoRepository
+    success
   end
 end

spec/services/repositories/destroy_rollback_service_spec.rb

@@ -43,16 +43,19 @@ RSpec.describe Repositories::DestroyRollbackService do
     expect(repository).to receive(:disk_path).and_return('foo')
     expect(repository).not_to receive(:before_delete)
 
-    result = subject
+    expect(subject[:status]).to eq :success
+  end
 
-    expect(result[:status]).to eq :success
+  it 'gracefully handles exception if the repository does not exist on disk' do
+    expect(repository).to receive(:before_delete).and_raise(Gitlab::Git::Repository::NoRepository)
+    expect(subject[:status]).to eq :success
   end
 
   context 'when move operation cannot be performed' do
     let(:service) { described_class.new(repository) }
 
     before do
-      allow(service).to receive(:mv_repository).and_return(false)
+      expect(service).to receive(:mv_repository).and_return(false)
     end
 
     it 'returns error' do
@@ -66,6 +69,14 @@ RSpec.describe Repositories::DestroyRollbackService do
 
       service.execute
     end
+
+    context 'when repository does not exist' do
+      it 'returns success' do
+        allow(service).to receive(:repo_exists?).and_return(true, false)
+
+        expect(service.execute[:status]).to eq :success
+      end
+    end
   end
 
   def destroy_project(project, user)

spec/services/repositories/destroy_service_spec.rb

@@ -69,22 +69,23 @@ RSpec.describe Repositories::DestroyService do
     expect(repository).to receive(:disk_path).and_return('foo')
     expect(repository).not_to receive(:before_delete)
 
-    result = subject
+    expect(subject[:status]).to eq :success
+  end
 
-    expect(result[:status]).to eq :success
+  it 'gracefully handles exception if the repository does not exist on disk' do
+    expect(repository).to receive(:before_delete).and_raise(Gitlab::Git::Repository::NoRepository)
+    expect(subject[:status]).to eq :success
   end
 
   context 'when move operation cannot be performed' do
     let(:service) { described_class.new(repository) }
 
     before do
-      allow(service).to receive(:mv_repository).and_return(false)
+      expect(service).to receive(:mv_repository).and_return(false)
     end
 
     it 'returns error' do
-      result = service.execute
-
-      expect(result[:status]).to eq :error
+      expect(service.execute[:status]).to eq :error
     end
 
     it 'logs the error' do
@@ -92,6 +93,15 @@ RSpec.describe Repositories::DestroyService do
 
       service.execute
     end
+
+    context 'when repository does not exist' do
+      it 'returns success' do
+        allow(service).to receive(:repo_exists?).and_return(true, false)
+
+        expect(Repositories::ShellDestroyService).not_to receive(:new)
+        expect(service.execute[:status]).to eq :success
+      end
+    end
   end
 
   context 'with a project wiki repository' do