diff options
author | Robert Speicher <robert@gitlab.com> | 2017-03-03 15:50:03 +0000 |
---|---|---|
committer | Felipe Artur <felipefac@gmail.com> | 2017-03-03 18:34:39 -0300 |
commit | f56366f1b86fc2a2ce2050a284c1b4b2402964d9 (patch) | |
tree | 424915c24c2e57346cf510d6878cabe9a4da81e4 | |
parent | bc1e2cc21dba3d216f7375b4fccc9e805dae713f (diff) | |
download | gitlab-ce-f56366f1b86fc2a2ce2050a284c1b4b2402964d9.tar.gz |
Merge branch 'dm-fix-cherry-pick' into 'master'
Fix cherry-picking or reverting through an MR
Closes #28711 and #28426
See merge request !9640
-rw-r--r-- | app/controllers/concerns/creates_commit.rb | 51 | ||||
-rw-r--r-- | app/controllers/projects/blob_controller.rb | 12 | ||||
-rw-r--r-- | app/controllers/projects/commit_controller.rb | 32 | ||||
-rw-r--r-- | app/models/repository.rb | 33 | ||||
-rw-r--r-- | app/services/commits/change_service.rb | 52 | ||||
-rw-r--r-- | app/views/projects/commit/_change.html.haml | 10 | ||||
-rw-r--r-- | changelogs/unreleased/dm-fix-cherry-pick.yml | 4 | ||||
-rw-r--r-- | features/steps/project/commits/revert.rb | 1 | ||||
-rw-r--r-- | lib/api/commits.rb | 2 | ||||
-rw-r--r-- | lib/api/v3/commits.rb | 196 | ||||
-rw-r--r-- | spec/controllers/projects/commit_controller_spec.rb | 36 | ||||
-rw-r--r-- | spec/features/projects/commit/cherry_pick_spec.rb | 1 | ||||
-rw-r--r-- | spec/models/repository_spec.rb | 16 |
13 files changed, 316 insertions, 130 deletions
diff --git a/app/controllers/concerns/creates_commit.rb b/app/controllers/concerns/creates_commit.rb index 88d180fcc2e..9ac8197e45a 100644 --- a/app/controllers/concerns/creates_commit.rb +++ b/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 unless initial_commit? 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) - merge_request_exists? ? existing_merge_request_path : new_merge_request_path + success_path + end end def new_merge_request_path @@ -92,46 +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 if can?(current_user, :push_code, @project) - # Edit file in this project @mr_source_project = @project + @target_branch ||= @ref else - # Merge request from fork to this 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 initial_commit? - @mr_target_branch.nil? || - !@mr_target_project.repository.branch_exists?(@mr_target_branch) - end + @mr_target_branch ||= @ref || @target_branch - 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) - - @target_branch + @mr_source_branch = @target_branch end end diff --git a/app/controllers/projects/blob_controller.rb b/app/controllers/projects/blob_controller.rb index 4c39fe98028..67099bf4ab6 100644 --- a/app/controllers/projects/blob_controller.rb +++ b/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)) @@ -61,10 +61,10 @@ class Projects::BlobController < Projects::ApplicationController end def destroy - create_commit(Files::DeleteService, success_notice: "The file has been successfully deleted.", - success_path: namespace_project_tree_path(@project.namespace, @project, @target_branch), - failure_view: :show, - failure_path: namespace_project_blob_path(@project.namespace, @project, @id)) + create_commit(Files::DestroyService, success_notice: "The file has been successfully deleted.", + 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 def diff diff --git a/app/controllers/projects/commit_controller.rb b/app/controllers/projects/commit_controller.rb index e10d7992db7..cc67f688d51 100644 --- a/app/controllers/projects/commit_controller.rb +++ b/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 @@ -94,7 +106,7 @@ class Projects::CommitController < Projects::ApplicationController @diffs = commit.diffs(opts) @notes_count = commit.notes.count - + @environment = EnvironmentsFinder.new(@project, current_user, commit: @commit).execute.last 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 diff --git a/app/models/repository.rb b/app/models/repository.rb index 56c582cd9be..1a6418a4491 100644 --- a/app/models/repository.rb +++ b/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. # @@ -941,17 +942,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, @@ -964,17 +966,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, @@ -998,9 +1001,8 @@ class Repository end end - def check_revert_content(target_commit, branch_name) - source_sha = commit(branch_name).sha - args = [target_commit.sha, source_sha] + def check_revert_content(target_commit, source_sha) + args = [target_commit.sha, source_sha] args << { mainline: 1 } if target_commit.merge_commit? revert_index = rugged.revert_commit(*args) @@ -1012,9 +1014,8 @@ class Repository tree_id end - def check_cherry_pick_content(target_commit, branch_name) - source_sha = commit(branch_name).sha - args = [target_commit.sha, source_sha] + def check_cherry_pick_content(target_commit, source_sha) + args = [target_commit.sha, source_sha] args << 1 if target_commit.merge_commit? cherry_pick_index = rugged.cherrypick_commit(*args) diff --git a/app/services/commits/change_service.rb b/app/services/commits/change_service.rb index 25e22f14e60..b1e4c214cf2 100644 --- a/app/services/commits/change_service.rb +++ b/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,34 +26,21 @@ 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, - start_project: @start_project, - start_branch_name: @start_branch) + repository.public_send( + action, + current_user, + @commit, + @target_branch, + start_project: @start_project, + start_branch_name: @start_branch) - success - else - error_msg = "Sorry, we cannot #{action.to_s.dasherize} this #{@commit.change_type_title(current_user)} automatically. + success + 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 + raise ChangeError, error_msg end def check_push_permissions @@ -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 diff --git a/app/views/projects/commit/_change.html.haml b/app/views/projects/commit/_change.html.haml index 421b3db342d..2ebd4f9069a 100644 --- a/app/views/projects/commit/_change.html.haml +++ b/app/views/projects/commit/_change.html.haml @@ -1,10 +1,10 @@ - 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 diff --git a/changelogs/unreleased/dm-fix-cherry-pick.yml b/changelogs/unreleased/dm-fix-cherry-pick.yml new file mode 100644 index 00000000000..e924b821d7e --- /dev/null +++ b/changelogs/unreleased/dm-fix-cherry-pick.yml @@ -0,0 +1,4 @@ +--- +title: Fix cherry-picking or reverting through an MR +merge_request: +author: diff --git a/features/steps/project/commits/revert.rb b/features/steps/project/commits/revert.rb index 94a5d4e2e4d..c9746407344 100644 --- a/features/steps/project/commits/revert.rb +++ b/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 diff --git a/lib/api/commits.rb b/lib/api/commits.rb index 173083d0ade..b323d9aa5a8 100644 --- a/lib/api/commits.rb +++ b/lib/api/commits.rb @@ -138,7 +138,7 @@ module API commit_params = { commit: commit, - create_merge_request: false, + start_branch: params[:branch], target_branch: params[:branch] } diff --git a/lib/api/v3/commits.rb b/lib/api/v3/commits.rb new file mode 100644 index 00000000000..d254d247042 --- /dev/null +++ b/lib/api/v3/commits.rb @@ -0,0 +1,196 @@ +require 'mime/types' + +module API + module V3 + class Commits < Grape::API + include PaginationParams + + before { authenticate! } + before { authorize! :download_code, user_project } + + params do + requires :id, type: String, desc: 'The ID of a project' + end + resource :projects do + desc 'Get a project repository commits' do + success ::API::Entities::RepoCommit + end + params do + optional :ref_name, type: String, desc: 'The name of a repository branch or tag, if not given the default branch is used' + optional :since, type: DateTime, desc: 'Only commits after or in this date will be returned' + optional :until, type: DateTime, desc: 'Only commits before or in this date will be returned' + optional :page, type: Integer, default: 0, desc: 'The page for pagination' + optional :per_page, type: Integer, default: 20, desc: 'The number of results per page' + optional :path, type: String, desc: 'The file path' + end + get ":id/repository/commits" do + ref = params[:ref_name] || user_project.try(:default_branch) || 'master' + offset = params[:page] * params[:per_page] + + commits = user_project.repository.commits(ref, + path: params[:path], + limit: params[:per_page], + offset: offset, + after: params[:since], + before: params[:until]) + + present commits, with: ::API::Entities::RepoCommit + end + + desc 'Commit multiple file changes as one commit' do + success ::API::Entities::RepoCommitDetail + detail 'This feature was introduced in GitLab 8.13' + end + params do + requires :branch_name, type: String, desc: 'The name of branch' + requires :commit_message, type: String, desc: 'Commit message' + requires :actions, type: Array[Hash], desc: 'Actions to perform in commit' + optional :author_email, type: String, desc: 'Author email for commit' + optional :author_name, type: String, desc: 'Author name for commit' + end + post ":id/repository/commits" do + authorize! :push_code, user_project + + attrs = declared_params.dup + branch = attrs.delete(:branch_name) + attrs.merge!(branch: branch, start_branch: branch, target_branch: branch) + + result = ::Files::MultiService.new(user_project, current_user, attrs).execute + + if result[:status] == :success + commit_detail = user_project.repository.commits(result[:result], limit: 1).first + present commit_detail, with: ::API::Entities::RepoCommitDetail + else + render_api_error!(result[:message], 400) + end + end + + desc 'Get a specific commit of a project' do + success ::API::Entities::RepoCommitDetail + failure [[404, 'Not Found']] + end + params do + requires :sha, type: String, desc: 'A commit sha, or the name of a branch or tag' + end + get ":id/repository/commits/:sha" do + commit = user_project.commit(params[:sha]) + + not_found! "Commit" unless commit + + present commit, with: ::API::Entities::RepoCommitDetail + end + + desc 'Get the diff for a specific commit of a project' do + failure [[404, 'Not Found']] + end + params do + requires :sha, type: String, desc: 'A commit sha, or the name of a branch or tag' + end + get ":id/repository/commits/:sha/diff" do + commit = user_project.commit(params[:sha]) + + not_found! "Commit" unless commit + + commit.raw_diffs.to_a + end + + desc "Get a commit's comments" do + success ::API::Entities::CommitNote + failure [[404, 'Not Found']] + end + params do + use :pagination + requires :sha, type: String, desc: 'A commit sha, or the name of a branch or tag' + end + get ':id/repository/commits/:sha/comments' do + commit = user_project.commit(params[:sha]) + + not_found! 'Commit' unless commit + notes = Note.where(commit_id: commit.id).order(:created_at) + + present paginate(notes), with: ::API::Entities::CommitNote + end + + desc 'Cherry pick commit into a branch' do + detail 'This feature was introduced in GitLab 8.15' + success ::API::Entities::RepoCommit + end + params do + requires :sha, type: String, desc: 'A commit sha to be cherry picked' + requires :branch, type: String, desc: 'The name of the branch' + end + post ':id/repository/commits/:sha/cherry_pick' do + authorize! :push_code, user_project + + commit = user_project.commit(params[:sha]) + not_found!('Commit') unless commit + + branch = user_project.repository.find_branch(params[:branch]) + not_found!('Branch') unless branch + + commit_params = { + commit: commit, + start_branch: params[:branch], + target_branch: params[:branch] + } + + result = ::Commits::CherryPickService.new(user_project, current_user, commit_params).execute + + if result[:status] == :success + branch = user_project.repository.find_branch(params[:branch]) + present user_project.repository.commit(branch.dereferenced_target), with: ::API::Entities::RepoCommit + else + render_api_error!(result[:message], 400) + end + end + + desc 'Post comment to commit' do + success ::API::Entities::CommitNote + end + params do + requires :sha, type: String, regexp: /\A\h{6,40}\z/, desc: "The commit's SHA" + requires :note, type: String, desc: 'The text of the comment' + optional :path, type: String, desc: 'The file path' + given :path do + requires :line, type: Integer, desc: 'The line number' + requires :line_type, type: String, values: %w(new old), default: 'new', desc: 'The type of the line' + end + end + post ':id/repository/commits/:sha/comments' do + commit = user_project.commit(params[:sha]) + not_found! 'Commit' unless commit + + opts = { + note: params[:note], + noteable_type: 'Commit', + commit_id: commit.id + } + + if params[:path] + commit.raw_diffs(all_diffs: true).each do |diff| + next unless diff.new_path == params[:path] + lines = Gitlab::Diff::Parser.new.parse(diff.diff.each_line) + + lines.each do |line| + next unless line.new_pos == params[:line] && line.type == params[:line_type] + break opts[:line_code] = Gitlab::Diff::LineCode.generate(diff.new_path, line.new_pos, line.old_pos) + end + + break if opts[:line_code] + end + + opts[:type] = LegacyDiffNote.name if opts[:line_code] + end + + note = ::Notes::CreateService.new(user_project, current_user, opts).execute + + if note.save + present note, with: ::API::Entities::CommitNote + else + render_api_error!("Failed to save note #{note.errors.messages}", 400) + end + end + end + end + end +end diff --git a/spec/controllers/projects/commit_controller_spec.rb b/spec/controllers/projects/commit_controller_spec.rb index ebd2d0e092b..c93185ddf51 100644 --- a/spec/controllers/projects/commit_controller_spec.rb +++ b/spec/controllers/projects/commit_controller_spec.rb @@ -164,9 +164,9 @@ describe Projects::CommitController do context 'when the revert was successful' do it 'redirects to the commits page' do post(:revert, - namespace_id: project.namespace.to_param, - project_id: project.to_param, - target_branch: 'master', + namespace_id: project.namespace, + project_id: project, + start_branch: 'master', id: commit.id) expect(response).to redirect_to namespace_project_commits_path(project.namespace, project, 'master') @@ -177,18 +177,18 @@ describe Projects::CommitController do context 'when the revert failed' do before do post(:revert, - namespace_id: project.namespace.to_param, - project_id: project.to_param, - target_branch: 'master', + namespace_id: project.namespace, + project_id: project, + start_branch: 'master', id: commit.id) end it 'redirects to the commit page' do # Reverting a commit that has been already reverted. post(:revert, - namespace_id: project.namespace.to_param, - project_id: project.to_param, - target_branch: 'master', + namespace_id: project.namespace, + project_id: project, + start_branch: 'master', id: commit.id) expect(response).to redirect_to namespace_project_commit_path(project.namespace, project, commit.id) @@ -213,9 +213,9 @@ describe Projects::CommitController do context 'when the cherry-pick was successful' do it 'redirects to the commits page' do post(:cherry_pick, - namespace_id: project.namespace.to_param, - project_id: project.to_param, - target_branch: 'master', + namespace_id: project.namespace, + project_id: project, + start_branch: 'master', id: master_pickable_commit.id) expect(response).to redirect_to namespace_project_commits_path(project.namespace, project, 'master') @@ -226,18 +226,18 @@ describe Projects::CommitController do context 'when the cherry_pick failed' do before do post(:cherry_pick, - namespace_id: project.namespace.to_param, - project_id: project.to_param, - target_branch: 'master', + namespace_id: project.namespace, + project_id: project, + start_branch: 'master', id: master_pickable_commit.id) end it 'redirects to the commit page' do # Cherry-picking a commit that has been already cherry-picked. post(:cherry_pick, - namespace_id: project.namespace.to_param, - project_id: project.to_param, - target_branch: 'master', + namespace_id: project.namespace, + project_id: project, + start_branch: 'master', id: master_pickable_commit.id) expect(response).to redirect_to namespace_project_commit_path(project.namespace, project, master_pickable_commit.id) diff --git a/spec/features/projects/commit/cherry_pick_spec.rb b/spec/features/projects/commit/cherry_pick_spec.rb index 7baf7913424..0b972d2a439 100644 --- a/spec/features/projects/commit/cherry_pick_spec.rb +++ b/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 diff --git a/spec/models/repository_spec.rb b/spec/models/repository_spec.rb index 838fd3754b2..8ed45808766 100644 --- a/spec/models/repository_spec.rb +++ b/spec/models/repository_spec.rb @@ -1113,16 +1113,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 @@ -1149,16 +1149,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 |