diff options
21 files changed, 559 insertions, 115 deletions
diff --git a/app/assets/stylesheets/framework/dropdowns.scss b/app/assets/stylesheets/framework/dropdowns.scss index e8eafa15899..1f565d78387 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..7d8f58ca5af 100644 --- a/app/assets/stylesheets/pages/merge_requests.scss +++ b/app/assets/stylesheets/pages/merge_requests.scss @@ -371,3 +371,13 @@ } } } + +.mr-version-switch { + background: $background-color; + padding: $gl-btn-padding; + color: $gl-placeholder-color; + + a.btn-link { + color: $gl-dark-link-color; + } +} diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb index 139680d2df9..a0b80c823fb 100644 --- a/app/controllers/projects/merge_requests_controller.rb +++ b/app/controllers/projects/merge_requests_controller.rb @@ -81,7 +81,12 @@ 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.rb b/app/models/merge_request.rb index f6d0d0c98f5..7c3845ef538 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -10,14 +10,16 @@ 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_one :merge_request_diff, + -> { order('merge_request_diffs.id DESC') } has_many :events, as: :target, dependent: :destroy serialize :merge_params, Hash - after_create :create_merge_request_diff, unless: :importing? - 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 @@ -184,8 +186,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 @@ -246,6 +248,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 @@ -254,14 +265,26 @@ class MergeRequest < ActiveRecord::Base @source_branch_sha || source_branch_head.try(:sha) end - def diff_refs - return unless diff_start_commit || diff_base_commit + def branch_merge_base_sha + branch_merge_base_commit.try(:sha) + end - Gitlab::Diff::DiffRefs.new( - base_sha: diff_base_sha, - start_sha: diff_start_sha, - head_sha: diff_head_sha - ) + def diff_refs + if persisted? + 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 + end end # Return diff_refs instance trying to not touch the git repository @@ -309,21 +332,31 @@ class MergeRequest < ActiveRecord::Base end end - def update_merge_request_diff + def ensure_merge_request_diff + 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 end end def reload_diff - return unless merge_request_diff && open? + return unless open? old_diff_refs = self.diff_refs - - merge_request_diff.reload_content - + create_merge_request_diff MergeRequests::MergeRequestDiffCacheService.new.execute(self) - new_diff_refs = self.diff_refs update_diff_notes_positions( diff --git a/app/models/merge_request_diff.rb b/app/models/merge_request_diff.rb index 32cc6a3bfea..8920641cfec 100644 --- a/app/models/merge_request_diff.rb +++ b/app/models/merge_request_diff.rb @@ -8,8 +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 @@ -24,12 +22,53 @@ class MergeRequestDiff < ActiveRecord::Base serialize :st_commits serialize :st_diffs - after_create :reload_content, unless: :importing? - after_save :keep_around_commits, 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? + + def self.select_without_diff + select(column_names - ['st_diffs']) + end - def reload_content + # 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 - reload_diffs + save_diffs + keep_around_commits + end + + def ensure_commits_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 + 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 + # 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 || merge_request.source_branch_sha end def size @@ -38,14 +77,15 @@ class MergeRequestDiff < ActiveRecord::Base def raw_diffs(options = {}) if options[:ignore_whitespace_change] - @raw_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, - ) - compare.diffs(options) - end + @diffs_no_whitespace ||= + begin + compare = Gitlab::Git::Compare.new( + repository.raw_repository, + safe_start_commit_sha, + safe_head_commit_sha + ) + compare.diffs(options) + end else @raw_diffs ||= {} @raw_diffs[options] ||= load_diffs(st_diffs, options) @@ -56,6 +96,11 @@ class MergeRequestDiff < ActiveRecord::Base @commits ||= load_commits(st_commits || []) end + def reload_commits + @commits = nil + commits + end + def last_commit commits.first end @@ -65,27 +110,39 @@ 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 unless head_commit_sha + + project.commit(head_commit_sha) + end + + def diff_refs + return unless start_commit || base_commit - project.commit(self.head_commit_sha) + Gitlab::Diff::DiffRefs.new( + base_sha: base_commit_sha, + start_sha: start_commit_sha, + head_sha: head_commit_sha + ) end def diff_refs_by_sha? base_commit_sha? && head_commit_sha? && start_commit_sha? end + private + def compare @compare ||= begin @@ -94,26 +151,12 @@ class MergeRequestDiff < ActiveRecord::Base Gitlab::Git::Compare.new( repository.raw_repository, - self.target_branch_sha, - self.source_branch_sha + safe_start_commit_sha, + safe_head_commit_sha ) end end - 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 - - commits - end - def dump_commits(commits) commits.map(&:to_hash) end @@ -122,26 +165,21 @@ 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 = {} - 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) @@ -162,16 +200,16 @@ 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 = [] 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? @@ -188,14 +226,7 @@ class MergeRequestDiff < ActiveRecord::Base end 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 end def project @@ -206,14 +237,10 @@ class MergeRequestDiff < ActiveRecord::Base project.repository end - def branch_base_commit - return unless self.source_branch_sha && self.target_branch_sha - - project.merge_base_commit(self.source_branch_sha, self.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 @@ -248,8 +275,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 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..15d94b16ced --- /dev/null +++ b/app/views/projects/merge_requests/show/_versions.html.haml @@ -0,0 +1,28 @@ +- 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 + Version: + %span.dropdown.inline + %a.btn-link.dropdown-toggle{ data: {toggle: :dropdown} } + %strong.monospace< + - if latest_diff == @merge_request_diff + #{"latest"} + - else + #{@merge_request_diff.head_commit.short_id} + %span.caret + %ul.dropdown-menu.dropdown-menu-selectable + - 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.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 + #{@merge_request_diff.base_commit.short_id}..#{@merge_request_diff.head_commit.short_id} 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 1de2cdcf026..0b61e19d998 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -70,11 +70,11 @@ ActiveRecord::Schema.define(version: 20160810142633) 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 @@ -84,8 +84,8 @@ ActiveRecord::Schema.define(version: 20160810142633) 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 @@ -165,8 +165,8 @@ ActiveRecord::Schema.define(version: 20160810142633) 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" @@ -457,10 +457,11 @@ ActiveRecord::Schema.define(version: 20160810142633) 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 "moved_to_id" + t.integer "lock_version", default: 0, null: false end add_index "issues", ["assignee_id"], name: "index_issues_on_assignee_id", using: :btree @@ -568,7 +569,7 @@ ActiveRecord::Schema.define(version: 20160810142633) 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 @@ -594,6 +595,7 @@ ActiveRecord::Schema.define(version: 20160810142633) do 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" end @@ -743,10 +745,10 @@ ActiveRecord::Schema.define(version: 20160810142633) 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 @@ -808,8 +810,8 @@ ActiveRecord::Schema.define(version: 20160810142633) 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", ["ci_id"], name: "index_projects_on_ci_id", using: :btree @@ -883,9 +885,9 @@ ActiveRecord::Schema.define(version: 20160810142633) do t.string "type" t.string "title" t.integer "project_id" - t.datetime "created_at" - t.datetime "updated_at" - t.boolean "active", default: false, null: false + t.datetime "created_at", null: false + t.datetime "updated_at", null: false + t.boolean "active", null: false t.text "properties" t.boolean "template", default: false t.boolean "push_events", default: true diff --git a/doc/api/merge_requests.md b/doc/api/merge_requests.md index 3e88a758936..66957647d00 100644 --- a/doc/api/merge_requests.md +++ b/doc/api/merge_requests.md @@ -894,3 +894,112 @@ 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 +``` + +| Attribute | Type | Required | Description | +| --------- | ------- | -------- | --------------------- | +| `id` | String | yes | The ID of the project | +| `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 +``` + +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", + "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 + }] +} +``` 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.  + +## 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 + + diff --git a/doc/workflow/merge_requests/versions.png b/doc/workflow/merge_requests/versions.png Binary files differnew file mode 100644 index 00000000000..c0a6dfe6806 --- /dev/null +++ b/doc/workflow/merge_requests/versions.png diff --git a/features/project/merge_requests.feature b/features/project/merge_requests.feature index 6bac6011467..7c70c341511 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 53d1aedf27f..0c63ffc2e3a 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/lib/api/api.rb b/lib/api/api.rb index d43af3f24e9..aa34110c34b 100644 --- a/lib/api/api.rb +++ b/lib/api/api.rb @@ -73,5 +73,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 ae74d14a4bb..b305fce9fcf 100644 --- a/lib/api/entities.rb +++ b/lib/api/entities.rb @@ -230,6 +230,19 @@ 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 + end + + class MergeRequestDiffFull < MergeRequestDiff + 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..07435d78468 --- /dev/null +++ b/lib/api/merge_request_diffs.rb @@ -0,0 +1,45 @@ +module API + # MergeRequestDiff API + class MergeRequestDiffs < Grape::API + before { authenticate! } + + resource :projects do + 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: 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 + 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 + + 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/features/merge_requests/merge_request_versions_spec.rb b/spec/features/merge_requests/merge_request_versions_spec.rb new file mode 100644 index 00000000000..577c910f11b --- /dev/null +++ b/spec/features/merge_requests/merge_request_versions_spec.rb @@ -0,0 +1,37 @@ +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 + 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 diff --git a/spec/models/merge_request_diff_spec.rb b/spec/models/merge_request_diff_spec.rb index 29f7396f862..2f3bb3ed2c4 100644 --- a/spec/models/merge_request_diff_spec.rb +++ b/spec/models/merge_request_diff_spec.rb @@ -1,6 +1,18 @@ require 'spec_helper' describe MergeRequestDiff, models: true do + describe 'create new record' do + 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) } + 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 let(:mr) { create(:merge_request, :with_diffs) } let(:mr_diff) { mr.merge_request_diff } diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb index 3270b877c1a..9de9f19a27c 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 @@ -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_diff = MergeRequestDiff.new + merge_request.merge_request_diffs.build expect(merge_request.merge_request_diff).to receive(:raw_diffs).with(hash_including(options)) @@ -316,7 +316,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 @@ -679,12 +679,15 @@ 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(:save_git_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 "executs diff cache service" do expect_any_instance_of(MergeRequests::MergeRequestDiffCacheService).to receive(:execute).with(subject) @@ -694,13 +697,15 @@ describe MergeRequest, models: true do 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( + base_commit_sha: commit.parent_id, + start_commit_sha: commit.parent_id, + head_commit_sha: commit.sha + ) + + subject.merge_request_diff(true) end expect(Notes::DiffPositionUpdateService).to receive(:new).with( @@ -710,14 +715,29 @@ 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 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') } + 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 +======= describe "#diff_sha_refs" do context "with diffs" do subject { create(:merge_request, :with_diffs) } @@ -738,6 +758,7 @@ describe MergeRequest, models: true do ) expect(subject.diff_sha_refs).to eq(expected_diff_refs) +>>>>>>> master 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..8f1e5ac9891 --- /dev/null +++ b/spec/requests/api/merge_request_diffs_spec.rb @@ -0,0 +1,49 @@ +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 + + 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 |
