Pass source_commit so that we save a few lookups

app/models/repository.rb

@@ -849,15 +849,12 @@ class Repository
     GitOperationService.new(user, self).with_branch(
       branch_name,
       source_branch_name: source_branch_name,
-      source_project: source_project) do
+      source_project: source_project) do |source_commit|
       index = rugged.index
-      branch_commit = source_project.repository.find_branch(
-        source_branch_name || branch_name)
 
-      parents = if branch_commit
-                  last_commit = branch_commit.dereferenced_target
-                  index.read_tree(last_commit.raw_commit.tree)
-                  [last_commit.sha]
+      parents = if source_commit
+                  index.read_tree(source_commit.raw_commit.tree)
+                  [source_commit.sha]
                 else
                   []
                 end
@@ -904,17 +901,17 @@ class Repository
   end
 
   def merge(user, merge_request, options = {})
-    our_commit = rugged.branches[merge_request.target_branch].target
+    GitOperationService.new(user, self).with_branch(
+      merge_request.target_branch) do |source_commit|
+      our_commit = source_commit.raw_commit
       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?
+      raise 'Invalid merge target' unless our_commit
+      raise 'Invalid merge source' unless their_commit
 
       merge_index = rugged.merge_commits(our_commit, their_commit)
-    return false if merge_index.conflicts?
+      break if merge_index.conflicts?
 
-    GitOperationService.new(user, self).with_branch(
-      merge_request.target_branch) do
       actual_options = options.merge(
         parents: [our_commit, their_commit],
         tree: merge_index.write_tree(rugged),
@@ -924,6 +921,8 @@ class Repository
       merge_request.update(in_progress_merge_commit_sha: commit_id)
       commit_id
     end
+  rescue Repository::CommitError # when merge_index.conflicts?
+    false
   end
 
   def revert(
@@ -936,10 +935,8 @@ class Repository
     GitOperationService.new(user, self).with_branch(
       branch_name,
       source_branch_name: source_branch_name,
-      source_project: source_project) do
+      source_project: source_project) do |source_commit|
 
-      source_sha = source_project.repository.find_source_sha(
-        source_branch_name || branch_name)
       committer = user_to_committer(user)
 
       Rugged::Commit.create(rugged,
@@ -947,7 +944,7 @@ class Repository
         author: committer,
         committer: committer,
         tree: revert_tree_id,
-        parents: [rugged.lookup(source_sha)])
+        parents: [source_commit.sha])
     end
   end
 
@@ -961,10 +958,8 @@ class Repository
     GitOperationService.new(user, self).with_branch(
       branch_name,
       source_branch_name: source_branch_name,
-      source_project: source_project) do
+      source_project: source_project) do |source_commit|
 
-      source_sha = source_project.repository.find_source_sha(
-        source_branch_name || branch_name)
       committer = user_to_committer(user)
 
       Rugged::Commit.create(rugged,
@@ -976,7 +971,7 @@ class Repository
         },
         committer: committer,
         tree: cherry_pick_tree_id,
-        parents: [rugged.lookup(source_sha)])
+        parents: [source_commit.sha])
     end
   end
 
@@ -1145,16 +1140,6 @@ class Repository
     end
   end
 
-  protected
-
-  def find_source_sha(branch_name)
-    if branch_exists?(branch_name)
-      find_branch(branch_name).dereferenced_target.sha
-    else
-      Gitlab::Git::BLANK_SHA
-    end
-  end
-
   private
 
   def git_action(index, action)

app/services/git_operation_service.rb

@@ -37,13 +37,16 @@ GitOperationService = Struct.new(:user, :repository) do
     check_with_branch_arguments!(
       branch_name, source_branch_name, source_project)
 
-    update_branch_with_hooks(branch_name) do |ref|
+    source_commit = source_project.repository.find_branch(
+      source_branch_name || branch_name).try(:dereferenced_target)
+
+    update_branch_with_hooks(branch_name) do
       if repository.project == source_project
-        yield(ref)
+        yield(source_commit)
       else
         repository.with_tmp_ref(
           source_project.repository, source_branch_name) do
-            yield(ref)
+            yield(source_commit)
           end
       end
     end
@@ -54,31 +57,29 @@ GitOperationService = Struct.new(:user, :repository) do
   def update_branch_with_hooks(branch_name)
     update_autocrlf_option
 
-    ref = Gitlab::Git::BRANCH_REF_PREFIX + branch_name
-    oldrev = Gitlab::Git::BLANK_SHA
     was_empty = repository.empty?
 
     # Make commit
-    newrev = yield(ref)
+    newrev = yield
 
     unless newrev
       raise Repository::CommitError.new('Failed to create commit')
     end
 
     branch = repository.find_branch(branch_name)
-    oldrev = if repository.rugged.lookup(newrev).parent_ids.empty? ||
-                branch.nil?
-               Gitlab::Git::BLANK_SHA
+    oldrev = if branch
+               # This could verify we're not losing commits
+               repository.rugged.merge_base(newrev, branch.target)
              else
-               repository.rugged.merge_base(
-                 newrev, branch.dereferenced_target.sha)
+               Gitlab::Git::BLANK_SHA
              end
 
+    ref = Gitlab::Git::BRANCH_REF_PREFIX + branch_name
     with_hooks_and_update_ref(ref, oldrev, newrev) do
       # If repo was empty expire cache
       repository.after_create if was_empty
       repository.after_create_branch if was_empty ||
-                                        oldrev == Gitlab::Git::BLANK_SHA
+                                        Gitlab::Git.blank_ref?(oldrev)
     end
 
     newrev

spec/models/repository_spec.rb

@@ -257,6 +257,24 @@ describe Repository, models: true do
       expect(newdir.path).to eq('newdir')
     end
 
+    context "when committing to another project" do
+      let(:forked_project) { create(:project) }
+
+      it "creates a fork and commit to the forked project" do
+        expect do
+          repository.commit_dir(user, 'newdir',
+            message: 'Create newdir', branch_name: 'patch',
+            source_branch_name: 'master', source_project: forked_project)
+        end.to change { repository.commits('master').count }.by(0)
+
+        expect(repository.branch_exists?('patch')).to be_truthy
+        expect(forked_project.repository.branch_exists?('patch')).to be_falsy
+
+        newdir = repository.tree('patch', 'newdir')
+        expect(newdir.path).to eq('newdir')
+      end
+    end
+
     context "when an author is specified" do
       it "uses the given email/name to set the commit's author" do
         expect do
@@ -758,9 +776,9 @@ describe Repository, models: true do
     end
 
     context 'when the update adds more than one commit' do
-      it 'runs without errors' do
-        old_rev = '33f3729a45c02fc67d00adb1b8bca394b0e761d9'
+      let(:old_rev) { '33f3729a45c02fc67d00adb1b8bca394b0e761d9' }
 
+      it 'runs without errors' do
         # old_rev is an ancestor of new_rev
         expect(repository.rugged.merge_base(old_rev, new_rev)).to eq(old_rev)
 
@@ -779,10 +797,10 @@ describe Repository, models: true do
     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).dereferenced_target.sha
+      let(:branch) { 'master' }
+      let(:old_rev) { repository.find_branch(branch).dereferenced_target.sha }
 
+      it 'raises an exception' do
         # The 'master' branch is NOT an ancestor of new_rev.
         expect(repository.rugged.merge_base(old_rev, new_rev)).not_to eq(old_rev)