Move Project#rename_repo to a service classrefactor-project-rename-repo

This moves the logic of Project#rename_repo and all methods _only_ used
by this method into a new service class: Projects::AfterRenameService.
By moving this code into a separate service class we can more easily
refactor it, and we also get rid of some RuboCop "disable" statements
automatically.

During the refactoring of this code, I removed most of the explicit
logging using Gitlab::AppLogger. The data that was logged would not be
useful when debugging renaming issues, as it does not add any value on
top of data provided by users.

I also removed a variety of comments that either mentioned something the
code does in literal form, or contained various grammatical errors.
Instead we now resort to more clearly named methods, removing the need
for code comments.

This method was chosen based on analysis in
https://gitlab.com/gitlab-org/release/framework/issues/28. In this issue
we determined this method has seen a total of 293 lines being changed in
it. We also noticed that RuboCop determined the ABC size
(https://www.softwarerenovation.com/ABCMetric.pdf) was too great.

4 files changed, 218 insertions, 169 deletions

diff --git a/spec/migrations/rename_more_reserved_project_names_spec.rb b/spec/migrations/rename_more_reserved_project_names_spec.rb
index 034e8a6a4e5..baf16c2ce53 100644
--- a/spec/migrations/rename_more_reserved_project_names_spec.rb
+++ b/spec/migrations/rename_more_reserved_project_names_spec.rb
@@ -31,7 +31,16 @@ describe RenameMoreReservedProjectNames, :delete do
 
       context 'when exception is raised during rename' do
         before do
-          allow(project).to receive(:rename_repo).and_raise(StandardError)
+          service = instance_double('service')
+
+          allow(service)
+            .to receive(:execute)
+            .and_raise(Projects::AfterRenameService::RenameFailedError)
+
+          allow(Projects::AfterRenameService)
+            .to receive(:new)
+            .with(project)
+            .and_return(service)
         end
 
         it 'captures exception from project rename' do
diff --git a/spec/migrations/rename_reserved_project_names_spec.rb b/spec/migrations/rename_reserved_project_names_spec.rb
index 592ac2b5fb9..7818aa0d560 100644
--- a/spec/migrations/rename_reserved_project_names_spec.rb
+++ b/spec/migrations/rename_reserved_project_names_spec.rb
@@ -35,7 +35,16 @@ describe RenameReservedProjectNames, :migration, schema: :latest do
 
       context 'when exception is raised during rename' do
         before do
-          allow(project).to receive(:rename_repo).and_raise(StandardError)
+          service = instance_double('service')
+
+          allow(service)
+            .to receive(:execute)
+            .and_raise(Projects::AfterRenameService::RenameFailedError)
+
+          allow(Projects::AfterRenameService)
+            .to receive(:new)
+            .with(project)
+            .and_return(service)
         end
 
         it 'captures exception from project rename' do
diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb
index a807c336165..fd6b8e62b53 100644
--- a/spec/models/project_spec.rb
+++ b/spec/models/project_spec.rb
@@ -2956,88 +2956,6 @@ describe Project do
       end
     end
 
-    describe '#rename_repo' do
-      before do
-        # Project#gitlab_shell returns a new instance of Gitlab::Shell on every
-        # call. This makes testing a bit easier.
-        allow(project).to receive(:gitlab_shell).and_return(gitlab_shell)
-        allow(project).to receive(:previous_changes).and_return('path' => ['foo'])
-        stub_feature_flags(skip_hashed_storage_upgrade: false)
-      end
-
-      it 'renames a repository' do
-        stub_container_registry_config(enabled: false)
-
-        expect(gitlab_shell).to receive(:mv_repository)
-          .ordered
-          .with(project.repository_storage, "#{project.namespace.full_path}/foo", "#{project.full_path}")
-          .and_return(true)
-
-        expect(gitlab_shell).to receive(:mv_repository)
-          .ordered
-          .with(project.repository_storage, "#{project.namespace.full_path}/foo.wiki", "#{project.full_path}.wiki")
-          .and_return(true)
-
-        expect_any_instance_of(SystemHooksService)
-          .to receive(:execute_hooks_for)
-            .with(project, :rename)
-
-        expect_any_instance_of(Gitlab::UploadsTransfer)
-          .to receive(:rename_project)
-            .with('foo', project.path, project.namespace.full_path)
-
-        expect(project).to receive(:expire_caches_before_rename)
-
-        project.rename_repo
-      end
-
-      context 'container registry with images' do
-        let(:container_repository) { create(:container_repository) }
-
-        before do
-          stub_container_registry_config(enabled: true)
-          stub_container_registry_tags(repository: :any, tags: ['tag'])
-          project.container_repositories << container_repository
-        end
-
-        subject { project.rename_repo }
-
-        it { expect { subject }.to raise_error(StandardError) }
-      end
-
-      context 'gitlab pages' do
-        before do
-          expect(project_storage).to receive(:rename_repo) { true }
-        end
-
-        it 'moves pages folder to new location' do
-          expect_any_instance_of(Gitlab::PagesTransfer).to receive(:rename_project)
-
-          project.rename_repo
-        end
-      end
-
-      context 'attachments' do
-        before do
-          expect(project_storage).to receive(:rename_repo) { true }
-        end
-
-        it 'moves uploads folder to new location' do
-          expect_any_instance_of(Gitlab::UploadsTransfer).to receive(:rename_project)
-
-          project.rename_repo
-        end
-      end
-
-      it 'updates project full path in .git/config' do
-        allow(project_storage).to receive(:rename_repo).and_return(true)
-
-        project.rename_repo
-
-        expect(rugged_config['gitlab.fullpath']).to eq(project.full_path)
-      end
-    end
-
     describe '#pages_path' do
       it 'returns a path where pages are stored' do
         expect(project.pages_path).to eq(File.join(Settings.pages.path, project.namespace.full_path, project.path))
@@ -3128,91 +3046,6 @@ describe Project do
       end
     end
 
-    describe '#rename_repo' do
-      before do
-        # Project#gitlab_shell returns a new instance of Gitlab::Shell on every
-        # call. This makes testing a bit easier.
-        allow(project).to receive(:gitlab_shell).and_return(gitlab_shell)
-        allow(project).to receive(:previous_changes).and_return('path' => ['foo'])
-        stub_feature_flags(skip_hashed_storage_upgrade: false)
-      end
-
-      context 'migration to hashed storage' do
-        it 'calls HashedStorageMigrationService with correct options' do
-          project = create(:project, :repository, :legacy_storage)
-          allow(project).to receive(:previous_changes).and_return('path' => ['foo'])
-
-          expect_next_instance_of(::Projects::HashedStorageMigrationService) do |service|
-            expect(service).to receive(:execute).and_return(true)
-          end
-
-          project.rename_repo
-        end
-      end
-
-      it 'renames a repository' do
-        stub_container_registry_config(enabled: false)
-
-        expect(gitlab_shell).not_to receive(:mv_repository)
-
-        expect_any_instance_of(SystemHooksService)
-          .to receive(:execute_hooks_for)
-            .with(project, :rename)
-
-        expect(project).to receive(:expire_caches_before_rename)
-
-        project.rename_repo
-      end
-
-      context 'container registry with images' do
-        let(:container_repository) { create(:container_repository) }
-
-        before do
-          stub_container_registry_config(enabled: true)
-          stub_container_registry_tags(repository: :any, tags: ['tag'])
-          project.container_repositories << container_repository
-        end
-
-        subject { project.rename_repo }
-
-        it { expect { subject }.to raise_error(StandardError) }
-      end
-
-      context 'gitlab pages' do
-        it 'moves pages folder to new location' do
-          expect_any_instance_of(Gitlab::PagesTransfer).to receive(:rename_project)
-
-          project.rename_repo
-        end
-      end
-
-      context 'attachments' do
-        it 'keeps uploads folder location unchanged' do
-          expect_any_instance_of(Gitlab::UploadsTransfer).not_to receive(:rename_project)
-
-          project.rename_repo
-        end
-
-        context 'when not rolled out' do
-          let(:project) { create(:project, :repository, storage_version: 1, skip_disk_validation: true) }
-
-          it 'moves pages folder to hashed storage' do
-            expect_next_instance_of(Projects::HashedStorage::MigrateAttachmentsService) do |service|
-              expect(service).to receive(:execute)
-            end
-
-            project.rename_repo
-          end
-        end
-      end
-
-      it 'updates project full path in .git/config' do
-        project.rename_repo
-
-        expect(rugged_config['gitlab.fullpath']).to eq(project.full_path)
-      end
-    end
-
     describe '#pages_path' do
       it 'returns a path where pages are stored' do
         expect(project.pages_path).to eq(File.join(Settings.pages.path, project.namespace.full_path, project.path))
diff --git a/spec/services/projects/after_rename_service_spec.rb b/spec/services/projects/after_rename_service_spec.rb
new file mode 100644
index 00000000000..b4718a07204
--- /dev/null
+++ b/spec/services/projects/after_rename_service_spec.rb
@@ -0,0 +1,198 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Projects::AfterRenameService do
+  let(:rugged_config) { rugged_repo(project.repository).config }
+
+  describe '#execute' do
+    context 'using legacy storage' do
+      let(:project) { create(:project, :repository, :legacy_storage) }
+      let(:gitlab_shell) { Gitlab::Shell.new }
+      let(:project_storage) { project.send(:storage) }
+
+      before do
+        # Project#gitlab_shell returns a new instance of Gitlab::Shell on every
+        # call. This makes testing a bit easier.
+        allow(project).to receive(:gitlab_shell).and_return(gitlab_shell)
+
+        allow(project)
+          .to receive(:previous_changes)
+          .and_return('path' => ['foo'])
+
+        allow(project)
+          .to receive(:path_was)
+          .and_return('foo')
+
+        stub_feature_flags(skip_hashed_storage_upgrade: false)
+      end
+
+      it 'renames a repository' do
+        stub_container_registry_config(enabled: false)
+
+        expect(gitlab_shell).to receive(:mv_repository)
+          .ordered
+          .with(project.repository_storage, "#{project.namespace.full_path}/foo", "#{project.full_path}")
+          .and_return(true)
+
+        expect(gitlab_shell).to receive(:mv_repository)
+          .ordered
+          .with(project.repository_storage, "#{project.namespace.full_path}/foo.wiki", "#{project.full_path}.wiki")
+          .and_return(true)
+
+        expect_any_instance_of(SystemHooksService)
+          .to receive(:execute_hooks_for)
+          .with(project, :rename)
+
+        expect_any_instance_of(Gitlab::UploadsTransfer)
+          .to receive(:rename_project)
+          .with('foo', project.path, project.namespace.full_path)
+
+        expect(project).to receive(:expire_caches_before_rename)
+
+        described_class.new(project).execute
+      end
+
+      context 'container registry with images' do
+        let(:container_repository) { create(:container_repository) }
+
+        before do
+          stub_container_registry_config(enabled: true)
+          stub_container_registry_tags(repository: :any, tags: ['tag'])
+          project.container_repositories << container_repository
+        end
+
+        it 'raises a RenameFailedError' do
+          expect { described_class.new(project).execute }
+            .to raise_error(described_class::RenameFailedError)
+        end
+      end
+
+      context 'gitlab pages' do
+        before do
+          expect(project_storage).to receive(:rename_repo) { true }
+        end
+
+        it 'moves pages folder to new location' do
+          expect_any_instance_of(Gitlab::PagesTransfer).to receive(:rename_project)
+
+          described_class.new(project).execute
+        end
+      end
+
+      context 'attachments' do
+        before do
+          expect(project_storage).to receive(:rename_repo) { true }
+        end
+
+        it 'moves uploads folder to new location' do
+          expect_any_instance_of(Gitlab::UploadsTransfer).to receive(:rename_project)
+
+          described_class.new(project).execute
+        end
+      end
+
+      it 'updates project full path in .git/config' do
+        allow(project_storage).to receive(:rename_repo).and_return(true)
+
+        described_class.new(project).execute
+
+        expect(rugged_config['gitlab.fullpath']).to eq(project.full_path)
+      end
+    end
+
+    context 'using hashed storage' do
+      let(:project) { create(:project, :repository, skip_disk_validation: true) }
+      let(:gitlab_shell) { Gitlab::Shell.new }
+      let(:hash) { Digest::SHA2.hexdigest(project.id.to_s) }
+      let(:hashed_prefix) { File.join('@hashed', hash[0..1], hash[2..3]) }
+      let(:hashed_path) { File.join(hashed_prefix, hash) }
+
+      before do
+        # Project#gitlab_shell returns a new instance of Gitlab::Shell on every
+        # call. This makes testing a bit easier.
+        allow(project).to receive(:gitlab_shell).and_return(gitlab_shell)
+        allow(project).to receive(:previous_changes).and_return('path' => ['foo'])
+
+        stub_feature_flags(skip_hashed_storage_upgrade: false)
+        stub_application_setting(hashed_storage_enabled: true)
+      end
+
+      context 'migration to hashed storage' do
+        it 'calls HashedStorageMigrationService with correct options' do
+          project = create(:project, :repository, :legacy_storage)
+          allow(project).to receive(:previous_changes).and_return('path' => ['foo'])
+
+          expect_next_instance_of(::Projects::HashedStorageMigrationService) do |service|
+            expect(service).to receive(:execute).and_return(true)
+          end
+
+          described_class.new(project).execute
+        end
+      end
+
+      it 'renames a repository' do
+        stub_container_registry_config(enabled: false)
+
+        expect(gitlab_shell).not_to receive(:mv_repository)
+
+        expect_any_instance_of(SystemHooksService)
+          .to receive(:execute_hooks_for)
+          .with(project, :rename)
+
+        expect(project).to receive(:expire_caches_before_rename)
+
+        described_class.new(project).execute
+      end
+
+      context 'container registry with images' do
+        let(:container_repository) { create(:container_repository) }
+
+        before do
+          stub_container_registry_config(enabled: true)
+          stub_container_registry_tags(repository: :any, tags: ['tag'])
+          project.container_repositories << container_repository
+        end
+
+        it 'raises a RenameFailedError' do
+          expect { described_class.new(project).execute }
+            .to raise_error(described_class::RenameFailedError)
+        end
+      end
+
+      context 'gitlab pages' do
+        it 'moves pages folder to new location' do
+          expect_any_instance_of(Gitlab::PagesTransfer).to receive(:rename_project)
+
+          described_class.new(project).execute
+        end
+      end
+
+      context 'attachments' do
+        it 'keeps uploads folder location unchanged' do
+          expect_any_instance_of(Gitlab::UploadsTransfer).not_to receive(:rename_project)
+
+          described_class.new(project).execute
+        end
+
+        context 'when not rolled out' do
+          let(:project) { create(:project, :repository, storage_version: 1, skip_disk_validation: true) }
+
+          it 'moves pages folder to hashed storage' do
+            expect_next_instance_of(Projects::HashedStorage::MigrateAttachmentsService) do |service|
+              expect(service).to receive(:execute)
+            end
+
+            described_class.new(project).execute
+          end
+        end
+      end
+
+      it 'updates project full path in .git/config' do
+        described_class.new(project).execute
+
+        expect(rugged_config['gitlab.fullpath']).to eq(project.full_path)
+      end
+    end
+  end
+end