Merge branch 'master' into 'master'

"Fixes #xxxx" now shows up in the issue log for non-default branches. #2190

I don't understand why the commits containing "closing references" (like `closes #xxxx`) were not mentioned in the corresponding issues **when pushed to a non-default branch**.

So I tried to discover how it works -- hence learning Ruby! I don't expect that MR to pass, this is my very first attempt of contribution.

**Update:** my modifications are now done. To sum up:
- when a commit with a reference `fixes #xxxx` is pushed to a non-default branch, a cross-reference to that issue will be created;
- when that same commit is pushed to a default branch, no cross-reference will be created because a message `This commit closes issue` will be emitted.
- I also refined some of the existing tests and added 2 tests on the new behavior on non-default branches.

See merge request !1150

3 files changed, 45 insertions, 28 deletions

diff --git a/CHANGELOG b/CHANGELOG
index dd5162a53db..fe0992b5a0c 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -9,6 +9,7 @@ v 8.0.0 (unreleased)
   - Allow displaying of archived projects in the admin interface (Artem Sidorenko)
   - Allow configuration of import sources for new projects (Artem Sidorenko)
   - Search for comments should be case insensetive
+  - Create cross-reference for closing references on commits pushed to non-default branches (Maël Valais)
 
 v 7.14.0 (unreleased)
   - Fix bug where non-project members of the target project could set labels on new merge requests.
diff --git a/app/services/git_push_service.rb b/app/services/git_push_service.rb
index 81535450ac1..0a73244774a 100644
--- a/app/services/git_push_service.rb
+++ b/app/services/git_push_service.rb
@@ -78,24 +78,29 @@ class GitPushService
       # For push with 1k commits it prevents 900+ requests in database
       author = nil
 
+      # Keep track of the issues that will be actually closed because they are on a default branch.
+      # Hence, when creating cross-reference notes, the not-closed issues (on non-default branches)
+      # will also have cross-reference.
+      actually_closed_issues = []
+
       if issues_to_close.present? && is_default_branch
         author ||= commit_user(commit)
-
+        actually_closed_issues = issues_to_close
         issues_to_close.each do |issue|
           Issues::CloseService.new(project, author, {}).execute(issue, commit)
         end
       end
 
       if project.default_issues_tracker?
-        create_cross_reference_notes(commit, issues_to_close)
+        create_cross_reference_notes(commit, actually_closed_issues)
       end
     end
   end
 
   def create_cross_reference_notes(commit, issues_to_close)
-    # Create cross-reference notes for any other references. Omit any issues that were referenced in an
-    # issue-closing phrase, or have already been mentioned from this commit (probably from this commit
-    # being pushed to a different branch).
+    # Create cross-reference notes for any other references than those given in issues_to_close.
+    # Omit any issues that were referenced in an issue-closing phrase, or have already been
+    # mentioned from this commit (probably from this commit being pushed to a different branch).
     refs = commit.references(project, user) - issues_to_close
     refs.reject! { |r| commit.has_mentioned?(r) }
 
diff --git a/spec/services/git_push_service_spec.rb b/spec/services/git_push_service_spec.rb
index 62cef9db534..c483060fd73 100644
--- a/spec/services/git_push_service_spec.rb
+++ b/spec/services/git_push_service_spec.rb
@@ -197,7 +197,7 @@ describe GitPushService do
     end
   end
 
-  describe "closing issues from pushed commits" do
+  describe "closing issues from pushed commits containing a closing reference" do
     let(:issue) { create :issue, project: project }
     let(:other_issue) { create :issue, project: project }
     let(:commit_author) { create :user }
@@ -215,36 +215,47 @@ describe GitPushService do
         and_return([closing_commit])
     end
 
-    it "closes issues with commit messages" do
-      service.execute(project, user, @oldrev, @newrev, @ref)
-
-      expect(Issue.find(issue.id)).to be_closed
-    end
+    context "to default branches" do
+      it "closes issues" do
+        service.execute(project, user, @oldrev, @newrev, @ref)
+        expect(Issue.find(issue.id)).to be_closed
+      end
 
-    it "doesn't create cross-reference notes for a closing reference" do
-      expect do
+      it "adds a note indicating that the issue is now closed" do
+        expect(SystemNoteService).to receive(:change_status).with(issue, project, commit_author, "closed", closing_commit)
         service.execute(project, user, @oldrev, @newrev, @ref)
-      end.not_to change { Note.where(project_id: project.id, system: true, commit_id: closing_commit.id).count }
-    end
+      end
 
-    it "doesn't close issues when pushed to non-default branches" do
-      allow(project).to receive(:default_branch).and_return('durf')
+      it "doesn't create additional cross-reference notes" do
+        expect(SystemNoteService).not_to receive(:cross_reference)
+        service.execute(project, user, @oldrev, @newrev, @ref)
+      end
 
-      # The push still shouldn't create cross-reference notes.
-      expect do
-        service.execute(project, user, @oldrev, @newrev, 'refs/heads/hurf')
-      end.not_to change { Note.where(project_id: project.id, system: true).count }
+      it "doesn't close issues when external issue tracker is in use" do
+        allow(project).to receive(:default_issues_tracker?).and_return(false)
 
-      expect(Issue.find(issue.id)).to be_opened
+        # The push still shouldn't create cross-reference notes.
+        expect do
+          service.execute(project, user, @oldrev, @newrev, 'refs/heads/hurf')
+        end.not_to change { Note.where(project_id: project.id, system: true).count }
+      end
     end
 
-    it "doesn't close issues when external issue tracker is in use" do
-      allow(project).to receive(:default_issues_tracker?).and_return(false)
+    context "to non-default branches" do
+      before do
+        # Make sure the "default" branch is different
+        allow(project).to receive(:default_branch).and_return('not-master')
+      end
+
+      it "creates cross-reference notes" do
+        expect(SystemNoteService).to receive(:cross_reference).with(issue, closing_commit, commit_author)
+        service.execute(project, user, @oldrev, @newrev, @ref)
+      end
 
-      # The push still shouldn't create cross-reference notes.
-      expect do
-        service.execute(project, user, @oldrev, @newrev, 'refs/heads/hurf')
-      end.not_to change { Note.where(project_id: project.id, system: true).count }
+      it "doesn't close issues" do
+        service.execute(project, user, @oldrev, @newrev, @ref)
+        expect(Issue.find(issue.id)).to be_opened
+      end
     end
   end