Merge pull request #1743 from riyad/fix-merging-bugs

Fix merging bugs

2 files changed, 74 insertions, 34 deletions

diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb
index f4644dacc61..70780b75d45 100644
--- a/app/models/merge_request.rb
+++ b/app/models/merge_request.rb
@@ -135,7 +135,8 @@ class MergeRequest < ActiveRecord::Base
   end
 
   def mark_as_unmergable
-    self.update_attributes state: CANNOT_BE_MERGED
+    self.state = CANNOT_BE_MERGED
+    self.save
   end
 
   def reloaded_commits
@@ -166,7 +167,7 @@ class MergeRequest < ActiveRecord::Base
   end
 
   def automerge!(current_user)
-    if Gitlab::Merge.new(self, current_user).merge && self.unmerged_commits.empty?
+    if Gitlab::Merge.new(self, current_user).merge! && self.unmerged_commits.empty?
       self.merge!(current_user.id)
       true
     end
diff --git a/lib/gitlab/merge.rb b/lib/gitlab/merge.rb
index 79f86fad37a..51a71e55f03 100644
--- a/lib/gitlab/merge.rb
+++ b/lib/gitlab/merge.rb
@@ -1,32 +1,53 @@
 module Gitlab
   class Merge
-    attr_accessor :project, :merge_request, :user
+    attr_accessor :merge_request, :project, :user
 
     def initialize(merge_request, user)
-      self.user = user
-      self.merge_request = merge_request
-      self.project = merge_request.project
+      @merge_request = merge_request
+      @project = merge_request.project
+      @user = user
     end
 
     def can_be_merged?
-      result = false
-      process do |repo, output|
-        result = !(output =~ /CONFLICT/)
+      in_locked_and_timed_satellite do |merge_repo|
+        merge_in_satellite!(merge_repo)
       end
-      result
     end
 
-    def merge
-      process do |repo, output|
-        if output =~ /CONFLICT/
-          false  
-        else 
-          !!repo.git.push({}, "origin", merge_request.target_branch)
+    # Merges the source branch into the target branch in the satellite and
+    # pushes it back to Gitolite.
+    # It also removes the source branch if requested in the merge request.
+    #
+    # Returns false if the merge produced conflicts
+    # Returns false if pushing from the satallite to Gitolite failed or was rejected
+    # Returns true otherwise
+    def merge!
+      in_locked_and_timed_satellite do |merge_repo|
+        if merge_in_satellite!(merge_repo)
+          # push merge back to Gitolite
+          # will raise CommandFailed when push fails
+          merge_repo.git.push({raise: true}, :origin, merge_request.target_branch)
+
+          # remove source branch
+          if merge_request.should_remove_source_branch && !project.root_ref?(merge_request.source_branch)
+            # will raise CommandFailed when push fails
+            merge_repo.git.push({raise: true}, :origin, ":#{merge_request.source_branch}")
+          end
+
+          # merge, push and branch removal successful
+          true
         end
       end
+    rescue Grit::Git::CommandFailed
+      false
     end
 
-    def process
+    private
+
+    # * Sets a 30s timeout for Git
+    # * Locks the satellite repo
+    # * Yields the prepared satallite repo
+    def in_locked_and_timed_satellite
       Grit::Git.with_timeout(30.seconds) do
         lock_file = Rails.root.join("tmp", "#{project.path}.lock")
 
@@ -37,29 +58,47 @@ module Gitlab
             raise "Satellite doesn't exist"
           end
 
-          project.satellite.clear
-
           Dir.chdir(project.satellite.path) do
-            merge_repo = Grit::Repo.new('.')
-            merge_repo.git.sh "git reset --hard"
-            merge_repo.git.sh "git fetch origin"
-            merge_repo.git.sh "git config user.name \"#{user.name}\""
-            merge_repo.git.sh "git config user.email \"#{user.email}\""
-            merge_repo.git.sh "git checkout -b #{merge_request.target_branch} origin/#{merge_request.target_branch}"
-            output = merge_repo.git.pull({}, "--no-ff", "origin", merge_request.source_branch)
-
-            #remove source-branch
-            if merge_request.should_remove_source_branch && !project.root_ref?(merge_request.source_branch)
-              merge_repo.git.sh "git push origin :#{merge_request.source_branch}"
-            end
-
-            yield(merge_repo, output)
+            repo = Grit::Repo.new('.')
+
+            return yield repo
           end
         end
       end
-
     rescue Grit::Git::GitTimeout
       return false
     end
+
+    # Merges the source_branch into the target_branch in the satellite.
+    #
+    # Note: it will clear out the satellite before doing anything
+    #
+    # Returns false if the merge produced conflicts
+    # Returns true otherwise
+    def merge_in_satellite!(repo)
+      prepare_satelite!(repo)
+
+      # create target branch in satellite at the corresponding commit from Gitolite
+      repo.git.checkout({b: true}, merge_request.target_branch, "origin/#{merge_request.target_branch}")
+
+      # merge the source branch from Gitolite into the satellite
+      # will raise CommandFailed when merge fails
+      repo.git.pull({no_ff: true, raise: true}, :origin, merge_request.source_branch)
+    rescue Grit::Git::CommandFailed
+      false
+    end
+
+    # * Clears the satellite
+    # * Updates the satellite from Gitolite
+    # * Sets up Git variables for the user
+    def prepare_satelite!(repo)
+      project.satellite.clear
+
+      repo.git.reset(hard: true)
+      repo.git.fetch({}, :origin)
+
+      repo.git.config({}, "user.name", user.name)
+      repo.git.config({}, "user.email", user.email)
+    end
   end
 end