Allow squashing MR into a single commit

1. Clone the repo to a temp directory.
2. Apply the MR as a patch.
3. Use `git commit -C` to copy the commit metadata from the HEAD commit
   of the source branch, and manually add the merging user as the
   committer.
4. Push back to a randomly-generated branch name.
5. Immediately delete that branch, so that it does not appear in the
   branch list, trigger CI builds, etc.
6. Return the SHA of the squashed commit we've just pushed to the
   MergeService.
7. In the MergeService, allow an arbitrary source to be used, and set
   that to either the source branch (if there is no squashing) or the
   squashed SHA.

app/models/merge_request.rb

@@ -812,6 +812,10 @@ class MergeRequest < ActiveRecord::Base
     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
@@ -820,14 +824,26 @@ class MergeRequest < ActiveRecord::Base
   end
 
   def clean_stuck_rebase
-    expiration_time = Time.now - 15.minutes
-
-    if File.new(rebase_dir_path).mtime < expiration_time
+    if File.mtime(rebase_dir_path) < 15.minutes.ago
       FileUtils.rm_rf(rebase_dir_path)
       true
     end
   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_squash
+    if File.mtime(squash_dir_path) < 15.minutes.ago
+      FileUtils.rm_rf(squash_dir_path)
+      true
+    end
+  end
+
   def diverged_commits_count
     cache = Rails.cache.read(:"merge_request_#{id}_diverged_commits")
 

app/models/repository.rb

@@ -1027,8 +1027,8 @@ class Repository
     end
   end
 
-  def merge(user, source, target_branch, merge_request, options = {})
-    our_commit = rugged.branches[target_branch].target
+  def merge(user, source, merge_request, options = {})
+    our_commit = rugged.branches[merge_request.target_branch].target
     their_commit = rugged.lookup(source)
 
     raise "Invalid merge target" if our_commit.nil?
@@ -1037,7 +1037,7 @@ class Repository
     merge_index = rugged.merge_commits(our_commit, their_commit)
     return false if merge_index.conflicts?
 
-    update_branch_with_hooks(user, target_branch) do
+    update_branch_with_hooks(user, merge_request.target_branch) do
       actual_options = options.merge(
         parents: [our_commit, their_commit],
         tree: merge_index.write_tree(rugged),

app/services/merge_requests/ff_merge_service.rb

@@ -10,7 +10,7 @@ module MergeRequests
 
     def commit
       repository.ff_merge(current_user,
-                          merge_request.diff_head_sha,
+                          source,
                           merge_request.target_branch,
                           merge_request: merge_request)
     ensure

app/services/merge_requests/merge_service.rb

@@ -6,7 +6,7 @@ module MergeRequests
   # Executed when you do merge via GitLab UI
   #
   class MergeService < MergeRequests::BaseService
-    attr_reader :merge_request
+    attr_reader :merge_request, :source
 
     def execute(merge_request)
       if project.merge_requests_ff_only_enabled && !self.is_a?(FfMergeService)
@@ -24,6 +24,18 @@ module MergeRequests
         return error(message)
       end
 
+      if merge_request.squash
+        squash_result = SquashService.new(project, current_user, params).execute(merge_request)
+
+        if squash_result[:success]
+          @source = squash_result[:squash_oid]
+        else
+          log_merge_error('Squashing this merge request failed', true)
+        end
+      else
+        @source = merge_request.diff_head_sha
+      end
+
       merge_request.in_locked_state do
         if commit
           after_merge
@@ -64,11 +76,7 @@ module MergeRequests
         committer: committer
       }
 
-      commit_id = repository.merge(current_user,
-                                   merge_request.diff_head_sha,
-                                   merge_request.target_branch,
-                                   merge_request,
-                                   options)
+      commit_id = repository.merge(current_user, source, merge_request, options)
 
       if commit_id
         merge_request.update(merge_commit_sha: commit_id)

app/services/merge_requests/squash_service.rb

+require 'securerandom'
+
+module MergeRequests
+  class SquashService < MergeRequests::BaseService
+    include Gitlab::Popen
+
+    attr_reader :merge_request, :repository, :rugged
+
+    def execute(merge_request)
+      @merge_request = merge_request
+      @repository = merge_request.target_project.repository
+      @rugged = repository.rugged
+
+      squash || error('Failed to squash. Should be done manually')
+    end
+
+    def squash
+      # 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 = SecureRandom.uuid
+
+      if merge_request.squash_in_progress?
+        log('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}),
+        nil,
+        git_env
+      )
+
+      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|
+        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}),
+        tree_path,
+        git_env.merge('GIT_COMMITTER_NAME' => current_user.name, 'GIT_COMMITTER_EMAIL' => current_user.email)
+      )
+
+      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)
+
+      unless status.zero?
+        log('Failed to push squashed branch:')
+        log(output)
+        return false
+      end
+
+      repository.rm_branch(current_user, temp_branch, skip_event: true)
+
+      success(squash_oid: target)
+    rescue => ex
+      log("Failed to squash merge request #{project.path_with_namespace}#{merge_request.to_reference}:")
+      log(ex.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
+  end
+end

changelogs/unreleased-ee/squash.yml

+---
+title: Allow squashing merge requests into a single commit
+merge_request:
+author:

spec/lib/gitlab/diff/position_tracer_spec.rb

@@ -1641,11 +1641,7 @@ describe Gitlab::Diff::PositionTracer, lib: true do
 
         merge_request = create(:merge_request, source_branch: second_create_file_commit.sha, target_branch: branch_name, source_project: project)
 
-        repository.merge(current_user,
-                         merge_request.diff_head_sha,
-                         merge_request.target_branch,
-                         merge_request,
-                         options)
+        repository.merge(current_user, merge_request.diff_head_sha, merge_request, options)
 
         project.commit(branch_name)
       end

spec/models/merge_request_spec.rb

@@ -788,36 +788,67 @@ describe MergeRequest, models: true do
   end
 
   describe '#rebase_in_progress?' do
-    it 'returns true' do
+    it 'returns true when there is a current rebase directory' do
       allow(File).to receive(:exist?).and_return(true)
-      allow(File).to receive(:new).and_return(double(:file, mtime: Time.now))
+      allow(File).to receive(:mtime).and_return(Time.now)
 
       expect(subject.rebase_in_progress?).to be_truthy
     end
 
-    it 'returns false' do
+    it 'returns false when there is no rebase directory' do
       allow(File).to receive(:exist?).with(subject.rebase_dir_path).and_return(false)
 
       expect(subject.rebase_in_progress?).to be_falsey
     end
 
-    it 'returns false if temporary file exists by is expired' do
+    it 'returns false when the rebase directory has expired' do
       allow(File).to receive(:exist?).and_return(true)
-      allow(File).to receive(:new).and_return(double(:file, mtime: Time.now - 2.hours))
+      allow(File).to receive(:mtime).and_return(20.minutes.ago)
 
       expect(subject.rebase_in_progress?).to be_falsey
     end
 
-    it 'returns false if source_project is removed' do
+    it 'returns false when the source project has been removed' do
       allow(subject).to receive(:source_project).and_return(nil)
       allow(File).to receive(:exist?).and_return(true)
-      allow(File).to receive(:new).and_return(double(:file, mtime: Time.now))
+      allow(File).to receive(:mtime).and_return(Time.now)
 
       expect(File).not_to have_received(:exist?)
       expect(subject.rebase_in_progress?).to be_falsey
     end
   end
 
+  describe '#squash_in_progress?' do
+    it 'returns true when there is a current squash directory' do
+      allow(File).to receive(:exist?).and_return(true)
+      allow(File).to receive(:mtime).and_return(Time.now)
+
+      expect(subject.squash_in_progress?).to be_truthy
+    end
+
+    it 'returns false when there is no squash directory' do
+      allow(File).to receive(:exist?).with(subject.squash_dir_path).and_return(false)
+
+      expect(subject.squash_in_progress?).to be_falsey
+    end
+
+    it 'returns false when the squash directory has expired' do
+      allow(File).to receive(:exist?).and_return(true)
+      allow(File).to receive(:mtime).and_return(20.minutes.ago)
+
+      expect(subject.squash_in_progress?).to be_falsey
+    end
+
+    it 'returns false when the source project has been removed' do
+      allow(subject).to receive(:source_project).and_return(nil)
+      allow(File).to receive(:exist?).and_return(true)
+      allow(File).to receive(:mtime).and_return(Time.now)
+
+      expect(File).not_to have_received(:exist?)
+      expect(subject.squash_in_progress?).to be_falsey
+    end
+  end
+
   describe '#commits_sha' do
     before do
       allow(subject.merge_request_diff).to receive(:commits_sha).

spec/models/repository_spec.rb

@@ -18,7 +18,6 @@ describe Repository, models: true do
 
     merge_commit_id = repository.merge(user,
                                        merge_request.diff_head_sha,
-                                       merge_request.target_branch,
                                        merge_request,
                                        commit_options)
 
@@ -1018,7 +1017,6 @@ describe Repository, models: true do
 
       merge_commit_id = repository.merge(user,
                                          merge_request.diff_head_sha,
-                                         merge_request.target_branch,
                                          merge_request,
                                          commit_options)
 

spec/services/merge_requests/refresh_service_spec.rb

@@ -101,12 +101,7 @@ describe MergeRequests::RefreshService, services: true do
         # Merge master -> feature branch
         author = { email: 'test@gitlab.com', time: Time.now, name: "Me" }
         commit_options = { message: 'Test message', committer: author, author: author }
-
-        @project.repository.merge(@user,
-                                  @merge_request.diff_head_sha,
-                                  @merge_request.target_branch,
-                                  @merge_request,
-                                  commit_options)
+        @project.repository.merge(@user, @merge_request.diff_head_sha, @merge_request, commit_options)
 
         commit = @project.repository.commit('feature')
         service.new(@project, @user).execute(@oldrev, commit.id, 'refs/heads/feature')

spec/services/merge_requests/squash_service_spec.rb

+require 'spec_helper'
+
+describe MergeRequests::SquashService do
+  let(:service) { described_class.new(project, user, {}) }
+  let(:user) { create(:user) }
+  let(:merge_request) { create(:merge_request, source_branch: 'feature_conflict', target_branch: 'master') }
+  let(:project) { merge_request.project }
+
+  before do
+    project.team << [user, :master]
+  end
+
+  describe '#execute' 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}/))
+      end
+
+      it 'cleans up the temporary directory' do
+        expect(service).to receive(:clean_dir).and_call_original
+
+        service.execute(merge_request)
+      end
+
+      context 'the squashed commit' do
+        let(:squashed_commit) do
+          squash_oid = service.execute(merge_request)[:squash_oid]
+
+          project.repository.commit(squash_oid)
+        end
+
+        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(squashed_commit.author_name).to eq(diff_head_commit.author_name)
+          expect(squashed_commit.author_email).to eq(diff_head_commit.author_email)
+          expect(squashed_commit.message).to eq(diff_head_commit.message)
+        end
+
+        it 'sets the current user as the committer' do
+          expect(squashed_commit.committer_name).to eq(user.name)
+          expect(squashed_commit.committer_email).to eq(user.email)
+        end
+      end
+    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']
+    }
+
+    stages.each do |stage, command|
+      context "when the #{stage} stage fails" do
+        let(:error) { 'A test error' }
+
+        before do
+          allow(service).to receive(:popen).and_return(['', 0])
+
+          allow(service).to receive(:popen).with(a_collection_starting_with(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)
+
+          service.execute(merge_request)
+        end
+
+        it 'returns an error' do
+          expect(service.execute(merge_request)).to match(status: :error,
+                                                          message: a_string_including('squash'))
+        end
+
+        it 'cleans up the temporary directory' do
+          expect(service).to receive(:clean_dir).and_call_original
+
+          service.execute(merge_request)
+        end
+      end
+    end
+
+    context 'when any other exception is thrown' do
+      let(:error) { 'A test error' }
+
+      before do
+        allow(SecureRandom).to receive(:uuid).and_raise(error)
+      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)
+
+        service.execute(merge_request)
+      end
+
+      it 'returns an error' do
+        expect(service.execute(merge_request)).to match(status: :error,
+                                                        message: a_string_including('squash'))
+      end
+
+      it 'cleans up the temporary directory' do
+        expect(service).to receive(:clean_dir).and_call_original
+
+        service.execute(merge_request)
+      end
+    end
+  end
+end