Merge branch 'feature/merge_commit_message' of /home/git/repositories/gitlab/gitlabhq

Dmitriy Zaporozhets
2 parents 9a1d0c17 99a2b31e
Showing 9 changed files with 102 additions and 66 deletions Show diff stats
app/assets/javascripts/merge_requests.js.coffee
app/controllers/projects/merge_requests_controller.rb
app/models/merge_request.rb
app/views/projects/merge_requests/show/_mr_accept.html.haml
features/project/merge_requests.feature
features/steps/project/project_merge_requests.rb
lib/gitlab/satellite/merge_action.rb
spec/controllers/merge_requests_controller_spec.rb
spec/lib/gitlab/satellite/merge_action_spec.rb
@@ -24,6 +24,8 @@ class MergeRequest
     modal = $('#modal_merge_info').modal(show: false)
+    disableButtonIfEmptyField '#merge_commit_message', '.accept_merge_request'
+
   # Local jQuery finder
   $: (selector) ->
     this.$el.find(selector)
@@ -125,7 +125,7 @@ class Projects::MergeRequestsController &lt; Projects::ApplicationController
     if @merge_request.opened? && @merge_request.can_be_merged?
       @merge_request.should_remove_source_branch = params[:should_remove_source_branch]
-      @merge_request.automerge!(current_user)
+      @merge_request.automerge!(current_user, params[:merge_commit_message])
       @status = true
     else
       @status = false
@@ -214,8 +214,8 @@ class MergeRequest &lt; ActiveRecord::Base
     self.merge
   end
-  def automerge!(current_user)
-    if Gitlab::Satellite::MergeAction.new(current_user, self).merge! && self.unmerged_commits.empty?
+  def automerge!(current_user, commit_message = nil)
+    if Gitlab::Satellite::MergeAction.new(current_user, self).merge!(commit_message) && self.unmerged_commits.empty?
       self.merge!(current_user.id)
       true
     end
@@ -319,6 +319,15 @@ class MergeRequest &lt; ActiveRecord::Base
         update_all(updated_at: Time.now)
   end
+  def merge_commit_message
+    message = "Merge branch '#{source_branch}' into '#{target_branch}'"
+    message << "\n\n"
+    message << title.to_s
+    message << "\n\n"
+    message << description.to_s
+    message
+  end
+
   private
   def dump_commits(commits)
@@ -13,7 +13,21 @@
           If you still want to do it manually -
           %strong
             = link_to "click here", "#modal_merge_info", class: "how_to_merge_link vlink", title: "How To Merge", "data-toggle" => "modal"
-          for instructions
+          for instructions.
+
+          %br
+          If you want to modify merge commit message -
+          %strong
+            = link_to "click here", "#", class: "modify-merge-commit-link js-toggle-visibility-link", title: "Modify merge commit message"
+
+        .js-toggle-visibility-container.hide
+          .form-group
+            = label_tag :merge_commit_message, "Commit message", class: 'control-label'
+            .col-sm-10
+              = text_area_tag :merge_commit_message, @merge_request.merge_commit_message, class: "form-control js-gfm-input", rows: 14, required: true
+              %p.hint
+                The recommended maximum line length is 52 characters for the first line and 72 characters for all following lines.
+
         .accept-group
           .pull-left
             = f.submit "Accept Merge Request", class: "btn btn-create accept_merge_request"
@@ -67,3 +67,13 @@ Feature: Project Merge Requests
     And I leave a comment on the diff page
     And I switch to the merge request's comments tab
     Then I should see a discussion has started on commit bcf03b5de6c
+
+  @javascript
+  Scenario: I accept merge request with custom commit message
+    Given project "Shop" have "Bug NS-05" open merge request with diffs inside
+    And merge request "Bug NS-05" is mergeable
+    And I visit merge request page "Bug NS-05"
+    And merge request is mergeable
+    Then I modify merge commit message
+    And I accept this merge request
+    Then I should see merged request
@@ -4,60 +4,60 @@ class ProjectMergeRequests &lt; Spinach::FeatureSteps
   include SharedNote
   include SharedPaths
-  Given 'I click link "New Merge Request"' do
+  step 'I click link "New Merge Request"' do
     click_link "New Merge Request"
   end
-  Given 'I click link "Bug NS-04"' do
+  step 'I click link "Bug NS-04"' do
     click_link "Bug NS-04"
   end
-  Given 'I click link "All"' do
+  step 'I click link "All"' do
     click_link "All"
   end
-  Given 'I click link "Closed"' do
+  step 'I click link "Closed"' do
     click_link "Closed"
   end
-  Then 'I should see merge request "Wiki Feature"' do
+  step 'I should see merge request "Wiki Feature"' do
     within '.merge-request' do
       page.should have_content "Wiki Feature"
     end
   end
-  Then 'I should see closed merge request "Bug NS-04"' do
+  step 'I should see closed merge request "Bug NS-04"' do
     merge_request = MergeRequest.find_by_title!("Bug NS-04")
     merge_request.closed?.should be_true
     page.should have_content "Closed by"
   end
-  Then 'I should see merge request "Bug NS-04"' do
+  step 'I should see merge request "Bug NS-04"' do
     page.should have_content "Bug NS-04"
   end
-  Then 'I should see "Bug NS-04" in merge requests' do
+  step 'I should see "Bug NS-04" in merge requests' do
     page.should have_content "Bug NS-04"
   end
-  Then 'I should see "Feature NS-03" in merge requests' do
+  step 'I should see "Feature NS-03" in merge requests' do
     page.should have_content "Feature NS-03"
   end
-  And 'I should not see "Feature NS-03" in merge requests' do
+  step 'I should not see "Feature NS-03" in merge requests' do
     page.should_not have_content "Feature NS-03"
   end
-  And 'I should not see "Bug NS-04" in merge requests' do
+  step 'I should not see "Bug NS-04" in merge requests' do
     page.should_not have_content "Bug NS-04"
   end
-  And 'I click link "Close"' do
+  step 'I click link "Close"' do
     click_link "Close"
   end
-  And 'I submit new merge request "Wiki Feature"' do
+  step 'I submit new merge request "Wiki Feature"' do
     fill_in "merge_request_title", with: "Wiki Feature"
     # this must come first, so that the target branch is set
@@ -76,7 +76,7 @@ class ProjectMergeRequests &lt; Spinach::FeatureSteps
     click_button "Submit merge request"
   end
-  And 'project "Shop" have "Bug NS-04" open merge request' do
+  step 'project "Shop" have "Bug NS-04" open merge request' do
     create(:merge_request,
            title: "Bug NS-04",
            source_project: project,
@@ -84,7 +84,7 @@ class ProjectMergeRequests &lt; Spinach::FeatureSteps
            author: project.users.first)
   end
-  And 'project "Shop" have "Bug NS-05" open merge request with diffs inside' do
+  step 'project "Shop" have "Bug NS-05" open merge request with diffs inside' do
     create(:merge_request_with_diffs,
            title: "Bug NS-05",
            source_project: project,
@@ -92,7 +92,7 @@ class ProjectMergeRequests &lt; Spinach::FeatureSteps
            author: project.users.first)
   end
-  And 'project "Shop" have "Feature NS-03" closed merge request' do
+  step 'project "Shop" have "Feature NS-03" closed merge request' do
     create(:closed_merge_request,
            title: "Feature NS-03",
            source_project: project,
@@ -100,19 +100,19 @@ class ProjectMergeRequests &lt; Spinach::FeatureSteps
            author: project.users.first)
   end
-  And 'I switch to the diff tab' do
+  step 'I switch to the diff tab' do
     visit diffs_project_merge_request_path(project, merge_request)
   end
-  And 'I switch to the merge requests comments tab' do
+  step 'I switch to the merge requests comments tab' do
     visit project_merge_request_path(project, merge_request)
   end
-  And 'I click on the first commit in the merge request' do
+  step 'I click on the first commit in the merge request' do
     click_link merge_request.commits.first.short_id(8)
   end
-  And 'I leave a comment on the diff page' do
+  step 'I leave a comment on the diff page' do
     init_diff_note
     within('.js-discussion-note-form') do
@@ -125,7 +125,7 @@ class ProjectMergeRequests &lt; Spinach::FeatureSteps
     end
   end
-  And 'I leave a comment like "Line is wrong" on line 185 of the first file' do
+  step 'I leave a comment like "Line is wrong" on line 185 of the first file' do
     init_diff_note
     within(".js-discussion-note-form") do
@@ -138,24 +138,47 @@ class ProjectMergeRequests &lt; Spinach::FeatureSteps
     end
   end
-  Then 'I should see a discussion has started on line 185' do
+  step 'I should see a discussion has started on line 185' do
     page.should have_content "#{current_user.name} started a discussion on this merge request diff"
     page.should have_content "app/assets/stylesheets/tree.scss:L185"
     page.should have_content "Line is wrong"
   end
-  Then 'I should see a discussion has started on commit bcf03b5de6c:L185' do
+  step 'I should see a discussion has started on commit bcf03b5de6c:L185' do
     page.should have_content "#{current_user.name} started a discussion on commit"
     page.should have_content "app/assets/stylesheets/tree.scss:L185"
     page.should have_content "Line is wrong"
   end
-  Then 'I should see a discussion has started on commit bcf03b5de6c' do
+  step 'I should see a discussion has started on commit bcf03b5de6c' do
     page.should have_content "#{current_user.name} started a discussion on commit bcf03b5de6c"
     page.should have_content "One comment to rule them all"
     page.should have_content "app/assets/stylesheets/tree.scss:L185"
   end
+  step 'merge request is mergeable' do
+    page.should have_content 'You can accept this request automatically'
+  end
+
+  step 'I modify merge commit message' do
+    find('.modify-merge-commit-link').click
+    fill_in 'merge_commit_message', with: "wow such merge"
+  end
+
+  step 'merge request "Bug NS-05" is mergeable' do
+    merge_request.mark_as_mergeable
+  end
+
+  step 'I accept this merge request' do
+    click_button "Accept Merge Request"
+  end
+
+  step 'I should see merged request' do
+    within '.page-title' do
+      page.should have_content "Merged"
+    end
+  end
+
   def project
     @project ||= Project.find_by_name!("Shop")
   end
@@ -24,10 +24,10 @@ module Gitlab
       # Returns false if the merge produced conflicts
       # Returns false if pushing from the satellite to the repository failed or was rejected
       # Returns true otherwise
-      def merge!
+      def merge!(merge_commit_message = nil)
         in_locked_and_timed_satellite do |merge_repo|
           prepare_satellite!(merge_repo)
-          if merge_in_satellite!(merge_repo)
+          if merge_in_satellite!(merge_repo, merge_commit_message)
             # push merge back to bare repo
             # will raise CommandFailed when push fails
             merge_repo.git.push(default_options, :origin, merge_request.target_branch)
@@ -49,14 +49,7 @@ module Gitlab
         in_locked_and_timed_satellite do |merge_repo|
           prepare_satellite!(merge_repo)
           update_satellite_source_and_target!(merge_repo)
-
-          if merge_request.for_fork?
-            diff = merge_repo.git.native(:diff, default_options, "origin/#{merge_request.target_branch}", "source/#{merge_request.source_branch}")
-          else
-            diff = merge_repo.git.native(:diff, default_options, "#{merge_request.target_branch}", "#{merge_request.source_branch}")
-          end
-
-          return diff
+          diff = merge_repo.git.native(:diff, default_options, "origin/#{merge_request.target_branch}", "source/#{merge_request.source_branch}")
         end
       rescue Grit::Git::CommandFailed => ex
         handle_exception(ex)
@@ -88,14 +81,7 @@ module Gitlab
         in_locked_and_timed_satellite do |merge_repo|
           prepare_satellite!(merge_repo)
           update_satellite_source_and_target!(merge_repo)
-
-          if (merge_request.for_fork?)
-            patch = merge_repo.git.format_patch(default_options({stdout: true}), "origin/#{merge_request.target_branch}..source/#{merge_request.source_branch}")
-          else
-            patch = merge_repo.git.format_patch(default_options({stdout: true}), "#{merge_request.target_branch}..#{merge_request.source_branch}")
-          end
-
-          return patch
+          patch = merge_repo.git.format_patch(default_options({stdout: true}), "origin/#{merge_request.target_branch}..source/#{merge_request.source_branch}")
         end
       rescue Grit::Git::CommandFailed => ex
         handle_exception(ex)
@@ -125,34 +111,26 @@ module Gitlab
       #
       # Returns false if the merge produced conflicts
       # Returns true otherwise
-      def merge_in_satellite!(repo)
+      def merge_in_satellite!(repo, message = nil)
         update_satellite_source_and_target!(repo)
+        message ||= "Merge branch '#{merge_request.source_branch}' into '#{merge_request.target_branch}'"
+
         # merge the source branch into the satellite
         # will raise CommandFailed when merge fails
-        if merge_request.for_fork?
-          repo.git.pull(default_options({no_ff: true}), 'source', merge_request.source_branch)
-        else
-          repo.git.pull(default_options({no_ff: true}), 'origin', merge_request.source_branch)
-        end
+        repo.git.merge(default_options({no_ff: true}), "-m #{message}", "source/#{merge_request.source_branch}")
       rescue Grit::Git::CommandFailed => ex
         handle_exception(ex)
       end
       # Assumes a satellite exists that is a fresh clone of the projects repo, prepares satellite for merges, diffs etc
       def update_satellite_source_and_target!(repo)
-        if merge_request.for_fork?
-          repo.remote_add('source', merge_request.source_project.repository.path_to_repo)
-          repo.remote_fetch('source')
-          repo.git.checkout(default_options({b: true}), merge_request.target_branch, "origin/#{merge_request.target_branch}")
-        else
-          repo.git.checkout(default_options, "#{merge_request.source_branch}")
-          repo.git.checkout(default_options({t: true}), "origin/#{merge_request.target_branch}")
-        end
+        repo.remote_add('source', merge_request.source_project.repository.path_to_repo)
+        repo.remote_fetch('source')
+        repo.git.checkout(default_options({b: true}), merge_request.target_branch, "origin/#{merge_request.target_branch}")
       rescue Grit::Git::CommandFailed => ex
         handle_exception(ex)
       end
-
     end
   end
 end
@@ -3,7 +3,7 @@ require &#39;spec_helper&#39;
 describe Projects::MergeRequestsController do
   let(:project) { create(:project_with_code) }
   let(:user)    { create(:user) }
-  let(:merge_request) { create(:merge_request_with_diffs, target_project: project, source_project: project, target_branch: "bcf03b5d~3", source_branch: "bcf03b5d") }
+  let(:merge_request) { create(:merge_request_with_diffs, target_project: project, source_project: project, target_branch: "stable", source_branch: "master") }
   before do
     sign_in(user)
@@ -61,7 +61,7 @@ describe Projects::MergeRequestsController do
       it "should really be a git email patch with commit" do
         get :show, project_id: project.to_param, id: merge_request.iid, format: format
-        expect(response.body[0..100]).to start_with("From #{merge_request.commits.last.id}")
+        expect(response.body[0..100]).to start_with("From 6ea87c47f0f8a24ae031c3fff17bc913889ecd00")
       end
       it "should contain git diffs" do
@@ -2,13 +2,12 @@ require &#39;spec_helper&#39;
 describe 'Gitlab::Satellite::MergeAction' do
   before(:each) do
-#    TestEnv.init(mailer: false, init_repos: true, repos: true)
     @master = ['master', 'b1e6a9dbf1c85e6616497a5e7bad9143a4bd0828']
     @one_after_stable = ['stable', '6ea87c47f0f8a24ae031c3fff17bc913889ecd00'] #this commit sha is one after stable
     @wiki_branch = ['wiki', '635d3e09b72232b6e92a38de6cc184147e5bcb41'] #this is the commit sha where the wiki branch goes off from master
     @conflicting_metior = ['metior', '313d96e42b313a0af5ab50fa233bf43e27118b3f'] #this branch conflicts with the wiki branch
-    #these commits are quite close together, itended to make string diffs/format patches small
+    # these commits are quite close together, itended to make string diffs/format patches small
     @close_commit1 = ['2_3_notes_fix', '8470d70da67355c9c009e4401746b1d5410af2e3']
     @close_commit2 = ['scss_refactoring', 'f0f14c8eaba69ebddd766498a9d0b0e79becd633']
   end
@@ -16,6 +15,7 @@ describe &#39;Gitlab::Satellite::MergeAction&#39; do
   let(:project) { create(:project_with_code) }
   let(:merge_request) { create(:merge_request, source_project: project, target_project: project) }
   let(:merge_request_fork) { create(:merge_request) }
+
   describe '#commits_between' do
     def verify_commits(commits, first_commit_sha, last_commit_sha)
       commits.each { |commit| commit.class.should == Gitlab::Git::Commit }
@@ -145,4 +145,4 @@ describe &#39;Gitlab::Satellite::MergeAction&#39; do
       end
     end
   end
-end
 \ No newline at end of file
+end
	@@ -24,6 +24,8 @@ class MergeRequest		@@ -24,6 +24,8 @@ class MergeRequest
24		24
25	modal = $('#modal_merge_info').modal(show: false)	25	modal = $('#modal_merge_info').modal(show: false)
26		26
		27	+ disableButtonIfEmptyField '#merge_commit_message', '.accept_merge_request'
		28	+
27	# Local jQuery finder	29	# Local jQuery finder
28	$: (selector) ->	30	$: (selector) ->
29	this.$el.find(selector)	31	this.$el.find(selector)
	@@ -125,7 +125,7 @@ class Projects::MergeRequestsController < Projects::ApplicationController		@@ -125,7 +125,7 @@ class Projects::MergeRequestsController < Projects::ApplicationController
125		125
126	if @merge_request.opened? && @merge_request.can_be_merged?	126	if @merge_request.opened? && @merge_request.can_be_merged?
127	@merge_request.should_remove_source_branch = params[:should_remove_source_branch]	127	@merge_request.should_remove_source_branch = params[:should_remove_source_branch]
128	- @merge_request.automerge!(current_user)	128	+ @merge_request.automerge!(current_user, params[:merge_commit_message])
129	@status = true	129	@status = true
130	else	130	else
131	@status = false	131	@status = false
	@@ -13,7 +13,21 @@		@@ -13,7 +13,21 @@
13	If you still want to do it manually -	13	If you still want to do it manually -
14	%strong	14	%strong
15	= link_to "click here", "#modal_merge_info", class: "how_to_merge_link vlink", title: "How To Merge", "data-toggle" => "modal"	15	= link_to "click here", "#modal_merge_info", class: "how_to_merge_link vlink", title: "How To Merge", "data-toggle" => "modal"
16	- for instructions	16	+ for instructions.
		17	+
		18	+ %br
		19	+ If you want to modify merge commit message -
		20	+ %strong
		21	+ = link_to "click here", "#", class: "modify-merge-commit-link js-toggle-visibility-link", title: "Modify merge commit message"
		22	+
		23	+ .js-toggle-visibility-container.hide
		24	+ .form-group
		25	+ = label_tag :merge_commit_message, "Commit message", class: 'control-label'
		26	+ .col-sm-10
		27	+ = text_area_tag :merge_commit_message, @merge_request.merge_commit_message, class: "form-control js-gfm-input", rows: 14, required: true
		28	+ %p.hint
		29	+ The recommended maximum line length is 52 characters for the first line and 72 characters for all following lines.
		30	+
17	.accept-group	31	.accept-group
18	.pull-left	32	.pull-left
19	= f.submit "Accept Merge Request", class: "btn btn-create accept_merge_request"	33	= f.submit "Accept Merge Request", class: "btn btn-create accept_merge_request"
	@@ -67,3 +67,13 @@ Feature: Project Merge Requests		@@ -67,3 +67,13 @@ Feature: Project Merge Requests
67	And I leave a comment on the diff page	67	And I leave a comment on the diff page
68	And I switch to the merge request's comments tab	68	And I switch to the merge request's comments tab
69	Then I should see a discussion has started on commit bcf03b5de6c	69	Then I should see a discussion has started on commit bcf03b5de6c
		70	+
		71	+ @javascript
		72	+ Scenario: I accept merge request with custom commit message
		73	+ Given project "Shop" have "Bug NS-05" open merge request with diffs inside
		74	+ And merge request "Bug NS-05" is mergeable
		75	+ And I visit merge request page "Bug NS-05"
		76	+ And merge request is mergeable
		77	+ Then I modify merge commit message
		78	+ And I accept this merge request
		79	+ Then I should see merged request
	@@ -3,7 +3,7 @@ require 'spec_helper'		@@ -3,7 +3,7 @@ require 'spec_helper'
3	describe Projects::MergeRequestsController do	3	describe Projects::MergeRequestsController do
4	let(:project) { create(:project_with_code) }	4	let(:project) { create(:project_with_code) }
5	let(:user) { create(:user) }	5	let(:user) { create(:user) }
6	- let(:merge_request) { create(:merge_request_with_diffs, target_project: project, source_project: project, target_branch: "bcf03b5d~3", source_branch: "bcf03b5d") }	6	+ let(:merge_request) { create(:merge_request_with_diffs, target_project: project, source_project: project, target_branch: "stable", source_branch: "master") }
7		7
8	before do	8	before do
9	sign_in(user)	9	sign_in(user)
	@@ -61,7 +61,7 @@ describe Projects::MergeRequestsController do		@@ -61,7 +61,7 @@ describe Projects::MergeRequestsController do
61	it "should really be a git email patch with commit" do	61	it "should really be a git email patch with commit" do
62	get :show, project_id: project.to_param, id: merge_request.iid, format: format	62	get :show, project_id: project.to_param, id: merge_request.iid, format: format
63		63
64	- expect(response.body[0..100]).to start_with("From #{merge_request.commits.last.id}")	64	+ expect(response.body[0..100]).to start_with("From 6ea87c47f0f8a24ae031c3fff17bc913889ecd00")
65	end	65	end
66		66
67	it "should contain git diffs" do	67	it "should contain git diffs" do