From a75026915863280c78fe73211e423bf6f4ce11c4 Mon Sep 17 00:00:00 2001 From: Jacob Vosmaer Date: Wed, 7 Sep 2016 16:26:06 +0200 Subject: Allow adding multiple commits in commit_with_hooks --- app/models/repository.rb | 6 +++++- spec/models/repository_spec.rb | 19 +++++++++++++++++++ 2 files changed, 24 insertions(+), 1 deletion(-) diff --git a/app/models/repository.rb b/app/models/repository.rb index 414b82516bc..83605982933 100644 --- a/app/models/repository.rb +++ b/app/models/repository.rb @@ -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) diff --git a/spec/models/repository_spec.rb b/spec/models/repository_spec.rb index afc7dc5db81..17eecf6bbd6 100644 --- a/spec/models/repository_spec.rb +++ b/spec/models/repository_spec.rb @@ -473,6 +473,25 @@ describe Repository, models: true do end end + context 'when the update adds more than one commit' do + it 'runs without errors' do + old_rev = '33f3729a45c02fc67d00adb1b8bca394b0e761d9' # ancestor of new_rev by more than one commit + branch = 'feature-ff-target' + repository.add_branch(user, branch, old_rev) + + expect { repository.commit_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 + # We use the fact that 'master' has diverged from 'feature' (new_rev): + # updating 'master' to new_rev would make us lose commits, which should + # not happen. + expect { repository.commit_with_hooks(user, 'master') { new_rev } }.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, '']) -- cgit v1.2.1 From 0f08bb86d8731edd40f9348335987f55a4ba9f11 Mon Sep 17 00:00:00 2001 From: Jacob Vosmaer Date: Wed, 7 Sep 2016 16:28:26 +0200 Subject: Rename {commit,update_branch}_with_hooks --- app/models/repository.rb | 18 +++++++++--------- spec/models/repository_spec.rb | 16 ++++++++-------- 2 files changed, 17 insertions(+), 17 deletions(-) diff --git a/app/models/repository.rb b/app/models/repository.rb index 83605982933..7b7090b8a73 100644 --- a/app/models/repository.rb +++ b/app/models/repository.rb @@ -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 diff --git a/spec/models/repository_spec.rb b/spec/models/repository_spec.rb index 17eecf6bbd6..bc899027d6b 100644 --- a/spec/models/repository_spec.rb +++ b/spec/models/repository_spec.rb @@ -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,20 +454,20 @@ 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 @@ -479,7 +479,7 @@ describe Repository, models: true do branch = 'feature-ff-target' repository.add_branch(user, branch, old_rev) - expect { repository.commit_with_hooks(user, branch) { new_rev } }.not_to raise_error + expect { repository.update_branch_with_hooks(user, branch) { new_rev } }.not_to raise_error end end @@ -488,7 +488,7 @@ describe Repository, models: true do # We use the fact that 'master' has diverged from 'feature' (new_rev): # updating 'master' to new_rev would make us lose commits, which should # not happen. - expect { repository.commit_with_hooks(user, 'master') { new_rev } }.to raise_error(Repository::CommitError) + expect { repository.update_branch_with_hooks(user, 'master') { new_rev } }.to raise_error(Repository::CommitError) end end @@ -497,7 +497,7 @@ describe Repository, models: true 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 @@ -516,7 +516,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 -- cgit v1.2.1 From 084bac8935f6532d28db71fbf14e68d9060337e8 Mon Sep 17 00:00:00 2001 From: Jacob Vosmaer Date: Wed, 7 Sep 2016 16:56:11 +0200 Subject: Express intentions as expectations --- spec/models/repository_spec.rb | 24 +++++++++++++++++++----- 1 file changed, 19 insertions(+), 5 deletions(-) diff --git a/spec/models/repository_spec.rb b/spec/models/repository_spec.rb index bc899027d6b..7624050878e 100644 --- a/spec/models/repository_spec.rb +++ b/spec/models/repository_spec.rb @@ -475,7 +475,14 @@ describe Repository, models: true do context 'when the update adds more than one commit' do it 'runs without errors' do - old_rev = '33f3729a45c02fc67d00adb1b8bca394b0e761d9' # ancestor of new_rev by more than one commit + 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) @@ -485,10 +492,17 @@ describe Repository, models: true do context 'when the update would remove commits from the target branch' do it 'raises an exception' do - # We use the fact that 'master' has diverged from 'feature' (new_rev): - # updating 'master' to new_rev would make us lose commits, which should - # not happen. - expect { repository.update_branch_with_hooks(user, 'master') { new_rev } }.to raise_error(Repository::CommitError) + 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 -- cgit v1.2.1