Commit de373fe8 authored by Kirill Smelkov's avatar Kirill Smelkov Committed by Alain Takoudjou

NXD Teach GitLab about patches

Teach GitLab not only to merge changes from a merge-request, but also to
apply patches posted to merge-request in a way like `git am` would do -
without merge commit and directly on top of current branch. Which way to
go is selected by user in web UI, and apply patches is the first option.

There are 3 cases:

- only 1 commit is present in MR -> the only available option is to
  apply that single commit as one patch without a merge

  ( There is no need for merge commit in this case at all: information
    about user who applied the patch goes to "Committer" field in resultant
    commit. Avoiding 1 merge per 1 patch results in cleaner history )

  It is also possible to review patch description directly in web UI,
  before doing the actual application, and correct / amend it as needed.

- several commits are present in MR:

  * it is possible to apply the patches directly on top of current
    branch. Again information about who applied what goes to "Committer"
    field.

  * it is possible to merge MR changes with making a merge commit.

    This variant is useful, when patches from a MR do several logical
    steps to reach one goal, and MR description contain cover letter for
    whole patch series.

    in this case original commits stay untouched and resulting merge
    will contain MR author as author, user who accepted MR as committer,
    and cover letter as merge commit message.

    NOTE we avoid useless "Merge branch X into Y" in merge message, and
        just put MR title into merge subject and MR description into merge
        description.

        This way it is more logical with more important information in
        merge subject and thus e.g. more handy to oversee what a merge brings,
        just by it subject, e.g. via looking at updates via

            gitk --first-parent ...

        or via web.

NOTE for pre-generated references to merge-request we now use full MR
    URL, instead of !<MR-n>. Full URLs work everywhere, not only on
    original site where MR was created, or even only in original repo
    and not its fork on the same site.

94ddf244
fixup! NXD Teach GitLab about patches

commit_with_hooks was replaced by GitOperationService: see https://gitlab.com/gitlab-org/gitlab-ce/commit/a52dc7cec70ef97b2755fb9cef7d6b489062310c

12a773b6
fixup! NXD Teach GitLab about patches: include merge description message

Include Merge description message in apply_patches and merge as topic by default. User can disable description in merge message by clicking on `Don't include description in commit message`.

/reviewed-on !4
parent be842143
...@@ -93,6 +93,12 @@ ...@@ -93,6 +93,12 @@
padding-top: 8px; padding-top: 8px;
line-height: 20px; line-height: 20px;
// text in-between buttons
&.inbetween-text {
margin-left: 0px; // disable `margin-left: 20px` in parent
margin-right: 7px; // compensate for margin-right in btn-grouped
}
&.right { &.right {
float: right; float: right;
padding-right: 0; padding-right: 0;
......
...@@ -275,7 +275,7 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo ...@@ -275,7 +275,7 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo
end end
def merge_params_attributes def merge_params_attributes
[:should_remove_source_branch, :commit_message] [:should_remove_source_branch, :commit_message, :apply_patches]
end end
def merge_when_pipeline_succeeds_active? def merge_when_pipeline_succeeds_active?
......
...@@ -678,7 +678,7 @@ class MergeRequest < ActiveRecord::Base ...@@ -678,7 +678,7 @@ class MergeRequest < ActiveRecord::Base
end end
message = [ message = [
"Merge branch '#{source_branch}' into '#{target_branch}'", #"Merge branch '#{source_branch}' into '#{target_branch}'",
title title
] ]
...@@ -687,7 +687,8 @@ class MergeRequest < ActiveRecord::Base ...@@ -687,7 +687,8 @@ class MergeRequest < ActiveRecord::Base
end end
message << "#{description}" if include_description && description.present? message << "#{description}" if include_description && description.present?
message << "See merge request #{to_reference}" #message << "See merge request #{to_reference}"
message << "/reviewed-on #{Gitlab::UrlBuilder.build(self)}"
message.join("\n\n") message.join("\n\n")
end end
......
...@@ -849,6 +849,51 @@ class Repository ...@@ -849,6 +849,51 @@ class Repository
false false
end end
# apply patches represented by [] of commits
def apply_patches(user, patches, target_branch, options = {})
base_commit = rugged.branches[target_branch].target
raise "Invalid apply target" if base_commit.nil?
res = nil
patches.each_with_index do |patch, i|
patch = rugged.lookup(patch.id) # Gitlab::Commit -> Rugged::Commit
args = [patch, base_commit]
args << { mainline: 1 } if patch.parents.size > 1
index = rugged.cherrypick_commit(*args)
raise "Conflict while applying #{patch.oid} to #{base_commit.oid}" if index.conflicts?
tree_id = index.write_tree(rugged)
# no check for empty diff: i.e. cherry-pick --allow-empty
committer = user_to_committer(user)
commit_options = {
message: patch.message,
author: patch.author,
committer: committer,
tree: tree_id,
parents: [base_commit],
}.merge(options)
# for all patches except last just create commit object without updating
# ref and invoking hooks
if i + 1 < patches.length then
base_commit = Rugged::Commit.create(rugged, commit_options)
# for last patch make sure to update ref and invoke hooks
else
res = GitOperationService.new(user, self).with_branch(target_branch) do |ref|
commit_options["update_ref"] = ref
Rugged::Commit.create(rugged, commit_options)
end
end
end
fail if res.nil? # assert
res
end
def revert( def revert(
user, commit, branch_name, user, commit, branch_name,
start_branch_name: nil, start_project: project) start_branch_name: nil, start_project: project)
...@@ -856,7 +901,6 @@ class Repository ...@@ -856,7 +901,6 @@ class Repository
branch_name, branch_name,
start_branch_name: start_branch_name, start_branch_name: start_branch_name,
start_project: start_project) do |start_commit| start_project: start_project) do |start_commit|
revert_tree_id = check_revert_content(commit, start_commit.sha) revert_tree_id = check_revert_content(commit, start_commit.sha)
unless revert_tree_id unless revert_tree_id
raise Repository::CreateTreeError.new('Failed to revert commit') raise Repository::CreateTreeError.new('Failed to revert commit')
......
...@@ -37,14 +37,30 @@ module MergeRequests ...@@ -37,14 +37,30 @@ module MergeRequests
def commit def commit
committer = repository.user_to_committer(current_user) committer = repository.user_to_committer(current_user)
# MR author with corrected time to be MR creation time.
# (NOTE rugged wants Time, not ActiveSupport::TimeWithZone)
mr_author = repository.user_to_committer(merge_request.author)
.merge({time: Time.parse(merge_request.created_at.to_s)})
mr_message= params[:commit_message] || merge_request.merge_commit_message
mr_1patch = merge_request.commits.size == 1
do_apply = params[:apply_patches] == "on" || mr_1patch
do_merge = !do_apply
# merge -> author=mr_author, message=mr_message
# apply -> author=patch author, message=mr_message if only 1 patch
options = { options = {
message: params[:commit_message] || merge_request.merge_commit_message, #message: params[:commit_message] || merge_request.merge_commit_message,
author: committer, #author: commiter,
committer: committer committer: committer
} }
options[:author] = mr_author if do_merge
options[:message] = mr_message if do_merge || (do_apply && merge_request.commits.size == 1)
commit_id = repository.merge(current_user, source, merge_request, options) commit_id = do_merge \
? repository.merge(current_user, source, merge_request, options)
: repository.apply_patches(current_user, merge_request.commits.reverse, merge_request.target_branch, options)
raise MergeError, 'Conflicts detected during merge' unless commit_id raise MergeError, 'Conflicts detected during merge' unless commit_id
......
...@@ -2,12 +2,12 @@ ...@@ -2,12 +2,12 @@
- nonce = SecureRandom.hex - nonce = SecureRandom.hex
- descriptions = local_assigns.slice(:message_with_description, :message_without_description) - descriptions = local_assigns.slice(:message_with_description, :message_without_description)
= label_tag "commit_message-#{nonce}", class: 'control-label' do = label_tag "commit_message-#{nonce}", class: 'control-label' do
#{ _('Commit message') } = local_assigns[:label_text] || #{ _('Commit message') }
.col-sm-10 .col-sm-10
.commit-message-container .commit-message-container
.max-width-marker .max-width-marker
= text_area_tag 'commit_message', = text_area_tag 'commit_message',
(params[:commit_message] || local_assigns[:text] || local_assigns[:placeholder]), (local_assigns[:message_with_description] || params[:commit_message] || local_assigns[:text] || local_assigns[:placeholder]),
class: 'form-control js-commit-message', placeholder: local_assigns[:placeholder], class: 'form-control js-commit-message', placeholder: local_assigns[:placeholder],
data: descriptions, data: descriptions,
required: true, rows: (local_assigns[:rows] || 3), required: true, rows: (local_assigns[:rows] || 3),
...@@ -17,9 +17,9 @@ ...@@ -17,9 +17,9 @@
Try to keep the first line under 52 characters Try to keep the first line under 52 characters
and the others under 72. and the others under 72.
- if descriptions.present? - if descriptions.present?
.hint.js-with-description-hint .hint.js-with-description-hint.hide
= link_to "#", class: "js-with-description-link" do = link_to "#", class: "js-with-description-link" do
Include description in commit message Include description in commit message
.hint.js-without-description-hint.hide .hint.js-without-description-hint
= link_to "#", class: "js-without-description-link" do = link_to "#", class: "js-without-description-link" do
Don't include description in commit message Don't include description in commit message
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