Commit 05f17e4d authored by Ahmad Sherif's avatar Ahmad Sherif

Remove repo reloading logic from Repository#find_branch

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

Fixes #42609
parent 6f32fa66
...@@ -173,15 +173,7 @@ class Repository ...@@ -173,15 +173,7 @@ class Repository
end end
def find_branch(name, fresh_repo: true) def find_branch(name, fresh_repo: true)
# Since the Repository object may have in-memory index changes, invalidating the memoized Repository object may raw_repository.find_branch(name, fresh_repo)
# 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)
end end
def find_tag(name) def find_tag(name)
......
...@@ -1196,7 +1196,8 @@ describe Gitlab::Git::Repository, seed_helper: true do ...@@ -1196,7 +1196,8 @@ describe Gitlab::Git::Repository, seed_helper: true do
context 'when Gitaly find_branch feature is disabled', :skip_gitaly_mock do context 'when Gitaly find_branch feature is disabled', :skip_gitaly_mock do
it_behaves_like 'finding a branch' it_behaves_like 'finding a branch'
it 'should reload Rugged::Repository and return master' do context 'force_reload is true' do
it 'should reload Rugged::Repository' do
expect(Rugged::Repository).to receive(:new).twice.and_call_original expect(Rugged::Repository).to receive(:new).twice.and_call_original
repository.find_branch('master') repository.find_branch('master')
...@@ -1206,6 +1207,18 @@ describe Gitlab::Git::Repository, seed_helper: true do ...@@ -1206,6 +1207,18 @@ describe Gitlab::Git::Repository, seed_helper: true do
expect(branch.name).to eq('master') expect(branch.name).to eq('master')
end end
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 end
describe '#ref_name_for_sha' do describe '#ref_name_for_sha' do
......
...@@ -960,19 +960,19 @@ describe Repository do ...@@ -960,19 +960,19 @@ describe Repository do
end end
describe '#find_branch' do describe '#find_branch' do
it 'loads a branch with a fresh repo' do context 'fresh_repo is true' do
expect(Gitlab::Git::Repository).to receive(:new).twice.and_call_original it 'delegates the call to raw_repository' do
expect(repository.raw_repository).to receive(:find_branch).with('master', true)
2.times do repository.find_branch('master', fresh_repo: true)
expect(repository.find_branch('feature')).not_to be_nil
end end
end end
it 'loads a branch with a cached repo' do context 'fresh_repo is false' do
expect(Gitlab::Git::Repository).to receive(:new).once.and_call_original it 'delegates the call to raw_repository' do
expect(repository.raw_repository).to receive(:find_branch).with('master', false)
2.times do repository.find_branch('master', fresh_repo: false)
expect(repository.find_branch('feature', fresh_repo: false)).not_to be_nil
end end
end end
end end
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment