Merge branch 'commit-project' into 'master'

Link cross-project cross-reference notes to correct project.

- Fixes #1431: Don't crash when an MR from a fork has a cross-reference comment from the target project on of its commits.
- Include commit comments in MR from a forked project.

To accomplish this, the Commit model needs to know about its project. Adding this attribute also simplifies a bunch of other methods that deal with a note's target, that previously had to have special logic for when it belonged to a commit.

See merge request !554

CHANGELOG

@@ -12,6 +12,8 @@ v 7.11.0 (unreleased)
   -
   - Show Atom feed buttons everywhere where applicable.
   - Add project activity atom feed.
+  - Don't crash when an MR from a fork has a cross-reference comment from the target project on of its commits.
+  - Include commit comments in MR from a forked project.
   -
   -
   -

app/controllers/projects/commit_controller.rb

@@ -10,11 +10,11 @@ class Projects::CommitController < Projects::ApplicationController
   def show
     return git_not_found! unless @commit
 
-    @line_notes = commit.notes(@project).inline
+    @line_notes = commit.notes.inline
     @diffs = @commit.diffs
     @note = @project.build_commit_note(commit)
-    @notes_count = commit.notes(@project).count
-    @notes = commit.notes(@project).not_inline.fresh
+    @notes_count = commit.notes.count
+    @notes = commit.notes.not_inline.fresh
     @noteable = @commit
     @comments_allowed = @reply_allowed = true
     @comments_target  = {
@@ -36,6 +36,6 @@ class Projects::CommitController < Projects::ApplicationController
   end
 
   def commit
-    @commit ||= @project.repository.commit(params[:id])
+    @commit ||= @project.commit(params[:id])
   end
 end

app/controllers/projects/merge_requests_controller.rb

@@ -146,7 +146,7 @@ class Projects::MergeRequestsController < Projects::ApplicationController
 
   def branch_to
     @target_project = selected_target_project
-    @commit = @target_project.repository.commit(params[:ref]) if params[:ref].present?
+    @commit = @target_project.commit(params[:ref]) if params[:ref].present?
   end
 
   def update_branches

app/mailers/emails/projects.rb

@@ -79,7 +79,7 @@ module Emails
       @disable_diffs = disable_diffs
 
       if @compare
-        @commits = Commit.decorate(compare.commits)
+        @commits = Commit.decorate(compare.commits, @project)
         @diffs   = compare.diffs
       end
 
@@ -101,8 +101,8 @@ module Emails
         if @commits.length > 1
           @target_url = namespace_project_compare_url(@project.namespace,
                                                       @project,
-                                                      from: Commit.new(@compare.base),
-                                                      to:   Commit.new(@compare.head))
+                                                      from: Commit.new(@compare.base, @project),
+                                                      to:   Commit.new(@compare.head, @project))
           @subject << "Deleted " if @reverse_compare
           @subject << "#{@commits.length} commits: #{@commits.first.title}"
         else

app/models/commit.rb

@@ -3,8 +3,12 @@ class Commit
   include StaticModel
   extend ActiveModel::Naming
   include Mentionable
+  include Participable
 
   attr_mentionable :safe_message
+  participant :author, :committer, :notes, :mentioned_users
+
+  attr_accessor :project
 
   # Safe amount of changes (files and lines) in one commit to render
   # Used to prevent 500 error on huge commits by suppressing diff
@@ -18,12 +22,12 @@ class Commit
   DIFF_HARD_LIMIT_LINES = 50000 unless defined?(DIFF_HARD_LIMIT_LINES)
 
   class << self
-    def decorate(commits)
+    def decorate(commits, project)
       commits.map do |commit|
         if commit.kind_of?(Commit)
           commit
         else
-          self.new(commit)
+          self.new(commit, project)
         end
       end
     end
@@ -41,10 +45,11 @@ class Commit
 
   attr_accessor :raw
 
-  def initialize(raw_commit)
+  def initialize(raw_commit, project)
     raise "Nil as raw commit passed" unless raw_commit
 
     @raw = raw_commit
+    @project = project
   end
 
   def id
@@ -100,7 +105,7 @@ class Commit
     description.present?
   end
 
-  def hook_attrs(project)
+  def hook_attrs
     path_with_namespace = project.path_with_namespace
 
     {
@@ -117,7 +122,7 @@ class Commit
 
   # Discover issues should be closed when this commit is pushed to a project's
   # default branch.
-  def closes_issues(project, current_user = self.committer)
+  def closes_issues(current_user = self.committer)
     Gitlab::ClosingIssueExtractor.new(project, current_user).closed_by_message(safe_message)
   end
 
@@ -134,22 +139,7 @@ class Commit
     User.find_for_commit(committer_email, committer_name)
   end
 
-  def participants(project, current_user = nil)
-    users = []
-    users << author
-    users << committer
-    
-    users.push *self.mentioned_users(current_user, project)
-
-    notes(project).each do |note|
-      users << note.author
-      users.push *note.mentioned_users(current_user, project)
-    end
-
-    users.uniq
-  end
-
-  def notes(project)
+  def notes
     project.notes.for_commit_id(self.id)
   end
 
@@ -169,6 +159,6 @@ class Commit
   end
 
   def parents
-    @parents ||= Commit.decorate(super)
+    @parents ||= Commit.decorate(super, project)
   end
 end

app/models/concerns/issuable.rb

@@ -7,6 +7,7 @@
 module Issuable
   extend ActiveSupport::Concern
   include Mentionable
+  include Participable
 
   included do
     belongs_to :author, class_name: "User"
@@ -45,6 +46,7 @@ module Issuable
              prefix: true
 
     attr_mentionable :title, :description
+    participant :author, :assignee, :notes, :mentioned_users
   end
 
   module ClassMethods
@@ -117,22 +119,6 @@ module Issuable
     upvotes + downvotes
   end
 
-  # Return all users participating on the discussion
-  def participants(current_user = self.author)
-    users = []
-    users << author
-    users << assignee if is_assigned?
-
-    users.push *self.mentioned_users(current_user)
-
-    notes.each do |note|
-      users << note.author
-      users.push *note.mentioned_users(current_user)
-    end
-
-    users.uniq
-  end
-
   def subscribed?(user)
     subscription = subscriptions.find_by_user_id(user.id)
 

app/models/concerns/mentionable.rb

@@ -64,7 +64,7 @@ module Mentionable
   def create_cross_references!(p = project, a = author, without = [])
     refs = references(p) - without
     refs.each do |ref|
-      Note.create_cross_reference_note(ref, local_reference, a, p)
+      Note.create_cross_reference_note(ref, local_reference, a)
     end
   end
 

app/models/concerns/participable.rb

+# == Participable concern
+#
+# Contains functionality related to objects that can have participants, such as
+# an author, an assignee and people mentioned in its description or comments.
+#
+# Used by Issue, Note, MergeRequest, Snippet and Commit.
+#
+# Usage:
+#
+#     class Issue < ActiveRecord::Base
+#       include Participable
+#
+#       # ...
+#
+#       participant :author, :assignee, :mentioned_users, :notes
+#     end
+#     
+#     issue = Issue.last
+#     users = issue.participants
+#     # `users` will contain the issue's author, its assignee,
+#     # all users returned by its #mentioned_users method,
+#     # as well as all participants to all of the issue's notes,
+#     # since Note implements Participable as well.
+#
+module Participable
+  extend ActiveSupport::Concern
+
+  module ClassMethods
+    def participant(*attrs)
+      participant_attrs.concat(attrs.map(&:to_s))
+    end
+
+    def participant_attrs
+      @participant_attrs ||= []
+    end
+  end
+
+  def participants(current_user = self.author)
+    self.class.participant_attrs.flat_map do |attr|
+      meth = method(attr)
+
+      value = 
+        if meth.arity == 1
+          meth.call(current_user)
+        else
+          meth.call
+        end
+
+      participants_for(value, current_user)
+    end.compact.uniq
+  end
+
+  private
+  
+  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) }
+    when Participable
+      value.participants(current_user)
+    end
+  end
+end

app/models/merge_request.rb

@@ -213,10 +213,13 @@ class MergeRequest < ActiveRecord::Base
     commits_for_notes_limit = 100
     commit_ids = commits.last(commits_for_notes_limit).map(&:id)
 
-    project.notes.where(
-      "(noteable_type = 'MergeRequest' AND noteable_id = :mr_id) OR (noteable_type = 'Commit' AND commit_id IN (:commit_ids))",
+    Note.where(
+      "(project_id = :target_project_id AND noteable_type = 'MergeRequest' AND noteable_id = :mr_id) OR" +
+      "(project_id = :source_project_id AND noteable_type = 'Commit' AND commit_id IN (:commit_ids))",
       mr_id: id,
-      commit_ids: commit_ids
+      commit_ids: commit_ids,
+      target_project_id: target_project_id,
+      source_project_id: source_project_id
     )
   end
 
@@ -242,7 +245,7 @@ class MergeRequest < ActiveRecord::Base
     }
 
     unless last_commit.nil?
-      attrs.merge!(last_commit: last_commit.hook_attrs(source_project))
+      attrs.merge!(last_commit: last_commit.hook_attrs)
     end
 
     attributes.merge!(attrs)
@@ -259,7 +262,7 @@ class MergeRequest < ActiveRecord::Base
   # Return the set of issues that will be closed if this merge request is accepted.
   def closes_issues(current_user = self.author)
     if target_branch == project.default_branch
-      issues = commits.flat_map { |c| c.closes_issues(project, current_user) }
+      issues = commits.flat_map { |c| c.closes_issues(current_user) }
       issues.push(*Gitlab::ClosingIssueExtractor.new(project, current_user).
                   closed_by_message(description))
       issues.uniq.sort_by(&:id)

app/models/merge_request_diff.rb

@@ -67,7 +67,7 @@ class MergeRequestDiff < ActiveRecord::Base
   end
 
   def load_commits(array)
-    array.map { |hash| Commit.new(Gitlab::Git::Commit.new(hash)) }
+    array.map { |hash| Commit.new(Gitlab::Git::Commit.new(hash), merge_request.source_project) }
   end
 
   def dump_diffs(diffs)
@@ -88,7 +88,7 @@ class MergeRequestDiff < ActiveRecord::Base
     commits = compare_result.commits
 
     if commits.present?
-      commits = Commit.decorate(commits).
+      commits = Commit.decorate(commits, merge_request.source_project).
         sort_by(&:created_at).
         reverse
     end

app/models/note.rb

@@ -23,10 +23,12 @@ require 'file_size_validator'
 class Note < ActiveRecord::Base
   include Mentionable
   include Gitlab::CurrentSettings
+  include Participable
 
   default_value_for :system, false
 
   attr_mentionable :note
+  participant :author, :mentioned_users
 
   belongs_to :project
   belongs_to :noteable, polymorphic: true
@@ -77,11 +79,11 @@ class Note < ActiveRecord::Base
     # +mentioner+'s description or an associated Note.
     # Create a system Note associated with +noteable+ with a GFM back-reference
     # to +mentioner+.
-    def create_cross_reference_note(noteable, mentioner, author, project)
-      gfm_reference = mentioner_gfm_ref(noteable, mentioner, project)
+    def create_cross_reference_note(noteable, mentioner, author)
+      gfm_reference = mentioner_gfm_ref(noteable, mentioner)
 
       note_options = {
-        project: project,
+        project: noteable.project,
         author: author,
         note: cross_reference_note_content(gfm_reference),
         system: true
@@ -236,7 +238,7 @@ class Note < ActiveRecord::Base
 
     # Determine whether or not a cross-reference note already exists.
     def cross_reference_exists?(noteable, mentioner)
-      gfm_reference = mentioner_gfm_ref(noteable, mentioner)
+      gfm_reference = mentioner_gfm_ref(noteable, mentioner, true)
       notes = if noteable.is_a?(Commit)
                 where(commit_id: noteable.id, noteable_type: 'Commit')
               else
@@ -269,43 +271,19 @@ class Note < ActiveRecord::Base
     # Prepend the mentioner's namespaced project path to the GFM reference for
     # cross-project references.  For same-project references, return the
     # unmodified GFM reference.
-    def mentioner_gfm_ref(noteable, mentioner, project = nil)
-      if mentioner.is_a?(Commit)
-        if project.nil?
-          return mentioner.gfm_reference.sub('commit ', 'commit %')
-        else
-          mentioning_project = project
-        end
-      else
-        mentioning_project = mentioner.project
-      end
-
-      noteable_project_id = noteable_project_id(noteable, mentioning_project)
-
-      full_gfm_reference(mentioning_project, noteable_project_id, mentioner)
-    end
-
-    # Return the ID of the project that +noteable+ belongs to, or nil if
-    # +noteable+ is a commit and is not part of the project that owns
-    # +mentioner+.
-    def noteable_project_id(noteable, mentioning_project)
-      if noteable.is_a?(Commit)
-        if mentioning_project.repository.commit(noteable.id)
-          # The noteable commit belongs to the mentioner's project
-          mentioning_project.id
-        else
-          nil
-        end
-      else
-        noteable.project.id
+    def mentioner_gfm_ref(noteable, mentioner, cross_reference = false)
+      if mentioner.is_a?(Commit) && cross_reference
+        return mentioner.gfm_reference.sub('commit ', 'commit %')
       end
+      
+      full_gfm_reference(mentioner.project, noteable.project, mentioner)
     end
 
     # Return the +mentioner+ GFM reference.  If the mentioner and noteable
     # projects are not the same, add the mentioning project's path to the
     # returned value.
-    def full_gfm_reference(mentioning_project, noteable_project_id, mentioner)
-      if mentioning_project.id == noteable_project_id
+    def full_gfm_reference(mentioning_project, noteable_project, mentioner)
+      if mentioning_project == noteable_project
         mentioner.gfm_reference
       else
         if mentioner.is_a?(Commit)
@@ -512,7 +490,7 @@ class Note < ActiveRecord::Base
   # override to return commits, which are not active record
   def noteable
     if for_commit?
-      project.repository.commit(commit_id)
+      project.commit(commit_id)
     else
       super
     end

app/models/project.rb

@@ -254,7 +254,11 @@ class Project < ActiveRecord::Base
   end
 
   def repository
-    @repository ||= Repository.new(path_with_namespace)
+    @repository ||= Repository.new(path_with_namespace, nil, self)
+  end
+
+  def commit(id = 'HEAD')
+    repository.commit(id)
   end
 
   def saved?

app/models/project_wiki.rb

@@ -112,7 +112,7 @@ class ProjectWiki
   end
 
   def repository
-    Repository.new(path_with_namespace, default_branch)
+    Repository.new(path_with_namespace, default_branch, @project)
   end
 
   def default_branch

app/models/protected_branch.rb

@@ -18,6 +18,6 @@ class ProtectedBranch < ActiveRecord::Base
   validates :project, presence: true
 
   def commit
-    project.repository.commit(self.name)
+    project.commit(self.name)
   end
 end

app/models/repository.rb

 class Repository
   include Gitlab::ShellAdapter
 
-  attr_accessor :raw_repository, :path_with_namespace
+  attr_accessor :raw_repository, :path_with_namespace, :project
 
-  def initialize(path_with_namespace, default_branch = nil)
+  def initialize(path_with_namespace, default_branch = nil, project = nil)
     @path_with_namespace = path_with_namespace
     @raw_repository = Gitlab::Git::Repository.new(path_to_repo) if path_with_namespace
+    @project = project
   rescue Gitlab::Git::Repository::NoRepository
     nil
   end
@@ -28,7 +29,7 @@ class Repository
   def commit(id = 'HEAD')
     return nil unless raw_repository
     commit = Gitlab::Git::Commit.find(raw_repository, id)
-    commit = Commit.new(commit) if commit
+    commit = Commit.new(commit, @project) if commit
     commit
   rescue Rugged::OdbError
     nil
@@ -42,13 +43,13 @@ class Repository
       limit: limit,
       offset: offset,
     )
-    commits = Commit.decorate(commits) if commits.present?
+    commits = Commit.decorate(commits, @project) if commits.present?
     commits
   end
 
   def commits_between(from, to)
     commits = Gitlab::Git::Commit.between(raw_repository, from, to)
-    commits = Commit.decorate(commits) if commits.present?
+    commits = Commit.decorate(commits, @project) if commits.present?
     commits
   end
 

app/models/snippet.rb

@@ -19,6 +19,7 @@ class Snippet < ActiveRecord::Base
   include Sortable
   include Linguist::BlobHelper
   include Gitlab::VisibilityLevel
+  include Participable
 
   default_value_for :visibility_level, Snippet::PRIVATE
 
@@ -47,6 +48,8 @@ class Snippet < ActiveRecord::Base
   scope :expired, -> { where(["expires_at IS NOT NULL AND expires_at < ?", Time.current]) }
   scope :non_expired, -> { where(["expires_at IS NULL OR expires_at > ?", Time.current]) }
 
+  participant :author, :notes
+
   def self.content_types
     [
       ".rb", ".py", ".pl", ".scala", ".c", ".cpp", ".java",
@@ -87,18 +90,6 @@ class Snippet < ActiveRecord::Base
     visibility_level
   end
 
-  def participants(current_user = self.author)
-    users = []
-    users << author
-    
-    notes.each do |note|
-      users << note.author
-      users.push *note.mentioned_users(current_user)
-    end
-
-    users.uniq
-  end
-
   class << self
     def search(query)
       where('(title LIKE :query OR file_name LIKE :query)', query: "%#{query}%")

app/services/create_tag_service.rb

@@ -38,7 +38,7 @@ class CreateTagService < BaseService
   end
 
   def create_push_data(project, user, tag)
-    commits = [project.repository.commit(tag.target)].compact
+    commits = [project.commit(tag.target)].compact
     Gitlab::PushDataBuilder.
       build(project, user, Gitlab::Git::BLANK_SHA, tag.target, "#{Gitlab::Git::TAG_REF_PREFIX}#{tag.name}", commits, tag.message)
   end

app/services/git_push_service.rb

@@ -70,7 +70,7 @@ class GitPushService
       # 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(project, user)
+      issues_to_close = commit.closes_issues(user)
 
       # Load commit author only if needed.
       # For push with 1k commits it prevents 900+ requests in database
@@ -94,7 +94,7 @@ class GitPushService
         author ||= commit_user(commit)
 
         refs.each do |r|
-          Note.create_cross_reference_note(r, commit, author, project)
+          Note.create_cross_reference_note(r, commit, author)
         end
       end
     end

app/services/git_tag_push_service.rb

@@ -25,7 +25,7 @@ class GitTagPushService
       tag_name = Gitlab::Git.ref_name(ref)
       tag = project.repository.find_tag(tag_name)
       if tag && tag.target == newrev
-        commit = project.repository.commit(tag.target)
+        commit = project.commit(tag.target)
         commits = [commit].compact
         message = tag.message
       end

app/services/merge_requests/build_service.rb

@@ -29,7 +29,7 @@ module MergeRequests
       # At this point we decide if merge request can be created
       # If we have at least one commit to merge -> creation allowed
       if commits.present?
-        merge_request.compare_commits = Commit.decorate(commits)
+        merge_request.compare_commits = Commit.decorate(commits, merge_request.source_project)
         merge_request.can_be_created = true
         merge_request.compare_failed = false
 

app/services/notes/create_service.rb

@@ -15,7 +15,7 @@ module Notes
           # Create a cross-reference note if this Note contains GFM that names an
           # issue, merge request, or commit.
           note.references.each do |mentioned|
-            Note.create_cross_reference_note(mentioned, note.noteable, note.author, note.project)
+            Note.create_cross_reference_note(mentioned, note.noteable, note.author)
           end
 
           execute_hooks(note)

app/services/notes/update_service.rb

@@ -13,8 +13,7 @@ module Notes
           # Create a cross-reference note if this Note contains GFM that
           # names an issue, merge request, or commit.
           note.references.each do |mentioned|
-            Note.create_cross_reference_note(mentioned, note.noteable,
-                                             note.author, note.project)
+            Note.create_cross_reference_note(mentioned, note.noteable, note.author)
           end
         end
       end

app/services/notification_service.rb

@@ -129,9 +129,7 @@ class NotificationService
 
     # Add all users participating in the thread (author, assignee, comment authors)
     participants = 
-      if target.is_a?(Commit)
-        target.participants(note.project, note.author)
-      elsif target.respond_to?(:participants)
+      if target.respond_to?(:participants)
         target.participants(note.author)
       else
         note.mentioned_users

app/services/projects/participants_service.rb

@@ -13,21 +13,19 @@ module Projects
     end
 
     def participants_in(type, id)
-      users = 
+      target = 
         case type
         when "Issue"
-          issue = project.issues.find_by_iid(id)
-          issue.participants(current_user) if issue
+          project.issues.find_by_iid(id)
         when "MergeRequest"
-          merge_request = project.merge_requests.find_by_iid(id)
-          merge_request.participants(current_user) if merge_request
+          project.merge_requests.find_by_iid(id)
         when "Commit"
-          commit = project.repository.commit(id)
-          commit.participants(project, current_user) if commit
+          project.commit(id)
         end
+        
+      return [] unless target
 
-      return [] unless users
-      
+      users = target.participants(current_user)
       sorted(users)
     end
 

app/views/projects/blame/show.html.haml

@@ -12,7 +12,7 @@
     .file-content.blame.highlight
       %table
         - @blame.each do |commit, lines, since|
-          - commit = Commit.new(commit)
+          - commit = Commit.new(commit, @project)
           %tr
             %td.blame-commit
               %span.commit

app/views/projects/commits/_commit.html.haml

@@ -12,7 +12,7 @@
       - if @note_counts
         - note_count = @note_counts.fetch(commit.id, 0)
       - else
-        - notes = commit.notes(project)
+        - notes = commit.notes
         - note_count = notes.user.count
 
       - if note_count > 0

app/views/projects/commits/_commit_list.html.haml

@@ -3,9 +3,9 @@
     Commits (#{@commits.count})
   - if @commits.size > MergeRequestDiff::COMMITS_SAFE_SIZE
     %ul.well-list
-      - Commit.decorate(@commits.first(MergeRequestDiff::COMMITS_SAFE_SIZE)).each do |commit|
+      - Commit.decorate(@commits.first(MergeRequestDiff::COMMITS_SAFE_SIZE), @project).each do |commit|
         = render "projects/commits/inline_commit", commit: commit, project: @project
       %li.warning-row.unstyled
         other #{@commits.size - MergeRequestDiff::COMMITS_SAFE_SIZE} commits hidden to prevent performance issues.
   - else
-    %ul.well-list= render Commit.decorate(@commits), project: @project
+    %ul.well-list= render Commit.decorate(@commits, @project), project: @project

app/workers/irker_worker.rb

@@ -137,8 +137,7 @@ class IrkerWorker
   end
 
   def commit_from_id(project, id)
-    commit = Gitlab::Git::Commit.find(project.repository, id)
-    Commit.new(commit)
+    project.commit(id)
   end
 
   def files_count(commit)

lib/api/commits.rb

@@ -32,7 +32,7 @@ module API
       #   GET /projects/:id/repository/commits/:sha
       get ":id/repository/commits/:sha" do
         sha = params[:sha]
-        commit = user_project.repository.commit(sha)
+        commit = user_project.commit(sha)
         not_found! "Commit" unless commit
         present commit, with: Entities::RepoCommitDetail
       end
@@ -46,7 +46,7 @@ module API
       #   GET /projects/:id/repository/commits/:sha/diff
       get ":id/repository/commits/:sha/diff" do
         sha = params[:sha]
-        commit = user_project.repository.commit(sha)
+        commit = user_project.commit(sha)
         not_found! "Commit" unless commit
         commit.diffs
       end
@@ -60,7 +60,7 @@ module API
       #   GET /projects/:id/repository/commits/:sha/comments
       get ':id/repository/commits/:sha/comments' do
         sha = params[:sha]
-        commit = user_project.repository.commit(sha)
+        commit = user_project.commit(sha)
         not_found! 'Commit' unless commit
         notes = Note.where(commit_id: commit.id)
         present paginate(notes), with: Entities::CommitNote
@@ -81,7 +81,7 @@ module API
         required_attributes! [:note]
 
         sha = params[:sha]
-        commit = user_project.repository.commit(sha)
+        commit = user_project.commit(sha)
         not_found! 'Commit' unless commit
         opts = {
           note: params[:note],

lib/api/entities.rb

@@ -251,11 +251,11 @@ module API
 
     class Compare < Grape::Entity
       expose :commit, using: Entities::RepoCommit do |compare, options|
-        Commit.decorate(compare.commits).last
+        Commit.decorate(compare.commits, nil).last
       end
 
       expose :commits, using: Entities::RepoCommit do |compare, options|
-        Commit.decorate(compare.commits)
+        Commit.decorate(compare.commits, nil)
       end
 
       expose :diffs, using: Entities::RepoDiff do |compare, options|

lib/api/files.rb

@@ -34,7 +34,7 @@ module API
         ref = attrs.delete(:ref)
         file_path = attrs.delete(:file_path)
 
-        commit = user_project.repository.commit(ref)
+        commit = user_project.commit(ref)
         not_found! 'Commit' unless commit
 
         blob = user_project.repository.blob_at(commit.sha, file_path)

lib/api/repositories.rb

@@ -62,7 +62,7 @@ module API
         ref = params[:ref_name] || user_project.try(:default_branch) || 'master'
         path = params[:path] || nil
 
-        commit = user_project.repository.commit(ref)
+        commit = user_project.commit(ref)
         not_found!('Tree') unless commit
 
         tree = user_project.repository.tree(commit.id, path)

lib/gitlab/identifier.rb

@@ -5,7 +5,7 @@ module Gitlab
     def identify(identifier, project, newrev)
       if identifier.blank?
         # Local push from gitlab
-        email = project.repository.commit(newrev).author_email rescue nil
+        email = project.commit(newrev).author_email rescue nil
         User.find_by(email: email) if email
 
       elsif identifier =~ /\Auser-\d+\Z/

lib/gitlab/markdown/commit_range_reference_filter.rb

@@ -84,7 +84,7 @@ module Gitlab
 
       def commit(id)
         unless @commit_map[id]
-          @commit_map[id] = project.repository.commit(id)
+          @commit_map[id] = project.commit(id)
         end
 
         @commit_map[id]

lib/gitlab/markdown/commit_reference_filter.rb

@@ -66,7 +66,7 @@ module Gitlab
 
       def commit_from_ref(project, commit_ref)
         if project && project.valid_repo?
-          project.repository.commit(commit_ref)
+          project.commit(commit_ref)
         end
       end
 

lib/gitlab/note_data_builder.rb

@@ -69,8 +69,8 @@ module Gitlab
 
       def build_data_for_commit(project, user, note)
         # commit_id is the SHA hash
-        commit = project.repository.commit(note.commit_id)
-        commit.hook_attrs(project)
+        commit = project.commit(note.commit_id)
+        commit.hook_attrs
       end
     end
   end

lib/gitlab/push_data_builder.rb

@@ -30,9 +30,7 @@ module Gitlab
         
         # For performance purposes maximum 20 latest commits
         # will be passed as post receive hook data.
-        commit_attrs = commits_limited.map do |commit|
-          commit.hook_attrs(project)
-        end
+        commit_attrs = commits_limited.map(&:hook_attrs)
 
         type = Gitlab::Git.tag_ref?(ref) ? "tag_push" : "push"
         # Hash to be passed as post_receive_data

spec/controllers/commit_controller_spec.rb

@@ -3,7 +3,7 @@ require 'spec_helper'
 describe Projects::CommitController do
   let(:project) { create(:project) }
   let(:user)    { create(:user) }
-  let(:commit)  { project.repository.commit("master") }
+  let(:commit)  { project.commit("master") }
 
   before do
     sign_in(user)

spec/features/gitlab_flavored_markdown_spec.rb

@@ -14,7 +14,7 @@ describe "GitLab Flavored Markdown", feature: true do
     Commit.any_instance.stub(title: "fix ##{issue.iid}\n\nask @#{fred.username} for details")
   end
 
-  let(:commit) { project.repository.commit }
+  let(:commit) { project.commit }
 
   before do
     login_as :user

spec/helpers/diff_helper_spec.rb

@@ -4,7 +4,7 @@ describe DiffHelper do
   include RepoHelpers
 
   let(:project) { create(:project) }
-  let(:commit) { project.repository.commit(sample_commit.id) }
+  let(:commit) { project.commit(sample_commit.id) }
   let(:diff) { commit.diffs.first }
   let(:diff_file) { Gitlab::Diff::File.new(diff) }
 

spec/helpers/gitlab_markdown_helper_spec.rb

@@ -6,7 +6,7 @@ describe GitlabMarkdownHelper do
   let!(:project) { create(:project) }
 
   let(:user)          { create(:user, username: 'gfm') }
-  let(:commit)        { project.repository.commit }
+  let(:commit)        { project.commit }
   let(:issue)         { create(:issue, project: project) }
   let(:merge_request) { create(:merge_request, source_project: project, target_project: project) }
   let(:snippet)       { create(:project_snippet, project: project) }

spec/helpers/tree_helper_spec.rb

@@ -6,7 +6,7 @@ describe TreeHelper do
 
     before {
       @repository = project.repository
-      @commit = project.repository.commit("e56497bb")
+      @commit = project.commit("e56497bb")
     }
 
     context "on a directory containing more than one file/directory" do

spec/lib/gitlab/diff/file_spec.rb

@@ -4,7 +4,7 @@ describe Gitlab::Diff::File do
   include RepoHelpers
 
   let(:project) { create(:project) }
-  let(:commit) { project.repository.commit(sample_commit.id) }
+  let(:commit) { project.commit(sample_commit.id) }
   let(:diff) { commit.diffs.first }
   let(:diff_file) { Gitlab::Diff::File.new(diff) }
 

spec/lib/gitlab/diff/parser_spec.rb

@@ -4,7 +4,7 @@ describe Gitlab::Diff::Parser do
   include RepoHelpers
 
   let(:project) { create(:project) }
-  let(:commit) { project.repository.commit(sample_commit.id) }
+  let(:commit) { project.commit(sample_commit.id) }
   let(:diff) { commit.diffs.first }
   let(:parser) { Gitlab::Diff::Parser.new }
 

spec/lib/gitlab/markdown/commit_range_reference_filter_spec.rb

@@ -5,8 +5,8 @@ module Gitlab::Markdown
     include ReferenceFilterSpecHelper
 
     let(:project) { create(:project) }
-    let(:commit1) { project.repository.commit }
-    let(:commit2) { project.repository.commit("HEAD~2") }
+    let(:commit1) { project.commit }
+    let(:commit2) { project.commit("HEAD~2") }
 
     it 'requires project context' do
       expect { described_class.call('Commit Range 1c002d..d200c1', {}) }.
@@ -86,8 +86,8 @@ module Gitlab::Markdown
     context 'cross-project reference' do
       let(:namespace) { create(:namespace, name: 'cross-reference') }
       let(:project2)  { create(:project, namespace: namespace) }
-      let(:commit1)   { project.repository.commit }
-      let(:commit2)   { project.repository.commit("HEAD~2") }
+      let(:commit1)   { project.commit }
+      let(:commit2)   { project.commit("HEAD~2") }
       let(:reference) { "#{project2.path_with_namespace}@#{commit1.id}...#{commit2.id}" }
 
       context 'when user can access reference' do

spec/lib/gitlab/markdown/commit_reference_filter_spec.rb

@@ -5,7 +5,7 @@ module Gitlab::Markdown
     include ReferenceFilterSpecHelper
 
     let(:project) { create(:project) }
-    let(:commit)  { project.repository.commit }
+    let(:commit)  { project.commit }
 
     it 'requires project context' do
       expect { described_class.call('Commit 1c002d', {}) }.
@@ -80,7 +80,7 @@ module Gitlab::Markdown
     context 'cross-project reference' do
       let(:namespace) { create(:namespace, name: 'cross-reference') }
       let(:project2)  { create(:project, namespace: namespace) }
-      let(:commit)    { project.repository.commit }
+      let(:commit)    { project.commit }
       let(:reference) { "#{project2.path_with_namespace}@#{commit.id}" }
 
       context 'when user can access reference' do

spec/lib/gitlab/reference_extractor_spec.rb

@@ -125,7 +125,7 @@ describe Gitlab::ReferenceExtractor do
   end
 
   it 'accesses valid commits' do
-    commit = project.repository.commit('master')
+    commit = project.commit('master')
 
     subject.analyze("this references commits #{commit.sha[0..6]} and 012345")
     extracted = subject.commits
@@ -135,8 +135,8 @@ describe Gitlab::ReferenceExtractor do
   end
 
   it 'accesses valid commit ranges' do
-    commit = project.repository.commit('master')
-    earlier_commit = project.repository.commit('master~2')
+    commit = project.commit('master')
+    earlier_commit = project.commit('master~2')
 
     subject.analyze("this references commits #{earlier_commit.sha[0..6]}...#{commit.sha[0..6]}")
     extracted = subject.commit_ranges

spec/mailers/notify_spec.rb

@@ -466,7 +466,7 @@ describe Notify do
       end
 
       describe 'on a commit' do
-        let(:commit) { project.repository.commit }
+        let(:commit) { project.commit }
 
         before(:each) { allow(note).to receive(:noteable).and_return(commit) }
 
@@ -670,8 +670,8 @@ describe Notify do
     let(:example_site_path) { root_path }
     let(:user) { create(:user) }
     let(:compare) { Gitlab::Git::Compare.new(project.repository.raw_repository, sample_image_commit.id, sample_commit.id) }
-    let(:commits) { Commit.decorate(compare.commits) }
-    let(:diff_path) { namespace_project_compare_path(project.namespace, project, from: Commit.new(compare.base), to: Commit.new(compare.head)) }
+    let(:commits) { Commit.decorate(compare.commits, nil) }
+    let(:diff_path) { namespace_project_compare_path(project.namespace, project, from: Commit.new(compare.base, project), to: Commit.new(compare.head, project)) }
     let(:send_from_committer_email) { false }
 
     subject { Notify.repository_push_email(project.id, 'devs@company.name', author_id: user.id, ref: 'refs/heads/master', action: :push, compare: compare, reverse_compare: false, send_from_committer_email: send_from_committer_email) }
@@ -774,7 +774,7 @@ describe Notify do
     let(:example_site_path) { root_path }
     let(:user) { create(:user) }
     let(:compare) { Gitlab::Git::Compare.new(project.repository.raw_repository, sample_commit.parent_id, sample_commit.id) }
-    let(:commits) { Commit.decorate(compare.commits) }
+    let(:commits) { Commit.decorate(compare.commits, nil) }
     let(:diff_path) { namespace_project_commit_path(project.namespace, project, commits.first) }
 
     subject { Notify.repository_push_email(project.id, 'devs@company.name', author_id: user.id, ref: 'refs/heads/master', action: :push, compare: compare) }

spec/models/commit_spec.rb

@@ -2,7 +2,7 @@ require 'spec_helper'
 
 describe Commit do
   let(:project) { create :project }
-  let(:commit) { project.repository.commit }
+  let(:commit) { project.commit }
 
   describe '#title' do
     it "returns no_commit_message when safe_message is blank" do
@@ -58,13 +58,13 @@ eos
 
     it 'detects issues that this commit is marked as closing' do
       commit.stub(safe_message: "Fixes ##{issue.iid}")
-      expect(commit.closes_issues(project)).to eq([issue])
+      expect(commit.closes_issues).to eq([issue])
     end
 
     it 'does not detect issues from other projects' do
       ext_ref = "#{other_project.path_with_namespace}##{other_issue.iid}"
       commit.stub(safe_message: "Fixes #{ext_ref}")
-      expect(commit.closes_issues(project)).to be_empty
+      expect(commit.closes_issues).to be_empty
     end
   end
 

spec/models/note_spec.rb

@@ -329,13 +329,13 @@ describe Note do
     let(:author)     { create(:user) }
     let(:issue)      { create(:issue, project: project) }
     let(:mergereq)   { create(:merge_request, :simple, target_project: project, source_project: project) }
-    let(:commit)     { project.repository.commit }
+    let(:commit)     { project.commit }
 
     # Test all of {issue, merge request, commit} in both the referenced and referencing
     # roles, to ensure that the correct information can be inferred from any argument.
 
     context 'issue from a merge request' do
-      subject { Note.create_cross_reference_note(issue, mergereq, author, project) }
+      subject { Note.create_cross_reference_note(issue, mergereq, author) }
 
       it { is_expected.to be_valid }
 
@@ -361,7 +361,7 @@ describe Note do
     end
 
     context 'issue from a commit' do
-      subject { Note.create_cross_reference_note(issue, commit, author, project) }
+      subject { Note.create_cross_reference_note(issue, commit, author) }
 
       it { is_expected.to be_valid }
 
@@ -377,7 +377,7 @@ describe Note do
     end
 
     context 'merge request from an issue' do
-      subject { Note.create_cross_reference_note(mergereq, issue, author, project) }
+      subject { Note.create_cross_reference_note(mergereq, issue, author) }
 
       it { is_expected.to be_valid }
 
@@ -398,7 +398,7 @@ describe Note do
     end
 
     context 'commit from a merge request' do
-      subject { Note.create_cross_reference_note(commit, mergereq, author, project) }
+      subject { Note.create_cross_reference_note(commit, mergereq, author) }
 
       it { is_expected.to be_valid }
 
@@ -419,13 +419,13 @@ describe Note do
     end
 
     context 'commit contained in a merge request' do
-      subject { Note.create_cross_reference_note(mergereq.commits.first, mergereq, author, project) }
+      subject { Note.create_cross_reference_note(mergereq.commits.first, mergereq, author) }
 
       it { is_expected.to be_nil }
     end
 
     context 'commit from issue' do
-      subject { Note.create_cross_reference_note(commit, issue, author, project) }
+      subject { Note.create_cross_reference_note(commit, issue, author) }
 
       it { is_expected.to be_valid }
 
@@ -452,7 +452,7 @@ describe Note do
 
     context 'commit from commit' do
       let(:parent_commit) { commit.parents.first }
-      subject { Note.create_cross_reference_note(commit, parent_commit, author, project) }
+      subject { Note.create_cross_reference_note(commit, parent_commit, author) }
 
       it { is_expected.to be_valid }
 
@@ -482,11 +482,11 @@ describe Note do
     let(:project) { create :project }
     let(:author) { create :user }
     let(:issue) { create :issue }
-    let(:commit0) { project.repository.commit }
-    let(:commit1) { project.repository.commit('HEAD~2') }
+    let(:commit0) { project.commit }
+    let(:commit1) { project.commit('HEAD~2') }
 
     before do
-      Note.create_cross_reference_note(issue, commit0, author, project)
+      Note.create_cross_reference_note(issue, commit0, author)
     end
 
     it 'detects if a mentionable has already been mentioned' do
@@ -499,7 +499,7 @@ describe Note do
 
     context 'commit on commit' do
       before do
-        Note.create_cross_reference_note(commit0, commit1, author, project)
+        Note.create_cross_reference_note(commit0, commit1, author)
       end
 
       it { expect(Note.cross_reference_exists?(commit0, commit1)).to be_truthy }
@@ -527,7 +527,7 @@ describe Note do
     let(:author) { create :user }
     let(:issue0) { create :issue, project: project }
     let(:issue1) { create :issue, project: second_project }
-    let!(:note) { Note.create_cross_reference_note(issue0, issue1, author, project) }
+    let!(:note) { Note.create_cross_reference_note(issue0, issue1, author) }
 
     it 'detects if a mentionable has already been mentioned' do
       expect(Note.cross_reference_exists?(issue0, issue1)).to be_truthy
@@ -562,7 +562,7 @@ describe Note do
     end
 
     it 'should identify cross-reference notes as system notes' do
-      @note = Note.create_cross_reference_note(issue, other, author, project)
+      @note = Note.create_cross_reference_note(issue, other, author)
       expect(@note).to be_system
     end
 

spec/services/git_push_service_spec.rb

@@ -44,7 +44,7 @@ describe GitPushService do
     before do
       service.execute(project, user, @oldrev, @newrev, @ref)
       @push_data = service.push_data
-      @commit = project.repository.commit(@newrev)
+      @commit = project.commit(@newrev)
     end
 
     subject { @push_data }
@@ -151,7 +151,7 @@ describe GitPushService do
   describe "cross-reference notes" do
     let(:issue) { create :issue, project: project }
     let(:commit_author) { create :user }
-    let(:commit) { project.repository.commit }
+    let(:commit) { project.commit }
 
     before do
       commit.stub({
@@ -164,22 +164,22 @@ describe GitPushService do
     end
 
     it "creates a note if a pushed commit mentions an issue" do
-      expect(Note).to receive(:create_cross_reference_note).with(issue, commit, commit_author, project)
+      expect(Note).to receive(:create_cross_reference_note).with(issue, commit, commit_author)
 
       service.execute(project, user, @oldrev, @newrev, @ref)
     end
 
     it "only creates a cross-reference note if one doesn't already exist" do
-      Note.create_cross_reference_note(issue, commit, user, project)
+      Note.create_cross_reference_note(issue, commit, user)
 
-      expect(Note).not_to receive(:create_cross_reference_note).with(issue, commit, commit_author, project)
+      expect(Note).not_to receive(:create_cross_reference_note).with(issue, commit, commit_author)
 
       service.execute(project, user, @oldrev, @newrev, @ref)
     end
 
     it "defaults to the pushing user if the commit's author is not known" do
       commit.stub(author_name: 'unknown name', author_email: 'unknown@email.com')
-      expect(Note).to receive(:create_cross_reference_note).with(issue, commit, user, project)
+      expect(Note).to receive(:create_cross_reference_note).with(issue, commit, user)
 
       service.execute(project, user, @oldrev, @newrev, @ref)
     end
@@ -188,7 +188,7 @@ describe GitPushService do
       allow(project.repository).to receive(:commits_between).with(@blankrev, @newrev).and_return([])
       allow(project.repository).to receive(:commits_between).with("master", @newrev).and_return([commit])
 
-      expect(Note).to receive(:create_cross_reference_note).with(issue, commit, commit_author, project)
+      expect(Note).to receive(:create_cross_reference_note).with(issue, commit, commit_author)
 
       service.execute(project, user, @blankrev, @newrev, 'refs/heads/other')
     end
@@ -198,7 +198,7 @@ describe GitPushService do
     let(:issue) { create :issue, project: project }
     let(:other_issue) { create :issue, project: project }
     let(:commit_author) { create :user }
-    let(:closing_commit) { project.repository.commit }
+    let(:closing_commit) { project.commit }
 
     before do
       closing_commit.stub({

spec/services/git_tag_push_service_spec.rb

@@ -19,7 +19,7 @@ describe GitTagPushService do
       @push_data = service.push_data
       @tag_name = Gitlab::Git.ref_name(@ref)
       @tag = project.repository.find_tag(@tag_name)
-      @commit = project.repository.commit(@tag.target)
+      @commit = project.commit(@tag.target)
     end
 
     subject { @push_data }

spec/services/notification_service_spec.rb

@@ -57,7 +57,7 @@ describe NotificationService do
         end
 
         it 'filters out "mentioned in" notes' do
-          mentioned_note = Note.create_cross_reference_note(mentioned_issue, issue, issue.author, issue.project)
+          mentioned_note = Note.create_cross_reference_note(mentioned_issue, issue, issue.author)
 
           expect(Notify).not_to receive(:note_issue_email)
           notification.new_note(mentioned_note)
@@ -128,7 +128,7 @@ describe NotificationService do
         end
 
         it 'filters out "mentioned in" notes' do
-          mentioned_note = Note.create_cross_reference_note(mentioned_issue, issue, issue.author, issue.project)
+          mentioned_note = Note.create_cross_reference_note(mentioned_issue, issue, issue.author)
 
           expect(Notify).not_to receive(:note_issue_email)
           notification.new_note(mentioned_note)

spec/support/mentionable_shared_examples.rb

@@ -53,7 +53,7 @@ def common_mentionable_setup
     extra_commits.each { |c| commitmap[c.short_id] = c }
 
     allow(project.repository).to receive(:commit) { |sha| commitmap[sha] }
-
+    
     set_mentionable_text.call(ref_string)
   end
 end
@@ -83,14 +83,14 @@ shared_examples 'a mentionable' do
 
     mentioned_objects.each do |referenced|
       expect(Note).to receive(:create_cross_reference_note).
-        with(referenced, subject.local_reference, author, project)
+        with(referenced, subject.local_reference, author)
     end
 
     subject.create_cross_references!(project, author)
   end
 
   it 'detects existing cross-references' do
-    Note.create_cross_reference_note(mentioned_issue, subject.local_reference, author, project)
+    Note.create_cross_reference_note(mentioned_issue, subject.local_reference, author)
 
     expect(subject).to have_mentioned(mentioned_issue)
     expect(subject).not_to have_mentioned(mentioned_mr)
@@ -107,6 +107,8 @@ shared_examples 'an editable mentionable' do
   end
 
   it 'creates new cross-reference notes when the mentionable text is edited' do
+    subject.save
+
     cross = ext_proj.path_with_namespace
 
     new_text = <<-MSG
@@ -132,10 +134,9 @@ shared_examples 'an editable mentionable' do
     # These two issues are new and should receive reference notes
     new_issues.each do |newref|
       expect(Note).to receive(:create_cross_reference_note).
-        with(newref, subject.local_reference, author, project)
+        with(newref, subject.local_reference, author)
     end
 
-    subject.save
     set_mentionable_text.call(new_text)
     subject.notice_added_references(project, author)
   end