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

2 files changed, 53 insertions, 16 deletions

diff --git a/app/models/repository.rb b/app/models/repository.rb
index 414b82516bc..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
@@ -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..7624050878e 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,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