Commit f56366f1 authored by Robert Speicher's avatar Robert Speicher Committed by Felipe Artur

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

Fix cherry-picking or reverting through an MR

Closes #28711 and #28426

See merge request !9640
parent bc1e2cc2
......@@ -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)
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
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
......@@ -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,8 +61,8 @@ 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),
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
......
......@@ -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
......@@ -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,8 +1001,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?
......@@ -1012,8 +1014,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?
......
......@@ -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
- 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
......@@ -138,7 +138,7 @@ module API
commit_params = {
commit: commit,
create_merge_request: false,
start_branch: params[:branch],
target_branch: params[:branch]
}
......
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
......@@ -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)
......
......@@ -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
......
......@@ -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
......
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