Commit 827d3914 authored by Kirill Smelkov's avatar Kirill Smelkov

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.
parent 16c4f14b
...@@ -44,6 +44,12 @@ ...@@ -44,6 +44,12 @@
padding-top: 12px; padding-top: 12px;
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;
a { a {
......
...@@ -323,7 +323,7 @@ class Projects::MergeRequestsController < Projects::ApplicationController ...@@ -323,7 +323,7 @@ class Projects::MergeRequestsController < Projects::ApplicationController
end end
def merge_params def merge_params
params.permit(:should_remove_source_branch, :commit_message) params.permit(:should_remove_source_branch, :commit_message, :apply_patches)
end end
# Make sure merge requests created before 8.0 # Make sure merge requests created before 8.0
......
...@@ -415,13 +415,14 @@ class MergeRequest < ActiveRecord::Base ...@@ -415,13 +415,14 @@ class MergeRequest < ActiveRecord::Base
end end
def merge_commit_message def merge_commit_message
message = "Merge branch '#{source_branch}' into '#{target_branch}'" #message = "Merge branch '#{source_branch}' into '#{target_branch}'"
message << "\n\n" #message << "\n\n"
message = ""
message << title.to_s message << title.to_s
  • @lab.nexedi.com shouldn't we include the word "Merge" in the subject ? When looking at history ( https://lab.nexedi.com/nexedi/erp5/commits/master ) it is not obvious that the commit comes from a merge.

    for example we could do

    message = "[Merge] " << title.to_s

    or

    message = "[Merge !#{iid}] " << title.to_s
  • @jerome I'm not agains it in principle, but before we do let us please better understand it. I mean what is the use-case for this?

    And also: a merge commit can be automatically detected just by looking at commit object - merge, in contrast to usual commit - has more than one parents. E.g. this commit

    erp5@505d155f

    has 2 parents.

    If we need, we can teach gitlab web UI to show "Merge" automatically somewhere when it sees commit with >1 parent, and it will work universally everywhere irregardless of the procedure used to apply patches / do the merge.

    Btw, when showing a merge commit, we can automatically show links to commits brought by that merge, etc.

    What do you think?

  • The use-case is when reading a list of commits subjects (git log --oneline for example), with gitlab default message Merge branch 'XX' into 'master' we could immediately see that this commmit was the result of a merge / merge request and not an usual commit. With this new message nothing in the subject indicates that this is a merge request and we have to look at the commit it self to find out.

  • Update: after discussion with @jerome it was agreed the way to go is to automatically mark merge commits at show time in web ui, not at commit generation time.

    (https://lab.nexedi.com/lab.nexedi.com/lab.nexedi.com/issues/11 for instance)

    Edited by Kirill Smelkov
Please register or sign in to reply
message << "\n\n" message << "\n\n"
message << description.to_s message << description.to_s
message << "\n\n" message << "\n\n" if !description.to_s.empty?
message << "See merge request !#{iid}" message << "/reviewed-on #{Gitlab::UrlBuilder.new(:merge_request).build(id)}"
message message
end end
......
...@@ -623,6 +623,51 @@ class Repository ...@@ -623,6 +623,51 @@ class Repository
end end
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 = commit_with_hooks(user, 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(user, commit, base_branch, target_branch = nil) def revert(user, commit, base_branch, target_branch = nil)
source_sha = find_branch(base_branch).target source_sha = find_branch(base_branch).target
target_branch ||= base_branch target_branch ||= base_branch
......
...@@ -27,14 +27,31 @@ module MergeRequests ...@@ -27,14 +27,31 @@ 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 = do_merge \
? repository.merge(current_user, merge_request.source_sha, merge_request.target_branch, options)
: repository.apply_patches(current_user, merge_request.commits.reverse, merge_request.target_branch, options)
commit_id = repository.merge(current_user, merge_request.source_sha, merge_request.target_branch, options)
merge_request.update(merge_commit_sha: commit_id) merge_request.update(merge_commit_sha: commit_id)
rescue StandardError => e rescue StandardError => e
merge_request.update(merge_error: "Something went wrong during merge") merge_request.update(merge_error: "Something went wrong during merge")
......
...@@ -23,8 +23,18 @@ ...@@ -23,8 +23,18 @@
= icon('warning fw') = icon('warning fw')
Merge Immediately Merge Immediately
- else - else
= f.button class: "btn btn-create btn-grouped js-merge-button accept_merge_request #{status_class}" do = f.button class: "btn btn-create btn-grouped js-apply-button accept_merge_request accept_merge_request_apply #{status_class}" do
Accept Merge Request - if @merge_request.commits.size == 1
Apply Patch
- else
Apply Patches
- if @merge_request.commits.size > 1
// do we really need accept-control.inbetween-text? make it simpler?
.accept-control.inbetween-text
or
= f.button class: "btn btn-create btn-grouped js-merge-button accept_merge_request_merge #{status_class}" do
Merge as Topic
- if @merge_request.can_remove_source_branch?(current_user) - if @merge_request.can_remove_source_branch?(current_user)
.accept-control.checkbox .accept-control.checkbox
= label_tag :should_remove_source_branch, class: "remove_source_checkbox" do = label_tag :should_remove_source_branch, class: "remove_source_checkbox" do
...@@ -33,20 +43,28 @@ ...@@ -33,20 +43,28 @@
.accept-control.right .accept-control.right
= link_to "#", class: "modify-merge-commit-link js-toggle-button" do = link_to "#", class: "modify-merge-commit-link js-toggle-button" do
= icon('edit') = icon('edit')
Modify commit message Modify #{@merge_request.commits.size > 1 ? "merge" : "commit"} message
.js-toggle-content.hide.prepend-top-default .js-toggle-content.hide.prepend-top-default
= render 'shared/commit_message_container', params: params, = render 'shared/commit_message_container', params: params,
label_text: @merge_request.commits.size > 1 ? "Merge message" : "Commit message",
text: @merge_request.merge_commit_message, text: @merge_request.merge_commit_message,
rows: 14, hint: true rows: 14, hint: true
= hidden_field_tag :merge_when_build_succeeds, "", autocomplete: "off" = hidden_field_tag :merge_when_build_succeeds, "", autocomplete: "off"
= hidden_field_tag :apply_patches, ""
:javascript :javascript
$('.accept-mr-form').on('ajax:send', function() { $('.accept-mr-form').on('ajax:send', function() {
$(".accept-mr-form :input").disable(); $(".accept-mr-form :input").disable();
}); });
$('.accept_merge_request').on('click', function() { $('.accept_merge_request_apply').on('click', function() {
$("#apply_patches").val("on");
$('.js-apply-button').html("<i class='fa fa-spinner fa-spin'></i> Apply in progress");
});
$('.accept_merge_request_merge').on('click', function() {
$("#apply_patches").val("off");
$('.js-merge-button').html("<i class='fa fa-spinner fa-spin'></i> Merge in progress"); $('.js-merge-button').html("<i class='fa fa-spinner fa-spin'></i> Merge in progress");
}); });
......
.form-group.commit_message-group .form-group.commit_message-group
- nonce = SecureRandom.hex - nonce = SecureRandom.hex
= 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
......
  • mentioned in commit slapos@138cfdb7

    Toggle commit list
  • In my opinion, this change is too big. I prefer either 1) make efforts to be accepted in upstream or 2) accept existing gitlab behaviour as it is. Maintaining such a big change is not so easy. (Do we have automated CI test for our patched branch ?)

  • This won't be accepted by upstream gitlab CE because it overlaps with GitLab EE (Enterprise Edition, proprietary licence) functionality (accept an MR with rebase).

    We just have to carry it with us, since several people in Nexedi really cares about having good history. In my view this is not a big patch. (we do not have automated CI for our gitlab-ce branch, until now I was doing all the testing manually).

    /cc @jm, @romain, @jerome

  • mentioned in merge request wendelin.core!14 (closed)

    Toggle commit list
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