Update squash_commit_sha only on successful merge

With https://gitlab.com/gitlab-org/gitlab/merge_requests/17052 we
started saving and exposing squash commit sha for squash & fast-forward
merge requests but we do it even if the merge fails. This commit fixes
this and add tests to ensure we do update the column only if the merge
was successful.

Related to https://gitlab.com/gitlab-org/gitlab/issues/35709.

app/services/merge_requests/ff_merge_service.rb

@@ -11,19 +11,21 @@ module MergeRequests
     private
 
     def commit
-      repository.ff_merge(current_user,
-                          source,
-                          merge_request.target_branch,
-                          merge_request: merge_request)
+      ff_merge = repository.ff_merge(current_user,
+                                     source,
+                                     merge_request.target_branch,
+                                     merge_request: merge_request)
+
+      if merge_request.squash
+        merge_request.update_column(:squash_commit_sha, merge_request.in_progress_merge_commit_sha)
+      end
+
+      ff_merge
     rescue Gitlab::Git::PreReceiveError => e
       raise MergeError, e.message
     rescue StandardError => e
       raise MergeError, "Something went wrong during merge: #{e.message}"
     ensure
-      if merge_request.squash
-        merge_request.update_column(:squash_commit_sha, merge_request.in_progress_merge_commit_sha)
-      end
-
       merge_request.update(in_progress_merge_commit_sha: nil)
     end
   end

changelogs/unreleased/35709-update-squash-commit-sha-only-on-successful-merge.yml

+---
+title: Update squash_commit_sha only on successful merge
+merge_request: 19688
+author:
+type: fixed

spec/services/merge_requests/ff_merge_service_spec.rb

@@ -23,33 +23,63 @@ describe MergeRequests::FfMergeService do
     context 'valid params' do
       let(:service) { described_class.new(project, user, {}) }
 
-      before do
-        allow(service).to receive(:execute_hooks)
-
+      def execute_ff_merge
         perform_enqueued_jobs do
           service.execute(merge_request)
         end
       end
 
+      before do
+        allow(service).to receive(:execute_hooks)
+      end
+
       it "does not create merge commit" do
+        execute_ff_merge
+
         source_branch_sha = merge_request.source_project.repository.commit(merge_request.source_branch).sha
         target_branch_sha = merge_request.target_project.repository.commit(merge_request.target_branch).sha
+
         expect(source_branch_sha).to eq(target_branch_sha)
       end
 
-      it { expect(merge_request).to be_valid }
-      it { expect(merge_request).to be_merged }
+      it 'keeps the merge request valid' do
+        expect { execute_ff_merge }
+          .not_to change { merge_request.valid? }
+      end
+
+      it 'updates the merge request to merged' do
+        expect { execute_ff_merge }
+          .to change { merge_request.merged? }
+          .from(false)
+          .to(true)
+      end
 
       it 'sends email to user2 about merge of new merge_request' do
+        execute_ff_merge
+
         email = ActionMailer::Base.deliveries.last
         expect(email.to.first).to eq(user2.email)
         expect(email.subject).to include(merge_request.title)
       end
 
       it 'creates system note about merge_request merge' do
+        execute_ff_merge
+
         note = merge_request.notes.last
         expect(note.note).to include 'merged'
       end
+
+      it 'does not update squash_commit_sha if it is not a squash' do
+        expect { execute_ff_merge }.not_to change { merge_request.squash_commit_sha }
+      end
+
+      it 'updates squash_commit_sha if it is a squash' do
+        merge_request.update!(squash: true)
+
+        expect { execute_ff_merge }
+          .to change { merge_request.squash_commit_sha }
+          .from(nil)
+      end
     end
 
     context "error handling" do
@@ -82,6 +112,16 @@ describe MergeRequests::FfMergeService do
         expect(merge_request.merge_error).to include(error_message)
         expect(Rails.logger).to have_received(:error).with(a_string_matching(error_message))
       end
+
+      it 'does not update squash_commit_sha if squash merge is not successful' do
+        merge_request.update!(squash: true)
+
+        expect(project.repository.raw).to receive(:ff_merge) do
+          raise 'Merge error'
+        end
+
+        expect { service.execute(merge_request) }.not_to change { merge_request.squash_commit_sha }
+      end
     end
   end
 end