Use `git worktree` for squashing

This avoids the need to:

1. Perform a full clone.
2. Deal with protected branches (because there is no push).
3. Deal with repository push events (because there is no push).

app/services/merge_requests/squash_service.rb

@@ -17,27 +17,16 @@ module MergeRequests
         return success(squash_sha: merge_request.diff_head_sha)
       end
 
-      # We will push to this ref, then immediately delete the ref. This is
-      # because we don't want a new branch to appear in the UI - we just want
-      # the commit to be present in the repo.
-      #
-      # Squashing would ideally be possible by applying a patch to a bare repo
-      # and creating a commit object, in which case wouldn't need this dance.
-      #
-      temp_branch = "temporary-gitlab-squash-branch-#{SecureRandom.uuid}"
-
       if merge_request.squash_in_progress?
         log_error('Squash task canceled: Another squash is already in progress')
         return false
       end
 
-      protected_branch = create_protected_branch_exception(temp_branch)
-
       run_git_command(
-        %W(clone -b #{merge_request.target_branch} -- #{repository.path_to_repo} #{tree_path}),
-        nil,
+        %W(worktree add #{tree_path} #{merge_request.target_branch} --detach),
+        repository.path_to_repo,
         git_env,
-        'clone repository for squash'
+        'add worktree for squash'
       )
 
       run_git_command(%w(apply --cached), tree_path, git_env, 'apply patch') do |stdin|
@@ -55,18 +44,9 @@ module MergeRequests
         %w(rev-parse HEAD),
         tree_path,
         git_env,
-        "get SHA of squashed branch #{temp_branch}"
-      )
-
-      run_git_command(
-        %W(push -f origin HEAD:#{temp_branch}),
-        tree_path,
-        git_env,
-        'push squashed branch'
+        'get SHA of squashed commit'
       )
 
-      remove_branch(temp_branch, squash_sha)
-
       success(squash_sha: squash_sha)
     rescue GitCommandError
       false
@@ -75,8 +55,6 @@ module MergeRequests
       log_error(e.message)
       false
     ensure
-      protected_branch.destroy if protected_branch
-
       clean_dir
     end
 
@@ -87,53 +65,5 @@ module MergeRequests
     def merge_request_to_patch
       @merge_request_to_patch ||= rugged.diff(merge_request.diff_base_sha, merge_request.diff_head_sha).patch
     end
-
-    def create_protected_branch_exception(temp_branch)
-      user_access = Gitlab::UserAccess.new(current_user, project: target_project)
-
-      return if user_access.can_push_to_branch?(temp_branch)
-
-      protected_branch_params = {
-        name: temp_branch,
-        push_access_levels_attributes: [{ user_id: current_user.id }],
-        merge_access_levels_attributes: [{ user_id: current_user.id }]
-      }
-
-      create_service = ProtectedBranches::CreateService.new(target_project, current_user, protected_branch_params)
-      protected_branch = create_service.execute(skip_authorization: true)
-
-      unless protected_branch.persisted?
-        raise "Failed to create protected branch override #{ref}"
-      end
-
-      protected_branch
-    end
-
-    # If the branch is protected, no-one can remove it, so we have to skip hooks
-    # in order to remove it. We also don't want a branch creation event left
-    # hanging around, so we look in the user's last 10 push events for this
-    # repository and find it from those.
-    #
-    def remove_branch(branch, rev)
-      full_ref = "#{Gitlab::Git::BRANCH_REF_PREFIX}#{branch}"
-      blank_sha = Gitlab::Git::BLANK_SHA
-      events = Event.code_push
-                 .where(project: target_project, author: current_user)
-                 .order('created_at DESC')
-                 .limit(10)
-
-      repository.before_remove_branch
-      repository.update_ref!(full_ref, blank_sha, rev)
-      repository.after_remove_branch
-
-      event = events.find do |event|
-        event.data[:before] == blank_sha &&
-          event.data[:after] == rev &&
-          event.data[:ref] == full_ref &&
-          event.data[:total_commits_count] == 1
-      end
-
-      event.destroy if event
-    end
   end
 end

app/services/protected_branches/create_service.rb

@@ -2,10 +2,8 @@ module ProtectedBranches
   class CreateService < BaseService
     attr_reader :protected_branch
 
-    def execute(skip_authorization: false)
-      unless skip_authorization || can?(current_user, :admin_project, project)
-        raise Gitlab::Access::AccessDeniedError
-      end
+    def execute
+      raise Gitlab::Access::AccessDeniedError unless can?(current_user, :admin_project, project)
 
       project.protected_branches.create(params)
     end

spec/services/merge_requests/squash_service_spec.rb

@@ -22,31 +22,6 @@ describe MergeRequests::SquashService do
   end
 
   shared_examples 'the squashed commit' do
-    context 'the squashed commit' do
-      let(:squash_sha) { service.execute(merge_request)[:squash_sha] }
-      let(:squash_commit) { project.repository.commit(squash_sha) }
-
-      it 'copies the author info and message from the last commit in the source branch' do
-        diff_head_commit = merge_request.diff_head_commit
-
-        expect(squash_commit.author_name).to eq(diff_head_commit.author_name)
-        expect(squash_commit.author_email).to eq(diff_head_commit.author_email)
-        expect(squash_commit.message).to eq(diff_head_commit.message)
-      end
-
-      it 'sets the current user as the committer' do
-        expect(squash_commit.committer_name).to eq(user.name.chomp('.'))
-        expect(squash_commit.committer_email).to eq(user.email)
-      end
-
-      it 'has the same diff as the merge request' do
-        rugged = project.repository.rugged
-        mr_diff = rugged.diff(merge_request.diff_base_sha, merge_request.diff_head_sha)
-        squash_diff = rugged.diff(merge_request.diff_start_sha, squash_sha)
-
-        expect(squash_diff.patch).to eq(mr_diff.patch)
-      end
-    end
   end
 
   describe '#execute' do
@@ -64,61 +39,6 @@ describe MergeRequests::SquashService do
       end
     end
 
-    # We don't run hooks in tests, so fake this case. This does involve
-    # duplicating logic from the service itself, but that is worth it to test
-    # this case.
-    #
-    context 'when the chosen branch name is protected with a wildcard' do
-      let!(:protected_branch) { create(:protected_branch, :no_one_can_push, name: '*', project: project) }
-
-      before do
-        user_access = Gitlab::UserAccess.new(user, project: project)
-
-        # If the branch is protected, then nobody can remove it, so we need to
-        # ensure we aren't executing hooks.
-        allow(GitHooksService).to receive(:new).and_raise(GitHooksService::PreReceiveError)
-
-        allow(service).to receive(:popen).and_call_original
-
-        allow(service).to receive(:popen).with(git_command('push'), anything, anything).and_wrap_original do |meth, cmd, *args|
-          ref = cmd.last.split(':').last
-
-          if user_access.can_push_to_branch?(ref)
-            meth.call(cmd, *args)
-          else
-            ['You are not allowed to push code to protected branches on this project', 1]
-          end
-        end
-      end
-
-      it 'allows the user to push to that protected branch' do
-        branch_params = a_hash_including(name: a_string_starting_with('temporary-gitlab-squash-branch'))
-
-        expect(ProtectedBranches::CreateService)
-          .to receive(:new).with(project, user, branch_params).and_call_original
-
-        service.execute(merge_request)
-      end
-
-      it 'returns the squashed commit SHA' do
-        result = service.execute(merge_request)
-
-        expect(result).to match(status: :success, squash_sha: a_string_matching(/\h{40}/))
-      end
-
-      it 'cleans up the temporary directory and the protected branch' do
-        expect(service).to receive(:clean_dir).and_call_original
-        expect_any_instance_of(ProtectedBranch).to receive(:destroy).and_call_original
-
-        expect { service.execute(merge_request) }
-          .not_to change { project.protected_branches.count }
-
-        expect(protected_branch).to be_persisted
-      end
-
-      include_examples 'the squashed commit'
-    end
-
     context 'when the squash succeeds' do
       it 'returns the squashed commit SHA' do
         result = service.execute(merge_request)
@@ -137,15 +57,39 @@ describe MergeRequests::SquashService do
         expect { service.execute(merge_request) }.not_to change { Event.count }
       end
 
-      include_examples 'the squashed commit'
+      context 'the squashed commit' do
+        let(:squash_sha) { service.execute(merge_request)[:squash_sha] }
+        let(:squash_commit) { project.repository.commit(squash_sha) }
+
+        it 'copies the author info and message from the last commit in the source branch' do
+          diff_head_commit = merge_request.diff_head_commit
+
+          expect(squash_commit.author_name).to eq(diff_head_commit.author_name)
+          expect(squash_commit.author_email).to eq(diff_head_commit.author_email)
+          expect(squash_commit.message).to eq(diff_head_commit.message)
+        end
+
+        it 'sets the current user as the committer' do
+          expect(squash_commit.committer_name).to eq(user.name.chomp('.'))
+          expect(squash_commit.committer_email).to eq(user.email)
+        end
+
+        it 'has the same diff as the merge request, but a different SHA' do
+          rugged = project.repository.rugged
+          mr_diff = rugged.diff(merge_request.diff_base_sha, merge_request.diff_head_sha)
+          squash_diff = rugged.diff(merge_request.diff_start_sha, squash_sha)
+
+          expect(squash_diff.patch).to eq(mr_diff.patch)
+          expect(squash_commit.sha).not_to eq(merge_request.diff_head_sha)
+        end
+      end
     end
 
     stages = {
-      'clone repository' => 'clone',
+      'add worktree for squash' => 'worktree',
       'apply patch' => 'apply',
       'commit squashed changes' => 'commit',
-      'get SHA of squashed branch' => 'rev-parse',
-      'push squashed branch' => 'push'
+      'get SHA of squashed commit' => 'rev-parse'
     }
 
     stages.each do |stage, command|
@@ -181,7 +125,7 @@ describe MergeRequests::SquashService do
       let(:error) { 'A test error' }
 
       before do
-        allow(SecureRandom).to receive(:uuid).and_raise(error)
+        allow(merge_request).to receive(:commits_count).and_raise(error)
       end
 
       it 'logs the MR reference and exception' do