diff --git a/app/models/repository.rb b/app/models/repository.rb
index 5b670cb4b8fa6c8a3dc504affbc886cad045afc2..09487b62f989fda2974ba0eae1f1891956655f39 100644
--- a/app/models/repository.rb
+++ b/app/models/repository.rb
@@ -769,9 +769,9 @@ class Repository
end
end
- def merge(user, source_sha, target_branch, options = {})
- our_commit = rugged.branches[target_branch].target
- their_commit = rugged.lookup(source_sha)
+ def merge(user, merge_request, options = {})
+ our_commit = rugged.branches[merge_request.target_branch].target
+ their_commit = rugged.lookup(merge_request.diff_head_sha)
raise "Invalid merge target" if our_commit.nil?
raise "Invalid merge source" if their_commit.nil?
@@ -779,14 +779,16 @@ class Repository
merge_index = rugged.merge_commits(our_commit, their_commit)
return false if merge_index.conflicts?
- commit_with_hooks(user, target_branch) do |ref|
+ commit_with_hooks(user, merge_request.target_branch) do |tmp_ref|
actual_options = options.merge(
parents: [our_commit, their_commit],
tree: merge_index.write_tree(rugged),
- update_ref: ref
+ update_ref: tmp_ref
)
- Rugged::Commit.create(rugged, actual_options)
+ commit_id = Rugged::Commit.create(rugged, actual_options)
+ merge_request.update(in_progress_merge_commit_sha: commit_id)
+ commit_id
end
end
diff --git a/app/services/merge_requests/merge_service.rb b/app/services/merge_requests/merge_service.rb
index f1b1d90c4578f81058ac5de0d88aab343c16b166..0dac0614141b69cc350a8bbb0ee11fae4d332a17 100644
--- a/app/services/merge_requests/merge_service.rb
+++ b/app/services/merge_requests/merge_service.rb
@@ -34,7 +34,7 @@ module MergeRequests
committer: committer
}
- commit_id = repository.merge(current_user, merge_request.diff_head_sha, merge_request.target_branch, options)
+ commit_id = repository.merge(current_user, merge_request, options)
merge_request.update(merge_commit_sha: commit_id)
rescue GitHooksService::PreReceiveError => e
merge_request.update(merge_error: e.message)
@@ -43,6 +43,8 @@ module MergeRequests
merge_request.update(merge_error: "Something went wrong during merge")
Rails.logger.error(e.message)
false
+ ensure
+ merge_request.update(in_progress_merge_commit_sha: nil)
end
def after_merge
diff --git a/db/migrate/20160629025435_add_column_in_progress_merge_commit_sha_to_merge_requests.rb b/db/migrate/20160629025435_add_column_in_progress_merge_commit_sha_to_merge_requests.rb
new file mode 100644
index 0000000000000000000000000000000000000000..7c5f76572ef80e92b6b44f360f10430972396b9d
--- /dev/null
+++ b/db/migrate/20160629025435_add_column_in_progress_merge_commit_sha_to_merge_requests.rb
@@ -0,0 +1,8 @@
+# See http://doc.gitlab.com/ce/development/migration_style_guide.html
+# for more information on how to write migrations for GitLab.
+
+class AddColumnInProgressMergeCommitShaToMergeRequests < ActiveRecord::Migration
+ def change
+ add_column :merge_requests, :in_progress_merge_commit_sha, :string
+ end
+end
diff --git a/db/schema.rb b/db/schema.rb
index 4562e6bb0c3395ea024888f7e94feb222bde033a..64019cf79bb4f987a049dc7723e58c083dac1eef 100644
--- a/db/schema.rb
+++ b/db/schema.rb
@@ -626,6 +626,7 @@ ActiveRecord::Schema.define(version: 20160712171823) do
t.string "merge_commit_sha"
t.datetime "deleted_at"
t.integer "lock_version", default: 0, null: false
+ t.string "in_progress_merge_commit_sha"
end
add_index "merge_requests", ["assignee_id"], name: "index_merge_requests_on_assignee_id", using: :btree
diff --git a/lib/gitlab/checks/matching_merge_request.rb b/lib/gitlab/checks/matching_merge_request.rb
new file mode 100644
index 0000000000000000000000000000000000000000..849848515da66a210ff00a0f2c9de3ce6f306f7f
--- /dev/null
+++ b/lib/gitlab/checks/matching_merge_request.rb
@@ -0,0 +1,18 @@
+module Gitlab
+ module Checks
+ class MatchingMergeRequest
+ def initialize(newrev, branch_name, project)
+ @newrev = newrev
+ @branch_name = branch_name
+ @project = project
+ end
+
+ def match?
+ @project.merge_requests
+ .with_state(:locked)
+ .where(in_progress_merge_commit_sha: @newrev, target_branch: @branch_name)
+ .exists?
+ end
+ end
+ end
+end
diff --git a/lib/gitlab/git_access.rb b/lib/gitlab/git_access.rb
index e20e33382622df468cde01f1519b926c3f9794f0..feaf845209e895b64a8767c486a1b1cff1f6d6df 100644
--- a/lib/gitlab/git_access.rb
+++ b/lib/gitlab/git_access.rb
@@ -177,10 +177,15 @@ module Gitlab
Gitlab::ForcePushCheck.force_push?(project, oldrev, newrev)
end
+
def protocol_allowed?
Gitlab::ProtocolAccess.allowed?(protocol)
end
+ def matching_merge_request?(newrev, branch_name)
+ Checks::MatchingMergeRequest.new(newrev, branch_name, project).match?
+ end
+
private
def protected_branch_action(oldrev, newrev, branch_name)
@@ -190,6 +195,8 @@ module Gitlab
elsif Gitlab::Git.blank_ref?(newrev)
# and we dont allow remove of protected branch
:remove_protected_branches
+ elsif matching_merge_request?(newrev, branch_name) && project.developers_can_merge_to_protected_branch?(branch_name)
+ :push_code
elsif project.developers_can_push_to_protected_branch?(branch_name)
:push_code
else
diff --git a/spec/lib/gitlab/diff/position_tracer_spec.rb b/spec/lib/gitlab/diff/position_tracer_spec.rb
index 08312e60f4a7f40b45df10e55a951b279bf919d5..c268f84c75900ddea0c604fb40503cf8685ea981 100644
--- a/spec/lib/gitlab/diff/position_tracer_spec.rb
+++ b/spec/lib/gitlab/diff/position_tracer_spec.rb
@@ -1639,7 +1639,8 @@ describe Gitlab::Diff::PositionTracer, lib: true do
committer: committer
}
- repository.merge(current_user, second_create_file_commit.sha, branch_name, options)
+ merge_request = create(:merge_request, source_branch: second_create_file_commit.sha, target_branch: branch_name, source_project: project)
+ repository.merge(current_user, merge_request, options)
project.commit(branch_name)
end
diff --git a/spec/lib/gitlab/git_access_spec.rb b/spec/lib/gitlab/git_access_spec.rb
index b90d9c724f13049a0391e6d73826e6a8e67b1882..50cf2f1e93f0eda7687387e3bf229c7a4e3b379b 100644
--- a/spec/lib/gitlab/git_access_spec.rb
+++ b/spec/lib/gitlab/git_access_spec.rb
@@ -181,96 +181,47 @@ describe Gitlab::GitAccess, lib: true do
end
describe 'push_access_check' do
- def protect_feature_branch
- create(:protected_branch, name: 'feature', project: project)
- end
+ before { merge_into_protected_branch }
+ let(:unprotected_branch) { FFaker::Internet.user_name }
- def changes
- {
- push_new_branch: "#{Gitlab::Git::BLANK_SHA} 570e7b2ab refs/heads/wow",
+ let(:changes) do
+ { push_new_branch: "#{Gitlab::Git::BLANK_SHA} 570e7b2ab refs/heads/wow",
push_master: '6f6d7e7ed 570e7b2ab refs/heads/master',
push_protected_branch: '6f6d7e7ed 570e7b2ab refs/heads/feature',
push_remove_protected_branch: "570e7b2ab #{Gitlab::Git::BLANK_SHA} "\
'refs/heads/feature',
push_tag: '6f6d7e7ed 570e7b2ab refs/tags/v1.0.0',
push_new_tag: "#{Gitlab::Git::BLANK_SHA} 570e7b2ab refs/tags/v7.8.9",
- push_all: ['6f6d7e7ed 570e7b2ab refs/heads/master', '6f6d7e7ed 570e7b2ab refs/heads/feature']
- }
- end
-
- def self.permissions_matrix
- {
- master: {
- push_new_branch: true,
- push_master: true,
- push_protected_branch: true,
- push_remove_protected_branch: false,
- push_tag: true,
- push_new_tag: true,
- push_all: true,
- },
-
- developer: {
- push_new_branch: true,
- push_master: true,
- push_protected_branch: false,
- push_remove_protected_branch: false,
- push_tag: false,
- push_new_tag: true,
- push_all: false,
- },
-
- reporter: {
- push_new_branch: false,
- push_master: false,
- push_protected_branch: false,
- push_remove_protected_branch: false,
- push_tag: false,
- push_new_tag: false,
- push_all: false,
- },
-
- guest: {
- push_new_branch: false,
- push_master: false,
- push_protected_branch: false,
- push_remove_protected_branch: false,
- push_tag: false,
- push_new_tag: false,
- push_all: false,
- }
- }
+ push_all: ['6f6d7e7ed 570e7b2ab refs/heads/master', '6f6d7e7ed 570e7b2ab refs/heads/feature'],
+ merge_into_protected_branch: "0b4bc9a #{merge_into_protected_branch} refs/heads/feature" }
end
- def self.updated_permissions_matrix
- updated_permissions_matrix = permissions_matrix.dup
- updated_permissions_matrix[:developer][:push_protected_branch] = true
- updated_permissions_matrix[:developer][:push_all] = true
- updated_permissions_matrix
+ def stub_git_hooks
+ # Running the `pre-receive` hook is expensive, and not necessary for this test.
+ allow_any_instance_of(GitHooksService).to receive(:execute).and_yield
end
- permissions_matrix.keys.each do |role|
- describe "#{role} access" do
- before { protect_feature_branch }
- before { project.team << [user, role] }
+ def merge_into_protected_branch
+ @protected_branch_merge_commit ||= begin
+ stub_git_hooks
+ project.repository.add_branch(user, unprotected_branch, 'feature')
+ target_branch = project.repository.lookup('feature')
+ source_branch = project.repository.commit_file(user, FFaker::InternetSE.login_user_name, FFaker::HipsterIpsum.paragraph, FFaker::HipsterIpsum.sentence, unprotected_branch, false)
+ rugged = project.repository.rugged
+ author = { email: "email@example.com", time: Time.now, name: "Example Git User" }
- permissions_matrix[role].each do |action, allowed|
- context action do
- subject { access.push_access_check(changes[action]) }
- it { expect(subject.allowed?).to allowed ? be_truthy : be_falsey }
- end
- end
+ merge_index = rugged.merge_commits(target_branch, source_branch)
+ Rugged::Commit.create(rugged, author: author, committer: author, message: "commit message", parents: [target_branch, source_branch], tree: merge_index.write_tree(rugged))
end
end
- context "with enabled developers push to protected branches " do
- updated_permissions_matrix.keys.each do |role|
+ def self.run_permission_checks(permissions_matrix)
+ permissions_matrix.keys.each do |role|
describe "#{role} access" do
- before { create(:protected_branch, name: 'feature', developers_can_push: true, project: project) }
before { project.team << [user, role] }
- updated_permissions_matrix[role].each do |action, allowed|
+ permissions_matrix[role].each do |action, allowed|
context action do
subject { access.push_access_check(changes[action]) }
@@ -280,5 +231,97 @@ describe Gitlab::GitAccess, lib: true do
end
end
end
+
+ permissions_matrix = {
+ master: {
+ push_new_branch: true,
+ push_master: true,
+ push_protected_branch: true,
+ push_remove_protected_branch: false,
+ push_tag: true,
+ push_new_tag: true,
+ push_all: true,
+ merge_into_protected_branch: true
+ },
+
+ developer: {
+ push_new_branch: true,
+ push_master: true,
+ push_protected_branch: false,
+ push_remove_protected_branch: false,
+ push_tag: false,
+ push_new_tag: true,
+ push_all: false,
+ merge_into_protected_branch: false
+ },
+
+ reporter: {
+ push_new_branch: false,
+ push_master: false,
+ push_protected_branch: false,
+ push_remove_protected_branch: false,
+ push_tag: false,
+ push_new_tag: false,
+ push_all: false,
+ merge_into_protected_branch: false
+ },
+
+ guest: {
+ push_new_branch: false,
+ push_master: false,
+ push_protected_branch: false,
+ push_remove_protected_branch: false,
+ push_tag: false,
+ push_new_tag: false,
+ push_all: false,
+ merge_into_protected_branch: false
+ }
+ }
+
+ [['feature', 'exact'], ['feat*', 'wildcard']].each do |protected_branch_name, protected_branch_type|
+ context do
+ before { create(:protected_branch, name: protected_branch_name, project: project) }
+
+ run_permission_checks(permissions_matrix)
+ end
+
+ context "when 'developers can push' is turned on for the #{protected_branch_type} protected branch" do
+ before { create(:protected_branch, name: protected_branch_name, developers_can_push: true, project: project) }
+
+ run_permission_checks(permissions_matrix.deep_merge(developer: { push_protected_branch: true, push_all: true, merge_into_protected_branch: true }))
+ end
+
+ context "when 'developers can merge' is turned on for the #{protected_branch_type} protected branch" do
+ before { create(:protected_branch, name: protected_branch_name, developers_can_merge: true, project: project) }
+
+ context "when a merge request exists for the given source/target branch" do
+ context "when the merge request is in progress" do
+ before do
+ create(:merge_request, source_project: project, source_branch: unprotected_branch, target_branch: 'feature', state: 'locked', in_progress_merge_commit_sha: merge_into_protected_branch)
+ end
+
+ run_permission_checks(permissions_matrix.deep_merge(developer: { merge_into_protected_branch: true }))
+ end
+
+ context "when the merge request is not in progress" do
+ before do
+ create(:merge_request, source_project: project, source_branch: unprotected_branch, target_branch: 'feature', in_progress_merge_commit_sha: nil)
+ end
+
+ run_permission_checks(permissions_matrix.deep_merge(developer: { merge_into_protected_branch: false }))
+ end
+ end
+
+ context "when a merge request does not exist for the given source/target branch" do
+ run_permission_checks(permissions_matrix.deep_merge(developer: { merge_into_protected_branch: false }))
+ end
+ end
+
+ context "when 'developers can merge' and 'developers can push' are turned on for the #{protected_branch_type} protected branch" do
+ before { create(:protected_branch, name: protected_branch_name, developers_can_merge: true, developers_can_push: true, project: project) }
+
+ run_permission_checks(permissions_matrix.deep_merge(developer: { push_protected_branch: true, push_all: true, merge_into_protected_branch: true }))
+ end
+ end
end
end
diff --git a/spec/models/repository_spec.rb b/spec/models/repository_spec.rb
index b39b958450c0ef17d9375824bc846f6ad024d993..e14cec589fe4bbdb92227c22bdea2c37d4f161f5 100644
--- a/spec/models/repository_spec.rb
+++ b/spec/models/repository_spec.rb
@@ -4,16 +4,17 @@ describe Repository, models: true do
include RepoHelpers
TestBlob = Struct.new(:name)
- let(:repository) { create(:project).repository }
+ let(:project) { create(:project) }
+ let(:repository) { project.repository }
let(:user) { create(:user) }
let(:commit_options) do
author = repository.user_to_committer(user)
{ message: 'Test message', committer: author, author: author }
end
let(:merge_commit) do
- source_sha = repository.find_branch('feature').target
- merge_commit_sha = repository.merge(user, source_sha, 'master', commit_options)
- repository.commit(merge_commit_sha)
+ merge_request = create(:merge_request, source_branch: 'feature', target_branch: 'master', source_project: project)
+ merge_commit_id = repository.merge(user, merge_request, commit_options)
+ repository.commit(merge_commit_id)
end
describe '#branch_names_contains' do
diff --git a/spec/services/merge_requests/refresh_service_spec.rb b/spec/services/merge_requests/refresh_service_spec.rb
index 06f56d85aa86955c733bc0ff0e90a51eff3b5469..ce643b3f860be7505ad8f04447eeeb12f159cdb6 100644
--- a/spec/services/merge_requests/refresh_service_spec.rb
+++ b/spec/services/merge_requests/refresh_service_spec.rb
@@ -88,8 +88,7 @@ describe MergeRequests::RefreshService, services: true do
# Merge master -> feature branch
author = { email: 'test@gitlab.com', time: Time.now, name: "Me" }
commit_options = { message: 'Test message', committer: author, author: author }
- master_commit = @project.repository.commit('master')
- @project.repository.merge(@user, master_commit.id, 'feature', commit_options)
+ @project.repository.merge(@user, @merge_request, commit_options)
commit = @project.repository.commit('feature')
service.new(@project, @user).execute(@oldrev, commit.id, 'refs/heads/feature')
reload_mrs