Move git operations for squash into Gitlab::Git

ee/app/models/ee/merge_request.rb

@@ -16,10 +16,6 @@ module EE
       delegate :sha, to: :base_pipeline, prefix: :base_pipeline, allow_nil: true
     end
 
-    def squash_dir_path
-      File.join(::Gitlab.config.shared.path, 'tmp/squash', source_project.id.to_s, id.to_s).to_s
-    end
-
     def rebase_in_progress?
       # The source project can be deleted
       return false unless source_project
@@ -31,14 +27,7 @@ module EE
       # The source project can be deleted
       return false unless source_project
 
-      File.exist?(squash_dir_path) && !clean_stuck_squash
-    end
-
-    def clean_stuck_squash
-      if File.mtime(squash_dir_path) < 15.minutes.ago
-        FileUtils.rm_rf(squash_dir_path)
-        true
-      end
+      source_project.repository.squash_in_progress?(id)
     end
 
     def squash

ee/app/services/merge_requests/squash_service.rb

@@ -2,12 +2,9 @@ require 'securerandom'
 
 module MergeRequests
   class SquashService < MergeRequests::WorkingCopyBaseService
-    attr_reader :repository, :rugged
-
     def execute(merge_request)
       @merge_request = merge_request
       @repository = target_project.repository
-      @rugged = repository.rugged
 
       squash || error('Failed to squash. Should be done manually.')
     end
@@ -25,113 +22,18 @@ module MergeRequests
         return error('Squash task canceled: another squash is already in progress.')
       end
 
-      run_git_command(
-        %W(worktree add --no-checkout --detach #{tree_path}),
-        repository.path_to_repo,
-        git_env,
-        'add worktree for squash'
-      )
-
-      configure_sparse_checkout
-
-      diff = git_command(%W(diff --binary #{diff_range}))
-      apply = git_command(%w(apply --index))
-
-      run_command(
-        ["#{diff.join(' ')} | #{apply.join(' ')}"],
-        tree_path,
-        git_env,
-        'apply patch'
-      )
-
-      run_git_command(
-        %W(commit --no-verify --message #{merge_request.title}),
-        tree_path,
-        git_env.merge('GIT_COMMITTER_NAME' => current_user.name,
-                      'GIT_COMMITTER_EMAIL' => current_user.email,
-                      'GIT_AUTHOR_NAME' => merge_request.author.name,
-                      'GIT_AUTHOR_EMAIL' => merge_request.author.email),
-        'commit squashed changes'
-      )
-
-      # May print a warning for ambiguous refs, but we can ignore that with
-      # `--quiet` and just take the SHA, if present. HEAD here always refers to
-      # the current HEAD commit, even if there is another ref called HEAD.
-      #
-      squash_sha = run_git_command(
-        %w(rev-parse --quiet --verify HEAD),
-        tree_path,
-        git_env,
-        'get SHA of squashed commit'
-      )
+      squash_sha = repository.squash(current_user, merge_request.id,
+        target_branch: merge_request.target_branch,
+        start_sha: merge_request.diff_start_sha,
+        end_sha: merge_request.diff_head_sha,
+        author: merge_request.author,
+        message: merge_request.title)
 
       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
-      clean_worktree
-    end
-
-    def tree_path
-      @tree_path ||= merge_request.squash_dir_path
-    end
-
-    def diff_range
-      @diff_range ||= "#{merge_request.diff_start_sha}...#{merge_request.diff_head_sha}"
-    end
-
-    def worktree_path
-      @worktree_path ||= File.join(repository.path_to_repo, 'worktrees', merge_request.id.to_s)
-    end
-
-    def clean_worktree
-      FileUtils.rm_rf(worktree_path) if File.exist?(worktree_path)
-    end
-
-    # Adding a worktree means checking out the repository. For large repos, this
-    # can be very expensive, so set up sparse checkout for the worktree to only
-    # check out the files we're interested in.
-    #
-    def configure_sparse_checkout
-      run_git_command(
-        %w(config core.sparseCheckout true),
-        repository.path_to_repo,
-        git_env,
-        'configure sparse checkout'
-      )
-
-      # Get the same diff we'll apply, excluding added files. (We can't check
-      # out files on the target branch if they don't exist yet!)
-      #
-      diff_files = run_git_command(
-        %W(diff --name-only --diff-filter=a --binary #{diff_range}),
-        repository.path_to_repo,
-        git_env,
-        'get files in diff'
-      )
-
-      # If only new files are introduced by this MR, then our sparse checkout
-      # doesn't need to have any files at all.
-      #
-      unless diff_files.empty?
-        worktree_info = File.join(worktree_path, 'info')
-
-        FileUtils.mkdir_p(worktree_info) unless File.directory?(worktree_info)
-
-        File.write(File.join(worktree_info, 'sparse-checkout'), diff_files)
-      end
-
-      run_git_command(
-        %W(checkout --detach #{merge_request.target_branch}),
-        tree_path,
-        git_env,
-        'check out target branch'
-      )
     end
   end
 end

ee/app/services/merge_requests/working_copy_base_service.rb

 module MergeRequests
   class WorkingCopyBaseService < MergeRequests::BaseService
-    GitCommandError = Class.new(StandardError)
-
-    include Gitlab::Popen
-
     attr_reader :merge_request
 
-    def git_command(command)
-      [Gitlab.config.git.bin_path] + command
-    end
-
-    def run_git_command(command, path, env, message = nil, &block)
-      run_command(git_command(command), path, env, message, &block)
-    end
-
-    def run_command(command, path, env, message = nil, &block)
-      output, status = popen(command, path, env, &block)
-
-      unless status.zero?
-        if message
-          log_error("Failed to #{message} with `#{command.join(' ')}`:")
-        else
-          log_error("`#{command.join(' ')}` failed:")
-        end
-
-        log_error(output, save_message_on_model: true)
-
-        raise GitCommandError
-      end
-
-      output.chomp
-    end
-
     def source_project
       @source_project ||= merge_request.source_project
     end
@@ -46,18 +16,6 @@ module MergeRequests
       merge_request.update(merge_error: message) if save_message_on_model
     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',
-        'GL_REPOSITORY' => Gitlab::GlRepository.gl_repository(project, false)
-      }
-    end
-
     # Don't try to print expensive instance variables.
     def inspect
       "#<#{self.class} #{merge_request.to_reference(full: true)}>"

lib/gitlab/git/repository.rb

@@ -7,7 +7,6 @@ module Gitlab
   module Git
     class Repository
       include Gitlab::Git::RepositoryMirroring
-      include Gitlab::Git::RepositoryWorktree
       include Gitlab::Git::Popen
 
       ALLOWED_OBJECT_DIRECTORIES_VARIABLES = %w[
@@ -1236,6 +1235,41 @@ module Gitlab
         fresh_worktree?(rebase_dir_path(rebase_id))
       end
 
+      def squash(user, squash_id, target_branch:, start_sha:, end_sha:, author:, message:)
+        squash_path = squash_dir_path(squash_id)
+        env = git_env_for_user(user).merge(
+          'GIT_AUTHOR_NAME' => author.name,
+          'GIT_AUTHOR_EMAIL' => author.email
+        )
+        diff_range = "#{start_sha}...#{end_sha}"
+        diff_files = run_git!(
+          %W(diff --name-only --diff-filter=a --binary #{diff_range})
+        ).chomp
+
+        with_worktree(squash_path, target_branch, sparse_checkout_files: diff_files, env: env) do
+          # Apply diff of the `diff_range` to the worktree
+          diff = run_git!(%W(diff --binary #{diff_range}))
+          run_git!(%w(apply --index), chdir: squash_path, env: env) do |stdin|
+            stdin.write(diff)
+          end
+
+          # Commit the `diff_range` diff
+          run_git!(%W(commit --no-verify --message #{message}), chdir: squash_path, env: env)
+
+          # Return the squash sha. May print a warning for ambiguous refs, but
+          # we can ignore that with `--quiet` and just take the SHA, if present.
+          # HEAD here always refers to the current HEAD commit, even if there is
+          # another ref called HEAD.
+          run_git!(
+            %w(rev-parse --quiet --verify HEAD), chdir: squash_path, env: env
+          ).chomp
+        end
+      end
+
+      def squash_in_progress?(squash_id)
+        fresh_worktree?(squash_dir_path(squash_id))
+      end
+
       def gitaly_repository
         Gitlab::GitalyClient::Util.repository(@storage, @relative_path, @gl_repository)
       end
@@ -1272,6 +1306,57 @@ module Gitlab
 
       private
 
+      def fresh_worktree?(path)
+        File.exist?(path) && !clean_stuck_worktree(path)
+      end
+
+      def with_worktree(worktree_path, branch, sparse_checkout_files: nil, env:)
+        worktree_git_path = File.join(path, 'worktrees', File.basename(worktree_path))
+        base_args = %w(worktree add --detach)
+
+        # Note that we _don't_ want to test for `.present?` here: If the caller
+        # passes an non nil empty value it means it still wants sparse checkout
+        # but just isn't interested in any file, perhaps because it wants to
+        # checkout files in by a changeset but that changeset only adds files.
+        if sparse_checkout_files
+          # Create worktree without checking out
+          run_git!(base_args + ['--no-checkout', worktree_path], env: env)
+
+          configure_sparse_checkout(worktree_git_path, sparse_checkout_files)
+
+          # After sparse checkout configuration, checkout `branch` in worktree
+          run_git!(%W(checkout --detach #{branch}), chdir: worktree_path, env: env)
+        else
+          # Create worktree and checkout `branch` in it
+          run_git!(base_args + [worktree_path, branch], env: env)
+        end
+
+        yield
+      ensure
+        FileUtils.rm_rf(worktree_path) if File.exist?(worktree_path)
+        FileUtils.rm_rf(worktree_git_path) if File.exist?(worktree_git_path)
+      end
+
+      def clean_stuck_worktree(path)
+        return false unless File.mtime(path) < 15.minutes.ago
+
+        FileUtils.rm_rf(path)
+        true
+      end
+
+      # Adding a worktree means checking out the repository. For large repos,
+      # this can be very expensive, so set up sparse checkout for the worktree
+      # to only check out the files we're interested in.
+      def configure_sparse_checkout(worktree_git_path, files)
+        run_git!(%w(config core.sparseCheckout true))
+
+        return if files.empty?
+
+        worktree_info_path = File.join(worktree_git_path, 'info')
+        FileUtils.mkdir_p(worktree_info_path)
+        File.write(File.join(worktree_info_path, 'sparse-checkout'), files)
+      end
+
       def rugged_fetch_source_branch(source_repository, source_branch, local_ref)
         with_repo_branch_commit(source_repository, source_branch) do |commit|
           if commit
@@ -1287,6 +1372,10 @@ module Gitlab
         File.join(::Gitlab.config.shared.path, 'tmp/rebase', gl_repository, id.to_s).to_s
       end
 
+      def squash_dir_path(id)
+        File.join(::Gitlab.config.shared.path, 'tmp/squash', gl_repository, id.to_s).to_s
+      end
+
       def git_env_for_user(user)
         {
           'GIT_COMMITTER_NAME' => user.name,

lib/gitlab/git/repository_worktree.rb

-module Gitlab
-  module Git
-    module RepositoryWorktree
-      def fresh_worktree?(path)
-        File.exist?(path) && !clean_stuck_worktree(path)
-      end
-
-      def with_worktree(path, target, env:)
-        run_git!(%W(worktree add --detach #{path} #{target}), env: env)
-
-        yield
-      ensure
-        FileUtils.rm_rf(path) if File.exist?(path)
-      end
-
-      def clean_stuck_worktree(path)
-        return false unless File.mtime(path) < 15.minutes.ago
-
-        FileUtils.rm_rf(path)
-        true
-      end
-    end
-  end
-end

spec/ee/spec/models/ee/merge_request_spec.rb

@@ -87,7 +87,7 @@ describe MergeRequest do
     end
 
     it 'returns false when there is no squash directory' do
-      allow(File).to receive(:exist?).with(subject.squash_dir_path).and_return(false)
+      allow(File).to receive(:exist?).and_return(false)
 
       expect(subject.squash_in_progress?).to be_falsey
     end

spec/services/merge_requests/squash_service_spec.rb

@@ -4,7 +4,11 @@ describe MergeRequests::SquashService do
   let(:service) { described_class.new(project, user, {}) }
   let(:user) { project.owner }
   let(:project) { create(:project, :repository) }
-
+  let(:repository) { project.repository.raw }
+  let(:log_error) { "Failed to squash merge request #{merge_request.to_reference(full: true)}:" }
+  let(:squash_dir_path) do
+    File.join(Gitlab.config.shared.path, 'tmp/squash', repository.gl_repository, merge_request.id.to_s)
+  end
   let(:merge_request_with_one_commit) do
     create(:merge_request,
            source_branch: 'feature', source_project: project,
@@ -32,9 +36,9 @@ describe MergeRequests::SquashService do
     end
 
     it 'cleans up the temporary directory' do
-      expect(service).to receive(:clean_dir).and_call_original
-
       service.execute(merge_request)
+
+      expect(File.exist?(squash_dir_path)).to be(false)
     end
 
     it 'does not keep the branch push event' do
@@ -78,7 +82,7 @@ describe MergeRequests::SquashService do
       end
 
       it 'does not perform any git actions' do
-        expect(service).not_to receive(:run_git_command)
+        expect(repository).not_to receive(:popen)
 
         service.execute(merge_request_with_one_commit)
       end
@@ -128,13 +132,13 @@ describe MergeRequests::SquashService do
             a_collection_starting_with([Gitlab.config.git.bin_path] + command.split)
           )
 
-          allow(service).to receive(:popen).and_return(['', 0])
-          allow(service).to receive(:popen).with(git_command, anything, anything).and_return([error, 1])
+          allow(repository).to receive(:popen).and_return(['', 0])
+          allow(repository).to receive(:popen).with(git_command, anything, anything).and_return([error, 1])
         end
 
         it 'logs the stage and output' do
-          expect(service).to receive(:log_error).with(a_string_including(stage))
-          expect(service).to receive(:log_error).with(error, save_message_on_model: true)
+          expect(service).to receive(:log_error).with(log_error)
+          expect(service).to receive(:log_error).with(error)
 
           service.execute(merge_request)
         end
@@ -145,7 +149,7 @@ describe MergeRequests::SquashService do
         end
 
         it 'cleans up the temporary directory' do
-          expect(service).to receive(:clean_dir).and_call_original
+          expect(File.exist?(squash_dir_path)).to be(false)
 
           service.execute(merge_request)
         end
@@ -173,9 +177,9 @@ describe MergeRequests::SquashService do
       end
 
       it 'cleans up the temporary directory' do
-        expect(service).to receive(:clean_dir).and_call_original
-
         service.execute(merge_request)
+
+        expect(File.exist?(squash_dir_path)).to be(false)
       end
     end
   end