Some updates from last code review.

9 files changed, 29 insertions, 34 deletions

diff --git a/app/assets/stylesheets/pages/merge_requests.scss b/app/assets/stylesheets/pages/merge_requests.scss
index 7d5a226122d..12fb030760c 100644
--- a/app/assets/stylesheets/pages/merge_requests.scss
+++ b/app/assets/stylesheets/pages/merge_requests.scss
@@ -238,10 +238,4 @@
   }
 }
 
-// Revert Merge Request modal
 
-#modal-revert-commit {
-  .js-create-merge-request-container {
-    line-height: $line-height-base;
-  }
-}
diff --git a/app/helpers/commits_helper.rb b/app/helpers/commits_helper.rb
index 172ac52c396..7ff539118d3 100644
--- a/app/helpers/commits_helper.rb
+++ b/app/helpers/commits_helper.rb
@@ -130,7 +130,7 @@ module CommitsHelper
 
     if can_collaborate_with_project?
       content_tag :span, 'data-toggle' => 'modal', 'data-target' => '#modal-revert-commit' do
-        link_to 'Revert', '#modal-revert-commit', 'data-toggle' => 'tooltip', 'data-original-title' => tooltip, class: "btn btn-close btn-#{btn_class}"
+        link_to 'Revert', '#modal-revert-commit', 'data-toggle' => 'tooltip', title: tooltip, class: "btn btn-default btn-grouped btn-#{btn_class}"
       end
     elsif can?(current_user, :fork_project, @project)
       continue_params = {
@@ -142,7 +142,7 @@ module CommitsHelper
         namespace_key: current_user.namespace.id,
         continue: continue_params)
 
-      link_to 'Revert', fork_path, class: 'btn btn-grouped btn-close', method: :post, 'data-toggle' => 'tooltip', 'data-original-title' => tooltip
+      link_to 'Revert', fork_path, class: 'btn btn-grouped btn-close', method: :post, 'data-toggle' => 'tooltip', title: tooltip
     end
   end
 
diff --git a/app/models/commit.rb b/app/models/commit.rb
index 628926e4886..e8528782b59 100644
--- a/app/models/commit.rb
+++ b/app/models/commit.rb
@@ -228,7 +228,7 @@ class Commit
   end
 
   def revert_message
-    "Revert \"#{title}\"" + "\n\n#{revert_description}"
+    %Q{Revert "#{title}"\n\n#{revert_description}}
   end
 
   def reverts_commit?(commit)
@@ -242,7 +242,7 @@ class Commit
   def merged_merge_request
     return @merged_merge_request if defined?(@merged_merge_request)
 
-    @merged_merge_request = merge_commit? && MergeRequest.find_by(merge_commit_sha: id)
+    @merged_merge_request = merge_commit? && project.merge_requests.find_by(merge_commit_sha: id)
   end
 
   def has_been_reverted?(current_user = nil, noteable = self)
diff --git a/app/models/repository.rb b/app/models/repository.rb
index e7eef6ea21e..be30a3b0906 100644
--- a/app/models/repository.rb
+++ b/app/models/repository.rb
@@ -626,9 +626,9 @@ class Repository
     args          << { mainline: 1 } if commit.merge_commit?
 
     revert_index = rugged.revert_commit(*args)
-    tree_id = revert_index.write_tree(rugged)
-
     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|
diff --git a/app/services/commits/revert_service.rb b/app/services/commits/revert_service.rb
index f4c6c55f615..43d1c766e35 100644
--- a/app/services/commits/revert_service.rb
+++ b/app/services/commits/revert_service.rb
@@ -28,7 +28,7 @@ module Commits
       unless repository.revert(current_user, @commit, revert_into)
         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_error(ReversionError, error_msg)
+        raise ReversionError, error_msg
       end
 
       success
@@ -41,19 +41,15 @@ module Commits
                                   .execute(@commit.revert_branch_name, @target_branch, source_project: @source_project)
 
       if result[:status] == :error
-        raise_error(ReversionError, "There was an error creating the source branch: #{result[:message]}")
+        raise ReversionError, "There was an error creating the source branch: #{result[:message]}"
       end
     end
 
-    def raise_error(klass, message)
-      raise klass.new(message)
-    end
-
     def validate
       allowed = ::Gitlab::GitAccess.new(current_user, project).can_push_to_branch?(@target_branch)
 
       unless allowed
-        raise_error(ValidationError, "You are not allowed to push into this branch")
+        raise_error('You are not allowed to push into this branch')
       end
 
       true
diff --git a/app/views/projects/commit/_revert.html.haml b/app/views/projects/commit/_revert.html.haml
index 5c9f6dbbccc..52ca3ed5b14 100644
--- a/app/views/projects/commit/_revert.html.haml
+++ b/app/views/projects/commit/_revert.html.haml
@@ -3,11 +3,11 @@
     .modal-content
       .modal-header
         %a.close{href: "#", "data-dismiss" => "modal"} ×
-        %h3.page-title== Revert: "#{title}"
+        %h3.page-title== Revert this #{revert_commit_type(commit)}
       .modal-body
-        = form_tag revert_namespace_project_commit_path(@project.namespace, @project, commit_id), method: :post, remote: false, class: 'form-horizontal js-create-dir-form js-requires-input' do
+        = form_tag revert_namespace_project_commit_path(@project.namespace, @project, commit.id), method: :post, remote: false, class: 'form-horizontal js-create-dir-form js-requires-input' do
           .form-group.branch
-            = label_tag 'target_branch', 'Revert in branch:', class: 'control-label'
+            = label_tag 'target_branch', 'Revert in branch', class: 'control-label'
             .col-sm-10
               = select_tag "target_branch", grouped_options_refs, class: "select2 select2-sm js-target-branch"
               - if can?(current_user, :push_code, @project)
diff --git a/app/views/projects/commit/show.html.haml b/app/views/projects/commit/show.html.haml
index 104934ef913..21e186120c3 100644
--- a/app/views/projects/commit/show.html.haml
+++ b/app/views/projects/commit/show.html.haml
@@ -13,4 +13,4 @@
          diff_refs: @diff_refs
 = render "projects/notes/notes_with_form"
 - if can_collaborate_with_project?
-  = render "projects/commit/revert", commit_id: @commit.id, title: @commit.title
+  = render "projects/commit/revert", commit: @commit, title: @commit.title
diff --git a/app/views/projects/merge_requests/_show.html.haml b/app/views/projects/merge_requests/_show.html.haml
index ce8e625c87f..4befb260859 100644
--- a/app/views/projects/merge_requests/_show.html.haml
+++ b/app/views/projects/merge_requests/_show.html.haml
@@ -86,7 +86,7 @@
 
 = render 'shared/issuable/sidebar', issuable: @merge_request
 - if @merge_request.merge_commit && !@merge_request.has_been_reverted?(current_user)
-  = render "projects/commit/revert", commit_id: @merge_request.merge_commit.id, title: @merge_request.title
+  = render "projects/commit/revert", commit: @merge_request.merge_commit, title: @merge_request.title
 
 :javascript
   var merge_request;
diff --git a/app/views/projects/merge_requests/widget/_merged.html.haml b/app/views/projects/merge_requests/widget/_merged.html.haml
index a12b6690397..09b1db5358e 100644
--- a/app/views/projects/merge_requests/widget/_merged.html.haml
+++ b/app/views/projects/merge_requests/widget/_merged.html.haml
@@ -6,24 +6,29 @@
       - if @merge_request.merge_event
         by #{link_to_member(@project, @merge_request.merge_event.author, avatar: true)}
         #{time_ago_with_tooltip(@merge_request.merge_event.created_at)}
-      - if @merge_request.merge_commit && !@merge_request.has_been_reverted?(current_user)
-        = revert_commit_link(@merge_request.merge_commit, namespace_project_merge_request_path(@project.namespace, @project, @merge_request), btn_class: 'sm')
     %div
-      - if !@merge_request.source_branch_exists? || (params[:delete_source] == 'true')
-        The changes were merged into
-        #{link_to @merge_request.target_branch, namespace_project_commits_path(@project.namespace, @project, @merge_request.target_branch), class: "label-branch"}.
-        The source branch has been removed.
+      - revert_button = capture_haml do
+        - if @merge_request.merge_commit && !@merge_request.has_been_reverted?(current_user)
+          = revert_commit_link(@merge_request.merge_commit, namespace_project_merge_request_path(@project.namespace, @project, @merge_request), btn_class: 'sm')
 
+      - if !@merge_request.source_branch_exists? || (params[:delete_source] == 'true')
+        %p
+          The changes were merged into
+          #{link_to @merge_request.target_branch, namespace_project_commits_path(@project.namespace, @project, @merge_request.target_branch), class: "label-branch"}.
+          The source branch has been removed.
+        - if revert_button.present?
+          .btn-group= revert_button
       - elsif @merge_request.can_remove_source_branch?(current_user)
         .remove_source_branch_widget
           %p
             The changes were merged into
             #{link_to @merge_request.target_branch, namespace_project_commits_path(@project.namespace, @project, @merge_request.target_branch), class: "label-branch"}.
             You can remove the source branch now.
-          = link_to namespace_project_branch_path(@merge_request.source_project.namespace, @merge_request.source_project, @merge_request.source_branch), remote: true, method: :delete, class: "btn btn-primary btn-sm remove_source_branch" do
-            %i.fa.fa-times
-            Remove Source Branch
-
+          .btn-group
+            = link_to namespace_project_branch_path(@merge_request.source_project.namespace, @merge_request.source_project, @merge_request.source_branch), remote: true, method: :delete, class: "btn btn-default btn-grouped btn-sm remove_source_branch" do
+              %i.fa.fa-trash-o
+              Remove Source Branch
+            = revert_button
         .remove_source_branch_widget.failed.hide
           %p
             Failed to remove source branch '#{@merge_request.source_branch}'.