Move common working copy operations to base class

app/services/merge_requests/rebase_service.rb

 module MergeRequests
-  # MergeService class
-  #
-  # Do git merge and in case of success
-  # mark merge request as merged and execute all hooks and notifications
-  # Executed when you do merge via GitLab UI
-  #
-  class RebaseService < MergeRequests::BaseService
-    include Gitlab::Popen
-
-    attr_reader :merge_request
-
+  class RebaseService < MergeRequests::WorkingCopyBaseService
     def execute(merge_request)
       @merge_request = merge_request
 
@@ -22,92 +12,53 @@ module MergeRequests
 
     def rebase
       if merge_request.rebase_in_progress?
-        log('Rebase task canceled: Another rebase is already in progress')
+        log_error('Rebase task canceled: Another rebase is already in progress')
         return false
       end
 
-      # Clone
-      output, status = popen(
-        %W(git clone -b #{merge_request.source_branch} -- #{source_project.repository.path_to_repo} #{tree_path}),
+      run_git_command(
+        %W(clone -b #{merge_request.source_branch} -- #{source_project.repository.path_to_repo} #{tree_path}),
         nil,
-        git_env
+        git_env,
+        'clone repository for rebase'
       )
 
-      unless status.zero?
-        log('Failed to clone repository for rebase:')
-        log(output)
-        return false
-      end
-
-      # Rebase
-      output, status = popen(
-        %W(git pull --rebase #{target_project.repository.path_to_repo} #{merge_request.target_branch}),
+      run_git_command(
+        %W(pull --rebase #{target_project.repository.path_to_repo} #{merge_request.target_branch}),
         tree_path,
-        git_env
+        git_env,
+        'rebase branch'
       )
 
-      unless status.zero?
-        log('Failed to rebase branch:')
-        log(output)
-        return false
-      end
-
-      output, status = popen(
-        %W(git rev-parse #{merge_request.source_branch}),
+      rebase_sha = run_git_command(
+        %W(rev-parse #{merge_request.source_branch}),
         tree_path,
-        git_env
+        git_env,
+        'get SHA of rebased branch'
       )
 
-      unless status.zero?
-        log('Failed to get SHA of rebased branch:')
-        log(output)
-        return false
-      end
+      merge_request.update_attributes(rebase_commit_sha: rebase_sha)
 
-      merge_request.update_attributes(rebase_commit_sha: output.chomp)
-      # Push
-      output, status = popen(
-        %W(git push -f origin #{merge_request.source_branch}),
+      run_git_command(
+        %W(push -f origin #{merge_request.source_branch}),
         tree_path,
-        git_env
+        git_env,
+        'push rebased branch'
       )
 
-      unless status.zero?
-        log('Failed to push rebased branch:')
-        log(output)
-        return false
-      end
-
       true
-    rescue => ex
-      log('Failed to rebase branch:')
-      log(ex.message)
+    rescue GitCommandError
+      false
+    rescue => e
+      log_error('Failed to rebase branch:')
+      log_error(e)
+      false
     ensure
       clean_dir
     end
 
-    def source_project
-      @source_project ||= merge_request.source_project
-    end
-
-    def target_project
-      @target_project ||= merge_request.target_project
-    end
-
     def tree_path
       @tree_path ||= merge_request.rebase_dir_path
     end
-
-    def log(message)
-      Gitlab::GitLogger.error(message)
-    end
-
-    def clean_dir
-      FileUtils.rm_rf(tree_path) if File.exist?(tree_path)
-    end
-
-    def git_env
-      { 'GL_ID' => Gitlab::GlId.gl_id(current_user), 'GL_PROTOCOL' => 'web' }
-    end
   end
 end

app/services/merge_requests/squash_service.rb

 require 'securerandom'
 
 module MergeRequests
-  class SquashService < MergeRequests::BaseService
-    include Gitlab::Popen
-
-    attr_reader :merge_request, :repository, :rugged
+  class SquashService < MergeRequests::WorkingCopyBaseService
+    attr_reader :repository, :rugged
 
     def execute(merge_request)
       @merge_request = merge_request
-      @repository = merge_request.target_project.repository
+      @repository = target_project.repository
       @rugged = repository.rugged
 
       squash || error('Failed to squash. Should be done manually')
@@ -25,96 +23,59 @@ module MergeRequests
       temp_branch = SecureRandom.uuid
 
       if merge_request.squash_in_progress?
-        log('Squash task canceled: Another squash is already in progress')
+        log_error('Squash task canceled: Another squash is already in progress')
         return false
       end
 
-      # Clone
-      output, status = popen(
-        %W(git clone -b #{merge_request.target_branch} -- #{repository.path_to_repo} #{tree_path}),
+      run_git_command(
+        %W(clone -b #{merge_request.target_branch} -- #{repository.path_to_repo} #{tree_path}),
         nil,
-        git_env
+        git_env,
+        'clone repository for squash'
       )
 
-      unless status.zero?
-        log('Failed to clone repository for squash:')
-        log(output)
-        return false
-      end
-
-      # Squash
-      output, status = popen(%w(git apply --cached), tree_path, git_env) do |stdin|
+      run_git_command(%w(apply --cached), tree_path, git_env, 'apply patch') do |stdin|
         stdin.puts(merge_request_to_patch)
       end
 
-      unless status.zero?
-        log('Failed to apply patch:')
-        log(output)
-        return false
-      end
-
-      output, status = popen(
-        %W(git commit -C #{merge_request.diff_head_sha}),
+      run_git_command(
+        %W(commit -C #{merge_request.diff_head_sha}),
         tree_path,
-        git_env.merge('GIT_COMMITTER_NAME' => current_user.name, 'GIT_COMMITTER_EMAIL' => current_user.email)
+        git_env.merge('GIT_COMMITTER_NAME' => current_user.name, 'GIT_COMMITTER_EMAIL' => current_user.email),
+        'commit squashed changes'
       )
 
-      unless status.zero?
-        log('Failed to commit squashed changes:')
-        log(output)
-        return false
-      end
-
-      output, status = popen(%w(git rev-parse HEAD), tree_path, git_env)
-
-      unless status.zero?
-        log("Failed to get SHA of squashed branch #{temp_branch}:")
-        log(output)
-        return false
-      end
-
-      target = output.chomp
-
-      # Push to temporary ref
-      output, status = popen(%W(git push -f origin HEAD:#{temp_branch}), tree_path, git_env)
+      squash_sha = run_git_command(
+        %w(rev-parse HEAD),
+        tree_path,
+        git_env,
+        "get SHA of squashed branch #{temp_branch}"
+      )
 
-      unless status.zero?
-        log('Failed to push squashed branch:')
-        log(output)
-        return false
-      end
+      run_git_command(
+        %W(push -f origin HEAD:#{temp_branch}),
+        tree_path,
+        git_env,
+        'push squashed branch'
+      )
 
-      repository.rm_branch(current_user, temp_branch, skip_event: true)
+      repository.rm_branch(current_user, temp_branch)
 
-      success(squash_oid: target)
-    rescue => ex
-      log("Failed to squash merge request #{project.path_with_namespace}#{merge_request.to_reference}:")
-      log(ex.message)
+      success(squash_sha: squash_sha)
+    rescue GitCommandError
+      false
+    rescue => e
+      log_error("Failed to squash merge request #{merge_request.to_reference(full: true)}:")
+      log_error(e.message)
       false
     ensure
       clean_dir
     end
 
-    def inspect
-      ''
-    end
-
     def tree_path
       @tree_path ||= merge_request.squash_dir_path
     end
 
-    def log(message)
-      Gitlab::GitLogger.error(message)
-    end
-
-    def clean_dir
-      FileUtils.rm_rf(tree_path) if File.exist?(tree_path)
-    end
-
-    def git_env
-      { 'GL_ID' => Gitlab::GlId.gl_id(current_user), 'GL_PROTOCOL' => 'web' }
-    end
-
     def merge_request_to_patch
       @merge_request_to_patch ||= rugged.diff(merge_request.diff_base_sha, merge_request.diff_head_sha).patch
     end

app/services/merge_requests/working_copy_base_service.rb

+module MergeRequests
+  class WorkingCopyBaseService < MergeRequests::BaseService
+    class GitCommandError < StandardError; end
+
+    include Gitlab::Popen
+
+    attr_reader :merge_request
+
+    def run_git_command(command, path, env, message = nil, &block)
+      git_command = [Gitlab.config.git.bin_path] + command
+      output, status = popen(git_command, path, env, &block)
+
+      unless status.zero?
+        log_error("Failed to #{message}:") if message
+        log_error(output)
+
+        raise GitCommandError
+      end
+
+      output.chomp
+    end
+
+    def source_project
+      @source_project ||= merge_request.source_project
+    end
+
+    def target_project
+      @target_project ||= merge_request.target_project
+    end
+
+    def log_error(message)
+      Gitlab::GitLogger.error(message)
+    end
+
+    def clean_dir
+      FileUtils.rm_rf(tree_path) if File.exist?(tree_path)
+    end
+
+    def git_env
+      { 'GL_ID' => Gitlab::GlId.gl_id(current_user), 'GL_PROTOCOL' => 'web' }
+    end
+
+    # Don't try to print expensive instance variables.
+    def inspect
+      "#<#{self.class} #{merge_request.to_reference(full: true)}>"
+    end
+  end
+end

spec/services/merge_requests/squash_service_spec.rb

@@ -14,7 +14,7 @@ describe MergeRequests::SquashService do
     context 'when the squash succeeds' do
       it 'returns the squashed commit SHA' do
         expect(service.execute(merge_request)).to match(status: :success,
-                                                        squash_oid: a_string_matching(/\h{40}/))
+                                                        squash_sha: a_string_matching(/\h{40}/))
       end
 
       it 'cleans up the temporary directory' do
@@ -25,7 +25,7 @@ describe MergeRequests::SquashService do
 
       context 'the squashed commit' do
         let(:squashed_commit) do
-          squash_oid = service.execute(merge_request)[:squash_oid]
+          squash_oid = service.execute(merge_request)[:squash_sha]
 
           project.repository.commit(squash_oid)
         end
@@ -46,11 +46,11 @@ describe MergeRequests::SquashService do
     end
 
     stages = {
-      'clone repository' => ['git', 'clone'],
-      'apply patch' => ['git', 'apply'],
-      'commit squashed changes' => ['git', 'commit'],
-      'get SHA of squashed branch' => ['git', 'rev-parse'],
-      'push squashed branch' => ['git', 'push']
+      'clone repository' => 'clone',
+      'apply patch' => 'apply',
+      'commit squashed changes' => 'commit',
+      'get SHA of squashed branch' => 'rev-parse',
+      'push squashed branch' => 'push'
     }
 
     stages.each do |stage, command|
@@ -58,16 +58,18 @@ describe MergeRequests::SquashService do
         let(:error) { 'A test error' }
 
         before do
+          git_command = a_collection_starting_with([Gitlab.config.git.bin_path, command])
+
           allow(service).to receive(:popen).and_return(['', 0])
 
-          allow(service).to receive(:popen).with(a_collection_starting_with(command), anything, anything) do
+          allow(service).to receive(:popen).with(git_command, anything, anything) do
             [error, 1]
           end
         end
 
         it 'logs the stage and output' do
-          expect(service).to receive(:log).with(a_string_including(stage))
-          expect(service).to receive(:log).with(error)
+          expect(service).to receive(:log_error).with(a_string_including(stage))
+          expect(service).to receive(:log_error).with(error)
 
           service.execute(merge_request)
         end
@@ -93,8 +95,8 @@ describe MergeRequests::SquashService do
       end
 
       it 'logs the MR reference and exception' do
-        expect(service).to receive(:log).with(a_string_including("#{project.path_with_namespace}#{merge_request.to_reference}"))
-        expect(service).to receive(:log).with(error)
+        expect(service).to receive(:log_error).with(a_string_including("#{project.path_with_namespace}#{merge_request.to_reference}"))
+        expect(service).to receive(:log_error).with(error)
 
         service.execute(merge_request)
       end