Commit ca8a44be authored by Douwe Maan's avatar Douwe Maan

Merge branch 'issue_13716' into 'master'

Check for conflicts before creating new revert branch

Fixes #13716 

See merge request !2953
parents 8eadb373 d9e01915
...@@ -654,30 +654,38 @@ class Repository ...@@ -654,30 +654,38 @@ class Repository
end end
end end
def revert(user, commit, base_branch, target_branch = nil) def revert(user, commit, base_branch, revert_tree_id = nil)
source_sha = find_branch(base_branch).target source_sha = find_branch(base_branch).target
target_branch ||= base_branch revert_tree_id ||= check_revert_content(commit, base_branch)
args = [commit.id, source_sha]
args << { mainline: 1 } if commit.merge_commit?
revert_index = rugged.revert_commit(*args) return false unless revert_tree_id
return false if revert_index.conflicts?
tree_id = revert_index.write_tree(rugged)
return false unless diff_exists?(source_sha, tree_id)
commit_with_hooks(user, target_branch) do |ref| commit_with_hooks(user, base_branch) do |ref|
committer = user_to_committer(user) committer = user_to_committer(user)
source_sha = Rugged::Commit.create(rugged, source_sha = Rugged::Commit.create(rugged,
message: commit.revert_message, message: commit.revert_message,
author: committer, author: committer,
committer: committer, committer: committer,
tree: tree_id, tree: revert_tree_id,
parents: [rugged.lookup(source_sha)], parents: [rugged.lookup(source_sha)],
update_ref: ref) update_ref: ref)
end end
end end
def check_revert_content(commit, base_branch)
source_sha = find_branch(base_branch).target
args = [commit.id, source_sha]
args << { mainline: 1 } if 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 diff_exists?(sha1, sha2) def diff_exists?(sha1, sha2)
rugged.diff(sha1, sha2).size > 0 rugged.diff(sha1, sha2).size > 0
end end
......
...@@ -17,28 +17,28 @@ module Commits ...@@ -17,28 +17,28 @@ module Commits
def commit def commit
revert_into = @create_merge_request ? @commit.revert_branch_name : @target_branch revert_into = @create_merge_request ? @commit.revert_branch_name : @target_branch
revert_tree_id = repository.check_revert_content(@commit, @target_branch)
if @create_merge_request if revert_tree_id
# Temporary branch exists and contains the revert commit create_target_branch(revert_into) if @create_merge_request
return success if repository.find_branch(revert_into)
create_target_branch repository.revert(current_user, @commit, revert_into, revert_tree_id)
end success
else
unless repository.revert(current_user, @commit, revert_into)
error_msg = "Sorry, we cannot revert this #{params[:revert_type_title]} automatically. error_msg = "Sorry, we cannot revert this #{params[:revert_type_title]} automatically.
It may have already been reverted, or a more recent commit may have updated some of its content." It may have already been reverted, or a more recent commit may have updated some of its content."
raise ReversionError, error_msg raise ReversionError, error_msg
end end
success
end end
private private
def create_target_branch def create_target_branch(new_branch)
# Temporary branch exists and contains the revert commit
return success if repository.find_branch(new_branch)
result = CreateBranchService.new(@project, current_user) result = CreateBranchService.new(@project, current_user)
.execute(@commit.revert_branch_name, @target_branch, source_project: @source_project) .execute(new_branch, @target_branch, source_project: @source_project)
if result[:status] == :error if result[:status] == :error
raise ReversionError, "There was an error creating the source branch: #{result[:message]}" raise ReversionError, "There was an error creating the source branch: #{result[:message]}"
......
...@@ -457,11 +457,38 @@ describe Repository, models: true do ...@@ -457,11 +457,38 @@ describe Repository, models: true do
end end
end end
describe '#revert_merge' do describe '#revert' do
it 'should revert the changes' do let(:new_image_commit) { repository.commit('33f3729a45c02fc67d00adb1b8bca394b0e761d9') }
repository.revert(user, merge_commit, 'master') let(:update_image_commit) { repository.commit('2f63565e7aac07bcdadb654e253078b727143ec4') }
expect(repository.blob_at_branch('master', 'files/ruby/feature.rb')).not_to be_present context 'when there is a conflict' do
it 'should abort the operation' do
expect(repository.revert(user, new_image_commit, 'master')).to eq(false)
end
end
context 'when commit was already reverted' do
it 'should abort the operation' do
repository.revert(user, update_image_commit, 'master')
expect(repository.revert(user, update_image_commit, 'master')).to eq(false)
end
end
context 'when commit can be reverted' do
it 'should revert the changes' do
expect(repository.revert(user, update_image_commit, 'master')).to be_truthy
end
end
context 'reverting a merge commit' do
it 'should revert the changes' do
merge_commit
expect(repository.blob_at_branch('master', 'files/ruby/feature.rb')).to be_present
repository.revert(user, merge_commit, 'master')
expect(repository.blob_at_branch('master', 'files/ruby/feature.rb')).not_to be_present
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