Merge branch 'fix-commits-manual-merge' into 'master'

Fix bug where manually merged branches in a MR would end up with an empty diff

This bug manifested in 8.1 with the refactoring of `RefreshService`. Here's what happens:

1. User create a new branch `foo`.
2. User creates a merge request for `foo`.
3. User merges `foo` into `master` by hand.
4. `RefreshService` reloads the merge request. Since `master` is equivalent to `foo`, this results in an empty diff.
5. `RefreshService` then closes the MR.

This wasn't an issue when you use the normal "Accept Merge Request" flow because the act of clicking the button closes the merge request, so step 4 never happens.

Closes #3314

See merge request !1758

3 files changed, 24 insertions, 4 deletions

diff --git a/CHANGELOG b/CHANGELOG
index 22012211164..19f288d0c7e 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -1,6 +1,7 @@
 Please view this file on the master branch, on stable branches it's out of date.
 
 v 8.2.0 (unreleased)
+  - Fix bug where manually merged branches in a MR would end up with an empty diff (Stan Hu)
   - Force update refs/merge-requests/X/head upon a push to the source branch of a merge request (Stan Hu)
   - Improved performance of finding users by one of their Email addresses
   - Improved performance of replacing references in comments
diff --git a/app/services/merge_requests/refresh_service.rb b/app/services/merge_requests/refresh_service.rb
index d68bc79ecc0..e180edb4bf3 100644
--- a/app/services/merge_requests/refresh_service.rb
+++ b/app/services/merge_requests/refresh_service.rb
@@ -7,17 +7,17 @@ module MergeRequests
       @branch_name = Gitlab::Git.ref_name(ref)
 
       find_new_commits
+      # Be sure to close outstanding MRs before reloading them to avoid generating an
+      # empty diff during a manual merge
+      close_merge_requests
       reload_merge_requests
 
       # Leave a system note if a branch was deleted/added
       if branch_added? || branch_removed?
         comment_mr_branch_presence_changed
-        comment_mr_with_commits
-      else
-        comment_mr_with_commits
-        close_merge_requests
       end
 
+      comment_mr_with_commits
       execute_mr_web_hooks
 
       true
diff --git a/spec/services/merge_requests/refresh_service_spec.rb b/spec/services/merge_requests/refresh_service_spec.rb
index 227ac995ec2..7ee4488521d 100644
--- a/spec/services/merge_requests/refresh_service_spec.rb
+++ b/spec/services/merge_requests/refresh_service_spec.rb
@@ -62,6 +62,25 @@ describe MergeRequests::RefreshService do
       it { expect(@fork_merge_request.notes.last.note).to include('changed to merged') }
     end
 
+    context 'manual merge of source branch' do
+      before do
+        # Merge master -> feature branch
+        author = { email: 'test@gitlab.com', time: Time.now, name: "Me" }
+        commit_options = { message: 'Test message', committer: author, author: author }
+        master_commit = @project.repository.commit('master')
+        @project.repository.merge(@user, master_commit.id, 'feature', commit_options)
+        commit = @project.repository.commit('feature')
+        service.new(@project, @user).execute(@oldrev, commit.id, 'refs/heads/feature')
+        reload_mrs
+      end
+
+      it { expect(@merge_request.notes.last.note).to include('changed to merged') }
+      it { expect(@merge_request).to be_merged }
+      it { expect(@merge_request.diffs.length).to be > 0 }
+      it { expect(@fork_merge_request).to be_merged }
+      it { expect(@fork_merge_request.notes.last.note).to include('changed to merged') }
+    end
+
     context 'push to fork repo source branch' do
       let(:refresh_service) { service.new(@fork_project, @user) }
       before do