From b0164771ec693ff58504ece560371ffec11f9ca9 Mon Sep 17 00:00:00 2001
From: Douwe Maan <douwe@gitlab.com>
Date: Mon, 12 Oct 2015 11:54:46 +0200
Subject: Simplify code around (cross)-references

---
 app/controllers/projects/issues_controller.rb      |  2 +-
 .../projects/merge_requests_controller.rb          |  2 +-
 app/models/concerns/mentionable.rb                 | 38 ++++++++--------
 app/models/concerns/participable.rb                | 16 +++----
 app/models/note.rb                                 |  2 +-
 app/services/git_push_service.rb                   | 50 +++++++---------------
 app/services/issues/create_service.rb              |  2 +-
 app/services/issues/update_service.rb              |  2 +-
 app/services/merge_requests/create_service.rb      |  2 +-
 app/services/merge_requests/update_service.rb      |  2 +-
 10 files changed, 49 insertions(+), 69 deletions(-)

(limited to 'app')

diff --git a/app/controllers/projects/issues_controller.rb b/app/controllers/projects/issues_controller.rb
index 0f89f2e88cc..4612abcbae8 100644
--- a/app/controllers/projects/issues_controller.rb
+++ b/app/controllers/projects/issues_controller.rb
@@ -55,7 +55,7 @@ class Projects::IssuesController < Projects::ApplicationController
   end
 
   def show
-    @participants = @issue.participants(current_user, @project)
+    @participants = @issue.participants(current_user)
     @note = @project.notes.new(noteable: @issue)
     @notes = @issue.notes.inc_author.fresh
     @noteable = @issue
diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb
index 7570934e727..98df6984bf7 100644
--- a/app/controllers/projects/merge_requests_controller.rb
+++ b/app/controllers/projects/merge_requests_controller.rb
@@ -246,7 +246,7 @@ class Projects::MergeRequestsController < Projects::ApplicationController
   end
 
   def define_show_vars
-    @participants = @merge_request.participants(current_user, @project)
+    @participants = @merge_request.participants(current_user)
 
     # Build a note object for comment form
     @note = @project.notes.new(noteable: @merge_request)
diff --git a/app/models/concerns/mentionable.rb b/app/models/concerns/mentionable.rb
index 5b0ae411642..694403949c1 100644
--- a/app/models/concerns/mentionable.rb
+++ b/app/models/concerns/mentionable.rb
@@ -43,53 +43,53 @@ module Mentionable
 
   # Determine whether or not a cross-reference Note has already been created between this Mentionable and
   # the specified target.
-  def has_mentioned?(target)
+  def cross_reference_exists?(target)
     SystemNoteService.cross_reference_exists?(target, local_reference)
   end
 
-  def mentioned_users(current_user = nil)
-    return [] if mentionable_text.blank?
-
+  def all_references(current_user = self.author, text = self.mentionable_text)
     ext = Gitlab::ReferenceExtractor.new(self.project, current_user)
-    ext.analyze(mentionable_text)
-    ext.users.uniq
+    ext.analyze(text)
+    ext
+  end
+
+  def mentioned_users(current_user = nil)
+    all_references(current_user).users.uniq
   end
 
   # Extract GFM references to other Mentionables from this Mentionable. Always excludes its #local_reference.
-  def references(p = project, current_user = self.author, text = mentionable_text)
+  def referenced_mentionables(current_user = self.author, text = self.mentionable_text)
     return [] if text.blank?
 
-    ext = Gitlab::ReferenceExtractor.new(p, current_user)
-    ext.analyze(text)
-
-    (ext.issues + ext.merge_requests + ext.commits).uniq - [local_reference]
+    refs = all_references(current_user, text)
+    (refs.issues + refs.merge_requests + refs.commits).uniq - [local_reference]
   end
 
   # Create a cross-reference Note for each GFM reference to another Mentionable found in +mentionable_text+.
-  def create_cross_references!(p = project, a = author, without = [])
-    refs = references(p)
-
+  def create_cross_references!(author = self.author, without = [])
+    refs = referenced_mentionables(author)
+    
     # We're using this method instead of Array diffing because that requires
     # both of the object's `hash` values to be the same, which may not be the
     # case for otherwise identical Commit objects.
-    refs.reject! { |ref| without.include?(ref) }
+    refs.reject! { |ref| without.include?(ref) || cross_reference_exists?(ref) }
 
     refs.each do |ref|
-      SystemNoteService.cross_reference(ref, local_reference, a)
+      SystemNoteService.cross_reference(ref, local_reference, author)
     end
   end
 
   # When a mentionable field is changed, creates cross-reference notes that
   # don't already exist
-  def create_new_cross_references!(p = project, a = author)
+  def create_new_cross_references!(author = self.author)
     changes = detect_mentionable_changes
 
     return if changes.empty?
 
     original_text = changes.collect { |_, vals| vals.first }.join(' ')
 
-    preexisting = references(p, self.author, original_text)
-    create_cross_references!(p, a, preexisting)
+    preexisting = referenced_mentionables(author, original_text)
+    create_cross_references!(author, preexisting)
   end
 
   private
diff --git a/app/models/concerns/participable.rb b/app/models/concerns/participable.rb
index 7c9597333dd..d697c125c7d 100644
--- a/app/models/concerns/participable.rb
+++ b/app/models/concerns/participable.rb
@@ -37,7 +37,7 @@ module Participable
 
   # Be aware that this method makes a lot of sql queries.
   # Save result into variable if you are going to reuse it inside same request
-  def participants(current_user = self.author, project = self.project)
+  def participants(current_user = self.author)
     participants = self.class.participant_attrs.flat_map do |attr|
       meth = method(attr)
 
@@ -48,13 +48,11 @@ module Participable
           meth.call
         end
 
-      participants_for(value, current_user, project)
+      participants_for(value, current_user)
     end.compact.uniq
 
-    if project
-      participants.select! do |user|
-        user.can?(:read_project, project)
-      end
+    participants.select! do |user|
+      user.can?(:read_project, self.project)
     end
 
     participants
@@ -62,14 +60,14 @@ module Participable
 
   private
 
-  def participants_for(value, current_user = nil, project = nil)
+  def participants_for(value, current_user = nil)
     case value
     when User
       [value]
     when Enumerable, ActiveRecord::Relation
-      value.flat_map { |v| participants_for(v, current_user, project) }
+      value.flat_map { |v| participants_for(v, current_user) }
     when Participable
-      value.participants(current_user, project)
+      value.participants(current_user)
     end
   end
 end
diff --git a/app/models/note.rb b/app/models/note.rb
index de3b6df88f7..13eb28f5718 100644
--- a/app/models/note.rb
+++ b/app/models/note.rb
@@ -358,7 +358,7 @@ class Note < ActiveRecord::Base
   end
 
   def set_references
-    create_new_cross_references!(project, author)
+    create_new_cross_references!
   end
 
   def system?
diff --git a/app/services/git_push_service.rb b/app/services/git_push_service.rb
index f9a8265d2d4..81d47602f13 100644
--- a/app/services/git_push_service.rb
+++ b/app/services/git_push_service.rb
@@ -74,48 +74,30 @@ class GitPushService
   def process_commit_messages(ref)
     is_default_branch = is_default_branch?(ref)
 
-    @push_commits.each do |commit|
-      # Close issues if these commits were pushed to the project's default branch and the commit message matches the
-      # closing regex. Exclude any mentioned Issues from cross-referencing even if the commits are being pushed to
-      # a different branch.
-      issues_to_close = commit.closes_issues(user)
+    authors = Hash.new do |hash, commit|
+      email = commit.author_email
+      return hash[email] if hash.has_key?(email)
 
-      # Load commit author only if needed.
-      # For push with 1k commits it prevents 900+ requests in database
-      author = nil
+      hash[email] = commit_user(commit)
+    end
 
+    @push_commits.each do |commit|
       # 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)
+      closed_issues = []
+
+      if is_default_branch
+        # Close issues if these commits were pushed to the project's default branch and the commit message matches the
+        # closing regex. Exclude any mentioned Issues from cross-referencing even if the commits are being pushed to
+        # a different branch.
+        closed_issues = commit.closes_issues(user)
+        closed_issues.each do |issue|
+          Issues::CloseService.new(project, authors[commit], {}).execute(issue, commit)
         end
       end
 
-      if project.default_issues_tracker?
-        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 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) }
-
-    if refs.present?
-      author ||= commit_user(commit)
-
-      refs.each do |r|
-        SystemNoteService.cross_reference(r, commit, author)
-      end
+      commit.create_cross_references!(authors[commit], closed_issues)
     end
   end
 
diff --git a/app/services/issues/create_service.rb b/app/services/issues/create_service.rb
index 1ea4b72216c..bcb380d3215 100644
--- a/app/services/issues/create_service.rb
+++ b/app/services/issues/create_service.rb
@@ -10,7 +10,7 @@ module Issues
         issue.update_attributes(label_ids: label_params)
         notification_service.new_issue(issue, current_user)
         event_service.open_issue(issue, current_user)
-        issue.create_cross_references!(issue.project, current_user)
+        issue.create_cross_references!(current_user)
         execute_hooks(issue, 'open')
       end
 
diff --git a/app/services/issues/update_service.rb b/app/services/issues/update_service.rb
index 2fc6ef7f356..2b5426ad452 100644
--- a/app/services/issues/update_service.rb
+++ b/app/services/issues/update_service.rb
@@ -35,7 +35,7 @@ module Issues
           create_title_change_note(issue, issue.previous_changes['title'].first)
         end
 
-        issue.create_new_cross_references!(issue.project, current_user)
+        issue.create_new_cross_references!
         execute_hooks(issue, 'update')
       end
 
diff --git a/app/services/merge_requests/create_service.rb b/app/services/merge_requests/create_service.rb
index 9651b16462c..009d5a6867e 100644
--- a/app/services/merge_requests/create_service.rb
+++ b/app/services/merge_requests/create_service.rb
@@ -18,7 +18,7 @@ module MergeRequests
         merge_request.update_attributes(label_ids: label_params)
         event_service.open_mr(merge_request, current_user)
         notification_service.new_merge_request(merge_request, current_user)
-        merge_request.create_cross_references!(merge_request.project, current_user)
+        merge_request.create_cross_references!(current_user)
         execute_hooks(merge_request)
       end
 
diff --git a/app/services/merge_requests/update_service.rb b/app/services/merge_requests/update_service.rb
index 25d79e22e39..ebbe0af803b 100644
--- a/app/services/merge_requests/update_service.rb
+++ b/app/services/merge_requests/update_service.rb
@@ -59,7 +59,7 @@ module MergeRequests
           merge_request.mark_as_unchecked
         end
 
-        merge_request.create_new_cross_references!(merge_request.project, current_user)
+        merge_request.create_new_cross_references!
         execute_hooks(merge_request, 'update')
       end
 
-- 
cgit v1.2.1


From 27d952b1197f2dc615c383c21eb287313d81c74c Mon Sep 17 00:00:00 2001
From: Douwe Maan <douwe@gitlab.com>
Date: Mon, 12 Oct 2015 14:30:44 +0200
Subject: Fix cross-references originating from notes

---
 app/models/concerns/mentionable.rb   |  4 ++--
 app/models/note.rb                   | 13 +++----------
 app/services/notes/create_service.rb |  8 +-------
 app/services/notes/update_service.rb |  2 +-
 4 files changed, 7 insertions(+), 20 deletions(-)

(limited to 'app')

diff --git a/app/models/concerns/mentionable.rb b/app/models/concerns/mentionable.rb
index 694403949c1..5f53ea25630 100644
--- a/app/models/concerns/mentionable.rb
+++ b/app/models/concerns/mentionable.rb
@@ -66,8 +66,8 @@ module Mentionable
   end
 
   # Create a cross-reference Note for each GFM reference to another Mentionable found in +mentionable_text+.
-  def create_cross_references!(author = self.author, without = [])
-    refs = referenced_mentionables(author)
+  def create_cross_references!(author = self.author, without = [], text = self.mentionable_text)
+    refs = referenced_mentionables(author, text)
     
     # We're using this method instead of Array diffing because that requires
     # both of the object's `hash` values to be the same, which may not be the
diff --git a/app/models/note.rb b/app/models/note.rb
index 13eb28f5718..ee0c14598f3 100644
--- a/app/models/note.rb
+++ b/app/models/note.rb
@@ -62,7 +62,6 @@ class Note < ActiveRecord::Base
 
   serialize :st_diff
   before_create :set_diff, if: ->(n) { n.line_code.present? }
-  after_update :set_references
 
   class << self
     def discussions_from_notes(notes)
@@ -333,15 +332,13 @@ class Note < ActiveRecord::Base
   end
 
   def noteable_type_name
-    if noteable_type.present?
-      noteable_type.downcase
-    end
+    noteable_type.downcase if noteable_type.present?
   end
 
   # FIXME: Hack for polymorphic associations with STI
   #        For more information visit http://api.rubyonrails.org/classes/ActiveRecord/Associations/ClassMethods.html#label-Polymorphic+Associations
-  def noteable_type=(sType)
-    super(sType.to_s.classify.constantize.base_class.to_s)
+  def noteable_type=(noteable_type)
+    super(noteable_type.to_s.classify.constantize.base_class.to_s)
   end
 
   # Reset notes events cache
@@ -357,10 +354,6 @@ class Note < ActiveRecord::Base
     Event.reset_event_cache_for(self)
   end
 
-  def set_references
-    create_new_cross_references!
-  end
-
   def system?
     read_attribute(:system)
   end
diff --git a/app/services/notes/create_service.rb b/app/services/notes/create_service.rb
index 482c0444049..2001dc89c33 100644
--- a/app/services/notes/create_service.rb
+++ b/app/services/notes/create_service.rb
@@ -11,13 +11,7 @@ module Notes
         # Skip system notes, like status changes and cross-references.
         unless note.system
           event_service.leave_note(note, note.author)
-
-          # Create a cross-reference note if this Note contains GFM that names an
-          # issue, merge request, or commit.
-          note.references.each do |mentioned|
-            SystemNoteService.cross_reference(mentioned, note.noteable, note.author)
-          end
-
+          note.create_cross_references!
           execute_hooks(note)
         end
       end
diff --git a/app/services/notes/update_service.rb b/app/services/notes/update_service.rb
index c22a9333ef6..6c2f08e5963 100644
--- a/app/services/notes/update_service.rb
+++ b/app/services/notes/update_service.rb
@@ -4,7 +4,7 @@ module Notes
       return note unless note.editable?
 
       note.update_attributes(params.merge(updated_by: current_user))
-
+      note.create_new_cross_references!
       note.reset_events_cache
 
       note
-- 
cgit v1.2.1


From 41075bad8daeed07bceaa44cbfe54ab1b470ca87 Mon Sep 17 00:00:00 2001
From: Douwe Maan <douwe@gitlab.com>
Date: Mon, 12 Oct 2015 14:39:33 +0200
Subject: Chaining ftw

---
 app/models/concerns/participable.rb | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

(limited to 'app')

diff --git a/app/models/concerns/participable.rb b/app/models/concerns/participable.rb
index d697c125c7d..22182445978 100644
--- a/app/models/concerns/participable.rb
+++ b/app/models/concerns/participable.rb
@@ -49,13 +49,9 @@ module Participable
         end
 
       participants_for(value, current_user)
-    end.compact.uniq
-
-    participants.select! do |user|
+    end.compact.uniq.select do |user|
       user.can?(:read_project, self.project)
     end
-
-    participants
   end
 
   private
-- 
cgit v1.2.1


From 7a0cc665ff5ad3969f36082baa162a2169c34612 Mon Sep 17 00:00:00 2001
From: Douwe Maan <douwe@gitlab.com>
Date: Mon, 12 Oct 2015 15:34:08 +0200
Subject: Remove useless assignment

---
 app/models/concerns/participable.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

(limited to 'app')

diff --git a/app/models/concerns/participable.rb b/app/models/concerns/participable.rb
index 22182445978..ffc874357fd 100644
--- a/app/models/concerns/participable.rb
+++ b/app/models/concerns/participable.rb
@@ -38,7 +38,7 @@ module Participable
   # Be aware that this method makes a lot of sql queries.
   # Save result into variable if you are going to reuse it inside same request
   def participants(current_user = self.author)
-    participants = self.class.participant_attrs.flat_map do |attr|
+    self.class.participant_attrs.flat_map do |attr|
       meth = method(attr)
 
       value =
-- 
cgit v1.2.1


From 276b3a7bc202bd9b51c8f5401f4c525227f3a4d8 Mon Sep 17 00:00:00 2001
From: Douwe Maan <douwe@gitlab.com>
Date: Wed, 14 Oct 2015 09:27:30 +0200
Subject: Make Mentionable#cross_reference_exists? private.

---
 app/models/concerns/mentionable.rb | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

(limited to 'app')

diff --git a/app/models/concerns/mentionable.rb b/app/models/concerns/mentionable.rb
index 5f53ea25630..b34def66d2e 100644
--- a/app/models/concerns/mentionable.rb
+++ b/app/models/concerns/mentionable.rb
@@ -41,12 +41,6 @@ module Mentionable
     self
   end
 
-  # Determine whether or not a cross-reference Note has already been created between this Mentionable and
-  # the specified target.
-  def cross_reference_exists?(target)
-    SystemNoteService.cross_reference_exists?(target, local_reference)
-  end
-
   def all_references(current_user = self.author, text = self.mentionable_text)
     ext = Gitlab::ReferenceExtractor.new(self.project, current_user)
     ext.analyze(text)
@@ -111,4 +105,10 @@ module Mentionable
     # Only include changed fields that are mentionable
     source.select { |key, val| mentionable.include?(key) }
   end
+  
+  # Determine whether or not a cross-reference Note has already been created between this Mentionable and
+  # the specified target.
+  def cross_reference_exists?(target)
+    SystemNoteService.cross_reference_exists?(target, local_reference)
+  end
 end
-- 
cgit v1.2.1