Commit 6cc4cf1e authored by Douwe Maan's avatar Douwe Maan
Browse files

Fix cherry-picking or reverting through an MR

parent efff35e9
......@@ -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 @@ def create_commit(service, success_path:, failure_path:, failure_view: nil, succ
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 @@ def update_flash_notice(success_notice)
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,42 +99,26 @@ def different_project?
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
else
# Merge request from fork to this project
current_user.fork_of(@project)
end
if can?(current_user, :push_code, @project)
@mr_source_project = @project
@target_branch ||= @ref
else
@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
@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
......@@ -24,7 +24,7 @@ def new
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 @@ def edit
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 @@ def preview
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
......
......@@ -51,23 +51,35 @@ def branches
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 @@ def failed_change_path
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 @@ def define_commit_vars
@diffs = commit.diffs(opts)
@notes_count = commit.notes.count
@environment = EnvironmentsFinder.new(@project, current_user, commit: @commit).execute.last
end
......@@ -118,11 +130,7 @@ def define_note_vars
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
......@@ -862,17 +862,18 @@ def merge(user, source, merge_request, options = {})
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::CommitError.new('Failed to revert commit')
end
committer = user_to_committer(user)
Rugged::Commit.create(rugged,
......@@ -885,17 +886,18 @@ def revert(
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::CommitError.new('Failed to cherry-pick commit')
end
committer = user_to_committer(user)
Rugged::Commit.create(rugged,
......@@ -919,9 +921,8 @@ def resolve_conflicts(user, branch_name, params)
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)
......@@ -933,9 +934,8 @@ def check_revert_content(target_commit, branch_name)
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)
......
......@@ -8,9 +8,9 @@ def execute
@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,33 +26,24 @@ def commit
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
success
rescue Repository::CommitError => e
if e.message =~ /Failed to/
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
else
raise
end
end
......@@ -66,16 +57,17 @@ def check_push_permissions
true
end
def validate_target_branch(new_branch)
# Temporary branch exists and contains the change commit
return if repository.find_branch(new_branch)
result = ValidateNewBranchService.new(@project, current_user)
.execute(new_branch)
def validate_target_branch
result = ValidateNewBranchService.new(@project, current_user).
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
- 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
......
---
title: Fix cherry-picking or reverting through an MR
merge_request:
author:
......@@ -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
......@@ -127,7 +127,7 @@ class Commits < Grape::API
commit_params = {
commit: commit,
create_merge_request: false,
start_branch: params[:branch],
target_branch: params[:branch]
}
......
......@@ -130,9 +130,7 @@ class Commits < Grape::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]
}
......
......@@ -166,7 +166,7 @@ def go(extra_params = {})
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 @@ def go(extra_params = {})
post(:revert,
namespace_id: project.namespace,
project_id: project,
target_branch: 'master',
start_branch: 'master',
id: commit.id)
end
......@@ -188,7 +188,7 @@ def go(extra_params = {})
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 @@ def go(extra_params = {})
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 @@ def go(extra_params = {})
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 @@ def go(extra_params = {})
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)
......
......@@ -60,6 +60,7 @@
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
......
......@@ -1112,16 +1112,16 @@
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 @@
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
......
Markdown is supported
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment