From 2acb8a3085ad64ce56625a9d63f01f9398e757a3 Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Mon, 25 Jul 2016 14:09:23 +0300 Subject: Change merge request diff index to be not uniq Signed-off-by: Dmitriy Zaporozhets --- ...20160725104020_merge_request_diff_remove_uniq.rb | 21 +++++++++++++++++++++ .../20160725104452_merge_request_diff_add_index.rb | 17 +++++++++++++++++ db/schema.rb | 14 +++++++------- 3 files changed, 45 insertions(+), 7 deletions(-) create mode 100644 db/migrate/20160725104020_merge_request_diff_remove_uniq.rb create mode 100644 db/migrate/20160725104452_merge_request_diff_add_index.rb diff --git a/db/migrate/20160725104020_merge_request_diff_remove_uniq.rb b/db/migrate/20160725104020_merge_request_diff_remove_uniq.rb new file mode 100644 index 00000000000..c8cbd2718ff --- /dev/null +++ b/db/migrate/20160725104020_merge_request_diff_remove_uniq.rb @@ -0,0 +1,21 @@ +# See http://doc.gitlab.com/ce/development/migration_style_guide.html +# for more information on how to write migrations for GitLab. + +class MergeRequestDiffRemoveUniq < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + disable_ddl_transaction! + + DOWNTIME = false + + def up + if index_exists?(:merge_request_diffs, :merge_request_id) + remove_index :merge_request_diffs, :merge_request_id + end + end + + def down + unless index_exists?(:merge_request_diffs, :merge_request_id) + add_concurrent_index :merge_request_diffs, :merge_request_id, unique: true + end + end +end diff --git a/db/migrate/20160725104452_merge_request_diff_add_index.rb b/db/migrate/20160725104452_merge_request_diff_add_index.rb new file mode 100644 index 00000000000..6d04242dd25 --- /dev/null +++ b/db/migrate/20160725104452_merge_request_diff_add_index.rb @@ -0,0 +1,17 @@ +class MergeRequestDiffAddIndex < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + disable_ddl_transaction! + + # Set this constant to true if this migration requires downtime. + DOWNTIME = false + + def up + add_concurrent_index :merge_request_diffs, :merge_request_id + end + + def down + if index_exists?(:merge_request_diffs, :merge_request_id) + remove_index :merge_request_diffs, :merge_request_id + end + end +end diff --git a/db/schema.rb b/db/schema.rb index 5b35a528e71..e3eecfebfd4 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -11,7 +11,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 20160726093600) do +ActiveRecord::Schema.define(version: 20160725104452) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -84,8 +84,8 @@ ActiveRecord::Schema.define(version: 20160726093600) do t.string "health_check_access_token" t.boolean "send_user_confirmation_email", default: false t.integer "container_registry_token_expire_delay", default: 5 - t.boolean "user_default_external", default: false, null: false t.text "after_sign_up_text" + t.boolean "user_default_external", default: false, null: false t.string "repository_storage", default: "default" t.string "enabled_git_access_protocol" t.boolean "domain_blacklist_enabled", default: false @@ -167,8 +167,8 @@ ActiveRecord::Schema.define(version: 20160726093600) do t.text "artifacts_metadata" t.integer "erased_by_id" t.datetime "erased_at" - t.string "environment" t.datetime "artifacts_expire_at" + t.string "environment" t.integer "artifacts_size" t.string "when" t.text "yaml_variables" @@ -605,7 +605,7 @@ ActiveRecord::Schema.define(version: 20160726093600) do t.string "start_commit_sha" end - add_index "merge_request_diffs", ["merge_request_id"], name: "index_merge_request_diffs_on_merge_request_id", unique: true, using: :btree + add_index "merge_request_diffs", ["merge_request_id"], name: "index_merge_request_diffs_on_merge_request_id", using: :btree create_table "merge_requests", force: :cascade do |t| t.string "target_branch", null: false @@ -782,10 +782,10 @@ ActiveRecord::Schema.define(version: 20160726093600) do t.integer "user_id", null: false t.string "token", null: false t.string "name", null: false - t.datetime "created_at", null: false - t.datetime "updated_at", null: false t.boolean "revoked", default: false t.datetime "expires_at" + t.datetime "created_at", null: false + t.datetime "updated_at", null: false end add_index "personal_access_tokens", ["token"], name: "index_personal_access_tokens_on_token", unique: true, using: :btree @@ -847,8 +847,8 @@ ActiveRecord::Schema.define(version: 20160726093600) do t.boolean "only_allow_merge_if_build_succeeds", default: false, null: false t.boolean "has_external_issue_tracker" t.string "repository_storage", default: "default", null: false - t.boolean "has_external_wiki" t.boolean "request_access_enabled", default: true, null: false + t.boolean "has_external_wiki" end add_index "projects", ["builds_enabled", "shared_runners_enabled"], name: "index_projects_on_builds_enabled_and_shared_runners_enabled", using: :btree -- 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(-) 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(+) 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 +----- spec/models/merge_request_spec.rb | 28 ++++++++++++++++------------ 2 files changed, 17 insertions(+), 17 deletions(-) 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( diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb index a0e3c26e542..03e4ae574ef 100644 --- a/spec/models/merge_request_spec.rb +++ b/spec/models/merge_request_spec.rb @@ -9,7 +9,7 @@ describe MergeRequest, models: true do it { is_expected.to belong_to(:target_project).with_foreign_key(:target_project_id).class_name('Project') } it { is_expected.to belong_to(:source_project).with_foreign_key(:source_project_id).class_name('Project') } it { is_expected.to belong_to(:merge_user).class_name("User") } - it { is_expected.to have_one(:merge_request_diff).dependent(:destroy) } + it { is_expected.to have_many(:merge_request_diffs).dependent(:destroy) } end describe 'modules' do @@ -134,7 +134,7 @@ describe MergeRequest, models: true do context 'when there are MR diffs' do it 'delegates to the MR diffs' do - merge_request.merge_request_diff = MergeRequestDiff.new + merge_request.merge_request_diffs.build expect(merge_request.merge_request_diff).to receive(:diffs).with(options) @@ -654,22 +654,26 @@ describe MergeRequest, models: true do let(:commit) { subject.project.commit(sample_commit.id) } - it "reloads the diff content" do - expect(subject.merge_request_diff).to receive(:reload_content) - + it "does not change existing merge request diff" do + expect(subject.merge_request_diff).not_to receive(:reload_content) subject.reload_diff end + it "creates new merge request diff" do + expect { subject.reload_diff }.to change { subject.merge_request_diffs.count }.by(1) + end + it "updates diff note positions" do old_diff_refs = subject.diff_refs - merge_request_diff = subject.merge_request_diff - # Update merge_request_diff so that #diff_refs will return commit.diff_refs - allow(merge_request_diff).to receive(:reload_content) do - merge_request_diff.base_commit_sha = commit.parent_id - merge_request_diff.start_commit_sha = commit.parent_id - merge_request_diff.head_commit_sha = commit.sha + allow(subject).to receive(:create_merge_request_diff) do + subject.merge_request_diffs.create( + importing: true, + base_commit_sha: commit.parent_id, + start_commit_sha: commit.parent_id, + head_commit_sha: commit.sha + ) end expect(Notes::DiffPositionUpdateService).to receive(:new).with( @@ -679,8 +683,8 @@ describe MergeRequest, models: true do new_diff_refs: commit.diff_refs, paths: note.position.paths ).and_call_original - expect_any_instance_of(Notes::DiffPositionUpdateService).to receive(:execute).with(note) + expect_any_instance_of(Notes::DiffPositionUpdateService).to receive(:execute).with(note) expect_any_instance_of(DiffNote).to receive(:save).once subject.reload_diff -- 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/controllers/projects/merge_requests_controller.rb | 8 +++++++- app/models/merge_request_diff.rb | 10 ++++++++++ .../projects/merge_requests/show/_diffs.html.haml | 5 +++-- .../projects/merge_requests/show/_versions.html.haml | 18 ++++++++++++++++++ 4 files changed, 38 insertions(+), 3 deletions(-) create mode 100644 app/views/projects/merge_requests/show/_versions.html.haml diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb index 03166294ddd..a886af713d5 100644 --- a/app/controllers/projects/merge_requests_controller.rb +++ b/app/controllers/projects/merge_requests_controller.rb @@ -81,7 +81,13 @@ class Projects::MergeRequestsController < Projects::ApplicationController def diffs apply_diff_view_cookie! - @merge_request_diff = @merge_request.merge_request_diff + @merge_request_diff = + if params[:diff_id] + @merge_request.merge_request_diffs.find(params[:diff_id]) + else + @merge_request.merge_request_diff + end + respond_to do |format| format.html { define_discussion_vars } 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 diff --git a/app/views/projects/merge_requests/show/_diffs.html.haml b/app/views/projects/merge_requests/show/_diffs.html.haml index 1b0bae86ad4..e26499cc144 100644 --- a/app/views/projects/merge_requests/show/_diffs.html.haml +++ b/app/views/projects/merge_requests/show/_diffs.html.haml @@ -1,6 +1,7 @@ - if @merge_request_diff.collected? - = render "projects/diffs/diffs", diffs: @merge_request.diffs(diff_options), - project: @merge_request.project, diff_refs: @merge_request.diff_refs + = render 'projects/merge_requests/show/versions' + = render "projects/diffs/diffs", diffs: @merge_request_diff.diffs(diff_options), + project: @merge_request.project, diff_refs: @merge_request_diff.diff_refs - elsif @merge_request_diff.empty? .nothing-here-block Nothing to merge from #{@merge_request.source_branch} into #{@merge_request.target_branch} - else diff --git a/app/views/projects/merge_requests/show/_versions.html.haml b/app/views/projects/merge_requests/show/_versions.html.haml new file mode 100644 index 00000000000..2764786d9bf --- /dev/null +++ b/app/views/projects/merge_requests/show/_versions.html.haml @@ -0,0 +1,18 @@ +- if @merge_request.merge_request_diffs.size > 1 + - latest_diff = @merge_request.merge_request_diff + .light-well + Merge Request version + + %span.dropdown.inline + %a.dropdown-toggle{ data: {toggle: :dropdown} } + %strong.monospace #{@merge_request_diff.base_commit.short_id}..#{@merge_request_diff.head_commit.short_id} + %span.caret + %ul.dropdown-menu + - @merge_request.merge_request_diffs.each do |merge_request_diff| + %li{ class: ('active' if merge_request_diff == latest_diff) } + = link_to diffs_namespace_project_merge_request_path(@project.namespace, @project, @merge_request, diff_id: merge_request_diff.id) do + %strong.monospace + #{merge_request_diff.base_commit.short_id}..#{merge_request_diff.head_commit.short_id} + %br + %small + = time_ago_with_tooltip(merge_request_diff.created_at) -- cgit v1.2.1 From dbd58dc4bb638be33f98336410ae6f4ede15f737 Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Tue, 26 Jul 2016 20:03:45 +0300 Subject: Improve merge request version switcher Signed-off-by: Dmitriy Zaporozhets --- app/assets/stylesheets/framework/dropdowns.scss | 6 ++++++ app/assets/stylesheets/pages/merge_requests.scss | 6 ++++++ .../merge_requests/show/_versions.html.haml | 24 ++++++++++++++-------- 3 files changed, 27 insertions(+), 9 deletions(-) diff --git a/app/assets/stylesheets/framework/dropdowns.scss b/app/assets/stylesheets/framework/dropdowns.scss index c54eb0d6479..28346423541 100644 --- a/app/assets/stylesheets/framework/dropdowns.scss +++ b/app/assets/stylesheets/framework/dropdowns.scss @@ -17,6 +17,12 @@ .dropdown { position: relative; + + .btn-link { + &:hover { + cursor: pointer; + } + } } .open { diff --git a/app/assets/stylesheets/pages/merge_requests.scss b/app/assets/stylesheets/pages/merge_requests.scss index 0a661e529f0..14ded0eec72 100644 --- a/app/assets/stylesheets/pages/merge_requests.scss +++ b/app/assets/stylesheets/pages/merge_requests.scss @@ -371,3 +371,9 @@ } } } + +.mr-version-switch { + background: $background-color; + padding: $gl-btn-padding; + border-bottom: 1px solid $border-color; +} diff --git a/app/views/projects/merge_requests/show/_versions.html.haml b/app/views/projects/merge_requests/show/_versions.html.haml index 2764786d9bf..5c0098bcf98 100644 --- a/app/views/projects/merge_requests/show/_versions.html.haml +++ b/app/views/projects/merge_requests/show/_versions.html.haml @@ -1,18 +1,24 @@ -- if @merge_request.merge_request_diffs.size > 1 - - latest_diff = @merge_request.merge_request_diff - .light-well - Merge Request version +- diffs_count = @merge_request.merge_request_diffs.count +- if diffs_count > 1 + - latest_diff = @merge_request.merge_request_diff + .mr-version-switch + Version:  %span.dropdown.inline - %a.dropdown-toggle{ data: {toggle: :dropdown} } - %strong.monospace #{@merge_request_diff.base_commit.short_id}..#{@merge_request_diff.head_commit.short_id} + %a.btn-link.dropdown-toggle{ data: {toggle: :dropdown} } + %strong.monospace< + - if latest_diff == @merge_request_diff + #{"latest"} + - else + #{@merge_request_diff.base_commit.short_id}..#{@merge_request_diff.head_commit.short_id} %span.caret - %ul.dropdown-menu + %ul.dropdown-menu.dropdown-menu-selectable - @merge_request.merge_request_diffs.each do |merge_request_diff| - %li{ class: ('active' if merge_request_diff == latest_diff) } - = link_to diffs_namespace_project_merge_request_path(@project.namespace, @project, @merge_request, diff_id: merge_request_diff.id) do + %li + = link_to diffs_namespace_project_merge_request_path(@project.namespace, @project, @merge_request, diff_id: merge_request_diff.id), class: ('is-active' if merge_request_diff == @merge_request_diff) do %strong.monospace #{merge_request_diff.base_commit.short_id}..#{merge_request_diff.head_commit.short_id} %br %small + = time_ago_with_tooltip(merge_request_diff.created_at) -- cgit v1.2.1 From 6190216541adb04fd0ce4b21862e6b253b868671 Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Wed, 27 Jul 2016 11:27:06 +0300 Subject: Add small visual improvements to merge request version switch Signed-off-by: Dmitriy Zaporozhets --- app/assets/stylesheets/pages/merge_requests.scss | 6 +++++- app/views/projects/merge_requests/show/_versions.html.haml | 9 ++++++--- 2 files changed, 11 insertions(+), 4 deletions(-) diff --git a/app/assets/stylesheets/pages/merge_requests.scss b/app/assets/stylesheets/pages/merge_requests.scss index 14ded0eec72..7d8f58ca5af 100644 --- a/app/assets/stylesheets/pages/merge_requests.scss +++ b/app/assets/stylesheets/pages/merge_requests.scss @@ -375,5 +375,9 @@ .mr-version-switch { background: $background-color; padding: $gl-btn-padding; - border-bottom: 1px solid $border-color; + color: $gl-placeholder-color; + + a.btn-link { + color: $gl-dark-link-color; + } } diff --git a/app/views/projects/merge_requests/show/_versions.html.haml b/app/views/projects/merge_requests/show/_versions.html.haml index 5c0098bcf98..8b7d866941c 100644 --- a/app/views/projects/merge_requests/show/_versions.html.haml +++ b/app/views/projects/merge_requests/show/_versions.html.haml @@ -10,15 +10,18 @@ - if latest_diff == @merge_request_diff #{"latest"} - else - #{@merge_request_diff.base_commit.short_id}..#{@merge_request_diff.head_commit.short_id} + #{@merge_request_diff.head_commit.short_id} %span.caret %ul.dropdown-menu.dropdown-menu-selectable - @merge_request.merge_request_diffs.each do |merge_request_diff| %li = link_to diffs_namespace_project_merge_request_path(@project.namespace, @project, @merge_request, diff_id: merge_request_diff.id), class: ('is-active' if merge_request_diff == @merge_request_diff) do %strong.monospace - #{merge_request_diff.base_commit.short_id}..#{merge_request_diff.head_commit.short_id} + #{merge_request_diff.head_commit.short_id} %br %small - = time_ago_with_tooltip(merge_request_diff.created_at) + + .pull-right + %span.monospace + git diff #{@merge_request_diff.base_commit.short_id}..#{@merge_request_diff.head_commit.short_id} -- 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(-) 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 5d5f2cf3ef7bc238f75d9a73ce94efceec6f690e Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Wed, 27 Jul 2016 17:55:12 +0300 Subject: Add feature test for merge request version Signed-off-by: Dmitriy Zaporozhets --- .../merge_requests/merge_request_versions_spec.rb | 38 ++++++++++++++++++++++ 1 file changed, 38 insertions(+) create mode 100644 spec/features/merge_requests/merge_request_versions_spec.rb diff --git a/spec/features/merge_requests/merge_request_versions_spec.rb b/spec/features/merge_requests/merge_request_versions_spec.rb new file mode 100644 index 00000000000..862ead4e72e --- /dev/null +++ b/spec/features/merge_requests/merge_request_versions_spec.rb @@ -0,0 +1,38 @@ +require 'spec_helper' + +feature 'Merge Request versions', js: true, feature: true do + before do + login_as :admin + merge_request = create(:merge_request, importing: true) + merge_request.merge_request_diffs.create(head_commit_sha: '6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9') + merge_request.merge_request_diffs.create(head_commit_sha: '5937ac0a7beb003549fc5fd26fc247adbce4a52e') + project = merge_request.source_project + visit diffs_namespace_project_merge_request_path(project.namespace, project, merge_request) + end + + it 'show the latest version of the diff' do + page.within '.mr-version-switch' do + expect(page).to have_content 'Version: latest' + end + + expect(page).to have_content '8 changed files' + end + + describe 'switch between versions' do + before do + page.within '.mr-version-switch' do + find('.btn-link').click + #find('a', text: '6f6d7e7e').click + click_link '6f6d7e7e' + end + end + + it 'should show older version' do + page.within '.mr-version-switch' do + expect(page).to have_content 'Version: 6f6d7e7e' + end + + expect(page).to have_content '5 changed files' + end + 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(-) 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(-) 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 +++- app/views/projects/merge_requests/show/_versions.html.haml | 3 ++- 2 files changed, 5 insertions(+), 2 deletions(-) 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 diff --git a/app/views/projects/merge_requests/show/_versions.html.haml b/app/views/projects/merge_requests/show/_versions.html.haml index 8b7d866941c..93a06154022 100644 --- a/app/views/projects/merge_requests/show/_versions.html.haml +++ b/app/views/projects/merge_requests/show/_versions.html.haml @@ -20,8 +20,9 @@ #{merge_request_diff.head_commit.short_id} %br %small + #{number_with_delimiter(merge_request_diff.commits.count)} #{'commit'.pluralize(merge_request_diff.commits.count)}, = time_ago_with_tooltip(merge_request_diff.created_at) .pull-right %span.monospace - git diff #{@merge_request_diff.base_commit.short_id}..#{@merge_request_diff.head_commit.short_id} + #{@merge_request_diff.base_commit.short_id}..#{@merge_request_diff.head_commit.short_id} -- 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(-) 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 ++------- spec/models/merge_request_spec.rb | 5 ++--- 2 files changed, 4 insertions(+), 10 deletions(-) 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( diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb index 03e4ae574ef..f095e23f7ac 100644 --- a/spec/models/merge_request_spec.rb +++ b/spec/models/merge_request_spec.rb @@ -291,7 +291,7 @@ describe MergeRequest, models: true do end it "can be removed if the last commit is the head of the source branch" do - allow(subject.source_project).to receive(:commit).and_return(subject.diff_head_commit) + allow(subject).to receive(:source_branch_head).and_return(subject.diff_head_commit) expect(subject.can_remove_source_branch?(user)).to be_truthy end @@ -655,7 +655,7 @@ describe MergeRequest, models: true do let(:commit) { subject.project.commit(sample_commit.id) } it "does not change existing merge request diff" do - expect(subject.merge_request_diff).not_to receive(:reload_content) + expect(subject.merge_request_diff).not_to receive(:save_git_content) subject.reload_diff end @@ -669,7 +669,6 @@ describe MergeRequest, models: true do # Update merge_request_diff so that #diff_refs will return commit.diff_refs allow(subject).to receive(:create_merge_request_diff) do subject.merge_request_diffs.create( - importing: true, base_commit_sha: commit.parent_id, start_commit_sha: commit.parent_id, head_commit_sha: commit.sha -- 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 ++++++++++------------ .../merge_requests/merge_request_versions_spec.rb | 1 - 2 files changed, 12 insertions(+), 17 deletions(-) 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 diff --git a/spec/features/merge_requests/merge_request_versions_spec.rb b/spec/features/merge_requests/merge_request_versions_spec.rb index 862ead4e72e..577c910f11b 100644 --- a/spec/features/merge_requests/merge_request_versions_spec.rb +++ b/spec/features/merge_requests/merge_request_versions_spec.rb @@ -22,7 +22,6 @@ feature 'Merge Request versions', js: true, feature: true do before do page.within '.mr-version-switch' do find('.btn-link').click - #find('a', text: '6f6d7e7e').click click_link '6f6d7e7e' end end -- 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 +--- spec/models/merge_request_diff_spec.rb | 17 +++++++++++++++++ 3 files changed, 21 insertions(+), 4 deletions(-) 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 diff --git a/spec/models/merge_request_diff_spec.rb b/spec/models/merge_request_diff_spec.rb index 9a637c94fbe..16bba82181b 100644 --- a/spec/models/merge_request_diff_spec.rb +++ b/spec/models/merge_request_diff_spec.rb @@ -1,6 +1,23 @@ require 'spec_helper' describe MergeRequestDiff, models: true do + describe 'initialize new object' do + subject { build(:merge_request).merge_request_diffs.build } + + it { expect(subject).to be_valid } + it { expect(subject.head_commit_sha).to eq('5937ac0a7beb003549fc5fd26fc247adbce4a52e') } + it { expect(subject.base_commit_sha).to eq('ae73cb07c9eeaf35924a10f713b364d32b2dd34f') } + it { expect(subject.start_commit_sha).to eq('0b4bc9a49b562e85de7cc9e834518ea6828729b9') } + end + + describe 'create new record' do + subject { create(:merge_request) } + + it { expect(subject).to be_valid } + it { expect(subject.commits.count).to eq(5) } + it { expect(subject.diffs.count).to eq(8) } + end + describe '#diffs' do let(:mr) { create(:merge_request, :with_diffs) } let(:mr_diff) { mr.merge_request_diff } -- 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/controllers/projects/merge_requests_controller.rb | 1 - app/models/merge_request.rb | 5 ----- app/services/merge_requests/create_service.rb | 1 + spec/factories/merge_requests.rb | 6 ++++++ spec/models/merge_request_diff_spec.rb | 3 ++- 5 files changed, 9 insertions(+), 7 deletions(-) diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb index a886af713d5..33188e88385 100644 --- a/app/controllers/projects/merge_requests_controller.rb +++ b/app/controllers/projects/merge_requests_controller.rb @@ -88,7 +88,6 @@ class Projects::MergeRequestsController < Projects::ApplicationController @merge_request.merge_request_diff end - respond_to do |format| format.html { define_discussion_vars } format.json { render json: { html: view_to_html_string("projects/merge_requests/show/_diffs") } } 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 diff --git a/app/services/merge_requests/create_service.rb b/app/services/merge_requests/create_service.rb index 96a25330af1..553d7443c86 100644 --- a/app/services/merge_requests/create_service.rb +++ b/app/services/merge_requests/create_service.rb @@ -16,6 +16,7 @@ module MergeRequests merge_request.target_project ||= source_project merge_request.author = current_user merge_request.merge_params['force_remove_source_branch'] = force_remove_source_branch + merge_request.merge_request_diffs.build if merge_request.save merge_request.update_attributes(label_ids: label_params) diff --git a/spec/factories/merge_requests.rb b/spec/factories/merge_requests.rb index c6a08d78b78..05851c49497 100644 --- a/spec/factories/merge_requests.rb +++ b/spec/factories/merge_requests.rb @@ -68,5 +68,11 @@ FactoryGirl.define do factory :closed_merge_request, traits: [:closed] factory :reopened_merge_request, traits: [:reopened] factory :merge_request_with_diffs, traits: [:with_diffs] + + after :create do |merge_request| + unless merge_request.merge_request_diff + merge_request.create_merge_request_diff + end + end end end diff --git a/spec/models/merge_request_diff_spec.rb b/spec/models/merge_request_diff_spec.rb index 16bba82181b..0a55515e8c6 100644 --- a/spec/models/merge_request_diff_spec.rb +++ b/spec/models/merge_request_diff_spec.rb @@ -11,9 +11,10 @@ describe MergeRequestDiff, models: true do end describe 'create new record' do - subject { create(:merge_request) } + subject { create(:merge_request).merge_request_diff } it { expect(subject).to be_valid } + it { expect(subject).to be_persisted } it { expect(subject.commits.count).to eq(5) } it { expect(subject.diffs.count).to eq(8) } 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 +++++++++++++++------------ app/services/merge_requests/create_service.rb | 1 - features/project/merge_requests.feature | 2 +- features/steps/project/merge_requests.rb | 6 +-- spec/factories/merge_requests.rb | 6 --- spec/models/merge_request_diff_spec.rb | 12 ++---- 7 files changed, 44 insertions(+), 52 deletions(-) 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 diff --git a/app/services/merge_requests/create_service.rb b/app/services/merge_requests/create_service.rb index 553d7443c86..96a25330af1 100644 --- a/app/services/merge_requests/create_service.rb +++ b/app/services/merge_requests/create_service.rb @@ -16,7 +16,6 @@ module MergeRequests merge_request.target_project ||= source_project merge_request.author = current_user merge_request.merge_params['force_remove_source_branch'] = force_remove_source_branch - merge_request.merge_request_diffs.build if merge_request.save merge_request.update_attributes(label_ids: label_params) diff --git a/features/project/merge_requests.feature b/features/project/merge_requests.feature index 21768c15c17..7673b4284ef 100644 --- a/features/project/merge_requests.feature +++ b/features/project/merge_requests.feature @@ -24,7 +24,7 @@ Feature: Project Merge Requests Scenario: I should see target branch when it is different from default Given project "Shop" have "Bug NS-06" open merge request When I visit project "Shop" merge requests page - Then I should see "other_branch" branch + Then I should see "feature_conflict" branch Scenario: I should not see the numbers of diverged commits if the branch is rebased on the target Given project "Shop" have "Bug NS-07" open merge request with rebased branch diff --git a/features/steps/project/merge_requests.rb b/features/steps/project/merge_requests.rb index da848afd48e..74e162aadee 100644 --- a/features/steps/project/merge_requests.rb +++ b/features/steps/project/merge_requests.rb @@ -56,8 +56,8 @@ class Spinach::Features::ProjectMergeRequests < Spinach::FeatureSteps expect(find('.merge-request-info')).not_to have_content "master" end - step 'I should see "other_branch" branch' do - expect(page).to have_content "other_branch" + step 'I should see "feature_conflict" branch' do + expect(page).to have_content "feature_conflict" end step 'I should see "Bug NS-04" in merge requests' do @@ -122,7 +122,7 @@ class Spinach::Features::ProjectMergeRequests < Spinach::FeatureSteps source_project: project, target_project: project, source_branch: 'fix', - target_branch: 'other_branch', + target_branch: 'feature_conflict', author: project.users.first, description: "# Description header" ) diff --git a/spec/factories/merge_requests.rb b/spec/factories/merge_requests.rb index 05851c49497..c6a08d78b78 100644 --- a/spec/factories/merge_requests.rb +++ b/spec/factories/merge_requests.rb @@ -68,11 +68,5 @@ FactoryGirl.define do factory :closed_merge_request, traits: [:closed] factory :reopened_merge_request, traits: [:reopened] factory :merge_request_with_diffs, traits: [:with_diffs] - - after :create do |merge_request| - unless merge_request.merge_request_diff - merge_request.create_merge_request_diff - end - end end end diff --git a/spec/models/merge_request_diff_spec.rb b/spec/models/merge_request_diff_spec.rb index 0a55515e8c6..f2954a39b08 100644 --- a/spec/models/merge_request_diff_spec.rb +++ b/spec/models/merge_request_diff_spec.rb @@ -1,15 +1,6 @@ require 'spec_helper' describe MergeRequestDiff, models: true do - describe 'initialize new object' do - subject { build(:merge_request).merge_request_diffs.build } - - it { expect(subject).to be_valid } - it { expect(subject.head_commit_sha).to eq('5937ac0a7beb003549fc5fd26fc247adbce4a52e') } - it { expect(subject.base_commit_sha).to eq('ae73cb07c9eeaf35924a10f713b364d32b2dd34f') } - it { expect(subject.start_commit_sha).to eq('0b4bc9a49b562e85de7cc9e834518ea6828729b9') } - end - describe 'create new record' do subject { create(:merge_request).merge_request_diff } @@ -17,6 +8,9 @@ describe MergeRequestDiff, models: true do it { expect(subject).to be_persisted } it { expect(subject.commits.count).to eq(5) } it { expect(subject.diffs.count).to eq(8) } + it { expect(subject.head_commit_sha).to eq('5937ac0a7beb003549fc5fd26fc247adbce4a52e') } + it { expect(subject.base_commit_sha).to eq('ae73cb07c9eeaf35924a10f713b364d32b2dd34f') } + it { expect(subject.start_commit_sha).to eq('0b4bc9a49b562e85de7cc9e834518ea6828729b9') } end describe '#diffs' do -- 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(-) 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(+) 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 159998a460be6ca1ea5efd135a5c3aefeab2d9b6 Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Tue, 2 Aug 2016 18:07:50 +0300 Subject: Fix merge request spec for update diff note Signed-off-by: Dmitriy Zaporozhets --- spec/models/merge_request_spec.rb | 2 ++ 1 file changed, 2 insertions(+) diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb index f095e23f7ac..eb382f9ef84 100644 --- a/spec/models/merge_request_spec.rb +++ b/spec/models/merge_request_spec.rb @@ -673,6 +673,8 @@ describe MergeRequest, models: true do start_commit_sha: commit.parent_id, head_commit_sha: commit.sha ) + + subject.merge_request_diff(true) end expect(Notes::DiffPositionUpdateService).to receive(:new).with( -- cgit v1.2.1 From 9a5f40878c6d71e7fd4858f7fa1d948af6c371ae Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Wed, 3 Aug 2016 11:32:29 +0300 Subject: Add API to list merge request diff versions Signed-off-by: Dmitriy Zaporozhets --- doc/api/merge_requests.md | 90 +++++++++++++++++++++++++++ lib/api/api.rb | 1 + lib/api/entities.rb | 11 ++++ lib/api/merge_request_diffs.rb | 25 ++++++++ spec/requests/api/merge_request_diffs_spec.rb | 32 ++++++++++ 5 files changed, 159 insertions(+) create mode 100644 lib/api/merge_request_diffs.rb create mode 100644 spec/requests/api/merge_request_diffs_spec.rb diff --git a/doc/api/merge_requests.md b/doc/api/merge_requests.md index e00882e6d5d..cf262dc4a80 100644 --- a/doc/api/merge_requests.md +++ b/doc/api/merge_requests.md @@ -894,3 +894,93 @@ Example response: "created_at": "2016-07-01T11:14:15.530Z" } ``` + +## Get MR diff versions + +Get a list of merge request diff versions. + +``` +GET /projects/:id/merge_requests/:merge_request_id/versions +``` + +Parameters: + +- `id` (required) - The ID of a project +- `merge_request_id` (required) - The ID of MR + + +```json +[{ + "id": 110, + "head_commit_sha": "33e2ee8579fda5bc36accc9c6fbd0b4fefda9e30", + "base_commit_sha": "eeb57dffe83deb686a60a71c16c32f71046868fd", + "start_commit_sha": "eeb57dffe83deb686a60a71c16c32f71046868fd", + "created_at": "2016-07-26T14:44:48.926Z", + "merge_request_id": 105, + "state": "collected", + "real_size": "1", + "commits": [{ + "id": "33e2ee8579fda5bc36accc9c6fbd0b4fefda9e30", + "short_id": "33e2ee85", + "title": "Change year to 2018", + "author_name": "Administrator", + "author_email": "admin@example.com", + "created_at": "2016-07-26T17:44:29.000+03:00", + "message": "Change year to 2018" + }, { + "id": "aa24655de48b36335556ac8a3cd8bb521f977cbd", + "short_id": "aa24655d", + "title": "Update LICENSE", + "author_name": "Administrator", + "author_email": "admin@example.com", + "created_at": "2016-07-25T17:21:53.000+03:00", + "message": "Update LICENSE" + }, { + "id": "3eed087b29835c48015768f839d76e5ea8f07a24", + "short_id": "3eed087b", + "title": "Add license", + "author_name": "Administrator", + "author_email": "admin@example.com", + "created_at": "2016-07-25T17:21:20.000+03:00", + "message": "Add license" + }], + "diffs": [{ + "old_path": "LICENSE", + "new_path": "LICENSE", + "a_mode": "0", + "b_mode": "100644", + "diff": "--- /dev/null\n+++ b/LICENSE\n@@ -0,0 +1,21 @@\n+The MIT License (MIT)\n+\n+Copyright (c) 2018 Administrator\n+\n+Permission is hereby granted, free of charge, to any person obtaining a copy\n+of this software and associated documentation files (the \"Software\"), to deal\n+in the Software without restriction, including without limitation the rights\n+to use, copy, modify, merge, publish, distribute, sublicense, and/or sell\n+copies of the Software, and to permit persons to whom the Software is\n+furnished to do so, subject to the following conditions:\n+\n+The above copyright notice and this permission notice shall be included in all\n+copies or substantial portions of the Software.\n+\n+THE SOFTWARE IS PROVIDED \"AS IS\", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR\n+IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,\n+FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE\n+AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER\n+LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,\n+OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE\n+SOFTWARE.\n", + "new_file": true, + "renamed_file": false, + "deleted_file": false + }] +}, { + "id": 108, + "head_commit_sha": "3eed087b29835c48015768f839d76e5ea8f07a24", + "base_commit_sha": "eeb57dffe83deb686a60a71c16c32f71046868fd", + "start_commit_sha": "eeb57dffe83deb686a60a71c16c32f71046868fd", + "created_at": "2016-07-25T14:21:33.028Z", + "merge_request_id": 105, + "state": "collected", + "real_size": "1", + "commits": [{ + "id": "3eed087b29835c48015768f839d76e5ea8f07a24", + "short_id": "3eed087b", + "title": "Add license", + "author_name": "Administrator", + "author_email": "admin@example.com", + "created_at": "2016-07-25T17:21:20.000+03:00", + "message": "Add license" + }], + "diffs": [{ + "old_path": "LICENSE", + "new_path": "LICENSE", + "a_mode": "0", + "b_mode": "100644", + "diff": "--- /dev/null\n+++ b/LICENSE\n@@ -0,0 +1,21 @@\n+The MIT License (MIT)\n+\n+Copyright (c) 2016 Administrator\n+\n+Permission is hereby granted, free of charge, to any person obtaining a copy\n+of this software and associated documentation files (the \"Software\"), to deal\n+in the Software without restriction, including without limitation the rights\n+to use, copy, modify, merge, publish, distribute, sublicense, and/or sell\n+copies of the Software, and to permit persons to whom the Software is\n+furnished to do so, subject to the following conditions:\n+\n+The above copyright notice and this permission notice shall be included in all\n+copies or substantial portions of the Software.\n+\n+THE SOFTWARE IS PROVIDED \"AS IS\", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR\n+IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,\n+FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE\n+AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER\n+LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,\n+OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE\n+SOFTWARE.\n", + "new_file": true, + "renamed_file": false, + "deleted_file": false + }] +}] +``` diff --git a/lib/api/api.rb b/lib/api/api.rb index bd16806892b..9d8f297191f 100644 --- a/lib/api/api.rb +++ b/lib/api/api.rb @@ -67,5 +67,6 @@ module API mount ::API::Triggers mount ::API::Users mount ::API::Variables + mount ::API::MergeRequestDiffs end end diff --git a/lib/api/entities.rb b/lib/api/entities.rb index 3e21b7a0b8a..af069063f0c 100644 --- a/lib/api/entities.rb +++ b/lib/api/entities.rb @@ -228,6 +228,17 @@ module API end end + class MergeRequestDiff < Grape::Entity + expose :id, :head_commit_sha, :base_commit_sha, :start_commit_sha, + :created_at, :merge_request_id, :state, :real_size + + expose :commits, using: Entities::RepoCommit + + expose :diffs, using: Entities::RepoDiff do |compare, _| + compare.diffs(all_diffs: true).to_a + end + end + class SSHKey < Grape::Entity expose :id, :title, :key, :created_at end diff --git a/lib/api/merge_request_diffs.rb b/lib/api/merge_request_diffs.rb new file mode 100644 index 00000000000..4bd149d1603 --- /dev/null +++ b/lib/api/merge_request_diffs.rb @@ -0,0 +1,25 @@ +module API + # MergeRequestDiff API + class MergeRequestDiffs < Grape::API + before { authenticate! } + + resource :projects do + # List merge requests diff versions + # + # Parameters: + # id (required) - The ID of a project + # merge_request_id (required) - The ID of MR + # + # Example: + # GET /projects/:id/merge_requests/:merge_request_id/versions + # + get ":id/merge_requests/:merge_request_id/versions" do + merge_request = user_project.merge_requests. + find(params[:merge_request_id]) + + authorize! :read_merge_request, merge_request + present merge_request.merge_request_diffs, with: Entities::MergeRequestDiff + end + end + end +end diff --git a/spec/requests/api/merge_request_diffs_spec.rb b/spec/requests/api/merge_request_diffs_spec.rb new file mode 100644 index 00000000000..db076b4b9a5 --- /dev/null +++ b/spec/requests/api/merge_request_diffs_spec.rb @@ -0,0 +1,32 @@ +require "spec_helper" + +describe API::API, 'MergeRequestDiffs', api: true do + include ApiHelpers + + let!(:user) { create(:user) } + let!(:merge_request) { create(:merge_request, importing: true) } + let!(:project) { merge_request.target_project } + + before do + merge_request.merge_request_diffs.create(head_commit_sha: '6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9') + merge_request.merge_request_diffs.create(head_commit_sha: '5937ac0a7beb003549fc5fd26fc247adbce4a52e') + project.team << [user, :master] + end + + describe 'GET /projects/:id/merge_requests/:merge_request_id/versions' do + context 'valid merge request' do + before { get api("/projects/#{project.id}/merge_requests/#{merge_request.id}/versions", user) } + let(:merge_request_diff) { merge_request.merge_request_diffs.first } + + it { expect(response.status).to eq 200 } + it { expect(json_response.size).to eq(merge_request.merge_request_diffs.size) } + it { expect(json_response.first['id']).to eq(merge_request_diff.id) } + it { expect(json_response.first['head_commit_sha']).to eq(merge_request_diff.head_commit_sha) } + end + + it 'returns a 404 when merge_request_id not found' do + get api("/projects/#{project.id}/merge_requests/999/versions", user) + expect(response).to have_http_status(404) + end + end +end -- cgit v1.2.1 From 69096ad9da77ce51d544a8f23f4e8873df273f4e Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Wed, 3 Aug 2016 11:58:09 +0300 Subject: Update merge request versions API to match styleguide Signed-off-by: Dmitriy Zaporozhets --- doc/api/merge_requests.md | 11 ++++++++--- lib/api/merge_request_diffs.rb | 19 ++++++++++--------- 2 files changed, 18 insertions(+), 12 deletions(-) diff --git a/doc/api/merge_requests.md b/doc/api/merge_requests.md index cf262dc4a80..68b5172ed0d 100644 --- a/doc/api/merge_requests.md +++ b/doc/api/merge_requests.md @@ -903,11 +903,16 @@ Get a list of merge request diff versions. GET /projects/:id/merge_requests/:merge_request_id/versions ``` -Parameters: +| Attribute | Type | Required | Description | +| --------- | ------- | -------- | --------------------- | +| `id` | integer | yes | The ID of the project | +| `merge_request_id` | integer | yes | The ID of the merge request | -- `id` (required) - The ID of a project -- `merge_request_id` (required) - The ID of MR +```bash +curl -H "PRIVATE-TOKEN: 9koXpg98eAheJpvBs5tK" https://gitlab.example.com/api/v3/projects/1/merge_requests/1/versions +``` +Example response: ```json [{ diff --git a/lib/api/merge_request_diffs.rb b/lib/api/merge_request_diffs.rb index 4bd149d1603..be954ff96ac 100644 --- a/lib/api/merge_request_diffs.rb +++ b/lib/api/merge_request_diffs.rb @@ -4,15 +4,16 @@ module API before { authenticate! } resource :projects do - # List merge requests diff versions - # - # Parameters: - # id (required) - The ID of a project - # merge_request_id (required) - The ID of MR - # - # Example: - # GET /projects/:id/merge_requests/:merge_request_id/versions - # + desc 'Get a list of merge request diff versions' do + detail 'This feature was introduced in GitLab 8.12.' + success Entities::MergeRequestDiff + end + + params do + requires :id, type: Integer, desc: 'The ID of a project' + requests :merge_request_id, type: Integer, desc: 'The ID of a merge request' + end + get ":id/merge_requests/:merge_request_id/versions" do merge_request = user_project.merge_requests. find(params[:merge_request_id]) -- cgit v1.2.1 From 08f5f897889dc7e7f00a65f56e988449350e60f2 Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Wed, 3 Aug 2016 12:35:20 +0300 Subject: Fix project id param for merge request version API Signed-off-by: Dmitriy Zaporozhets --- doc/api/merge_requests.md | 2 +- lib/api/merge_request_diffs.rb | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/doc/api/merge_requests.md b/doc/api/merge_requests.md index 68b5172ed0d..0c06cfcd876 100644 --- a/doc/api/merge_requests.md +++ b/doc/api/merge_requests.md @@ -905,7 +905,7 @@ GET /projects/:id/merge_requests/:merge_request_id/versions | Attribute | Type | Required | Description | | --------- | ------- | -------- | --------------------- | -| `id` | integer | yes | The ID of the project | +| `id` | String | yes | The ID of the project | | `merge_request_id` | integer | yes | The ID of the merge request | ```bash diff --git a/lib/api/merge_request_diffs.rb b/lib/api/merge_request_diffs.rb index be954ff96ac..a2f92f81107 100644 --- a/lib/api/merge_request_diffs.rb +++ b/lib/api/merge_request_diffs.rb @@ -10,8 +10,8 @@ module API end params do - requires :id, type: Integer, desc: 'The ID of a project' - requests :merge_request_id, type: Integer, desc: 'The ID of a merge request' + requires :id, type: String, desc: 'The ID of a project' + requires :merge_request_id, type: Integer, desc: 'The ID of a merge request' end get ":id/merge_requests/:merge_request_id/versions" do -- 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 +++++++++++++++++++++++++++-------- spec/models/merge_request_spec.rb | 17 +++++++++++++++++ 2 files changed, 44 insertions(+), 8 deletions(-) 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 diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb index eb382f9ef84..d32bec53a28 100644 --- a/spec/models/merge_request_spec.rb +++ b/spec/models/merge_request_spec.rb @@ -691,4 +691,21 @@ describe MergeRequest, models: true do subject.reload_diff end end + + describe '#branch_merge_base_commit' do + context 'source and target branch exist' do + it { expect(subject.branch_merge_base_commit.sha).to eq('ae73cb07c9eeaf35924a10f713b364d32b2dd34f') } + it { expect(subject.branch_merge_base_commit).to be_a(Commit) } + end + + context 'when the target branch does not exist' do + before do + subject.project.repository.raw_repository.delete_branch(subject.target_branch) + end + + it 'returns nil' do + expect(subject.branch_merge_base_commit).to be_nil + end + 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 ++++ app/views/projects/merge_requests/show/_versions.html.haml | 8 ++++---- 2 files changed, 8 insertions(+), 4 deletions(-) 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 diff --git a/app/views/projects/merge_requests/show/_versions.html.haml b/app/views/projects/merge_requests/show/_versions.html.haml index 93a06154022..84c2b83e330 100644 --- a/app/views/projects/merge_requests/show/_versions.html.haml +++ b/app/views/projects/merge_requests/show/_versions.html.haml @@ -1,7 +1,7 @@ -- diffs_count = @merge_request.merge_request_diffs.count +- merge_request_diffs = @merge_request.merge_request_diffs.select_without_diff +- latest_diff = merge_request_diffs.first -- if diffs_count > 1 - - latest_diff = @merge_request.merge_request_diff +- if merge_request_diffs.size > 1 .mr-version-switch Version:  %span.dropdown.inline @@ -13,7 +13,7 @@ #{@merge_request_diff.head_commit.short_id} %span.caret %ul.dropdown-menu.dropdown-menu-selectable - - @merge_request.merge_request_diffs.each do |merge_request_diff| + - merge_request_diffs.each do |merge_request_diff| %li = link_to diffs_namespace_project_merge_request_path(@project.namespace, @project, @merge_request, diff_id: merge_request_diff.id), class: ('is-active' if merge_request_diff == @merge_request_diff) do %strong.monospace -- cgit v1.2.1 From 445edcfacc583ba613a4b865b104d7d7e1f7ec0f Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Fri, 12 Aug 2016 13:08:16 +0300 Subject: Remove unnecessary   in merge request versions dropdown Signed-off-by: Dmitriy Zaporozhets --- app/views/projects/merge_requests/show/_versions.html.haml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/views/projects/merge_requests/show/_versions.html.haml b/app/views/projects/merge_requests/show/_versions.html.haml index 84c2b83e330..15d94b16ced 100644 --- a/app/views/projects/merge_requests/show/_versions.html.haml +++ b/app/views/projects/merge_requests/show/_versions.html.haml @@ -3,7 +3,7 @@ - if merge_request_diffs.size > 1 .mr-version-switch - Version:  + Version: %span.dropdown.inline %a.btn-link.dropdown-toggle{ data: {toggle: :dropdown} } %strong.monospace< -- cgit v1.2.1 From ac072132b8f0e36badf297208a5964109dbed126 Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Fri, 12 Aug 2016 14:44:49 +0300 Subject: Add single merge request diff API endpoint Signed-off-by: Dmitriy Zaporozhets --- doc/api/merge_requests.md | 72 ++++++++++++++++----------- lib/api/entities.rb | 2 + lib/api/merge_request_diffs.rb | 19 +++++++ spec/requests/api/merge_request_diffs_spec.rb | 17 +++++++ 4 files changed, 81 insertions(+), 29 deletions(-) diff --git a/doc/api/merge_requests.md b/doc/api/merge_requests.md index 0c06cfcd876..130488bf7c1 100644 --- a/doc/api/merge_requests.md +++ b/doc/api/merge_requests.md @@ -916,6 +916,48 @@ Example response: ```json [{ + "id": 110, + "head_commit_sha": "33e2ee8579fda5bc36accc9c6fbd0b4fefda9e30", + "base_commit_sha": "eeb57dffe83deb686a60a71c16c32f71046868fd", + "start_commit_sha": "eeb57dffe83deb686a60a71c16c32f71046868fd", + "created_at": "2016-07-26T14:44:48.926Z", + "merge_request_id": 105, + "state": "collected", + "real_size": "1" +}, { + "id": 108, + "head_commit_sha": "3eed087b29835c48015768f839d76e5ea8f07a24", + "base_commit_sha": "eeb57dffe83deb686a60a71c16c32f71046868fd", + "start_commit_sha": "eeb57dffe83deb686a60a71c16c32f71046868fd", + "created_at": "2016-07-25T14:21:33.028Z", + "merge_request_id": 105, + "state": "collected", + "real_size": "1" +}] +``` + +## Get a single MR diff version + +Get a single merge request diff version. + +``` +GET /projects/:id/merge_requests/:merge_request_id/versions/:version_id +``` + +| Attribute | Type | Required | Description | +| --------- | ------- | -------- | --------------------- | +| `id` | String | yes | The ID of the project | +| `merge_request_id` | integer | yes | The ID of the merge request | +| `version_id` | integer | yes | The ID of the merge request diff version | + +```bash +curl -H "PRIVATE-TOKEN: 9koXpg98eAheJpvBs5tK" https://gitlab.example.com/api/v3/projects/1/merge_requests/1/versions/1 +``` + +Example response: + +```json +{ "id": 110, "head_commit_sha": "33e2ee8579fda5bc36accc9c6fbd0b4fefda9e30", "base_commit_sha": "eeb57dffe83deb686a60a71c16c32f71046868fd", @@ -959,33 +1001,5 @@ Example response: "renamed_file": false, "deleted_file": false }] -}, { - "id": 108, - "head_commit_sha": "3eed087b29835c48015768f839d76e5ea8f07a24", - "base_commit_sha": "eeb57dffe83deb686a60a71c16c32f71046868fd", - "start_commit_sha": "eeb57dffe83deb686a60a71c16c32f71046868fd", - "created_at": "2016-07-25T14:21:33.028Z", - "merge_request_id": 105, - "state": "collected", - "real_size": "1", - "commits": [{ - "id": "3eed087b29835c48015768f839d76e5ea8f07a24", - "short_id": "3eed087b", - "title": "Add license", - "author_name": "Administrator", - "author_email": "admin@example.com", - "created_at": "2016-07-25T17:21:20.000+03:00", - "message": "Add license" - }], - "diffs": [{ - "old_path": "LICENSE", - "new_path": "LICENSE", - "a_mode": "0", - "b_mode": "100644", - "diff": "--- /dev/null\n+++ b/LICENSE\n@@ -0,0 +1,21 @@\n+The MIT License (MIT)\n+\n+Copyright (c) 2016 Administrator\n+\n+Permission is hereby granted, free of charge, to any person obtaining a copy\n+of this software and associated documentation files (the \"Software\"), to deal\n+in the Software without restriction, including without limitation the rights\n+to use, copy, modify, merge, publish, distribute, sublicense, and/or sell\n+copies of the Software, and to permit persons to whom the Software is\n+furnished to do so, subject to the following conditions:\n+\n+The above copyright notice and this permission notice shall be included in all\n+copies or substantial portions of the Software.\n+\n+THE SOFTWARE IS PROVIDED \"AS IS\", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR\n+IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,\n+FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE\n+AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER\n+LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,\n+OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE\n+SOFTWARE.\n", - "new_file": true, - "renamed_file": false, - "deleted_file": false - }] -}] +} ``` diff --git a/lib/api/entities.rb b/lib/api/entities.rb index af069063f0c..f582b2ce250 100644 --- a/lib/api/entities.rb +++ b/lib/api/entities.rb @@ -231,7 +231,9 @@ module API class MergeRequestDiff < Grape::Entity expose :id, :head_commit_sha, :base_commit_sha, :start_commit_sha, :created_at, :merge_request_id, :state, :real_size + end + class MergeRequestDiffFull < MergeRequestDiff expose :commits, using: Entities::RepoCommit expose :diffs, using: Entities::RepoDiff do |compare, _| diff --git a/lib/api/merge_request_diffs.rb b/lib/api/merge_request_diffs.rb index a2f92f81107..07435d78468 100644 --- a/lib/api/merge_request_diffs.rb +++ b/lib/api/merge_request_diffs.rb @@ -21,6 +21,25 @@ module API authorize! :read_merge_request, merge_request present merge_request.merge_request_diffs, with: Entities::MergeRequestDiff end + + desc 'Get a single merge request diff version' do + detail 'This feature was introduced in GitLab 8.12.' + success Entities::MergeRequestDiffFull + end + + params do + requires :id, type: String, desc: 'The ID of a project' + requires :merge_request_id, type: Integer, desc: 'The ID of a merge request' + requires :version_id, type: Integer, desc: 'The ID of a merge request diff version' + end + + get ":id/merge_requests/:merge_request_id/versions/:version_id" do + merge_request = user_project.merge_requests. + find(params[:merge_request_id]) + + authorize! :read_merge_request, merge_request + present merge_request.merge_request_diffs.find(params[:version_id]), with: Entities::MergeRequestDiffFull + end end end end diff --git a/spec/requests/api/merge_request_diffs_spec.rb b/spec/requests/api/merge_request_diffs_spec.rb index db076b4b9a5..8f1e5ac9891 100644 --- a/spec/requests/api/merge_request_diffs_spec.rb +++ b/spec/requests/api/merge_request_diffs_spec.rb @@ -29,4 +29,21 @@ describe API::API, 'MergeRequestDiffs', api: true do expect(response).to have_http_status(404) end end + + describe 'GET /projects/:id/merge_requests/:merge_request_id/versions/:version_id' do + context 'valid merge request' do + before { get api("/projects/#{project.id}/merge_requests/#{merge_request.id}/versions/#{merge_request_diff.id}", user) } + let(:merge_request_diff) { merge_request.merge_request_diffs.first } + + it { expect(response.status).to eq 200 } + it { expect(json_response['id']).to eq(merge_request_diff.id) } + it { expect(json_response['head_commit_sha']).to eq(merge_request_diff.head_commit_sha) } + it { expect(json_response['diffs'].size).to eq(merge_request_diff.diffs.size) } + end + + it 'returns a 404 when merge_request_id not found' do + get api("/projects/#{project.id}/merge_requests/#{merge_request.id}/versions/999", user) + expect(response).to have_http_status(404) + end + end end -- cgit v1.2.1 From a8fe213ebbb4a1aa48a20c908ba989190d20f2a1 Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Fri, 12 Aug 2016 15:40:00 +0300 Subject: Add documentation for merge request versions Signed-off-by: Dmitriy Zaporozhets --- doc/workflow/merge_requests.md | 8 ++++++++ doc/workflow/merge_requests/versions.png | Bin 0 -> 100566 bytes 2 files changed, 8 insertions(+) create mode 100644 doc/workflow/merge_requests/versions.png diff --git a/doc/workflow/merge_requests.md b/doc/workflow/merge_requests.md index d2ec56e6504..a81b1080ca6 100644 --- a/doc/workflow/merge_requests.md +++ b/doc/workflow/merge_requests.md @@ -61,3 +61,11 @@ If you click the "Hide whitespace changes" button, you can see the diff without It is also working on commits compare view. ![Commit Compare](merge_requests/commit_compare.png) + +## Merge Requests versions + +Every time you push to merge request branch, a new version of merge request diff +is created. When you visit the merge request page you see latest version of changes. +However you can select an older one from version dropdown + +![Merge Request Versions](merge_requests/versions.png) diff --git a/doc/workflow/merge_requests/versions.png b/doc/workflow/merge_requests/versions.png new file mode 100644 index 00000000000..c0a6dfe6806 Binary files /dev/null and b/doc/workflow/merge_requests/versions.png differ -- 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(-) 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(-) 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 --- .../projects/merge_requests_controller.rb | 2 +- app/models/merge_request.rb | 6 +- app/models/merge_request_diff.rb | 14 +++-- .../projects/merge_requests/show/_diffs.html.haml | 1 + lib/gitlab/diff/file_collection/merge_request.rb | 73 ---------------------- .../diff/file_collection/merge_request_diff.rb | 73 ++++++++++++++++++++++ 6 files changed, 87 insertions(+), 82 deletions(-) delete mode 100644 lib/gitlab/diff/file_collection/merge_request.rb create mode 100644 lib/gitlab/diff/file_collection/merge_request_diff.rb diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb index a0b80c823fb..a4b1262896c 100644 --- a/app/controllers/projects/merge_requests_controller.rb +++ b/app/controllers/projects/merge_requests_controller.rb @@ -91,7 +91,7 @@ class Projects::MergeRequestsController < Projects::ApplicationController respond_to do |format| format.html { define_discussion_vars } format.json do - @diffs = @merge_request.diffs(diff_options) + @diffs = @merge_request_diff.diffs(diff_options) render json: { html: view_to_html_string("projects/merge_requests/show/_diffs") } end 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 diff --git a/app/views/projects/merge_requests/show/_diffs.html.haml b/app/views/projects/merge_requests/show/_diffs.html.haml index 013b05628fa..99c71e1454a 100644 --- a/app/views/projects/merge_requests/show/_diffs.html.haml +++ b/app/views/projects/merge_requests/show/_diffs.html.haml @@ -1,4 +1,5 @@ - if @merge_request_diff.collected? + = render 'projects/merge_requests/show/versions' = render "projects/diffs/diffs", diffs: @diffs - elsif @merge_request_diff.empty? .nothing-here-block Nothing to merge from #{@merge_request.source_branch} into #{@merge_request.target_branch} diff --git a/lib/gitlab/diff/file_collection/merge_request.rb b/lib/gitlab/diff/file_collection/merge_request.rb deleted file mode 100644 index 4f946908e2f..00000000000 --- a/lib/gitlab/diff/file_collection/merge_request.rb +++ /dev/null @@ -1,73 +0,0 @@ -module Gitlab - module Diff - module FileCollection - class MergeRequest < Base - def initialize(merge_request, diff_options:) - @merge_request = merge_request - - super(merge_request, - project: merge_request.project, - diff_options: diff_options, - diff_refs: merge_request.diff_refs) - end - - def diff_files - super.tap { |_| store_highlight_cache } - end - - private - - # Extracted method to highlight in the same iteration to the diff_collection. - def decorate_diff!(diff) - diff_file = super - cache_highlight!(diff_file) if cacheable? - diff_file - end - - def highlight_diff_file_from_cache!(diff_file, cache_diff_lines) - diff_file.highlighted_diff_lines = cache_diff_lines.map do |line| - Gitlab::Diff::Line.init_from_hash(line) - end - end - - # - # If we find the highlighted diff files lines on the cache we replace existing diff_files lines (no highlighted) - # for the highlighted ones, so we just skip their execution. - # If the highlighted diff files lines are not cached we calculate and cache them. - # - # The content of the cache is a Hash where the key correspond to the file_path and the values are Arrays of - # hashes that represent serialized diff lines. - # - def cache_highlight!(diff_file) - file_path = diff_file.file_path - - if highlight_cache[file_path] - highlight_diff_file_from_cache!(diff_file, highlight_cache[file_path]) - else - highlight_cache[file_path] = diff_file.highlighted_diff_lines.map(&:to_hash) - end - end - - def highlight_cache - return @highlight_cache if defined?(@highlight_cache) - - @highlight_cache = Rails.cache.read(cache_key) || {} - @highlight_cache_was_empty = @highlight_cache.empty? - @highlight_cache - end - - def store_highlight_cache - Rails.cache.write(cache_key, highlight_cache) if @highlight_cache_was_empty - end - - def cacheable? - @merge_request.merge_request_diff.present? - end - - def cache_key - [@merge_request.merge_request_diff, 'highlighted-diff-files', diff_options] - end - end - end - end -end diff --git a/lib/gitlab/diff/file_collection/merge_request_diff.rb b/lib/gitlab/diff/file_collection/merge_request_diff.rb new file mode 100644 index 00000000000..36348b33943 --- /dev/null +++ b/lib/gitlab/diff/file_collection/merge_request_diff.rb @@ -0,0 +1,73 @@ +module Gitlab + module Diff + module FileCollection + class MergeRequestDiff < Base + def initialize(merge_request_diff, diff_options:) + @merge_request_diff = merge_request_diff + + super(merge_request_diff, + project: merge_request_diff.project, + diff_options: diff_options, + diff_refs: merge_request_diff.diff_refs) + end + + def diff_files + super.tap { |_| store_highlight_cache } + end + + private + + # Extracted method to highlight in the same iteration to the diff_collection. + def decorate_diff!(diff) + diff_file = super + cache_highlight!(diff_file) if cacheable? + diff_file + end + + def highlight_diff_file_from_cache!(diff_file, cache_diff_lines) + diff_file.highlighted_diff_lines = cache_diff_lines.map do |line| + Gitlab::Diff::Line.init_from_hash(line) + end + end + + # + # If we find the highlighted diff files lines on the cache we replace existing diff_files lines (no highlighted) + # for the highlighted ones, so we just skip their execution. + # If the highlighted diff files lines are not cached we calculate and cache them. + # + # The content of the cache is a Hash where the key correspond to the file_path and the values are Arrays of + # hashes that represent serialized diff lines. + # + def cache_highlight!(diff_file) + file_path = diff_file.file_path + + if highlight_cache[file_path] + highlight_diff_file_from_cache!(diff_file, highlight_cache[file_path]) + else + highlight_cache[file_path] = diff_file.highlighted_diff_lines.map(&:to_hash) + end + end + + def highlight_cache + return @highlight_cache if defined?(@highlight_cache) + + @highlight_cache = Rails.cache.read(cache_key) || {} + @highlight_cache_was_empty = @highlight_cache.empty? + @highlight_cache + end + + def store_highlight_cache + Rails.cache.write(cache_key, highlight_cache) if @highlight_cache_was_empty + end + + def cacheable? + @merge_request_diff.present? + end + + def cache_key + [@merge_request_diff, 'highlighted-diff-files', diff_options] + end + end + end + end +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 +- spec/models/merge_request_spec.rb | 13 +++++++------ 4 files changed, 11 insertions(+), 27 deletions(-) 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, diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb index 9de9f19a27c..7242c4fc8d3 100644 --- a/spec/models/merge_request_spec.rb +++ b/spec/models/merge_request_spec.rb @@ -723,7 +723,6 @@ describe MergeRequest, models: true do end end -<<<<<<< HEAD describe '#branch_merge_base_commit' do context 'source and target branch exist' do it { expect(subject.branch_merge_base_commit.sha).to eq('ae73cb07c9eeaf35924a10f713b364d32b2dd34f') } @@ -737,8 +736,11 @@ describe MergeRequest, models: true do it 'returns nil' do expect(subject.branch_merge_base_commit).to be_nil -======= - describe "#diff_sha_refs" do + end + end + end + + describe "#diff_refs" do context "with diffs" do subject { create(:merge_request, :with_diffs) } @@ -747,7 +749,7 @@ describe MergeRequest, models: true do expect_any_instance_of(Repository).not_to receive(:commit) - subject.diff_sha_refs + subject.diff_refs end it "returns expected diff_refs" do @@ -757,8 +759,7 @@ describe MergeRequest, models: true do head_sha: subject.merge_request_diff.head_commit_sha ) - expect(subject.diff_sha_refs).to eq(expected_diff_refs) ->>>>>>> master + expect(subject.diff_refs).to eq(expected_diff_refs) end end end -- cgit v1.2.1 From a94553a90c16b58c9cc9d56488e3aa3c35b8d699 Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Mon, 15 Aug 2016 18:22:26 +0300 Subject: Fix merge request diffs spec Signed-off-by: Dmitriy Zaporozhets --- spec/models/merge_request_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb index 7242c4fc8d3..c066b65e7ba 100644 --- a/spec/models/merge_request_spec.rb +++ b/spec/models/merge_request_spec.rb @@ -159,7 +159,7 @@ describe MergeRequest, models: true do context 'when there are MR diffs' do it 'delegates to the MR diffs' do - merge_request.merge_request_diffs.build + merge_request.save expect(merge_request.merge_request_diff).to receive(:raw_diffs).with(hash_including(options)) -- 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(+) 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 74461ccc1f1503c86102b7d8e790ebac0d28fc0b Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Fri, 19 Aug 2016 12:35:40 +0300 Subject: Fix merge request versions api doc Signed-off-by: Dmitriy Zaporozhets --- doc/api/merge_requests.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/doc/api/merge_requests.md b/doc/api/merge_requests.md index 66957647d00..1cf4940198f 100644 --- a/doc/api/merge_requests.md +++ b/doc/api/merge_requests.md @@ -909,7 +909,7 @@ GET /projects/:id/merge_requests/:merge_request_id/versions | `merge_request_id` | integer | yes | The ID of the merge request | ```bash -curl -H "PRIVATE-TOKEN: 9koXpg98eAheJpvBs5tK" https://gitlab.example.com/api/v3/projects/1/merge_requests/1/versions +curl --header "PRIVATE-TOKEN: 9koXpg98eAheJpvBs5tK" https://gitlab.example.com/api/v3/projects/1/merge_requests/1/versions ``` Example response: @@ -951,7 +951,7 @@ GET /projects/:id/merge_requests/:merge_request_id/versions/:version_id | `version_id` | integer | yes | The ID of the merge request diff version | ```bash -curl -H "PRIVATE-TOKEN: 9koXpg98eAheJpvBs5tK" https://gitlab.example.com/api/v3/projects/1/merge_requests/1/versions/1 +curl --header "PRIVATE-TOKEN: 9koXpg98eAheJpvBs5tK" https://gitlab.example.com/api/v3/projects/1/merge_requests/1/versions/1 ``` Example response: -- 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 ++++++++++++++++--------------- spec/models/merge_request_spec.rb | 6 +++--- 3 files changed, 24 insertions(+), 19 deletions(-) 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:) diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb index 573c8c6c9ce..18d178be49c 100644 --- a/spec/models/merge_request_spec.rb +++ b/spec/models/merge_request_spec.rb @@ -782,7 +782,7 @@ describe MergeRequest, models: true do end end - describe "#diff_refs" do + describe "#diff_sha_refs" do context "with diffs" do subject { create(:merge_request, :with_diffs) } @@ -791,7 +791,7 @@ describe MergeRequest, models: true do expect_any_instance_of(Repository).not_to receive(:commit) - subject.diff_refs + subject.diff_sha_refs end it "returns expected diff_refs" do @@ -801,7 +801,7 @@ describe MergeRequest, models: true do head_sha: subject.merge_request_diff.head_commit_sha ) - expect(subject.diff_refs).to eq(expected_diff_refs) + expect(subject.diff_sha_refs).to eq(expected_diff_refs) end end end -- cgit v1.2.1 From dde44b83ddeae75a5c14810d0571ee5c7ab0a26d Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Mon, 22 Aug 2016 15:30:23 +0300 Subject: Fix merge request diff api Signed-off-by: Dmitriy Zaporozhets --- lib/api/entities.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/api/entities.rb b/lib/api/entities.rb index 3f050a8fd81..3241cb7ca56 100644 --- a/lib/api/entities.rb +++ b/lib/api/entities.rb @@ -246,7 +246,7 @@ module API expose :commits, using: Entities::RepoCommit expose :diffs, using: Entities::RepoDiff do |compare, _| - compare.diffs(all_diffs: true).to_a + compare.raw_diffs(all_diffs: true).to_a end end -- cgit v1.2.1 From d7a9dcc1b14d6abc723b232dbedd025469511cb7 Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Mon, 22 Aug 2016 15:44:48 +0300 Subject: Fix merge request diff cache service spec Signed-off-by: Dmitriy Zaporozhets --- spec/services/merge_requests/merge_request_diff_cache_service_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/services/merge_requests/merge_request_diff_cache_service_spec.rb b/spec/services/merge_requests/merge_request_diff_cache_service_spec.rb index c4b87468275..807f89e80b7 100644 --- a/spec/services/merge_requests/merge_request_diff_cache_service_spec.rb +++ b/spec/services/merge_requests/merge_request_diff_cache_service_spec.rb @@ -6,7 +6,7 @@ describe MergeRequests::MergeRequestDiffCacheService do describe '#execute' do it 'retrieves the diff files to cache the highlighted result' do merge_request = create(:merge_request) - cache_key = [merge_request.merge_request_diff, 'highlighted-diff-files', Gitlab::Diff::FileCollection::MergeRequest.default_options] + cache_key = [merge_request.merge_request_diff, 'highlighted-diff-files', Gitlab::Diff::FileCollection::MergeRequestDiff.default_options] expect(Rails.cache).to receive(:read).with(cache_key).and_return({}) expect(Rails.cache).to receive(:write).with(cache_key, anything) -- cgit v1.2.1 From 7b3fd81673e595b48b4c0e5f1ca5f1ab806f8ac3 Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Mon, 22 Aug 2016 18:21:28 +0300 Subject: Disable comments on older merge request diff Signed-off-by: Dmitriy Zaporozhets --- app/controllers/projects/merge_requests_controller.rb | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb index 438dd928853..eff959c781c 100644 --- a/app/controllers/projects/merge_requests_controller.rb +++ b/app/controllers/projects/merge_requests_controller.rb @@ -93,6 +93,11 @@ class Projects::MergeRequestsController < Projects::ApplicationController respond_to do |format| format.html { define_discussion_vars } format.json do + if @merge_request_diff != @merge_request.merge_request_diff + # Disable comments if browsing older version of the diff + @diff_notes_disabled = true + end + @diffs = @merge_request_diff.diffs(diff_options) render json: { html: view_to_html_string("projects/merge_requests/show/_diffs") } -- cgit v1.2.1 From 4b1a98ba9847579db32da95ed479e2da098eb155 Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Mon, 22 Aug 2016 21:15:06 +0300 Subject: Fix db schema via db:migrate:reset Signed-off-by: Dmitriy Zaporozhets --- db/schema.rb | 34 ++++++++++++++++------------------ 1 file changed, 16 insertions(+), 18 deletions(-) diff --git a/db/schema.rb b/db/schema.rb index 4947745b232..77f114ede45 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -70,11 +70,11 @@ ActiveRecord::Schema.define(version: 20160819221833) do t.string "recaptcha_site_key" t.string "recaptcha_private_key" t.integer "metrics_port", default: 8089 + t.boolean "akismet_enabled", default: false + t.string "akismet_api_key" t.integer "metrics_sample_interval", default: 15 t.boolean "sentry_enabled", default: false t.string "sentry_dsn" - t.boolean "akismet_enabled", default: false - t.string "akismet_api_key" t.boolean "email_author_in_body", default: false t.integer "default_group_visibility" t.boolean "repository_checks_enabled", default: false @@ -467,11 +467,10 @@ ActiveRecord::Schema.define(version: 20160819221833) do t.string "state" t.integer "iid" t.integer "updated_by_id" - t.integer "moved_to_id" t.boolean "confidential", default: false t.datetime "deleted_at" t.date "due_date" - t.integer "lock_version", default: 0, null: false + t.integer "moved_to_id" end add_index "issues", ["assignee_id"], name: "index_issues_on_assignee_id", using: :btree @@ -614,13 +613,12 @@ ActiveRecord::Schema.define(version: 20160819221833) do t.datetime "locked_at" t.integer "updated_by_id" t.string "merge_error" + t.text "merge_params" t.boolean "merge_when_build_succeeds", default: false, null: false t.integer "merge_user_id" t.string "merge_commit_sha" t.datetime "deleted_at" - t.integer "lock_version", default: 0, null: false t.string "in_progress_merge_commit_sha" - t.text "merge_params" end add_index "merge_requests", ["assignee_id"], name: "index_merge_requests_on_assignee_id", using: :btree @@ -915,9 +913,9 @@ ActiveRecord::Schema.define(version: 20160819221833) do t.string "type" t.string "title" t.integer "project_id" - t.datetime "created_at", null: false - t.datetime "updated_at", null: false - t.boolean "active", null: false + t.datetime "created_at" + t.datetime "updated_at" + t.boolean "active", default: false, null: false t.text "properties" t.boolean "template", default: false t.boolean "push_events", default: true @@ -962,8 +960,8 @@ ActiveRecord::Schema.define(version: 20160819221833) do t.string "noteable_type" t.string "title" t.text "description" - t.datetime "created_at", null: false - t.datetime "updated_at", null: false + t.datetime "created_at", null: false + t.datetime "updated_at", null: false t.boolean "submitted_as_ham", default: false, null: false end @@ -1034,13 +1032,13 @@ ActiveRecord::Schema.define(version: 20160819221833) do add_index "u2f_registrations", ["user_id"], name: "index_u2f_registrations_on_user_id", using: :btree create_table "user_agent_details", force: :cascade do |t| - t.string "user_agent", null: false - t.string "ip_address", null: false - t.integer "subject_id", null: false - t.string "subject_type", null: false + t.string "user_agent", null: false + t.string "ip_address", null: false + t.integer "subject_id", null: false + t.string "subject_type", null: false t.boolean "submitted", default: false, null: false - t.datetime "created_at", null: false - t.datetime "updated_at", null: false + t.datetime "created_at", null: false + t.datetime "updated_at", null: false end create_table "users", force: :cascade do |t| @@ -1156,4 +1154,4 @@ ActiveRecord::Schema.define(version: 20160819221833) do add_foreign_key "protected_branch_merge_access_levels", "protected_branches" add_foreign_key "protected_branch_push_access_levels", "protected_branches" add_foreign_key "u2f_registrations", "users" -end \ No newline at end of file +end -- cgit v1.2.1 From 03671c30df268c787420cb163beb41d208a3c9e7 Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Mon, 22 Aug 2016 21:17:17 +0300 Subject: Add changelog item for merge request versions feature Signed-off-by: Dmitriy Zaporozhets --- CHANGELOG | 3 +++ 1 file changed, 3 insertions(+) diff --git a/CHANGELOG b/CHANGELOG index c6a4fe5f5b8..88a2acd15d2 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -1,5 +1,8 @@ Please view this file on the master branch, on stable branches it's out of date. +v 8.12.0 (unreleased) + - Add merge request versions !5467 + v 8.11.0 (unreleased) - Use test coverage value from the latest successful pipeline in badge. !5862 - Add test coverage report badge. !5708 -- 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/controllers/projects/merge_requests_controller.rb | 2 +- app/models/merge_request_diff.rb | 16 ++++++++-------- .../projects/merge_requests/show/_versions.html.haml | 3 +-- spec/models/merge_request_diff_spec.rb | 9 +++++++++ 4 files changed, 19 insertions(+), 11 deletions(-) diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb index eff959c781c..3be9bdb076d 100644 --- a/app/controllers/projects/merge_requests_controller.rb +++ b/app/controllers/projects/merge_requests_controller.rb @@ -93,7 +93,7 @@ class Projects::MergeRequestsController < Projects::ApplicationController respond_to do |format| format.html { define_discussion_vars } format.json do - if @merge_request_diff != @merge_request.merge_request_diff + unless @merge_request_diff.latest? # Disable comments if browsing older version of the diff @diff_notes_disabled = true end 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) diff --git a/app/views/projects/merge_requests/show/_versions.html.haml b/app/views/projects/merge_requests/show/_versions.html.haml index 15d94b16ced..08c54f0aeff 100644 --- a/app/views/projects/merge_requests/show/_versions.html.haml +++ b/app/views/projects/merge_requests/show/_versions.html.haml @@ -1,5 +1,4 @@ - merge_request_diffs = @merge_request.merge_request_diffs.select_without_diff -- latest_diff = merge_request_diffs.first - if merge_request_diffs.size > 1 .mr-version-switch @@ -7,7 +6,7 @@ %span.dropdown.inline %a.btn-link.dropdown-toggle{ data: {toggle: :dropdown} } %strong.monospace< - - if latest_diff == @merge_request_diff + - if @merge_request_diff.latest? #{"latest"} - else #{@merge_request_diff.head_commit.short_id} diff --git a/spec/models/merge_request_diff_spec.rb b/spec/models/merge_request_diff_spec.rb index 2f3bb3ed2c4..e5b185dc3f6 100644 --- a/spec/models/merge_request_diff_spec.rb +++ b/spec/models/merge_request_diff_spec.rb @@ -13,6 +13,15 @@ describe MergeRequestDiff, models: true do it { expect(subject.start_commit_sha).to eq('0b4bc9a49b562e85de7cc9e834518ea6828729b9') } end + describe '#latest' do + let!(:mr) { create(:merge_request, :with_diffs) } + let!(:first_diff) { mr.merge_request_diff } + let!(:last_diff) { mr.create_merge_request_diff } + + it { expect(last_diff.latest?).to be_truthy } + it { expect(first_diff.latest?).to be_falsey } + end + describe '#diffs' do let(:mr) { create(:merge_request, :with_diffs) } let(:mr_diff) { mr.merge_request_diff } -- cgit v1.2.1 From 70fe671c299513a43797dc1e7ab0e32f14503175 Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Thu, 25 Aug 2016 11:16:03 +0300 Subject: Make it more obvious when the user isn't currently viewing the latest mr version Signed-off-by: Dmitriy Zaporozhets --- app/views/projects/merge_requests/show/_versions.html.haml | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/app/views/projects/merge_requests/show/_versions.html.haml b/app/views/projects/merge_requests/show/_versions.html.haml index 08c54f0aeff..2da70ce7137 100644 --- a/app/views/projects/merge_requests/show/_versions.html.haml +++ b/app/views/projects/merge_requests/show/_versions.html.haml @@ -22,6 +22,10 @@ #{number_with_delimiter(merge_request_diff.commits.count)} #{'commit'.pluralize(merge_request_diff.commits.count)}, = time_ago_with_tooltip(merge_request_diff.created_at) + - unless @merge_request_diff.latest? + %span.prepend-left-default + = icon('info-circle') + This version is not the latest one. Comments are disabled .pull-right %span.monospace #{@merge_request_diff.base_commit.short_id}..#{@merge_request_diff.head_commit.short_id} -- 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(-) 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