diff options
author | Jacob Vosmaer <jacob@gitlab.com> | 2017-08-23 17:14:21 +0200 |
---|---|---|
committer | Jacob Vosmaer <jacob@gitlab.com> | 2017-08-31 15:35:59 +0200 |
commit | 8ad690b0d4f9db528c40e7f523e6f8219c944d48 (patch) | |
tree | cafe45b09c7f18a196b69897a77387b19cfaaff1 | |
parent | 3e092caa91853afeab3bb01be10869e45c39de5d (diff) | |
download | gitlab-ce-8ad690b0d4f9db528c40e7f523e6f8219c944d48.tar.gz |
Prepare GitOperationService for move to Gitlab::Git
-rw-r--r-- | app/models/merge_request.rb | 9 | ||||
-rw-r--r-- | app/models/repository.rb | 103 | ||||
-rw-r--r-- | app/services/compare_service.rb | 22 | ||||
-rw-r--r-- | app/services/git_operation_service.rb | 27 | ||||
-rw-r--r-- | lib/gitlab/conflict/file_collection.rb | 2 | ||||
-rw-r--r-- | lib/gitlab/git/repository.rb | 100 | ||||
-rw-r--r-- | spec/models/repository_spec.rb | 62 | ||||
-rw-r--r-- | spec/workers/git_garbage_collect_worker_spec.rb | 6 |
8 files changed, 179 insertions, 152 deletions
diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index 5be2f6d4e82..7c7376dccbe 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -949,13 +949,6 @@ class MergeRequest < ActiveRecord::Base private def write_ref - target_project.repository.with_repo_branch_commit( - source_project.repository, source_branch) do |commit| - if commit - target_project.repository.write_ref(ref_path, commit.sha) - else - raise Rugged::ReferenceError, 'source repository is empty' - end - end + target_project.repository.fetch_source_branch(source_project.repository, source_branch, ref_path) end end diff --git a/app/models/repository.rb b/app/models/repository.rb index d29d2a83708..a4b71ad4b61 100644 --- a/app/models/repository.rb +++ b/app/models/repository.rb @@ -95,19 +95,6 @@ class Repository "#<#{self.class.name}:#{@disk_path}>" end - # - # Git repository can contains some hidden refs like: - # /refs/notes/* - # /refs/git-as-svn/* - # /refs/pulls/* - # This refs by default not visible in project page and not cloned to client side. - # - # This method return true if repository contains some content visible in project page. - # - def has_visible_content? - branch_count > 0 - end - def commit(ref = 'HEAD') return nil unless exists? @@ -184,7 +171,7 @@ class Repository return false unless newrev - GitOperationService.new(user, self).add_branch(branch_name, newrev) + GitOperationService.new(user, raw_repository).add_branch(branch_name, newrev) after_create_branch find_branch(branch_name) @@ -196,7 +183,7 @@ class Repository return false unless newrev - GitOperationService.new(user, self).add_tag(tag_name, newrev, options) + GitOperationService.new(user, raw_repository).add_tag(tag_name, newrev, options) find_tag(tag_name) end @@ -205,7 +192,7 @@ class Repository before_remove_branch branch = find_branch(branch_name) - GitOperationService.new(user, self).rm_branch(branch) + GitOperationService.new(user, raw_repository).rm_branch(branch) after_remove_branch true @@ -215,7 +202,7 @@ class Repository before_remove_tag tag = find_tag(tag_name) - GitOperationService.new(user, self).rm_tag(tag) + GitOperationService.new(user, raw_repository).rm_tag(tag) after_remove_tag true @@ -784,16 +771,30 @@ class Repository multi_action(**options) end + def with_branch(user, *args) + result = GitOperationService.new(user, raw_repository).with_branch(*args) do |start_commit| + yield start_commit + end + + newrev, should_run_after_create, should_run_after_create_branch = result + + after_create if should_run_after_create + after_create_branch if should_run_after_create_branch + + newrev + end + # rubocop:disable Metrics/ParameterLists def multi_action( user:, branch_name:, message:, actions:, author_email: nil, author_name: nil, start_branch_name: nil, start_project: project) - GitOperationService.new(user, self).with_branch( + with_branch( + user, branch_name, start_branch_name: start_branch_name, - start_project: start_project) do |start_commit| + start_repository: start_project.repository.raw_repository) do |start_commit| index = Gitlab::Git::Index.new(raw_repository) @@ -846,7 +847,8 @@ class Repository end def merge(user, source, merge_request, options = {}) - GitOperationService.new(user, self).with_branch( + with_branch( + user, merge_request.target_branch) do |start_commit| our_commit = start_commit.sha their_commit = source @@ -873,10 +875,11 @@ class Repository def revert( user, commit, branch_name, start_branch_name: nil, start_project: project) - GitOperationService.new(user, self).with_branch( + with_branch( + user, branch_name, start_branch_name: start_branch_name, - start_project: start_project) do |start_commit| + start_repository: start_project.repository.raw_repository) do |start_commit| revert_tree_id = check_revert_content(commit, start_commit.sha) unless revert_tree_id @@ -896,10 +899,11 @@ class Repository def cherry_pick( user, commit, branch_name, start_branch_name: nil, start_project: project) - GitOperationService.new(user, self).with_branch( + with_branch( + user, branch_name, start_branch_name: start_branch_name, - start_project: start_project) do |start_commit| + start_repository: start_project.repository.raw_repository) do |start_commit| cherry_pick_tree_id = check_cherry_pick_content(commit, start_commit.sha) unless cherry_pick_tree_id @@ -921,7 +925,7 @@ class Repository end def resolve_conflicts(user, branch_name, params) - GitOperationService.new(user, self).with_branch(branch_name) do + with_branch(user, branch_name) do committer = user_to_committer(user) create_commit(params.merge(author: committer, committer: committer)) @@ -1011,25 +1015,6 @@ class Repository run_git(args).first.lines.map(&:strip) end - def with_repo_branch_commit(start_repository, start_branch_name) - return yield nil if start_repository.empty_repo? - - if start_repository == self - yield commit(start_branch_name) - else - sha = start_repository.commit(start_branch_name).sha - - if branch_commit = commit(sha) - yield branch_commit - else - with_repo_tmp_commit( - start_repository, start_branch_name, sha) do |tmp_commit| - yield tmp_commit - end - end - end - end - def add_remote(name, url) raw_repository.remote_add(name, url) rescue Rugged::ConfigError @@ -1047,14 +1032,12 @@ class Repository gitlab_shell.fetch_remote(repository_storage_path, disk_path, remote, forced: forced, no_tags: no_tags) end - def fetch_ref(source_path, source_ref, target_ref) - args = %W(fetch --no-tags -f #{source_path} #{source_ref}:#{target_ref}) - message, status = run_git(args) - - # Make sure ref was created, and raise Rugged::ReferenceError when not - raise Rugged::ReferenceError, message if status != 0 + def fetch_source_branch(source_repository, source_branch, local_ref) + raw_repository.fetch_source_branch(source_repository.raw_repository, source_branch, local_ref) + end - target_ref + def compare_source_branch(target_branch_name, source_repository, source_branch_name, straight:) + raw_repository.compare_source_branch(target_branch_name, source_repository.raw_repository, source_branch_name, straight: straight) end def create_ref(ref, ref_path) @@ -1135,12 +1118,6 @@ class Repository private - def run_git(args) - circuit_breaker.perform do - Gitlab::Popen.popen([Gitlab.config.git.bin_path, *args], path_to_repo) - end - end - def blob_data_at(sha, path) blob = blob_at(sha, path) return unless blob @@ -1236,16 +1213,4 @@ class Repository .commits_by_message(query, revision: ref, path: path, limit: limit, offset: offset) .map { |c| commit(c) } end - - def with_repo_tmp_commit(start_repository, start_branch_name, sha) - tmp_ref = fetch_ref( - start_repository.path_to_repo, - "#{Gitlab::Git::BRANCH_REF_PREFIX}#{start_branch_name}", - "refs/tmp/#{SecureRandom.hex}/head" - ) - - yield commit(sha) - ensure - delete_refs(tmp_ref) if tmp_ref - end end diff --git a/app/services/compare_service.rb b/app/services/compare_service.rb index a5ae4927412..53f16a236d2 100644 --- a/app/services/compare_service.rb +++ b/app/services/compare_service.rb @@ -11,26 +11,8 @@ class CompareService end def execute(target_project, target_branch, straight: false) - # If compare with other project we need to fetch ref first - target_project.repository.with_repo_branch_commit( - start_project.repository, - start_branch_name) do |commit| - break unless commit + raw_compare = target_project.repository.compare_source_branch(target_branch, start_project.repository, start_branch_name, straight: straight) - compare(commit.sha, target_project, target_branch, straight: straight) - end - end - - private - - def compare(source_sha, target_project, target_branch, straight:) - raw_compare = Gitlab::Git::Compare.new( - target_project.repository.raw_repository, - target_branch, - source_sha, - straight: straight - ) - - Compare.new(raw_compare, target_project, straight: straight) + Compare.new(raw_compare, target_project, straight: straight) if raw_compare end end diff --git a/app/services/git_operation_service.rb b/app/services/git_operation_service.rb index 6b7a56e6922..b484e9f6f2b 100644 --- a/app/services/git_operation_service.rb +++ b/app/services/git_operation_service.rb @@ -5,6 +5,11 @@ class GitOperationService committer = Gitlab::Git::Committer.from_user(committer) if committer.is_a?(User) @committer = committer + # Refactoring aid + unless new_repository.is_a?(Gitlab::Git::Repository) + raise "expected a Gitlab::Git::Repository, got #{new_repository}" + end + @repository = new_repository end @@ -55,10 +60,14 @@ class GitOperationService def with_branch( branch_name, start_branch_name: nil, - start_project: repository.project, + start_repository: repository, &block) - start_repository = start_project.repository + # Refactoring aid + unless start_repository.is_a?(Gitlab::Git::Repository) + raise "expected a Gitlab::Git::Repository, got #{start_repository}" + end + start_branch_name = nil if start_repository.empty_repo? if start_branch_name && !start_repository.branch_exists?(start_branch_name) @@ -75,6 +84,7 @@ class GitOperationService private + # Returns [newrev, should_run_after_create, should_run_after_create_branch] def update_branch_with_hooks(branch_name) update_autocrlf_option @@ -93,12 +103,7 @@ class GitOperationService ref = Gitlab::Git::BRANCH_REF_PREFIX + branch_name update_ref_in_hooks(ref, newrev, oldrev) - # If repo was empty expire cache - repository.after_create if was_empty - repository.after_create_branch if - was_empty || Gitlab::Git.blank_ref?(oldrev) - - newrev + [newrev, was_empty, was_empty || Gitlab::Git.blank_ref?(oldrev)] end def find_oldrev_from_branch(newrev, branch) @@ -140,7 +145,7 @@ class GitOperationService command = %W[#{Gitlab.config.git.bin_path} update-ref --stdin -z] _, status = Gitlab::Popen.popen( command, - repository.path_to_repo) do |stdin| + repository.path) do |stdin| stdin.write("update #{ref}\x00#{newrev}\x00#{oldrev}\x00") end @@ -152,8 +157,8 @@ class GitOperationService end def update_autocrlf_option - if repository.raw_repository.autocrlf != :input - repository.raw_repository.autocrlf = :input + if repository.autocrlf != :input + repository.autocrlf = :input end end end diff --git a/lib/gitlab/conflict/file_collection.rb b/lib/gitlab/conflict/file_collection.rb index d671867e7c7..90f83e0f810 100644 --- a/lib/gitlab/conflict/file_collection.rb +++ b/lib/gitlab/conflict/file_collection.rb @@ -18,7 +18,7 @@ module Gitlab new(merge_request, project).tap do |file_collection| project .repository - .with_repo_branch_commit(merge_request.target_project.repository, merge_request.target_branch) do + .with_repo_branch_commit(merge_request.target_project.repository.raw_repository, merge_request.target_branch) do yield file_collection end diff --git a/lib/gitlab/git/repository.rb b/lib/gitlab/git/repository.rb index fb6504bdea0..7a145107d3d 100644 --- a/lib/gitlab/git/repository.rb +++ b/lib/gitlab/git/repository.rb @@ -731,6 +731,106 @@ module Gitlab end end + def with_repo_branch_commit(start_repository, start_branch_name) + raise "expected Gitlab::Git::Repository, got #{start_repository}" unless start_repository.is_a?(Gitlab::Git::Repository) + + return yield nil if start_repository.empty_repo? + + if start_repository == self + yield commit(start_branch_name) + else + sha = start_repository.commit(start_branch_name).sha + + if branch_commit = commit(sha) + yield branch_commit + else + with_repo_tmp_commit( + start_repository, start_branch_name, sha) do |tmp_commit| + yield tmp_commit + end + end + end + end + + def with_repo_tmp_commit(start_repository, start_branch_name, sha) + tmp_ref = fetch_ref( + start_repository.path, + "#{Gitlab::Git::BRANCH_REF_PREFIX}#{start_branch_name}", + "refs/tmp/#{SecureRandom.hex}/head" + ) + + yield commit(sha) + ensure + delete_refs(tmp_ref) if tmp_ref + end + + def fetch_source_branch(source_repository, source_branch, local_ref) + with_repo_branch_commit(source_repository, source_branch) do |commit| + if commit + write_ref(local_ref, commit.sha) + else + raise Rugged::ReferenceError, 'source repository is empty' + end + end + end + + def compare_source_branch(target_branch_name, source_repository, source_branch_name, straight:) + with_repo_branch_commit(source_repository, source_branch_name) do |commit| + break unless commit + + Gitlab::Git::Compare.new( + self, + target_branch_name, + commit.sha, + straight: straight + ) + end + end + + def write_ref(ref_path, sha) + rugged.references.create(ref_path, sha, force: true) + end + + def fetch_ref(source_path, source_ref, target_ref) + args = %W(fetch --no-tags -f #{source_path} #{source_ref}:#{target_ref}) + message, status = run_git(args) + + # Make sure ref was created, and raise Rugged::ReferenceError when not + raise Rugged::ReferenceError, message if status != 0 + + target_ref + end + + # Refactoring aid; allows us to copy code from app/models/repository.rb + def run_git(args) + circuit_breaker.perform do + popen([Gitlab.config.git.bin_path, *args], path) + end + end + + # Refactoring aid; allows us to copy code from app/models/repository.rb + def commit(ref = 'HEAD') + Gitlab::Git::Commit.find(self, ref) + end + + # Refactoring aid; allows us to copy code from app/models/repository.rb + def empty_repo? + !exists? || !has_visible_content? + end + + # + # Git repository can contains some hidden refs like: + # /refs/notes/* + # /refs/git-as-svn/* + # /refs/pulls/* + # This refs by default not visible in project page and not cloned to client side. + # + # This method return true if repository contains some content visible in project page. + # + def has_visible_content? + branch_count > 0 + end + def gitaly_repository Gitlab::GitalyClient::Util.repository(@storage, @relative_path) end diff --git a/spec/models/repository_spec.rb b/spec/models/repository_spec.rb index 34e1a955309..a7736185c84 100644 --- a/spec/models/repository_spec.rb +++ b/spec/models/repository_spec.rb @@ -886,7 +886,7 @@ describe Repository, models: true do context 'when pre hooks were successful' do it 'runs without errors' do expect_any_instance_of(Gitlab::Git::HooksService).to receive(:execute) - .with(committer, repository, old_rev, blank_sha, 'refs/heads/feature') + .with(committer, repository.raw_repository, old_rev, blank_sha, 'refs/heads/feature') expect { repository.rm_branch(user, 'feature') }.not_to raise_error end @@ -932,20 +932,20 @@ describe Repository, models: true do service = Gitlab::Git::HooksService.new expect(Gitlab::Git::HooksService).to receive(:new).and_return(service) expect(service).to receive(:execute) - .with(committer, target_repository, old_rev, new_rev, updating_ref) + .with(committer, target_repository.raw_repository, old_rev, new_rev, updating_ref) .and_yield(service).and_return(true) end it 'runs without errors' do expect do - GitOperationService.new(committer, repository).with_branch('feature') do + GitOperationService.new(committer, repository.raw_repository).with_branch('feature') do new_rev end end.not_to raise_error end it 'ensures the autocrlf Git option is set to :input' do - service = GitOperationService.new(committer, repository) + service = GitOperationService.new(committer, repository.raw_repository) expect(service).to receive(:update_autocrlf_option) @@ -956,7 +956,7 @@ describe Repository, models: true do it 'updates the head' do expect(repository.find_branch('feature').dereferenced_target.id).to eq(old_rev) - GitOperationService.new(committer, repository).with_branch('feature') do + GitOperationService.new(committer, repository.raw_repository).with_branch('feature') do new_rev end @@ -971,13 +971,13 @@ describe Repository, models: true do let(:updating_ref) { 'refs/heads/master' } it 'fetch_ref and create the branch' do - expect(target_project.repository).to receive(:fetch_ref) + expect(target_project.repository.raw_repository).to receive(:fetch_ref) .and_call_original - GitOperationService.new(committer, target_repository) + GitOperationService.new(committer, target_repository.raw_repository) .with_branch( 'master', - start_project: project, + start_repository: project.repository.raw_repository, start_branch_name: 'feature') { new_rev } expect(target_repository.branch_names).to contain_exactly('master') @@ -990,8 +990,8 @@ describe Repository, models: true do it 'does not fetch_ref and just pass the commit' do expect(target_repository).not_to receive(:fetch_ref) - GitOperationService.new(committer, target_repository) - .with_branch('feature', start_project: project) { new_rev } + GitOperationService.new(committer, target_repository.raw_repository) + .with_branch('feature', start_repository: project.repository.raw_repository) { new_rev } end end end @@ -1000,7 +1000,7 @@ describe Repository, models: true do let(:target_project) { create(:project, :empty_repo) } before do - expect(target_project.repository).to receive(:run_git) + expect(target_project.repository.raw_repository).to receive(:run_git) end it 'raises Rugged::ReferenceError' do @@ -1009,9 +1009,9 @@ describe Repository, models: true do end expect do - GitOperationService.new(committer, target_project.repository) + GitOperationService.new(committer, target_project.repository.raw_repository) .with_branch('feature', - start_project: project, + start_repository: project.repository.raw_repository, &:itself) end.to raise_reference_error end @@ -1031,7 +1031,7 @@ describe Repository, models: true do repository.add_branch(user, branch, old_rev) expect do - GitOperationService.new(committer, repository).with_branch(branch) do + GitOperationService.new(committer, repository.raw_repository).with_branch(branch) do new_rev end end.not_to raise_error @@ -1049,7 +1049,7 @@ describe Repository, models: true do # Updating 'master' to new_rev would lose the commits on 'master' that # are not contained in new_rev. This should not be allowed. expect do - GitOperationService.new(committer, repository).with_branch(branch) do + GitOperationService.new(committer, repository.raw_repository).with_branch(branch) do new_rev end end.to raise_error(Repository::CommitError) @@ -1061,31 +1061,13 @@ describe Repository, models: true do allow_any_instance_of(Gitlab::Git::Hook).to receive(:trigger).and_return([false, '']) expect do - GitOperationService.new(committer, repository).with_branch('feature') do + GitOperationService.new(committer, repository.raw_repository).with_branch('feature') do new_rev end end.to raise_error(Gitlab::Git::HooksService::PreReceiveError) end end - context 'when target branch is different from source branch' do - before do - allow_any_instance_of(Gitlab::Git::Hook).to receive(:trigger).and_return([true, '']) - end - - it 'expires branch cache' do - expect(repository).not_to receive(:expire_exists_cache) - expect(repository).not_to receive(:expire_root_ref_cache) - expect(repository).not_to receive(:expire_emptiness_caches) - expect(repository).to receive(:expire_branches_cache) - - GitOperationService.new(committer, repository) - .with_branch('new-feature') do - new_rev - end - end - end - context 'when repository is empty' do before do allow_any_instance_of(Gitlab::Git::Hook).to receive(:trigger).and_return([true, '']) @@ -1139,7 +1121,7 @@ describe Repository, models: true do describe 'when there are no branches' do before do - allow(repository).to receive(:branch_count).and_return(0) + allow(repository.raw_repository).to receive(:branch_count).and_return(0) end it { is_expected.to eq(false) } @@ -1147,7 +1129,7 @@ describe Repository, models: true do describe 'when there are branches' do it 'returns true' do - expect(repository).to receive(:branch_count).and_return(3) + expect(repository.raw_repository).to receive(:branch_count).and_return(3) expect(subject).to eq(true) end @@ -1161,7 +1143,7 @@ describe Repository, models: true do end it 'sets autocrlf to :input' do - GitOperationService.new(nil, repository).send(:update_autocrlf_option) + GitOperationService.new(nil, repository.raw_repository).send(:update_autocrlf_option) expect(repository.raw_repository.autocrlf).to eq(:input) end @@ -1176,7 +1158,7 @@ describe Repository, models: true do expect(repository.raw_repository).not_to receive(:autocrlf=) .with(:input) - GitOperationService.new(nil, repository).send(:update_autocrlf_option) + GitOperationService.new(nil, repository.raw_repository).send(:update_autocrlf_option) end end end @@ -1759,14 +1741,14 @@ describe Repository, models: true do describe '#update_ref' do it 'can create a ref' do - GitOperationService.new(nil, repository).send(:update_ref, 'refs/heads/foobar', 'refs/heads/master', Gitlab::Git::BLANK_SHA) + GitOperationService.new(nil, repository.raw_repository).send(:update_ref, 'refs/heads/foobar', 'refs/heads/master', Gitlab::Git::BLANK_SHA) expect(repository.find_branch('foobar')).not_to be_nil end it 'raises CommitError when the ref update fails' do expect do - GitOperationService.new(nil, repository).send(:update_ref, 'refs/heads/master', 'refs/heads/master', Gitlab::Git::BLANK_SHA) + GitOperationService.new(nil, repository.raw_repository).send(:update_ref, 'refs/heads/master', 'refs/heads/master', Gitlab::Git::BLANK_SHA) end.to raise_error(Repository::CommitError) end end diff --git a/spec/workers/git_garbage_collect_worker_spec.rb b/spec/workers/git_garbage_collect_worker_spec.rb index 05f971dfd13..b4099457478 100644 --- a/spec/workers/git_garbage_collect_worker_spec.rb +++ b/spec/workers/git_garbage_collect_worker_spec.rb @@ -23,8 +23,8 @@ describe GitGarbageCollectWorker do expect_any_instance_of(Repository).to receive(:after_create_branch).and_call_original expect_any_instance_of(Repository).to receive(:branch_names).and_call_original - expect_any_instance_of(Repository).to receive(:branch_count).and_call_original - expect_any_instance_of(Repository).to receive(:has_visible_content?).and_call_original + expect_any_instance_of(Gitlab::Git::Repository).to receive(:branch_count).and_call_original + expect_any_instance_of(Gitlab::Git::Repository).to receive(:has_visible_content?).and_call_original subject.perform(project.id) end @@ -143,7 +143,7 @@ describe GitGarbageCollectWorker do tree: old_commit.tree, parents: [old_commit] ) - GitOperationService.new(nil, project.repository).send( + GitOperationService.new(nil, project.repository.raw_repository).send( :update_ref, "refs/heads/#{SecureRandom.hex(6)}", new_commit_sha, |