Merge branch '65803-invalidate-branches-cache-on-refresh' into 'master'

Only expire branch cache once per push

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

8 files changed, 109 insertions, 13 deletions

diff --git a/app/models/repository.rb b/app/models/repository.rb
index a89f573e3d6..58abfaef801 100644
--- a/app/models/repository.rb
+++ b/app/models/repository.rb
@@ -460,8 +460,8 @@ class Repository
   end
 
   # Runs code after a new branch has been created.
-  def after_create_branch
-    expire_branches_cache
+  def after_create_branch(expire_cache: true)
+    expire_branches_cache if expire_cache
 
     repository_event(:push_branch)
   end
@@ -474,8 +474,8 @@ class Repository
   end
 
   # Runs code after an existing branch has been removed.
-  def after_remove_branch
-    expire_branches_cache
+  def after_remove_branch(expire_cache: true)
+    expire_branches_cache if expire_cache
   end
 
   def method_missing(msg, *args, &block)
diff --git a/app/services/git/branch_hooks_service.rb b/app/services/git/branch_hooks_service.rb
index c41f445c3c4..431a5aedf2e 100644
--- a/app/services/git/branch_hooks_service.rb
+++ b/app/services/git/branch_hooks_service.rb
@@ -63,7 +63,7 @@ module Git
     end
 
     def branch_create_hooks
-      project.repository.after_create_branch
+      project.repository.after_create_branch(expire_cache: false)
       project.after_create_default_branch if default_branch?
     end
 
@@ -78,7 +78,7 @@ module Git
     end
 
     def branch_remove_hooks
-      project.repository.after_remove_branch
+      project.repository.after_remove_branch(expire_cache: false)
     end
 
     # Schedules processing of commit messages
diff --git a/app/workers/post_receive.rb b/app/workers/post_receive.rb
index dba7837bd12..d8dfbc0faf7 100644
--- a/app/workers/post_receive.rb
+++ b/app/workers/post_receive.rb
@@ -42,6 +42,9 @@ class PostReceive
     user = identify_user(post_received)
     return false unless user
 
+    # Expire the branches cache so we have updated data for this push
+    post_received.project.repository.expire_branches_cache if post_received.includes_branches?
+
     post_received.enum_for(:changes_refs).with_index do |(oldrev, newrev, ref), index|
       service_klass =
         if Gitlab::Git.tag_ref?(ref)
diff --git a/changelogs/unreleased/65803-invalidate-branches-cache-on-refresh.yml b/changelogs/unreleased/65803-invalidate-branches-cache-on-refresh.yml
new file mode 100644
index 00000000000..217db8aa05a
--- /dev/null
+++ b/changelogs/unreleased/65803-invalidate-branches-cache-on-refresh.yml
@@ -0,0 +1,5 @@
+---
+title: Invalidate branches cache on PostReceive
+merge_request: 31653
+author:
+type: fixed
diff --git a/lib/gitlab/git_post_receive.rb b/lib/gitlab/git_post_receive.rb
index d98b85fecc4..6c7f23a673c 100644
--- a/lib/gitlab/git_post_receive.rb
+++ b/lib/gitlab/git_post_receive.rb
@@ -27,6 +27,12 @@ module Gitlab
       end
     end
 
+    def includes_branches?
+      enum_for(:changes_refs).any? do |_oldrev, _newrev, ref|
+        Gitlab::Git.branch_ref?(ref)
+      end
+    end
+
     private
 
     def deserialize_changes(changes)
diff --git a/spec/lib/gitlab/git_post_receive_spec.rb b/spec/lib/gitlab/git_post_receive_spec.rb
new file mode 100644
index 00000000000..1911e954df9
--- /dev/null
+++ b/spec/lib/gitlab/git_post_receive_spec.rb
@@ -0,0 +1,52 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe ::Gitlab::GitPostReceive do
+  let(:project) { create(:project) }
+
+  subject { described_class.new(project, "project-#{project.id}", changes.dup, {}) }
+
+  describe '#includes_branches?' do
+    context 'with no branches' do
+      let(:changes) do
+        <<~EOF
+          654321 210987 refs/nobranches/tag1
+          654322 210986 refs/tags/test1
+          654323 210985 refs/merge-requests/mr1
+        EOF
+      end
+
+      it 'returns false' do
+        expect(subject.includes_branches?).to be_falsey
+      end
+    end
+
+    context 'with branches' do
+      let(:changes) do
+        <<~EOF
+          654322 210986 refs/heads/test1
+          654321 210987 refs/tags/tag1
+          654323 210985 refs/merge-requests/mr1
+        EOF
+      end
+
+      it 'returns true' do
+        expect(subject.includes_branches?).to be_truthy
+      end
+    end
+
+    context 'with malformed changes' do
+      let(:changes) do
+        <<~EOF
+          ref/heads/1 a
+          somebranch refs/heads/2
+        EOF
+      end
+
+      it 'returns false' do
+        expect(subject.includes_branches?).to be_falsey
+      end
+    end
+  end
+end
diff --git a/spec/models/repository_spec.rb b/spec/models/repository_spec.rb
index 12dff440ce2..fa243876632 100644
--- a/spec/models/repository_spec.rb
+++ b/spec/models/repository_spec.rb
@@ -1781,6 +1781,12 @@ describe Repository do
 
       repository.after_create_branch
     end
+
+    it 'does not expire the branch caches when specified' do
+      expect(repository).not_to receive(:expire_branches_cache)
+
+      repository.after_create_branch(expire_cache: false)
+    end
   end
 
   describe '#after_remove_branch' do
@@ -1789,6 +1795,12 @@ describe Repository do
 
       repository.after_remove_branch
     end
+
+    it 'does not expire the branch caches when specified' do
+      expect(repository).not_to receive(:expire_branches_cache)
+
+      repository.after_remove_branch(expire_cache: false)
+    end
   end
 
   describe '#after_create' do
diff --git a/spec/workers/post_receive_spec.rb b/spec/workers/post_receive_spec.rb
index 39f1beb4efa..081d95d4d79 100644
--- a/spec/workers/post_receive_spec.rb
+++ b/spec/workers/post_receive_spec.rb
@@ -57,13 +57,25 @@ describe PostReceive do
     context 'with changes' do
       before do
         allow_any_instance_of(Gitlab::GitPostReceive).to receive(:identify).and_return(project.owner)
+        allow(Gitlab::GlRepository).to receive(:parse).and_return([project, Gitlab::GlRepository::PROJECT])
       end
 
       context "branches" do
-        let(:changes) { "123456 789012 refs/heads/tést" }
+        let(:changes) do
+          <<~EOF
+            '123456 789012 refs/heads/tést1'
+            '123456 789012 refs/heads/tést2'
+          EOF
+        end
 
-        it "calls Git::BranchPushService" do
-          expect_next_instance_of(Git::BranchPushService) do |service|
+        it 'expires the branches cache' do
+          expect(project.repository).to receive(:expire_branches_cache).once
+
+          described_class.new.perform(gl_repository, key_id, base64_changes)
+        end
+
+        it 'calls Git::BranchPushService' do
+          expect_any_instance_of(Git::BranchPushService) do |service|
             expect(service).to receive(:execute).and_return(true)
           end
 
@@ -73,16 +85,22 @@ describe PostReceive do
         end
       end
 
-      context "tags" do
-        let(:changes) { "123456 789012 refs/tags/tag" }
+      context 'tags' do
+        let(:changes) { '123456 789012 refs/tags/tag' }
+
+        it 'does not expire branches cache' do
+          expect(project.repository).not_to receive(:expire_branches_cache)
 
-        it "calls Git::TagPushService" do
-          expect(Git::BranchPushService).not_to receive(:execute)
+          described_class.new.perform(gl_repository, key_id, base64_changes)
+        end
 
+        it 'calls Git::TagPushService' do
           expect_next_instance_of(Git::TagPushService) do |service|
             expect(service).to receive(:execute).and_return(true)
           end
 
+          expect(Git::BranchPushService).not_to receive(:new)
+
           described_class.new.perform(gl_repository, key_id, base64_changes)
         end
       end