From 20037e61122a688366060f9427506962048e77ed Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Thu, 7 Jul 2016 21:03:21 +0800 Subject: Introduce Project#builds_for(build_name, ref = 'HEAD'): So that we could find the particular builds according to build_name and ref. It would be used to find the latest build artifacts from a particular branch or tag. --- app/models/commit_status.rb | 8 +++++++- app/models/project.rb | 8 ++++++++ 2 files changed, 15 insertions(+), 1 deletion(-) (limited to 'app/models') diff --git a/app/models/commit_status.rb b/app/models/commit_status.rb index e437e3417a8..6828705dbc8 100644 --- a/app/models/commit_status.rb +++ b/app/models/commit_status.rb @@ -16,7 +16,13 @@ class CommitStatus < ActiveRecord::Base alias_attribute :author, :user - scope :latest, -> { where(id: unscope(:select).select('max(id)').group(:name, :commit_id)) } + scope :latest, -> do + id = unscope(:select). + select("max(#{table_name}.id)"). + group(:name, :commit_id) + + where(id: id) + end scope :retried, -> { where.not(id: latest) } scope :ordered, -> { order(:name) } scope :ignored, -> { where(allow_failure: true, status: [:failed, :canceled]) } diff --git a/app/models/project.rb b/app/models/project.rb index 029026a4e56..293dbd52359 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -429,6 +429,14 @@ class Project < ActiveRecord::Base repository.commit(id) end + def builds_for(build_name, ref = 'HEAD') + sha = commit(ref).sha + + builds.joins(:pipeline). + merge(Ci::Pipeline.where(sha: sha)). + where(name: build_name) + end + def merge_base_commit(first_commit_id, second_commit_id) sha = repository.merge_base(first_commit_id, second_commit_id) repository.commit(sha) if sha -- cgit v1.2.1 From d0451a050d5c4a3d343077d0820451af5058636b Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Tue, 12 Jul 2016 18:56:41 +0800 Subject: Test for Project#builds_for and return empty array for nothing --- app/models/project.rb | 3 +++ 1 file changed, 3 insertions(+) (limited to 'app/models') diff --git a/app/models/project.rb b/app/models/project.rb index f3266a1b197..35ffb0a415d 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -430,6 +430,9 @@ class Project < ActiveRecord::Base end def builds_for(build_name, ref = 'HEAD') + ct = commit(ref) + return [] unless ct + sha = commit(ref).sha builds.joins(:pipeline). -- cgit v1.2.1 From 6597c213c341ae072216c125a97f94a174fc3dfa Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Tue, 12 Jul 2016 19:28:21 +0800 Subject: Prefer empty relation rather than arrays --- app/models/project.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'app/models') diff --git a/app/models/project.rb b/app/models/project.rb index 35ffb0a415d..bc15f8c4138 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -431,7 +431,7 @@ class Project < ActiveRecord::Base def builds_for(build_name, ref = 'HEAD') ct = commit(ref) - return [] unless ct + return builds.none unless ct sha = commit(ref).sha -- cgit v1.2.1 From c94cff3e13d3f5ab55816ba2e84f48a659462441 Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Tue, 12 Jul 2016 19:29:59 +0800 Subject: Prefer if over return --- app/models/project.rb | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) (limited to 'app/models') diff --git a/app/models/project.rb b/app/models/project.rb index bc15f8c4138..366817079bb 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -431,13 +431,17 @@ class Project < ActiveRecord::Base def builds_for(build_name, ref = 'HEAD') ct = commit(ref) - return builds.none unless ct - sha = commit(ref).sha + if ct.nil? + builds.none - builds.joins(:pipeline). - merge(Ci::Pipeline.where(sha: sha)). - where(name: build_name) + else + sha = ct.sha + + builds.joins(:pipeline). + merge(Ci::Pipeline.where(sha: sha)). + where(name: build_name) + end end def merge_base_commit(first_commit_id, second_commit_id) -- cgit v1.2.1 From d0b9112fefcf0ee01d9df2dd9c2f1076738a53f1 Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Tue, 12 Jul 2016 23:28:41 +0800 Subject: save some lines and a local variable --- app/models/project.rb | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) (limited to 'app/models') diff --git a/app/models/project.rb b/app/models/project.rb index 366817079bb..2a9d68d10d2 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -436,10 +436,8 @@ class Project < ActiveRecord::Base builds.none else - sha = ct.sha - builds.joins(:pipeline). - merge(Ci::Pipeline.where(sha: sha)). + merge(Ci::Pipeline.where(sha: ct.sha)). where(name: build_name) end end -- cgit v1.2.1 From 66b91ce9ac70025093d52247ecfaa3f47536809f Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Thu, 14 Jul 2016 14:24:17 +0800 Subject: Avoid shadowing CommitStatus#id, feedback: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/5142#note_13047163 --- app/models/commit_status.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'app/models') diff --git a/app/models/commit_status.rb b/app/models/commit_status.rb index 6828705dbc8..baabbd785cc 100644 --- a/app/models/commit_status.rb +++ b/app/models/commit_status.rb @@ -17,11 +17,11 @@ class CommitStatus < ActiveRecord::Base alias_attribute :author, :user scope :latest, -> do - id = unscope(:select). + max_id = unscope(:select). select("max(#{table_name}.id)"). group(:name, :commit_id) - where(id: id) + where(id: max_id) end scope :retried, -> { where.not(id: latest) } scope :ordered, -> { order(:name) } -- cgit v1.2.1 From 5544852825d637dfe24b53e93b1e95d21767783c Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Thu, 14 Jul 2016 14:25:07 +0800 Subject: Remove blank line between if/else clause, feedback: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/5142#note_13047184 --- app/models/project.rb | 1 - 1 file changed, 1 deletion(-) (limited to 'app/models') diff --git a/app/models/project.rb b/app/models/project.rb index 2a9d68d10d2..48bb9743439 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -434,7 +434,6 @@ class Project < ActiveRecord::Base if ct.nil? builds.none - else builds.joins(:pipeline). merge(Ci::Pipeline.where(sha: ct.sha)). -- cgit v1.2.1 From fab8bc5313a56c5f22e56903de2cb9c86df79fe4 Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Thu, 14 Jul 2016 14:26:04 +0800 Subject: More descriptive local variable, feedback: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/5142#note_13047190 --- app/models/project.rb | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) (limited to 'app/models') diff --git a/app/models/project.rb b/app/models/project.rb index 48bb9743439..793cf2d70fb 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -430,13 +430,13 @@ class Project < ActiveRecord::Base end def builds_for(build_name, ref = 'HEAD') - ct = commit(ref) + commit_object = commit(ref) - if ct.nil? + if commit_object.nil? builds.none else builds.joins(:pipeline). - merge(Ci::Pipeline.where(sha: ct.sha)). + merge(Ci::Pipeline.where(sha: commit_object.sha)). where(name: build_name) end end -- cgit v1.2.1 From 53a9dee6cb54b75fa2999b4a33a59928b3b73ec3 Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Fri, 15 Jul 2016 02:22:29 +0800 Subject: Introduce Project#latest_success_builds_for: So it's more accessible for views to access the names of jobs. Only filter Build#name from where we really need to download it. --- app/models/project.rb | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) (limited to 'app/models') diff --git a/app/models/project.rb b/app/models/project.rb index 793cf2d70fb..384841dbb9a 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -429,15 +429,18 @@ class Project < ActiveRecord::Base repository.commit(ref) end - def builds_for(build_name, ref = 'HEAD') + def latest_success_builds_for(ref = 'HEAD') + builds_for(ref).success.latest + end + + def builds_for(ref = 'HEAD') commit_object = commit(ref) if commit_object.nil? builds.none else builds.joins(:pipeline). - merge(Ci::Pipeline.where(sha: commit_object.sha)). - where(name: build_name) + merge(Ci::Pipeline.where(sha: commit_object.sha)) end end -- cgit v1.2.1 From 5c1f75e983c88d4c884a15e9f84550fd256fb07f Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Mon, 18 Jul 2016 16:47:47 +0800 Subject: Use ci_commits.gl_project_id instead of ci_builds.gl_project_id: Feedback: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/5142#note_13125513 --- app/models/project.rb | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) (limited to 'app/models') diff --git a/app/models/project.rb b/app/models/project.rb index 384841dbb9a..d6e37e66a8b 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -437,10 +437,11 @@ class Project < ActiveRecord::Base commit_object = commit(ref) if commit_object.nil? - builds.none + Ci::Build.none else - builds.joins(:pipeline). - merge(Ci::Pipeline.where(sha: commit_object.sha)) + Ci::Build.joins(:pipeline). + merge(Ci::Pipeline.where(sha: commit_object.sha, + project: self)) end end -- cgit v1.2.1 From 85409a5a10d22bebbc54a9c7b7c76e7c0e11b208 Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Mon, 18 Jul 2016 20:10:50 +0800 Subject: Use ci_commits.ref (Pipeline#ref) to find builds --- app/models/project.rb | 11 ++--------- 1 file changed, 2 insertions(+), 9 deletions(-) (limited to 'app/models') diff --git a/app/models/project.rb b/app/models/project.rb index d6e37e66a8b..770ec1c8a68 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -434,15 +434,8 @@ class Project < ActiveRecord::Base end def builds_for(ref = 'HEAD') - commit_object = commit(ref) - - if commit_object.nil? - Ci::Build.none - else - Ci::Build.joins(:pipeline). - merge(Ci::Pipeline.where(sha: commit_object.sha, - project: self)) - end + Ci::Build.joins(:pipeline). + merge(Ci::Pipeline.where(ref: ref, project: self)) end def merge_base_commit(first_commit_id, second_commit_id) -- cgit v1.2.1 From af86b8c2c2b6fb08ea55eb89f1dd20aba81862ae Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Mon, 18 Jul 2016 21:11:53 +0800 Subject: Latest success pipelines (rather than builds) --- app/models/ci/pipeline.rb | 8 ++++++++ app/models/commit_status.rb | 4 ++-- app/models/project.rb | 7 ++----- 3 files changed, 12 insertions(+), 7 deletions(-) (limited to 'app/models') diff --git a/app/models/ci/pipeline.rb b/app/models/ci/pipeline.rb index fa4071e2482..431a91004cd 100644 --- a/app/models/ci/pipeline.rb +++ b/app/models/ci/pipeline.rb @@ -18,6 +18,14 @@ module Ci after_touch :update_state after_save :keep_around_commits + scope :latest, -> do + max_id = unscope(:select). + select("max(#{table_name}.id)"). + group(:ref) + + where(id: max_id) + end + def self.truncate_sha(sha) sha[0...8] end diff --git a/app/models/commit_status.rb b/app/models/commit_status.rb index baabbd785cc..3e97fe68d4b 100644 --- a/app/models/commit_status.rb +++ b/app/models/commit_status.rb @@ -18,8 +18,8 @@ class CommitStatus < ActiveRecord::Base scope :latest, -> do max_id = unscope(:select). - select("max(#{table_name}.id)"). - group(:name, :commit_id) + select("max(#{table_name}.id)"). + group(:name, :commit_id) where(id: max_id) end diff --git a/app/models/project.rb b/app/models/project.rb index 770ec1c8a68..1578fe67581 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -430,12 +430,9 @@ class Project < ActiveRecord::Base end def latest_success_builds_for(ref = 'HEAD') - builds_for(ref).success.latest - end - - def builds_for(ref = 'HEAD') Ci::Build.joins(:pipeline). - merge(Ci::Pipeline.where(ref: ref, project: self)) + merge(pipelines.where(ref: ref).success.latest). + with_artifacts end def merge_base_commit(first_commit_id, second_commit_id) -- cgit v1.2.1 From e51d4a05b7195a98b48548c4c7260886f956b6d2 Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Mon, 18 Jul 2016 21:17:16 +0800 Subject: We should actually give latest success builds as well --- app/models/project.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'app/models') diff --git a/app/models/project.rb b/app/models/project.rb index 1578fe67581..d26aa8073e8 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -432,7 +432,7 @@ class Project < ActiveRecord::Base def latest_success_builds_for(ref = 'HEAD') Ci::Build.joins(:pipeline). merge(pipelines.where(ref: ref).success.latest). - with_artifacts + with_artifacts.success.latest end def merge_base_commit(first_commit_id, second_commit_id) -- cgit v1.2.1 From 57a78c37c72b2d697bc863ebfb84d3ca61ba9d7b Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Mon, 18 Jul 2016 23:17:43 +0800 Subject: Show notice if builds are not from latest pipeline --- app/models/ci/build.rb | 3 +++ app/models/project.rb | 12 +++++++++--- 2 files changed, 12 insertions(+), 3 deletions(-) (limited to 'app/models') diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb index ffac3a22efc..9af04964b85 100644 --- a/app/models/ci/build.rb +++ b/app/models/ci/build.rb @@ -15,6 +15,9 @@ module Ci scope :with_artifacts, ->() { where.not(artifacts_file: nil) } scope :with_expired_artifacts, ->() { with_artifacts.where('artifacts_expire_at < ?', Time.now) } scope :last_month, ->() { where('created_at > ?', Date.today - 1.month) } + scope :latest_success_with_artifacts, ->() do + with_artifacts.success.latest + end mount_uploader :artifacts_file, ArtifactUploader mount_uploader :artifacts_metadata, ArtifactUploader diff --git a/app/models/project.rb b/app/models/project.rb index 12851c5d0ec..77431c3f538 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -429,10 +429,16 @@ class Project < ActiveRecord::Base repository.commit(ref) end - def latest_success_builds_for(ref = 'HEAD') + # ref can't be HEAD or SHA, can only be branch/tag name + def latest_success_pipeline_for(ref = 'master') + pipelines.where(ref: ref).success.latest + end + + # ref can't be HEAD or SHA, can only be branch/tag name + def latest_success_builds_for(ref = 'master') Ci::Build.joins(:pipeline). - merge(pipelines.where(ref: ref).success.latest). - with_artifacts.success.latest + merge(latest_success_pipeline_for(ref)). + latest_success_with_artifacts end def merge_base_commit(first_commit_id, second_commit_id) -- cgit v1.2.1 From fba2ec45b3bf493611f2d7e7e13a21c39bc654e0 Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Tue, 19 Jul 2016 17:08:21 +0800 Subject: Just use order(id: :desc) for latest stuffs: We don't need that subquery for group by ref and alike here. --- app/models/ci/build.rb | 2 +- app/models/ci/pipeline.rb | 10 +--------- app/models/project.rb | 2 +- 3 files changed, 3 insertions(+), 11 deletions(-) (limited to 'app/models') diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb index 9af04964b85..c048eff0f80 100644 --- a/app/models/ci/build.rb +++ b/app/models/ci/build.rb @@ -16,7 +16,7 @@ module Ci scope :with_expired_artifacts, ->() { with_artifacts.where('artifacts_expire_at < ?', Time.now) } scope :last_month, ->() { where('created_at > ?', Date.today - 1.month) } scope :latest_success_with_artifacts, ->() do - with_artifacts.success.latest + with_artifacts.success.order(id: :desc) end mount_uploader :artifacts_file, ArtifactUploader diff --git a/app/models/ci/pipeline.rb b/app/models/ci/pipeline.rb index a8e6a23e1c4..148b056789a 100644 --- a/app/models/ci/pipeline.rb +++ b/app/models/ci/pipeline.rb @@ -20,14 +20,6 @@ module Ci after_touch :update_state after_save :keep_around_commits - scope :latest, -> do - max_id = unscope(:select). - select("max(#{table_name}.id)"). - group(:ref) - - where(id: max_id) - end - def self.truncate_sha(sha) sha[0...8] end @@ -226,7 +218,7 @@ module Ci def keep_around_commits return unless project - + project.repository.keep_around(self.sha) project.repository.keep_around(self.before_sha) end diff --git a/app/models/project.rb b/app/models/project.rb index 77431c3f538..30e8ade99ff 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -431,7 +431,7 @@ class Project < ActiveRecord::Base # ref can't be HEAD or SHA, can only be branch/tag name def latest_success_pipeline_for(ref = 'master') - pipelines.where(ref: ref).success.latest + pipelines.where(ref: ref).success.order(id: :desc) end # ref can't be HEAD or SHA, can only be branch/tag name -- cgit v1.2.1 From 0538e1e934484e76575164314fe8451374e4a4c8 Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Tue, 19 Jul 2016 17:09:32 +0800 Subject: Support SHA for downloading artifacts: So if we also query against SHA, we could actually support SHA. If there's a branch or tag also named like SHA this could be ambiguous, but since we could already do that in Git, I think it's probably fine, people would be aware they shouldn't use the same name anyway. --- app/models/project.rb | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) (limited to 'app/models') diff --git a/app/models/project.rb b/app/models/project.rb index 30e8ade99ff..c1cb1558132 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -429,12 +429,15 @@ class Project < ActiveRecord::Base repository.commit(ref) end - # ref can't be HEAD or SHA, can only be branch/tag name + # ref can't be HEAD, can only be branch/tag name or SHA def latest_success_pipeline_for(ref = 'master') - pipelines.where(ref: ref).success.order(id: :desc) + table = Ci::Pipeline.quoted_table_name + # TODO: Use `where(ref: ref).or(sha: ref)` in Rails 5 + pipelines.where("#{table}.ref = ? OR #{table}.sha = ?", ref, ref). + success.order(id: :desc) end - # ref can't be HEAD or SHA, can only be branch/tag name + # ref can't be HEAD, can only be branch/tag name or SHA def latest_success_builds_for(ref = 'master') Ci::Build.joins(:pipeline). merge(latest_success_pipeline_for(ref)). -- cgit v1.2.1 From 85ceb8b72f5a67d21bc9530fe835fdece98f3d4e Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Tue, 19 Jul 2016 17:51:45 +0800 Subject: Rename latest_success* to latest_successful: Feedback: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/5142#note_13164642 --- app/models/ci/build.rb | 2 +- app/models/project.rb | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) (limited to 'app/models') diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb index c048eff0f80..65dfe4f0190 100644 --- a/app/models/ci/build.rb +++ b/app/models/ci/build.rb @@ -15,7 +15,7 @@ module Ci scope :with_artifacts, ->() { where.not(artifacts_file: nil) } scope :with_expired_artifacts, ->() { with_artifacts.where('artifacts_expire_at < ?', Time.now) } scope :last_month, ->() { where('created_at > ?', Date.today - 1.month) } - scope :latest_success_with_artifacts, ->() do + scope :latest_successful_with_artifacts, ->() do with_artifacts.success.order(id: :desc) end diff --git a/app/models/project.rb b/app/models/project.rb index c1cb1558132..60928bf9922 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -430,7 +430,7 @@ class Project < ActiveRecord::Base end # ref can't be HEAD, can only be branch/tag name or SHA - def latest_success_pipeline_for(ref = 'master') + def latest_successful_pipeline_for(ref = 'master') table = Ci::Pipeline.quoted_table_name # TODO: Use `where(ref: ref).or(sha: ref)` in Rails 5 pipelines.where("#{table}.ref = ? OR #{table}.sha = ?", ref, ref). @@ -438,10 +438,10 @@ class Project < ActiveRecord::Base end # ref can't be HEAD, can only be branch/tag name or SHA - def latest_success_builds_for(ref = 'master') + def latest_successful_builds_for(ref = 'master') Ci::Build.joins(:pipeline). - merge(latest_success_pipeline_for(ref)). - latest_success_with_artifacts + merge(latest_successful_pipeline_for(ref)). + latest_successful_with_artifacts end def merge_base_commit(first_commit_id, second_commit_id) -- cgit v1.2.1 From 2afcc9e910bde32507e14be452c9f7681f5497e4 Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Mon, 25 Jul 2016 14:10:21 +0300 Subject: Create new version of merge request diff on push instead of updating existing one Signed-off-by: Dmitriy Zaporozhets --- app/models/merge_request.rb | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) (limited to 'app/models') diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index f1b9c1d6feb..ef0b7da048e 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -10,8 +10,7 @@ class MergeRequest < ActiveRecord::Base belongs_to :source_project, foreign_key: :source_project_id, class_name: "Project" belongs_to :merge_user, class_name: "User" - has_one :merge_request_diff, dependent: :destroy - + has_many :merge_request_diffs, dependent: :destroy has_many :events, as: :target, dependent: :destroy serialize :merge_params, Hash @@ -294,7 +293,9 @@ class MergeRequest < ActiveRecord::Base end def reload_diff - return unless merge_request_diff && open? + return unless open? + + merge_request_diff = merge_request_diffs.create old_diff_refs = self.diff_refs @@ -691,4 +692,8 @@ class MergeRequest < ActiveRecord::Base def keep_around_commit project.repository.keep_around(self.merge_commit_sha) end + + def merge_request_diff + merge_request_diffs.first + end end -- cgit v1.2.1 From ceff8106433187613eb97d13952fc96c4806b847 Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Mon, 25 Jul 2016 17:04:55 +0300 Subject: Create merge request diff on merge request creation Signed-off-by: Dmitriy Zaporozhets --- app/models/merge_request.rb | 4 ++++ 1 file changed, 4 insertions(+) (limited to 'app/models') diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index ef0b7da048e..88c5987d485 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -286,6 +286,10 @@ class MergeRequest < ActiveRecord::Base end end + def create_merge_request_diff + merge_request_diffs.create + end + def update_merge_request_diff if source_branch_changed? || target_branch_changed? reload_diff -- cgit v1.2.1 From 94ca25c9b8c62f9995fbd571c33954754950e1da Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Mon, 25 Jul 2016 18:42:57 +0300 Subject: Improve merge request diff creation and update tests Signed-off-by: Dmitriy Zaporozhets --- app/models/merge_request.rb | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) (limited to 'app/models') diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index 88c5987d485..cc1d0c18437 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -299,12 +299,8 @@ class MergeRequest < ActiveRecord::Base def reload_diff return unless open? - merge_request_diff = merge_request_diffs.create - old_diff_refs = self.diff_refs - - merge_request_diff.reload_content - + create_merge_request_diff new_diff_refs = self.diff_refs update_diff_notes_positions( -- cgit v1.2.1 From b8fef7eb5948344f4d442a52637cad168f4c5bf1 Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Tue, 26 Jul 2016 18:24:25 +0300 Subject: Add ability to render different merge request versions Signed-off-by: Dmitriy Zaporozhets --- app/models/merge_request_diff.rb | 10 ++++++++++ 1 file changed, 10 insertions(+) (limited to 'app/models') diff --git a/app/models/merge_request_diff.rb b/app/models/merge_request_diff.rb index 3f520c8f3ff..a92f597225a 100644 --- a/app/models/merge_request_diff.rb +++ b/app/models/merge_request_diff.rb @@ -96,6 +96,16 @@ class MergeRequestDiff < ActiveRecord::Base end end + def diff_refs + return unless start_commit || base_commit + + Gitlab::Diff::DiffRefs.new( + base_sha: base_commit_sha, + start_sha: start_commit_sha, + head_sha: head_commit_sha + ) + end + private # Collect array of Git::Commit objects -- cgit v1.2.1 From 09fa0139281d7a76d6b40991f7672187fea40e67 Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Wed, 27 Jul 2016 14:41:19 +0300 Subject: Refactor merge request diff Signed-off-by: Dmitriy Zaporozhets --- app/models/merge_request_diff.rb | 89 +++++++++++++++++----------------------- 1 file changed, 38 insertions(+), 51 deletions(-) (limited to 'app/models') diff --git a/app/models/merge_request_diff.rb b/app/models/merge_request_diff.rb index a92f597225a..07bceda049c 100644 --- a/app/models/merge_request_diff.rb +++ b/app/models/merge_request_diff.rb @@ -8,7 +8,8 @@ class MergeRequestDiff < ActiveRecord::Base belongs_to :merge_request - delegate :source_branch_sha, :target_branch_sha, :target_branch, :source_branch, to: :merge_request, prefix: nil + delegate :source_branch_sha, :target_branch_sha, + :target_branch, :source_branch, to: :merge_request, prefix: nil state_machine :state, initial: :empty do state :collected @@ -24,9 +25,16 @@ class MergeRequestDiff < ActiveRecord::Base serialize :st_commits serialize :st_diffs + after_initialize :set_diff_range after_create :reload_content, unless: :importing? after_save :keep_around_commits, unless: :importing? + def set_diff_range + self.start_commit_sha ||= target_branch_sha + self.head_commit_sha ||= source_branch_sha + self.base_commit_sha ||= branch_base_sha + end + def reload_content reload_commits reload_diffs @@ -41,8 +49,8 @@ class MergeRequestDiff < ActiveRecord::Base @diffs_no_whitespace ||= begin compare = Gitlab::Git::Compare.new( repository.raw_repository, - self.start_commit_sha || self.target_branch_sha, - self.head_commit_sha || self.source_branch_sha, + start_commit_sha, + head_commit_sha ) compare.diffs(options) end @@ -65,35 +73,21 @@ class MergeRequestDiff < ActiveRecord::Base end def base_commit - return unless self.base_commit_sha + return unless base_commit_sha - project.commit(self.base_commit_sha) + project.commit(base_commit_sha) end def start_commit - return unless self.start_commit_sha + return unless start_commit_sha - project.commit(self.start_commit_sha) + project.commit(start_commit_sha) end def head_commit - return last_commit unless self.head_commit_sha + return last_commit unless head_commit_sha - project.commit(self.head_commit_sha) - end - - def compare - @compare ||= - begin - # Update ref for merge request - merge_request.fetch_ref - - Gitlab::Git::Compare.new( - repository.raw_repository, - self.target_branch_sha, - self.source_branch_sha - ) - end + project.commit(head_commit_sha) end def diff_refs @@ -108,16 +102,18 @@ class MergeRequestDiff < ActiveRecord::Base private - # Collect array of Git::Commit objects - # between target and source branches - def unmerged_commits - commits = compare.commits - - if commits.present? - commits = Commit.decorate(commits, merge_request.source_project).reverse - end + def compare + @compare ||= + begin + # Update ref for merge request + merge_request.fetch_ref - commits + Gitlab::Git::Compare.new( + repository.raw_repository, + start_commit_sha, + head_commit_sha + ) + end end def dump_commits(commits) @@ -133,21 +129,16 @@ class MergeRequestDiff < ActiveRecord::Base def reload_commits new_attributes = {} - commit_objects = unmerged_commits + commits = compare.commits - if commit_objects.present? - new_attributes[:st_commits] = dump_commits(commit_objects) + if commits.present? + commits = Commit.decorate(commits, merge_request.source_project).reverse + new_attributes[:st_commits] = dump_commits(commits) end update_columns_serialized(new_attributes) end - # Collect array of Git::Diff objects - # between target and source branches - def unmerged_diffs - compare.diffs(Commit.max_diff_options) - end - def dump_diffs(diffs) if diffs.respond_to?(:map) diffs.map(&:to_hash) @@ -177,7 +168,7 @@ class MergeRequestDiff < ActiveRecord::Base if commits.size.zero? new_attributes[:state] = :empty else - diff_collection = unmerged_diffs + diff_collection = compare.diffs(Commit.max_diff_options) if diff_collection.overflow? # Set our state to 'overflow' to make the #empty? and #collected? @@ -195,10 +186,6 @@ class MergeRequestDiff < ActiveRecord::Base new_attributes[:st_diffs] = new_diffs - new_attributes[:start_commit_sha] = self.target_branch_sha - new_attributes[:head_commit_sha] = self.source_branch_sha - new_attributes[:base_commit_sha] = branch_base_sha - update_columns_serialized(new_attributes) keep_around_commits @@ -213,9 +200,9 @@ class MergeRequestDiff < ActiveRecord::Base end def branch_base_commit - return unless self.source_branch_sha && self.target_branch_sha + return unless source_branch_sha && target_branch_sha - project.merge_base_commit(self.source_branch_sha, self.target_branch_sha) + project.merge_base_commit(source_branch_sha, target_branch_sha) end def branch_base_sha @@ -254,8 +241,8 @@ class MergeRequestDiff < ActiveRecord::Base end def keep_around_commits - repository.keep_around(target_branch_sha) - repository.keep_around(source_branch_sha) - repository.keep_around(branch_base_sha) + repository.keep_around(start_commit_sha) + repository.keep_around(head_commit_sha) + repository.keep_around(base_commit_sha) end end -- cgit v1.2.1 From 988836bc807a6e7ba5362fd8bddd330d1f3bf19b Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Thu, 28 Jul 2016 11:36:30 +0300 Subject: Refactor MergeRequestDiff model Since MergeRequestDiff is not about branches and current state of merge request diff anymore I removed most of branch related method and added validation for head/start/base commit sha. From this point MergeRequestDiff is about saving diff between branches only once at moment of creation. Once created MergeRequestDiff should not be changes. Because of that we should not rely on changes in source/target branches when read from MergeRequestDiff Signed-off-by: Dmitriy Zaporozhets --- app/models/merge_request_diff.rb | 32 +++++++++++++++++--------------- 1 file changed, 17 insertions(+), 15 deletions(-) (limited to 'app/models') diff --git a/app/models/merge_request_diff.rb b/app/models/merge_request_diff.rb index 07bceda049c..1e529c84706 100644 --- a/app/models/merge_request_diff.rb +++ b/app/models/merge_request_diff.rb @@ -8,9 +8,6 @@ class MergeRequestDiff < ActiveRecord::Base belongs_to :merge_request - delegate :source_branch_sha, :target_branch_sha, - :target_branch, :source_branch, to: :merge_request, prefix: nil - state_machine :state, initial: :empty do state :collected state :overflow @@ -25,14 +22,23 @@ class MergeRequestDiff < ActiveRecord::Base serialize :st_commits serialize :st_diffs - after_initialize :set_diff_range - after_create :reload_content, unless: :importing? + validates :head_commit_sha, presence: true + validates :start_commit_sha, presence: true + validates :base_commit_sha, presence: true + + after_initialize :ensure_head_commit_sha, if: :persisted? + before_create :set_diff_range, unless: :importing? + after_create :reload_content, unless: :importing? after_save :keep_around_commits, unless: :importing? + def ensure_head_commit_sha + self.head_commit_sha ||= last_commit.sha + end + def set_diff_range - self.start_commit_sha ||= target_branch_sha - self.head_commit_sha ||= source_branch_sha - self.base_commit_sha ||= branch_base_sha + self.start_commit_sha ||= merge_request.target_branch_sha + self.head_commit_sha ||= merge_request.source_branch_sha + self.base_commit_sha ||= find_base_sha end def reload_content @@ -199,14 +205,10 @@ class MergeRequestDiff < ActiveRecord::Base project.repository end - def branch_base_commit - return unless source_branch_sha && target_branch_sha - - project.merge_base_commit(source_branch_sha, target_branch_sha) - end + def find_base_sha + return unless head_commit_sha && start_commit_sha - def branch_base_sha - branch_base_commit.try(:sha) + project.merge_base_commit(head_commit_sha, start_commit_sha).try(:sha) end def utf8_st_diffs -- cgit v1.2.1 From 964742f600f5fc5e07272982c4d3847ccc76ce00 Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Thu, 28 Jul 2016 13:46:27 +0300 Subject: Ensure merge request is created with valid diff object * Add merge_request_diff validation to merge_request model * Improve initialize of merge_request_diff object * Rename some merge_request_diff methods for clarity Signed-off-by: Dmitriy Zaporozhets --- app/models/merge_request.rb | 7 ++++++- app/models/merge_request_diff.rb | 41 +++++++++++++++++++++------------------- 2 files changed, 28 insertions(+), 20 deletions(-) (limited to 'app/models') diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index cc1d0c18437..618829891a0 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -15,7 +15,7 @@ class MergeRequest < ActiveRecord::Base serialize :merge_params, Hash - after_create :create_merge_request_diff, unless: :importing? + before_validation :ensure_merge_request_diff, on: :create, unless: :importing? after_update :update_merge_request_diff delegate :commits, :real_size, to: :merge_request_diff, prefix: nil @@ -95,6 +95,7 @@ class MergeRequest < ActiveRecord::Base validates :merge_user, presence: true, if: :merge_when_build_succeeds? validate :validate_branches, unless: [:allow_broken, :importing?] validate :validate_fork + validates_associated :merge_request_diff, on: :create, unless: [:allow_broken, :importing?] scope :by_branch, ->(branch_name) { where("(source_branch LIKE :branch) OR (target_branch LIKE :branch)", branch: branch_name) } scope :cared, ->(user) { where('assignee_id = :user OR author_id = :user', user: user.id) } @@ -286,6 +287,10 @@ class MergeRequest < ActiveRecord::Base end end + def ensure_merge_request_diff + merge_request_diff || merge_request_diffs.build + end + def create_merge_request_diff merge_request_diffs.create end diff --git a/app/models/merge_request_diff.rb b/app/models/merge_request_diff.rb index 1e529c84706..da1e317027d 100644 --- a/app/models/merge_request_diff.rb +++ b/app/models/merge_request_diff.rb @@ -22,28 +22,31 @@ class MergeRequestDiff < ActiveRecord::Base serialize :st_commits serialize :st_diffs - validates :head_commit_sha, presence: true validates :start_commit_sha, presence: true + validates :head_commit_sha, presence: true validates :base_commit_sha, presence: true - after_initialize :ensure_head_commit_sha, if: :persisted? - before_create :set_diff_range, unless: :importing? - after_create :reload_content, unless: :importing? - after_save :keep_around_commits, unless: :importing? - - def ensure_head_commit_sha - self.head_commit_sha ||= last_commit.sha - end + after_initialize :set_diff_range, unless: :importing? + after_create :save_git_content, unless: :importing? + after_save :keep_around_commits, unless: :importing? def set_diff_range - self.start_commit_sha ||= merge_request.target_branch_sha - self.head_commit_sha ||= merge_request.source_branch_sha - self.base_commit_sha ||= find_base_sha + if persisted? + # Workaround for old MergeRequestDiff object + # that does not have head_commit_sha in the database + self.head_commit_sha ||= last_commit.sha + else + self.start_commit_sha ||= merge_request.target_branch_sha + self.head_commit_sha ||= merge_request.source_branch_sha + self.base_commit_sha ||= find_base_sha + end end - def reload_content - reload_commits - reload_diffs + # Collect information about commits and diff from repository + # and save it to the database as serialized data + def save_git_content + save_commits + save_diffs end def size @@ -130,9 +133,9 @@ class MergeRequestDiff < ActiveRecord::Base array.map { |hash| Commit.new(Gitlab::Git::Commit.new(hash), merge_request.source_project) } end - # Reload all commits related to current merge request from repo + # Load all commits related to current merge request diff from repo # and save it as array of hashes in st_commits db field - def reload_commits + def save_commits new_attributes = {} commits = compare.commits @@ -165,9 +168,9 @@ class MergeRequestDiff < ActiveRecord::Base end end - # Reload diffs between branches related to current merge request from repo + # Load diffs between branches related to current merge request diff from repo # and save it as array of hashes in st_diffs db field - def reload_diffs + def save_diffs new_attributes = {} new_diffs = [] -- cgit v1.2.1 From 5cad2d290219d29aa2be6e991b42b73ac9d87754 Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Thu, 28 Jul 2016 17:26:54 +0300 Subject: Add improvements to merge request versions * show commits count in the merge request version dropdown * initialize base/start commit sha for old merge request diffs from repo Signed-off-by: Dmitriy Zaporozhets --- app/models/merge_request_diff.rb | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) (limited to 'app/models') diff --git a/app/models/merge_request_diff.rb b/app/models/merge_request_diff.rb index da1e317027d..2bc0b2636f4 100644 --- a/app/models/merge_request_diff.rb +++ b/app/models/merge_request_diff.rb @@ -34,7 +34,9 @@ class MergeRequestDiff < ActiveRecord::Base if persisted? # Workaround for old MergeRequestDiff object # that does not have head_commit_sha in the database - self.head_commit_sha ||= last_commit.sha + self.start_commit_sha ||= merge_request.target_branch_sha + self.head_commit_sha ||= last_commit.sha + self.base_commit_sha ||= find_base_sha else self.start_commit_sha ||= merge_request.target_branch_sha self.head_commit_sha ||= merge_request.source_branch_sha -- cgit v1.2.1 From 8e031ce3b29b186858ddda2a28a6ea8f98b2f749 Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Thu, 28 Jul 2016 17:41:35 +0300 Subject: Remove requirement for base_commit_sha to allow creation of merge requests for orphaned branches Signed-off-by: Dmitriy Zaporozhets --- app/models/merge_request_diff.rb | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) (limited to 'app/models') diff --git a/app/models/merge_request_diff.rb b/app/models/merge_request_diff.rb index 2bc0b2636f4..da0c15ddeea 100644 --- a/app/models/merge_request_diff.rb +++ b/app/models/merge_request_diff.rb @@ -22,9 +22,8 @@ class MergeRequestDiff < ActiveRecord::Base serialize :st_commits serialize :st_diffs - validates :start_commit_sha, presence: true - validates :head_commit_sha, presence: true - validates :base_commit_sha, presence: true + validates :start_commit_sha, presence: true, unless: :importing? + validates :head_commit_sha, presence: true, unless: :importing? after_initialize :set_diff_range, unless: :importing? after_create :save_git_content, unless: :importing? -- cgit v1.2.1 From b0a023842d4e9fc29db7fa6c7168528d9e4ac09d Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Thu, 28 Jul 2016 18:41:57 +0300 Subject: Reload merge request association reload when update code Signed-off-by: Dmitriy Zaporozhets --- app/models/merge_request.rb | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) (limited to 'app/models') diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index 618829891a0..08523e45c65 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -246,13 +246,7 @@ class MergeRequest < ActiveRecord::Base end def diff_refs - return unless diff_start_commit || diff_base_commit - - Gitlab::Diff::DiffRefs.new( - base_sha: diff_base_sha, - start_sha: diff_start_sha, - head_sha: diff_head_sha - ) + merge_request_diff.diff_refs end def validate_branches @@ -306,6 +300,7 @@ class MergeRequest < ActiveRecord::Base old_diff_refs = self.diff_refs create_merge_request_diff + merge_request_diffs.reload new_diff_refs = self.diff_refs update_diff_notes_positions( -- cgit v1.2.1 From 0e974b52d8f5806c24fdc80ef5b62b261f5be2ba Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Thu, 28 Jul 2016 22:36:18 +0300 Subject: Refactor MergeRequestDiff initialize method Signed-off-by: Dmitriy Zaporozhets --- app/models/merge_request_diff.rb | 28 ++++++++++++---------------- 1 file changed, 12 insertions(+), 16 deletions(-) (limited to 'app/models') diff --git a/app/models/merge_request_diff.rb b/app/models/merge_request_diff.rb index da0c15ddeea..940a1016302 100644 --- a/app/models/merge_request_diff.rb +++ b/app/models/merge_request_diff.rb @@ -25,22 +25,18 @@ class MergeRequestDiff < ActiveRecord::Base validates :start_commit_sha, presence: true, unless: :importing? validates :head_commit_sha, presence: true, unless: :importing? - after_initialize :set_diff_range, unless: :importing? - after_create :save_git_content, unless: :importing? - after_save :keep_around_commits, unless: :importing? - - def set_diff_range - if persisted? - # Workaround for old MergeRequestDiff object - # that does not have head_commit_sha in the database - self.start_commit_sha ||= merge_request.target_branch_sha - self.head_commit_sha ||= last_commit.sha - self.base_commit_sha ||= find_base_sha - else - self.start_commit_sha ||= merge_request.target_branch_sha - self.head_commit_sha ||= merge_request.source_branch_sha - self.base_commit_sha ||= find_base_sha - end + after_initialize :initialize_commits_sha, unless: :importing? + after_create :save_git_content, unless: :importing? + after_save :keep_around_commits, unless: :importing? + + # Those variables are used for collecting commits and diff from git repository. + # After object is created those sha are stored in the database. + # However some old MergeRequestDiff records don't + # have those variables in the database so we try to initialize it + def initialize_commits_sha + self.start_commit_sha ||= merge_request.target_branch_sha + self.head_commit_sha ||= last_commit.try(:sha) || merge_request.source_branch_sha + self.base_commit_sha ||= find_base_sha end # Collect information about commits and diff from repository -- cgit v1.2.1 From 3c1dca0301366c63d1800aa11e73a82e68e120d0 Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Thu, 28 Jul 2016 23:40:13 +0300 Subject: Add more tests to merge_request_diff and improve initialize Signed-off-by: Dmitriy Zaporozhets --- app/models/merge_request.rb | 4 +++- app/models/merge_request_diff.rb | 4 +--- 2 files changed, 4 insertions(+), 4 deletions(-) (limited to 'app/models') diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index 08523e45c65..6d36a5ba288 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -246,7 +246,9 @@ class MergeRequest < ActiveRecord::Base end def diff_refs - merge_request_diff.diff_refs + if merge_request_diff + merge_request_diff.diff_refs + end end def validate_branches diff --git a/app/models/merge_request_diff.rb b/app/models/merge_request_diff.rb index 940a1016302..b668b62cdac 100644 --- a/app/models/merge_request_diff.rb +++ b/app/models/merge_request_diff.rb @@ -35,7 +35,7 @@ class MergeRequestDiff < ActiveRecord::Base # have those variables in the database so we try to initialize it def initialize_commits_sha self.start_commit_sha ||= merge_request.target_branch_sha - self.head_commit_sha ||= last_commit.try(:sha) || merge_request.source_branch_sha + self.head_commit_sha ||= persisted? ? last_commit.sha : merge_request.source_branch_sha self.base_commit_sha ||= find_base_sha end @@ -191,9 +191,7 @@ class MergeRequestDiff < ActiveRecord::Base end new_attributes[:st_diffs] = new_diffs - update_columns_serialized(new_attributes) - keep_around_commits end -- cgit v1.2.1 From f8aeb8cdac98108bca5d1be2a382c32df6a500e5 Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Mon, 1 Aug 2016 18:41:21 +0300 Subject: Change merge request diff creation from callback to part of the service Signed-off-by: Dmitriy Zaporozhets --- app/models/merge_request.rb | 5 ----- 1 file changed, 5 deletions(-) (limited to 'app/models') diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index 6d36a5ba288..85048f928dc 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -15,7 +15,6 @@ class MergeRequest < ActiveRecord::Base serialize :merge_params, Hash - before_validation :ensure_merge_request_diff, on: :create, unless: :importing? after_update :update_merge_request_diff delegate :commits, :real_size, to: :merge_request_diff, prefix: nil @@ -283,10 +282,6 @@ class MergeRequest < ActiveRecord::Base end end - def ensure_merge_request_diff - merge_request_diff || merge_request_diffs.build - end - def create_merge_request_diff merge_request_diffs.create end -- cgit v1.2.1 From 6515ec09bbfa25027d1b8a1240e09bc1c6edbcfb Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Tue, 2 Aug 2016 15:38:03 +0300 Subject: Chnage the way how merge request diff is initialized and saved Signed-off-by: Dmitriy Zaporozhets --- app/models/merge_request.rb | 15 +++++------ app/models/merge_request_diff.rb | 54 +++++++++++++++++++++++----------------- 2 files changed, 37 insertions(+), 32 deletions(-) (limited to 'app/models') diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index 85048f928dc..6f2216ab5db 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -11,11 +11,13 @@ class MergeRequest < ActiveRecord::Base belongs_to :merge_user, class_name: "User" has_many :merge_request_diffs, dependent: :destroy + has_one :merge_request_diff has_many :events, as: :target, dependent: :destroy serialize :merge_params, Hash - after_update :update_merge_request_diff + after_create :ensure_merge_request_diff, unless: :importing? + after_update :reload_diff_if_branch_changed delegate :commits, :real_size, to: :merge_request_diff, prefix: nil @@ -94,7 +96,6 @@ class MergeRequest < ActiveRecord::Base validates :merge_user, presence: true, if: :merge_when_build_succeeds? validate :validate_branches, unless: [:allow_broken, :importing?] validate :validate_fork - validates_associated :merge_request_diff, on: :create, unless: [:allow_broken, :importing?] scope :by_branch, ->(branch_name) { where("(source_branch LIKE :branch) OR (target_branch LIKE :branch)", branch: branch_name) } scope :cared, ->(user) { where('assignee_id = :user OR author_id = :user', user: user.id) } @@ -282,11 +283,11 @@ class MergeRequest < ActiveRecord::Base end end - def create_merge_request_diff - merge_request_diffs.create + def ensure_merge_request_diff + merge_request_diff || create_merge_request_diff end - def update_merge_request_diff + def reload_diff_if_branch_changed if source_branch_changed? || target_branch_changed? reload_diff end @@ -689,8 +690,4 @@ class MergeRequest < ActiveRecord::Base def keep_around_commit project.repository.keep_around(self.merge_commit_sha) end - - def merge_request_diff - merge_request_diffs.first - end end diff --git a/app/models/merge_request_diff.rb b/app/models/merge_request_diff.rb index b668b62cdac..074d8f5d40a 100644 --- a/app/models/merge_request_diff.rb +++ b/app/models/merge_request_diff.rb @@ -22,28 +22,28 @@ class MergeRequestDiff < ActiveRecord::Base serialize :st_commits serialize :st_diffs - validates :start_commit_sha, presence: true, unless: :importing? - validates :head_commit_sha, presence: true, unless: :importing? + # For compatibility with old MergeRequestDiff which + # does not store those variables in database + after_initialize :ensure_commits_sha, if: :persisted? - after_initialize :initialize_commits_sha, unless: :importing? + # All diff information is collected from repository after object is created. + # It allows you to override variables like head_commit_sha before getting diff. after_create :save_git_content, unless: :importing? - after_save :keep_around_commits, unless: :importing? - - # Those variables are used for collecting commits and diff from git repository. - # After object is created those sha are stored in the database. - # However some old MergeRequestDiff records don't - # have those variables in the database so we try to initialize it - def initialize_commits_sha - self.start_commit_sha ||= merge_request.target_branch_sha - self.head_commit_sha ||= persisted? ? last_commit.sha : merge_request.source_branch_sha - self.base_commit_sha ||= find_base_sha - end # Collect information about commits and diff from repository # and save it to the database as serialized data def save_git_content + ensure_commits_sha save_commits + reload_commits save_diffs + keep_around_commits + end + + def ensure_commits_sha + self.start_commit_sha ||= merge_request.target_branch_sha + self.head_commit_sha ||= last_commit.try(:sha) || merge_request.source_branch_sha + self.base_commit_sha ||= find_base_sha end def size @@ -52,14 +52,15 @@ class MergeRequestDiff < ActiveRecord::Base def diffs(options={}) if options[:ignore_whitespace_change] - @diffs_no_whitespace ||= begin - compare = Gitlab::Git::Compare.new( - repository.raw_repository, - start_commit_sha, - head_commit_sha - ) - compare.diffs(options) - end + @diffs_no_whitespace ||= + begin + compare = Gitlab::Git::Compare.new( + repository.raw_repository, + start_commit_sha, + head_commit_sha + ) + compare.diffs(options) + end else @diffs ||= {} @diffs[options] ||= load_diffs(st_diffs, options) @@ -70,6 +71,11 @@ class MergeRequestDiff < ActiveRecord::Base @commits ||= load_commits(st_commits || []) end + def reload_commits + @commits = nil + commits + end + def last_commit commits.first end @@ -192,7 +198,6 @@ class MergeRequestDiff < ActiveRecord::Base new_attributes[:st_diffs] = new_diffs update_columns_serialized(new_attributes) - keep_around_commits end def project @@ -207,6 +212,9 @@ class MergeRequestDiff < ActiveRecord::Base return unless head_commit_sha && start_commit_sha project.merge_base_commit(head_commit_sha, start_commit_sha).try(:sha) + rescue Rugged::OdbError + # In case head or start commit does not exist in the repository any more + nil end def utf8_st_diffs -- cgit v1.2.1 From 7cb8ceb5b9b7da6f9bd5f80a50eeca6727ac5902 Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Tue, 2 Aug 2016 16:28:37 +0300 Subject: Fix creation and reload of the merge request diff Signed-off-by: Dmitriy Zaporozhets --- app/models/merge_request.rb | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) (limited to 'app/models') diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index 6f2216ab5db..8b0b413343c 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -11,7 +11,9 @@ class MergeRequest < ActiveRecord::Base belongs_to :merge_user, class_name: "User" has_many :merge_request_diffs, dependent: :destroy - has_one :merge_request_diff + has_one :merge_request_diff, + -> { order('merge_request_diffs.id DESC') } + has_many :events, as: :target, dependent: :destroy serialize :merge_params, Hash @@ -287,6 +289,15 @@ class MergeRequest < ActiveRecord::Base merge_request_diff || create_merge_request_diff end + def create_merge_request_diff + merge_request_diffs.create + reload_merge_request_diff + end + + def reload_merge_request_diff + merge_request_diff(true) + end + def reload_diff_if_branch_changed if source_branch_changed? || target_branch_changed? reload_diff @@ -298,7 +309,6 @@ class MergeRequest < ActiveRecord::Base old_diff_refs = self.diff_refs create_merge_request_diff - merge_request_diffs.reload new_diff_refs = self.diff_refs update_diff_notes_positions( -- cgit v1.2.1 From fc8d2fbc946d144d2a9d075d67a9f1fe1035da0e Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Tue, 2 Aug 2016 17:52:20 +0300 Subject: Build diff_refs for merge request if merge request diff does not exist Signed-off-by: Dmitriy Zaporozhets --- app/models/merge_request.rb | 6 ++++++ 1 file changed, 6 insertions(+) (limited to 'app/models') diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index 8b0b413343c..56d5157298f 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -250,6 +250,12 @@ class MergeRequest < ActiveRecord::Base def diff_refs if merge_request_diff merge_request_diff.diff_refs + elsif diff_start_commit || diff_base_commit + Gitlab::Diff::DiffRefs.new( + base_sha: diff_base_sha, + start_sha: diff_start_sha, + head_sha: diff_head_sha + ) end end -- cgit v1.2.1 From d99d5198c2df6f931664b8096bcbfc28e8221145 Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Mon, 8 Aug 2016 23:00:11 +0300 Subject: Add MergeRequest#branch_merge_base_commit method Signed-off-by: Dmitriy Zaporozhets --- app/models/merge_request.rb | 35 +++++++++++++++++++++++++++-------- 1 file changed, 27 insertions(+), 8 deletions(-) (limited to 'app/models') diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index 56d5157298f..7e3444882ea 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -177,8 +177,8 @@ class MergeRequest < ActiveRecord::Base def diff_base_commit if persisted? merge_request_diff.base_commit - elsif diff_start_commit && diff_head_commit - self.target_project.merge_base_commit(diff_start_sha, diff_head_sha) + else + branch_merge_base_commit end end @@ -239,6 +239,15 @@ class MergeRequest < ActiveRecord::Base target_project.repository.commit(target_branch) if target_branch_ref end + def branch_merge_base_commit + start_sha = target_branch_sha + head_sha = source_branch_sha + + if start_sha && head_sha + target_project.merge_base_commit(start_sha, head_sha) + end + end + def target_branch_sha @target_branch_sha || target_branch_head.try(:sha) end @@ -247,15 +256,25 @@ class MergeRequest < ActiveRecord::Base @source_branch_sha || source_branch_head.try(:sha) end + def branch_merge_base_sha + branch_merge_base_commit.try(:sha) + end + def diff_refs if merge_request_diff merge_request_diff.diff_refs - elsif diff_start_commit || diff_base_commit - Gitlab::Diff::DiffRefs.new( - base_sha: diff_base_sha, - start_sha: diff_start_sha, - head_sha: diff_head_sha - ) + else + start_sha = target_branch_sha + head_sha = source_branch_sha + base_sha = branch_merge_base_sha + + if start_sha || base_sha + Gitlab::Diff::DiffRefs.new( + base_sha: base_sha, + start_sha: start_sha, + head_sha: head_sha + ) + end end end -- cgit v1.2.1 From 28e33df46bdffac3dc9388b56035db38dcdab5e3 Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Tue, 9 Aug 2016 15:16:50 +0300 Subject: Load merge request versions without loading whole diff from database Signed-off-by: Dmitriy Zaporozhets --- app/models/merge_request_diff.rb | 4 ++++ 1 file changed, 4 insertions(+) (limited to 'app/models') diff --git a/app/models/merge_request_diff.rb b/app/models/merge_request_diff.rb index 074d8f5d40a..24e09c4d57c 100644 --- a/app/models/merge_request_diff.rb +++ b/app/models/merge_request_diff.rb @@ -30,6 +30,10 @@ class MergeRequestDiff < ActiveRecord::Base # It allows you to override variables like head_commit_sha before getting diff. after_create :save_git_content, unless: :importing? + def self.select_without_diff + select(column_names - ['st_diffs']) + end + # Collect information about commits and diff from repository # and save it to the database as serialized data def save_git_content -- cgit v1.2.1 From 3eae0641ef0708f9b223abbe0070e332ea0b20ac Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Wed, 10 Aug 2016 18:40:10 +0800 Subject: Introduce Pipeline#latest and Pipeline.latest_for: So that we could easily access it for the view --- app/models/ci/pipeline.rb | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) (limited to 'app/models') diff --git a/app/models/ci/pipeline.rb b/app/models/ci/pipeline.rb index bce6a992af6..bc1190537da 100644 --- a/app/models/ci/pipeline.rb +++ b/app/models/ci/pipeline.rb @@ -21,8 +21,12 @@ module Ci after_save :keep_around_commits # ref can't be HEAD or SHA, can only be branch/tag name - scope :latest_successful_for, ->(ref = default_branch) do - where(ref: ref).success.order(id: :desc).limit(1) + scope :latest_successful_for, ->(ref) do + latest(ref).success + end + + scope :latest_for, ->(ref) do + where(ref: ref).order(id: :desc).limit(1) end def self.truncate_sha(sha) @@ -98,6 +102,10 @@ module Ci end end + def latest + project.pipelines.latest_for(ref).first + end + def latest? return false unless ref commit = project.commit(ref) -- cgit v1.2.1 From 517249858e41694f51b67461b313d5a34c2a466c Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Wed, 10 Aug 2016 21:07:26 +0800 Subject: It's latest_for, not just latest --- app/models/ci/pipeline.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'app/models') diff --git a/app/models/ci/pipeline.rb b/app/models/ci/pipeline.rb index bc1190537da..50f9ee7fc66 100644 --- a/app/models/ci/pipeline.rb +++ b/app/models/ci/pipeline.rb @@ -22,7 +22,7 @@ module Ci # ref can't be HEAD or SHA, can only be branch/tag name scope :latest_successful_for, ->(ref) do - latest(ref).success + latest_for(ref).success end scope :latest_for, ->(ref) do -- cgit v1.2.1 From 4b559c9afb34b80b910efec514653c6ea65adba8 Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Thu, 11 Aug 2016 17:26:04 +0800 Subject: Reverse ref and sha in args and rename pipeline to pipeline_for --- app/models/merge_request.rb | 3 ++- app/models/project.rb | 7 ++++--- 2 files changed, 6 insertions(+), 4 deletions(-) (limited to 'app/models') diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index b1fb3ce5d69..7c8e938df75 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -666,7 +666,8 @@ class MergeRequest < ActiveRecord::Base end def pipeline - @pipeline ||= source_project.pipeline(diff_head_sha, source_branch) if diff_head_sha && source_project + return unless diff_head_sha && source_project + @pipeline ||= source_project.pipeline_for(source_branch, diff_head_sha) end def merge_commit diff --git a/app/models/project.rb b/app/models/project.rb index d306f86f783..dc9b4b38a10 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -1086,12 +1086,13 @@ class Project < ActiveRecord::Base !namespace.share_with_group_lock end - def pipeline(sha, ref) + def pipeline_for(ref, sha) pipelines.order(id: :desc).find_by(sha: sha, ref: ref) end - def ensure_pipeline(sha, ref, current_user = nil) - pipeline(sha, ref) || pipelines.create(sha: sha, ref: ref, user: current_user) + def ensure_pipeline(ref, sha, current_user = nil) + pipeline_for(ref, sha) || + pipelines.create(sha: sha, ref: ref, user: current_user) end def enable_ci -- cgit v1.2.1 From 0a9d9f7d5096aa742564e704a96fa7c40eeaf007 Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Thu, 11 Aug 2016 17:55:23 +0800 Subject: Fetch the current SHA if SHA was not passed --- app/models/project.rb | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) (limited to 'app/models') diff --git a/app/models/project.rb b/app/models/project.rb index dc9b4b38a10..2969bec0bf7 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -1086,7 +1086,8 @@ class Project < ActiveRecord::Base !namespace.share_with_group_lock end - def pipeline_for(ref, sha) + def pipeline_for(ref, sha = commit(ref).try(:sha)) + return unless sha pipelines.order(id: :desc).find_by(sha: sha, ref: ref) end -- cgit v1.2.1 From 2a435e1d116065496fcb82f9b2182f7037d4c8b3 Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Thu, 11 Aug 2016 18:04:52 +0800 Subject: Remove Pipeline#latest in favour of Project#pipeline_for(ref) --- app/models/ci/pipeline.rb | 4 ---- 1 file changed, 4 deletions(-) (limited to 'app/models') diff --git a/app/models/ci/pipeline.rb b/app/models/ci/pipeline.rb index 50f9ee7fc66..9621bddf8dc 100644 --- a/app/models/ci/pipeline.rb +++ b/app/models/ci/pipeline.rb @@ -102,10 +102,6 @@ module Ci end end - def latest - project.pipelines.latest_for(ref).first - end - def latest? return false unless ref commit = project.commit(ref) -- cgit v1.2.1 From d84aa560331a646016880e4d2c5c0a3b3d4b32a6 Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Thu, 11 Aug 2016 18:09:26 +0800 Subject: Make Pipeline.latest_successful_for return the record --- app/models/ci/pipeline.rb | 8 ++------ app/models/project.rb | 2 +- 2 files changed, 3 insertions(+), 7 deletions(-) (limited to 'app/models') diff --git a/app/models/ci/pipeline.rb b/app/models/ci/pipeline.rb index 9621bddf8dc..0289a51eedd 100644 --- a/app/models/ci/pipeline.rb +++ b/app/models/ci/pipeline.rb @@ -21,12 +21,8 @@ module Ci after_save :keep_around_commits # ref can't be HEAD or SHA, can only be branch/tag name - scope :latest_successful_for, ->(ref) do - latest_for(ref).success - end - - scope :latest_for, ->(ref) do - where(ref: ref).order(id: :desc).limit(1) + def self.latest_successful_for(ref) + where(ref: ref).order(id: :desc).success.first end def self.truncate_sha(sha) diff --git a/app/models/project.rb b/app/models/project.rb index 2969bec0bf7..7aa34cdeec8 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -432,7 +432,7 @@ class Project < ActiveRecord::Base # ref can't be HEAD, can only be branch/tag name or SHA def latest_successful_builds_for(ref = default_branch) - latest_pipeline = pipelines.latest_successful_for(ref).first + latest_pipeline = pipelines.latest_successful_for(ref) if latest_pipeline latest_pipeline.builds.latest.with_artifacts -- cgit v1.2.1 From 29ac60d7fbb8208880408dbf98a94acd0ae73730 Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Mon, 15 Aug 2016 15:20:36 +0300 Subject: Change the way old merge request diff handled Signed-off-by: Dmitriy Zaporozhets --- app/models/merge_request.rb | 2 +- app/models/merge_request_diff.rb | 29 ++++++++++++++++++----------- 2 files changed, 19 insertions(+), 12 deletions(-) (limited to 'app/models') diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index 7e3444882ea..b7c2afb0920 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -261,7 +261,7 @@ class MergeRequest < ActiveRecord::Base end def diff_refs - if merge_request_diff + if persisted? merge_request_diff.diff_refs else start_sha = target_branch_sha diff --git a/app/models/merge_request_diff.rb b/app/models/merge_request_diff.rb index 24e09c4d57c..4c18775c44a 100644 --- a/app/models/merge_request_diff.rb +++ b/app/models/merge_request_diff.rb @@ -22,10 +22,6 @@ class MergeRequestDiff < ActiveRecord::Base serialize :st_commits serialize :st_diffs - # For compatibility with old MergeRequestDiff which - # does not store those variables in database - after_initialize :ensure_commits_sha, if: :persisted? - # All diff information is collected from repository after object is created. # It allows you to override variables like head_commit_sha before getting diff. after_create :save_git_content, unless: :importing? @@ -50,6 +46,20 @@ class MergeRequestDiff < ActiveRecord::Base self.base_commit_sha ||= find_base_sha end + # This method will rely on repository branch sha + # in case start_commit_sha is nil. Its necesarry for old merge request diff + # created before version 8.4 to work + def safe_start_commit_sha + start_commit_sha || merge_request.target_branch_sha + end + + # This method will rely on repository branch sha + # in case head_commit_sha is nil. Its necesarry for old merge request diff + # created before version 8.4 to work + def safe_head_commit_sha + head_commit_sha || last_commit.try(:sha) || merge_request.source_branch_sha + end + def size real_size.presence || diffs.size end @@ -60,8 +70,8 @@ class MergeRequestDiff < ActiveRecord::Base begin compare = Gitlab::Git::Compare.new( repository.raw_repository, - start_commit_sha, - head_commit_sha + safe_start_commit_sha, + safe_head_commit_sha ) compare.diffs(options) end @@ -126,8 +136,8 @@ class MergeRequestDiff < ActiveRecord::Base Gitlab::Git::Compare.new( repository.raw_repository, - start_commit_sha, - head_commit_sha + safe_start_commit_sha, + safe_head_commit_sha ) end end @@ -216,9 +226,6 @@ class MergeRequestDiff < ActiveRecord::Base return unless head_commit_sha && start_commit_sha project.merge_base_commit(head_commit_sha, start_commit_sha).try(:sha) - rescue Rugged::OdbError - # In case head or start commit does not exist in the repository any more - nil end def utf8_st_diffs -- cgit v1.2.1 From 94a7198ade54595d72797cab09db2c2a89172535 Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Mon, 15 Aug 2016 16:25:29 +0300 Subject: Fix merge request diff create and head_commit_sha compatibility Signed-off-by: Dmitriy Zaporozhets --- app/models/merge_request_diff.rb | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-) (limited to 'app/models') diff --git a/app/models/merge_request_diff.rb b/app/models/merge_request_diff.rb index 4c18775c44a..950b00f1001 100644 --- a/app/models/merge_request_diff.rb +++ b/app/models/merge_request_diff.rb @@ -42,8 +42,19 @@ class MergeRequestDiff < ActiveRecord::Base def ensure_commits_sha self.start_commit_sha ||= merge_request.target_branch_sha - self.head_commit_sha ||= last_commit.try(:sha) || merge_request.source_branch_sha + self.head_commit_sha ||= merge_request.source_branch_sha self.base_commit_sha ||= find_base_sha + save + end + + # Override head_commit_sha to keep compatibility with merge request diff + # created before version 8.4 that does not store head_commit_sha in separate db field. + def head_commit_sha + if persisted? && super.nil? + last_commit.try(:sha) + else + super + end end # This method will rely on repository branch sha @@ -57,7 +68,7 @@ class MergeRequestDiff < ActiveRecord::Base # in case head_commit_sha is nil. Its necesarry for old merge request diff # created before version 8.4 to work def safe_head_commit_sha - head_commit_sha || last_commit.try(:sha) || merge_request.source_branch_sha + head_commit_sha || merge_request.source_branch_sha end def size @@ -111,7 +122,7 @@ class MergeRequestDiff < ActiveRecord::Base end def head_commit - return last_commit unless head_commit_sha + return unless head_commit_sha project.commit(head_commit_sha) end -- cgit v1.2.1 From 643a368fa437725cbfffcfdc251055c4d125438c Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Mon, 15 Aug 2016 17:57:19 +0300 Subject: Make merge request diff works with new FileCollection logic Signed-off-by: Dmitriy Zaporozhets --- app/models/merge_request.rb | 6 +++--- app/models/merge_request_diff.rb | 14 +++++++++----- 2 files changed, 12 insertions(+), 8 deletions(-) (limited to 'app/models') diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index 7c3845ef538..104f9c7223d 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -172,10 +172,10 @@ class MergeRequest < ActiveRecord::Base end def diffs(diff_options = nil) - if self.compare - self.compare.diffs(diff_options) + if compare + compare.diffs(diff_options) else - Gitlab::Diff::FileCollection::MergeRequest.new(self, diff_options: diff_options) + merge_request_diff.diffs(diff_options) end end diff --git a/app/models/merge_request_diff.rb b/app/models/merge_request_diff.rb index 8920641cfec..9a34d099acd 100644 --- a/app/models/merge_request_diff.rb +++ b/app/models/merge_request_diff.rb @@ -141,7 +141,13 @@ class MergeRequestDiff < ActiveRecord::Base base_commit_sha? && head_commit_sha? && start_commit_sha? end - private + def diffs(diff_options = nil) + Gitlab::Diff::FileCollection::MergeRequestDiff.new(self, diff_options: diff_options) + end + + def project + merge_request.target_project + end def compare @compare ||= @@ -157,6 +163,8 @@ class MergeRequestDiff < ActiveRecord::Base end end + private + def dump_commits(commits) commits.map(&:to_hash) end @@ -229,10 +237,6 @@ class MergeRequestDiff < ActiveRecord::Base update_columns_serialized(new_attributes) end - def project - merge_request.target_project - end - def repository project.repository end -- cgit v1.2.1 From 49d63dc131da899ac2c91c26fe7e22f02da34dbd Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Mon, 15 Aug 2016 18:11:18 +0300 Subject: Fix and refactor merge request diff_refs method Signed-off-by: Dmitriy Zaporozhets --- app/models/diff_note.rb | 6 +----- app/models/merge_request.rb | 17 ++--------------- app/models/merge_request_diff.rb | 2 +- 3 files changed, 4 insertions(+), 21 deletions(-) (limited to 'app/models') diff --git a/app/models/diff_note.rb b/app/models/diff_note.rb index c816deb4e0c..364e1a8341b 100644 --- a/app/models/diff_note.rb +++ b/app/models/diff_note.rb @@ -79,11 +79,7 @@ class DiffNote < Note end def noteable_diff_refs - if noteable.respond_to?(:diff_sha_refs) - noteable.diff_sha_refs - else - noteable.diff_refs - end + noteable.diff_refs end def set_original_position diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index 104f9c7223d..007c2aa74e1 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -270,7 +270,7 @@ class MergeRequest < ActiveRecord::Base end def diff_refs - if persisted? + if merge_request_diff && merge_request_diff.diff_refs_by_sha? merge_request_diff.diff_refs else start_sha = target_branch_sha @@ -287,19 +287,6 @@ class MergeRequest < ActiveRecord::Base end end - # Return diff_refs instance trying to not touch the git repository - def diff_sha_refs - if merge_request_diff && merge_request_diff.diff_refs_by_sha? - return Gitlab::Diff::DiffRefs.new( - base_sha: merge_request_diff.base_commit_sha, - start_sha: merge_request_diff.start_commit_sha, - head_sha: merge_request_diff.head_commit_sha - ) - else - diff_refs - end - end - def validate_branches if target_project == source_project && target_branch == source_branch errors.add :branch_conflict, "You can not use same project/branch for source and target" @@ -716,7 +703,7 @@ class MergeRequest < ActiveRecord::Base end def support_new_diff_notes? - diff_sha_refs && diff_sha_refs.complete? + diff_refs && diff_refs.complete? end def update_diff_notes_positions(old_diff_refs:, new_diff_refs:) diff --git a/app/models/merge_request_diff.rb b/app/models/merge_request_diff.rb index 9a34d099acd..a769d31ac93 100644 --- a/app/models/merge_request_diff.rb +++ b/app/models/merge_request_diff.rb @@ -128,7 +128,7 @@ class MergeRequestDiff < ActiveRecord::Base end def diff_refs - return unless start_commit || base_commit + return unless start_commit_sha || base_commit_sha Gitlab::Diff::DiffRefs.new( base_sha: base_commit_sha, -- cgit v1.2.1 From 11f840bfa5b93fdd0687c9c4f2a5a2e7abbc17ac Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Tue, 16 Aug 2016 21:04:06 +0800 Subject: An empty line after guard, feedback: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/5142#note_13904931 --- app/models/merge_request.rb | 1 + 1 file changed, 1 insertion(+) (limited to 'app/models') diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index cb60b626e75..96fecf7712a 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -676,6 +676,7 @@ class MergeRequest < ActiveRecord::Base def pipeline return unless diff_head_sha && source_project + @pipeline ||= source_project.pipeline_for(source_branch, diff_head_sha) end -- cgit v1.2.1 From e067e699c3250882ea08594243b6c6d3ad84d6e0 Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Fri, 19 Aug 2016 12:35:16 +0300 Subject: Make sure merge request is fetched before collecting base sha in merge request diff Signed-off-by: Dmitriy Zaporozhets --- app/models/merge_request_diff.rb | 1 + 1 file changed, 1 insertion(+) (limited to 'app/models') diff --git a/app/models/merge_request_diff.rb b/app/models/merge_request_diff.rb index a769d31ac93..42ab6b620bd 100644 --- a/app/models/merge_request_diff.rb +++ b/app/models/merge_request_diff.rb @@ -41,6 +41,7 @@ class MergeRequestDiff < ActiveRecord::Base end def ensure_commits_sha + merge_request.fetch_ref self.start_commit_sha ||= merge_request.target_branch_sha self.head_commit_sha ||= merge_request.source_branch_sha self.base_commit_sha ||= find_base_sha -- cgit v1.2.1 From 1aba3a5c0e7c2b727f4317aab19bbc662f0fd727 Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Fri, 19 Aug 2016 17:42:49 +0800 Subject: Unify pipeline_for(ref, nil) and pipeline_for(ref), feedback: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/5142#note_14073464 --- app/models/project.rb | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) (limited to 'app/models') diff --git a/app/models/project.rb b/app/models/project.rb index 678fca7afd1..e97e6abfef9 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -1094,8 +1094,11 @@ class Project < ActiveRecord::Base !namespace.share_with_group_lock end - def pipeline_for(ref, sha = commit(ref).try(:sha)) + def pipeline_for(ref, sha = nil) + sha ||= commit(ref).try(:sha) + return unless sha + pipelines.order(id: :desc).find_by(sha: sha, ref: ref) end -- cgit v1.2.1 From a927a9bf6baa99be94181c3ab980947621ae2fe0 Mon Sep 17 00:00:00 2001 From: Gokmen Goksel Date: Mon, 25 Jul 2016 20:59:39 -0700 Subject: Support integration with Koding (online IDE) Koding: #index: landing page for Koding integration If enabled it will provide a link to open remote Koding instance url for now we are also providing the sneak preview video for how integration works in detail. Repository: check whether .koding.yml file exists on repository Projects: landing page: show Run in IDE (Koding) button if repo has stack file Projects: MR: show Run in IDE Koding button if repo has stack file on active branch ProjectHelpers: add_koding_stack: stack generator for provided project With this helper we will auto-generate the required stack template for a given project. For the feature we can request this base template from the running Koding instance on integration. Currently this will provide users to create a t2.nano instance on aws and it'll automatically configures the instance for basic requirements. Projects: empty state and landing page provide shortcuts to create stack projects_helper: use branch on checkout and provide an entry point This ${var.koding_queryString_branch} will be replaced with the branch provided in query string which will allow us to use same stack template for different branches of the same repository. ref: https://github.com/koding/koding/pull/8597/commits/b8c0e43c4c24bf132670aa8a3cfb0d634acfd09b projects_helper: provide sha info in query string to use existing vms With this change we'll be able to query existing vms on Koding side based on the commit id that they've created. ref: https://github.com/koding/koding/pull/8597/commits/1d630fadf31963fa6ccd3bed92e526761a30a343 Integration: Docs: Koding documentation added Disable /koding route if integration is disabled Use application settings to enable Koding Projects_helper: better indentation with strip_heredoc usage Projects_helper: return koding_url as is if there is no project provided current_settings: set koding_enabled: false by default Koding_Controller: to render not_found once integration is disabled Dashboard_specs: update spec for Koding enabled case Projects_Helper: make repo dynamic ref: https://github.com/koding/koding/pull/8597/commits/4d615242f45aaea4c4986be84ecc612b0bb1514c Updated documentation to have right format --- app/models/application_setting.rb | 6 ++++++ app/models/repository.rb | 10 +++++++++- 2 files changed, 15 insertions(+), 1 deletion(-) (limited to 'app/models') diff --git a/app/models/application_setting.rb b/app/models/application_setting.rb index 8c19d9dc9c8..f0bcb2d7cda 100644 --- a/app/models/application_setting.rb +++ b/app/models/application_setting.rb @@ -55,6 +55,10 @@ class ApplicationSetting < ActiveRecord::Base presence: true, if: :akismet_enabled + validates :koding_url, + presence: true, + if: :koding_enabled + validates :max_attachment_size, presence: true, numericality: { only_integer: true, greater_than: 0 } @@ -149,6 +153,8 @@ class ApplicationSetting < ActiveRecord::Base two_factor_grace_period: 48, recaptcha_enabled: false, akismet_enabled: false, + koding_enabled: false, + koding_url: nil, repository_checks_enabled: true, disabled_oauth_sign_in_sources: [], send_user_confirmation_email: false, diff --git a/app/models/repository.rb b/app/models/repository.rb index 2494c266cd2..bdc3b9d1c1c 100644 --- a/app/models/repository.rb +++ b/app/models/repository.rb @@ -277,7 +277,7 @@ class Repository def cache_keys %i(size commit_count readme version contribution_guide changelog - license_blob license_key gitignore) + license_blob license_key gitignore koding_yml) end # Keys for data on branch/tag operations. @@ -553,6 +553,14 @@ class Repository end end + def koding_yml + return nil unless head_exists? + + cache.fetch(:koding_yml) do + file_on_head(/\A\.koding\.yml\z/) + end + end + def gitlab_ci_yml return nil unless head_exists? -- cgit v1.2.1 From 37bf35f0bcba28e271789542fb8c81a6c77236b6 Mon Sep 17 00:00:00 2001 From: Felipe Artur Date: Tue, 26 Jul 2016 18:21:20 -0300 Subject: Todos sorting dropdown --- app/models/concerns/issuable.rb | 19 ++++--------------- app/models/concerns/sortable.rb | 14 ++++++++++++++ app/models/todo.rb | 19 +++++++++++++++++++ 3 files changed, 37 insertions(+), 15 deletions(-) (limited to 'app/models') diff --git a/app/models/concerns/issuable.rb b/app/models/concerns/issuable.rb index cbae1cd439b..afb5ce37c06 100644 --- a/app/models/concerns/issuable.rb +++ b/app/models/concerns/issuable.rb @@ -131,7 +131,10 @@ module Issuable end def order_labels_priority(excluded_labels: []) - select("#{table_name}.*, (#{highest_label_priority(excluded_labels).to_sql}) AS highest_priority"). + condition_field = "#{table_name}.id" + highest_priority = highest_label_priority(name, condition_field, excluded_labels: excluded_labels).to_sql + + select("#{table_name}.*, (#{highest_priority}) AS highest_priority"). group(arel_table[:id]). reorder(Gitlab::Database.nulls_last_order('highest_priority', 'ASC')) end @@ -159,20 +162,6 @@ module Issuable grouping_columns end - - private - - def highest_label_priority(excluded_labels) - query = Label.select(Label.arel_table[:priority].minimum). - joins(:label_links). - where(label_links: { target_type: name }). - where("label_links.target_id = #{table_name}.id"). - reorder(nil) - - query.where.not(title: excluded_labels) if excluded_labels.present? - - query - end end def today? diff --git a/app/models/concerns/sortable.rb b/app/models/concerns/sortable.rb index 8b47b9e0abd..1ebecd86af9 100644 --- a/app/models/concerns/sortable.rb +++ b/app/models/concerns/sortable.rb @@ -35,5 +35,19 @@ module Sortable all end end + + private + + def highest_label_priority(object_types, condition_field, excluded_labels: []) + query = Label.select(Label.arel_table[:priority].minimum). + joins(:label_links). + where(label_links: { target_type: object_types }). + where("label_links.target_id = #{condition_field}"). + reorder(nil) + + query.where.not(title: excluded_labels) if excluded_labels.present? + + query + end end end diff --git a/app/models/todo.rb b/app/models/todo.rb index 8d7a5965aa1..6ae9956ade5 100644 --- a/app/models/todo.rb +++ b/app/models/todo.rb @@ -1,4 +1,6 @@ class Todo < ActiveRecord::Base + include Sortable + ASSIGNED = 1 MENTIONED = 2 BUILD_FAILED = 3 @@ -41,6 +43,23 @@ class Todo < ActiveRecord::Base after_save :keep_around_commit + class << self + def sort(method) + method == "priority" ? order_by_labels_priority : order_by(method) + end + + # Order by priority depending on which issue/merge request the Todo belongs to + # Todos with highest priority first then oldest todos + # Need to order by created_at last because of differences on Mysql and Postgres when joining by type "Merge_request/Issue" + def order_by_labels_priority + highest_priority = highest_label_priority(["Issue", "MergeRequest"], "todos.target_id").to_sql + + select("#{table_name}.*, (#{highest_priority}) AS highest_priority"). + order(Gitlab::Database.nulls_last_order('highest_priority', 'ASC')). + order('todos.created_at') + end + end + def build_failed? action == BUILD_FAILED end -- cgit v1.2.1 From d741303a5992b0d259a125baa62b66ca4df209da Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Fri, 19 Aug 2016 17:22:59 -0500 Subject: Call `set_discussion_id` again in DiffNote `before_validation` because the order is important --- app/models/diff_note.rb | 3 +++ 1 file changed, 3 insertions(+) (limited to 'app/models') diff --git a/app/models/diff_note.rb b/app/models/diff_note.rb index aa54189fea9..c8320ff87fa 100644 --- a/app/models/diff_note.rb +++ b/app/models/diff_note.rb @@ -16,6 +16,9 @@ class DiffNote < Note after_initialize :ensure_original_discussion_id before_validation :set_original_position, :update_position, on: :create before_validation :set_line_code, :set_original_discussion_id + # We need to do this again, because it's already in `Note`, but is affected by + # `update_position` and needs to run after that. + before_validation :set_discussion_id after_save :keep_around_commits class << self -- cgit v1.2.1 From 97dddf39cd98d49f02b9adae3b7aeb3b28c434a6 Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Fri, 19 Aug 2016 19:07:02 -0500 Subject: =?UTF-8?q?Disable=20=E2=80=9Cissue=20by=20email=E2=80=9D=20featur?= =?UTF-8?q?e=20until=20it=20uses=20a=20different=20token?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- app/models/project.rb | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) (limited to 'app/models') diff --git a/app/models/project.rb b/app/models/project.rb index f9c48a546e6..1855760e694 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -611,7 +611,10 @@ class Project < ActiveRecord::Base end def new_issue_address(author) - if Gitlab::IncomingEmail.enabled? && author + # This feature is disabled for the time being. + return nil + + if Gitlab::IncomingEmail.enabled? && author # rubocop:disable Lint/UnreachableCode Gitlab::IncomingEmail.reply_address( "#{path_with_namespace}+#{author.authentication_token}") end -- cgit v1.2.1 From 41d89533e61f6009b5d800afc00c184e2807eafd Mon Sep 17 00:00:00 2001 From: Stan Hu Date: Fri, 19 Aug 2016 22:22:52 -0700 Subject: Fix assorted rspec failures due to stale, cached user permissions RequestStore is disabled in tests, but the Ability class was caching user permissions based on the user and project ID of previous test runs. Revise code to use RequestStore only if it is active. --- app/models/ability.rb | 48 +++++++++++++++++++++++++++--------------------- 1 file changed, 27 insertions(+), 21 deletions(-) (limited to 'app/models') diff --git a/app/models/ability.rb b/app/models/ability.rb index 07f703f205d..b82632ccc0b 100644 --- a/app/models/ability.rb +++ b/app/models/ability.rb @@ -166,38 +166,44 @@ class Ability end def project_abilities(user, project) - rules = [] key = "/user/#{user.id}/project/#{project.id}" - RequestStore.store[key] ||= begin - # Push abilities on the users team role - rules.push(*project_team_rules(project.team, user)) + if RequestStore.active? + RequestStore.store[key] ||= uncached_project_abilities(user, project) + else + uncached_project_abilities(user, project) + end + end - owner = user.admin? || - project.owner == user || - (project.group && project.group.has_owner?(user)) + def uncached_project_abilities(user, project) + rules = [] + # Push abilities on the users team role + rules.push(*project_team_rules(project.team, user)) - if owner - rules.push(*project_owner_rules) - end + owner = user.admin? || + project.owner == user || + (project.group && project.group.has_owner?(user)) - if project.public? || (project.internal? && !user.external?) - rules.push(*public_project_rules) + if owner + rules.push(*project_owner_rules) + end - # Allow to read builds for internal projects - rules << :read_build if project.public_builds? + if project.public? || (project.internal? && !user.external?) + rules.push(*public_project_rules) - unless owner || project.team.member?(user) || project_group_member?(project, user) - rules << :request_access if project.request_access_enabled - end - end + # Allow to read builds for internal projects + rules << :read_build if project.public_builds? - if project.archived? - rules -= project_archived_rules + unless owner || project.team.member?(user) || project_group_member?(project, user) + rules << :request_access if project.request_access_enabled end + end - rules - project_disabled_features_rules(project) + if project.archived? + rules -= project_archived_rules end + + rules - project_disabled_features_rules(project) end def project_team_rules(team, user) -- cgit v1.2.1 From 1954bb17eca49d375c92a4b8fa7f52fa39873a7d Mon Sep 17 00:00:00 2001 From: Stan Hu Date: Sat, 20 Aug 2016 06:33:53 -0700 Subject: Make Ability#project_abilities return unique values and fix counts --- app/models/ability.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'app/models') diff --git a/app/models/ability.rb b/app/models/ability.rb index b82632ccc0b..a49dd703926 100644 --- a/app/models/ability.rb +++ b/app/models/ability.rb @@ -203,7 +203,7 @@ class Ability rules -= project_archived_rules end - rules - project_disabled_features_rules(project) + (rules - project_disabled_features_rules(project)).uniq end def project_team_rules(team, user) -- cgit v1.2.1 From 5cb488e8a1a10432c1c5a322b2d4748cb754277f Mon Sep 17 00:00:00 2001 From: Stan Hu Date: Sat, 20 Aug 2016 09:18:06 -0700 Subject: Fix Error 500 resulting when loading network graph `discussion_id` may not be present when the SELECT call for notes does not include this attribute. Don't attempt to set the discussion ID unless the model contains the attribute. Closes #21119, #21128 --- app/models/note.rb | 2 ++ 1 file changed, 2 insertions(+) (limited to 'app/models') diff --git a/app/models/note.rb b/app/models/note.rb index 3bbf5db0b70..f2656df028b 100644 --- a/app/models/note.rb +++ b/app/models/note.rb @@ -259,6 +259,8 @@ class Note < ActiveRecord::Base def ensure_discussion_id return unless self.persisted? + # Needed in case the SELECT statement doesn't ask for `discussion_id` + return unless self.has_attribute?(:discussion_id) return if self.discussion_id set_discussion_id -- cgit v1.2.1 From eb355dec8768ef128795309c6c9ffa296a6eee22 Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Mon, 22 Aug 2016 14:18:58 +0300 Subject: Restore diff_sha_refs method Signed-off-by: Dmitriy Zaporozhets --- app/models/diff_note.rb | 6 +++++- app/models/merge_request.rb | 31 ++++++++++++++++--------------- 2 files changed, 21 insertions(+), 16 deletions(-) (limited to 'app/models') diff --git a/app/models/diff_note.rb b/app/models/diff_note.rb index 0c23c1c1934..c8320ff87fa 100644 --- a/app/models/diff_note.rb +++ b/app/models/diff_note.rb @@ -118,7 +118,11 @@ class DiffNote < Note end def noteable_diff_refs - noteable.diff_refs + if noteable.respond_to?(:diff_sha_refs) + noteable.diff_sha_refs + else + noteable.diff_refs + end end def set_original_position diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index 14b785e6bd4..615e550cf05 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -265,28 +265,29 @@ class MergeRequest < ActiveRecord::Base @source_branch_sha || source_branch_head.try(:sha) end - def branch_merge_base_sha - branch_merge_base_commit.try(:sha) + def diff_refs + return unless diff_start_commit || diff_base_commit + + Gitlab::Diff::DiffRefs.new( + base_sha: diff_base_sha, + start_sha: diff_start_sha, + head_sha: diff_head_sha + ) end - def diff_refs + # Return diff_refs instance trying to not touch the git repository + def diff_sha_refs if merge_request_diff && merge_request_diff.diff_refs_by_sha? merge_request_diff.diff_refs else - start_sha = target_branch_sha - head_sha = source_branch_sha - base_sha = branch_merge_base_sha - - if start_sha || base_sha - Gitlab::Diff::DiffRefs.new( - base_sha: base_sha, - start_sha: start_sha, - head_sha: head_sha - ) - end + diff_refs end end + def branch_merge_base_sha + branch_merge_base_commit.try(:sha) + end + def validate_branches if target_project == source_project && target_branch == source_branch errors.add :branch_conflict, "You can not use same project/branch for source and target" @@ -748,7 +749,7 @@ class MergeRequest < ActiveRecord::Base end def has_complete_diff_refs? - diff_refs && diff_refs.complete? + diff_sha_refs && diff_sha_refs.complete? end def update_diff_notes_positions(old_diff_refs:, new_diff_refs:) -- cgit v1.2.1 From 8f9a7ca854ffda26c5ce9aed2aec10bf155d0463 Mon Sep 17 00:00:00 2001 From: Valery Sizov Date: Mon, 1 Aug 2016 18:34:17 +0300 Subject: Revert the revert of Optimistic Locking --- app/models/concerns/issuable.rb | 6 ++++++ 1 file changed, 6 insertions(+) (limited to 'app/models') diff --git a/app/models/concerns/issuable.rb b/app/models/concerns/issuable.rb index afb5ce37c06..8e11d4f57cf 100644 --- a/app/models/concerns/issuable.rb +++ b/app/models/concerns/issuable.rb @@ -87,6 +87,12 @@ module Issuable User.find(assignee_id_was).update_cache_counts if assignee_id_was assignee.update_cache_counts if assignee end + + # We want to use optimistic lock for cases when only title or description are involved + # http://api.rubyonrails.org/classes/ActiveRecord/Locking/Optimistic.html + def locking_enabled? + title_changed? || description_changed? + end end module ClassMethods -- cgit v1.2.1 From 7e6af85490e289ffcc868390654748009ecc67fa Mon Sep 17 00:00:00 2001 From: Patricio Cano Date: Mon, 22 Aug 2016 21:36:04 -0500 Subject: Also check if Akismet is enabled, before showing the `Submit as spam` button. --- app/models/concerns/spammable.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'app/models') diff --git a/app/models/concerns/spammable.rb b/app/models/concerns/spammable.rb index ce54fe5d3bf..1aa97debe42 100644 --- a/app/models/concerns/spammable.rb +++ b/app/models/concerns/spammable.rb @@ -23,7 +23,7 @@ module Spammable def submittable_as_spam? if user_agent_detail - user_agent_detail.submittable? + user_agent_detail.submittable? && current_application_settings.akismet_enabled else false end -- cgit v1.2.1 From 3c09000e18dcbf6a74ed1f749db3184e309cf081 Mon Sep 17 00:00:00 2001 From: Douglas Barbosa Alexandre Date: Wed, 10 Aug 2016 18:22:21 -0300 Subject: Does not halt the GitHub import process when an error occurs --- app/models/project.rb | 2 -- 1 file changed, 2 deletions(-) (limited to 'app/models') diff --git a/app/models/project.rb b/app/models/project.rb index 1855760e694..8cf093be4c3 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -471,8 +471,6 @@ class Project < ActiveRecord::Base end def reset_cache_and_import_attrs - update(import_error: nil) - ProjectCacheWorker.perform_async(self.id) self.import_data.destroy if self.import_data -- cgit v1.2.1 From cfe512d6e926a3f757d18e5269214c06b8f9643d Mon Sep 17 00:00:00 2001 From: Paco Guzman Date: Wed, 24 Aug 2016 13:13:26 +0200 Subject: Show "Create Merge Request" widget for push events to fork projects on the source project --- app/models/user.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'app/models') diff --git a/app/models/user.rb b/app/models/user.rb index 48e83ab7e56..ad3cfbc03e4 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -489,10 +489,10 @@ class User < ActiveRecord::Base (personal_projects.count.to_f / projects_limit) * 100 end - def recent_push(project_id = nil) + def recent_push(project_ids = nil) # Get push events not earlier than 2 hours ago events = recent_events.code_push.where("created_at > ?", Time.now - 2.hours) - events = events.where(project_id: project_id) if project_id + events = events.where(project_id: project_ids) if project_ids # Use the latest event that has not been pushed or merged recently events.recent.find do |event| -- cgit v1.2.1 From 23bed91b3fb21a92b836011677cc75c884188f10 Mon Sep 17 00:00:00 2001 From: De Wet Blomerus Date: Thu, 25 Aug 2016 04:55:32 +0200 Subject: rename Statuseable to HasStatus --- app/models/ci/pipeline.rb | 2 +- app/models/commit_status.rb | 2 +- app/models/concerns/has_status.rb | 93 ++++++++++++++++++++++++++++++++++++++ app/models/concerns/statuseable.rb | 93 -------------------------------------- 4 files changed, 95 insertions(+), 95 deletions(-) create mode 100644 app/models/concerns/has_status.rb delete mode 100644 app/models/concerns/statuseable.rb (limited to 'app/models') diff --git a/app/models/ci/pipeline.rb b/app/models/ci/pipeline.rb index 087abe4cbb1..255fb33bdeb 100644 --- a/app/models/ci/pipeline.rb +++ b/app/models/ci/pipeline.rb @@ -1,7 +1,7 @@ module Ci class Pipeline < ActiveRecord::Base extend Ci::Model - include Statuseable + include HasStatus self.table_name = 'ci_commits' diff --git a/app/models/commit_status.rb b/app/models/commit_status.rb index 84ceeac7d3e..9114af18496 100644 --- a/app/models/commit_status.rb +++ b/app/models/commit_status.rb @@ -1,5 +1,5 @@ class CommitStatus < ActiveRecord::Base - include Statuseable + include HasStatus include Importable self.table_name = 'ci_builds' diff --git a/app/models/concerns/has_status.rb b/app/models/concerns/has_status.rb new file mode 100644 index 00000000000..f7b8352405c --- /dev/null +++ b/app/models/concerns/has_status.rb @@ -0,0 +1,93 @@ +module HasStatus + extend ActiveSupport::Concern + + AVAILABLE_STATUSES = %w[created pending running success failed canceled skipped] + STARTED_STATUSES = %w[running success failed skipped] + ACTIVE_STATUSES = %w[pending running] + COMPLETED_STATUSES = %w[success failed canceled] + + class_methods do + def status_sql + scope = all.relevant + builds = scope.select('count(*)').to_sql + success = scope.success.select('count(*)').to_sql + ignored = scope.ignored.select('count(*)').to_sql if scope.respond_to?(:ignored) + ignored ||= '0' + pending = scope.pending.select('count(*)').to_sql + running = scope.running.select('count(*)').to_sql + canceled = scope.canceled.select('count(*)').to_sql + skipped = scope.skipped.select('count(*)').to_sql + + deduce_status = "(CASE + WHEN (#{builds})=0 THEN NULL + WHEN (#{builds})=(#{skipped}) THEN 'skipped' + WHEN (#{builds})=(#{success})+(#{ignored})+(#{skipped}) THEN 'success' + WHEN (#{builds})=(#{pending})+(#{skipped}) THEN 'pending' + WHEN (#{builds})=(#{canceled})+(#{success})+(#{ignored})+(#{skipped}) THEN 'canceled' + WHEN (#{running})+(#{pending})>0 THEN 'running' + ELSE 'failed' + END)" + + deduce_status + end + + def status + all.pluck(self.status_sql).first + end + + def started_at + all.minimum(:started_at) + end + + def finished_at + all.maximum(:finished_at) + end + end + + included do + validates :status, inclusion: { in: AVAILABLE_STATUSES } + + state_machine :status, initial: :created do + state :created, value: 'created' + state :pending, value: 'pending' + state :running, value: 'running' + state :failed, value: 'failed' + state :success, value: 'success' + state :canceled, value: 'canceled' + state :skipped, value: 'skipped' + end + + scope :created, -> { where(status: 'created') } + scope :relevant, -> { where.not(status: 'created') } + scope :running, -> { where(status: 'running') } + scope :pending, -> { where(status: 'pending') } + scope :success, -> { where(status: 'success') } + scope :failed, -> { where(status: 'failed') } + scope :canceled, -> { where(status: 'canceled') } + scope :skipped, -> { where(status: 'skipped') } + scope :running_or_pending, -> { where(status: [:running, :pending]) } + scope :finished, -> { where(status: [:success, :failed, :canceled]) } + end + + def started? + STARTED_STATUSES.include?(status) && started_at + end + + def active? + ACTIVE_STATUSES.include?(status) + end + + def complete? + COMPLETED_STATUSES.include?(status) + end + + private + + def calculate_duration + if started_at && finished_at + finished_at - started_at + elsif started_at + Time.now - started_at + end + end +end diff --git a/app/models/concerns/statuseable.rb b/app/models/concerns/statuseable.rb deleted file mode 100644 index 750f937b724..00000000000 --- a/app/models/concerns/statuseable.rb +++ /dev/null @@ -1,93 +0,0 @@ -module Statuseable - extend ActiveSupport::Concern - - AVAILABLE_STATUSES = %w[created pending running success failed canceled skipped] - STARTED_STATUSES = %w[running success failed skipped] - ACTIVE_STATUSES = %w[pending running] - COMPLETED_STATUSES = %w[success failed canceled] - - class_methods do - def status_sql - scope = all.relevant - builds = scope.select('count(*)').to_sql - success = scope.success.select('count(*)').to_sql - ignored = scope.ignored.select('count(*)').to_sql if scope.respond_to?(:ignored) - ignored ||= '0' - pending = scope.pending.select('count(*)').to_sql - running = scope.running.select('count(*)').to_sql - canceled = scope.canceled.select('count(*)').to_sql - skipped = scope.skipped.select('count(*)').to_sql - - deduce_status = "(CASE - WHEN (#{builds})=0 THEN NULL - WHEN (#{builds})=(#{skipped}) THEN 'skipped' - WHEN (#{builds})=(#{success})+(#{ignored})+(#{skipped}) THEN 'success' - WHEN (#{builds})=(#{pending})+(#{skipped}) THEN 'pending' - WHEN (#{builds})=(#{canceled})+(#{success})+(#{ignored})+(#{skipped}) THEN 'canceled' - WHEN (#{running})+(#{pending})>0 THEN 'running' - ELSE 'failed' - END)" - - deduce_status - end - - def status - all.pluck(self.status_sql).first - end - - def started_at - all.minimum(:started_at) - end - - def finished_at - all.maximum(:finished_at) - end - end - - included do - validates :status, inclusion: { in: AVAILABLE_STATUSES } - - state_machine :status, initial: :created do - state :created, value: 'created' - state :pending, value: 'pending' - state :running, value: 'running' - state :failed, value: 'failed' - state :success, value: 'success' - state :canceled, value: 'canceled' - state :skipped, value: 'skipped' - end - - scope :created, -> { where(status: 'created') } - scope :relevant, -> { where.not(status: 'created') } - scope :running, -> { where(status: 'running') } - scope :pending, -> { where(status: 'pending') } - scope :success, -> { where(status: 'success') } - scope :failed, -> { where(status: 'failed') } - scope :canceled, -> { where(status: 'canceled') } - scope :skipped, -> { where(status: 'skipped') } - scope :running_or_pending, -> { where(status: [:running, :pending]) } - scope :finished, -> { where(status: [:success, :failed, :canceled]) } - end - - def started? - STARTED_STATUSES.include?(status) && started_at - end - - def active? - ACTIVE_STATUSES.include?(status) - end - - def complete? - COMPLETED_STATUSES.include?(status) - end - - private - - def calculate_duration - if started_at && finished_at - finished_at - started_at - elsif started_at - Time.now - started_at - end - end -end -- cgit v1.2.1 From d64c15c5c391e8f49a9f261ba0e59bafb99d97fe Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Thu, 25 Aug 2016 10:59:30 +0300 Subject: Add code improvements to merge request version feature * Add MergeRequestDiff#latest? * Remove unnecessary variable assignment Signed-off-by: Dmitriy Zaporozhets --- app/models/merge_request_diff.rb | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) (limited to 'app/models') diff --git a/app/models/merge_request_diff.rb b/app/models/merge_request_diff.rb index 42ab6b620bd..e353bdb24b8 100644 --- a/app/models/merge_request_diff.rb +++ b/app/models/merge_request_diff.rb @@ -79,14 +79,10 @@ class MergeRequestDiff < ActiveRecord::Base def raw_diffs(options = {}) if options[:ignore_whitespace_change] @diffs_no_whitespace ||= - begin - compare = Gitlab::Git::Compare.new( - repository.raw_repository, - safe_start_commit_sha, - safe_head_commit_sha - ) - compare.diffs(options) - end + Gitlab::Git::Compare.new( + repository.raw_repository, + safe_start_commit_sha, + safe_head_commit_sha).diffs(options) else @raw_diffs ||= {} @raw_diffs[options] ||= load_diffs(st_diffs, options) @@ -164,6 +160,10 @@ class MergeRequestDiff < ActiveRecord::Base end end + def latest? + self == merge_request.merge_request_diff + end + private def dump_commits(commits) -- cgit v1.2.1 From a943ccf10ecf2af3331927cb83268fbc70f43634 Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Thu, 25 Aug 2016 11:58:17 +0300 Subject: Change the way merge request diff compare works * remove ref fetch (we do it during creation anyway) * remove safe_head_commit_sha for diff compare (do not depend on the source branch) Signed-off-by: Dmitriy Zaporozhets --- app/models/merge_request_diff.rb | 24 ++++++------------------ 1 file changed, 6 insertions(+), 18 deletions(-) (limited to 'app/models') diff --git a/app/models/merge_request_diff.rb b/app/models/merge_request_diff.rb index e353bdb24b8..445179a4487 100644 --- a/app/models/merge_request_diff.rb +++ b/app/models/merge_request_diff.rb @@ -65,13 +65,6 @@ class MergeRequestDiff < ActiveRecord::Base start_commit_sha || merge_request.target_branch_sha end - # This method will rely on repository branch sha - # in case head_commit_sha is nil. Its necesarry for old merge request diff - # created before version 8.4 to work - def safe_head_commit_sha - head_commit_sha || merge_request.source_branch_sha - end - def size real_size.presence || raw_diffs.size end @@ -82,7 +75,7 @@ class MergeRequestDiff < ActiveRecord::Base Gitlab::Git::Compare.new( repository.raw_repository, safe_start_commit_sha, - safe_head_commit_sha).diffs(options) + head_commit_sha).diffs(options) else @raw_diffs ||= {} @raw_diffs[options] ||= load_diffs(st_diffs, options) @@ -148,16 +141,11 @@ class MergeRequestDiff < ActiveRecord::Base def compare @compare ||= - begin - # Update ref for merge request - merge_request.fetch_ref - - Gitlab::Git::Compare.new( - repository.raw_repository, - safe_start_commit_sha, - safe_head_commit_sha - ) - end + Gitlab::Git::Compare.new( + repository.raw_repository, + safe_start_commit_sha, + head_commit_sha + ) end def latest? -- cgit v1.2.1 From 4c8e9a8d27c34fe97216e12d17782f982818fb5c Mon Sep 17 00:00:00 2001 From: "Z.J. van de Weg" Date: Thu, 18 Aug 2016 15:00:20 +0200 Subject: Remove gitorious --- app/models/application_setting.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'app/models') diff --git a/app/models/application_setting.rb b/app/models/application_setting.rb index f0bcb2d7cda..246477ffe88 100644 --- a/app/models/application_setting.rb +++ b/app/models/application_setting.rb @@ -146,7 +146,7 @@ class ApplicationSetting < ActiveRecord::Base default_project_visibility: Settings.gitlab.default_projects_features['visibility_level'], default_snippet_visibility: Settings.gitlab.default_projects_features['visibility_level'], domain_whitelist: Settings.gitlab['domain_whitelist'], - import_sources: %w[github bitbucket gitlab gitorious google_code fogbugz git gitlab_project], + import_sources: %w[github bitbucket gitlab google_code fogbugz git gitlab_project], shared_runners_enabled: Settings.gitlab_ci['shared_runners_enabled'], max_artifacts_size: Settings.artifacts['max_size'], require_two_factor_authentication: false, -- cgit v1.2.1 From 4a3d1a5838685ff95a6c9416bb3e453ad143f50d Mon Sep 17 00:00:00 2001 From: Sean McGivern Date: Wed, 24 Aug 2016 15:20:06 +0100 Subject: Handle case where conflicts aren't on disk yet --- app/models/merge_request.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'app/models') diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index 5330a07ee35..7fa52d0b7a7 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -778,7 +778,7 @@ class MergeRequest < ActiveRecord::Base begin @conflicts_can_be_resolved_in_ui = conflicts.files.each(&:lines) - rescue Gitlab::Conflict::Parser::ParserError, Gitlab::Conflict::FileCollection::ConflictSideMissing + rescue Rugged::OdbError, Gitlab::Conflict::Parser::ParserError, Gitlab::Conflict::FileCollection::ConflictSideMissing @conflicts_can_be_resolved_in_ui = false end end -- cgit v1.2.1 From 67beaf6fb989e4f7981769b1f01bf383fc0de652 Mon Sep 17 00:00:00 2001 From: Sean McGivern Date: Thu, 25 Aug 2016 12:32:35 +0100 Subject: Don't show conflicts when there are none --- app/models/merge_request.rb | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) (limited to 'app/models') diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index 7fa52d0b7a7..19a684d1d0c 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -777,7 +777,11 @@ class MergeRequest < ActiveRecord::Base return @conflicts_can_be_resolved_in_ui = false unless has_complete_diff_refs? begin - @conflicts_can_be_resolved_in_ui = conflicts.files.each(&:lines) + # Try to parse each conflict. If the MR's mergeable status hasn't been updated, + # ensure that we don't say there are conflicts to resolve when there are no conflict + # files. + conflicts.files.each(&:lines) + @conflicts_can_be_resolved_in_ui = conflicts.files.length > 0 rescue Rugged::OdbError, Gitlab::Conflict::Parser::ParserError, Gitlab::Conflict::FileCollection::ConflictSideMissing @conflicts_can_be_resolved_in_ui = false end -- cgit v1.2.1 From 6280fd3777920670ee42111fddf29576cbf85988 Mon Sep 17 00:00:00 2001 From: Paco Guzman Date: Tue, 23 Aug 2016 11:27:22 +0200 Subject: Reduce number of database queries on builds tab --- app/models/ci/build.rb | 2 +- app/models/ci/pipeline.rb | 2 +- app/models/commit_status.rb | 2 ++ 3 files changed, 4 insertions(+), 2 deletions(-) (limited to 'app/models') diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb index 096b3b801af..23c8de6f650 100644 --- a/app/models/ci/build.rb +++ b/app/models/ci/build.rb @@ -352,7 +352,7 @@ module Ci end def artifacts? - !artifacts_expired? && artifacts_file.exists? + !artifacts_expired? && self[:artifacts_file].present? end def artifacts_metadata? diff --git a/app/models/ci/pipeline.rb b/app/models/ci/pipeline.rb index 087abe4cbb1..aff908ea16c 100644 --- a/app/models/ci/pipeline.rb +++ b/app/models/ci/pipeline.rb @@ -83,7 +83,7 @@ module Ci end def stages_with_latest_statuses - statuses.latest.order(:stage_idx).group_by(&:stage) + statuses.latest.includes(project: :namespace).order(:stage_idx).group_by(&:stage) end def project_id diff --git a/app/models/commit_status.rb b/app/models/commit_status.rb index 84ceeac7d3e..1b403fa646d 100644 --- a/app/models/commit_status.rb +++ b/app/models/commit_status.rb @@ -25,6 +25,8 @@ class CommitStatus < ActiveRecord::Base scope :retried, -> { where.not(id: latest) } scope :ordered, -> { order(:name) } scope :ignored, -> { where(allow_failure: true, status: [:failed, :canceled]) } + scope :latest_ci_stages, -> { latest.ordered.includes(project: :namespace) } + scope :retried_ci_stages, -> { retried.ordered.includes(project: :namespace) } state_machine :status do event :enqueue do -- cgit v1.2.1 From 79fd77953005062bfd89161770d1c0166774bb3d Mon Sep 17 00:00:00 2001 From: Sean McGivern Date: Thu, 25 Aug 2016 13:46:59 +0100 Subject: Allow setting branch refs for MR --- app/models/merge_request.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'app/models') diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index 5330a07ee35..918be8b7e66 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -238,12 +238,12 @@ class MergeRequest < ActiveRecord::Base def source_branch_head source_branch_ref = @source_branch_sha || source_branch - source_project.repository.commit(source_branch) if source_branch_ref + source_project.repository.commit(source_branch_ref) if source_branch_ref end def target_branch_head target_branch_ref = @target_branch_sha || target_branch - target_project.repository.commit(target_branch) if target_branch_ref + target_project.repository.commit(target_branch_ref) if target_branch_ref end def target_branch_sha -- cgit v1.2.1 From f16eabfc3335119015302798cb270445ddc0dc89 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hannes=20Rosen=C3=B6gger?= <123haynes@gmail.com> Date: Fri, 12 Aug 2016 16:29:13 +0200 Subject: Display project icon from default branch This commit makes sure that the project icon is being read from the default branch instead of 'master' --- app/models/project.rb | 1 + app/models/repository.rb | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) (limited to 'app/models') diff --git a/app/models/project.rb b/app/models/project.rb index 8cf093be4c3..0e4fb94f8eb 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -1035,6 +1035,7 @@ class Project < ActiveRecord::Base "refs/heads/#{branch}", force: true) repository.copy_gitattributes(branch) + repository.expire_avatar_cache(branch) reload_default_branch end diff --git a/app/models/repository.rb b/app/models/repository.rb index bdc3b9d1c1c..91bdafdac99 100644 --- a/app/models/repository.rb +++ b/app/models/repository.rb @@ -1065,7 +1065,7 @@ class Repository @avatar ||= cache.fetch(:avatar) do AVATAR_FILES.find do |file| - blob_at_branch('master', file) + blob_at_branch(root_ref, file) end end end -- cgit v1.2.1 From 6686084c6502f10fd7e6b8963ab52526cb6831bf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alejandro=20Rodr=C3=ADguez?= Date: Fri, 26 Aug 2016 17:20:00 -0300 Subject: Fix "Wiki" link not appearing in navigation for projects with external wiki --- app/models/ability.rb | 2 +- app/models/project.rb | 4 ++++ 2 files changed, 5 insertions(+), 1 deletion(-) (limited to 'app/models') diff --git a/app/models/ability.rb b/app/models/ability.rb index a49dd703926..c1df4a865f6 100644 --- a/app/models/ability.rb +++ b/app/models/ability.rb @@ -355,7 +355,7 @@ class Ability rules += named_abilities('project_snippet') end - unless project.wiki_enabled + unless project.has_wiki? rules += named_abilities('wiki') end diff --git a/app/models/project.rb b/app/models/project.rb index 0e4fb94f8eb..0fa41ebbec3 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -680,6 +680,10 @@ class Project < ActiveRecord::Base update_column(:has_external_issue_tracker, services.external_issue_trackers.any?) end + def has_wiki? + wiki_enabled? || has_external_wiki? + end + def external_wiki if has_external_wiki.nil? cache_has_external_wiki # Populate -- cgit v1.2.1 From 76872372376e57cd7d55ba9b9c63b25fe53c82df Mon Sep 17 00:00:00 2001 From: barthc Date: Wed, 17 Aug 2016 12:21:06 +0100 Subject: prevent authored awardable thumbs votes prevent authored awardable thumbs votes prevent authored awardable thumbs votes --- app/models/concerns/awardable.rb | 9 +++++++++ app/models/concerns/issuable.rb | 4 ++++ app/models/note.rb | 4 ++++ 3 files changed, 17 insertions(+) (limited to 'app/models') diff --git a/app/models/concerns/awardable.rb b/app/models/concerns/awardable.rb index 800a16ab246..e25420c0edf 100644 --- a/app/models/concerns/awardable.rb +++ b/app/models/concerns/awardable.rb @@ -59,6 +59,15 @@ module Awardable true end + def awardable_votes?(name) + AwardEmoji::UPVOTE_NAME == name || AwardEmoji::DOWNVOTE_NAME == name + end + + def user_can_award?(current_user, name) + name = normalize_name(name) + !(self.user_authored?(current_user) && awardable_votes?(name)) + end + def awarded_emoji?(emoji_name, current_user) award_emoji.where(name: emoji_name, user: current_user).exists? end diff --git a/app/models/concerns/issuable.rb b/app/models/concerns/issuable.rb index 8e11d4f57cf..22231b2e0f0 100644 --- a/app/models/concerns/issuable.rb +++ b/app/models/concerns/issuable.rb @@ -196,6 +196,10 @@ module Issuable end end + def user_authored?(user) + user == author + end + def subscribed_without_subscriptions?(user) participants(user).include?(user) end diff --git a/app/models/note.rb b/app/models/note.rb index f2656df028b..b94e3cff2ce 100644 --- a/app/models/note.rb +++ b/app/models/note.rb @@ -223,6 +223,10 @@ class Note < ActiveRecord::Base end end + def user_authored?(user) + user == author + end + def award_emoji? can_be_award_emoji? && contains_emoji_only? end -- cgit v1.2.1 From 2b33b24a3a5174cb391bf6c93643838f743e3dd2 Mon Sep 17 00:00:00 2001 From: Clement Ho Date: Sat, 27 Aug 2016 12:36:08 -0500 Subject: Shorten task status phrase --- app/models/concerns/taskable.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'app/models') diff --git a/app/models/concerns/taskable.rb b/app/models/concerns/taskable.rb index df2a9e3e84b..a3ac577cf3e 100644 --- a/app/models/concerns/taskable.rb +++ b/app/models/concerns/taskable.rb @@ -52,11 +52,11 @@ module Taskable end # Return a string that describes the current state of this Taskable's task - # list items, e.g. "20 tasks (12 completed, 8 remaining)" + # list items, e.g. "12 of 20 tasks completed" def task_status return '' if description.blank? sum = tasks.summary - "#{sum.item_count} tasks (#{sum.complete_count} completed, #{sum.incomplete_count} remaining)" + "#{sum.complete_count} of #{sum.item_count} #{'task'.pluralize(sum.item_count)} completed" end end -- cgit v1.2.1 From 43d50117187db1d8e034dbfc01e894a108f55369 Mon Sep 17 00:00:00 2001 From: Sean McGivern Date: Thu, 25 Aug 2016 16:42:20 +0100 Subject: Fix diff comments on legacy MRs --- app/models/legacy_diff_note.rb | 4 ++++ 1 file changed, 4 insertions(+) (limited to 'app/models') diff --git a/app/models/legacy_diff_note.rb b/app/models/legacy_diff_note.rb index 40277a9b139..0e1649aafe5 100644 --- a/app/models/legacy_diff_note.rb +++ b/app/models/legacy_diff_note.rb @@ -53,6 +53,10 @@ class LegacyDiffNote < Note self.line_code end + def to_discussion + Discussion.new([self]) + end + # Check if this note is part of an "active" discussion # # This will always return true for anything except MergeRequest noteables, -- cgit v1.2.1 From c9c2503c5186a38302ed606f793b52ffa394f52c Mon Sep 17 00:00:00 2001 From: Katarzyna Kobierska Date: Tue, 26 Jul 2016 13:57:43 +0200 Subject: User can edit closed MR with deleted fork Add test for closed MR without fork Add view test visibility of Reopen and Close buttons Fix controller tests and validation method Fix missing space Remove unused variables from test closed_without_fork? method refactoring Add information about missing fork When closed MR without fork can't edit target branch Tests for closed MR edit view Fix indentation and rebase, refactoring --- app/models/merge_request.rb | 33 ++++++++++++++++++--------------- 1 file changed, 18 insertions(+), 15 deletions(-) (limited to 'app/models') diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index 1d05e4a85d1..b41a1f0c547 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -91,13 +91,13 @@ class MergeRequest < ActiveRecord::Base end end - validates :source_project, presence: true, unless: [:allow_broken, :importing?] + validates :source_project, presence: true, unless: [:allow_broken, :importing?, :closed_without_fork?] validates :source_branch, presence: true validates :target_project, presence: true validates :target_branch, presence: true validates :merge_user, presence: true, if: :merge_when_build_succeeds? - validate :validate_branches, unless: [:allow_broken, :importing?] - validate :validate_fork + validate :validate_branches, unless: [:allow_broken, :importing?, :closed_without_fork?] + validate :validate_fork, unless: :closed_without_fork? scope :by_branch, ->(branch_name) { where("(source_branch LIKE :branch) OR (target_branch LIKE :branch)", branch: branch_name) } scope :cared, ->(user) { where('assignee_id = :user OR author_id = :user', user: user.id) } @@ -305,19 +305,22 @@ class MergeRequest < ActiveRecord::Base def validate_fork return true unless target_project && source_project + return true if target_project == source_project + return true unless fork_missing? - if target_project == source_project - true - else - # If source and target projects are different - # we should check if source project is actually a fork of target project - if source_project.forked_from?(target_project) - true - else - errors.add :validate_fork, - 'Source project is not a fork of target project' - end - end + errors.add :validate_fork, + 'Source project is not a fork of target project' + end + + def closed_without_fork? + closed? && fork_missing? + end + + def fork_missing? + return false unless for_fork? + return true unless source_project + + !source_project.forked_from?(target_project) end def ensure_merge_request_diff -- cgit v1.2.1 From 7226631102ef00c2d880bc6c1e099e52f4fa8659 Mon Sep 17 00:00:00 2001 From: Katarzyna Kobierska Date: Thu, 25 Aug 2016 15:08:31 +0200 Subject: Improve grammar and fix CHANGELOG --- app/models/merge_request.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'app/models') diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index b41a1f0c547..27ca5d119d5 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -309,7 +309,7 @@ class MergeRequest < ActiveRecord::Base return true unless fork_missing? errors.add :validate_fork, - 'Source project is not a fork of target project' + 'Source project is not a fork of the target project' end def closed_without_fork? -- cgit v1.2.1 From 2d8d94a788eb0bf3885ee67bda9638556425fa4b Mon Sep 17 00:00:00 2001 From: Katarzyna Kobierska Date: Tue, 30 Aug 2016 13:31:39 +0200 Subject: Change method name --- app/models/merge_request.rb | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) (limited to 'app/models') diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index 27ca5d119d5..a8dd4a306cf 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -306,17 +306,17 @@ class MergeRequest < ActiveRecord::Base def validate_fork return true unless target_project && source_project return true if target_project == source_project - return true unless fork_missing? + return true unless forked_source_project_missing? errors.add :validate_fork, 'Source project is not a fork of the target project' end def closed_without_fork? - closed? && fork_missing? + closed? && forked_source_project_missing? end - def fork_missing? + def forked_source_project_missing? return false unless for_fork? return true unless source_project -- cgit v1.2.1 From 9b57ad382e69044eb851f64cc0eb35896baa712a Mon Sep 17 00:00:00 2001 From: Sean McGivern Date: Tue, 30 Aug 2016 16:30:42 +0100 Subject: Move #to_discussion to NoteOnDiff --- app/models/concerns/note_on_diff.rb | 4 ++++ app/models/diff_note.rb | 4 ---- app/models/legacy_diff_note.rb | 4 ---- 3 files changed, 4 insertions(+), 8 deletions(-) (limited to 'app/models') diff --git a/app/models/concerns/note_on_diff.rb b/app/models/concerns/note_on_diff.rb index a881fb83b7f..b8dd27a7afe 100644 --- a/app/models/concerns/note_on_diff.rb +++ b/app/models/concerns/note_on_diff.rb @@ -28,4 +28,8 @@ module NoteOnDiff def can_be_award_emoji? false end + + def to_discussion + Discussion.new([self]) + end end diff --git a/app/models/diff_note.rb b/app/models/diff_note.rb index c8320ff87fa..4442cefc7e9 100644 --- a/app/models/diff_note.rb +++ b/app/models/diff_note.rb @@ -107,10 +107,6 @@ class DiffNote < Note self.noteable.find_diff_discussion(self.discussion_id) end - def to_discussion - Discussion.new([self]) - end - private def supported? diff --git a/app/models/legacy_diff_note.rb b/app/models/legacy_diff_note.rb index 0e1649aafe5..40277a9b139 100644 --- a/app/models/legacy_diff_note.rb +++ b/app/models/legacy_diff_note.rb @@ -53,10 +53,6 @@ class LegacyDiffNote < Note self.line_code end - def to_discussion - Discussion.new([self]) - end - # Check if this note is part of an "active" discussion # # This will always return true for anything except MergeRequest noteables, -- cgit v1.2.1 From 7d119bab4816355ce0156f24e0117e6e1d81588f Mon Sep 17 00:00:00 2001 From: "http://jneen.net/" Date: Tue, 9 Aug 2016 13:56:54 -0700 Subject: re-enable the cyclomatic complexity checker --- app/models/ability.rb | 1 - 1 file changed, 1 deletion(-) (limited to 'app/models') diff --git a/app/models/ability.rb b/app/models/ability.rb index c1df4a865f6..fcd7740d79f 100644 --- a/app/models/ability.rb +++ b/app/models/ability.rb @@ -1,6 +1,5 @@ class Ability class << self - # rubocop: disable Metrics/CyclomaticComplexity def allowed(user, subject) return anonymous_abilities(user, subject) if user.nil? return [] unless user.is_a?(User) -- cgit v1.2.1 From 99ee86206e3e19dd93910a4e7a3a5b6e3a7add9a Mon Sep 17 00:00:00 2001 From: "http://jneen.net/" Date: Mon, 8 Aug 2016 10:07:15 -0700 Subject: remove six, and use a Set instead --- app/models/ability.rb | 25 +++++++++++++++++++------ 1 file changed, 19 insertions(+), 6 deletions(-) (limited to 'app/models') diff --git a/app/models/ability.rb b/app/models/ability.rb index fcd7740d79f..622f481a4fc 100644 --- a/app/models/ability.rb +++ b/app/models/ability.rb @@ -1,7 +1,23 @@ class Ability class << self + + end + + def allowed?(user, action, subject) + allowed(user, subject).include?(action) + end + def allowed(user, subject) - return anonymous_abilities(user, subject) if user.nil? + return uncached_allowed(user, subject) unless RequestStore.active? + + user_key = user ? user.id : 'anonymous' + subject_key = subject ? "#{subject.class.name}/#{subject.id}" : 'global' + key = "/ability/#{user_key}/#{subject_key}" + RequestStore[key] ||= Set.new(uncached_allowed(user, subject)).freeze + end + + def uncached_allowed(user, subject) + return anonymous_abilities(subject) if user.nil? return [] unless user.is_a?(User) return [] if user.blocked? @@ -586,11 +602,8 @@ class Ability end def abilities - @abilities ||= begin - abilities = Six.new - abilities << self - abilities - end + warn 'Ability.abilities is deprecated, use Ability.allowed?(user, action, subject) instead' + self end private -- cgit v1.2.1 From 8702cef27146ab62d44065af3f3d388c7effcedb Mon Sep 17 00:00:00 2001 From: "http://jneen.net/" Date: Mon, 8 Aug 2016 14:02:29 -0700 Subject: don't double-cache project_abilites --- app/models/ability.rb | 13 ++----------- 1 file changed, 2 insertions(+), 11 deletions(-) (limited to 'app/models') diff --git a/app/models/ability.rb b/app/models/ability.rb index 622f481a4fc..595e6be6642 100644 --- a/app/models/ability.rb +++ b/app/models/ability.rb @@ -181,17 +181,8 @@ class Ability end def project_abilities(user, project) - key = "/user/#{user.id}/project/#{project.id}" - - if RequestStore.active? - RequestStore.store[key] ||= uncached_project_abilities(user, project) - else - uncached_project_abilities(user, project) - end - end - - def uncached_project_abilities(user, project) rules = [] + # Push abilities on the users team role rules.push(*project_team_rules(project.team, user)) @@ -218,7 +209,7 @@ class Ability rules -= project_archived_rules end - (rules - project_disabled_features_rules(project)).uniq + rules - project_disabled_features_rules(project) end def project_team_rules(team, user) -- cgit v1.2.1 From c218dd90dabb0ddff7fab09abbb348fe1c56201b Mon Sep 17 00:00:00 2001 From: "http://jneen.net/" Date: Tue, 23 Aug 2016 17:29:40 -0700 Subject: make almost everything on Ability private --- app/models/ability.rb | 90 +++++++++++++++++++++++++-------------------------- 1 file changed, 44 insertions(+), 46 deletions(-) (limited to 'app/models') diff --git a/app/models/ability.rb b/app/models/ability.rb index 595e6be6642..3eb8a5f6e03 100644 --- a/app/models/ability.rb +++ b/app/models/ability.rb @@ -1,6 +1,48 @@ class Ability class << self + # Given a list of users and a project this method returns the users that can + # read the given project. + def users_that_can_read_project(users, project) + if project.public? + users + else + users.select do |user| + if user.admin? + true + elsif project.internal? && !user.external? + true + elsif project.owner == user + true + elsif project.team.members.include?(user) + true + else + false + end + end + end + end + # Returns an Array of Issues that can be read by the given user. + # + # issues - The issues to reduce down to those readable by the user. + # user - The User for which to check the issues + def issues_readable_by_user(issues, user = nil) + return issues if user && user.admin? + + issues.select { |issue| issue.visible_to_user?(user) } + end + + # TODO: make this private and use the actual abilities stuff for this + def can_edit_note?(user, note) + return false if !note.editable? || !user.present? + return true if note.author == user || user.admin? + + if note.project + max_access_level = note.project.team.max_member_access(user.id) + max_access_level >= Gitlab::Access::MASTER + else + false + end end def allowed?(user, action, subject) @@ -16,6 +58,8 @@ class Ability RequestStore[key] ||= Set.new(uncached_allowed(user, subject)).freeze end + private + def uncached_allowed(user, subject) return anonymous_abilities(subject) if user.nil? return [] unless user.is_a?(User) @@ -44,38 +88,6 @@ class Ability end.concat(global_abilities(user)) end - # Given a list of users and a project this method returns the users that can - # read the given project. - def users_that_can_read_project(users, project) - if project.public? - users - else - users.select do |user| - if user.admin? - true - elsif project.internal? && !user.external? - true - elsif project.owner == user - true - elsif project.team.members.include?(user) - true - else - false - end - end - end - end - - # Returns an Array of Issues that can be read by the given user. - # - # issues - The issues to reduce down to those readable by the user. - # user - The User for which to check the issues - def issues_readable_by_user(issues, user = nil) - return issues if user && user.admin? - - issues.select { |issue| issue.visible_to_user?(user) } - end - # List of possible abilities for anonymous user def anonymous_abilities(user, subject) if subject.is_a?(PersonalSnippet) @@ -420,18 +432,6 @@ class Ability GroupProjectsFinder.new(group).execute(user).any? end - def can_edit_note?(user, note) - return false if !note.editable? || !user.present? - return true if note.author == user || user.admin? - - if note.project - max_access_level = note.project.team.max_member_access(user.id) - max_access_level >= Gitlab::Access::MASTER - else - false - end - end - def namespace_abilities(user, namespace) rules = [] @@ -597,8 +597,6 @@ class Ability self end - private - def restricted_public_level? current_application_settings.restricted_visibility_levels.include?(Gitlab::VisibilityLevel::PUBLIC) end -- cgit v1.2.1 From 5853c96b49010aaf33b85caeb94dfc18873d5656 Mon Sep 17 00:00:00 2001 From: "http://jneen.net/" Date: Mon, 8 Aug 2016 11:55:13 -0700 Subject: remove Ability.abilities --- app/models/ability.rb | 5 ----- app/models/event.rb | 2 +- app/models/merge_request.rb | 2 +- app/models/user.rb | 6 +----- 4 files changed, 3 insertions(+), 12 deletions(-) (limited to 'app/models') diff --git a/app/models/ability.rb b/app/models/ability.rb index 3eb8a5f6e03..891c5ba9276 100644 --- a/app/models/ability.rb +++ b/app/models/ability.rb @@ -592,11 +592,6 @@ class Ability [:read_user] end - def abilities - warn 'Ability.abilities is deprecated, use Ability.allowed?(user, action, subject) instead' - self - end - def restricted_public_level? current_application_settings.restricted_visibility_levels.include?(Gitlab::VisibilityLevel::PUBLIC) end diff --git a/app/models/event.rb b/app/models/event.rb index fd736d12359..a0b7b0dc2b5 100644 --- a/app/models/event.rb +++ b/app/models/event.rb @@ -65,7 +65,7 @@ class Event < ActiveRecord::Base elsif created_project? true elsif issue? || issue_note? - Ability.abilities.allowed?(user, :read_issue, note? ? note_target : target) + Ability.allowed?(user, :read_issue, note? ? note_target : target) else ((merge_request? || note?) && target.present?) || milestone? end diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index 62163e74000..57d673a5f25 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -411,7 +411,7 @@ class MergeRequest < ActiveRecord::Base def can_remove_source_branch?(current_user) !source_project.protected_branch?(source_branch) && !source_project.root_ref?(source_branch) && - Ability.abilities.allowed?(current_user, :push_code, source_project) && + Ability.allowed?(current_user, :push_code, source_project) && diff_head_commit == source_branch_head end diff --git a/app/models/user.rb b/app/models/user.rb index ad3cfbc03e4..8f5958333d7 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -460,16 +460,12 @@ class User < ActiveRecord::Base can?(:create_group, nil) end - def abilities - Ability.abilities - end - def can_select_namespace? several_namespaces? || admin end def can?(action, subject) - abilities.allowed?(self, action, subject) + Ability.allowed?(self, action, subject) end def first_name -- cgit v1.2.1 From e208765a92748086cacbc56225e827c8463750a5 Mon Sep 17 00:00:00 2001 From: "http://jneen.net/" Date: Thu, 11 Aug 2016 15:12:52 -0700 Subject: add policies, and factor out ProjectPolicy --- app/models/ability.rb | 35 ++++------------------------------- 1 file changed, 4 insertions(+), 31 deletions(-) (limited to 'app/models') diff --git a/app/models/ability.rb b/app/models/ability.rb index 891c5ba9276..4f0ffa09a1f 100644 --- a/app/models/ability.rb +++ b/app/models/ability.rb @@ -71,7 +71,7 @@ class Ability def abilities_by_subject_class(user:, subject:) case subject when CommitStatus then commit_status_abilities(user, subject) - when Project then project_abilities(user, subject) + when Project then ProjectPolicy.new(user, subject).abilities when Issue then issue_abilities(user, subject) when Note then note_abilities(user, subject) when ProjectSnippet then project_snippet_abilities(user, subject) @@ -85,7 +85,7 @@ class Ability when ExternalIssue, Deployment, Environment then project_abilities(user, subject.project) when Ci::Runner then runner_abilities(user, subject) else [] - end.concat(global_abilities(user)) + end + global_abilities(user) end # List of possible abilities for anonymous user @@ -193,35 +193,8 @@ class Ability end def project_abilities(user, project) - rules = [] - - # Push abilities on the users team role - rules.push(*project_team_rules(project.team, user)) - - owner = user.admin? || - project.owner == user || - (project.group && project.group.has_owner?(user)) - - if owner - rules.push(*project_owner_rules) - end - - if project.public? || (project.internal? && !user.external?) - rules.push(*public_project_rules) - - # Allow to read builds for internal projects - rules << :read_build if project.public_builds? - - unless owner || project.team.member?(user) || project_group_member?(project, user) - rules << :request_access if project.request_access_enabled - end - end - - if project.archived? - rules -= project_archived_rules - end - - rules - project_disabled_features_rules(project) + # temporary patch, deleteme before merge + ProjectPolicy.new(user, project).abilities.to_a end def project_team_rules(team, user) -- cgit v1.2.1 From 1ca9b3354a350b83d1e025b3d46280bc5bb60f2b Mon Sep 17 00:00:00 2001 From: "http://jneen.net/" Date: Fri, 12 Aug 2016 11:36:16 -0700 Subject: add support for anonymous abilities --- app/models/ability.rb | 194 ++------------------------------------------------ 1 file changed, 6 insertions(+), 188 deletions(-) (limited to 'app/models') diff --git a/app/models/ability.rb b/app/models/ability.rb index 4f0ffa09a1f..5d2cbde4c0e 100644 --- a/app/models/ability.rb +++ b/app/models/ability.rb @@ -71,7 +71,7 @@ class Ability def abilities_by_subject_class(user:, subject:) case subject when CommitStatus then commit_status_abilities(user, subject) - when Project then ProjectPolicy.new(user, subject).abilities + when Project then ProjectPolicy.abilities(user, subject) when Issue then issue_abilities(user, subject) when Note then note_abilities(user, subject) when ProjectSnippet then project_snippet_abilities(user, subject) @@ -96,8 +96,10 @@ class Ability anonymous_project_snippet_abilities(subject) elsif subject.is_a?(CommitStatus) anonymous_commit_status_abilities(subject) - elsif subject.is_a?(Project) || subject.respond_to?(:project) - anonymous_project_abilities(subject) + elsif subject.is_a?(Project) + ProjectPolicy.abilities(nil, subject) + elsif subject.respond_to?(:project) + ProjectPolicy.abilities(nil, subject.project) elsif subject.is_a?(Group) || subject.respond_to?(:group) anonymous_group_abilities(subject) elsif subject.is_a?(User) @@ -194,174 +196,7 @@ class Ability def project_abilities(user, project) # temporary patch, deleteme before merge - ProjectPolicy.new(user, project).abilities.to_a - end - - def project_team_rules(team, user) - # Rules based on role in project - if team.master?(user) - project_master_rules - elsif team.developer?(user) - project_dev_rules - elsif team.reporter?(user) - project_report_rules - elsif team.guest?(user) - project_guest_rules - else - [] - end - end - - def public_project_rules - @public_project_rules ||= project_guest_rules + [ - :download_code, - :fork_project, - :read_commit_status, - :read_pipeline, - :read_container_image - ] - end - - def project_guest_rules - @project_guest_rules ||= [ - :read_project, - :read_wiki, - :read_issue, - :read_board, - :read_list, - :read_label, - :read_milestone, - :read_project_snippet, - :read_project_member, - :read_merge_request, - :read_note, - :create_project, - :create_issue, - :create_note, - :upload_file - ] - end - - def project_report_rules - @project_report_rules ||= project_guest_rules + [ - :download_code, - :fork_project, - :create_project_snippet, - :update_issue, - :admin_issue, - :admin_label, - :admin_list, - :read_commit_status, - :read_build, - :read_container_image, - :read_pipeline, - :read_environment, - :read_deployment - ] - end - - def project_dev_rules - @project_dev_rules ||= project_report_rules + [ - :admin_merge_request, - :update_merge_request, - :create_commit_status, - :update_commit_status, - :create_build, - :update_build, - :create_pipeline, - :update_pipeline, - :create_merge_request, - :create_wiki, - :push_code, - :resolve_note, - :create_container_image, - :update_container_image, - :create_environment, - :create_deployment - ] - end - - def project_archived_rules - @project_archived_rules ||= [ - :create_merge_request, - :push_code, - :push_code_to_protected_branches, - :update_merge_request, - :admin_merge_request - ] - end - - def project_master_rules - @project_master_rules ||= project_dev_rules + [ - :push_code_to_protected_branches, - :update_project_snippet, - :update_environment, - :update_deployment, - :admin_milestone, - :admin_project_snippet, - :admin_project_member, - :admin_merge_request, - :admin_note, - :admin_wiki, - :admin_project, - :admin_commit_status, - :admin_build, - :admin_container_image, - :admin_pipeline, - :admin_environment, - :admin_deployment - ] - end - - def project_owner_rules - @project_owner_rules ||= project_master_rules + [ - :change_namespace, - :change_visibility_level, - :rename_project, - :remove_project, - :archive_project, - :remove_fork_project, - :destroy_merge_request, - :destroy_issue - ] - end - - def project_disabled_features_rules(project) - rules = [] - - unless project.issues_enabled - rules += named_abilities('issue') - end - - unless project.merge_requests_enabled - rules += named_abilities('merge_request') - end - - unless project.issues_enabled or project.merge_requests_enabled - rules += named_abilities('label') - rules += named_abilities('milestone') - end - - unless project.snippets_enabled - rules += named_abilities('project_snippet') - end - - unless project.has_wiki? - rules += named_abilities('wiki') - end - - unless project.builds_enabled - rules += named_abilities('build') - rules += named_abilities('pipeline') - rules += named_abilities('environment') - rules += named_abilities('deployment') - end - - unless project.container_registry_enabled - rules += named_abilities('container_image') - end - - rules + ProjectPolicy.abilities(user, project).to_a end def group_abilities(user, group) @@ -569,15 +404,6 @@ class Ability current_application_settings.restricted_visibility_levels.include?(Gitlab::VisibilityLevel::PUBLIC) end - def named_abilities(name) - [ - :"read_#{name}", - :"create_#{name}", - :"update_#{name}", - :"admin_#{name}" - ] - end - def filter_confidential_issues_abilities(user, issue, rules) return rules if user.admin? || !issue.confidential? @@ -589,13 +415,5 @@ class Ability rules end - - def project_group_member?(project, user) - project.group && - ( - project.group.members.exists?(user_id: user.id) || - project.group.requesters.exists?(user_id: user.id) - ) - end end end -- cgit v1.2.1 From 4d904bf3521b4600db228c48214f3892e86ac72a Mon Sep 17 00:00:00 2001 From: "http://jneen.net/" Date: Tue, 16 Aug 2016 11:10:34 -0700 Subject: port issues to Issu{able,e}Policy --- app/models/ability.rb | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) (limited to 'app/models') diff --git a/app/models/ability.rb b/app/models/ability.rb index 5d2cbde4c0e..1ea97855e04 100644 --- a/app/models/ability.rb +++ b/app/models/ability.rb @@ -72,7 +72,7 @@ class Ability case subject when CommitStatus then commit_status_abilities(user, subject) when Project then ProjectPolicy.abilities(user, subject) - when Issue then issue_abilities(user, subject) + when Issue then IssuePolicy.abilities(user, subject) when Note then note_abilities(user, subject) when ProjectSnippet then project_snippet_abilities(user, subject) when PersonalSnippet then personal_snippet_abilities(user, subject) @@ -89,7 +89,7 @@ class Ability end # List of possible abilities for anonymous user - def anonymous_abilities(user, subject) + def anonymous_abilities(subject) if subject.is_a?(PersonalSnippet) anonymous_personal_snippet_abilities(subject) elsif subject.is_a?(ProjectSnippet) @@ -98,6 +98,8 @@ class Ability anonymous_commit_status_abilities(subject) elsif subject.is_a?(Project) ProjectPolicy.abilities(nil, subject) + elsif subject.is_a?(Issue) + IssuePolicy.abilities(nil, subject) elsif subject.respond_to?(:project) ProjectPolicy.abilities(nil, subject.project) elsif subject.is_a?(Group) || subject.respond_to?(:group) -- cgit v1.2.1 From 092861093066f6b474c2dc72de34acf64380a3e6 Mon Sep 17 00:00:00 2001 From: "http://jneen.net/" Date: Tue, 16 Aug 2016 11:32:08 -0700 Subject: add and use MergeRequestPolicy --- app/models/ability.rb | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) (limited to 'app/models') diff --git a/app/models/ability.rb b/app/models/ability.rb index 1ea97855e04..b8e3e97b351 100644 --- a/app/models/ability.rb +++ b/app/models/ability.rb @@ -70,13 +70,14 @@ class Ability def abilities_by_subject_class(user:, subject:) case subject - when CommitStatus then commit_status_abilities(user, subject) when Project then ProjectPolicy.abilities(user, subject) when Issue then IssuePolicy.abilities(user, subject) + when MergeRequest then MergeRequestPolicy.abilities(user, subject) + + when CommitStatus then commit_status_abilities(user, subject) when Note then note_abilities(user, subject) when ProjectSnippet then project_snippet_abilities(user, subject) when PersonalSnippet then personal_snippet_abilities(user, subject) - when MergeRequest then merge_request_abilities(user, subject) when Group then group_abilities(user, subject) when Namespace then namespace_abilities(user, subject) when GroupMember then group_member_abilities(user, subject) @@ -100,6 +101,8 @@ class Ability ProjectPolicy.abilities(nil, subject) elsif subject.is_a?(Issue) IssuePolicy.abilities(nil, subject) + elsif subject.is_a?(MergeRequest) + MergeRequestPolicy.abilities(nil, subject) elsif subject.respond_to?(:project) ProjectPolicy.abilities(nil, subject.project) elsif subject.is_a?(Group) || subject.respond_to?(:group) -- cgit v1.2.1 From 16fe6dc7b159a0e6b68a586065de1f95d6acecfa Mon Sep 17 00:00:00 2001 From: "http://jneen.net/" Date: Tue, 16 Aug 2016 12:05:44 -0700 Subject: port CommitStatus/Build --- app/models/ability.rb | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) (limited to 'app/models') diff --git a/app/models/ability.rb b/app/models/ability.rb index b8e3e97b351..c89cc9b2e17 100644 --- a/app/models/ability.rb +++ b/app/models/ability.rb @@ -74,7 +74,8 @@ class Ability when Issue then IssuePolicy.abilities(user, subject) when MergeRequest then MergeRequestPolicy.abilities(user, subject) - when CommitStatus then commit_status_abilities(user, subject) + when Ci::Build then Ci::BuildPolicy.abilities(user, subject) + when CommitStatus then CommitStatus.abilities(user, subject) when Note then note_abilities(user, subject) when ProjectSnippet then project_snippet_abilities(user, subject) when PersonalSnippet then personal_snippet_abilities(user, subject) -- cgit v1.2.1 From 3656d3b88a01a50a5eaf66a16b6ac47d3c58352c Mon Sep 17 00:00:00 2001 From: "http://jneen.net/" Date: Tue, 16 Aug 2016 12:55:44 -0700 Subject: add automatic detection of the policy class --- app/models/ability.rb | 18 +++--------------- 1 file changed, 3 insertions(+), 15 deletions(-) (limited to 'app/models') diff --git a/app/models/ability.rb b/app/models/ability.rb index c89cc9b2e17..ac5e82c14d2 100644 --- a/app/models/ability.rb +++ b/app/models/ability.rb @@ -61,6 +61,9 @@ class Ability private def uncached_allowed(user, subject) + policy_class = BasePolicy.class_for(subject) rescue nil + return policy_class.abilities(user, subject) if policy_class + return anonymous_abilities(subject) if user.nil? return [] unless user.is_a?(User) return [] if user.blocked? @@ -70,13 +73,6 @@ class Ability def abilities_by_subject_class(user:, subject:) case subject - when Project then ProjectPolicy.abilities(user, subject) - when Issue then IssuePolicy.abilities(user, subject) - when MergeRequest then MergeRequestPolicy.abilities(user, subject) - - when Ci::Build then Ci::BuildPolicy.abilities(user, subject) - when CommitStatus then CommitStatus.abilities(user, subject) - when Note then note_abilities(user, subject) when ProjectSnippet then project_snippet_abilities(user, subject) when PersonalSnippet then personal_snippet_abilities(user, subject) when Group then group_abilities(user, subject) @@ -96,14 +92,6 @@ class Ability anonymous_personal_snippet_abilities(subject) elsif subject.is_a?(ProjectSnippet) anonymous_project_snippet_abilities(subject) - elsif subject.is_a?(CommitStatus) - anonymous_commit_status_abilities(subject) - elsif subject.is_a?(Project) - ProjectPolicy.abilities(nil, subject) - elsif subject.is_a?(Issue) - IssuePolicy.abilities(nil, subject) - elsif subject.is_a?(MergeRequest) - MergeRequestPolicy.abilities(nil, subject) elsif subject.respond_to?(:project) ProjectPolicy.abilities(nil, subject.project) elsif subject.is_a?(Group) || subject.respond_to?(:group) -- cgit v1.2.1 From 3fdcebfdda31b0cc0f5641489bb4066b1f815df3 Mon Sep 17 00:00:00 2001 From: "http://jneen.net/" Date: Tue, 16 Aug 2016 14:09:44 -0700 Subject: trim dead code --- app/models/ability.rb | 81 --------------------------------------------------- 1 file changed, 81 deletions(-) (limited to 'app/models') diff --git a/app/models/ability.rb b/app/models/ability.rb index ac5e82c14d2..323597c8888 100644 --- a/app/models/ability.rb +++ b/app/models/ability.rb @@ -73,7 +73,6 @@ class Ability def abilities_by_subject_class(user:, subject:) case subject - when ProjectSnippet then project_snippet_abilities(user, subject) when PersonalSnippet then personal_snippet_abilities(user, subject) when Group then group_abilities(user, subject) when Namespace then namespace_abilities(user, subject) @@ -140,13 +139,6 @@ class Ability end end - def anonymous_commit_status_abilities(subject) - rules = anonymous_project_abilities(subject.project) - # If subject is Ci::Build which inherits from CommitStatus filter the abilities - rules = filter_build_abilities(rules) if subject.is_a?(Ci::Build) - rules - end - def anonymous_group_abilities(subject) rules = [] @@ -169,14 +161,6 @@ class Ability end end - def anonymous_project_snippet_abilities(snippet) - if snippet.public? - [:read_project_snippet] - else - [] - end - end - def anonymous_user_abilities [:read_user] unless restricted_public_level? end @@ -248,46 +232,6 @@ class Ability rules.flatten end - [:issue, :merge_request].each do |name| - define_method "#{name}_abilities" do |user, subject| - rules = [] - - if subject.author == user || (subject.respond_to?(:assignee) && subject.assignee == user) - rules += [ - :"read_#{name}", - :"update_#{name}", - ] - end - - rules += project_abilities(user, subject.project) - rules = filter_confidential_issues_abilities(user, subject, rules) if subject.is_a?(Issue) - rules - end - end - - def note_abilities(user, note) - rules = [] - - if note.author == user - rules += [ - :read_note, - :update_note, - :admin_note, - :resolve_note - ] - end - - if note.respond_to?(:project) && note.project - rules += project_abilities(user, note.project) - end - - if note.for_merge_request? && note.noteable.author == user - rules << :resolve_note - end - - rules - end - def personal_snippet_abilities(user, snippet) rules = [] @@ -306,24 +250,6 @@ class Ability rules end - def project_snippet_abilities(user, snippet) - rules = [] - - if snippet.author == user || user.admin? - rules += [ - :read_project_snippet, - :update_project_snippet, - :admin_project_snippet - ] - end - - if snippet.public? || (snippet.internal? && !user.external?) || (snippet.private? && snippet.project.team.member?(user)) - rules << :read_project_snippet - end - - rules - end - def group_member_abilities(user, subject) rules = [] target_user = subject.user @@ -362,13 +288,6 @@ class Ability rules end - def commit_status_abilities(user, subject) - rules = project_abilities(user, subject.project) - # If subject is Ci::Build which inherits from CommitStatus filter the abilities - rules = filter_build_abilities(rules) if subject.is_a?(Ci::Build) - rules - end - def filter_build_abilities(rules) # If we can't read build we should also not have that # ability when looking at this in context of commit_status -- cgit v1.2.1 From 4016c5351362a409b9d8bb258e0330089cdb4394 Mon Sep 17 00:00:00 2001 From: "http://jneen.net/" Date: Tue, 16 Aug 2016 15:31:01 -0700 Subject: port personal snippets --- app/models/ability.rb | 33 +-------------------------------- 1 file changed, 1 insertion(+), 32 deletions(-) (limited to 'app/models') diff --git a/app/models/ability.rb b/app/models/ability.rb index 323597c8888..c5392379b32 100644 --- a/app/models/ability.rb +++ b/app/models/ability.rb @@ -73,7 +73,6 @@ class Ability def abilities_by_subject_class(user:, subject:) case subject - when PersonalSnippet then personal_snippet_abilities(user, subject) when Group then group_abilities(user, subject) when Namespace then namespace_abilities(user, subject) when GroupMember then group_member_abilities(user, subject) @@ -87,11 +86,7 @@ class Ability # List of possible abilities for anonymous user def anonymous_abilities(subject) - if subject.is_a?(PersonalSnippet) - anonymous_personal_snippet_abilities(subject) - elsif subject.is_a?(ProjectSnippet) - anonymous_project_snippet_abilities(subject) - elsif subject.respond_to?(:project) + if subject.respond_to?(:project) ProjectPolicy.abilities(nil, subject.project) elsif subject.is_a?(Group) || subject.respond_to?(:group) anonymous_group_abilities(subject) @@ -153,14 +148,6 @@ class Ability rules end - def anonymous_personal_snippet_abilities(snippet) - if snippet.public? - [:read_personal_snippet] - else - [] - end - end - def anonymous_user_abilities [:read_user] unless restricted_public_level? end @@ -232,24 +219,6 @@ class Ability rules.flatten end - def personal_snippet_abilities(user, snippet) - rules = [] - - if snippet.author == user - rules += [ - :read_personal_snippet, - :update_personal_snippet, - :admin_personal_snippet - ] - end - - if snippet.public? || (snippet.internal? && !user.external?) - rules << :read_personal_snippet - end - - rules - end - def group_member_abilities(user, subject) rules = [] target_user = subject.user -- cgit v1.2.1 From ccfa032ebc101339c1c0842d0fbeb5b555db9278 Mon Sep 17 00:00:00 2001 From: "http://jneen.net/" Date: Tue, 16 Aug 2016 16:28:47 -0700 Subject: port groups --- app/models/ability.rb | 39 +++------------------------------------ 1 file changed, 3 insertions(+), 36 deletions(-) (limited to 'app/models') diff --git a/app/models/ability.rb b/app/models/ability.rb index c5392379b32..2360bf3d46c 100644 --- a/app/models/ability.rb +++ b/app/models/ability.rb @@ -73,7 +73,6 @@ class Ability def abilities_by_subject_class(user:, subject:) case subject - when Group then group_abilities(user, subject) when Namespace then namespace_abilities(user, subject) when GroupMember then group_member_abilities(user, subject) when ProjectMember then project_member_abilities(user, subject) @@ -88,8 +87,8 @@ class Ability def anonymous_abilities(subject) if subject.respond_to?(:project) ProjectPolicy.abilities(nil, subject.project) - elsif subject.is_a?(Group) || subject.respond_to?(:group) - anonymous_group_abilities(subject) + elsif subject.respond_to?(:group) + GroupPolicy.abilities(nil, subject.group) elsif subject.is_a?(User) anonymous_user_abilities else @@ -164,38 +163,6 @@ class Ability ProjectPolicy.abilities(user, project).to_a end - def group_abilities(user, group) - rules = [] - rules << :read_group if can_read_group?(user, group) - - owner = user.admin? || group.has_owner?(user) - master = owner || group.has_master?(user) - - # Only group masters and group owners can create new projects - if master - rules += [ - :create_projects, - :admin_milestones - ] - end - - # Only group owner and administrators can admin group - if owner - rules += [ - :admin_group, - :admin_namespace, - :admin_group_member, - :change_visibility_level - ] - end - - if group.public? || (group.internal? && !user.external?) - rules << :request_access if group.request_access_enabled && group.users.exclude?(user) - end - - rules.flatten - end - def can_read_group?(user, group) return true if user.admin? return true if group.public? @@ -225,7 +192,7 @@ class Ability group = subject.group unless group.last_owner?(target_user) - can_manage = group_abilities(user, group).include?(:admin_group_member) + can_manage = allowed?(user, :admin_group_member, group) if can_manage rules << :update_group_member -- cgit v1.2.1 From 2944022835d872b472d8691082ef67aa3057d2b4 Mon Sep 17 00:00:00 2001 From: "http://jneen.net/" Date: Tue, 16 Aug 2016 16:29:10 -0700 Subject: trim more dead code --- app/models/ability.rb | 53 +-------------------------------------------------- 1 file changed, 1 insertion(+), 52 deletions(-) (limited to 'app/models') diff --git a/app/models/ability.rb b/app/models/ability.rb index 2360bf3d46c..794fb1223e3 100644 --- a/app/models/ability.rb +++ b/app/models/ability.rb @@ -96,57 +96,6 @@ class Ability end end - def anonymous_project_abilities(subject) - project = if subject.is_a?(Project) - subject - else - subject.project - end - - if project && project.public? - rules = [ - :read_project, - :read_board, - :read_list, - :read_wiki, - :read_label, - :read_milestone, - :read_project_snippet, - :read_project_member, - :read_merge_request, - :read_note, - :read_pipeline, - :read_commit_status, - :read_container_image, - :download_code - ] - - # Allow to read builds by anonymous user if guests are allowed - rules << :read_build if project.public_builds? - - # Allow to read issues by anonymous user if issue is not confidential - rules << :read_issue unless subject.is_a?(Issue) && subject.confidential? - - rules - project_disabled_features_rules(project) - else - [] - end - end - - def anonymous_group_abilities(subject) - rules = [] - - group = if subject.is_a?(Group) - subject - else - subject.group - end - - rules << :read_group if group.public? - - rules - end - def anonymous_user_abilities [:read_user] unless restricted_public_level? end @@ -211,7 +160,7 @@ class Ability project = subject.project unless target_user == project.owner - can_manage = project_abilities(user, project).include?(:admin_project_member) + can_manage = allowed?(user, :admin_project_member, project) if can_manage rules << :update_project_member -- cgit v1.2.1 From 5019185edd7718b262eb5ae94f21763f230f0557 Mon Sep 17 00:00:00 2001 From: "http://jneen.net/" Date: Thu, 18 Aug 2016 09:52:35 -0700 Subject: port runners, namespaces, group/project_members --- app/models/ability.rb | 58 --------------------------------------------------- 1 file changed, 58 deletions(-) (limited to 'app/models') diff --git a/app/models/ability.rb b/app/models/ability.rb index 794fb1223e3..7c4210f0706 100644 --- a/app/models/ability.rb +++ b/app/models/ability.rb @@ -73,12 +73,8 @@ class Ability def abilities_by_subject_class(user:, subject:) case subject - when Namespace then namespace_abilities(user, subject) - when GroupMember then group_member_abilities(user, subject) - when ProjectMember then project_member_abilities(user, subject) when User then user_abilities when ExternalIssue, Deployment, Environment then project_abilities(user, subject.project) - when Ci::Runner then runner_abilities(user, subject) else [] end + global_abilities(user) end @@ -112,48 +108,6 @@ class Ability ProjectPolicy.abilities(user, project).to_a end - def can_read_group?(user, group) - return true if user.admin? - return true if group.public? - return true if group.internal? && !user.external? - return true if group.users.include?(user) - - GroupProjectsFinder.new(group).execute(user).any? - end - - def namespace_abilities(user, namespace) - rules = [] - - # Only namespace owner and administrators can admin it - if namespace.owner == user || user.admin? - rules += [ - :create_projects, - :admin_namespace - ] - end - - rules.flatten - end - - def group_member_abilities(user, subject) - rules = [] - target_user = subject.user - group = subject.group - - unless group.last_owner?(target_user) - can_manage = allowed?(user, :admin_group_member, group) - - if can_manage - rules << :update_group_member - rules << :destroy_group_member - elsif user == target_user - rules << :destroy_group_member - end - end - - rules - end - def project_member_abilities(user, subject) rules = [] target_user = subject.user @@ -182,18 +136,6 @@ class Ability rules end - def runner_abilities(user, runner) - if user.is_admin? - [:assign_runner] - elsif runner.is_shared? || runner.locked? - [] - elsif user.ci_authorized_runners.include?(runner) - [:assign_runner] - else - [] - end - end - def user_abilities [:read_user] end -- cgit v1.2.1 From a340829c42617b40696408c3097d6476970e8b87 Mon Sep 17 00:00:00 2001 From: "http://jneen.net/" Date: Thu, 18 Aug 2016 09:59:17 -0700 Subject: port UserPolicy --- app/models/ability.rb | 11 ----------- 1 file changed, 11 deletions(-) (limited to 'app/models') diff --git a/app/models/ability.rb b/app/models/ability.rb index 7c4210f0706..fe171cd1a8b 100644 --- a/app/models/ability.rb +++ b/app/models/ability.rb @@ -73,7 +73,6 @@ class Ability def abilities_by_subject_class(user:, subject:) case subject - when User then user_abilities when ExternalIssue, Deployment, Environment then project_abilities(user, subject.project) else [] end + global_abilities(user) @@ -85,17 +84,11 @@ class Ability ProjectPolicy.abilities(nil, subject.project) elsif subject.respond_to?(:group) GroupPolicy.abilities(nil, subject.group) - elsif subject.is_a?(User) - anonymous_user_abilities else [] end end - def anonymous_user_abilities - [:read_user] unless restricted_public_level? - end - def global_abilities(user) rules = [] rules << :create_group if user.can_create_group @@ -136,10 +129,6 @@ class Ability rules end - def user_abilities - [:read_user] - end - def restricted_public_level? current_application_settings.restricted_visibility_levels.include?(Gitlab::VisibilityLevel::PUBLIC) end -- cgit v1.2.1 From 06ba2602c59e5f6627d892ed9fdb2dafade5768b Mon Sep 17 00:00:00 2001 From: "http://jneen.net/" Date: Thu, 18 Aug 2016 10:39:49 -0700 Subject: take the dive - only use abilities from Policies --- app/models/ability.rb | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) (limited to 'app/models') diff --git a/app/models/ability.rb b/app/models/ability.rb index fe171cd1a8b..b57ada715df 100644 --- a/app/models/ability.rb +++ b/app/models/ability.rb @@ -61,14 +61,7 @@ class Ability private def uncached_allowed(user, subject) - policy_class = BasePolicy.class_for(subject) rescue nil - return policy_class.abilities(user, subject) if policy_class - - return anonymous_abilities(subject) if user.nil? - return [] unless user.is_a?(User) - return [] if user.blocked? - - abilities_by_subject_class(user: user, subject: subject) + BasePolicy.class_for(subject).abilities(user, subject) end def abilities_by_subject_class(user:, subject:) -- cgit v1.2.1 From b3b7fb1fe7b876487b1464aa5779bacec7276742 Mon Sep 17 00:00:00 2001 From: "http://jneen.net/" Date: Tue, 23 Aug 2016 17:56:41 -0700 Subject: remove the rest of the dead code --- app/models/ability.rb | 74 --------------------------------------------------- 1 file changed, 74 deletions(-) (limited to 'app/models') diff --git a/app/models/ability.rb b/app/models/ability.rb index b57ada715df..8ccbb9bee9c 100644 --- a/app/models/ability.rb +++ b/app/models/ability.rb @@ -63,79 +63,5 @@ class Ability def uncached_allowed(user, subject) BasePolicy.class_for(subject).abilities(user, subject) end - - def abilities_by_subject_class(user:, subject:) - case subject - when ExternalIssue, Deployment, Environment then project_abilities(user, subject.project) - else [] - end + global_abilities(user) - end - - # List of possible abilities for anonymous user - def anonymous_abilities(subject) - if subject.respond_to?(:project) - ProjectPolicy.abilities(nil, subject.project) - elsif subject.respond_to?(:group) - GroupPolicy.abilities(nil, subject.group) - else - [] - end - end - - def global_abilities(user) - rules = [] - rules << :create_group if user.can_create_group - rules << :read_users_list - rules - end - - def project_abilities(user, project) - # temporary patch, deleteme before merge - ProjectPolicy.abilities(user, project).to_a - end - - def project_member_abilities(user, subject) - rules = [] - target_user = subject.user - project = subject.project - - unless target_user == project.owner - can_manage = allowed?(user, :admin_project_member, project) - - if can_manage - rules << :update_project_member - rules << :destroy_project_member - elsif user == target_user - rules << :destroy_project_member - end - end - - rules - end - - def filter_build_abilities(rules) - # If we can't read build we should also not have that - # ability when looking at this in context of commit_status - %w(read create update admin).each do |rule| - rules.delete(:"#{rule}_commit_status") unless rules.include?(:"#{rule}_build") - end - rules - end - - def restricted_public_level? - current_application_settings.restricted_visibility_levels.include?(Gitlab::VisibilityLevel::PUBLIC) - end - - def filter_confidential_issues_abilities(user, issue, rules) - return rules if user.admin? || !issue.confidential? - - unless issue.author == user || issue.assignee == user || issue.project.team.member?(user, Gitlab::Access::REPORTER) - rules.delete(:admin_issue) - rules.delete(:read_issue) - rules.delete(:update_issue) - end - - rules - end end end -- cgit v1.2.1 From 57def53c84091a56f3a2443d214fe80f2c026d00 Mon Sep 17 00:00:00 2001 From: "http://jneen.net/" Date: Tue, 30 Aug 2016 11:09:21 -0700 Subject: factor out a RuleSet so that `delegate!` retains @cannot --- app/models/ability.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'app/models') diff --git a/app/models/ability.rb b/app/models/ability.rb index 8ccbb9bee9c..fa8f8bc3a5f 100644 --- a/app/models/ability.rb +++ b/app/models/ability.rb @@ -55,7 +55,7 @@ class Ability user_key = user ? user.id : 'anonymous' subject_key = subject ? "#{subject.class.name}/#{subject.id}" : 'global' key = "/ability/#{user_key}/#{subject_key}" - RequestStore[key] ||= Set.new(uncached_allowed(user, subject)).freeze + RequestStore[key] ||= uncached_allowed(user, subject).freeze end private -- cgit v1.2.1 From 29f818e6165e2e6d4a523270d115a491e261478a Mon Sep 17 00:00:00 2001 From: barthc Date: Tue, 30 Aug 2016 20:57:47 +0100 Subject: prevent authored awardable thumbs votes --- app/models/concerns/awardable.rb | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) (limited to 'app/models') diff --git a/app/models/concerns/awardable.rb b/app/models/concerns/awardable.rb index e25420c0edf..83f5bc1fa9e 100644 --- a/app/models/concerns/awardable.rb +++ b/app/models/concerns/awardable.rb @@ -64,8 +64,11 @@ module Awardable end def user_can_award?(current_user, name) - name = normalize_name(name) - !(self.user_authored?(current_user) && awardable_votes?(name)) + if user_authored?(current_user) + !awardable_votes?(normalize_name(name)) + else + true + end end def awarded_emoji?(emoji_name, current_user) -- cgit v1.2.1 From 9d8fbcc03847820eeda61e9d765693161f3619c5 Mon Sep 17 00:00:00 2001 From: Patricio Cano Date: Wed, 24 Aug 2016 17:08:23 -0500 Subject: Added project specific enable/disable setting for LFS --- app/models/project.rb | 4 ++++ 1 file changed, 4 insertions(+) (limited to 'app/models') diff --git a/app/models/project.rb b/app/models/project.rb index c34064f96ce..c271448946c 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -390,6 +390,10 @@ class Project < ActiveRecord::Base end end + def lfs_enabled? + (Gitlab.config.lfs.enabled && enable_lfs) || (enable_lfs.nil? && Gitlab.config.lfs.enabled) + end + def repository_storage_path Gitlab.config.repositories.storages[repository_storage] end -- cgit v1.2.1 From 63a97c11928d483cfad87d11f83c7df84c29743d Mon Sep 17 00:00:00 2001 From: Patricio Cano Date: Thu, 25 Aug 2016 17:59:31 -0500 Subject: Syntax and style fixes. --- app/models/project.rb | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) (limited to 'app/models') diff --git a/app/models/project.rb b/app/models/project.rb index c271448946c..7a5933bfe5e 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -391,7 +391,10 @@ class Project < ActiveRecord::Base end def lfs_enabled? - (Gitlab.config.lfs.enabled && enable_lfs) || (enable_lfs.nil? && Gitlab.config.lfs.enabled) + return false unless Gitlab.config.lfs.enabled + return Gitlab.config.lfs.enabled if enable_lfs.nil? + + enable_lfs end def repository_storage_path -- cgit v1.2.1 From cf37d623e197dae5cc7efb021c1b1d85ca9674ee Mon Sep 17 00:00:00 2001 From: Patricio Cano Date: Tue, 30 Aug 2016 17:17:45 -0500 Subject: Renamed `enable_lfs` to `lfs_enabled` for the Project field, and related fixes. --- app/models/project.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'app/models') diff --git a/app/models/project.rb b/app/models/project.rb index 7a5933bfe5e..e5027af4a0e 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -392,9 +392,9 @@ class Project < ActiveRecord::Base def lfs_enabled? return false unless Gitlab.config.lfs.enabled - return Gitlab.config.lfs.enabled if enable_lfs.nil? + return Gitlab.config.lfs.enabled if self[:lfs_enabled].nil? - enable_lfs + self[:lfs_enabled] end def repository_storage_path -- cgit v1.2.1 From a103a5d9cc2f496854d75492fe3f759fad0a913f Mon Sep 17 00:00:00 2001 From: Douglas Barbosa Alexandre Date: Tue, 30 Aug 2016 17:56:26 -0300 Subject: Add option to confidential issues events to trigger Webhooks --- app/models/hooks/web_hook.rb | 1 + 1 file changed, 1 insertion(+) (limited to 'app/models') diff --git a/app/models/hooks/web_hook.rb b/app/models/hooks/web_hook.rb index f365dee3141..595602e80fe 100644 --- a/app/models/hooks/web_hook.rb +++ b/app/models/hooks/web_hook.rb @@ -4,6 +4,7 @@ class WebHook < ActiveRecord::Base default_value_for :push_events, true default_value_for :issues_events, false + default_value_for :confidential_issues_events, false default_value_for :note_events, false default_value_for :merge_requests_events, false default_value_for :tag_push_events, false -- cgit v1.2.1 From dd64f8aaf867516043dbbf559595a0ed9671ab3b Mon Sep 17 00:00:00 2001 From: Douglas Barbosa Alexandre Date: Tue, 30 Aug 2016 18:39:25 -0300 Subject: Add option to confidential issues events to trigger services --- app/models/project_services/hipchat_service.rb | 2 +- app/models/project_services/slack_service.rb | 2 +- app/models/service.rb | 3 ++- 3 files changed, 4 insertions(+), 3 deletions(-) (limited to 'app/models') diff --git a/app/models/project_services/hipchat_service.rb b/app/models/project_services/hipchat_service.rb index d7c986c1a91..afebd3b6a12 100644 --- a/app/models/project_services/hipchat_service.rb +++ b/app/models/project_services/hipchat_service.rb @@ -39,7 +39,7 @@ class HipchatService < Service end def supported_events - %w(push issue merge_request note tag_push build) + %w(push issue confidential_issue merge_request note tag_push build) end def execute(data) diff --git a/app/models/project_services/slack_service.rb b/app/models/project_services/slack_service.rb index abbc780dc1a..e6c943db2bf 100644 --- a/app/models/project_services/slack_service.rb +++ b/app/models/project_services/slack_service.rb @@ -44,7 +44,7 @@ class SlackService < Service end def supported_events - %w(push issue merge_request note tag_push build wiki_page) + %w(push issue confidential_issue merge_request note tag_push build wiki_page) end def execute(data) diff --git a/app/models/service.rb b/app/models/service.rb index 09b4717a523..7333f8d381b 100644 --- a/app/models/service.rb +++ b/app/models/service.rb @@ -7,6 +7,7 @@ class Service < ActiveRecord::Base default_value_for :active, false default_value_for :push_events, true default_value_for :issues_events, true + default_value_for :confidential_issues_events, true default_value_for :merge_requests_events, true default_value_for :tag_push_events, true default_value_for :note_events, true @@ -100,7 +101,7 @@ class Service < ActiveRecord::Base end def supported_events - %w(push tag_push issue merge_request wiki_page) + %w(push tag_push issue confidential_issue merge_request wiki_page) end def execute(data) -- cgit v1.2.1 From 21f10af0956c69b6a5bad6b36b7d6e12e60e7867 Mon Sep 17 00:00:00 2001 From: Douglas Barbosa Alexandre Date: Tue, 30 Aug 2016 19:11:46 -0300 Subject: Scope hooks thal will run for confidential issues --- app/models/hooks/project_hook.rb | 1 + app/models/service.rb | 1 + 2 files changed, 2 insertions(+) (limited to 'app/models') diff --git a/app/models/hooks/project_hook.rb b/app/models/hooks/project_hook.rb index 836a75b0608..c631e7a7df5 100644 --- a/app/models/hooks/project_hook.rb +++ b/app/models/hooks/project_hook.rb @@ -2,6 +2,7 @@ class ProjectHook < WebHook belongs_to :project scope :issue_hooks, -> { where(issues_events: true) } + scope :confidential_issue_hooks, -> { where(confidential_issues_events: true) } scope :note_hooks, -> { where(note_events: true) } scope :merge_request_hooks, -> { where(merge_requests_events: true) } scope :build_hooks, -> { where(build_events: true) } diff --git a/app/models/service.rb b/app/models/service.rb index 7333f8d381b..198e7247838 100644 --- a/app/models/service.rb +++ b/app/models/service.rb @@ -34,6 +34,7 @@ class Service < ActiveRecord::Base scope :push_hooks, -> { where(push_events: true, active: true) } scope :tag_push_hooks, -> { where(tag_push_events: true, active: true) } scope :issue_hooks, -> { where(issues_events: true, active: true) } + scope :confidential_issue_hooks, -> { where(confidential_issues_events: true, active: true) } scope :merge_request_hooks, -> { where(merge_requests_events: true, active: true) } scope :note_hooks, -> { where(note_events: true, active: true) } scope :build_hooks, -> { where(build_events: true, active: true) } -- cgit v1.2.1 From e71df3cdd20d5c9bcf4ecc64771a1d88babfddf8 Mon Sep 17 00:00:00 2001 From: Andrew Smith Date: Tue, 30 Aug 2016 00:01:00 +1000 Subject: Order award tooltips by their created_at date --- app/models/concerns/awardable.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'app/models') diff --git a/app/models/concerns/awardable.rb b/app/models/concerns/awardable.rb index 800a16ab246..e6feaf7df4f 100644 --- a/app/models/concerns/awardable.rb +++ b/app/models/concerns/awardable.rb @@ -2,7 +2,7 @@ module Awardable extend ActiveSupport::Concern included do - has_many :award_emoji, -> { includes(:user) }, as: :awardable, dependent: :destroy + has_many :award_emoji, -> { includes(:user).order(:id) }, as: :awardable, dependent: :destroy if self < Participable # By default we always load award_emoji user association -- cgit v1.2.1 From 0d8352973bfbd3d1857657fce35284fcaf241cf7 Mon Sep 17 00:00:00 2001 From: winniehell Date: Sun, 17 Jul 2016 02:03:56 +0200 Subject: Use JavaScript tooltips for mentions (!5301) --- app/models/commit.rb | 9 --------- app/models/commit_range.rb | 7 ------- 2 files changed, 16 deletions(-) (limited to 'app/models') diff --git a/app/models/commit.rb b/app/models/commit.rb index 817d063e4a2..e64fd1e0c1b 100644 --- a/app/models/commit.rb +++ b/app/models/commit.rb @@ -108,15 +108,6 @@ class Commit @diff_line_count end - # Returns a string describing the commit for use in a link title - # - # Example - # - # "Commit: Alex Denisov - Project git clone panel" - def link_title - "Commit: #{author_name} - #{title}" - end - # Returns the commits title. # # Usually, the commit title is the first line of the commit message. diff --git a/app/models/commit_range.rb b/app/models/commit_range.rb index 630ee9601e0..656a242c265 100644 --- a/app/models/commit_range.rb +++ b/app/models/commit_range.rb @@ -4,12 +4,10 @@ # # range = CommitRange.new('f3f85602...e86e1013', project) # range.exclude_start? # => false -# range.reference_title # => "Commits f3f85602 through e86e1013" # range.to_s # => "f3f85602...e86e1013" # # range = CommitRange.new('f3f856029bc5f966c5a7ee24cf7efefdd20e6019..e86e1013709735be5bb767e2b228930c543f25ae', project) # range.exclude_start? # => true -# range.reference_title # => "Commits f3f85602^ through e86e1013" # range.to_param # => {from: "f3f856029bc5f966c5a7ee24cf7efefdd20e6019^", to: "e86e1013709735be5bb767e2b228930c543f25ae"} # range.to_s # => "f3f85602..e86e1013" # @@ -109,11 +107,6 @@ class CommitRange reference end - # Returns a String for use in a link's title attribute - def reference_title - "Commits #{sha_start} through #{sha_to}" - end - # Return a Hash of parameters for passing to a URL helper # # See `namespace_project_compare_url` -- cgit v1.2.1 From d326d3428da89b943bb5f1d4d396f21b3e999ff7 Mon Sep 17 00:00:00 2001 From: Stan Hu Date: Mon, 29 Aug 2016 07:46:30 -0700 Subject: Optimize branch lookups and force a repository reload for Repository#find_branch If `git gc` runs and `Repository` has an instance to `Rugged::Repository`, a bug in libgit2 may cause the instance to return a stale value or a missing branch. This change not only optimizes the branch lookup so we don't have to iterate through every branch, but it also works around the `git gc` issue by forcing a repository reload every time `Repository#find_branch` is called. See: https://github.com/libgit2/libgit2/issues/1534 Closes #15392, #21470 --- app/models/repository.rb | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) (limited to 'app/models') diff --git a/app/models/repository.rb b/app/models/repository.rb index 91bdafdac99..f891e8374d2 100644 --- a/app/models/repository.rb +++ b/app/models/repository.rb @@ -120,8 +120,21 @@ class Repository commits end - def find_branch(name) - raw_repository.branches.find { |branch| branch.name == name } + def find_branch(name, fresh_repo: true) + # Since the Repository object may have in-memory index changes, invalidating the memoized Repository object may + # cause unintended side effects. Because finding a branch is a read-only operation, we can safely instantiate + # a new repo here to ensure a consistent state to avoid a libgit2 bug where concurrent access (e.g. via git gc) + # may cause the branch to "disappear" erroneously or have the wrong SHA. + # + # See: https://github.com/libgit2/libgit2/issues/1534 and https://gitlab.com/gitlab-org/gitlab-ce/issues/15392 + raw_repo = + if fresh_repo + Gitlab::Git::Repository.new(path_to_repo) + else + raw_repository + end + + raw_repo.find_branch(name) end def find_tag(name) -- cgit v1.2.1 From ed51734030f94aa7e0636d8527b4bdae05c9de6b Mon Sep 17 00:00:00 2001 From: Tomasz Maczukin Date: Tue, 21 Jun 2016 13:12:02 +0200 Subject: Handle error on trace raw download with old builds (DB stored) --- app/models/ci/build.rb | 4 ++++ 1 file changed, 4 insertions(+) (limited to 'app/models') diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb index 23c8de6f650..76142c4b2b6 100644 --- a/app/models/ci/build.rb +++ b/app/models/ci/build.rb @@ -208,6 +208,10 @@ module Ci end end + def has_trace_file? + File.exist?(path_to_trace) || (project.ci_id && File.exist?(old_path_to_trace)) + end + def has_trace? raw_trace.present? end -- cgit v1.2.1 From 56011f9f697b7ca42f786735a2249014dd3ef18d Mon Sep 17 00:00:00 2001 From: Tomasz Maczukin Date: Thu, 25 Aug 2016 13:53:20 +0200 Subject: Refactorize CI::Build model --- app/models/ci/build.rb | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) (limited to 'app/models') diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb index 76142c4b2b6..8a8f848d328 100644 --- a/app/models/ci/build.rb +++ b/app/models/ci/build.rb @@ -209,7 +209,7 @@ module Ci end def has_trace_file? - File.exist?(path_to_trace) || (project.ci_id && File.exist?(old_path_to_trace)) + File.exist?(path_to_trace) || has_old_trace_file? end def has_trace? @@ -219,7 +219,7 @@ module Ci def raw_trace if File.file?(path_to_trace) File.read(path_to_trace) - elsif project.ci_id && File.file?(old_path_to_trace) + elsif has_old_trace_file? # Temporary fix for build trace data integrity File.read(old_path_to_trace) else @@ -228,6 +228,14 @@ module Ci end end + ## + # Deprecated + # + # This is a hotfix for CI build data integrity, see #4246 + def has_old_trace_file? + project.ci_id && File.exist?(old_path_to_trace) + end + def trace trace = raw_trace if project && trace.present? && project.runners_token.present? -- cgit v1.2.1 From c8861da76772d781f677a76506f590edc23ba251 Mon Sep 17 00:00:00 2001 From: Tomasz Maczukin Date: Tue, 30 Aug 2016 12:47:31 +0200 Subject: Update specs - add mocks to simulate old versions --- app/models/ci/build.rb | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) (limited to 'app/models') diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb index 8a8f848d328..f219cee4a62 100644 --- a/app/models/ci/build.rb +++ b/app/models/ci/build.rb @@ -217,7 +217,7 @@ module Ci end def raw_trace - if File.file?(path_to_trace) + if File.exist?(path_to_trace) File.read(path_to_trace) elsif has_old_trace_file? # Temporary fix for build trace data integrity @@ -274,6 +274,14 @@ module Ci end end + def trace_file_path + if has_old_trace_file? + old_path_to_trace + else + path_to_trace + end + end + def dir_to_trace File.join( Settings.gitlab_ci.builds_path, -- cgit v1.2.1 From 892dea67717c0efbd6a28f7639f34535ec0a8747 Mon Sep 17 00:00:00 2001 From: Felipe Artur Date: Mon, 1 Aug 2016 19:31:21 -0300 Subject: Project tools visibility level --- .../concerns/project_features_compatibility.rb | 37 +++++++++++++ app/models/project.rb | 27 ++++++---- app/models/project_feature.rb | 63 ++++++++++++++++++++++ app/models/user.rb | 2 +- 4 files changed, 118 insertions(+), 11 deletions(-) create mode 100644 app/models/concerns/project_features_compatibility.rb create mode 100644 app/models/project_feature.rb (limited to 'app/models') diff --git a/app/models/concerns/project_features_compatibility.rb b/app/models/concerns/project_features_compatibility.rb new file mode 100644 index 00000000000..9216122923e --- /dev/null +++ b/app/models/concerns/project_features_compatibility.rb @@ -0,0 +1,37 @@ +# Makes api V3 compatible with old project features permissions methods +# +# After migrating issues_enabled merge_requests_enabled builds_enabled snippets_enabled and wiki_enabled +# fields to a new table "project_features", support for the old fields is still needed in the API. + +module ProjectFeaturesCompatibility + extend ActiveSupport::Concern + + def wiki_enabled=(value) + write_feature_attribute(:wiki_access_level, value) + end + + def builds_enabled=(value) + write_feature_attribute(:builds_access_level, value) + end + + def merge_requests_enabled=(value) + write_feature_attribute(:merge_requests_access_level, value) + end + + def issues_enabled=(value) + write_feature_attribute(:issues_access_level, value) + end + + def snippets_enabled=(value) + write_feature_attribute(:snippets_access_level, value) + end + + private + + def write_feature_attribute(field, value) + build_project_feature unless project_feature + + access_level = value == "true" ? ProjectFeature::ENABLED : ProjectFeature::DISABLED + project_feature.update_attribute(field, access_level) + end +end diff --git a/app/models/project.rb b/app/models/project.rb index e5027af4a0e..a6de2c48071 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -11,24 +11,23 @@ class Project < ActiveRecord::Base include AfterCommitQueue include CaseSensitivity include TokenAuthenticatable + include ProjectFeaturesCompatibility extend Gitlab::ConfigHelper UNKNOWN_IMPORT_URL = 'http://unknown.git' + delegate :feature_available?, :builds_enabled?, :wiki_enabled?, :merge_requests_enabled?, to: :project_feature, allow_nil: true + default_value_for :archived, false default_value_for :visibility_level, gitlab_config_features.visibility_level - default_value_for :issues_enabled, gitlab_config_features.issues - default_value_for :merge_requests_enabled, gitlab_config_features.merge_requests - default_value_for :builds_enabled, gitlab_config_features.builds - default_value_for :wiki_enabled, gitlab_config_features.wiki - default_value_for :snippets_enabled, gitlab_config_features.snippets default_value_for :container_registry_enabled, gitlab_config_features.container_registry default_value_for(:repository_storage) { current_application_settings.repository_storage } default_value_for(:shared_runners_enabled) { current_application_settings.shared_runners_enabled } after_create :ensure_dir_exist after_save :ensure_dir_exist, if: :namespace_id_changed? + after_initialize :setup_project_feature # set last_activity_at to the same as created_at after_create :set_last_activity_at @@ -62,10 +61,10 @@ class Project < ActiveRecord::Base belongs_to :group, -> { where(type: Group) }, foreign_key: 'namespace_id' belongs_to :namespace - has_one :board, dependent: :destroy - has_one :last_event, -> {order 'events.created_at DESC'}, class_name: 'Event', foreign_key: 'project_id' + has_one :board, dependent: :destroy + # Project services has_many :services has_one :campfire_service, dependent: :destroy @@ -130,6 +129,7 @@ class Project < ActiveRecord::Base has_many :notification_settings, dependent: :destroy, as: :source has_one :import_data, dependent: :destroy, class_name: "ProjectImportData" + has_one :project_feature, dependent: :destroy has_many :commit_statuses, dependent: :destroy, class_name: 'CommitStatus', foreign_key: :gl_project_id has_many :pipelines, dependent: :destroy, class_name: 'Ci::Pipeline', foreign_key: :gl_project_id @@ -142,6 +142,7 @@ class Project < ActiveRecord::Base has_many :deployments, dependent: :destroy accepts_nested_attributes_for :variables, allow_destroy: true + accepts_nested_attributes_for :project_feature delegate :name, to: :owner, allow_nil: true, prefix: true delegate :members, to: :team, prefix: true @@ -159,8 +160,6 @@ class Project < ActiveRecord::Base length: { within: 0..255 }, format: { with: Gitlab::Regex.project_path_regex, message: Gitlab::Regex.project_path_regex_message } - validates :issues_enabled, :merge_requests_enabled, - :wiki_enabled, inclusion: { in: [true, false] } validates :namespace, presence: true validates_uniqueness_of :name, scope: :namespace_id validates_uniqueness_of :path, scope: :namespace_id @@ -196,6 +195,9 @@ class Project < ActiveRecord::Base scope :for_milestones, ->(ids) { joins(:milestones).where('milestones.id' => ids).distinct } scope :with_push, -> { joins(:events).where('events.action = ?', Event::PUSHED) } + scope :with_builds_enabled, -> { joins('LEFT JOIN project_features ON projects.id = project_features.project_id').where('project_features.builds_access_level IS NULL or project_features.builds_access_level > 0') } + scope :with_issues_enabled, -> { joins('LEFT JOIN project_features ON projects.id = project_features.project_id').where('project_features.issues_access_level IS NULL or project_features.issues_access_level > 0') } + scope :active, -> { joins(:issues, :notes, :merge_requests).order('issues.created_at, notes.created_at, merge_requests.created_at DESC') } scope :abandoned, -> { where('projects.last_activity_at < ?', 6.months.ago) } @@ -1121,7 +1123,7 @@ class Project < ActiveRecord::Base end def enable_ci - self.builds_enabled = true + project_feature.update_attribute(:builds_access_level, ProjectFeature::ENABLED) end def any_runners?(&block) @@ -1288,6 +1290,11 @@ class Project < ActiveRecord::Base private + # Prevents the creation of project_feature record for every project + def setup_project_feature + build_project_feature unless project_feature + end + def default_branch_protected? current_application_settings.default_branch_protection == Gitlab::Access::PROTECTION_FULL || current_application_settings.default_branch_protection == Gitlab::Access::PROTECTION_DEV_CAN_MERGE diff --git a/app/models/project_feature.rb b/app/models/project_feature.rb new file mode 100644 index 00000000000..9c602c582bd --- /dev/null +++ b/app/models/project_feature.rb @@ -0,0 +1,63 @@ +class ProjectFeature < ActiveRecord::Base + # == Project features permissions + # + # Grants access level to project tools + # + # Tools can be enabled only for users, everyone or disabled + # Access control is made only for non private projects + # + # levels: + # + # Disabled: not enabled for anyone + # Private: enabled only for team members + # Enabled: enabled for everyone able to access the project + # + + # Permision levels + DISABLED = 0 + PRIVATE = 10 + ENABLED = 20 + + FEATURES = %i(issues merge_requests wiki snippets builds) + + belongs_to :project + + def feature_available?(feature, user) + raise ArgumentError, 'invalid project feature' unless FEATURES.include?(feature) + + get_permission(user, public_send("#{feature}_access_level")) + end + + def builds_enabled? + return true unless builds_access_level + + builds_access_level > DISABLED + end + + def wiki_enabled? + return true unless wiki_access_level + + wiki_access_level > DISABLED + end + + def merge_requests_enabled? + return true unless merge_requests_access_level + + merge_requests_access_level > DISABLED + end + + private + + def get_permission(user, level) + case level + when DISABLED + false + when PRIVATE + user && (project.team.member?(user) || user.admin?) + when ENABLED + true + else + true + end + end +end diff --git a/app/models/user.rb b/app/models/user.rb index 8f5958333d7..6996740eebd 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -433,7 +433,7 @@ class User < ActiveRecord::Base # # This logic is duplicated from `Ability#project_abilities` into a SQL form. def projects_where_can_admin_issues - authorized_projects(Gitlab::Access::REPORTER).non_archived.where.not(issues_enabled: false) + authorized_projects(Gitlab::Access::REPORTER).non_archived.with_issues_enabled end def is_admin? -- cgit v1.2.1 From 52fe6098861bf36601be6555d2b39f366795ddd3 Mon Sep 17 00:00:00 2001 From: Tomasz Maczukin Date: Thu, 1 Sep 2016 22:17:05 +0200 Subject: Refactor Ci::Build#raw_trace --- app/models/ci/build.rb | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) (limited to 'app/models') diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb index f219cee4a62..61052437318 100644 --- a/app/models/ci/build.rb +++ b/app/models/ci/build.rb @@ -217,11 +217,8 @@ module Ci end def raw_trace - if File.exist?(path_to_trace) - File.read(path_to_trace) - elsif has_old_trace_file? - # Temporary fix for build trace data integrity - File.read(old_path_to_trace) + if File.exist?(trace_file_path) + File.read(trace_file_path) else # backward compatibility read_attribute :trace -- cgit v1.2.1 From a93a610bac7d9ee7c0908592b6a5d91ef0d94333 Mon Sep 17 00:00:00 2001 From: Jacob Vosmaer Date: Tue, 30 Aug 2016 16:06:40 +0200 Subject: Use 'git update-ref' for safer web commits --- app/models/repository.rb | 41 +++++++++++++++++++++-------------------- 1 file changed, 21 insertions(+), 20 deletions(-) (limited to 'app/models') diff --git a/app/models/repository.rb b/app/models/repository.rb index f891e8374d2..b0644259af8 100644 --- a/app/models/repository.rb +++ b/app/models/repository.rb @@ -149,7 +149,7 @@ class Repository return false unless target GitHooksService.new.execute(user, path_to_repo, oldrev, target, ref) do - rugged.branches.create(branch_name, target) + update_ref!(ref, target, oldrev) end after_create_branch @@ -181,7 +181,7 @@ class Repository ref = Gitlab::Git::BRANCH_REF_PREFIX + branch_name GitHooksService.new.execute(user, path_to_repo, oldrev, newrev, ref) do - rugged.branches.delete(branch_name) + update_ref!(ref, newrev, oldrev) end after_remove_branch @@ -215,6 +215,21 @@ class Repository rugged.references.exist?(ref) end + def update_ref!(name, newrev, oldrev) + # We use 'git update-ref' because libgit2/rugged currently does not + # offer 'compare and swap' ref updates. Without compare-and-swap we can + # (and have!) accidentally reset the ref to an earlier state, clobbering + # commits. See also https://github.com/libgit2/libgit2/issues/1534. + command = %w[git update-ref --stdin -z] + output, status = Gitlab::Popen.popen(command, path_to_repo) do |stdin| + stdin.write("update #{name}\x00#{newrev}\x00#{oldrev}\x00") + end + + return if status.zero? + + raise CommitError.new("error updating ref #{name} #{oldrev}->#{newrev}\n#{output}") + end + # Makes sure a commit is kept around when Git garbage collection runs. # Git GC will delete commits from the repository that are no longer in any # branches or tags, but we want to keep some of these commits around, for @@ -1014,15 +1029,10 @@ class Repository def commit_with_hooks(current_user, branch) update_autocrlf_option - oldrev = Gitlab::Git::BLANK_SHA ref = Gitlab::Git::BRANCH_REF_PREFIX + branch target_branch = find_branch(branch) was_empty = empty? - if !was_empty && target_branch - oldrev = target_branch.target.id - end - # Make commit newrev = yield(ref) @@ -1030,24 +1040,15 @@ class Repository raise CommitError.new('Failed to create commit') end + oldrev = rugged.lookup(newrev).parent_ids.first || Gitlab::Git::BLANK_SHA + GitHooksService.new.execute(current_user, path_to_repo, oldrev, newrev, ref) do + update_ref!(ref, newrev, oldrev) + if was_empty || !target_branch - # Create branch - rugged.references.create(ref, newrev) - # If repo was empty expire cache after_create if was_empty after_create_branch - else - # Update head - current_head = find_branch(branch).target.id - - # Make sure target branch was not changed during pre-receive hook - if current_head == oldrev - rugged.references.update(ref, newrev) - else - raise CommitError.new('Commit was rejected because branch received new push') - end end end -- cgit v1.2.1 From ffef94f17e16a9ca9f0c1d1b970f2354d5144357 Mon Sep 17 00:00:00 2001 From: Jacob Vosmaer Date: Fri, 2 Sep 2016 17:54:09 +0200 Subject: Make error message appropriate for end users --- app/models/repository.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'app/models') diff --git a/app/models/repository.rb b/app/models/repository.rb index b0644259af8..414b82516bc 100644 --- a/app/models/repository.rb +++ b/app/models/repository.rb @@ -221,13 +221,13 @@ class Repository # (and have!) accidentally reset the ref to an earlier state, clobbering # commits. See also https://github.com/libgit2/libgit2/issues/1534. command = %w[git update-ref --stdin -z] - output, status = Gitlab::Popen.popen(command, path_to_repo) do |stdin| + _, status = Gitlab::Popen.popen(command, path_to_repo) do |stdin| stdin.write("update #{name}\x00#{newrev}\x00#{oldrev}\x00") end return if status.zero? - raise CommitError.new("error updating ref #{name} #{oldrev}->#{newrev}\n#{output}") + raise CommitError.new("Could not update branch #{name.sub('refs/heads/', '')}. Please refresh and try again.") end # Makes sure a commit is kept around when Git garbage collection runs. -- cgit v1.2.1