Allow caching of negative FindCommit matchessh-cache-negative-entries-find-commit

When FindCommit ref caching is enabled, negative matches would
previously not be cached. However, if a source branch is deleted,
there's no need to keep looking up the same commit. This change caches
the result of a nil commit.

3 files changed, 40 insertions, 18 deletions

diff --git a/changelogs/unreleased/sh-cache-negative-entries-find-commit.yml b/changelogs/unreleased/sh-cache-negative-entries-find-commit.yml
new file mode 100644
index 00000000000..98eb13ee620
--- /dev/null
+++ b/changelogs/unreleased/sh-cache-negative-entries-find-commit.yml
@@ -0,0 +1,5 @@
+---
+title: Allow caching of negative FindCommit matches
+merge_request: 29952
+author:
+type: performance
diff --git a/lib/gitlab/gitaly_client/commit_service.rb b/lib/gitlab/gitaly_client/commit_service.rb
index d21b98d36ea..a80ce462ab0 100644
--- a/lib/gitlab/gitaly_client/commit_service.rb
+++ b/lib/gitlab/gitaly_client/commit_service.rb
@@ -271,26 +271,30 @@ module Gitlab
       end
 
       def find_commit(revision)
-        if 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
-          # tests).
-          key = {
-            storage: @gitaly_repo.storage_name,
-            relative_path: @gitaly_repo.relative_path,
-            commit_id: revision
-          }
-          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?
+        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
+        # tests).
+        key = {
+          storage: @gitaly_repo.storage_name,
+          relative_path: @gitaly_repo.relative_path,
+          commit_id: revision
+        }
+        return Gitlab::SafeRequestStore[key] if Gitlab::SafeRequestStore.exist?(key)
+
+        commit = call_find_commit(revision)
+
+        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
diff --git a/spec/lib/gitlab/gitaly_client/commit_service_spec.rb b/spec/lib/gitlab/gitaly_client/commit_service_spec.rb
index 6d6107ca3e7..ba6abba4e61 100644
--- a/spec/lib/gitlab/gitaly_client/commit_service_spec.rb
+++ b/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))