Merge branch 'issue_13716' into 'master'

Check for conflicts before creating new revert branch

Fixes #13716 

See merge request !2953

3 files changed, 62 insertions, 27 deletions

diff --git a/app/models/repository.rb b/app/models/repository.rb
index a214a69d749..c135ab61f6a 100644
--- a/app/models/repository.rb
+++ b/app/models/repository.rb
@@ -654,30 +654,38 @@ 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)
 
-    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)
+    return false unless revert_tree_id
 
-    commit_with_hooks(user, target_branch) do |ref|
+    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: tree_id,
+        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?
+
+    tree_id = revert_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
diff --git a/app/services/commits/revert_service.rb b/app/services/commits/revert_service.rb
index 43d1c766e35..9cb918d7a2e 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]}"
diff --git a/spec/models/repository_spec.rb b/spec/models/repository_spec.rb
index 51ae2c04ed0..1c7d66398cb 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