Some updates from last code review.

5 files changed, 30 insertions, 28 deletions

diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb
index 0fcb3063f29..e9d07614785 100644
--- a/app/controllers/projects/merge_requests_controller.rb
+++ b/app/controllers/projects/merge_requests_controller.rb
@@ -193,18 +193,18 @@ class Projects::MergeRequestsController < Projects::ApplicationController
   end
 
   def revert
-    target_branch_exists = @merge_request.target_branch_exists?
-    url_params = { merge_request: {
-      source_branch: @merge_request.revert_branch_name,
-      target_branch: @merge_request.target_branch,
-      source_project_id: @merge_request.target_project_id,
-      target_project_id: @merge_request.target_project_id,
-      description: @merge_request.revert_description
-    }}
-
-    if target_branch_exists
+    url_params = {
+      merge_request: { source_branch: @merge_request.revert_branch_name,
+                       target_branch: @merge_request.target_branch,
+                       source_project_id: @merge_request.target_project_id,
+                       target_project_id: @merge_request.target_project_id,
+                       description: @merge_request.revert_description }
+    }
+
+    if @merge_request.target_branch_exists? && @merge_request.merge_commit_sha.present?
       @repository.revert_merge(current_user, @merge_request.merge_commit_sha,
-                               @merge_request.revert_branch_name, @merge_request.revert_title)
+                               @merge_request.revert_branch_name, @merge_request.target_branch,
+                               @merge_request.revert_title)
 
       redirect_to new_namespace_project_merge_request_url(@project.namespace, @project, url_params)
     else
diff --git a/app/helpers/application_helper.rb b/app/helpers/application_helper.rb
index b7e7d0c5a3b..44a62143cf7 100644
--- a/app/helpers/application_helper.rb
+++ b/app/helpers/application_helper.rb
@@ -135,7 +135,7 @@ module ApplicationHelper
     # Skip if user removed branch right after that
     return false unless project.repository.branch_names.include?(event.branch_name)
 
-    return false if Gitlab::Git::REVERT_BRANCH_PATTERN === event.branch_name
+    return false if event.branch_name =~ Gitlab::Git::REVERT_BRANCH_PATTERN
 
     true
   end
diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb
index 15ebce359ce..4a7d57cbe10 100644
--- a/app/models/merge_request.rb
+++ b/app/models/merge_request.rb
@@ -535,11 +535,11 @@ class MergeRequest < ActiveRecord::Base
   end
 
   def revert_branch_name
-    "revert-#{iid}-#{target_branch}"
+    "revert-#{iid}-#{source_branch}"
   end
 
   def revert_title
-   "Revert \"#{title}\""
+    "Revert \"#{title}\""
   end
 
   def revert_description
diff --git a/app/models/repository.rb b/app/models/repository.rb
index 872066a02e4..cca7afadbec 100644
--- a/app/models/repository.rb
+++ b/app/models/repository.rb
@@ -622,25 +622,27 @@ class Repository
     merge_commit_sha
   end
 
-  def revert_merge(user, merge_commit_id, new_branch_name, commit_message)
+  def revert_merge(user, merge_commit_id, new_branch_name, target_branch, commit_message)
     # branch exists and it's highly probable that it has the revert commit
     return if find_branch(new_branch_name)
 
-    add_branch(user, new_branch_name, merge_commit_id)
+    target_sha = find_branch(target_branch).target
 
-    new_index = rugged.revert_commit(merge_commit_id, merge_commit_id, mainline: 1)
-    committer = user_to_committer(user)
+    commit_with_hooks(user, new_branch_name) do |ref|
+      new_index = rugged.revert_commit(merge_commit_id, target_sha, mainline: 1)
+      committer = user_to_committer(user)
 
-    options = {
-      message: commit_message,
-      author: committer,
-      committer: committer,
-      tree: new_index.write_tree(rugged),
-      parents: [rugged.lookup(merge_commit_id)],
-      update_ref: "refs/heads/#{new_branch_name}"
-    }
+      options = {
+        message: commit_message,
+        author: committer,
+        committer: committer,
+        tree: new_index.write_tree(rugged),
+        parents: [rugged.lookup(target_sha)],
+        update_ref: ref
+      }
 
-    Rugged::Commit.create(rugged, options)
+      Rugged::Commit.create(rugged, options)
+    end
   end
 
   def merged_to_root_ref?(branch_name)
diff --git a/app/views/projects/merge_requests/show/_mr_title.html.haml b/app/views/projects/merge_requests/show/_mr_title.html.haml
index e6be1a5a19b..5d29181631a 100644
--- a/app/views/projects/merge_requests/show/_mr_title.html.haml
+++ b/app/views/projects/merge_requests/show/_mr_title.html.haml
@@ -18,5 +18,5 @@
           Edit
       - if @merge_request.closed?
         = link_to 'Reopen', merge_request_path(@merge_request, merge_request: {state_event: :reopen }), method: :put, class: 'btn btn-nr btn-grouped btn-reopen reopen-mr-link', title: "Reopen merge request"
-      - if @merge_request.merged?
+      - if @merge_request.merged? && @merge_request.merge_commit_sha.present?
         = link_to 'Revert', revert_namespace_project_merge_request_path(@project.namespace, @project, @merge_request), method: :post, class: "btn btn-grouped btn-close", title: "Revert merge request"