diff options
author | Rémy Coutable <remy@rymai.me> | 2017-09-19 20:09:11 +0300 |
---|---|---|
committer | Rémy Coutable <remy@rymai.me> | 2017-09-19 20:09:11 +0300 |
commit | 343301bc86e8b07ff9b6f485871aac7bb3bc6339 (patch) | |
tree | c4c81ad9857eb18f61597ff629b76fc3132ece17 | |
parent | f13158a090066b7266096ae58506d0d7611b5600 (diff) | |
parent | 75509fac17790bae546b2905aa684f85fdbd9c86 (diff) |
Merge branch 'gitaly-prepare-cherry-pick-revert' into 'master'
Prepare cherry-pick and revert for migration to Gitaly
See merge request gitlab-org/gitlab-ce!14339
-rw-r--r-- | app/models/repository.rb | 94 | ||||
-rw-r--r-- | app/services/commits/change_service.rb | 6 | ||||
-rw-r--r-- | lib/gitlab/git.rb | 9 | ||||
-rw-r--r-- | lib/gitlab/git/commit.rb | 4 | ||||
-rw-r--r-- | lib/gitlab/git/operation_service.rb | 4 | ||||
-rw-r--r-- | lib/gitlab/git/repository.rb | 85 | ||||
-rw-r--r-- | spec/models/repository_spec.rb | 24 |
7 files changed, 136 insertions, 90 deletions
diff --git a/app/models/repository.rb b/app/models/repository.rb index 9d1de4f4306..f11cf1b065d 100644 --- a/app/models/repository.rb +++ b/app/models/repository.rb @@ -834,10 +834,6 @@ class Repository } end - def user_to_committer(user) - Gitlab::Git.committer_hash(email: user.email, name: user.name) - end - def can_be_merged?(source_sha, target_branch) our_commit = rugged.branches[target_branch].target their_commit = rugged.lookup(source_sha) @@ -859,54 +855,34 @@ class Repository end def revert( - user, commit, branch_name, + user, commit, branch_name, message, start_branch_name: nil, start_project: project) - with_branch( - user, - branch_name, - start_branch_name: start_branch_name, - start_repository: start_project.repository.raw_repository) do |start_commit| - - revert_tree_id = check_revert_content(commit, start_commit.sha) - unless revert_tree_id - raise Repository::CreateTreeError.new('Failed to revert commit') - end - committer = user_to_committer(user) - - create_commit(message: commit.revert_message(user), - author: committer, - committer: committer, - tree: revert_tree_id, - parents: [start_commit.sha]) + with_cache_hooks do + raw_repository.revert( + user: user, + commit: commit.raw, + branch_name: branch_name, + message: message, + start_branch_name: start_branch_name, + start_repository: start_project.repository.raw_repository + ) end end def cherry_pick( - user, commit, branch_name, + user, commit, branch_name, message, start_branch_name: nil, start_project: project) - with_branch( - user, - branch_name, - start_branch_name: start_branch_name, - 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 - raise Repository::CreateTreeError.new('Failed to cherry-pick commit') - end - - committer = user_to_committer(user) - - create_commit(message: commit.cherry_pick_message(user), - author: { - email: commit.author_email, - name: commit.author_name, - time: commit.authored_date - }, - committer: committer, - tree: cherry_pick_tree_id, - parents: [start_commit.sha]) + with_cache_hooks do + raw_repository.cherry_pick( + user: user, + commit: commit.raw, + branch_name: branch_name, + message: message, + start_branch_name: start_branch_name, + start_repository: start_project.repository.raw_repository + ) end end @@ -918,36 +894,6 @@ class Repository end end - def check_revert_content(target_commit, source_sha) - args = [target_commit.sha, source_sha] - args << { mainline: 1 } if target_commit.merge_commit? - - revert_index = rugged.revert_commit(*args) - return false if revert_index.conflicts? - - tree_id = revert_index.write_tree(rugged) - return false unless diff_exists?(source_sha, tree_id) - - tree_id - end - - def check_cherry_pick_content(target_commit, source_sha) - args = [target_commit.sha, source_sha] - args << 1 if target_commit.merge_commit? - - cherry_pick_index = rugged.cherrypick_commit(*args) - return false if cherry_pick_index.conflicts? - - tree_id = cherry_pick_index.write_tree(rugged) - return false unless diff_exists?(source_sha, tree_id) - - tree_id - end - - def diff_exists?(sha1, sha2) - rugged.diff(sha1, sha2).size > 0 - end - def merged_to_root_ref?(branch_name) branch_commit = commit(branch_name) root_ref_commit = commit(root_ref) diff --git a/app/services/commits/change_service.rb b/app/services/commits/change_service.rb index 85c2fcf9ea6..b9d0173a2d0 100644 --- a/app/services/commits/change_service.rb +++ b/app/services/commits/change_service.rb @@ -12,14 +12,18 @@ module Commits raise NotImplementedError unless repository.respond_to?(action) # rubocop:disable GitlabSecurity/PublicSend + message = @commit.public_send(:"#{action}_message", current_user) + + # rubocop:disable GitlabSecurity/PublicSend repository.public_send( action, current_user, @commit, @branch_name, + message, start_project: @start_project, start_branch_name: @start_branch) - rescue Repository::CreateTreeError + rescue Gitlab::Git::Repository::CreateTreeError error_msg = "Sorry, we cannot #{action.to_s.dasherize} this #{@commit.change_type_title(current_user)} automatically. This #{@commit.change_type_title(current_user)} may already have been #{action.to_s.dasherize}ed, or a more recent commit may have updated some of its content." raise ChangeError, error_msg diff --git a/lib/gitlab/git.rb b/lib/gitlab/git.rb index b4b6326cfdd..c78fe63f9b5 100644 --- a/lib/gitlab/git.rb +++ b/lib/gitlab/git.rb @@ -57,6 +57,15 @@ module Gitlab def version Gitlab::VersionInfo.parse(Gitlab::Popen.popen(%W(#{Gitlab.config.git.bin_path} --version)).first) end + + def check_namespace!(*objects) + expected_namespace = self.name + '::' + objects.each do |object| + unless object.class.name.start_with?(expected_namespace) + raise ArgumentError, "expected object in #{expected_namespace}, got #{object}" + end + end + end end end end diff --git a/lib/gitlab/git/commit.rb b/lib/gitlab/git/commit.rb index 1f370686186..1957c254c28 100644 --- a/lib/gitlab/git/commit.rb +++ b/lib/gitlab/git/commit.rb @@ -413,6 +413,10 @@ module Gitlab end end + def merge_commit? + parent_ids.size > 1 + end + private def init_from_hash(hash) diff --git a/lib/gitlab/git/operation_service.rb b/lib/gitlab/git/operation_service.rb index dcdec818f5e..6f054ed3c6c 100644 --- a/lib/gitlab/git/operation_service.rb +++ b/lib/gitlab/git/operation_service.rb @@ -15,9 +15,7 @@ module Gitlab end # Refactoring aid - unless new_repository.is_a?(Gitlab::Git::Repository) - raise "expected a Gitlab::Git::Repository, got #{new_repository}" - end + Gitlab::Git.check_namespace!(new_repository) @repository = new_repository end diff --git a/lib/gitlab/git/repository.rb b/lib/gitlab/git/repository.rb index 18210bcab4e..4b000bd31e2 100644 --- a/lib/gitlab/git/repository.rb +++ b/lib/gitlab/git/repository.rb @@ -19,6 +19,7 @@ module Gitlab InvalidRef = Class.new(StandardError) GitError = Class.new(StandardError) DeleteBranchError = Class.new(StandardError) + CreateTreeError = Class.new(StandardError) class << self # Unlike `new`, `create` takes the storage path, not the storage name @@ -684,6 +685,88 @@ module Gitlab nil end + def revert(user:, commit:, branch_name:, message:, start_branch_name:, start_repository:) + OperationService.new(user, self).with_branch( + branch_name, + start_branch_name: start_branch_name, + start_repository: start_repository + ) do |start_commit| + + Gitlab::Git.check_namespace!(commit, start_repository) + + revert_tree_id = check_revert_content(commit, start_commit.sha) + raise CreateTreeError unless revert_tree_id + + committer = user_to_committer(user) + + create_commit(message: message, + author: committer, + committer: committer, + tree: revert_tree_id, + parents: [start_commit.sha]) + end + end + + def check_revert_content(target_commit, source_sha) + args = [target_commit.sha, source_sha] + args << { mainline: 1 } if target_commit.merge_commit? + + revert_index = rugged.revert_commit(*args) + return false if revert_index.conflicts? + + tree_id = revert_index.write_tree(rugged) + return false unless diff_exists?(source_sha, tree_id) + + tree_id + end + + def cherry_pick(user:, commit:, branch_name:, message:, start_branch_name:, start_repository:) + OperationService.new(user, self).with_branch( + branch_name, + start_branch_name: start_branch_name, + start_repository: start_repository + ) do |start_commit| + + Gitlab::Git.check_namespace!(commit, start_repository) + + cherry_pick_tree_id = check_cherry_pick_content(commit, start_commit.sha) + raise CreateTreeError unless cherry_pick_tree_id + + committer = user_to_committer(user) + + create_commit(message: message, + author: { + email: commit.author_email, + name: commit.author_name, + time: commit.authored_date + }, + committer: committer, + tree: cherry_pick_tree_id, + parents: [start_commit.sha]) + end + end + + def check_cherry_pick_content(target_commit, source_sha) + args = [target_commit.sha, source_sha] + args << 1 if target_commit.merge_commit? + + cherry_pick_index = rugged.cherrypick_commit(*args) + return false if cherry_pick_index.conflicts? + + tree_id = cherry_pick_index.write_tree(rugged) + return false unless diff_exists?(source_sha, tree_id) + + tree_id + end + + def diff_exists?(sha1, sha2) + rugged.diff(sha1, sha2).size > 0 + end + + def user_to_committer(user) + Gitlab::Git.committer_hash(email: user.email, name: user.name) + end + def create_commit(params = {}) params[:message].delete!("\r") @@ -835,7 +918,7 @@ module Gitlab 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) + Gitlab::Git.check_namespace!(start_repository) return yield nil if start_repository.empty_repo? diff --git a/spec/models/repository_spec.rb b/spec/models/repository_spec.rb index 60cd7e70055..76bb658b10d 100644 --- a/spec/models/repository_spec.rb +++ b/spec/models/repository_spec.rb @@ -1311,24 +1311,25 @@ describe Repository, models: true do describe '#revert' do let(:new_image_commit) { repository.commit('33f3729a45c02fc67d00adb1b8bca394b0e761d9') } let(:update_image_commit) { repository.commit('2f63565e7aac07bcdadb654e253078b727143ec4') } + let(:message) { 'revert message' } context 'when there is a conflict' do it 'raises an error' do - expect { repository.revert(user, new_image_commit, 'master') }.to raise_error(/Failed to/) + expect { repository.revert(user, new_image_commit, 'master', message) }.to raise_error(Gitlab::Git::Repository::CreateTreeError) end end context 'when commit was already reverted' do it 'raises an error' do - repository.revert(user, update_image_commit, 'master') + repository.revert(user, update_image_commit, 'master', message) - expect { repository.revert(user, update_image_commit, 'master') }.to raise_error(/Failed to/) + expect { repository.revert(user, update_image_commit, 'master', message) }.to raise_error(Gitlab::Git::Repository::CreateTreeError) end end context 'when commit can be reverted' do it 'reverts the changes' do - expect(repository.revert(user, update_image_commit, 'master')).to be_truthy + expect(repository.revert(user, update_image_commit, 'master', message)).to be_truthy end end @@ -1337,7 +1338,7 @@ describe Repository, models: true do merge_commit expect(repository.blob_at_branch('master', 'files/ruby/feature.rb')).to be_present - repository.revert(user, merge_commit, 'master') + repository.revert(user, merge_commit, 'master', message) expect(repository.blob_at_branch('master', 'files/ruby/feature.rb')).not_to be_present end end @@ -1347,24 +1348,25 @@ describe Repository, models: true do let(:conflict_commit) { repository.commit('c642fe9b8b9f28f9225d7ea953fe14e74748d53b') } let(:pickable_commit) { repository.commit('7d3b0f7cff5f37573aea97cebfd5692ea1689924') } let(:pickable_merge) { repository.commit('e56497bb5f03a90a51293fc6d516788730953899') } + let(:message) { 'cherry-pick message' } context 'when there is a conflict' do it 'raises an error' do - expect { repository.cherry_pick(user, conflict_commit, 'master') }.to raise_error(/Failed to/) + expect { repository.cherry_pick(user, conflict_commit, 'master', message) }.to raise_error(Gitlab::Git::Repository::CreateTreeError) end end context 'when commit was already cherry-picked' do it 'raises an error' do - repository.cherry_pick(user, pickable_commit, 'master') + repository.cherry_pick(user, pickable_commit, 'master', message) - expect { repository.cherry_pick(user, pickable_commit, 'master') }.to raise_error(/Failed to/) + expect { repository.cherry_pick(user, pickable_commit, 'master', message) }.to raise_error(Gitlab::Git::Repository::CreateTreeError) end end context 'when commit can be cherry-picked' do it 'cherry-picks the changes' do - expect(repository.cherry_pick(user, pickable_commit, 'master')).to be_truthy + expect(repository.cherry_pick(user, pickable_commit, 'master', message)).to be_truthy end end @@ -1372,11 +1374,11 @@ describe Repository, models: true do it 'cherry-picks the changes' do expect(repository.blob_at_branch('improve/awesome', 'foo/bar/.gitkeep')).to be_nil - cherry_pick_commit_sha = repository.cherry_pick(user, pickable_merge, 'improve/awesome') + cherry_pick_commit_sha = repository.cherry_pick(user, pickable_merge, 'improve/awesome', message) cherry_pick_commit_message = project.commit(cherry_pick_commit_sha).message expect(repository.blob_at_branch('improve/awesome', 'foo/bar/.gitkeep')).not_to be_nil - expect(cherry_pick_commit_message).to include('cherry picked from') + expect(cherry_pick_commit_message).to eq(message) end end end |