Merge branch 'dm-fix-cherry-pick' into 'master'

Fix cherry-picking or reverting through an MR

Closes #28711 and #28426

See merge request !9640

app/controllers/concerns/creates_commit.rb

@@ -4,10 +4,9 @@ module CreatesCommit
   def create_commit(service, success_path:, failure_path:, failure_view: nil, success_notice: nil)
     set_commit_variables
 
-    start_branch = @mr_target_branch
     commit_params = @commit_params.merge(
       start_project: @mr_target_project,
-      start_branch: start_branch,
+      start_branch: @mr_target_branch,
       target_branch: @mr_source_branch
     )
 
@@ -17,12 +16,16 @@ module CreatesCommit
     if result[:status] == :success
       update_flash_notice(success_notice)
 
+      success_path = final_success_path(success_path)
+
       respond_to do |format|
-        format.html { redirect_to final_success_path(success_path) }
-        format.json { render json: { message: "success", filePath: final_success_path(success_path) } }
+        format.html { redirect_to success_path }
+        format.json { render json: { message: "success", filePath: success_path } }
       end
     else
       flash[:alert] = result[:message]
+      failure_path = failure_path.call if failure_path.respond_to?(:call)
+
       respond_to do |format|
         format.html do
           if failure_view
@@ -58,9 +61,13 @@ module CreatesCommit
   end
 
   def final_success_path(success_path)
-    return success_path unless create_merge_request?
-
+    if create_merge_request?
       merge_request_exists? ? existing_merge_request_path : new_merge_request_path
+    else
+      success_path = success_path.call if success_path.respond_to?(:call)
+
+      success_path
+    end
   end
 
   def new_merge_request_path
@@ -92,42 +99,26 @@ module CreatesCommit
   end
 
   def create_merge_request?
-    # XXX: Even if the field is set, if we're checking the same branch
+    # Even if the field is set, if we're checking the same branch
     # as the target branch in the same project,
     # we don't want to create a merge request.
     params[:create_merge_request].present? &&
-      (different_project? || @ref != @target_branch)
+      (different_project? || @mr_target_branch != @mr_source_branch)
   end
 
-  # TODO: We should really clean this up
   def set_commit_variables
-    @mr_source_project =
     if can?(current_user, :push_code, @project)
-        # Edit file in this project
-        @project
+      @mr_source_project = @project
+      @target_branch ||= @ref
     else
-        # Merge request from fork to this project
-        current_user.fork_of(@project)
+      @mr_source_project = current_user.fork_of(@project)
+      @target_branch ||= @mr_source_project.repository.next_branch('patch')
     end
 
     # Merge request to this project
     @mr_target_project = @project
-    @mr_target_branch = @ref || @target_branch
-
-    @mr_source_branch = guess_mr_source_branch
-  end
-
-  def guess_mr_source_branch
-    # XXX: Happens when viewing a commit without a branch. In this case,
-    # @target_branch would be the default branch for @mr_source_project,
-    # however we want a generated new branch here. Thus we can't use
-    # @target_branch, but should pass nil to indicate that we want a new
-    # branch instead of @target_branch.
-    return if
-      create_merge_request? &&
-          # XXX: Don't understand why rubocop prefers this indention
-          @mr_source_project.repository.branch_exists?(@target_branch)
+    @mr_target_branch ||= @ref || @target_branch
 
-    @target_branch
+    @mr_source_branch = @target_branch
   end
 end

app/controllers/projects/blob_controller.rb

@@ -24,7 +24,7 @@ class Projects::BlobController < Projects::ApplicationController
 
   def create
     create_commit(Files::CreateService, success_notice: "The file has been successfully created.",
-                                        success_path: namespace_project_blob_path(@project.namespace, @project, File.join(@target_branch, @file_path)),
+                                        success_path: -> { namespace_project_blob_path(@project.namespace, @project, File.join(@target_branch, @file_path)) },
                                         failure_view: :new,
                                         failure_path: namespace_project_new_blob_path(@project.namespace, @project, @ref))
   end
@@ -40,7 +40,7 @@ class Projects::BlobController < Projects::ApplicationController
 
   def update
     @path = params[:file_path] if params[:file_path].present?
-    create_commit(Files::UpdateService, success_path: after_edit_path,
+    create_commit(Files::UpdateService, success_path: -> { after_edit_path },
                                         failure_view: :edit,
                                         failure_path: namespace_project_blob_path(@project.namespace, @project, @id))
 
@@ -62,7 +62,7 @@ class Projects::BlobController < Projects::ApplicationController
 
   def destroy
     create_commit(Files::DestroyService, success_notice: "The file has been successfully deleted.",
-                                         success_path: namespace_project_tree_path(@project.namespace, @project, @target_branch),
+                                         success_path: -> { namespace_project_tree_path(@project.namespace, @project, @target_branch) },
                                          failure_view: :show,
                                          failure_path: namespace_project_blob_path(@project.namespace, @project, @id))
   end

app/controllers/projects/commit_controller.rb

@@ -51,23 +51,35 @@ class Projects::CommitController < Projects::ApplicationController
   def revert
     assign_change_commit_vars
 
-    return render_404 if @target_branch.blank?
+    return render_404 if @start_branch.blank?
+
+    @target_branch = create_new_branch? ? @commit.revert_branch_name : @start_branch
+
+    @mr_target_branch = @start_branch
 
     create_commit(Commits::RevertService, success_notice: "The #{@commit.change_type_title(current_user)} has been successfully reverted.",
-                                          success_path: successful_change_path, failure_path: failed_change_path)
+                                          success_path: -> { successful_change_path }, failure_path: failed_change_path)
   end
 
   def cherry_pick
     assign_change_commit_vars
 
-    return render_404 if @target_branch.blank?
+    return render_404 if @start_branch.blank?
+
+    @target_branch = create_new_branch? ? @commit.cherry_pick_branch_name : @start_branch
+
+    @mr_target_branch = @start_branch
 
     create_commit(Commits::CherryPickService, success_notice: "The #{@commit.change_type_title(current_user)} has been successfully cherry-picked.",
-                                              success_path: successful_change_path, failure_path: failed_change_path)
+                                              success_path: -> { successful_change_path }, failure_path: failed_change_path)
   end
 
   private
 
+  def create_new_branch?
+    params[:create_merge_request].present? || !can?(current_user, :push_code, @project)
+  end
+
   def successful_change_path
     referenced_merge_request_url || namespace_project_commits_url(@project.namespace, @project, @target_branch)
   end
@@ -78,7 +90,7 @@ class Projects::CommitController < Projects::ApplicationController
 
   def referenced_merge_request_url
     if merge_request = @commit.merged_merge_request(current_user)
-      namespace_project_merge_request_url(@project.namespace, @project, merge_request)
+      namespace_project_merge_request_url(merge_request.target_project.namespace, merge_request.target_project, merge_request)
     end
   end
 
@@ -118,11 +130,7 @@ class Projects::CommitController < Projects::ApplicationController
   end
 
   def assign_change_commit_vars
-    @commit = project.commit(params[:id])
-    @target_branch = params[:target_branch]
-    @commit_params = {
-      commit: @commit,
-      create_merge_request: params[:create_merge_request].present? || different_project?
-    }
+    @start_branch = params[:start_branch]
+    @commit_params = { commit: @commit }
   end
 end

app/models/repository.rb

@@ -6,6 +6,7 @@ class Repository
   attr_accessor :path_with_namespace, :project
 
   CommitError = Class.new(StandardError)
+  CreateTreeError = Class.new(StandardError)
 
   # Methods that cache data from the Git repository.
   #
@@ -862,17 +863,18 @@ class Repository
   end
 
   def revert(
-    user, commit, branch_name, revert_tree_id = nil,
+    user, commit, branch_name,
     start_branch_name: nil, start_project: project)
-    revert_tree_id ||= check_revert_content(commit, branch_name)
-
-    return false unless revert_tree_id
-
     GitOperationService.new(user, self).with_branch(
       branch_name,
       start_branch_name: start_branch_name,
       start_project: start_project) do |start_commit|
 
+      revert_tree_id = check_revert_content(commit, start_commit.sha)
+      unless revert_tree_id
+        raise Repository::CreateTreeError.new('Failed to revert commit')
+      end
+
       committer = user_to_committer(user)
 
       Rugged::Commit.create(rugged,
@@ -885,17 +887,18 @@ class Repository
   end
 
   def cherry_pick(
-    user, commit, branch_name, cherry_pick_tree_id = nil,
+    user, commit, branch_name,
     start_branch_name: nil, start_project: project)
-    cherry_pick_tree_id ||= check_cherry_pick_content(commit, branch_name)
-
-    return false unless cherry_pick_tree_id
-
     GitOperationService.new(user, self).with_branch(
       branch_name,
       start_branch_name: start_branch_name,
       start_project: start_project) do |start_commit|
 
+      cherry_pick_tree_id = check_cherry_pick_content(commit, start_commit.sha)
+      unless cherry_pick_tree_id
+        raise Repository::CreateTreeError.new('Failed to cherry-pick commit')
+      end
+
       committer = user_to_committer(user)
 
       Rugged::Commit.create(rugged,
@@ -919,8 +922,7 @@ class Repository
     end
   end
 
-  def check_revert_content(target_commit, branch_name)
-    source_sha = commit(branch_name).sha
+  def check_revert_content(target_commit, source_sha)
     args = [target_commit.sha, source_sha]
     args << { mainline: 1 } if target_commit.merge_commit?
 
@@ -933,8 +935,7 @@ class Repository
     tree_id
   end
 
-  def check_cherry_pick_content(target_commit, branch_name)
-    source_sha = commit(branch_name).sha
+  def check_cherry_pick_content(target_commit, source_sha)
     args = [target_commit.sha, source_sha]
     args << 1 if target_commit.merge_commit?
 

app/services/commits/change_service.rb

@@ -8,9 +8,9 @@ module Commits
       @start_branch = params[:start_branch]
       @target_branch = params[:target_branch]
       @commit = params[:commit]
-      @create_merge_request = params[:create_merge_request].present?
 
-      check_push_permissions unless @create_merge_request
+      check_push_permissions
+
       commit
     rescue Repository::CommitError, Gitlab::Git::Repository::InvalidBlobName, GitHooksService::PreReceiveError,
            ValidationError, ChangeError => ex
@@ -26,35 +26,22 @@ module Commits
     def commit_change(action)
       raise NotImplementedError unless repository.respond_to?(action)
 
-      if @create_merge_request
-        into = @commit.public_send("#{action}_branch_name")
-        tree_branch = @start_branch
-      else
-        into = tree_branch = @target_branch
-      end
-
-      tree_id = repository.public_send(
-        "check_#{action}_content", @commit, tree_branch)
-
-      if tree_id
-        validate_target_branch(into) if @create_merge_request
+      validate_target_branch if different_branch?
 
       repository.public_send(
         action,
         current_user,
         @commit,
-          into,
-          tree_id,
+        @target_branch,
         start_project: @start_project,
         start_branch_name: @start_branch)
 
       success
-      else
+    rescue Repository::CreateTreeError
       error_msg = "Sorry, we cannot #{action.to_s.dasherize} this #{@commit.change_type_title(current_user)} automatically.
                      A #{action.to_s.dasherize} may have already been performed with this #{@commit.change_type_title(current_user)}, or a more recent commit may have updated some of its content."
       raise ChangeError, error_msg
     end
-    end
 
     def check_push_permissions
       allowed = ::Gitlab::UserAccess.new(current_user, project: project).can_push_to_branch?(@target_branch)
@@ -66,16 +53,17 @@ module Commits
       true
     end
 
-    def validate_target_branch(new_branch)
-      # Temporary branch exists and contains the change commit
-      return if repository.find_branch(new_branch)
-
+    def validate_target_branch
       result = ValidateNewBranchService.new(@project, current_user)
-        .execute(new_branch)
+        .execute(@target_branch)
 
       if result[:status] == :error
         raise ChangeError, "There was an error creating the source branch: #{result[:message]}"
       end
     end
+
+    def different_branch?
+      @start_branch != @target_branch || @start_project != @project
+    end
   end
 end

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

 - case type.to_s
 - when 'revert'
   - label = 'Revert'
-  - target_label = 'Revert in branch'
+  - branch_label = 'Revert in branch'
 - when 'cherry-pick'
   - label = 'Cherry-pick'
-  - target_label = 'Pick into branch'
+  - branch_label = 'Pick into branch'
 
 .modal{ id: "modal-#{type}-commit" }
   .modal-dialog
@@ -15,10 +15,10 @@
       .modal-body
         = form_tag [type.underscore, @project.namespace.becomes(Namespace), @project, commit], method: :post, remote: false, class: "form-horizontal js-#{type}-form js-requires-input" do
           .form-group.branch
-            = label_tag 'target_branch', target_label, class: 'control-label'
+            = label_tag 'start_branch', branch_label, class: 'control-label'
             .col-sm-10
-              = hidden_field_tag :target_branch, @project.default_branch, id: 'target_branch'
-              = dropdown_tag(@project.default_branch, options: { title: "Switch branch", filter: true, placeholder: "Search branches", toggle_class: 'js-project-refs-dropdown js-target-branch dynamic', dropdown_class: 'dropdown-menu-selectable', data: { field_name: "target_branch", selected: @project.default_branch, target_branch: @project.default_branch, refs_url: namespace_project_branches_path(@project.namespace, @project), submit_form_on_click: false } })
+              = hidden_field_tag :start_branch, @project.default_branch, id: 'start_branch'
+              = dropdown_tag(@project.default_branch, options: { title: "Switch branch", filter: true, placeholder: "Search branches", toggle_class: 'js-project-refs-dropdown js-target-branch dynamic', dropdown_class: 'dropdown-menu-selectable', data: { field_name: "start_branch", selected: @project.default_branch, start_branch: @project.default_branch, refs_url: namespace_project_branches_path(@project.namespace, @project), submit_form_on_click: false } })
 
               - if can?(current_user, :push_code, @project)
                 .js-create-merge-request-container

changelogs/unreleased/dm-fix-cherry-pick.yml

+---
+title: Fix cherry-picking or reverting through an MR
+merge_request:
+author:

features/steps/project/commits/revert.rb

@@ -36,5 +36,6 @@ class Spinach::Features::RevertCommits < Spinach::FeatureSteps
 
   step 'I should see the new merge request notice' do
     page.should have_content('The commit has been successfully reverted. You can now submit a merge request to get this change into the original branch.')
+    page.should have_content("From revert-#{Commit.truncate_sha(sample_commit.id)} into master")
   end
 end

lib/api/commits.rb

@@ -127,7 +127,7 @@ module API
 
         commit_params = {
           commit: commit,
-          create_merge_request: false,
+          start_branch: params[:branch],
           target_branch: params[:branch]
         }
 

lib/api/v3/commits.rb

@@ -130,9 +130,7 @@ module API
 
           commit_params = {
             commit: commit,
-            create_merge_request: false,
-            source_project: user_project,
-            source_branch: commit.cherry_pick_branch_name,
+            start_branch: params[:branch],
             target_branch: params[:branch]
           }
 

spec/controllers/projects/commit_controller_spec.rb

@@ -166,7 +166,7 @@ describe Projects::CommitController do
         post(:revert,
             namespace_id: project.namespace,
             project_id: project,
-            target_branch: 'master',
+            start_branch: 'master',
             id: commit.id)
 
         expect(response).to redirect_to namespace_project_commits_path(project.namespace, project, 'master')
@@ -179,7 +179,7 @@ describe Projects::CommitController do
         post(:revert,
             namespace_id: project.namespace,
             project_id: project,
-            target_branch: 'master',
+            start_branch: 'master',
             id: commit.id)
       end
 
@@ -188,7 +188,7 @@ describe Projects::CommitController do
         post(:revert,
             namespace_id: project.namespace,
             project_id: project,
-            target_branch: 'master',
+            start_branch: 'master',
             id: commit.id)
 
         expect(response).to redirect_to namespace_project_commit_path(project.namespace, project, commit.id)
@@ -215,7 +215,7 @@ describe Projects::CommitController do
         post(:cherry_pick,
             namespace_id: project.namespace,
             project_id: project,
-            target_branch: 'master',
+            start_branch: 'master',
             id: master_pickable_commit.id)
 
         expect(response).to redirect_to namespace_project_commits_path(project.namespace, project, 'master')
@@ -228,7 +228,7 @@ describe Projects::CommitController do
         post(:cherry_pick,
             namespace_id: project.namespace,
             project_id: project,
-            target_branch: 'master',
+            start_branch: 'master',
             id: master_pickable_commit.id)
       end
 
@@ -237,7 +237,7 @@ describe Projects::CommitController do
         post(:cherry_pick,
             namespace_id: project.namespace,
             project_id: project,
-            target_branch: 'master',
+            start_branch: 'master',
             id: master_pickable_commit.id)
 
         expect(response).to redirect_to namespace_project_commit_path(project.namespace, project, master_pickable_commit.id)

spec/features/projects/commit/cherry_pick_spec.rb

@@ -60,6 +60,7 @@ describe 'Cherry-pick Commits' do
         click_button 'Cherry-pick'
       end
       expect(page).to have_content('The commit has been successfully cherry-picked. You can now submit a merge request to get this change into the original branch.')
+      expect(page).to have_content("From cherry-pick-#{master_pickable_commit.short_id} into master")
     end
   end
 

spec/models/repository_spec.rb

@@ -1112,16 +1112,16 @@ describe Repository, models: true do
     let(:update_image_commit) { repository.commit('2f63565e7aac07bcdadb654e253078b727143ec4') }
 
     context 'when there is a conflict' do
-      it 'aborts the operation' do
-        expect(repository.revert(user, new_image_commit, 'master')).to eq(false)
+      it 'raises an error' do
+        expect { repository.revert(user, new_image_commit, 'master') }.to raise_error(/Failed to/)
       end
     end
 
     context 'when commit was already reverted' do
-      it 'aborts the operation' do
+      it 'raises an error' do
         repository.revert(user, update_image_commit, 'master')
 
-        expect(repository.revert(user, update_image_commit, 'master')).to eq(false)
+        expect { repository.revert(user, update_image_commit, 'master') }.to raise_error(/Failed to/)
       end
     end
 
@@ -1148,16 +1148,16 @@ describe Repository, models: true do
     let(:pickable_merge) { repository.commit('e56497bb5f03a90a51293fc6d516788730953899') }
 
     context 'when there is a conflict' do
-      it 'aborts the operation' do
-        expect(repository.cherry_pick(user, conflict_commit, 'master')).to eq(false)
+      it 'raises an error' do
+        expect { repository.cherry_pick(user, conflict_commit, 'master') }.to raise_error(/Failed to/)
       end
     end
 
     context 'when commit was already cherry-picked' do
-      it 'aborts the operation' do
+      it 'raises an error' do
         repository.cherry_pick(user, pickable_commit, 'master')
 
-        expect(repository.cherry_pick(user, pickable_commit, 'master')).to eq(false)
+        expect { repository.cherry_pick(user, pickable_commit, 'master') }.to raise_error(/Failed to/)
       end
     end