diff options
-rw-r--r-- | changelogs/unreleased/sh-avoid-fetching-temp-refs-within-project.yml | 2 | ||||
-rw-r--r-- | lib/gitlab/git/repository.rb | 32 | ||||
-rw-r--r-- | spec/lib/gitlab/git/repository_spec.rb | 46 |
3 files changed, 61 insertions, 19 deletions
diff --git a/changelogs/unreleased/sh-avoid-fetching-temp-refs-within-project.yml b/changelogs/unreleased/sh-avoid-fetching-temp-refs-within-project.yml index 9b747f5d83d..7511543f7f6 100644 --- a/changelogs/unreleased/sh-avoid-fetching-temp-refs-within-project.yml +++ b/changelogs/unreleased/sh-avoid-fetching-temp-refs-within-project.yml @@ -1,5 +1,5 @@ --- title: Don't create a temp reference for branch comparisons within project -merge_request: +merge_request: 24038 author: type: fixed diff --git a/lib/gitlab/git/repository.rb b/lib/gitlab/git/repository.rb index d7f5720c795..c12cb6a6434 100644 --- a/lib/gitlab/git/repository.rb +++ b/lib/gitlab/git/repository.rb @@ -732,20 +732,27 @@ module Gitlab end def compare_source_branch(target_branch_name, source_repository, source_branch_name, straight:) - if source_repository == self - return Gitlab::Git::Compare.new(self, target_branch_name, source_branch_name, straight: straight) - end + reachable_ref = + if source_repository == self + source_branch_name + else + # If a tmp ref was created before for a separate repo comparison (forks), + # we're able to short-circuit the tmp ref re-creation: + # 1. Take the SHA from the source repo + # 2. Read that in the current "target" repo + # 3. If that SHA is still known (readable), it means GC hasn't + # cleaned it up yet, so we can use it instead re-writing the tmp ref. + source_commit_id = source_repository.commit(source_branch_name)&.sha + commit(source_commit_id)&.sha if source_commit_id + end + + return compare(target_branch_name, reachable_ref, straight: straight) if reachable_ref tmp_ref = "refs/tmp/#{SecureRandom.hex}" return unless fetch_source_branch!(source_repository, source_branch_name, tmp_ref) - Gitlab::Git::Compare.new( - self, - target_branch_name, - tmp_ref, - straight: straight - ) + compare(target_branch_name, tmp_ref, straight: straight) ensure delete_refs(tmp_ref) if tmp_ref end @@ -1003,6 +1010,13 @@ module Gitlab private + def compare(base_ref, head_ref, straight:) + Gitlab::Git::Compare.new(self, + base_ref, + head_ref, + straight: straight) + end + def empty_diff_stats Gitlab::Git::DiffStatsCollection.new([]) end diff --git a/spec/lib/gitlab/git/repository_spec.rb b/spec/lib/gitlab/git/repository_spec.rb index aa800db4f16..45fe5d72937 100644 --- a/spec/lib/gitlab/git/repository_spec.rb +++ b/spec/lib/gitlab/git/repository_spec.rb @@ -1967,6 +1967,8 @@ describe Gitlab::Git::Repository, :seed_helper do end describe '#compare_source_branch' do + let(:repository) { Gitlab::Git::Repository.new('default', TEST_GITATTRIBUTES_REPO_PATH, '', 'group/project') } + context 'within same repository' do it 'does not create a temp ref' do expect(repository).not_to receive(:fetch_source_branch!) @@ -1985,19 +1987,45 @@ describe Gitlab::Git::Repository, :seed_helper do end context 'with different repositories' do - it 'creates a temp ref' do - expect(repository).to receive(:fetch_source_branch!).and_call_original - expect(repository).to receive(:delete_refs).and_call_original + context 'when ref is known by source repo, but not by target' do + before do + mutable_repository.write_ref('another-branch', 'feature') + end - compare = repository.compare_source_branch('master', mutable_repository, 'feature', straight: false) - expect(compare).to be_a(Gitlab::Git::Compare) - expect(compare.commits.count).to be > 0 + it 'creates temp ref' do + expect(repository).not_to receive(:fetch_source_branch!) + expect(repository).not_to receive(:delete_refs) + + compare = repository.compare_source_branch('master', mutable_repository, 'another-branch', straight: false) + expect(compare).to be_a(Gitlab::Git::Compare) + expect(compare.commits.count).to be > 0 + end end - it 'returns nil when source ref does not exist' do - compare = repository.compare_source_branch('master', mutable_repository, 'non-existent-branch', straight: false) + context 'when ref is known by source and target repos' do + before do + mutable_repository.write_ref('another-branch', 'feature') + repository.write_ref('another-branch', 'feature') + end + + it 'does not create a temp ref' do + expect(repository).not_to receive(:fetch_source_branch!) + expect(repository).not_to receive(:delete_refs) + + compare = repository.compare_source_branch('master', mutable_repository, 'another-branch', straight: false) + expect(compare).to be_a(Gitlab::Git::Compare) + expect(compare.commits.count).to be > 0 + end + end - expect(compare).to be_nil + context 'when ref is unknown by source repo' do + it 'returns nil when source ref does not exist' do + expect(repository).to receive(:fetch_source_branch!).and_call_original + expect(repository).to receive(:delete_refs).and_call_original + + compare = repository.compare_source_branch('master', mutable_repository, 'non-existent-branch', straight: false) + expect(compare).to be_nil + end end end end |