Merge branch 'sh-cache-negative-entries-find-commit' into 'master'

Allow caching of negative FindCommit matches

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

changelogs/unreleased/sh-cache-negative-entries-find-commit.yml

+---
+title: Allow caching of negative FindCommit matches
+merge_request: 29952
+author:
+type: performance

lib/gitlab/gitaly_client/commit_service.rb

@@ -271,7 +271,8 @@ module Gitlab
       end
 
       def find_commit(revision)
-        if Gitlab::SafeRequestStore.active?
+        return call_find_commit(revision) unless Gitlab::SafeRequestStore.active?
+
         # We don't use Gitlab::SafeRequestStore.fetch(key) { ... } directly
         # because `revision` can be a branch name, so we can't use it as a key
         # as it could point to another commit later on (happens a lot in
@@ -284,13 +285,16 @@ module Gitlab
         return Gitlab::SafeRequestStore[key] if Gitlab::SafeRequestStore.exist?(key)
 
         commit = call_find_commit(revision)
-          return unless commit
 
-          key[:commit_id] = commit.id unless GitalyClient.ref_name_caching_allowed?
+        if GitalyClient.ref_name_caching_allowed?
           Gitlab::SafeRequestStore[key] = commit
-        else
-          call_find_commit(revision)
+          return commit
         end
+
+        return unless commit
+
+        key[:commit_id] = commit.id
+        Gitlab::SafeRequestStore[key] = commit
       end
 
       # rubocop: disable CodeReuse/ActiveRecord

spec/lib/gitlab/gitaly_client/commit_service_spec.rb

@@ -223,6 +223,19 @@ describe Gitlab::GitalyClient::CommitService do
       end
 
       context 'when caching of the ref name is enabled' do
+        it 'caches negative entries' do
+          expect_any_instance_of(Gitaly::CommitService::Stub).to receive(:find_commit).once.and_return(double(commit: nil))
+
+          commit = nil
+          2.times do
+            ::Gitlab::GitalyClient.allow_ref_name_caching do
+              commit = described_class.new(repository).find_commit('master')
+            end
+          end
+
+          expect(commit).to eq(nil)
+        end
+
         it 'returns a cached commit' do
           expect_any_instance_of(Gitaly::CommitService::Stub).to receive(:find_commit).once.and_return(double(commit: commit_dbl))