From 05f17e4d720756cb5aea9e7697a6fa549372e536 Mon Sep 17 00:00:00 2001
From: Ahmad Sherif <me@ahmadsherif.com>
Date: Tue, 30 Jan 2018 22:33:00 +0100
Subject: [PATCH] Remove repo reloading logic from Repository#find_branch

Gitlab::Git::Repository#find_branch has a similar logic.

Fixes #42609
---
 app/models/repository.rb               | 10 +---------
 spec/lib/gitlab/git/repository_spec.rb | 25 +++++++++++++++++++------
 spec/models/repository_spec.rb         | 16 ++++++++--------
 3 files changed, 28 insertions(+), 23 deletions(-)

diff --git a/app/models/repository.rb b/app/models/repository.rb
index 6c776301ac2..44ecedd7834 100644
--- a/app/models/repository.rb
+++ b/app/models/repository.rb
@@ -173,15 +173,7 @@ class Repository
   end
 
   def find_branch(name, fresh_repo: true)
-    # Since the Repository object may have in-memory index changes, invalidating the memoized Repository object may
-    # cause unintended side effects. Because finding a branch is a read-only operation, we can safely instantiate
-    # a new repo here to ensure a consistent state to avoid a libgit2 bug where concurrent access (e.g. via git gc)
-    # may cause the branch to "disappear" erroneously or have the wrong SHA.
-    #
-    # See: https://github.com/libgit2/libgit2/issues/1534 and https://gitlab.com/gitlab-org/gitlab-ce/issues/15392
-    raw_repo = fresh_repo ? initialize_raw_repository : raw_repository
-
-    raw_repo.find_branch(name)
+    raw_repository.find_branch(name, fresh_repo)
   end
 
   def find_tag(name)
diff --git a/spec/lib/gitlab/git/repository_spec.rb b/spec/lib/gitlab/git/repository_spec.rb
index ac7c0270916..41673895283 100644
--- a/spec/lib/gitlab/git/repository_spec.rb
+++ b/spec/lib/gitlab/git/repository_spec.rb
@@ -1196,14 +1196,27 @@ describe Gitlab::Git::Repository, seed_helper: true do
     context 'when Gitaly find_branch feature is disabled', :skip_gitaly_mock do
       it_behaves_like 'finding a branch'
 
-      it 'should reload Rugged::Repository and return master' do
-        expect(Rugged::Repository).to receive(:new).twice.and_call_original
+      context 'force_reload is true' do
+        it 'should reload Rugged::Repository' do
+          expect(Rugged::Repository).to receive(:new).twice.and_call_original
 
-        repository.find_branch('master')
-        branch = repository.find_branch('master', force_reload: true)
+          repository.find_branch('master')
+          branch = repository.find_branch('master', force_reload: true)
 
-        expect(branch).to be_a_kind_of(Gitlab::Git::Branch)
-        expect(branch.name).to eq('master')
+          expect(branch).to be_a_kind_of(Gitlab::Git::Branch)
+          expect(branch.name).to eq('master')
+        end
+      end
+
+      context 'force_reload is false' do
+        it 'should not reload Rugged::Repository' do
+          expect(Rugged::Repository).to receive(:new).once.and_call_original
+
+          branch = repository.find_branch('master', force_reload: false)
+
+          expect(branch).to be_a_kind_of(Gitlab::Git::Branch)
+          expect(branch.name).to eq('master')
+        end
       end
     end
   end
diff --git a/spec/models/repository_spec.rb b/spec/models/repository_spec.rb
index d4070b320ed..76baf41f9d6 100644
--- a/spec/models/repository_spec.rb
+++ b/spec/models/repository_spec.rb
@@ -960,19 +960,19 @@ describe Repository do
   end
 
   describe '#find_branch' do
-    it 'loads a branch with a fresh repo' do
-      expect(Gitlab::Git::Repository).to receive(:new).twice.and_call_original
+    context 'fresh_repo is true' do
+      it 'delegates the call to raw_repository' do
+        expect(repository.raw_repository).to receive(:find_branch).with('master', true)
 
-      2.times do
-        expect(repository.find_branch('feature')).not_to be_nil
+        repository.find_branch('master', fresh_repo: true)
       end
     end
 
-    it 'loads a branch with a cached repo' do
-      expect(Gitlab::Git::Repository).to receive(:new).once.and_call_original
+    context 'fresh_repo is false' do
+      it 'delegates the call to raw_repository' do
+        expect(repository.raw_repository).to receive(:find_branch).with('master', false)
 
-      2.times do
-        expect(repository.find_branch('feature', fresh_repo: false)).not_to be_nil
+        repository.find_branch('master', fresh_repo: false)
       end
     end
   end