From 7c6287c878c3106492c1f96d7624e99aa6c8b398 Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Mon, 13 Jan 2014 13:17:38 +0200 Subject: [PATCH 1/9] Add support of custom message to merge action of satellite Signed-off-by: Dmitriy Zaporozhets --- lib/gitlab/satellite/merge_action.rb | 27 ++++++++++++--------------- 1 file changed, 12 insertions(+), 15 deletions(-) diff --git a/lib/gitlab/satellite/merge_action.rb b/lib/gitlab/satellite/merge_action.rb index 54afd6ab95c..be81136ac6e 100644 --- a/lib/gitlab/satellite/merge_action.rb +++ b/lib/gitlab/satellite/merge_action.rb @@ -125,34 +125,31 @@ 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) + merge_message = "Merge branch '#{merge_request.source_branch}' into '#{merge_request.target_branch}'" + + if message + merge_message << "\n\n" + merge_message << message + end + # 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 #{merge_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 From 6a80083294bdd09f6e6d1d97a9512c434dc4e73d Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Mon, 13 Jan 2014 16:16:45 +0200 Subject: [PATCH 2/9] Use same code for diff and patch inside MergeAction Signed-off-by: Dmitriy Zaporozhets --- lib/gitlab/satellite/merge_action.rb | 18 ++---------------- spec/lib/gitlab/satellite/merge_action_spec.rb | 6 +++--- 2 files changed, 5 insertions(+), 19 deletions(-) diff --git a/lib/gitlab/satellite/merge_action.rb b/lib/gitlab/satellite/merge_action.rb index be81136ac6e..4d8fd374a19 100644 --- a/lib/gitlab/satellite/merge_action.rb +++ b/lib/gitlab/satellite/merge_action.rb @@ -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) diff --git a/spec/lib/gitlab/satellite/merge_action_spec.rb b/spec/lib/gitlab/satellite/merge_action_spec.rb index e40ff73b7f0..d2f026f96e8 100644 --- a/spec/lib/gitlab/satellite/merge_action_spec.rb +++ b/spec/lib/gitlab/satellite/merge_action_spec.rb @@ -2,13 +2,12 @@ require 'spec_helper' 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 'Gitlab::Satellite::MergeAction' 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 'Gitlab::Satellite::MergeAction' do end end end -end \ No newline at end of file +end From d84b0fa6b5c84774e300e5fe6415defb30c441fe Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Mon, 13 Jan 2014 16:49:35 +0200 Subject: [PATCH 3/9] Add merge_commit_message argument for Satellite::MergeAction#merge --- lib/gitlab/satellite/merge_action.rb | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-) diff --git a/lib/gitlab/satellite/merge_action.rb b/lib/gitlab/satellite/merge_action.rb index 4d8fd374a19..85615f282c4 100644 --- a/lib/gitlab/satellite/merge_action.rb +++ b/lib/gitlab/satellite/merge_action.rb @@ -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) @@ -114,16 +114,11 @@ module Gitlab def merge_in_satellite!(repo, message = nil) update_satellite_source_and_target!(repo) - merge_message = "Merge branch '#{merge_request.source_branch}' into '#{merge_request.target_branch}'" - - if message - merge_message << "\n\n" - merge_message << message - end + 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 - repo.git.merge(default_options({no_ff: true}), "-m #{merge_message}", "source/#{merge_request.source_branch}") + repo.git.merge(default_options({no_ff: true}), "-m #{message}", "source/#{merge_request.source_branch}") rescue Grit::Git::CommandFailed => ex handle_exception(ex) end From db966364de38d896961abba74c20a69bd3d6361c Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Mon, 13 Jan 2014 17:20:30 +0200 Subject: [PATCH 4/9] MergeRequest#automerge accepts custom message now. MergeRequest#merge_commit_message generate default one Signed-off-by: Dmitriy Zaporozhets --- app/models/merge_request.rb | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index e59aee8b445..39165631894 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -214,8 +214,8 @@ class MergeRequest < 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, merge_commit_message = nil) + if Gitlab::Satellite::MergeAction.new(current_user, self).merge!(merge_commit_message) && self.unmerged_commits.empty? self.merge!(current_user.id) true end @@ -319,6 +319,14 @@ class MergeRequest < 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 + message << "\n\n" + message << description + end + private def dump_commits(commits) From 8781459124b87912b105bab3c2b67c46f5457db9 Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Mon, 13 Jan 2014 17:21:59 +0200 Subject: [PATCH 5/9] Use params[:merge_commit_message] in MergeRequestController for automerge Signed-off-by: Dmitriy Zaporozhets --- app/controllers/projects/merge_requests_controller.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb index bba08bdd782..39d4f77b73c 100644 --- a/app/controllers/projects/merge_requests_controller.rb +++ b/app/controllers/projects/merge_requests_controller.rb @@ -123,7 +123,7 @@ class Projects::MergeRequestsController < 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 From 0de6e2da38058e4628d20cfb06433e873609a70f Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Mon, 13 Jan 2014 17:54:02 +0200 Subject: [PATCH 6/9] Add UI for custom Merge request commit message Signed-off-by: Dmitriy Zaporozhets --- .../merge_requests/show/_mr_accept.html.haml | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/app/views/projects/merge_requests/show/_mr_accept.html.haml b/app/views/projects/merge_requests/show/_mr_accept.html.haml index d210d4667fd..342f2707369 100644 --- a/app/views/projects/merge_requests/show/_mr_accept.html.haml +++ b/app/views/projects/merge_requests/show/_mr_accept.html.haml @@ -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: "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" From 09d5868c2286f927ce2cc23dfcc2c446723985b3 Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Tue, 14 Jan 2014 10:47:28 +0200 Subject: [PATCH 7/9] Fix mr controller specs Signed-off-by: Dmitriy Zaporozhets --- app/models/merge_request.rb | 9 +++++---- spec/controllers/merge_requests_controller_spec.rb | 4 ++-- 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index 39165631894..da7aebd944f 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -214,8 +214,8 @@ class MergeRequest < ActiveRecord::Base self.merge end - def automerge!(current_user, merge_commit_message = nil) - if Gitlab::Satellite::MergeAction.new(current_user, self).merge!(merge_commit_message) && 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 @@ -322,9 +322,10 @@ class MergeRequest < ActiveRecord::Base def merge_commit_message message = "Merge branch '#{source_branch}' into '#{target_branch}'" message << "\n\n" - message << title + message << title.to_s message << "\n\n" - message << description + message << description.to_s + message end private diff --git a/spec/controllers/merge_requests_controller_spec.rb b/spec/controllers/merge_requests_controller_spec.rb index 69708edd8b1..f237f358452 100644 --- a/spec/controllers/merge_requests_controller_spec.rb +++ b/spec/controllers/merge_requests_controller_spec.rb @@ -3,7 +3,7 @@ require 'spec_helper' 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 From 9247490109b9d4e4691d304e7bfb4bca6d54e209 Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Tue, 14 Jan 2014 11:49:54 +0200 Subject: [PATCH 8/9] Tests for accept MR with custom message Signed-off-by: Dmitriy Zaporozhets --- .../merge_requests/show/_mr_accept.html.haml | 2 +- features/project/merge_requests.feature | 10 +++ .../steps/project/project_merge_requests.rb | 71 ++++++++++++------- 3 files changed, 58 insertions(+), 25 deletions(-) diff --git a/app/views/projects/merge_requests/show/_mr_accept.html.haml b/app/views/projects/merge_requests/show/_mr_accept.html.haml index 342f2707369..352d843f49f 100644 --- a/app/views/projects/merge_requests/show/_mr_accept.html.haml +++ b/app/views/projects/merge_requests/show/_mr_accept.html.haml @@ -18,7 +18,7 @@ %br If you want to modify merge commit message - %strong - = link_to "click here", "#", class: "js-toggle-visibility-link", title: "Modify merge commit message" + = 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 diff --git a/features/project/merge_requests.feature b/features/project/merge_requests.feature index 63f27c3acc3..4e4ac1a68e4 100644 --- a/features/project/merge_requests.feature +++ b/features/project/merge_requests.feature @@ -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 diff --git a/features/steps/project/project_merge_requests.rb b/features/steps/project/project_merge_requests.rb index 7b0f8cc7ad2..8ef952cf3d1 100644 --- a/features/steps/project/project_merge_requests.rb +++ b/features/steps/project/project_merge_requests.rb @@ -4,60 +4,60 @@ class ProjectMergeRequests < 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 < 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 < 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 < 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 < 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 request\'s comments tab' do + step 'I switch to the merge request\'s 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 < 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 < 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 From 99a2b31e98f3934407b782ee4a2d2a26c8ec4e2a Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Tue, 14 Jan 2014 19:56:52 +0200 Subject: [PATCH 9/9] Dont allow merge without commit message Signed-off-by: Dmitriy Zaporozhets --- app/assets/javascripts/merge_requests.js.coffee | 2 ++ 1 file changed, 2 insertions(+) diff --git a/app/assets/javascripts/merge_requests.js.coffee b/app/assets/javascripts/merge_requests.js.coffee index 047683f00b2..ff843c68d68 100644 --- a/app/assets/javascripts/merge_requests.js.coffee +++ b/app/assets/javascripts/merge_requests.js.coffee @@ -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)