Merge branch '40711-fix-forking-hashed-projects' into 'master'

Fix the fork project functionality for projects with hashed storage

Closes #40711

See merge request gitlab-org/gitlab-ce!15671

8 files changed, 61 insertions, 72 deletions

diff --git a/GITLAB_SHELL_VERSION b/GITLAB_SHELL_VERSION
index c5b7013b9c5..509b0b618ad 100644
--- a/GITLAB_SHELL_VERSION
+++ b/GITLAB_SHELL_VERSION
@@ -1 +1 @@
-5.9.4
+5.10.0
diff --git a/app/models/project.rb b/app/models/project.rb
index eaf4f555d3b..f9c82029912 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -562,8 +562,7 @@ class Project < ActiveRecord::Base
       if forked?
         RepositoryForkWorker.perform_async(id,
                                            forked_from_project.repository_storage_path,
-                                           forked_from_project.full_path,
-                                           self.namespace.full_path)
+                                           forked_from_project.disk_path)
       else
         RepositoryImportWorker.perform_async(self.id)
       end
diff --git a/app/workers/repository_fork_worker.rb b/app/workers/repository_fork_worker.rb
index 264706e3e23..001c11b73e1 100644
--- a/app/workers/repository_fork_worker.rb
+++ b/app/workers/repository_fork_worker.rb
@@ -8,18 +8,18 @@ class RepositoryForkWorker
 
   sidekiq_options status_expiration: StuckImportJobsWorker::IMPORT_JOBS_EXPIRATION
 
-  def perform(project_id, forked_from_repository_storage_path, source_path, target_path)
+  def perform(project_id, forked_from_repository_storage_path, source_disk_path)
     project = Project.find(project_id)
 
     return unless start_fork(project)
 
     Gitlab::Metrics.add_event(:fork_repository,
-                              source_path: source_path,
-                              target_path: target_path)
+                              source_path: source_disk_path,
+                              target_path: project.disk_path)
 
-    result = gitlab_shell.fork_repository(forked_from_repository_storage_path, source_path,
-                                          project.repository_storage_path, target_path)
-    raise ForkError, "Unable to fork project #{project_id} for repository #{source_path} -> #{target_path}" unless result
+    result = gitlab_shell.fork_repository(forked_from_repository_storage_path, source_disk_path,
+                                          project.repository_storage_path, project.disk_path)
+    raise ForkError, "Unable to fork project #{project_id} for repository #{source_disk_path} -> #{project.disk_path}" unless result
 
     project.repository.after_import
     raise ForkError, "Project #{project_id} had an invalid repository after fork" unless project.valid_repo?
diff --git a/changelogs/unreleased/40711-fix-forking-hashed-projects.yml b/changelogs/unreleased/40711-fix-forking-hashed-projects.yml
new file mode 100644
index 00000000000..116d7d4e9cf
--- /dev/null
+++ b/changelogs/unreleased/40711-fix-forking-hashed-projects.yml
@@ -0,0 +1,5 @@
+---
+title: Fix the fork project functionality for projects with hashed storage
+merge_request: 15671
+author:
+type: fixed
diff --git a/lib/gitlab/shell.rb b/lib/gitlab/shell.rb
index 996d213fdb4..a22a63665be 100644
--- a/lib/gitlab/shell.rb
+++ b/lib/gitlab/shell.rb
@@ -143,20 +143,27 @@ module Gitlab
                                  storage, "#{path}.git", "#{new_path}.git"])
     end
 
-    # Fork repository to new namespace
+    # Fork repository to new path
     # forked_from_storage - forked-from project's storage path
-    # path - project path with namespace
+    # forked_from_disk_path - project disk path
     # forked_to_storage - forked-to project's storage path
-    # fork_namespace - namespace for forked project
+    # forked_to_disk_path - forked project disk path
     #
     # Ex.
-    #  fork_repository("/path/to/forked_from/storage", "gitlab/gitlab-ci", "/path/to/forked_to/storage", "randx")
+    #  fork_repository("/path/to/forked_from/storage", "gitlab/gitlab-ci", "/path/to/forked_to/storage", "new-namespace/gitlab-ci")
     #
     # Gitaly note: JV: not easy to migrate because this involves two Gitaly servers, not one.
-    def fork_repository(forked_from_storage, path, forked_to_storage, fork_namespace)
-      gitlab_shell_fast_execute([gitlab_shell_projects_path, 'fork-project',
-                                 forked_from_storage, "#{path}.git", forked_to_storage,
-                                 fork_namespace])
+    def fork_repository(forked_from_storage, forked_from_disk_path, forked_to_storage, forked_to_disk_path)
+      gitlab_shell_fast_execute(
+        [
+          gitlab_shell_projects_path,
+          'fork-repository',
+          forked_from_storage,
+          "#{forked_from_disk_path}.git",
+          forked_to_storage,
+          "#{forked_to_disk_path}.git"
+        ]
+      )
     end
 
     # Remove repository from file system
diff --git a/spec/lib/gitlab/shell_spec.rb b/spec/lib/gitlab/shell_spec.rb
index 2158b2837e2..eec6858a5de 100644
--- a/spec/lib/gitlab/shell_spec.rb
+++ b/spec/lib/gitlab/shell_spec.rb
@@ -200,18 +200,18 @@ describe Gitlab::Shell do
     describe '#fork_repository' do
       it 'returns true when the command succeeds' do
         expect(Gitlab::Popen).to receive(:popen)
-          .with([projects_path, 'fork-project', 'current/storage', 'project/path.git', 'new/storage', 'new-namespace'],
+          .with([projects_path, 'fork-repository', 'current/storage', 'project/path.git', 'new/storage', 'fork/path.git'],
                 nil, popen_vars).and_return([nil, 0])
 
-        expect(gitlab_shell.fork_repository('current/storage', 'project/path', 'new/storage', 'new-namespace')).to be true
+        expect(gitlab_shell.fork_repository('current/storage', 'project/path', 'new/storage', 'fork/path')).to be true
       end
 
       it 'return false when the command fails' do
         expect(Gitlab::Popen).to receive(:popen)
-          .with([projects_path, 'fork-project', 'current/storage', 'project/path.git', 'new/storage', 'new-namespace'],
+          .with([projects_path, 'fork-repository', 'current/storage', 'project/path.git', 'new/storage', 'fork/path.git'],
                 nil, popen_vars).and_return(["error", 1])
 
-        expect(gitlab_shell.fork_repository('current/storage', 'project/path', 'new/storage', 'new-namespace')).to be false
+        expect(gitlab_shell.fork_repository('current/storage', 'project/path', 'new/storage', 'fork/path')).to be false
       end
     end
 
diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb
index 521b7bd70ba..34e160aa599 100644
--- a/spec/models/project_spec.rb
+++ b/spec/models/project_spec.rb
@@ -1717,8 +1717,7 @@ describe Project do
         expect(RepositoryForkWorker).to receive(:perform_async).with(
           project.id,
           forked_from_project.repository_storage_path,
-          forked_from_project.disk_path,
-          project.namespace.full_path).and_return(import_jid)
+          forked_from_project.disk_path).and_return(import_jid)
 
         expect(project.add_import_job).to eq(import_jid)
       end
diff --git a/spec/workers/repository_fork_worker_spec.rb b/spec/workers/repository_fork_worker_spec.rb
index e881ec37ae5..74c85848b7e 100644
--- a/spec/workers/repository_fork_worker_spec.rb
+++ b/spec/workers/repository_fork_worker_spec.rb
@@ -1,8 +1,8 @@
 require 'spec_helper'
 
 describe RepositoryForkWorker do
-  let(:project) { create(:project, :repository, :import_scheduled) }
-  let(:fork_project) { create(:project, :repository, forked_from_project: project) }
+  let(:project) { create(:project, :repository) }
+  let(:fork_project) { create(:project, :repository, :import_scheduled, forked_from_project: project) }
   let(:shell) { Gitlab::Shell.new }
 
   subject { described_class.new }
@@ -12,50 +12,39 @@ describe RepositoryForkWorker do
   end
 
   describe "#perform" do
+    def perform!
+      subject.perform(fork_project.id, '/test/path', project.disk_path)
+    end
+
+    def expect_fork_repository
+      expect(shell).to receive(:fork_repository).with(
+        '/test/path',
+        project.disk_path,
+        fork_project.repository_storage_path,
+        fork_project.disk_path
+      )
+    end
+
     describe 'when a worker was reset without cleanup' do
       let(:jid) { '12345678' }
-      let(:started_project) { create(:project, :repository, :import_started) }
 
       it 'creates a new repository from a fork' do
         allow(subject).to receive(:jid).and_return(jid)
 
-        expect(shell).to receive(:fork_repository).with(
-          '/test/path',
-          project.full_path,
-          project.repository_storage_path,
-          fork_project.namespace.full_path
-        ).and_return(true)
-
-        subject.perform(
-          project.id,
-          '/test/path',
-          project.full_path,
-          fork_project.namespace.full_path)
+        expect_fork_repository.and_return(true)
+
+        perform!
       end
     end
 
     it "creates a new repository from a fork" do
-      expect(shell).to receive(:fork_repository).with(
-        '/test/path',
-        project.full_path,
-        project.repository_storage_path,
-        fork_project.namespace.full_path
-      ).and_return(true)
+      expect_fork_repository.and_return(true)
 
-      subject.perform(
-        project.id,
-        '/test/path',
-        project.full_path,
-        fork_project.namespace.full_path)
+      perform!
     end
 
     it 'flushes various caches' do
-      expect(shell).to receive(:fork_repository).with(
-        '/test/path',
-        project.full_path,
-        project.repository_storage_path,
-        fork_project.namespace.full_path
-      ).and_return(true)
+      expect_fork_repository.and_return(true)
 
       expect_any_instance_of(Repository).to receive(:expire_emptiness_caches)
         .and_call_original
@@ -63,32 +52,22 @@ describe RepositoryForkWorker do
       expect_any_instance_of(Repository).to receive(:expire_exists_cache)
         .and_call_original
 
-      subject.perform(project.id, '/test/path', project.full_path,
-                      fork_project.namespace.full_path)
+      perform!
     end
 
     it "handles bad fork" do
-      source_path = project.full_path
-      target_path = fork_project.namespace.full_path
-      error_message = "Unable to fork project #{project.id} for repository #{source_path} -> #{target_path}"
+      error_message = "Unable to fork project #{fork_project.id} for repository #{project.full_path} -> #{fork_project.full_path}"
 
-      expect(shell).to receive(:fork_repository).and_return(false)
+      expect_fork_repository.and_return(false)
 
-      expect do
-        subject.perform(project.id, '/test/path', source_path, target_path)
-      end.to raise_error(RepositoryForkWorker::ForkError, error_message)
+      expect { perform! }.to raise_error(RepositoryForkWorker::ForkError, error_message)
     end
 
     it 'handles unexpected error' do
-      source_path = project.full_path
-      target_path = fork_project.namespace.full_path
-
-      allow_any_instance_of(Gitlab::Shell).to receive(:fork_repository).and_raise(RuntimeError)
+      expect_fork_repository.and_raise(RuntimeError)
 
-      expect do
-        subject.perform(project.id, '/test/path', source_path, target_path)
-      end.to raise_error(RepositoryForkWorker::ForkError)
-      expect(project.reload.import_status).to eq('failed')
+      expect { perform! }.to raise_error(RepositoryForkWorker::ForkError)
+      expect(fork_project.reload.import_status).to eq('failed')
     end
   end
 end