From 0c116d8ed6736de0d08ca838ae03aeca7dcaf142 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rub=C3=A9n=20D=C3=A1vila?= Date: Wed, 24 Feb 2016 12:00:34 -0500 Subject: [PATCH 1/2] Check for conflicts before creating target branch. --- app/models/repository.rb | 38 ++++++++++++++++---------- app/services/commits/revert_service.rb | 22 +++++++-------- 2 files changed, 34 insertions(+), 26 deletions(-) diff --git a/app/models/repository.rb b/app/models/repository.rb index a214a69d74..16a8aa3ad0 100644 --- a/app/models/repository.rb +++ b/app/models/repository.rb @@ -654,11 +654,28 @@ class Repository end end - def revert(user, commit, base_branch, target_branch = nil) - source_sha = find_branch(base_branch).target - target_branch ||= base_branch - args = [commit.id, source_sha] - args << { mainline: 1 } if commit.merge_commit? + def revert(user, commit, base_branch, revert_tree_id = nil) + source_sha = find_branch(base_branch).target + revert_tree_id = check_revert_content(commit, base_branch) unless revert_tree_id + + return false unless revert_tree_id + + commit_with_hooks(user, base_branch) do |ref| + committer = user_to_committer(user) + source_sha = Rugged::Commit.create(rugged, + message: commit.revert_message, + author: committer, + committer: committer, + tree: revert_tree_id, + parents: [rugged.lookup(source_sha)], + update_ref: ref) + 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? @@ -666,16 +683,7 @@ class Repository tree_id = revert_index.write_tree(rugged) return false unless diff_exists?(source_sha, tree_id) - commit_with_hooks(user, target_branch) do |ref| - committer = user_to_committer(user) - source_sha = Rugged::Commit.create(rugged, - message: commit.revert_message, - author: committer, - committer: committer, - tree: tree_id, - parents: [rugged.lookup(source_sha)], - update_ref: ref) - end + tree_id end def diff_exists?(sha1, sha2) diff --git a/app/services/commits/revert_service.rb b/app/services/commits/revert_service.rb index 43d1c766e3..9cb918d7a2 100644 --- a/app/services/commits/revert_service.rb +++ b/app/services/commits/revert_service.rb @@ -17,28 +17,28 @@ module Commits def commit 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 - # Temporary branch exists and contains the revert commit - return success if repository.find_branch(revert_into) + if revert_tree_id + create_target_branch(revert_into) if @create_merge_request - create_target_branch - end - - unless repository.revert(current_user, @commit, revert_into) + repository.revert(current_user, @commit, revert_into, revert_tree_id) + success + else 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." raise ReversionError, error_msg end - - success end 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) - .execute(@commit.revert_branch_name, @target_branch, source_project: @source_project) + .execute(new_branch, @target_branch, source_project: @source_project) if result[:status] == :error raise ReversionError, "There was an error creating the source branch: #{result[:message]}" From d9e01915924836722b4ed67e26535076efba1be4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rub=C3=A9n=20D=C3=A1vila?= Date: Tue, 1 Mar 2016 19:14:52 -0500 Subject: [PATCH 2/2] Add some specs for Repository#revert. --- app/models/repository.rb | 2 +- spec/models/repository_spec.rb | 35 ++++++++++++++++++++++++++++++---- 2 files changed, 32 insertions(+), 5 deletions(-) diff --git a/app/models/repository.rb b/app/models/repository.rb index 16a8aa3ad0..c135ab61f6 100644 --- a/app/models/repository.rb +++ b/app/models/repository.rb @@ -656,7 +656,7 @@ class Repository def revert(user, commit, base_branch, revert_tree_id = nil) source_sha = find_branch(base_branch).target - revert_tree_id = check_revert_content(commit, base_branch) unless revert_tree_id + revert_tree_id ||= check_revert_content(commit, base_branch) return false unless revert_tree_id diff --git a/spec/models/repository_spec.rb b/spec/models/repository_spec.rb index 51ae2c04ed..1c7d66398c 100644 --- a/spec/models/repository_spec.rb +++ b/spec/models/repository_spec.rb @@ -457,11 +457,38 @@ describe Repository, models: true do end end - describe '#revert_merge' do - it 'should revert the changes' do - repository.revert(user, merge_commit, 'master') + describe '#revert' do + let(:new_image_commit) { repository.commit('33f3729a45c02fc67d00adb1b8bca394b0e761d9') } + 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