From 902b5347dcbb8d93d5b055d89d8d0414fdeade74 Mon Sep 17 00:00:00 2001 From: Jacob Vosmaer Date: Fri, 8 Sep 2017 14:00:53 +0200 Subject: [PATCH] Prepare Repository#merge for migration to Gitaly --- app/models/repository.rb | 59 +++++++------------ app/services/merge_requests/merge_service.rb | 10 +--- lib/gitlab/git/operation_service.rb | 7 ++- lib/gitlab/git/repository.rb | 37 ++++++++++++ spec/lib/gitlab/diff/position_tracer_spec.rb | 10 +--- spec/models/commit_spec.rb | 7 +-- spec/models/repository_spec.rb | 22 +++---- .../merge_requests/refresh_service_spec.rb | 4 +- 8 files changed, 77 insertions(+), 79 deletions(-) diff --git a/app/models/repository.rb b/app/models/repository.rb index f2b54705e7b..af9911ea045 100644 --- a/app/models/repository.rb +++ b/app/models/repository.rb @@ -768,17 +768,23 @@ class Repository multi_action(**options) end + def with_cache_hooks + result = yield + + return unless result + + after_create if result.repo_created? + after_create_branch if result.branch_created? + + result.newrev + end + def with_branch(user, *args) - result = Gitlab::Git::OperationService.new(user, raw_repository).with_branch(*args) do |start_commit| - yield start_commit + with_cache_hooks do + Gitlab::Git::OperationService.new(user, raw_repository).with_branch(*args) do |start_commit| + yield start_commit + end 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 @@ -843,30 +849,13 @@ class Repository end end - def merge(user, source, merge_request, options = {}) - with_branch( - user, - merge_request.target_branch) do |start_commit| - our_commit = start_commit.sha - their_commit = source - - raise 'Invalid merge target' unless our_commit - raise 'Invalid merge source' unless their_commit - - merge_index = rugged.merge_commits(our_commit, their_commit) - break if merge_index.conflicts? - - actual_options = options.merge( - parents: [our_commit, their_commit], - tree: merge_index.write_tree(rugged) - ) - - commit_id = create_commit(actual_options) - merge_request.update(in_progress_merge_commit_sha: commit_id) - commit_id + def merge(user, source_sha, merge_request, message) + with_cache_hooks do + raw_repository.merge(user, source_sha, merge_request.target_branch, message) do |commit_id| + merge_request.update(in_progress_merge_commit_sha: commit_id) + nil # Return value does not matter. + end end - rescue Gitlab::Git::CommitError # when merge_index.conflicts? - false end def revert( @@ -1157,12 +1146,6 @@ class Repository Gitlab::Metrics.add_event(event, { path: full_path }.merge(tags)) end - def create_commit(params = {}) - params[:message].delete!("\r") - - Rugged::Commit.create(rugged, params) - end - def last_commit_for_path_by_gitaly(sha, path) c = raw_repository.gitaly_commit_client.last_commit_for_path(sha, path) commit(c) diff --git a/app/services/merge_requests/merge_service.rb b/app/services/merge_requests/merge_service.rb index b2b6c5627fb..07cbd8f92a9 100644 --- a/app/services/merge_requests/merge_service.rb +++ b/app/services/merge_requests/merge_service.rb @@ -38,15 +38,9 @@ module MergeRequests private def commit - committer = repository.user_to_committer(current_user) + message = params[:commit_message] || merge_request.merge_commit_message - options = { - message: params[:commit_message] || merge_request.merge_commit_message, - author: committer, - committer: committer - } - - commit_id = repository.merge(current_user, source, merge_request, options) + commit_id = repository.merge(current_user, source, merge_request, message) raise MergeError, 'Conflicts detected during merge' unless commit_id diff --git a/lib/gitlab/git/operation_service.rb b/lib/gitlab/git/operation_service.rb index 347e3b5165e..dcdec818f5e 100644 --- a/lib/gitlab/git/operation_service.rb +++ b/lib/gitlab/git/operation_service.rb @@ -1,6 +1,11 @@ module Gitlab module Git class OperationService + WithBranchResult = Struct.new(:newrev, :repo_created, :branch_created) do + alias_method :repo_created?, :repo_created + alias_method :branch_created?, :branch_created + end + attr_reader :user, :repository def initialize(user, new_repository) @@ -107,7 +112,7 @@ module Gitlab ref = Gitlab::Git::BRANCH_REF_PREFIX + branch_name update_ref_in_hooks(ref, newrev, oldrev) - [newrev, was_empty, was_empty || Gitlab::Git.blank_ref?(oldrev)] + WithBranchResult.new(newrev, was_empty, was_empty || Gitlab::Git.blank_ref?(oldrev)) end def find_oldrev_from_branch(newrev, branch) diff --git a/lib/gitlab/git/repository.rb b/lib/gitlab/git/repository.rb index 32a265b15f2..c499ff101b5 100644 --- a/lib/gitlab/git/repository.rb +++ b/lib/gitlab/git/repository.rb @@ -653,6 +653,43 @@ module Gitlab tags.find { |tag| tag.name == name } end + def merge(user, source_sha, target_branch, message) + committer = Gitlab::Git.committer_hash(email: user.email, name: user.name) + + OperationService.new(user, self).with_branch(target_branch) do |start_commit| + our_commit = start_commit.sha + their_commit = source_sha + + raise 'Invalid merge target' unless our_commit + raise 'Invalid merge source' unless their_commit + + merge_index = rugged.merge_commits(our_commit, their_commit) + break if merge_index.conflicts? + + options = { + parents: [our_commit, their_commit], + tree: merge_index.write_tree(rugged), + message: message, + author: committer, + committer: committer + } + + commit_id = create_commit(options) + + yield commit_id + + commit_id + end + rescue Gitlab::Git::CommitError # when merge_index.conflicts? + nil + end + + def create_commit(params = {}) + params[:message].delete!("\r") + + Rugged::Commit.create(rugged, params) + end + # Delete the specified branch from the repository def delete_branch(branch_name) gitaly_migrate(:delete_branch) do |is_enabled| diff --git a/spec/lib/gitlab/diff/position_tracer_spec.rb b/spec/lib/gitlab/diff/position_tracer_spec.rb index 8beebc10040..4fa30d8df8b 100644 --- a/spec/lib/gitlab/diff/position_tracer_spec.rb +++ b/spec/lib/gitlab/diff/position_tracer_spec.rb @@ -1761,17 +1761,9 @@ describe Gitlab::Diff::PositionTracer do let(:merge_commit) do update_file_again_commit - committer = repository.user_to_committer(current_user) - - options = { - message: "Merge branches", - author: committer, - committer: committer - } - merge_request = create(:merge_request, source_branch: second_create_file_commit.sha, target_branch: branch_name, source_project: project) - repository.merge(current_user, merge_request.diff_head_sha, merge_request, options) + repository.merge(current_user, merge_request.diff_head_sha, merge_request, "Merge branches") project.commit(branch_name) end diff --git a/spec/models/commit_spec.rb b/spec/models/commit_spec.rb index 11e64a0f877..e3cfa149e3a 100644 --- a/spec/models/commit_spec.rb +++ b/spec/models/commit_spec.rb @@ -207,11 +207,6 @@ eos context 'of a merge commit' do let(:repository) { project.repository } - let(:commit_options) do - author = repository.user_to_committer(user) - { message: 'Test message', committer: author, author: author } - end - let(:merge_request) do create(:merge_request, source_branch: 'video', @@ -224,7 +219,7 @@ eos merge_commit_id = repository.merge(user, merge_request.diff_head_sha, merge_request, - commit_options) + 'Test message') repository.commit(merge_commit_id) end diff --git a/spec/models/repository_spec.rb b/spec/models/repository_spec.rb index 53280f2c1cf..60cd7e70055 100644 --- a/spec/models/repository_spec.rb +++ b/spec/models/repository_spec.rb @@ -10,10 +10,7 @@ describe Repository, models: true do let(:user) { create(:user) } let(:git_user) { Gitlab::Git::User.from_gitlab(user) } - let(:commit_options) do - author = repository.user_to_committer(user) - { message: 'Test message', committer: author, author: author } - end + let(:message) { 'Test message' } let(:merge_commit) do merge_request = create(:merge_request, source_branch: 'feature', target_branch: 'master', source_project: project) @@ -21,7 +18,7 @@ describe Repository, models: true do merge_commit_id = repository.merge(user, merge_request.diff_head_sha, merge_request, - commit_options) + message) repository.commit(merge_commit_id) end @@ -1287,10 +1284,7 @@ describe Repository, models: true do describe '#merge' do let(:merge_request) { create(:merge_request, source_branch: 'feature', target_branch: 'master', source_project: project) } - let(:commit_options) do - author = repository.user_to_committer(user) - { message: 'Test \r\n\r\n message', committer: author, author: author } - end + let(:message) { 'Test \r\n\r\n message' } it 'merges the code and returns the commit id' do expect(merge_commit).to be_present @@ -1298,19 +1292,19 @@ describe Repository, models: true do end it 'sets the `in_progress_merge_commit_sha` flag for the given merge request' do - merge_commit_id = merge(repository, user, merge_request, commit_options) + merge_commit_id = merge(repository, user, merge_request, message) expect(merge_request.in_progress_merge_commit_sha).to eq(merge_commit_id) end it 'removes carriage returns from commit message' do - merge_commit_id = merge(repository, user, merge_request, commit_options) + merge_commit_id = merge(repository, user, merge_request, message) - expect(repository.commit(merge_commit_id).message).to eq(commit_options[:message].delete("\r")) + expect(repository.commit(merge_commit_id).message).to eq(message.delete("\r")) end - def merge(repository, user, merge_request, options = {}) - repository.merge(user, merge_request.diff_head_sha, merge_request, options) + def merge(repository, user, merge_request, message) + repository.merge(user, merge_request.diff_head_sha, merge_request, message) end end diff --git a/spec/services/merge_requests/refresh_service_spec.rb b/spec/services/merge_requests/refresh_service_spec.rb index 2af2485eeed..64e676f22a0 100644 --- a/spec/services/merge_requests/refresh_service_spec.rb +++ b/spec/services/merge_requests/refresh_service_spec.rb @@ -150,9 +150,7 @@ describe MergeRequests::RefreshService do context 'manual merge of source branch' do before do # Merge master -> feature branch - author = { email: 'test@gitlab.com', time: Time.now, name: "Me" } - commit_options = { message: 'Test message', committer: author, author: author } - @project.repository.merge(@user, @merge_request.diff_head_sha, @merge_request, commit_options) + @project.repository.merge(@user, @merge_request.diff_head_sha, @merge_request, 'Test message') commit = @project.repository.commit('feature') service.new(@project, @user).execute(@oldrev, commit.id, 'refs/heads/feature') reload_mrs