Remove GitlabShell#create_repository and wrappers

Project repository and wiki repository creation was indirected through
Gitlab::Shell unnecessarily. `GitlabShell#create_repository` build an
(incomplete) `Gitlab::Git::Repository` to run the creation code, but
all users already had access to `Repository` instances.

app/models/project.rb

@@ -1460,13 +1460,14 @@ class Project < ApplicationRecord
     # Forked import is handled asynchronously
     return if forked? && !force
 
-    if gitlab_shell.create_project_repository(self)
-      repository.after_create
-      true
-    else
-      errors.add(:base, _('Failed to create repository via gitlab-shell'))
-      false
-    end
+    repository.create_repository
+    repository.after_create
+
+    true
+  rescue => err
+    Gitlab::ErrorTracking.track_exception(err, project: { id: id, full_path: full_path, disk_path: disk_path })
+    errors.add(:base, _('Failed to create repository'))
+    false
   end
 
   def hook_attrs(backward: true)

app/models/project_wiki.rb

@@ -2,6 +2,7 @@
 
 class ProjectWiki
   include Storage::LegacyProjectWiki
+  include Gitlab::Utils::StrongMemoize
 
   MARKUPS = {
     'Markdown' => :markdown,
@@ -63,14 +64,15 @@ class ProjectWiki
 
   # Returns the Gitlab::Git::Wiki object.
   def wiki
-    @wiki ||= begin
-      gl_repository = Gitlab::GlRepository::WIKI.identifier_for_container(project)
-      raw_repository = Gitlab::Git::Repository.new(project.repository_storage, disk_path + '.git', gl_repository, full_path)
+    strong_memoize(:wiki) do
+      repository.create_if_not_exists
+      raise CouldNotCreateWikiError unless repository_exists?
 
-      create_repo!(raw_repository) unless raw_repository.exists?
-
-      Gitlab::Git::Wiki.new(raw_repository)
+      Gitlab::Git::Wiki.new(repository.raw)
     end
+  rescue => err
+    Gitlab::ErrorTracking.track_exception(err, project_wiki: { project_id: project.id, full_path: full_path, disk_path: disk_path })
+    raise CouldNotCreateWikiError
   end
 
   def repository_exists?
@@ -192,14 +194,6 @@ class ProjectWiki
 
   private
 
-  def create_repo!(raw_repository)
-    gitlab_shell.create_wiki_repository(project)
-
-    raise CouldNotCreateWikiError unless raw_repository.exists?
-
-    repository.after_create
-  end
-
   def commit_details(action, message = nil, title = nil)
     commit_message = message.presence || default_message(action, title)
     git_user = Gitlab::Git::User.from_gitlab(@user)

app/services/projects/update_repository_storage_service.rb

@@ -52,11 +52,14 @@ module Projects
       checksum = repository.checksum
 
       # Initialize a git repository on the target path
-      gitlab_shell.create_repository(new_storage_key, raw_repository.relative_path, full_path)
-      new_repository = Gitlab::Git::Repository.new(new_storage_key,
-                                                   raw_repository.relative_path,
-                                                   raw_repository.gl_repository,
-                                                   full_path)
+      new_repository = Gitlab::Git::Repository.new(
+        new_storage_key,
+        raw_repository.relative_path,
+        raw_repository.gl_repository,
+        full_path
+      )
+
+      new_repository.create_repository
 
       new_repository.replicate(raw_repository)
       new_checksum = new_repository.checksum

ee/app/services/geo/repository_base_sync_service.rb

@@ -72,10 +72,7 @@ module Geo
       log_info("Attempting to fetch repository via git")
 
       # `git fetch` needs an empty bare repository to fetch into
-      unless gitlab_shell.create_repository(project.repository_storage, disk_path_temp, project.full_path)
-        raise Gitlab::Shell::Error, 'Can not create a temporary repository'
-      end
-
+      temp_repo.create_repository
       fetch_geo_mirror(temp_repo)
 
       set_temp_repository_as_main

lib/backup/repository.rb

@@ -4,7 +4,6 @@ require 'yaml'
 
 module Backup
   class Repository
-    include Gitlab::ShellAdapter
     attr_reader :progress
 
     def initialize(progress)
@@ -71,23 +70,14 @@ module Backup
     def restore
       Project.find_each(batch_size: 1000) do |project|
         progress.print " * #{project.full_path} ... "
-        path_to_project_bundle = path_to_bundle(project)
 
-        project.repository.remove rescue nil
-        restore_repo_success = nil
-
-        if File.exist?(path_to_project_bundle)
+        restore_repo_success =
           begin
-            project.repository.create_from_bundle(path_to_project_bundle)
-            restore_custom_hooks(project)
-            restore_repo_success = true
-          rescue => e
-            restore_repo_success = false
-            progress.puts "Error: #{e}".color(:red)
+            try_restore_repository(project)
+          rescue => err
+            progress.puts "Error: #{err}".color(:red)
+            false
           end
-        else
-          restore_repo_success = gitlab_shell.create_project_repository(project)
-        end
 
         if restore_repo_success
           progress.puts "[DONE]".color(:green)
@@ -118,6 +108,20 @@ module Backup
 
     protected
 
+    def try_restore_repository(project)
+      path_to_project_bundle = path_to_bundle(project)
+      project.repository.remove rescue nil
+
+      if File.exist?(path_to_project_bundle)
+        project.repository.create_from_bundle(path_to_project_bundle)
+        restore_custom_hooks(project)
+      else
+        project.repository.create_repository
+      end
+
+      true
+    end
+
     def path_to_bundle(project)
       File.join(backup_repos_path, project.disk_path + '.bundle')
     end

lib/gitlab/shell.rb

@@ -77,47 +77,6 @@ module Gitlab
       end
     end
 
-    # Initialize a new project repository using a Project model
-    #
-    # @param [Project] project
-    # @return [Boolean] whether repository could be created
-    def create_project_repository(project)
-      create_repository(project.repository_storage, project.disk_path, project.full_path)
-    end
-
-    # Initialize a new wiki repository using a Project model
-    #
-    # @param [Project] project
-    # @return [Boolean] whether repository could be created
-    def create_wiki_repository(project)
-      create_repository(project.repository_storage, project.wiki.disk_path, project.wiki.full_path)
-    end
-
-    # Init new repository
-    #
-    # @example Create a repository
-    #   create_repository("default", "path/to/gitlab-ci", "gitlab/gitlab-ci")
-    #
-    # @param [String] storage the shard key
-    # @param [String] disk_path project path on disk
-    # @param [String] gl_project_path project name
-    # @return [Boolean] whether repository could be created
-    def create_repository(storage, disk_path, gl_project_path)
-      relative_path = disk_path.dup
-      relative_path << '.git' unless relative_path.end_with?('.git')
-
-      # During creation of a repository, gl_repository may not be known
-      # because that depends on a yet-to-be assigned project ID in the
-      # database (e.g. project-1234), so for now it is blank.
-      repository = Gitlab::Git::Repository.new(storage, relative_path, '', gl_project_path)
-      wrapped_gitaly_errors { repository.gitaly_repository_client.create_repository }
-
-      true
-    rescue => err # Once the Rugged codes gets removes this can be improved
-      Rails.logger.error("Failed to add repository #{storage}/#{disk_path}: #{err}") # rubocop:disable Gitlab/RailsLogger
-      false
-    end
-
     # Import wiki repository from external service
     #
     # @param [Project] project

locale/gitlab.pot

@@ -8348,7 +8348,7 @@ msgstr ""
 msgid "Failed to create a branch for this issue. Please try again."
 msgstr ""
 
-msgid "Failed to create repository via gitlab-shell"
+msgid "Failed to create repository"
 msgstr ""
 
 msgid "Failed to create resources"

spec/lib/backup/repository_spec.rb

@@ -50,9 +50,9 @@ describe Backup::Repository do
 
     describe 'command failure' do
       before do
-        allow_next_instance_of(Gitlab::Shell) do |instance|
-          allow(instance).to receive(:create_repository).and_return(false)
-        end
+        # Allow us to set expectations on the project directly
+        expect(Project).to receive(:find_each).and_yield(project)
+        expect(project.repository).to receive(:create_repository) { raise 'Fail in tests' }
       end
 
       context 'hashed storage' do

spec/lib/gitlab/shell_spec.rb

@@ -8,7 +8,6 @@ describe Gitlab::Shell do
   let(:repository) { project.repository }
   let(:gitlab_shell) { described_class.new }
 
-  it { is_expected.to respond_to :create_repository }
   it { is_expected.to respond_to :remove_repository }
   it { is_expected.to respond_to :fork_repository }
 
@@ -55,30 +54,6 @@ describe Gitlab::Shell do
       allow(Gitlab.config.gitlab_shell).to receive(:git_timeout).and_return(800)
     end
 
-    describe '#create_repository' do
-      let(:repository_storage) { 'default' }
-      let(:repository_storage_path) do
-        Gitlab::GitalyClient::StorageSettings.allow_disk_access do
-          Gitlab.config.repositories.storages[repository_storage].legacy_disk_path
-        end
-      end
-      let(:repo_name) { 'project/path' }
-      let(:created_path) { File.join(repository_storage_path, repo_name + '.git') }
-
-      after do
-        FileUtils.rm_rf(created_path)
-      end
-
-      it 'returns false when the command fails' do
-        FileUtils.mkdir_p(File.dirname(created_path))
-        # This file will block the creation of the repo's .git directory. That
-        # should cause #create_repository to fail.
-        FileUtils.touch(created_path)
-
-        expect(gitlab_shell.create_repository(repository_storage, repo_name, repo_name)).to be_falsy
-      end
-    end
-
     describe '#remove_repository' do
       let!(:project) { create(:project, :repository, :legacy_storage) }
       let(:disk_path) { "#{project.disk_path}.git" }

spec/models/project_spec.rb

@@ -1921,30 +1921,15 @@ describe Project do
 
   describe '#create_repository' do
     let(:project) { create(:project, :repository) }
-    let(:shell) { Gitlab::Shell.new }
-
-    before do
-      allow(project).to receive(:gitlab_shell).and_return(shell)
-    end
 
     context 'using a regular repository' do
       it 'creates the repository' do
-        expect(shell).to receive(:create_repository)
-          .with(project.repository_storage, project.disk_path, project.full_path)
-          .and_return(true)
-
-        expect(project.repository).to receive(:after_create)
-
+        expect(project.repository).to receive(:create_repository)
         expect(project.create_repository).to eq(true)
       end
 
       it 'adds an error if the repository could not be created' do
-        expect(shell).to receive(:create_repository)
-          .with(project.repository_storage, project.disk_path, project.full_path)
-          .and_return(false)
-
-        expect(project.repository).not_to receive(:after_create)
-
+        expect(project.repository).to receive(:create_repository) { raise 'Fail in test' }
         expect(project.create_repository).to eq(false)
         expect(project.errors).not_to be_empty
       end
@@ -1953,7 +1938,7 @@ describe Project do
     context 'using a forked repository' do
       it 'does nothing' do
         expect(project).to receive(:forked?).and_return(true)
-        expect(shell).not_to receive(:create_repository)
+        expect(project.repository).not_to receive(:create_repository)
 
         project.create_repository
       end
@@ -1962,28 +1947,16 @@ describe Project do
 
   describe '#ensure_repository' do
     let(:project) { create(:project, :repository) }
-    let(:shell) { Gitlab::Shell.new }
-
-    before do
-      allow(project).to receive(:gitlab_shell).and_return(shell)
-    end
 
     it 'creates the repository if it not exist' do
-      allow(project).to receive(:repository_exists?)
-        .and_return(false)
-
-      allow(shell).to receive(:create_repository)
-        .with(project.repository_storage, project.disk_path, project.full_path)
-        .and_return(true)
-
+      allow(project).to receive(:repository_exists?).and_return(false)
       expect(project).to receive(:create_repository).with(force: true)
 
       project.ensure_repository
     end
 
     it 'does not create the repository if it exists' do
-      allow(project).to receive(:repository_exists?)
-        .and_return(true)
+      allow(project).to receive(:repository_exists?).and_return(true)
 
       expect(project).not_to receive(:create_repository)
 
@@ -1992,13 +1965,8 @@ describe Project do
 
     it 'creates the repository if it is a fork' do
       expect(project).to receive(:forked?).and_return(true)
-
-      allow(project).to receive(:repository_exists?)
-        .and_return(false)
-
-      expect(shell).to receive(:create_repository)
-        .with(project.repository_storage, project.disk_path, project.full_path)
-        .and_return(true)
+      expect(project).to receive(:repository_exists?).and_return(false)
+      expect(project.repository).to receive(:create_repository) { true }
 
       project.ensure_repository
     end

spec/models/project_wiki_spec.rb

@@ -97,9 +97,7 @@ describe ProjectWiki do
     it "raises CouldNotCreateWikiError if it can't create the wiki repository" do
       # Create a fresh project which will not have a wiki
       project_wiki = described_class.new(create(:project), user)
-      gitlab_shell = double(:gitlab_shell)
-      allow(gitlab_shell).to receive(:create_wiki_repository)
-      allow(project_wiki).to receive(:gitlab_shell).and_return(gitlab_shell)
+      expect(project_wiki.repository).to receive(:create_if_not_exists) { false }
 
       expect { project_wiki.send(:wiki) }.to raise_exception(ProjectWiki::CouldNotCreateWikiError)
     end
@@ -416,26 +414,12 @@ describe ProjectWiki do
     end
   end
 
-  describe '#create_repo!' do
-    let(:project) { create(:project) }
-
-    it 'creates a repository' do
-      expect(raw_repository.exists?).to eq(false)
-      expect(subject.repository).to receive(:after_create)
-
-      subject.send(:create_repo!, raw_repository)
-
-      expect(raw_repository.exists?).to eq(true)
-    end
-  end
-
   describe '#ensure_repository' do
     let(:project) { create(:project) }
 
     it 'creates the repository if it not exist' do
       expect(raw_repository.exists?).to eq(false)
 
-      expect(subject).to receive(:create_repo!).and_call_original
       subject.ensure_repository
 
       expect(raw_repository.exists?).to eq(true)

spec/services/projects/fork_service_spec.rb

@@ -312,6 +312,8 @@ describe Projects::ForkService do
 
       # Stub everything required to move a project to a Gitaly shard that does not exist
       stub_storage_settings('test_second_storage' => { 'path' => TestEnv::SECOND_STORAGE_PATH })
+      allow_any_instance_of(Gitlab::Git::Repository).to receive(:create_repository)
+        .and_return(true)
       allow_any_instance_of(Gitlab::Git::Repository).to receive(:replicate)
       allow_any_instance_of(Gitlab::Git::Repository).to receive(:checksum)
         .and_return(::Gitlab::Git::BLANK_SHA)

spec/services/projects/update_repository_storage_service_spec.rb

@@ -32,6 +32,8 @@ describe Projects::UpdateRepositoryStorageService do
             project.repository.path_to_repo
           end
 
+          expect(project_repository_double).to receive(:create_repository)
+            .and_return(true)
           expect(project_repository_double).to receive(:replicate)
             .with(project.repository.raw)
           expect(project_repository_double).to receive(:checksum)
@@ -58,6 +60,8 @@ describe Projects::UpdateRepositoryStorageService do
 
       context 'when the move fails' do
         it 'unmarks the repository as read-only without updating the repository storage' do
+          expect(project_repository_double).to receive(:create_repository)
+            .and_return(true)
           expect(project_repository_double).to receive(:replicate)
             .with(project.repository.raw)
             .and_raise(Gitlab::Git::CommandError)
@@ -73,6 +77,8 @@ describe Projects::UpdateRepositoryStorageService do
 
       context 'when the checksum does not match' do
         it 'unmarks the repository as read-only without updating the repository storage' do
+          expect(project_repository_double).to receive(:create_repository)
+            .and_return(true)
           expect(project_repository_double).to receive(:replicate)
             .with(project.repository.raw)
           expect(project_repository_double).to receive(:checksum)
@@ -91,6 +97,8 @@ describe Projects::UpdateRepositoryStorageService do
         let!(:pool) { create(:pool_repository, :ready, source_project: project) }
 
         it 'leaves the pool' do
+          expect(project_repository_double).to receive(:create_repository)
+            .and_return(true)
           expect(project_repository_double).to receive(:replicate)
             .with(project.repository.raw)
           expect(project_repository_double).to receive(:checksum)

spec/support/shared_examples/services/projects/update_repository_storage_service_shared_examples.rb

@@ -22,11 +22,15 @@ RSpec.shared_examples 'moves repository to another storage' do |repository_type|
 
   context 'when the move succeeds', :clean_gitlab_redis_shared_state do
     before do
+      allow(project_repository_double).to receive(:create_repository)
+        .and_return(true)
       allow(project_repository_double).to receive(:replicate)
         .with(project.repository.raw)
       allow(project_repository_double).to receive(:checksum)
         .and_return(project_repository_checksum)
 
+      allow(repository_double).to receive(:create_repository)
+        .and_return(true)
       allow(repository_double).to receive(:replicate)
         .with(repository.raw)
       allow(repository_double).to receive(:checksum)
@@ -90,11 +94,15 @@ RSpec.shared_examples 'moves repository to another storage' do |repository_type|
 
   context "when the move of the #{repository_type} repository fails" do
     it 'unmarks the repository as read-only without updating the repository storage' do
+      allow(project_repository_double).to receive(:create_repository)
+        .and_return(true)
       allow(project_repository_double).to receive(:replicate)
         .with(project.repository.raw)
       allow(project_repository_double).to receive(:checksum)
         .and_return(project_repository_checksum)
 
+      allow(repository_double).to receive(:create_repository)
+        .and_return(true)
       allow(repository_double).to receive(:replicate)
         .with(repository.raw)
         .and_raise(Gitlab::Git::CommandError)
@@ -111,11 +119,15 @@ RSpec.shared_examples 'moves repository to another storage' do |repository_type|
 
   context "when the checksum of the #{repository_type} repository does not match" do
     it 'unmarks the repository as read-only without updating the repository storage' do
+      allow(project_repository_double).to receive(:create_repository)
+        .and_return(true)
       allow(project_repository_double).to receive(:replicate)
         .with(project.repository.raw)
       allow(project_repository_double).to receive(:checksum)
         .and_return(project_repository_checksum)
 
+      allow(repository_double).to receive(:create_repository)
+        .and_return(true)
       allow(repository_double).to receive(:replicate)
         .with(repository.raw)
       allow(repository_double).to receive(:checksum)