From 84a1590252c63c710bceaa7a394799cdc5109505 Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Tue, 21 Apr 2015 15:09:15 +0200 Subject: Let commit model know about its project. --- app/mailers/emails/projects.rb | 6 +++--- app/models/commit.rb | 11 +++++++---- app/models/merge_request_diff.rb | 4 ++-- app/models/project.rb | 6 +++++- app/models/project_wiki.rb | 2 +- app/models/repository.rb | 11 ++++++----- app/services/merge_requests/build_service.rb | 2 +- app/views/projects/blame/show.html.haml | 2 +- app/views/projects/commits/_commit_list.html.haml | 4 ++-- app/workers/irker_worker.rb | 3 +-- lib/api/entities.rb | 4 ++-- spec/mailers/notify_spec.rb | 6 +++--- 12 files changed, 34 insertions(+), 27 deletions(-) diff --git a/app/mailers/emails/projects.rb b/app/mailers/emails/projects.rb index 0dbb2939bb3..9cb7077e59d 100644 --- a/app/mailers/emails/projects.rb +++ b/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 diff --git a/app/models/commit.rb b/app/models/commit.rb index 1cabc060c2a..d4e9ebacac6 100644 --- a/app/models/commit.rb +++ b/app/models/commit.rb @@ -6,6 +6,8 @@ class Commit attr_mentionable :safe_message + 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 +20,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 +43,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 @@ -169,6 +172,6 @@ class Commit end def parents - @parents ||= Commit.decorate(super) + @parents ||= Commit.decorate(super, project) end end diff --git a/app/models/merge_request_diff.rb b/app/models/merge_request_diff.rb index acac1ca4cf7..df1c2b78758 100644 --- a/app/models/merge_request_diff.rb +++ b/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 diff --git a/app/models/project.rb b/app/models/project.rb index 64ee2c2212b..4c1404ee9f8 100644 --- a/app/models/project.rb +++ b/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) + repository.commit(id) end def saved? diff --git a/app/models/project_wiki.rb b/app/models/project_wiki.rb index 772c868d9cd..0706a1ca0d1 100644 --- a/app/models/project_wiki.rb +++ b/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 diff --git a/app/models/repository.rb b/app/models/repository.rb index 263a436d521..1b8c74028d9 100644 --- a/app/models/repository.rb +++ b/app/models/repository.rb @@ -1,11 +1,12 @@ 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 diff --git a/app/services/merge_requests/build_service.rb b/app/services/merge_requests/build_service.rb index a44b91166e8..956480938c3 100644 --- a/app/services/merge_requests/build_service.rb +++ b/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 diff --git a/app/views/projects/blame/show.html.haml b/app/views/projects/blame/show.html.haml index e6a859fea8f..89dd68d6471 100644 --- a/app/views/projects/blame/show.html.haml +++ b/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 diff --git a/app/views/projects/commits/_commit_list.html.haml b/app/views/projects/commits/_commit_list.html.haml index 2ee7d73bd20..ce60fbdf032 100644 --- a/app/views/projects/commits/_commit_list.html.haml +++ b/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 diff --git a/app/workers/irker_worker.rb b/app/workers/irker_worker.rb index 8b50f423984..84a54656df2 100644 --- a/app/workers/irker_worker.rb +++ b/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) diff --git a/lib/api/entities.rb b/lib/api/entities.rb index 36332bc6514..79b4afa40ba 100644 --- a/lib/api/entities.rb +++ b/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| diff --git a/spec/mailers/notify_spec.rb b/spec/mailers/notify_spec.rb index b297fbd5119..6f7f5835b94 100644 --- a/spec/mailers/notify_spec.rb +++ b/spec/mailers/notify_spec.rb @@ -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) } -- cgit v1.2.1 From 8ed7ac9d443563a62a6aa03a2ec72b9fcb0d2df1 Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Tue, 21 Apr 2015 15:13:40 +0200 Subject: Use project.commit convenience method. --- app/controllers/projects/commit_controller.rb | 2 +- app/controllers/projects/merge_requests_controller.rb | 2 +- app/models/note.rb | 4 ++-- app/models/project.rb | 2 +- app/models/protected_branch.rb | 2 +- app/services/create_tag_service.rb | 2 +- app/services/git_tag_push_service.rb | 2 +- app/services/projects/participants_service.rb | 2 +- lib/api/commits.rb | 8 ++++---- lib/api/files.rb | 2 +- lib/api/repositories.rb | 2 +- lib/gitlab/identifier.rb | 2 +- lib/gitlab/markdown/commit_range_reference_filter.rb | 2 +- lib/gitlab/markdown/commit_reference_filter.rb | 2 +- lib/gitlab/note_data_builder.rb | 2 +- spec/controllers/commit_controller_spec.rb | 2 +- spec/features/gitlab_flavored_markdown_spec.rb | 2 +- spec/helpers/diff_helper_spec.rb | 2 +- spec/helpers/gitlab_markdown_helper_spec.rb | 2 +- spec/helpers/tree_helper_spec.rb | 2 +- spec/lib/gitlab/diff/file_spec.rb | 2 +- spec/lib/gitlab/diff/parser_spec.rb | 2 +- spec/lib/gitlab/markdown/commit_range_reference_filter_spec.rb | 8 ++++---- spec/lib/gitlab/markdown/commit_reference_filter_spec.rb | 4 ++-- spec/lib/gitlab/reference_extractor_spec.rb | 6 +++--- spec/mailers/notify_spec.rb | 2 +- spec/models/commit_spec.rb | 2 +- spec/models/note_spec.rb | 6 +++--- spec/services/git_push_service_spec.rb | 6 +++--- spec/services/git_tag_push_service_spec.rb | 2 +- 30 files changed, 44 insertions(+), 44 deletions(-) diff --git a/app/controllers/projects/commit_controller.rb b/app/controllers/projects/commit_controller.rb index 8a1b7899be3..35df421e09d 100644 --- a/app/controllers/projects/commit_controller.rb +++ b/app/controllers/projects/commit_controller.rb @@ -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 diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb index ab75f2ddb73..41b4e55a598 100644 --- a/app/controllers/projects/merge_requests_controller.rb +++ b/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 diff --git a/app/models/note.rb b/app/models/note.rb index e3c571a1574..99e86ac0250 100644 --- a/app/models/note.rb +++ b/app/models/note.rb @@ -290,7 +290,7 @@ class Note < ActiveRecord::Base # +mentioner+. def noteable_project_id(noteable, mentioning_project) if noteable.is_a?(Commit) - if mentioning_project.repository.commit(noteable.id) + if mentioning_project.commit(noteable.id) # The noteable commit belongs to the mentioner's project mentioning_project.id else @@ -512,7 +512,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 diff --git a/app/models/project.rb b/app/models/project.rb index 4c1404ee9f8..293ee04f228 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -257,7 +257,7 @@ class Project < ActiveRecord::Base @repository ||= Repository.new(path_with_namespace, nil, self) end - def commit(id) + def commit(id = 'HEAD') repository.commit(id) end diff --git a/app/models/protected_branch.rb b/app/models/protected_branch.rb index 97207ba1272..8ebd790a89e 100644 --- a/app/models/protected_branch.rb +++ b/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 diff --git a/app/services/create_tag_service.rb b/app/services/create_tag_service.rb index 25f9e203246..1a7318048b3 100644 --- a/app/services/create_tag_service.rb +++ b/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 diff --git a/app/services/git_tag_push_service.rb b/app/services/git_tag_push_service.rb index bf203bbd692..075a6118da2 100644 --- a/app/services/git_tag_push_service.rb +++ b/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 diff --git a/app/services/projects/participants_service.rb b/app/services/projects/participants_service.rb index c2d8f48f6e4..05106fa7898 100644 --- a/app/services/projects/participants_service.rb +++ b/app/services/projects/participants_service.rb @@ -22,7 +22,7 @@ module Projects merge_request = project.merge_requests.find_by_iid(id) merge_request.participants(current_user) if merge_request when "Commit" - commit = project.repository.commit(id) + commit = project.commit(id) commit.participants(project, current_user) if commit end diff --git a/lib/api/commits.rb b/lib/api/commits.rb index 0de4e720ffe..23270b1c0f4 100644 --- a/lib/api/commits.rb +++ b/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], diff --git a/lib/api/files.rb b/lib/api/files.rb index 3176ef0e256..e0ea6d7dd1d 100644 --- a/lib/api/files.rb +++ b/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) diff --git a/lib/api/repositories.rb b/lib/api/repositories.rb index 1fbf3dca3c6..2d96c9666d2 100644 --- a/lib/api/repositories.rb +++ b/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) diff --git a/lib/gitlab/identifier.rb b/lib/gitlab/identifier.rb index 6e4de197eeb..3e5d728f3bc 100644 --- a/lib/gitlab/identifier.rb +++ b/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/ diff --git a/lib/gitlab/markdown/commit_range_reference_filter.rb b/lib/gitlab/markdown/commit_range_reference_filter.rb index 1128c1bed7a..baa97bee9bf 100644 --- a/lib/gitlab/markdown/commit_range_reference_filter.rb +++ b/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] diff --git a/lib/gitlab/markdown/commit_reference_filter.rb b/lib/gitlab/markdown/commit_reference_filter.rb index 745de6402cf..66598127f6e 100644 --- a/lib/gitlab/markdown/commit_reference_filter.rb +++ b/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 diff --git a/lib/gitlab/note_data_builder.rb b/lib/gitlab/note_data_builder.rb index 644dec45dca..0f2abd1b49c 100644 --- a/lib/gitlab/note_data_builder.rb +++ b/lib/gitlab/note_data_builder.rb @@ -69,7 +69,7 @@ module Gitlab def build_data_for_commit(project, user, note) # commit_id is the SHA hash - commit = project.repository.commit(note.commit_id) + commit = project.commit(note.commit_id) commit.hook_attrs(project) end end diff --git a/spec/controllers/commit_controller_spec.rb b/spec/controllers/commit_controller_spec.rb index 3394a1f863f..2cfa399a047 100644 --- a/spec/controllers/commit_controller_spec.rb +++ b/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) diff --git a/spec/features/gitlab_flavored_markdown_spec.rb b/spec/features/gitlab_flavored_markdown_spec.rb index fca1a06eb88..133beba7b98 100644 --- a/spec/features/gitlab_flavored_markdown_spec.rb +++ b/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 diff --git a/spec/helpers/diff_helper_spec.rb b/spec/helpers/diff_helper_spec.rb index 5bd09793b11..95719b4b49f 100644 --- a/spec/helpers/diff_helper_spec.rb +++ b/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) } diff --git a/spec/helpers/gitlab_markdown_helper_spec.rb b/spec/helpers/gitlab_markdown_helper_spec.rb index 64f130e4ae4..e309dbb6a2f 100644 --- a/spec/helpers/gitlab_markdown_helper_spec.rb +++ b/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) } diff --git a/spec/helpers/tree_helper_spec.rb b/spec/helpers/tree_helper_spec.rb index 8271e00f41b..2013b3e4c2a 100644 --- a/spec/helpers/tree_helper_spec.rb +++ b/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 diff --git a/spec/lib/gitlab/diff/file_spec.rb b/spec/lib/gitlab/diff/file_spec.rb index 40eb45e37ca..8b7946f3117 100644 --- a/spec/lib/gitlab/diff/file_spec.rb +++ b/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) } diff --git a/spec/lib/gitlab/diff/parser_spec.rb b/spec/lib/gitlab/diff/parser_spec.rb index 918f6d0ead4..4d5d1431683 100644 --- a/spec/lib/gitlab/diff/parser_spec.rb +++ b/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 } diff --git a/spec/lib/gitlab/markdown/commit_range_reference_filter_spec.rb b/spec/lib/gitlab/markdown/commit_range_reference_filter_spec.rb index 5ebdc8926e2..f0804ce0056 100644 --- a/spec/lib/gitlab/markdown/commit_range_reference_filter_spec.rb +++ b/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 diff --git a/spec/lib/gitlab/markdown/commit_reference_filter_spec.rb b/spec/lib/gitlab/markdown/commit_reference_filter_spec.rb index 71fd2db2c58..f792d7f696e 100644 --- a/spec/lib/gitlab/markdown/commit_reference_filter_spec.rb +++ b/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 diff --git a/spec/lib/gitlab/reference_extractor_spec.rb b/spec/lib/gitlab/reference_extractor_spec.rb index 6fba140f69d..0f4ea2a24ed 100644 --- a/spec/lib/gitlab/reference_extractor_spec.rb +++ b/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 diff --git a/spec/mailers/notify_spec.rb b/spec/mailers/notify_spec.rb index 6f7f5835b94..d28b4768545 100644 --- a/spec/mailers/notify_spec.rb +++ b/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) } diff --git a/spec/models/commit_spec.rb b/spec/models/commit_spec.rb index 506bc3339b6..9c37c036eae 100644 --- a/spec/models/commit_spec.rb +++ b/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 diff --git a/spec/models/note_spec.rb b/spec/models/note_spec.rb index e7e6717e775..095eb1500a9 100644 --- a/spec/models/note_spec.rb +++ b/spec/models/note_spec.rb @@ -329,7 +329,7 @@ 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. @@ -482,8 +482,8 @@ 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) diff --git a/spec/services/git_push_service_spec.rb b/spec/services/git_push_service_spec.rb index aa9b15dd9ec..246bc1cc33d 100644 --- a/spec/services/git_push_service_spec.rb +++ b/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({ @@ -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({ diff --git a/spec/services/git_tag_push_service_spec.rb b/spec/services/git_tag_push_service_spec.rb index a050fdf6c0e..76f69b396e0 100644 --- a/spec/services/git_tag_push_service_spec.rb +++ b/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 } -- cgit v1.2.1 From 27af24c1c951385bccd298c98044d57ff22ccd1c Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Tue, 21 Apr 2015 15:15:49 +0200 Subject: No longer needed to pass project argument to commit methods. --- app/controllers/projects/commit_controller.rb | 6 +++--- app/models/commit.rb | 14 +++++++------- app/models/merge_request.rb | 4 ++-- app/services/git_push_service.rb | 2 +- app/services/notification_service.rb | 4 +--- app/services/projects/participants_service.rb | 16 +++++++--------- app/views/projects/commits/_commit.html.haml | 2 +- lib/gitlab/note_data_builder.rb | 2 +- lib/gitlab/push_data_builder.rb | 3 +-- spec/models/commit_spec.rb | 4 ++-- 10 files changed, 26 insertions(+), 31 deletions(-) diff --git a/app/controllers/projects/commit_controller.rb b/app/controllers/projects/commit_controller.rb index 35df421e09d..78d42d695b6 100644 --- a/app/controllers/projects/commit_controller.rb +++ b/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 = { diff --git a/app/models/commit.rb b/app/models/commit.rb index d4e9ebacac6..1985793c600 100644 --- a/app/models/commit.rb +++ b/app/models/commit.rb @@ -103,7 +103,7 @@ class Commit description.present? end - def hook_attrs(project) + def hook_attrs path_with_namespace = project.path_with_namespace { @@ -120,7 +120,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 @@ -137,22 +137,22 @@ class Commit User.find_for_commit(committer_email, committer_name) end - def participants(project, current_user = nil) + def participants(current_user = nil) users = [] users << author users << committer - users.push *self.mentioned_users(current_user, project) + users.push *self.mentioned_users(current_user) - notes(project).each do |note| + notes.each do |note| users << note.author - users.push *note.mentioned_users(current_user, project) + users.push *note.mentioned_users(current_user) end users.uniq end - def notes(project) + def notes project.notes.for_commit_id(self.id) end diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index 9c9e2762507..e242cae8ea1 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -242,7 +242,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 +259,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) diff --git a/app/services/git_push_service.rb b/app/services/git_push_service.rb index 31e0167d247..3affd6d6463 100644 --- a/app/services/git_push_service.rb +++ b/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 diff --git a/app/services/notification_service.rb b/app/services/notification_service.rb index c7e45a2c2c7..0d7ffbeebd9 100644 --- a/app/services/notification_service.rb +++ b/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 diff --git a/app/services/projects/participants_service.rb b/app/services/projects/participants_service.rb index 05106fa7898..b91590a1a90 100644 --- a/app/services/projects/participants_service.rb +++ b/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.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 diff --git a/app/views/projects/commits/_commit.html.haml b/app/views/projects/commits/_commit.html.haml index 8e1aaa4d051..083fca9b658 100644 --- a/app/views/projects/commits/_commit.html.haml +++ b/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 diff --git a/lib/gitlab/note_data_builder.rb b/lib/gitlab/note_data_builder.rb index 0f2abd1b49c..ea6b0ee796d 100644 --- a/lib/gitlab/note_data_builder.rb +++ b/lib/gitlab/note_data_builder.rb @@ -70,7 +70,7 @@ module Gitlab def build_data_for_commit(project, user, note) # commit_id is the SHA hash commit = project.commit(note.commit_id) - commit.hook_attrs(project) + commit.hook_attrs end end end diff --git a/lib/gitlab/push_data_builder.rb b/lib/gitlab/push_data_builder.rb index f8da452e4c0..f44eb872122 100644 --- a/lib/gitlab/push_data_builder.rb +++ b/lib/gitlab/push_data_builder.rb @@ -30,8 +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) + commit_attrs = commits_limited.map(&:hook_attrs) end type = Gitlab::Git.tag_ref?(ref) ? "tag_push" : "push" diff --git a/spec/models/commit_spec.rb b/spec/models/commit_spec.rb index 9c37c036eae..ad2ac143d97 100644 --- a/spec/models/commit_spec.rb +++ b/spec/models/commit_spec.rb @@ -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 -- cgit v1.2.1 From 0ff778c0f4a3b599d0f36a1deee5607d03e52b9a Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Tue, 21 Apr 2015 15:19:23 +0200 Subject: Link cross-project cross-reference notes to correct project. --- app/models/concerns/mentionable.rb | 2 +- app/models/note.rb | 46 +++++++---------------------- app/services/git_push_service.rb | 2 +- app/services/notes/create_service.rb | 2 +- app/services/notes/update_service.rb | 3 +- spec/models/note_spec.rb | 22 +++++++------- spec/services/git_push_service_spec.rb | 10 +++---- spec/services/notification_service_spec.rb | 4 +-- spec/support/mentionable_shared_examples.rb | 6 ++-- 9 files changed, 36 insertions(+), 61 deletions(-) diff --git a/app/models/concerns/mentionable.rb b/app/models/concerns/mentionable.rb index aad5e514793..3ef3e8b67d8 100644 --- a/app/models/concerns/mentionable.rb +++ b/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 diff --git a/app/models/note.rb b/app/models/note.rb index 99e86ac0250..26739426a88 100644 --- a/app/models/note.rb +++ b/app/models/note.rb @@ -77,11 +77,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 +236,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, nil) notes = if noteable.is_a?(Commit) where(commit_id: noteable.id, noteable_type: 'Commit') else @@ -269,43 +269,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.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, mentioner_project = mentioner.project) + if mentioner.is_a?(Commit) && project.nil? + 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.id == noteable_project mentioner.gfm_reference else if mentioner.is_a?(Commit) diff --git a/app/services/git_push_service.rb b/app/services/git_push_service.rb index 3affd6d6463..1b889e0da8b 100644 --- a/app/services/git_push_service.rb +++ b/app/services/git_push_service.rb @@ -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 diff --git a/app/services/notes/create_service.rb b/app/services/notes/create_service.rb index e969061f229..d19a6c2eca3 100644 --- a/app/services/notes/create_service.rb +++ b/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) diff --git a/app/services/notes/update_service.rb b/app/services/notes/update_service.rb index 63431b82471..45a0db761ec 100644 --- a/app/services/notes/update_service.rb +++ b/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 diff --git a/spec/models/note_spec.rb b/spec/models/note_spec.rb index 095eb1500a9..4a6bfdb2910 100644 --- a/spec/models/note_spec.rb +++ b/spec/models/note_spec.rb @@ -335,7 +335,7 @@ describe Note do # 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 } @@ -486,7 +486,7 @@ describe Note do 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 diff --git a/spec/services/git_push_service_spec.rb b/spec/services/git_push_service_spec.rb index 246bc1cc33d..af37e8319a4 100644 --- a/spec/services/git_push_service_spec.rb +++ b/spec/services/git_push_service_spec.rb @@ -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 diff --git a/spec/services/notification_service_spec.rb b/spec/services/notification_service_spec.rb index 6d2bc41c2b9..2a54b2e920a 100644 --- a/spec/services/notification_service_spec.rb +++ b/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) diff --git a/spec/support/mentionable_shared_examples.rb b/spec/support/mentionable_shared_examples.rb index 25ec5cbe6f2..dde80b1e1dd 100644 --- a/spec/support/mentionable_shared_examples.rb +++ b/spec/support/mentionable_shared_examples.rb @@ -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) @@ -132,7 +132,7 @@ 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 -- cgit v1.2.1 From e739eb036df23db4a03681190bf07ba0b8f1302c Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Tue, 21 Apr 2015 15:23:20 +0200 Subject: Move participants method to shared Participable concern. --- app/models/commit.rb | 17 ++------------- app/models/concerns/issuable.rb | 18 ++-------------- app/models/concerns/participable.rb | 42 +++++++++++++++++++++++++++++++++++++ app/models/note.rb | 2 ++ app/models/snippet.rb | 15 +++---------- 5 files changed, 51 insertions(+), 43 deletions(-) create mode 100644 app/models/concerns/participable.rb diff --git a/app/models/commit.rb b/app/models/commit.rb index 1985793c600..be5a118bfec 100644 --- a/app/models/commit.rb +++ b/app/models/commit.rb @@ -3,8 +3,10 @@ class Commit include StaticModel extend ActiveModel::Naming include Mentionable + include Participable attr_mentionable :safe_message + participant :author, :committer, :notes, :mentioned_users attr_accessor :project @@ -137,21 +139,6 @@ class Commit User.find_for_commit(committer_email, committer_name) end - def participants(current_user = nil) - users = [] - users << author - users << committer - - 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 notes project.notes.for_commit_id(self.id) end diff --git a/app/models/concerns/issuable.rb b/app/models/concerns/issuable.rb index a21d9bdfe8a..97846b06d72 100644 --- a/app/models/concerns/issuable.rb +++ b/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) diff --git a/app/models/concerns/participable.rb b/app/models/concerns/participable.rb new file mode 100644 index 00000000000..d78810f2eab --- /dev/null +++ b/app/models/concerns/participable.rb @@ -0,0 +1,42 @@ +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 Array + value.flat_map { |v| participants_for(v, current_user) } + when Participable + value.participants(current_user) + when Mentionable + value.mentioned_users(current_user) + end + end +end diff --git a/app/models/note.rb b/app/models/note.rb index 26739426a88..e1cd971132b 100644 --- a/app/models/note.rb +++ b/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 diff --git a/app/models/snippet.rb b/app/models/snippet.rb index c11c28805eb..d2af26539b6 100644 --- a/app/models/snippet.rb +++ b/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}%") -- cgit v1.2.1 From 4f2e4c76561ba5b921b16d0ba4245f594c399e06 Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Tue, 21 Apr 2015 15:38:14 +0200 Subject: Select MR commit notes from source project. --- app/models/merge_request.rb | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index e242cae8ea1..49a00697ee1 100644 --- a/app/models/merge_request.rb +++ b/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 -- cgit v1.2.1 From 4dd2f881336d2be6086019477d7927d0a1661002 Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Tue, 21 Apr 2015 15:42:30 +0200 Subject: Update changelog. --- CHANGELOG | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGELOG b/CHANGELOG index 135603d2b11..0d6b0f2d942 100644 --- a/CHANGELOG +++ b/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. - - - -- cgit v1.2.1 From 2456695422f01c677cf287f0021d01a08dd60c30 Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Tue, 21 Apr 2015 17:07:21 +0200 Subject: Fix errors. --- app/models/note.rb | 4 ++-- lib/gitlab/push_data_builder.rb | 1 - 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/app/models/note.rb b/app/models/note.rb index e1cd971132b..f2a9680ff57 100644 --- a/app/models/note.rb +++ b/app/models/note.rb @@ -272,7 +272,7 @@ class Note < ActiveRecord::Base # cross-project references. For same-project references, return the # unmodified GFM reference. def mentioner_gfm_ref(noteable, mentioner, mentioner_project = mentioner.project) - if mentioner.is_a?(Commit) && project.nil? + if mentioner.is_a?(Commit) && mentioner_project.nil? return mentioner.gfm_reference.sub('commit ', 'commit %') end @@ -283,7 +283,7 @@ class Note < ActiveRecord::Base # projects are not the same, add the mentioning project's path to the # returned value. def full_gfm_reference(mentioning_project, noteable_project, mentioner) - if mentioning_project.id == noteable_project + if mentioning_project == noteable_project mentioner.gfm_reference else if mentioner.is_a?(Commit) diff --git a/lib/gitlab/push_data_builder.rb b/lib/gitlab/push_data_builder.rb index f44eb872122..f97784f5abb 100644 --- a/lib/gitlab/push_data_builder.rb +++ b/lib/gitlab/push_data_builder.rb @@ -31,7 +31,6 @@ module Gitlab # For performance purposes maximum 20 latest commits # will be passed as post receive hook data. commit_attrs = commits_limited.map(&:hook_attrs) - end type = Gitlab::Git.tag_ref?(ref) ? "tag_push" : "push" # Hash to be passed as post_receive_data -- cgit v1.2.1 From 0db79443eeaca358f74793f0dafe52f52c5b4497 Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Tue, 21 Apr 2015 17:41:18 +0200 Subject: Satisfy Rubocop. --- app/models/concerns/participable.rb | 23 ++++++++++++----------- 1 file changed, 12 insertions(+), 11 deletions(-) diff --git a/app/models/concerns/participable.rb b/app/models/concerns/participable.rb index d78810f2eab..f59dbc471ac 100644 --- a/app/models/concerns/participable.rb +++ b/app/models/concerns/participable.rb @@ -27,16 +27,17 @@ module Participable end private - def participants_for(value, current_user = nil) - case value - when User - [value] - when Array - value.flat_map { |v| participants_for(v, current_user) } - when Participable - value.participants(current_user) - when Mentionable - value.mentioned_users(current_user) - end + + def participants_for(value, current_user = nil) + case value + when User + [value] + when Array + value.flat_map { |v| participants_for(v, current_user) } + when Participable + value.participants(current_user) + when Mentionable + value.mentioned_users(current_user) end + end end -- cgit v1.2.1 From 889832578afcf668a15deca1fe3c84b9498bcd1d Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Tue, 21 Apr 2015 18:38:26 +0200 Subject: Fix small issues. --- app/models/note.rb | 8 ++++---- spec/support/mentionable_shared_examples.rb | 5 +++-- 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/app/models/note.rb b/app/models/note.rb index f2a9680ff57..cbce6786683 100644 --- a/app/models/note.rb +++ b/app/models/note.rb @@ -238,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, nil) + gfm_reference = mentioner_gfm_ref(noteable, mentioner, true) notes = if noteable.is_a?(Commit) where(commit_id: noteable.id, noteable_type: 'Commit') else @@ -271,12 +271,12 @@ 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, mentioner_project = mentioner.project) - if mentioner.is_a?(Commit) && mentioner_project.nil? + 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) + full_gfm_reference(mentioner.project, noteable.project, mentioner) end # Return the +mentioner+ GFM reference. If the mentioner and noteable diff --git a/spec/support/mentionable_shared_examples.rb b/spec/support/mentionable_shared_examples.rb index dde80b1e1dd..63800602e01 100644 --- a/spec/support/mentionable_shared_examples.rb +++ b/spec/support/mentionable_shared_examples.rb @@ -52,9 +52,10 @@ def common_mentionable_setup } extra_commits.each { |c| commitmap[c.short_id] = c } - allow(project.repository).to receive(:commit) { |sha| commitmap[sha] } - + allow(project).to receive(:commit) { |sha| commitmap[sha] } + set_mentionable_text.call(ref_string) + subject.save end end -- cgit v1.2.1 From 6f8ff08c65f954707cb426dace802a32a02a04f6 Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Thu, 23 Apr 2015 17:27:03 +0200 Subject: Fix behavior for Enumerable-like ActiveRecord relations. --- app/models/concerns/participable.rb | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/app/models/concerns/participable.rb b/app/models/concerns/participable.rb index f59dbc471ac..a01b28ea8db 100644 --- a/app/models/concerns/participable.rb +++ b/app/models/concerns/participable.rb @@ -32,12 +32,10 @@ module Participable case value when User [value] - when Array + when Enumerable, ActiveRecord::Relation value.flat_map { |v| participants_for(v, current_user) } when Participable value.participants(current_user) - when Mentionable - value.mentioned_users(current_user) end end end -- cgit v1.2.1 From 04020d5e2038e9acd31147c16dc07fb5a2360a24 Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Thu, 23 Apr 2015 17:27:45 +0200 Subject: Explain purpose and usage. --- app/models/concerns/participable.rb | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/app/models/concerns/participable.rb b/app/models/concerns/participable.rb index a01b28ea8db..7a5e4876ff2 100644 --- a/app/models/concerns/participable.rb +++ b/app/models/concerns/participable.rb @@ -1,3 +1,27 @@ +# == 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 -- cgit v1.2.1 From 077a8ec270b58ed099d13447841d51b2df70541b Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Thu, 23 Apr 2015 18:46:52 +0200 Subject: Fix tests. --- spec/support/mentionable_shared_examples.rb | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/spec/support/mentionable_shared_examples.rb b/spec/support/mentionable_shared_examples.rb index 63800602e01..9d0af29ff99 100644 --- a/spec/support/mentionable_shared_examples.rb +++ b/spec/support/mentionable_shared_examples.rb @@ -52,10 +52,9 @@ def common_mentionable_setup } extra_commits.each { |c| commitmap[c.short_id] = c } - allow(project).to receive(:commit) { |sha| commitmap[sha] } + allow(project.repository).to receive(:commit) { |sha| commitmap[sha] } set_mentionable_text.call(ref_string) - subject.save end end @@ -108,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 @@ -136,7 +137,6 @@ shared_examples 'an editable mentionable' do with(newref, subject.local_reference, author) end - subject.save set_mentionable_text.call(new_text) subject.notice_added_references(project, author) end -- cgit v1.2.1