Merge branch 'issue_3409' into 'master'

Add ability to revert changes introduced by Merge Requests or Commits

Closes #3409 

See merge request !1990

CHANGELOG

@@ -70,6 +70,7 @@ v 8.5.0 (unreleased)
     all Omniauth providers to do so.
   - Allow existing users to auto link their SAML credentials by logging in via SAML
   - Make it possible to erase a build (trace, artifacts) using UI and API
+  - Ability to revert changes from a Merge Request or Commit
 v 8.4.4
   - Update omniauth-saml gem to 1.4.2

Gemfile

@@ -50,7 +50,7 @@ gem "browser", '~> 1.0.0'
 # Extracting information from a git repository
 # Provide access to Gitlab::Git library
-gem "gitlab_git", '~> 8.1'
+gem "gitlab_git", '~> 8.2'
 # LDAP Auth
 # GitLab fork with several improvements to original library. For full list of changes

Gemfile.lock

@@ -340,7 +340,7 @@ GEM
       json
     get_process_mem (0.2.0)
     gherkin-ruby (0.3.2)
-    github-linguist (4.7.3)
+    github-linguist (4.7.5)
       charlock_holmes (~> 0.7.3)
       escape_utils (~> 1.1.0)
       mime-types (>= 1.19)
@@ -357,11 +357,11 @@ GEM
       posix-spawn (~> 0.3)
     gitlab_emoji (0.3.1)
       gemojione (~> 2.2, >= 2.2.1)
-    gitlab_git (8.1.0)
+    gitlab_git (8.2.0)
       activesupport (~> 4.0)
       charlock_holmes (~> 0.7.3)
       github-linguist (~> 4.7.0)
-      rugged (~> 0.23.3)
+      rugged (~> 0.24.0b13)
     gitlab_meta (7.0)
     gitlab_omniauth-ldap (1.2.1)
       net-ldap (~> 0.9)
@@ -700,7 +700,7 @@ GEM
     rubyntlm (0.5.2)
     rubypants (0.2.0)
     rufus-scheduler (3.1.10)
-    rugged (0.23.3)
+    rugged (0.24.0b13)
     safe_yaml (1.0.4)
     sanitize (2.1.0)
       nokogiri (>= 1.4.4)
@@ -932,7 +932,7 @@ DEPENDENCIES
   github-markup (~> 1.3.1)
   gitlab-flowdock-git-hook (~> 1.0.1)
   gitlab_emoji (~> 0.3.0)
-  gitlab_git (~> 8.1)
+  gitlab_git (~> 8.2)
   gitlab_meta (= 7.0)
   gitlab_omniauth-ldap (~> 1.2.1)
   gollum-lib (~> 4.1.0)

app/assets/stylesheets/pages/merge_requests.scss

@@ -237,3 +237,5 @@
     }
   }
 }
+
+

app/controllers/application_controller.rb

@@ -25,7 +25,7 @@ class ApplicationController < ActionController::Base
   helper_method :abilities, :can?, :current_application_settings
   helper_method :import_sources_enabled?, :github_import_enabled?, :github_import_configured?, :gitlab_import_enabled?, :gitlab_import_configured?, :bitbucket_import_enabled?, :bitbucket_import_configured?, :gitorious_import_enabled?, :google_code_import_enabled?, :fogbugz_import_enabled?, :git_import_enabled?
-  helper_method :repository
+  helper_method :repository, :can_collaborate_with_project?
   rescue_from Encoding::CompatibilityError do |exception|
     log_exception(exception)
@@ -410,6 +410,13 @@ class ApplicationController < ActionController::Base
     current_user.nil? && root_path == request.path
   end
+  def can_collaborate_with_project?(project = nil)
+    project ||= @project
+
+    can?(current_user, :push_code, project) ||
+      (current_user && current_user.already_forked?(project))
+  end
+
   private
   def set_default_sort

app/controllers/concerns/creates_commit.rb

@@ -13,17 +13,11 @@ module CreatesCommit
     result = service.new(@tree_edit_project, current_user, commit_params).execute
     if result[:status] == :success
-      flash[:notice] = success_notice || "Your changes have been successfully committed."
-
-      if create_merge_request?
-        success_path = new_merge_request_path
-        target = different_project? ? "project" : "branch"
-        flash[:notice] << " You can now submit a merge request to get this change into the original #{target}."
-      end
+      update_flash_notice(success_notice)
       respond_to do |format|
-        format.html { redirect_to success_path }
-        format.json { render json: { message: "success", filePath: success_path } }
+        format.html { redirect_to final_success_path(success_path) }
+        format.json { render json: { message: "success", filePath: final_success_path(success_path) } }
       end
     else
       flash[:alert] = result[:message]
@@ -41,14 +35,32 @@ module CreatesCommit
   end
   def authorize_edit_tree!
-    return if can?(current_user, :push_code, project)
-    return if current_user && current_user.already_forked?(project)
+    return if can_collaborate_with_project?
     access_denied!
   end
   private
+  def update_flash_notice(success_notice)
+    flash[:notice] = success_notice || "Your changes have been successfully committed."
+
+    if create_merge_request?
+      if merge_request_exists?
+        flash[:notice] = nil
+      else
+        target = different_project? ? "project" : "branch"
+        flash[:notice] << " You can now submit a merge request to get this change into the original #{target}."
+      end
+    end
+  end
+
+  def final_success_path(success_path)
+    return success_path unless create_merge_request?
+
+    merge_request_exists? ? existing_merge_request_path : new_merge_request_path
+  end
+
   def new_merge_request_path
     new_namespace_project_merge_request_path(
       @mr_source_project.namespace,
@@ -62,6 +74,19 @@ module CreatesCommit
     )
   end
+  def existing_merge_request_path
+    namespace_project_merge_request_path(@mr_target_project.namespace, @mr_target_project, @merge_request)
+  end
+
+  def merge_request_exists?
+    return @merge_request if defined?(@merge_request)
+
+    @merge_request = @mr_target_project.merge_requests.opened.find_by(
+      source_branch: @mr_source_branch,
+      target_branch: @mr_target_branch
+    )
+  end
+
   def different_project?
     @mr_source_project != @mr_target_project
   end
@@ -75,7 +100,7 @@ module CreatesCommit
   end
   def set_commit_variables
-    @mr_source_branch = @target_branch
+    @mr_source_branch ||= @target_branch
     if can?(current_user, :push_code, @project)
       # Edit file in this project
@@ -89,7 +114,7 @@ module CreatesCommit
       else
         # Merge request to this project
         @mr_target_project = @project
-        @mr_target_branch = @ref
+        @mr_target_branch ||= @ref
       end
     else
       # Edit file in fork
@@ -97,7 +122,7 @@ module CreatesCommit
       # Merge request from fork to this project
       @mr_source_project = @tree_edit_project
       @mr_target_project = @project
-      @mr_target_branch = @ref
+      @mr_target_branch  ||= @ref
     end
   end
 end

app/controllers/projects/commit_controller.rb

@@ -2,6 +2,8 @@
 #
 # Not to be confused with CommitsController, plural.
 class Projects::CommitController < Projects::ApplicationController
+  include CreatesCommit
+
   # Authorize
   before_action :require_non_empty_project
   before_action :authorize_download_code!, except: [:cancel_builds, :retry_builds]
@@ -9,6 +11,7 @@ class Projects::CommitController < Projects::ApplicationController
   before_action :authorize_read_commit_status!, only: [:builds]
   before_action :commit
   before_action :define_show_vars, only: [:show, :builds]
+  before_action :authorize_edit_tree!, only: [:revert]
   def show
     apply_diff_view_cookie!
@@ -55,8 +58,37 @@ class Projects::CommitController < Projects::ApplicationController
     render layout: false
   end
+  def revert
+    assign_revert_commit_vars
+
+    return render_404 if @target_branch.blank?
+
+    create_commit(Commits::RevertService, success_notice: "The #{revert_type_title} has been successfully reverted.",
+                                          success_path: successful_revert_path, failure_path: failed_revert_path)
+  end
+
   private
+  def revert_type_title
+    @commit.merged_merge_request ? 'merge request' : 'commit'
+  end
+
+  def successful_revert_path
+    return referenced_merge_request_url if @commit.merged_merge_request
+
+    namespace_project_commits_url(@project.namespace, @project, @target_branch)
+  end
+
+  def failed_revert_path
+    return referenced_merge_request_url if @commit.merged_merge_request
+
+    namespace_project_commit_url(@project.namespace, @project, params[:id])
+  end
+
+  def referenced_merge_request_url
+    namespace_project_merge_request_url(@project.namespace, @project, @commit.merged_merge_request)
+  end
+
   def commit
     @commit ||= @project.commit(params[:id])
   end
@@ -79,4 +111,16 @@ class Projects::CommitController < Projects::ApplicationController
     @statuses = ci_commit.statuses if ci_commit
   end
+
+  def assign_revert_commit_vars
+    @commit = project.commit(params[:id])
+    @target_branch = params[:target_branch]
+    @mr_source_branch = @commit.revert_branch_name
+    @mr_target_branch = @target_branch
+    @commit_params = {
+      commit: @commit,
+      revert_type_title: revert_type_title,
+      create_merge_request: params[:create_merge_request].present? || different_project?
+    }
+  end
 end

app/helpers/commits_helper.rb

@@ -123,6 +123,37 @@ module CommitsHelper
     )
   end
+  def revert_commit_link(commit, continue_to_path, btn_class: nil)
+    return unless current_user
+
+    tooltip = "Revert this #{revert_commit_type(commit)} in a new merge request"
+
+    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', title: tooltip, class: "btn btn-default btn-grouped btn-#{btn_class}"
+      end
+    elsif can?(current_user, :fork_project, @project)
+      continue_params = {
+        to: continue_to_path,
+        notice: edit_in_new_fork_notice + ' Try to revert this commit again.',
+        notice_now: edit_in_new_fork_notice_now
+      }
+      fork_path = namespace_project_forks_path(@project.namespace, @project,
+        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', title: tooltip
+    end
+  end
+
+  def revert_commit_type(commit)
+    if commit.merged_merge_request
+      'merge request'
+    else
+      'commit'
+    end
+  end
+
   protected
   # Private: Returns a link to a person. If the person has a matching user and

app/helpers/tree_helper.rb

@@ -56,8 +56,7 @@ module TreeHelper
     return false unless on_top_of_branch?(project, ref)
-    can?(current_user, :push_code, project) ||
-      (current_user && current_user.already_forked?(project))
+    can_collaborate_with_project?(project)
   end
   def tree_edit_branch(project = @project, ref = @ref)

app/models/commit.rb

@@ -215,6 +215,44 @@ class Commit
     ci_commit.try(:status) || :not_found
   end
+  def revert_branch_name
+    "revert-#{short_id}"
+  end
+
+  def revert_description
+    if merged_merge_request
+      "This reverts merge request #{merged_merge_request.to_reference}"
+    else
+      "This reverts commit #{sha}"
+    end
+  end
+
+  def revert_message
+    %Q{Revert "#{title}"\n\n#{revert_description}}
+  end
+
+  def reverts_commit?(commit)
+    description.include?(commit.revert_description)
+  end
+
+  def merge_commit?
+    parents.size > 1
+  end
+
+  def merged_merge_request
+    return @merged_merge_request if defined?(@merged_merge_request)
+
+    @merged_merge_request = project.merge_requests.find_by(merge_commit_sha: id) if merge_commit?
+  end
+
+  def has_been_reverted?(current_user = nil, noteable = self)
+    Gitlab::ReferenceExtractor.lazily do
+      noteable.notes.system.flat_map do |note|
+        note.all_references(current_user).commits
+      end
+    end.any? { |commit_ref| commit_ref.reverts_commit?(self) }
+  end
+
   private
   def repo_changes

app/models/merge_request.rb

@@ -24,6 +24,7 @@
 #  merge_params              :text
 #  merge_when_build_succeeds :boolean          default(FALSE), not null
 #  merge_user_id             :integer
+#  merge_commit_sha          :string
 #
 require Rails.root.join("app/models/commit")
@@ -532,4 +533,12 @@ class MergeRequest < ActiveRecord::Base
     [diff_base_commit, last_commit]
   end
+
+  def merge_commit
+    @merge_commit ||= project.commit(merge_commit_sha) if merge_commit_sha
+  end
+
+  def can_be_reverted?(current_user = nil)
+    merge_commit && !merge_commit.has_been_reverted?(current_user, self)
+  end
 end

app/models/repository.rb

@@ -619,6 +619,34 @@ 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?
+
+    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)
+
+    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
+  end
+
+  def diff_exists?(sha1, sha2)
+    rugged.diff(sha1, sha2).size > 0
+  end
+
   def merged_to_root_ref?(branch_name)
     branch_commit = commit(branch_name)
     root_ref_commit = commit(root_ref)
@@ -700,10 +728,11 @@ class Repository
     oldrev = Gitlab::Git::BLANK_SHA
     ref = Gitlab::Git::BRANCH_REF_PREFIX + branch
+    target_branch = find_branch(branch)
     was_empty = empty?
-    unless was_empty
-      oldrev = find_branch(branch).target
+    if !was_empty && target_branch
+      oldrev = target_branch.target
     end
     with_tmp_ref(oldrev) do |tmp_ref|
@@ -715,7 +744,7 @@ class Repository
       end
       GitHooksService.new.execute(current_user, path_to_repo, oldrev, newrev, ref) do
-        if was_empty
+        if was_empty || !target_branch
           # Create branch
           rugged.references.create(ref, newrev)
         else
@@ -730,6 +759,8 @@ class Repository
           end
         end
       end
+
+      newrev
     end
   end

app/services/commits/revert_service.rb

+module Commits
+  class RevertService < ::BaseService
+    class ValidationError < StandardError; end
+    class ReversionError < StandardError; end
+
+    def execute
+      @source_project = params[:source_project] || @project
+      @target_branch = params[:target_branch]
+      @commit = params[:commit]
+      @create_merge_request = params[:create_merge_request].present?
+
+      validate and commit
+    rescue Repository::CommitError, Gitlab::Git::Repository::InvalidBlobName, GitHooksService::PreReceiveError,
+           ValidationError, ReversionError => ex
+      error(ex.message)
+    end
+
+    def commit
+      revert_into = @create_merge_request ? @commit.revert_branch_name : @target_branch
+
+      if @create_merge_request
+        # Temporary branch exists and contains the revert commit
+        return success if repository.find_branch(revert_into)
+
+        create_target_branch
+      end
+
+      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 ReversionError, error_msg
+      end
+
+      success
+    end
+
+    private
+
+    def create_target_branch
+      result = CreateBranchService.new(@project, current_user)
+                                  .execute(@commit.revert_branch_name, @target_branch, source_project: @source_project)
+
+      if result[:status] == :error
+        raise ReversionError, "There was an error creating the source branch: #{result[:message]}"
+      end
+    end
+
+    def validate
+      allowed = ::Gitlab::GitAccess.new(current_user, project).can_push_to_branch?(@target_branch)
+
+      unless allowed
+        raise_error('You are not allowed to push into this branch')
+      end
+
+      true
+    end
+  end
+end

app/services/merge_requests/build_service.rb

@@ -56,7 +56,7 @@ module MergeRequests
       if commits && commits.count == 1
         commit = commits.first
         merge_request.title       = commit.title
-        merge_request.description = commit.description.try(:strip)
+        merge_request.description ||= commit.description.try(:strip)
       else
         merge_request.title = merge_request.source_branch.titleize.humanize
       end

app/services/merge_requests/merge_service.rb

@@ -34,7 +34,8 @@ module MergeRequests
         committer: committer
       }
-      repository.merge(current_user, merge_request.source_sha, merge_request.target_branch, options)
+      commit_id = repository.merge(current_user, merge_request.source_sha, merge_request.target_branch, options)
+      merge_request.update(merge_commit_sha: commit_id)
     rescue StandardError => e
       merge_request.update(merge_error: "Something went wrong during merge")
       Rails.logger.error(e.message)

app/views/projects/commit/_commit_box.html.haml

@@ -16,6 +16,8 @@
     = link_to namespace_project_tree_path(@project.namespace, @project, @commit), class: "btn btn-grouped" do
       = icon('files-o')
       Browse Files
+    - unless @commit.has_been_reverted?(current_user)
+      = revert_commit_link(@commit, namespace_project_commit_path(@project.namespace, @project, @commit.id))
   %div
 %p

app/views/projects/commit/_revert.html.haml

+#modal-revert-commit.modal
+  .modal-dialog
+    .modal-content
+      .modal-header
+        %a.close{href: "#", "data-dismiss" => "modal"} ×
+        %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-group.branch
+            = 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)
+                .js-create-merge-request-container
+                  .checkbox
+                    - nonce = SecureRandom.hex
+                    = label_tag "create_merge_request-#{nonce}" do
+                      = check_box_tag 'create_merge_request', 1, true, class: 'js-create-merge-request', id: "create_merge_request-#{nonce}"
+                      Start a <strong>new merge request</strong> with these changes
+              - else
+                = hidden_field_tag 'create_merge_request', 1
+          .form-actions
+            = submit_tag "Revert", class: 'btn btn-create'
+            = link_to "Cancel", '#', class: "btn btn-cancel", "data-dismiss" => "modal"
+
+            - unless can?(current_user, :push_code, @project)
+              .inline.prepend-left-10
+                = commit_in_fork_help
+
+:javascript
+  new NewCommitForm($('.js-create-dir-form'))

app/views/projects/commit/show.html.haml

@@ -12,3 +12,5 @@
 = render "projects/diffs/diffs", diffs: @diffs, project: @project,
          diff_refs: @diff_refs
 = render "projects/notes/notes_with_form"
+- if can_collaborate_with_project?
+  = render "projects/commit/revert", commit: @commit, title: @commit.title

app/views/projects/merge_requests/_show.html.haml

@@ -85,6 +85,8 @@
         = spinner
 = render 'shared/issuable/sidebar', issuable: @merge_request
+- if @merge_request.can_be_reverted?
+  = render "projects/commit/revert", commit: @merge_request.merge_commit, title: @merge_request.title
 :javascript
   var merge_request;

app/views/projects/merge_requests/widget/_merged.html.haml

@@ -8,20 +8,18 @@
         #{time_ago_with_tooltip(@merge_request.merge_event.created_at)}
     %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.
-
+        %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.
+        = render 'projects/merge_requests/widget/merged_buttons'
       - 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
-
+          = render 'projects/merge_requests/widget/merged_buttons', source_branch_exists: true
         .remove_source_branch_widget.failed.hide
           %p
             Failed to remove source branch '#{@merge_request.source_branch}'.