Merge branch 'fj-refactor-code-before-version-snippets' into 'master'

Refactor code involved in git workflow

See merge request gitlab-org/gitlab!22266

app/controllers/projects/git_http_client_controller.rb

@@ -3,6 +3,7 @@
 class Projects::GitHttpClientController < Projects::ApplicationController
   include ActionController::HttpAuthentication::Basic
   include KerberosSpnegoHelper
+  include Gitlab::Utils::StrongMemoize
 
   attr_reader :authentication_result, :redirected_path
 
@@ -47,7 +48,7 @@ class Projects::GitHttpClientController < Projects::ApplicationController
         send_final_spnego_response
         return # Allow access
       end
-    elsif project && download_request? && http_allowed? && Guest.can?(:download_code, project)
+    elsif http_download_allowed?
 
       @authentication_result = Gitlab::Auth::Result.new(nil, project, :none, [:download_code])
 
@@ -89,11 +90,9 @@ class Projects::GitHttpClientController < Projects::ApplicationController
   end
 
   def repository
+    strong_memoize(:repository) do
       repo_type.repository_for(project)
     end
-
-  def wiki?
-    repo_type.wiki?
   end
 
   def repo_type
@@ -113,8 +112,10 @@ class Projects::GitHttpClientController < Projects::ApplicationController
     authentication_result.ci?(project)
   end
 
-  def http_allowed?
-    Gitlab::ProtocolAccess.allowed?('http')
+  def http_download_allowed?
+    Gitlab::ProtocolAccess.allowed?('http') &&
+    download_request? &&
+    project && Guest.can?(:download_code, project)
   end
 end
 

app/controllers/projects/git_http_controller.rb

@@ -75,17 +75,19 @@ class Projects::GitHttpController < Projects::GitHttpClientController
   end
 
   def enqueue_fetch_statistics_update
-    return if wiki?
-    return unless project.daily_statistics_enabled?
+    return if repo_type.wiki?
+    return unless project&.daily_statistics_enabled?
 
     ProjectDailyStatisticsWorker.perform_async(project.id)
   end
 
   def access
-    @access ||= access_klass.new(access_actor, project,
-      'http', authentication_abilities: authentication_abilities,
-              namespace_path: params[:namespace_id], project_path: project_path,
-              redirected_path: redirected_path, auth_result_type: auth_result_type)
+    @access ||= access_klass.new(access_actor, project, 'http',
+      authentication_abilities: authentication_abilities,
+      namespace_path: params[:namespace_id],
+      project_path: project_path,
+      redirected_path: redirected_path,
+      auth_result_type: auth_result_type)
   end
 
   def access_actor

app/models/repository.rb

@@ -95,7 +95,7 @@ class Repository
   def path_to_repo
     @path_to_repo ||=
       begin
-        storage = Gitlab.config.repositories.storages[@project.repository_storage]
+        storage = Gitlab.config.repositories.storages[project.repository_storage]
 
         File.expand_path(
           File.join(storage.legacy_disk_path, disk_path + '.git')
@@ -128,7 +128,7 @@ class Repository
     commits = Gitlab::Git::Commit.batch_by_oid(raw_repository, oids)
 
     if commits.present?
-      Commit.decorate(commits, @project)
+      Commit.decorate(commits, project)
     else
       []
     end
@@ -159,14 +159,14 @@ class Repository
     }
 
     commits = Gitlab::Git::Commit.where(options)
-    commits = Commit.decorate(commits, @project) if commits.present?
+    commits = Commit.decorate(commits, project) if commits.present?
 
     CommitCollection.new(project, commits, ref)
   end
 
   def commits_between(from, to)
     commits = Gitlab::Git::Commit.between(raw_repository, from, to)
-    commits = Commit.decorate(commits, @project) if commits.present?
+    commits = Commit.decorate(commits, project) if commits.present?
     commits
   end
 
@@ -695,13 +695,13 @@ class Repository
     commits = raw_repository.list_last_commits_for_tree(sha, path, offset: offset, limit: limit)
 
     commits.each do |path, commit|
-      commits[path] = ::Commit.new(commit, @project)
+      commits[path] = ::Commit.new(commit, project)
     end
   end
 
   def last_commit_for_path(sha, path)
     commit = raw_repository.last_commit_for_path(sha, path)
-    ::Commit.new(commit, @project) if commit
+    ::Commit.new(commit, project) if commit
   end
 
   def last_commit_id_for_path(sha, path)
@@ -1136,7 +1136,7 @@ class Repository
                Gitlab::Git::Commit.find(raw_repository, oid_or_ref)
              end
 
-    ::Commit.new(commit, @project) if commit
+    ::Commit.new(commit, project) if commit
   end
 
   def cache

app/services/merge_requests/get_urls_service.rb

@@ -9,7 +9,7 @@ module MergeRequests
     end
 
     def execute(changes)
-      return [] unless project.printing_merge_request_link_enabled
+      return [] unless project&.printing_merge_request_link_enabled
 
       branches = get_branches(changes)
       merge_requests_map = opened_merge_requests_from_source_branches(branches)

ee/lib/ee/gitlab/geo_git_access.rb

@@ -13,7 +13,7 @@ module EE
       protected
 
       def project_or_wiki
-        @project
+        project
       end
 
       private

lib/api/helpers/internal_helpers.rb

@@ -120,21 +120,13 @@ module API
       end
 
       def gl_project_path
-        if wiki?
-          project.wiki.full_path
-        else
-          project.full_path
-        end
+        repository.full_path
       end
 
       # Return the repository depending on whether we want the wiki or the
       # regular repository
       def repository
-        if repo_type.wiki?
-          project.wiki.repository
-        else
-          project.repository
-        end
+        @repository ||= repo_type.repository_for(project)
       end
 
       # Return the Gitaly Address if it is enabled

lib/api/internal/base.rb

@@ -224,9 +224,9 @@ module API
 
           response.add_merge_request_urls(merge_request_urls)
 
-          # A user is not guaranteed to be returned; an orphaned write deploy
+          # Neither User nor Project are guaranteed to be returned; an orphaned write deploy
           # key could be used
-          if user
+          if user && project
             redirect_message = Gitlab::Checks::ProjectMoved.fetch_message(user.id, project.id)
             project_created_message = Gitlab::Checks::ProjectCreated.fetch_message(user.id, project.id)
 

lib/gitlab/repo_path.rb

@@ -32,9 +32,12 @@ module Gitlab
 
     def self.find_project(project_path)
       project = Project.find_by_full_path(project_path, follow_redirects: true)
-      was_redirected = project && project.full_path.casecmp(project_path) != 0
 
-      [project, was_redirected]
+      [project, redirected?(project, project_path)]
+    end
+
+    def self.redirected?(project, project_path)
+      project && project.full_path.casecmp(project_path) != 0
     end
   end
 end

lib/gitlab/repository_cache.rb

@@ -7,7 +7,8 @@ module Gitlab
 
     def initialize(repository, extra_namespace: nil, backend: Rails.cache)
       @repository = repository
-      @namespace = "#{repository.full_path}:#{repository.project.id}"
+      @namespace = "#{repository.full_path}"
+      @namespace += ":#{repository.project.id}" if repository.project
       @namespace = "#{@namespace}:#{extra_namespace}" if extra_namespace
       @backend = backend
     end

lib/gitlab/repository_set_cache.rb

@@ -7,7 +7,8 @@ module Gitlab
 
     def initialize(repository, extra_namespace: nil, expires_in: 2.weeks)
       @repository = repository
-      @namespace = "#{repository.full_path}:#{repository.project.id}"
+      @namespace = "#{repository.full_path}"
+      @namespace += ":#{repository.project.id}" if repository.project
       @namespace = "#{@namespace}:#{extra_namespace}" if extra_namespace
       @expires_in = expires_in
     end

lib/gitlab/workhorse.rb

@@ -22,18 +22,16 @@ module Gitlab
       def git_http_ok(repository, repo_type, user, action, show_all_refs: false)
         raise "Unsupported action: #{action}" unless ALLOWED_GIT_HTTP_ACTIONS.include?(action.to_s)
 
-        project = repository.project
-
         attrs = {
           GL_ID: Gitlab::GlId.gl_id(user),
-          GL_REPOSITORY: repo_type.identifier_for_subject(project),
+          GL_REPOSITORY: repo_type.identifier_for_subject(repository.project),
           GL_USERNAME: user&.username,
           ShowAllRefs: show_all_refs,
           Repository: repository.gitaly_repository.to_h,
           GitConfigOptions: [],
           GitalyServer: {
-            address: Gitlab::GitalyClient.address(project.repository_storage),
-            token: Gitlab::GitalyClient.token(project.repository_storage),
+            address: Gitlab::GitalyClient.address(repository.storage),
+            token: Gitlab::GitalyClient.token(repository.storage),
             features: Feature::Gitaly.server_feature_flags
           }
         }

spec/controllers/projects/git_http_controller_spec.rb

@@ -3,10 +3,17 @@
 require 'spec_helper'
 
 describe Projects::GitHttpController do
+  let_it_be(:project) { create(:project, :public, :repository) }
+  let(:project_params) do
+    {
+      namespace_id: project.namespace.to_param,
+      project_id: project.path + '.git'
+    }
+  end
+  let(:params) { project_params }
+
   describe 'HEAD #info_refs' do
     it 'returns 403' do
-      project = create(:project, :public, :repository)
-
       head :info_refs, params: { namespace_id: project.namespace.to_param, project_id: project.path + '.git' }
 
       expect(response.status).to eq(403)
@@ -14,18 +21,17 @@ describe Projects::GitHttpController do
   end
 
   describe 'GET #info_refs' do
+    let(:params) { project_params.merge(service: 'git-upload-pack') }
+
     it 'returns 401 for unauthenticated requests to public repositories when http protocol is disabled' do
       stub_application_setting(enabled_git_access_protocol: 'ssh')
-      project = create(:project, :public, :repository)
 
-      get :info_refs, params: { service: 'git-upload-pack', namespace_id: project.namespace.to_param, project_id: project.path + '.git' }
+      get :info_refs, params: params
 
       expect(response.status).to eq(401)
     end
 
     context 'with exceptions' do
-      let(:project) { create(:project, :public, :repository) }
-
       before do
         allow(controller).to receive(:verify_workhorse_api!).and_return(true)
       end
@@ -33,7 +39,7 @@ describe Projects::GitHttpController do
       it 'returns 503 with GRPC Unavailable' do
         allow(controller).to receive(:access_check).and_raise(GRPC::Unavailable)
 
-        get :info_refs, params: { service: 'git-upload-pack', namespace_id: project.namespace.to_param, project_id: project.path + '.git' }
+        get :info_refs, params: params
 
         expect(response.status).to eq(503)
       end
@@ -41,11 +47,27 @@ describe Projects::GitHttpController do
       it 'returns 503 with timeout error' do
         allow(controller).to receive(:access_check).and_raise(Gitlab::GitAccess::TimeoutError)
 
-        get :info_refs, params: { service: 'git-upload-pack', namespace_id: project.namespace.to_param, project_id: project.path + '.git' }
+        get :info_refs, params: params
 
         expect(response.status).to eq(503)
         expect(response.body).to eq 'Gitlab::GitAccess::TimeoutError'
       end
     end
   end
+
+  describe 'POST #git_upload_pack' do
+    before do
+      allow(controller).to receive(:authenticate_user).and_return(true)
+      allow(controller).to receive(:verify_workhorse_api!).and_return(true)
+      allow(controller).to receive(:access_check).and_return(nil)
+    end
+
+    after do
+      post :git_upload_pack, params: params
+    end
+
+    it 'updates project statistics' do
+      expect(ProjectDailyStatisticsWorker).to receive(:perform_async)
+    end
+  end
 end

spec/lib/gitlab/repository_cache_spec.rb

@@ -3,8 +3,8 @@
 require 'spec_helper'
 
 describe Gitlab::RepositoryCache do
+  let_it_be(:project) { create(:project) }
   let(:backend) { double('backend').as_null_object }
-  let(:project) { create(:project) }
   let(:repository) { project.repository }
   let(:namespace) { "#{repository.full_path}:#{project.id}" }
   let(:cache) { described_class.new(repository, backend: backend) }

spec/lib/gitlab/repository_set_cache_spec.rb

@@ -3,7 +3,7 @@
 require 'spec_helper'
 
 describe Gitlab::RepositorySetCache, :clean_gitlab_redis_cache do
-  let(:project) { create(:project) }
+  let_it_be(:project) { create(:project) }
   let(:repository) { project.repository }
   let(:namespace) { "#{repository.full_path}:#{project.id}" }
   let(:cache) { described_class.new(repository) }

spec/requests/api/internal/base_spec.rb

@@ -1000,6 +1000,22 @@ describe API::Internal::Base do
       it 'does not try to notify that project moved' do
         allow_any_instance_of(Gitlab::Identifier).to receive(:identify).and_return(nil)
 
+        expect(Gitlab::Checks::ProjectMoved).not_to receive(:fetch_message)
+
+        post api('/internal/post_receive'), params: valid_params
+
+        expect(response).to have_gitlab_http_status(200)
+      end
+    end
+
+    context 'when project is nil' do
+      let(:gl_repository) { 'project-foo' }
+
+      it 'does not try to notify that project moved' do
+        allow(Gitlab::GlRepository).to receive(:parse).and_return([nil, Gitlab::GlRepository::PROJECT])
+
+        expect(Gitlab::Checks::ProjectMoved).not_to receive(:fetch_message)
+
         post api('/internal/post_receive'), params: valid_params
 
         expect(response).to have_gitlab_http_status(200)

spec/services/merge_requests/get_urls_service_spec.rb

@@ -45,6 +45,13 @@ describe MergeRequests::GetUrlsService do
       end
     end
 
+    context 'when project is nil' do
+      let(:project) { nil }
+      let(:changes) { default_branch_changes }
+
+      it_behaves_like 'no_merge_request_url'
+    end
+
     context 'pushing to default branch' do
       let(:changes) { default_branch_changes }
 

spec/support/helpers/test_env.rb

@@ -245,8 +245,8 @@ module TestEnv
     end
   end
 
-  def copy_repo(project, bare_repo:, refs:)
-    target_repo_path = File.expand_path(repos_path + "/#{project.disk_path}.git")
+  def copy_repo(subject, bare_repo:, refs:)
+    target_repo_path = File.expand_path(repos_path + "/#{subject.disk_path}.git")
 
     FileUtils.mkdir_p(target_repo_path)
     FileUtils.cp_r("#{File.expand_path(bare_repo)}/.", target_repo_path)