Merge branch 'gitaly-worktree-refactor' into 'master'

Move git operations for squash and rebase into Gitlab::Git

Closes gitaly#761

See merge request gitlab-org/gitlab-ee!3543

ee/app/models/ee/merge_request.rb

@@ -16,40 +16,18 @@ module EE
       delegate :sha, to: :base_pipeline, prefix: :base_pipeline, allow_nil: true
     end
 
-    def rebase_dir_path
-      File.join(::Gitlab.config.shared.path, 'tmp/rebase', source_project.id.to_s, id.to_s).to_s
-    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
 
-      File.exist?(rebase_dir_path) && !clean_stuck_rebase
+      source_project.repository.rebase_in_progress?(id)
     end
 
     def squash_in_progress?
       # The source project can be deleted
       return false unless source_project
 
-      File.exist?(squash_dir_path) && !clean_stuck_squash
-    end
-
-    def clean_stuck_rebase
-      if File.mtime(rebase_dir_path) < 15.minutes.ago
-        FileUtils.rm_rf(rebase_dir_path)
-        true
-      end
-    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/models/ee/repository.rb

@@ -38,5 +38,20 @@ module EE
     def delete_remote_branches(remote, branches)
       gitlab_shell.delete_remote_branches(repository_storage_path, disk_path, remote, branches)
     end
+
+    def rebase(user, merge_request)
+      raw.rebase(user, merge_request.id, branch: merge_request.source_branch,
+                                         branch_sha: merge_request.source_branch_sha,
+                                         remote_repository: merge_request.target_project.repository.raw,
+                                         remote_branch: merge_request.target_branch)
+    end
+
+    def squash(user, merge_request)
+      raw.squash(user, merge_request.id, 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)
+    end
   end
 end

ee/app/services/merge_requests/rebase_service.rb

@@ -16,48 +16,15 @@ module MergeRequests
         return false
       end
 
-      run_git_command(
-        %W(worktree add --detach #{tree_path} #{merge_request.source_branch}),
-        repository.path_to_repo,
-        git_env,
-        'add worktree for rebase'
-      )
-
-      run_git_command(
-        %W(pull --rebase #{target_project.repository.path_to_repo} #{merge_request.target_branch}),
-        tree_path,
-        git_env.merge('GIT_COMMITTER_NAME' => current_user.name,
-                      'GIT_COMMITTER_EMAIL' => current_user.email),
-        'rebase branch'
-      )
-
-      rebase_sha = run_git_command(
-        %w(rev-parse HEAD),
-        tree_path,
-        git_env,
-        'get SHA of rebased branch'
-      )
-
-      Gitlab::Git::OperationService.new(current_user, project.repository.raw_repository)
-        .update_branch(merge_request.source_branch, rebase_sha, merge_request.source_branch_sha)
+      rebase_sha = repository.rebase(current_user, merge_request)
 
       merge_request.update_attributes(rebase_commit_sha: rebase_sha)
 
       true
-    rescue GitCommandError
-      false
     rescue => e
       log_error('Failed to rebase branch:')
       log_error(e.message, save_message_on_model: true)
       false
-    ensure
-      clean_dir
-    end
-
-    private
-
-    def tree_path
-      @tree_path ||= merge_request.rebase_dir_path
     end
   end
 end

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,13 @@ 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)
 
       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)}>"

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

@@ -51,7 +51,7 @@ describe MergeRequest do
     end
 
     it 'returns false when there is no rebase directory' do
-      allow(File).to receive(:exist?).with(subject.rebase_dir_path).and_return(false)
+      allow(File).to receive(:exist?).and_return(false)
 
       expect(subject.rebase_in_progress?).to be_falsey
     end
@@ -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/rebase_service_spec.rb

@@ -10,6 +10,7 @@ describe MergeRequests::RebaseService do
            target_branch: 'master')
   end
   let(:project) { merge_request.project }
+  let(:repository) { project.repository.raw }
 
   subject(:service) { described_class.new(project, user, {}) }
 
@@ -37,7 +38,7 @@ describe MergeRequests::RebaseService do
 
     context 'when unexpected error occurs' do
       before do
-        allow(service).to receive(:run_git_command).and_raise('Something went wrong')
+        allow(repository).to receive(:run_git!).and_raise('Something went wrong')
       end
 
       it 'saves the error message' do
@@ -54,7 +55,7 @@ describe MergeRequests::RebaseService do
 
     context 'with git command failure' do
       before do
-        allow(service).to receive(:popen).and_return(['Something went wrong', 1])
+        allow(repository).to receive(:run_git!).and_raise(Gitlab::Git::Repository::GitError, 'Something went wrong')
       end
 
       it 'saves the error message' do
@@ -96,12 +97,9 @@ describe MergeRequests::RebaseService do
 
       context 'git commands' do
         it 'sets GL_REPOSITORY env variable when calling git commands' do
-          expect_any_instance_of(described_class)
-            .to receive(:run_git_command).exactly(3).with(
-              anything,
-              anything,
-              hash_including('GL_REPOSITORY'),
-              anything)
+          expect(repository).to receive(:popen).exactly(3)
+            .with(anything, anything, hash_including('GL_REPOSITORY'))
+            .and_return(['', 0])
 
           service.execute(merge_request)
         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