diff options
author | Robert Speicher <robert@gitlab.com> | 2016-02-19 21:39:31 +0300 |
---|---|---|
committer | Robert Speicher <robert@gitlab.com> | 2016-02-19 21:39:31 +0300 |
commit | a5c7aea3a95e14fd02ceea9b1ff6678b3ce37e68 (patch) | |
tree | 3d830cea95dbd23815f77d709f02815bb6bf130f | |
parent | 803d6f8f050b3cef1c8f01439385ac736977bddb (diff) | |
parent | cc5ff3b5a4d63dd079e02029be36646261770f66 (diff) |
Merge branch 'issue_3409' into 'master'
Add ability to revert changes introduced by Merge Requests or Commits
Closes #3409
See merge request !1990
38 files changed, 629 insertions, 36 deletions
diff --git a/CHANGELOG b/CHANGELOG index d7e7795dcb1..b4578592600 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -70,6 +70,7 @@ v 8.5.0 (unreleased) all Omniauth providers to do so. - Allow existing users to auto link their SAML credentials by logging in via SAML - Make it possible to erase a build (trace, artifacts) using UI and API + - Ability to revert changes from a Merge Request or Commit v 8.4.4 - Update omniauth-saml gem to 1.4.2 @@ -50,7 +50,7 @@ gem "browser", '~> 1.0.0' # Extracting information from a git repository # Provide access to Gitlab::Git library -gem "gitlab_git", '~> 8.1' +gem "gitlab_git", '~> 8.2' # LDAP Auth # GitLab fork with several improvements to original library. For full list of changes diff --git a/Gemfile.lock b/Gemfile.lock index 10750805ca4..4681adf2bd0 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -340,7 +340,7 @@ GEM json get_process_mem (0.2.0) gherkin-ruby (0.3.2) - github-linguist (4.7.3) + github-linguist (4.7.5) charlock_holmes (~> 0.7.3) escape_utils (~> 1.1.0) mime-types (>= 1.19) @@ -357,11 +357,11 @@ GEM posix-spawn (~> 0.3) gitlab_emoji (0.3.1) gemojione (~> 2.2, >= 2.2.1) - gitlab_git (8.1.0) + gitlab_git (8.2.0) activesupport (~> 4.0) charlock_holmes (~> 0.7.3) github-linguist (~> 4.7.0) - rugged (~> 0.23.3) + rugged (~> 0.24.0b13) gitlab_meta (7.0) gitlab_omniauth-ldap (1.2.1) net-ldap (~> 0.9) @@ -700,7 +700,7 @@ GEM rubyntlm (0.5.2) rubypants (0.2.0) rufus-scheduler (3.1.10) - rugged (0.23.3) + rugged (0.24.0b13) safe_yaml (1.0.4) sanitize (2.1.0) nokogiri (>= 1.4.4) @@ -932,7 +932,7 @@ DEPENDENCIES github-markup (~> 1.3.1) gitlab-flowdock-git-hook (~> 1.0.1) gitlab_emoji (~> 0.3.0) - gitlab_git (~> 8.1) + gitlab_git (~> 8.2) gitlab_meta (= 7.0) gitlab_omniauth-ldap (~> 1.2.1) gollum-lib (~> 4.1.0) diff --git a/app/assets/stylesheets/pages/merge_requests.scss b/app/assets/stylesheets/pages/merge_requests.scss index 6b497cd56ed..12fb030760c 100644 --- a/app/assets/stylesheets/pages/merge_requests.scss +++ b/app/assets/stylesheets/pages/merge_requests.scss @@ -237,3 +237,5 @@ } } } + + diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 2c329b60a19..fb74919ea23 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -25,7 +25,7 @@ class ApplicationController < ActionController::Base helper_method :abilities, :can?, :current_application_settings helper_method :import_sources_enabled?, :github_import_enabled?, :github_import_configured?, :gitlab_import_enabled?, :gitlab_import_configured?, :bitbucket_import_enabled?, :bitbucket_import_configured?, :gitorious_import_enabled?, :google_code_import_enabled?, :fogbugz_import_enabled?, :git_import_enabled? - helper_method :repository + helper_method :repository, :can_collaborate_with_project? rescue_from Encoding::CompatibilityError do |exception| log_exception(exception) @@ -410,6 +410,13 @@ class ApplicationController < ActionController::Base current_user.nil? && root_path == request.path end + def can_collaborate_with_project?(project = nil) + project ||= @project + + can?(current_user, :push_code, project) || + (current_user && current_user.already_forked?(project)) + end + private def set_default_sort diff --git a/app/controllers/concerns/creates_commit.rb b/app/controllers/concerns/creates_commit.rb index b9eb0a22f88..787416c17ab 100644 --- a/app/controllers/concerns/creates_commit.rb +++ b/app/controllers/concerns/creates_commit.rb @@ -13,17 +13,11 @@ module CreatesCommit result = service.new(@tree_edit_project, current_user, commit_params).execute if result[:status] == :success - flash[:notice] = success_notice || "Your changes have been successfully committed." - - if create_merge_request? - success_path = new_merge_request_path - target = different_project? ? "project" : "branch" - flash[:notice] << " You can now submit a merge request to get this change into the original #{target}." - end + update_flash_notice(success_notice) respond_to do |format| - format.html { redirect_to success_path } - format.json { render json: { message: "success", filePath: success_path } } + format.html { redirect_to final_success_path(success_path) } + format.json { render json: { message: "success", filePath: final_success_path(success_path) } } end else flash[:alert] = result[:message] @@ -41,14 +35,32 @@ module CreatesCommit end def authorize_edit_tree! - return if can?(current_user, :push_code, project) - return if current_user && current_user.already_forked?(project) + return if can_collaborate_with_project? access_denied! end private + def update_flash_notice(success_notice) + flash[:notice] = success_notice || "Your changes have been successfully committed." + + if create_merge_request? + if merge_request_exists? + flash[:notice] = nil + else + target = different_project? ? "project" : "branch" + flash[:notice] << " You can now submit a merge request to get this change into the original #{target}." + end + end + end + + def final_success_path(success_path) + return success_path unless create_merge_request? + + merge_request_exists? ? existing_merge_request_path : new_merge_request_path + end + def new_merge_request_path new_namespace_project_merge_request_path( @mr_source_project.namespace, @@ -62,6 +74,19 @@ module CreatesCommit ) end + def existing_merge_request_path + namespace_project_merge_request_path(@mr_target_project.namespace, @mr_target_project, @merge_request) + end + + def merge_request_exists? + return @merge_request if defined?(@merge_request) + + @merge_request = @mr_target_project.merge_requests.opened.find_by( + source_branch: @mr_source_branch, + target_branch: @mr_target_branch + ) + end + def different_project? @mr_source_project != @mr_target_project end @@ -75,7 +100,7 @@ module CreatesCommit end def set_commit_variables - @mr_source_branch = @target_branch + @mr_source_branch ||= @target_branch if can?(current_user, :push_code, @project) # Edit file in this project @@ -89,7 +114,7 @@ module CreatesCommit else # Merge request to this project @mr_target_project = @project - @mr_target_branch = @ref + @mr_target_branch ||= @ref end else # Edit file in fork @@ -97,7 +122,7 @@ module CreatesCommit # Merge request from fork to this project @mr_source_project = @tree_edit_project @mr_target_project = @project - @mr_target_branch = @ref + @mr_target_branch ||= @ref end end end diff --git a/app/controllers/projects/commit_controller.rb b/app/controllers/projects/commit_controller.rb index 36951b91372..97d31a4229a 100644 --- a/app/controllers/projects/commit_controller.rb +++ b/app/controllers/projects/commit_controller.rb @@ -2,6 +2,8 @@ # # Not to be confused with CommitsController, plural. class Projects::CommitController < Projects::ApplicationController + include CreatesCommit + # Authorize before_action :require_non_empty_project before_action :authorize_download_code!, except: [:cancel_builds, :retry_builds] @@ -9,6 +11,7 @@ class Projects::CommitController < Projects::ApplicationController before_action :authorize_read_commit_status!, only: [:builds] before_action :commit before_action :define_show_vars, only: [:show, :builds] + before_action :authorize_edit_tree!, only: [:revert] def show apply_diff_view_cookie! @@ -55,8 +58,37 @@ class Projects::CommitController < Projects::ApplicationController render layout: false end + def revert + assign_revert_commit_vars + + return render_404 if @target_branch.blank? + + create_commit(Commits::RevertService, success_notice: "The #{revert_type_title} has been successfully reverted.", + success_path: successful_revert_path, failure_path: failed_revert_path) + end + private + def revert_type_title + @commit.merged_merge_request ? 'merge request' : 'commit' + end + + def successful_revert_path + return referenced_merge_request_url if @commit.merged_merge_request + + namespace_project_commits_url(@project.namespace, @project, @target_branch) + end + + def failed_revert_path + return referenced_merge_request_url if @commit.merged_merge_request + + namespace_project_commit_url(@project.namespace, @project, params[:id]) + end + + def referenced_merge_request_url + namespace_project_merge_request_url(@project.namespace, @project, @commit.merged_merge_request) + end + def commit @commit ||= @project.commit(params[:id]) end @@ -79,4 +111,16 @@ class Projects::CommitController < Projects::ApplicationController @statuses = ci_commit.statuses if ci_commit end + + def assign_revert_commit_vars + @commit = project.commit(params[:id]) + @target_branch = params[:target_branch] + @mr_source_branch = @commit.revert_branch_name + @mr_target_branch = @target_branch + @commit_params = { + commit: @commit, + revert_type_title: revert_type_title, + create_merge_request: params[:create_merge_request].present? || different_project? + } + end end diff --git a/app/helpers/commits_helper.rb b/app/helpers/commits_helper.rb index 1d14ee52cfc..7ff539118d3 100644 --- a/app/helpers/commits_helper.rb +++ b/app/helpers/commits_helper.rb @@ -123,6 +123,37 @@ module CommitsHelper ) end + def revert_commit_link(commit, continue_to_path, btn_class: nil) + return unless current_user + + tooltip = "Revert this #{revert_commit_type(commit)} in a new merge request" + + if can_collaborate_with_project? + content_tag :span, 'data-toggle' => 'modal', 'data-target' => '#modal-revert-commit' do + link_to 'Revert', '#modal-revert-commit', 'data-toggle' => 'tooltip', title: tooltip, class: "btn btn-default btn-grouped btn-#{btn_class}" + end + elsif can?(current_user, :fork_project, @project) + continue_params = { + to: continue_to_path, + notice: edit_in_new_fork_notice + ' Try to revert this commit again.', + notice_now: edit_in_new_fork_notice_now + } + fork_path = namespace_project_forks_path(@project.namespace, @project, + namespace_key: current_user.namespace.id, + continue: continue_params) + + link_to 'Revert', fork_path, class: 'btn btn-grouped btn-close', method: :post, 'data-toggle' => 'tooltip', title: tooltip + end + end + + def revert_commit_type(commit) + if commit.merged_merge_request + 'merge request' + else + 'commit' + end + end + protected # Private: Returns a link to a person. If the person has a matching user and diff --git a/app/helpers/tree_helper.rb b/app/helpers/tree_helper.rb index 2ad7c80dae0..4920ca5af6e 100644 --- a/app/helpers/tree_helper.rb +++ b/app/helpers/tree_helper.rb @@ -56,8 +56,7 @@ module TreeHelper return false unless on_top_of_branch?(project, ref) - can?(current_user, :push_code, project) || - (current_user && current_user.already_forked?(project)) + can_collaborate_with_project?(project) end def tree_edit_branch(project = @project, ref = @ref) diff --git a/app/models/commit.rb b/app/models/commit.rb index 23b771aebb7..b99abb540ea 100644 --- a/app/models/commit.rb +++ b/app/models/commit.rb @@ -215,6 +215,44 @@ class Commit ci_commit.try(:status) || :not_found end + def revert_branch_name + "revert-#{short_id}" + end + + def revert_description + if merged_merge_request + "This reverts merge request #{merged_merge_request.to_reference}" + else + "This reverts commit #{sha}" + end + end + + def revert_message + %Q{Revert "#{title}"\n\n#{revert_description}} + end + + def reverts_commit?(commit) + description.include?(commit.revert_description) + end + + def merge_commit? + parents.size > 1 + end + + def merged_merge_request + return @merged_merge_request if defined?(@merged_merge_request) + + @merged_merge_request = project.merge_requests.find_by(merge_commit_sha: id) if merge_commit? + end + + def has_been_reverted?(current_user = nil, noteable = self) + Gitlab::ReferenceExtractor.lazily do + noteable.notes.system.flat_map do |note| + note.all_references(current_user).commits + end + end.any? { |commit_ref| commit_ref.reverts_commit?(self) } + end + private def repo_changes diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index ea2b0e075f6..1543ef311d7 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -24,6 +24,7 @@ # merge_params :text # merge_when_build_succeeds :boolean default(FALSE), not null # merge_user_id :integer +# merge_commit_sha :string # require Rails.root.join("app/models/commit") @@ -532,4 +533,12 @@ class MergeRequest < ActiveRecord::Base [diff_base_commit, last_commit] end + + def merge_commit + @merge_commit ||= project.commit(merge_commit_sha) if merge_commit_sha + end + + def can_be_reverted?(current_user = nil) + merge_commit && !merge_commit.has_been_reverted?(current_user, self) + end end diff --git a/app/models/repository.rb b/app/models/repository.rb index 73aa67d46ec..be30a3b0906 100644 --- a/app/models/repository.rb +++ b/app/models/repository.rb @@ -619,6 +619,34 @@ class Repository end end + def revert(user, commit, base_branch, target_branch = nil) + source_sha = find_branch(base_branch).target + target_branch ||= base_branch + args = [commit.id, source_sha] + args << { mainline: 1 } if commit.merge_commit? + + revert_index = rugged.revert_commit(*args) + return false if revert_index.conflicts? + + tree_id = revert_index.write_tree(rugged) + return false unless diff_exists?(source_sha, tree_id) + + commit_with_hooks(user, target_branch) do |ref| + committer = user_to_committer(user) + source_sha = Rugged::Commit.create(rugged, + message: commit.revert_message, + author: committer, + committer: committer, + tree: tree_id, + parents: [rugged.lookup(source_sha)], + update_ref: ref) + end + end + + def diff_exists?(sha1, sha2) + rugged.diff(sha1, sha2).size > 0 + end + def merged_to_root_ref?(branch_name) branch_commit = commit(branch_name) root_ref_commit = commit(root_ref) @@ -700,10 +728,11 @@ class Repository oldrev = Gitlab::Git::BLANK_SHA ref = Gitlab::Git::BRANCH_REF_PREFIX + branch + target_branch = find_branch(branch) was_empty = empty? - unless was_empty - oldrev = find_branch(branch).target + if !was_empty && target_branch + oldrev = target_branch.target end with_tmp_ref(oldrev) do |tmp_ref| @@ -715,7 +744,7 @@ class Repository end GitHooksService.new.execute(current_user, path_to_repo, oldrev, newrev, ref) do - if was_empty + if was_empty || !target_branch # Create branch rugged.references.create(ref, newrev) else @@ -730,6 +759,8 @@ class Repository end end end + + newrev end end diff --git a/app/services/commits/revert_service.rb b/app/services/commits/revert_service.rb new file mode 100644 index 00000000000..43d1c766e35 --- /dev/null +++ b/app/services/commits/revert_service.rb @@ -0,0 +1,58 @@ +module Commits + class RevertService < ::BaseService + class ValidationError < StandardError; end + class ReversionError < StandardError; end + + def execute + @source_project = params[:source_project] || @project + @target_branch = params[:target_branch] + @commit = params[:commit] + @create_merge_request = params[:create_merge_request].present? + + validate and commit + rescue Repository::CommitError, Gitlab::Git::Repository::InvalidBlobName, GitHooksService::PreReceiveError, + ValidationError, ReversionError => ex + error(ex.message) + end + + def commit + revert_into = @create_merge_request ? @commit.revert_branch_name : @target_branch + + if @create_merge_request + # Temporary branch exists and contains the revert commit + return success if repository.find_branch(revert_into) + + create_target_branch + end + + unless repository.revert(current_user, @commit, revert_into) + error_msg = "Sorry, we cannot revert this #{params[:revert_type_title]} automatically. + It may have already been reverted, or a more recent commit may have updated some of its content." + raise ReversionError, error_msg + end + + success + end + + private + + def create_target_branch + result = CreateBranchService.new(@project, current_user) + .execute(@commit.revert_branch_name, @target_branch, source_project: @source_project) + + if result[:status] == :error + raise ReversionError, "There was an error creating the source branch: #{result[:message]}" + end + end + + def validate + allowed = ::Gitlab::GitAccess.new(current_user, project).can_push_to_branch?(@target_branch) + + unless allowed + raise_error('You are not allowed to push into this branch') + end + + true + end + end +end diff --git a/app/services/merge_requests/build_service.rb b/app/services/merge_requests/build_service.rb index a9b29f9654d..c0700d953dd 100644 --- a/app/services/merge_requests/build_service.rb +++ b/app/services/merge_requests/build_service.rb @@ -56,7 +56,7 @@ module MergeRequests if commits && commits.count == 1 commit = commits.first merge_request.title = commit.title - merge_request.description = commit.description.try(:strip) + merge_request.description ||= commit.description.try(:strip) else merge_request.title = merge_request.source_branch.titleize.humanize end diff --git a/app/services/merge_requests/merge_service.rb b/app/services/merge_requests/merge_service.rb index e8bef250d8b..9a58383b398 100644 --- a/app/services/merge_requests/merge_service.rb +++ b/app/services/merge_requests/merge_service.rb @@ -34,7 +34,8 @@ module MergeRequests committer: committer } - repository.merge(current_user, merge_request.source_sha, merge_request.target_branch, options) + commit_id = repository.merge(current_user, merge_request.source_sha, merge_request.target_branch, options) + merge_request.update(merge_commit_sha: commit_id) rescue StandardError => e merge_request.update(merge_error: "Something went wrong during merge") Rails.logger.error(e.message) diff --git a/app/views/projects/commit/_commit_box.html.haml b/app/views/projects/commit/_commit_box.html.haml index bbe820b8842..71995fcc487 100644 --- a/app/views/projects/commit/_commit_box.html.haml +++ b/app/views/projects/commit/_commit_box.html.haml @@ -16,6 +16,8 @@ = link_to namespace_project_tree_path(@project.namespace, @project, @commit), class: "btn btn-grouped" do = icon('files-o') Browse Files + - unless @commit.has_been_reverted?(current_user) + = revert_commit_link(@commit, namespace_project_commit_path(@project.namespace, @project, @commit.id)) %div %p diff --git a/app/views/projects/commit/_revert.html.haml b/app/views/projects/commit/_revert.html.haml new file mode 100644 index 00000000000..52ca3ed5b14 --- /dev/null +++ b/app/views/projects/commit/_revert.html.haml @@ -0,0 +1,31 @@ +#modal-revert-commit.modal + .modal-dialog + .modal-content + .modal-header + %a.close{href: "#", "data-dismiss" => "modal"} × + %h3.page-title== Revert this #{revert_commit_type(commit)} + .modal-body + = form_tag revert_namespace_project_commit_path(@project.namespace, @project, commit.id), method: :post, remote: false, class: 'form-horizontal js-create-dir-form js-requires-input' do + .form-group.branch + = label_tag 'target_branch', 'Revert in branch', class: 'control-label' + .col-sm-10 + = select_tag "target_branch", grouped_options_refs, class: "select2 select2-sm js-target-branch" + - if can?(current_user, :push_code, @project) + .js-create-merge-request-container + .checkbox + - nonce = SecureRandom.hex + = label_tag "create_merge_request-#{nonce}" do + = check_box_tag 'create_merge_request', 1, true, class: 'js-create-merge-request', id: "create_merge_request-#{nonce}" + Start a <strong>new merge request</strong> with these changes + - else + = hidden_field_tag 'create_merge_request', 1 + .form-actions + = submit_tag "Revert", class: 'btn btn-create' + = link_to "Cancel", '#', class: "btn btn-cancel", "data-dismiss" => "modal" + + - unless can?(current_user, :push_code, @project) + .inline.prepend-left-10 + = commit_in_fork_help + +:javascript + new NewCommitForm($('.js-create-dir-form')) diff --git a/app/views/projects/commit/show.html.haml b/app/views/projects/commit/show.html.haml index 05dbe5ebea4..21e186120c3 100644 --- a/app/views/projects/commit/show.html.haml +++ b/app/views/projects/commit/show.html.haml @@ -12,3 +12,5 @@ = render "projects/diffs/diffs", diffs: @diffs, project: @project, diff_refs: @diff_refs = render "projects/notes/notes_with_form" +- if can_collaborate_with_project? + = render "projects/commit/revert", commit: @commit, title: @commit.title diff --git a/app/views/projects/merge_requests/_show.html.haml b/app/views/projects/merge_requests/_show.html.haml index da67645bc2b..648512e5379 100644 --- a/app/views/projects/merge_requests/_show.html.haml +++ b/app/views/projects/merge_requests/_show.html.haml @@ -85,6 +85,8 @@ = spinner = render 'shared/issuable/sidebar', issuable: @merge_request +- if @merge_request.can_be_reverted? + = render "projects/commit/revert", commit: @merge_request.merge_commit, title: @merge_request.title :javascript var merge_request; diff --git a/app/views/projects/merge_requests/widget/_merged.html.haml b/app/views/projects/merge_requests/widget/_merged.html.haml index d1d602eecdc..3abae9f0bf6 100644 --- a/app/views/projects/merge_requests/widget/_merged.html.haml +++ b/app/views/projects/merge_requests/widget/_merged.html.haml @@ -8,20 +8,18 @@ #{time_ago_with_tooltip(@merge_request.merge_event.created_at)} %div - if !@merge_request.source_branch_exists? || (params[:delete_source] == 'true') - The changes were merged into - #{link_to @merge_request.target_branch, namespace_project_commits_path(@project.namespace, @project, @merge_request.target_branch), class: "label-branch"}. - The source branch has been removed. - + %p + The changes were merged into + #{link_to @merge_request.target_branch, namespace_project_commits_path(@project.namespace, @project, @merge_request.target_branch), class: "label-branch"}. + The source branch has been removed. + = render 'projects/merge_requests/widget/merged_buttons' - elsif @merge_request.can_remove_source_branch?(current_user) .remove_source_branch_widget %p The changes were merged into #{link_to @merge_request.target_branch, namespace_project_commits_path(@project.namespace, @project, @merge_request.target_branch), class: "label-branch"}. You can remove the source branch now. - = link_to namespace_project_branch_path(@merge_request.source_project.namespace, @merge_request.source_project, @merge_request.source_branch), remote: true, method: :delete, class: "btn btn-primary btn-sm remove_source_branch" do - %i.fa.fa-times - Remove Source Branch - + = render 'projects/merge_requests/widget/merged_buttons', source_branch_exists: true .remove_source_branch_widget.failed.hide %p Failed to remove source branch '#{@merge_request.source_branch}'. diff --git a/app/views/projects/merge_requests/widget/_merged_buttons.haml b/app/views/projects/merge_requests/widget/_merged_buttons.haml new file mode 100644 index 00000000000..85a3a6ba9e2 --- /dev/null +++ b/app/views/projects/merge_requests/widget/_merged_buttons.haml @@ -0,0 +1,11 @@ +- source_branch_exists = local_assigns.fetch(:source_branch_exists, false) +- mr_can_be_reverted = @merge_request.can_be_reverted? + +- if source_branch_exists || mr_can_be_reverted + .btn-group + - if source_branch_exists + = link_to namespace_project_branch_path(@merge_request.source_project.namespace, @merge_request.source_project, @merge_request.source_branch), remote: true, method: :delete, class: "btn btn-default btn-grouped btn-sm remove_source_branch" do + = icon('trash-o') + Remove Source Branch + - if mr_can_be_reverted + = revert_commit_link(@merge_request.merge_commit, namespace_project_merge_request_path(@project.namespace, @project, @merge_request), btn_class: 'sm') diff --git a/config/routes.rb b/config/routes.rb index c6541a8979c..0ed3f1731f8 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -502,6 +502,7 @@ Rails.application.routes.draw do get :builds post :cancel_builds post :retry_builds + post :revert end end diff --git a/db/migrate/20160129155512_add_merge_commit_sha_to_merge_requests.rb b/db/migrate/20160129155512_add_merge_commit_sha_to_merge_requests.rb new file mode 100644 index 00000000000..f0d94226514 --- /dev/null +++ b/db/migrate/20160129155512_add_merge_commit_sha_to_merge_requests.rb @@ -0,0 +1,5 @@ +class AddMergeCommitShaToMergeRequests < ActiveRecord::Migration + def change + add_column :merge_requests, :merge_commit_sha, :string + end +end diff --git a/db/schema.rb b/db/schema.rb index 2d1c6dbf0e4..344c384b6d0 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -525,6 +525,7 @@ ActiveRecord::Schema.define(version: 20160209130428) do t.text "merge_params" t.boolean "merge_when_build_succeeds", default: false, null: false t.integer "merge_user_id" + t.string "merge_commit_sha" end add_index "merge_requests", ["assignee_id"], name: "index_merge_requests_on_assignee_id", using: :btree diff --git a/doc/workflow/README.md b/doc/workflow/README.md index bf62ab41053..5199ff9390a 100644 --- a/doc/workflow/README.md +++ b/doc/workflow/README.md @@ -17,6 +17,7 @@ - [Releases](releases.md) - [Milestones](milestones.md) - [Merge Requests](merge_requests.md) +- [Revert changes](revert_changes.md) - ["Work In Progress" Merge Requests](wip_merge_requests.md) - [Merge When Build Succeeds](merge_when_build_succeeds.md) - [Manage large binaries with Git LFS](lfs/manage_large_binaries_with_git_lfs.md) diff --git a/doc/workflow/img/revert_changes_commit.png b/doc/workflow/img/revert_changes_commit.png Binary files differnew file mode 100644 index 00000000000..d84211e20db --- /dev/null +++ b/doc/workflow/img/revert_changes_commit.png diff --git a/doc/workflow/img/revert_changes_commit_modal.png b/doc/workflow/img/revert_changes_commit_modal.png Binary files differnew file mode 100644 index 00000000000..e94d151a2af --- /dev/null +++ b/doc/workflow/img/revert_changes_commit_modal.png diff --git a/doc/workflow/img/revert_changes_mr.png b/doc/workflow/img/revert_changes_mr.png Binary files differnew file mode 100644 index 00000000000..7adad88463b --- /dev/null +++ b/doc/workflow/img/revert_changes_mr.png diff --git a/doc/workflow/img/revert_changes_mr_modal.png b/doc/workflow/img/revert_changes_mr_modal.png Binary files differnew file mode 100644 index 00000000000..9da78f84828 --- /dev/null +++ b/doc/workflow/img/revert_changes_mr_modal.png diff --git a/doc/workflow/revert_changes.md b/doc/workflow/revert_changes.md new file mode 100644 index 00000000000..399366b0cdc --- /dev/null +++ b/doc/workflow/revert_changes.md @@ -0,0 +1,64 @@ +# Reverting changes + +_**Note:** This feature was [introduced][ce-1990] in GitLab 8.5._ + +--- + +GitLab implements Git's powerful feature to [revert any commit][git-revert] +with introducing a **Revert** button in Merge Requests and commit details. + +## Reverting a Merge Request + +_**Note:** The **Revert** button will only be available for Merge Requests +created since GitLab 8.5. However, you can still revert a Merge Request +by reverting the merge commit from the list of Commits page._ + +After the Merge Request has been merged, a **Revert** button will be available +to revert the changes introduced by that Merge Request: + +![Revert Merge Request](img/revert_changes_mr.png) + +--- + +You can revert the changes directly into the selected branch or you can opt to +create a new Merge Request with the revert changes: + +![Revert Merge Request modal](img/revert_changes_mr_modal.png) + +--- + +After the Merge Request has been reverted, the **Revert** button will not be +available anymore. + +## Reverting a Commit + +You can revert a Commit from the Commit details page: + +![Revert commit](img/revert_changes_commit.png) + +--- + +Similar to reverting a Merge Request, you can opt to revert the changes +directly into the target branch or create a new Merge Request to revert the +changes: + +![Revert commit modal](img/revert_changes_commit_modal.png) + +--- + +After the Commit has been reverted, the **Revert** button will not be available +anymore. + +Please note that when reverting merge commits, the mainline will always be the +first parent. If you want to use a different mainline then you need to do that +from the command line. + +Here is a quick example to revert a merge commit using the second parent as the +mainline: + +```bash +git revert -m 2 7a39eb0 +``` + +[ce-1990]: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/1990 "Revert button Merge Request" +[git-revert]: https://git-scm.com/docs/git-revert "Git revert documentation" diff --git a/features/project/commits/revert.feature b/features/project/commits/revert.feature new file mode 100644 index 00000000000..7a2effafe03 --- /dev/null +++ b/features/project/commits/revert.feature @@ -0,0 +1,28 @@ +@project_commits +Feature: Revert Commits + Background: + Given I sign in as a user + And I own a project + And I visit my project's commits page + + Scenario: I revert a commit + Given I click on commit link + And I click on the revert button + And I revert the changes directly + Then I should see the revert commit notice + + Scenario: I revert a commit that was previously reverted + Given I click on commit link + And I click on the revert button + And I revert the changes directly + And I visit my project's commits page + And I click on commit link + And I click on the revert button + And I revert the changes directly + Then I should see a revert error + + Scenario: I revert a commit in a new merge request + Given I click on commit link + And I click on the revert button + And I revert the changes in a new merge request + Then I should see the new merge request notice diff --git a/features/project/merge_requests/revert.feature b/features/project/merge_requests/revert.feature new file mode 100644 index 00000000000..d767b088883 --- /dev/null +++ b/features/project/merge_requests/revert.feature @@ -0,0 +1,30 @@ +@project_merge_requests +Feature: Revert Merge Requests + Background: + Given There is an open Merge Request + And I am signed in as a developer of the project + And I am on the Merge Request detail page + And I click on Accept Merge Request + + @javascript + Scenario: I revert a merge request + Given I click on the revert button + And I revert the changes directly + Then I should see the revert merge request notice + + @javascript + Scenario: I revert a merge request that was previously reverted + Given I click on the revert button + And I revert the changes directly + And I am on the Merge Request detail page + And I click on the revert button + And I revert the changes directly + Then I should see a revert error + + @javascript + Scenario: I revert a merge request in a new merge request + Given I click on the revert button + And I am on the Merge Request detail page + And I click on the revert button + And I revert the changes in a new merge request + Then I should see the new merge request notice diff --git a/features/steps/project/commits/revert.rb b/features/steps/project/commits/revert.rb new file mode 100644 index 00000000000..94a5d4e2e4d --- /dev/null +++ b/features/steps/project/commits/revert.rb @@ -0,0 +1,40 @@ +class Spinach::Features::RevertCommits < Spinach::FeatureSteps + include SharedAuthentication + include SharedProject + include SharedPaths + include SharedDiffNote + include RepoHelpers + + step 'I click on commit link' do + visit namespace_project_commit_path(@project.namespace, @project, sample_commit.id) + end + + step 'I click on the revert button' do + find("a[href='#modal-revert-commit']").click + end + + step 'I revert the changes directly' do + page.within('#modal-revert-commit') do + uncheck 'create_merge_request' + click_button 'Revert' + end + end + + step 'I should see the revert commit notice' do + page.should have_content('The commit has been successfully reverted.') + end + + step 'I should see a revert error' do + page.should have_content('Sorry, we cannot revert this commit automatically.') + end + + step 'I revert the changes in a new merge request' do + page.within('#modal-revert-commit') do + click_button 'Revert' + end + end + + step 'I should see the new merge request notice' do + page.should have_content('The commit has been successfully reverted. You can now submit a merge request to get this change into the original branch.') + end +end diff --git a/features/steps/project/merge_requests/revert.rb b/features/steps/project/merge_requests/revert.rb new file mode 100644 index 00000000000..c5a4cfce6f0 --- /dev/null +++ b/features/steps/project/merge_requests/revert.rb @@ -0,0 +1,56 @@ +class Spinach::Features::RevertMergeRequests < Spinach::FeatureSteps + include LoginHelpers + include GitlabRoutingHelper + + step 'I click on the revert button' do + find("a[href='#modal-revert-commit']").click + end + + step 'I revert the changes directly' do + page.within('#modal-revert-commit') do + uncheck 'create_merge_request' + click_button 'Revert' + end + end + + step 'I should see the revert merge request notice' do + page.should have_content('The merge request has been successfully reverted.') + end + + step 'I should not see the revert button' do + expect(page).not_to have_selector(:xpath, "a[href='#modal-revert-commit']") + end + + step 'I am on the Merge Request detail page' do + visit merge_request_path(@merge_request) + end + + step 'I click on Accept Merge Request' do + click_button('Accept Merge Request') + end + + step 'I am signed in as a developer of the project' do + login_as(@user) + end + + step 'There is an open Merge Request' do + @user = create(:user) + @project = create(:project, :public) + @project_member = create(:project_member, user: @user, project: @project, access_level: ProjectMember::DEVELOPER) + @merge_request = create(:merge_request, :with_diffs, :simple, source_project: @project) + end + + step 'I should see a revert error' do + page.should have_content('Sorry, we cannot revert this merge request automatically.') + end + + step 'I revert the changes in a new merge request' do + page.within('#modal-revert-commit') do + click_button 'Revert' + end + end + + step 'I should see the new merge request notice' do + page.should have_content('The merge request has been successfully reverted. You can now submit a merge request to get this change into the original branch.') + end +end diff --git a/spec/controllers/commit_controller_spec.rb b/spec/controllers/commit_controller_spec.rb index 7793bf1e421..bbe400dad88 100644 --- a/spec/controllers/commit_controller_spec.rb +++ b/spec/controllers/commit_controller_spec.rb @@ -143,4 +143,53 @@ describe Projects::CommitController do expect(assigns(:tags)).to include("v1.1.0") end end + + describe '#revert' do + context 'when target branch is not provided' do + it 'should render the 404 page' do + post(:revert, + namespace_id: project.namespace.to_param, + project_id: project.to_param, + id: commit.id) + + expect(response).not_to be_success + expect(response.status).to eq(404) + end + end + + context 'when the revert was successful' do + it 'should redirect to the commits page' do + post(:revert, + namespace_id: project.namespace.to_param, + project_id: project.to_param, + target_branch: 'master', + id: commit.id) + + expect(response).to redirect_to namespace_project_commits_path(project.namespace, project, 'master') + expect(flash[:notice]).to eq('The commit has been successfully reverted.') + end + end + + context 'when the revert failed' do + before do + post(:revert, + namespace_id: project.namespace.to_param, + project_id: project.to_param, + target_branch: 'master', + id: commit.id) + end + + it 'should redirect to the commit page' do + # Reverting a commit that has been already reverted. + post(:revert, + namespace_id: project.namespace.to_param, + project_id: project.to_param, + target_branch: 'master', + id: commit.id) + + expect(response).to redirect_to namespace_project_commit_path(project.namespace, project, commit.id) + expect(flash[:alert]).to match('Sorry, we cannot revert this commit automatically.') + end + end + end end diff --git a/spec/factories/merge_requests.rb b/spec/factories/merge_requests.rb index 0c6a881f868..00de7bb5294 100644 --- a/spec/factories/merge_requests.rb +++ b/spec/factories/merge_requests.rb @@ -24,6 +24,7 @@ # merge_params :text # merge_when_build_succeeds :boolean default(FALSE), not null # merge_user_id :integer +# merge_commit_sha :string # FactoryGirl.define do diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb index f35b48601ad..c51f34034d7 100644 --- a/spec/models/merge_request_spec.rb +++ b/spec/models/merge_request_spec.rb @@ -24,6 +24,7 @@ # merge_params :text # merge_when_build_succeeds :boolean default(FALSE), not null # merge_user_id :integer +# merge_commit_sha :string # require 'spec_helper' diff --git a/spec/models/repository_spec.rb b/spec/models/repository_spec.rb index ed91b62c534..9242f755449 100644 --- a/spec/models/repository_spec.rb +++ b/spec/models/repository_spec.rb @@ -5,6 +5,15 @@ describe Repository, models: true do let(:repository) { create(:project).repository } let(:user) { create(:user) } + let(:commit_options) do + author = repository.user_to_committer(user) + { message: 'Test message', committer: author, author: author } + end + let(:merge_commit) do + source_sha = repository.find_branch('feature').target + merge_commit_id = repository.merge(user, source_sha, 'master', commit_options) + repository.commit(merge_commit_id) + end describe :branch_names_contains do subject { repository.branch_names_contains(sample_commit.id) } @@ -426,4 +435,19 @@ describe Repository, models: true do it { is_expected.not_to include('e56497bb5f03a90a51293fc6d516788730953899') } end + + describe '#merge' do + it 'should merge the code and return the commit id' do + expect(merge_commit).to be_present + expect(repository.blob_at(merge_commit.id, 'files/ruby/feature.rb')).to be_present + end + end + + describe '#revert_merge' do + it 'should revert the changes' do + repository.revert(user, merge_commit, 'master') + + expect(repository.blob_at_branch('master', 'files/ruby/feature.rb')).not_to be_present + end + end end |