Commit 55cc8777 authored by Douwe Maan's avatar Douwe Maan

Merge branch 'ref-update-multiple-commits' into 'master'

Allow updating a branch by multiple commits

In https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/6130 we made
Repository#commit_with_hooks a bit too strict; it only allowed
updating a branch by one commit. It turns out there is a feature in
GitLab Enterprise Edition where we update by multiple commits.

This change makes that possible, adds tests, and renames the method to
the more appropriate 'update_branch_with_hooks'.

See merge request !6246
parents 7f07b1b6 084bac89
......@@ -757,7 +757,7 @@ class Repository
end
def commit_dir(user, path, message, branch)
commit_with_hooks(user, branch) do |ref|
update_branch_with_hooks(user, branch) do |ref|
committer = user_to_committer(user)
options = {}
options[:committer] = committer
......@@ -774,7 +774,7 @@ class Repository
end
def commit_file(user, path, content, message, branch, update)
commit_with_hooks(user, branch) do |ref|
update_branch_with_hooks(user, branch) do |ref|
committer = user_to_committer(user)
options = {}
options[:committer] = committer
......@@ -796,7 +796,7 @@ class Repository
end
def update_file(user, path, content, branch:, previous_path:, message:)
commit_with_hooks(user, branch) do |ref|
update_branch_with_hooks(user, branch) do |ref|
committer = user_to_committer(user)
options = {}
options[:committer] = committer
......@@ -823,7 +823,7 @@ class Repository
end
def remove_file(user, path, message, branch)
commit_with_hooks(user, branch) do |ref|
update_branch_with_hooks(user, branch) do |ref|
committer = user_to_committer(user)
options = {}
options[:committer] = committer
......@@ -871,7 +871,7 @@ class Repository
merge_index = rugged.merge_commits(our_commit, their_commit)
return false if merge_index.conflicts?
commit_with_hooks(user, merge_request.target_branch) do
update_branch_with_hooks(user, merge_request.target_branch) do
actual_options = options.merge(
parents: [our_commit, their_commit],
tree: merge_index.write_tree(rugged),
......@@ -889,7 +889,7 @@ class Repository
return false unless revert_tree_id
commit_with_hooks(user, base_branch) do
update_branch_with_hooks(user, base_branch) do
committer = user_to_committer(user)
source_sha = Rugged::Commit.create(rugged,
message: commit.revert_message,
......@@ -906,7 +906,7 @@ class Repository
return false unless cherry_pick_tree_id
commit_with_hooks(user, base_branch) do
update_branch_with_hooks(user, base_branch) do
committer = user_to_committer(user)
source_sha = Rugged::Commit.create(rugged,
message: commit.message,
......@@ -922,7 +922,7 @@ class Repository
end
def resolve_conflicts(user, branch, params)
commit_with_hooks(user, branch) do
update_branch_with_hooks(user, branch) do
committer = user_to_committer(user)
Rugged::Commit.create(rugged, params.merge(author: committer, committer: committer))
......@@ -1026,7 +1026,7 @@ class Repository
Gitlab::Popen.popen(args, path_to_repo)
end
def commit_with_hooks(current_user, branch)
def update_branch_with_hooks(current_user, branch)
update_autocrlf_option
ref = Gitlab::Git::BRANCH_REF_PREFIX + branch
......@@ -1040,7 +1040,11 @@ class Repository
raise CommitError.new('Failed to create commit')
end
oldrev = rugged.lookup(newrev).parent_ids.first || Gitlab::Git::BLANK_SHA
if rugged.lookup(newrev).parent_ids.empty? || target_branch.nil?
oldrev = Gitlab::Git::BLANK_SHA
else
oldrev = rugged.merge_base(newrev, target_branch.target.sha)
end
GitHooksService.new.execute(current_user, path_to_repo, oldrev, newrev, ref) do
update_ref!(ref, newrev, oldrev)
......
......@@ -441,7 +441,7 @@ describe Repository, models: true do
end
end
describe '#commit_with_hooks' do
describe '#update_branch_with_hooks' do
let(:old_rev) { '0b4bc9a49b562e85de7cc9e834518ea6828729b9' } # git rev-parse feature
let(:new_rev) { 'a74ae73c1ccde9b974a70e82b901588071dc142a' } # commit whose parent is old_rev
......@@ -454,31 +454,64 @@ describe Repository, models: true do
it 'runs without errors' do
expect do
repository.commit_with_hooks(user, 'feature') { new_rev }
repository.update_branch_with_hooks(user, 'feature') { new_rev }
end.not_to raise_error
end
it 'ensures the autocrlf Git option is set to :input' do
expect(repository).to receive(:update_autocrlf_option)
repository.commit_with_hooks(user, 'feature') { new_rev }
repository.update_branch_with_hooks(user, 'feature') { new_rev }
end
context "when the branch wasn't empty" do
it 'updates the head' do
expect(repository.find_branch('feature').target.id).to eq(old_rev)
repository.commit_with_hooks(user, 'feature') { new_rev }
repository.update_branch_with_hooks(user, 'feature') { new_rev }
expect(repository.find_branch('feature').target.id).to eq(new_rev)
end
end
end
context 'when the update adds more than one commit' do
it 'runs without errors' do
old_rev = '33f3729a45c02fc67d00adb1b8bca394b0e761d9'
# old_rev is an ancestor of new_rev
expect(repository.rugged.merge_base(old_rev, new_rev)).to eq(old_rev)
# old_rev is not a direct ancestor (parent) of new_rev
expect(repository.rugged.lookup(new_rev).parent_ids).not_to include(old_rev)
branch = 'feature-ff-target'
repository.add_branch(user, branch, old_rev)
expect { repository.update_branch_with_hooks(user, branch) { new_rev } }.not_to raise_error
end
end
context 'when the update would remove commits from the target branch' do
it 'raises an exception' do
branch = 'master'
old_rev = repository.find_branch(branch).target.sha
# The 'master' branch is NOT an ancestor of new_rev.
expect(repository.rugged.merge_base(old_rev, new_rev)).not_to eq(old_rev)
# Updating 'master' to new_rev would lose the commits on 'master' that
# are not contained in new_rev. This should not be allowed.
expect do
repository.update_branch_with_hooks(user, branch) { new_rev }
end.to raise_error(Repository::CommitError)
end
end
context 'when pre hooks failed' do
it 'gets an error' do
allow_any_instance_of(Gitlab::Git::Hook).to receive(:trigger).and_return([false, ''])
expect do
repository.commit_with_hooks(user, 'feature') { new_rev }
repository.update_branch_with_hooks(user, 'feature') { new_rev }
end.to raise_error(GitHooksService::PreReceiveError)
end
end
......@@ -497,7 +530,7 @@ describe Repository, models: true do
expect(repository).to receive(:expire_has_visible_content_cache)
expect(repository).to receive(:expire_branch_count_cache)
repository.commit_with_hooks(user, 'new-feature') { new_rev }
repository.update_branch_with_hooks(user, 'new-feature') { new_rev }
end
end
......
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