Merge branch 'squash-default' into 'master'

Squash default

See merge request !1272

app/services/merge_requests/squash_service.rb

@@ -23,13 +23,15 @@ module MergeRequests
       end
 
       run_git_command(
-        %W(worktree add #{tree_path} #{merge_request.target_branch} --detach),
+        %W(worktree add --no-checkout --detach #{tree_path}),
         repository.path_to_repo,
         git_env,
         'add worktree for squash'
       )
 
-      diff = git_command(%W(diff --binary #{merge_request.diff_start_sha}...#{merge_request.diff_head_sha}))
+      configure_sparse_checkout
+
+      diff = git_command(%W(diff --binary #{diff_range}))
       apply = git_command(%w(apply --index))
 
       run_command(
@@ -40,9 +42,12 @@ module MergeRequests
       )
 
       run_git_command(
-        %W(commit -C #{merge_request.diff_head_sha}),
+        %W(commit --no-verify -m #{merge_request.title}),
         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,
+                      'GIT_AUTHOR_NAME' => merge_request.author.name,
+                      'GIT_AUTHOR_EMAIL' => merge_request.author.email),
         'commit squashed changes'
       )
 
@@ -62,10 +67,64 @@ module MergeRequests
       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

app/services/merge_requests/working_copy_base_service.rb

@@ -41,7 +41,7 @@ module MergeRequests
     end
 
     def log_error(message)
-      Gitlab::GitLogger.error(message)
+      Gitlab::GitLogger.error("#{self.class.name} error (#{merge_request.to_reference(full: true)}): #{message}")
     end
 
     def clean_dir

doc/user/project/merge_requests/squash_and_merge.md

@@ -35,8 +35,8 @@ This can then be overridden at the time of accepting the merge request:
 
 The squashed commit has the following metadata:
 
-* Message: taken from the last commit in the source branch.
-* Author: taken from the last commit in the source branch.
+* Message: the title of the merge request.
+* Author: the author of the merge request.
 * Committer: the user who initiated the squash.
 
 ## Squashing and [fast-forward merge][ff-merge]

spec/features/merge_requests/squash_spec.rb

@@ -12,11 +12,10 @@ feature 'Squashing merge requests', js: true, feature: true do
   shared_examples 'squash' do
     it 'squashes the commits into a single commit, and adds a merge commit' do
       latest_master_commits = project.repository.commits_between(original_head.sha, 'master').map(&:raw)
-      last_mr_commit = project.repository.commit(source_branch)
 
       squash_commit = an_object_having_attributes(sha: a_string_matching(/\h{40}/),
-                                                  message: "#{last_mr_commit.message}\n",
-                                                  author_name: last_mr_commit.author_name,
+                                                  message: "Csv\n",
+                                                  author_name: user.name,
                                                   committer_name: user.name)
 
       merge_commit = an_object_having_attributes(sha: a_string_matching(/\h{40}/),

spec/services/merge_requests/squash_service_spec.rb

@@ -5,83 +5,102 @@ describe MergeRequests::SquashService do
   let(:user) { project.owner }
   let(:project) { create(:project) }
 
-  let(:merge_request) do
+  let(:merge_request_with_one_commit) do
+    create(:merge_request,
+           source_branch: 'feature.custom-highlighting', source_project: project,
+           target_branch: 'master', target_project: project)
+  end
+
+  let(:merge_request_with_only_new_files) do
     create(:merge_request,
            source_branch: 'video', source_project: project,
            target_branch: 'master', target_project: project)
   end
 
-  let(:merge_request_with_one_commit) do
+  let(:merge_request_with_large_files) do
     create(:merge_request,
-           source_branch: 'feature.custom-highlighting', source_project: project,
+           source_branch: 'squash-large-files', source_project: project,
            target_branch: 'master', target_project: project)
   end
 
-  describe '#execute' do
-    context 'when there is only one commit in the merge request' do
-      it 'returns that commit SHA' do
-        result = service.execute(merge_request_with_one_commit)
+  shared_examples 'the squash succeeds' do
+    it 'returns the squashed commit SHA' do
+      result = service.execute(merge_request)
 
-        expect(result).to match(status: :success, squash_sha: merge_request_with_one_commit.diff_head_sha)
-      end
+      expect(result).to match(status: :success, squash_sha: a_string_matching(/\h{40}/))
+      expect(result[:squash_sha]).not_to eq(merge_request.diff_head_sha)
+    end
 
-      it 'does not perform any git actions' do
-        expect(service).not_to receive(:run_git_command)
+    it 'cleans up the temporary directory' do
+      expect(service).to receive(:clean_dir).and_call_original
 
-        service.execute(merge_request_with_one_commit)
-      end
+      service.execute(merge_request)
     end
 
-    context 'when the squash succeeds' do
-      it 'returns the squashed commit SHA' do
-        result = service.execute(merge_request)
+    it 'does not keep the branch push event' do
+      expect { service.execute(merge_request) }.not_to change { Event.count }
+    end
 
-        expect(result).to match(status: :success, squash_sha: a_string_matching(/\h{40}/))
-        expect(result[:squash_sha]).not_to eq(merge_request.diff_head_sha)
-      end
+    context 'the squashed commit' do
+      let(:squash_sha) { service.execute(merge_request)[:squash_sha] }
+      let(:squash_commit) { project.repository.commit(squash_sha) }
 
-      it 'cleans up the temporary directory' do
-        expect(service).to receive(:clean_dir).and_call_original
+      it 'copies the author info and message from the merge request' do
+        expect(squash_commit.author_name).to eq(merge_request.author.name)
+        expect(squash_commit.author_email).to eq(merge_request.author.email)
 
-        service.execute(merge_request)
+        # Commit messages have a trailing newline, but titles don't.
+        expect(squash_commit.message.chomp).to eq(merge_request.title)
       end
 
-      it 'does not keep the branch push event' do
-        expect { service.execute(merge_request) }.not_to change { Event.count }
+      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
 
-      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
+      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_commit.author_name).to eq(diff_head_commit.author_name)
-          expect(squash_commit.author_email).to eq(diff_head_commit.author_email)
+        expect(squash_diff.patch.length).to eq(mr_diff.patch.length)
+        expect(squash_commit.sha).not_to eq(merge_request.diff_head_sha)
+      end
+    end
+  end
 
-          # The commit message on the 'real' commit doesn't have a trailing newline
-          expect(squash_commit.message.chomp).to eq(diff_head_commit.message)
-        end
+  describe '#execute' do
+    context 'when there is only one commit in the merge request' do
+      it 'returns that commit SHA' do
+        result = service.execute(merge_request_with_one_commit)
 
-        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
+        expect(result).to match(status: :success, squash_sha: merge_request_with_one_commit.diff_head_sha)
+      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)
+      it 'does not perform any git actions' do
+        expect(service).not_to receive(:run_git_command)
 
-          expect(squash_diff.patch).to eq(mr_diff.patch)
-          expect(squash_commit.sha).not_to eq(merge_request.diff_head_sha)
-        end
+        service.execute(merge_request_with_one_commit)
       end
     end
 
+    context 'when squashing only new files' do
+      let(:merge_request) { merge_request_with_only_new_files }
+
+      include_examples 'the squash succeeds'
+    end
+
+    context 'when squashing with files too large to display' do
+      let(:merge_request) { merge_request_with_large_files }
+
+      include_examples 'the squash succeeds'
+    end
+
     stages = {
       'add worktree for squash' => 'worktree',
+      'configure sparse checkout' => 'config',
+      'get files in diff' => 'diff --name-only',
+      'check out target branch' => 'checkout',
       'apply patch' => 'diff --binary',
       'commit squashed changes' => 'commit',
       'get SHA of squashed commit' => 'rev-parse'
@@ -89,13 +108,14 @@ describe MergeRequests::SquashService do
 
     stages.each do |stage, command|
       context "when the #{stage} stage fails" do
+        let(:merge_request) { merge_request_with_only_new_files }
         let(:error) { 'A test error' }
 
         before do
           git_command = a_collection_containing_exactly(
             a_string_starting_with("#{Gitlab.config.git.bin_path} #{command}")
           ).or(
-            a_collection_starting_with([Gitlab.config.git.bin_path, command])
+            a_collection_starting_with([Gitlab.config.git.bin_path] + command.split)
           )
 
           allow(service).to receive(:popen).and_return(['', 0])
@@ -123,6 +143,7 @@ describe MergeRequests::SquashService do
     end
 
     context 'when any other exception is thrown' do
+      let(:merge_request) { merge_request_with_only_new_files }
       let(:error) { 'A test error' }
 
       before do

spec/support/test_env.rb

@@ -38,7 +38,8 @@ module TestEnv
     'conflict-too-large'                 => '39fa04f',
     'deleted-image-test'                 => '6c17798',
     'wip'                                => 'b9238ee',
-    'csv'                                => '3dd0896'
+    'csv'                                => '3dd0896',
+    'squash-large-files'                 => '54cec52'
   }
 
   # gitlab-test-fork is a fork of gitlab-fork, but we don't necessarily