diff --git a/app/models/repository.rb b/app/models/repository.rb index edfb236a91a..e46f167da75 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 96a442f782f..df9f8a84aa5 100644 --- a/spec/lib/gitlab/git/repository_spec.rb +++ b/spec/lib/gitlab/git/repository_spec.rb @@ -1162,14 +1162,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 1102b1c9006..54ca7ed0737 100644 --- a/spec/models/repository_spec.rb +++ b/spec/models/repository_spec.rb @@ -959,19 +959,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