diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb index bdd614973b7..7f9c238507e 100644 --- a/app/controllers/projects/merge_requests_controller.rb +++ b/app/controllers/projects/merge_requests_controller.rb @@ -60,53 +60,27 @@ class Projects::MergeRequestsController < Projects::ApplicationController end def new - params[:merge_request] ||= ActionController::Parameters.new( - source_project: @project - ) + params[:merge_request] ||= ActionController::Parameters.new(source_project: @project) + @merge_request = MergeRequests::BuildService.new(project, current_user, merge_request_params).execute - @merge_request = MergeRequest.new(merge_request_params) - @merge_request.source_project = @project unless @merge_request.source_project - @merge_request.target_project ||= (@project.forked_from_project || @project) - @target_branches = @merge_request.target_project.nil? ? [] : @merge_request.target_project.repository.branch_names - @merge_request.target_branch ||= @merge_request.target_project.default_branch - @source_project = @merge_request.source_project + @target_branches = if @merge_request.target_project + @merge_request.target_project.repository.branch_names + else + [] + end - if @merge_request.target_branch && @merge_request.source_branch - compare_action = Gitlab::Satellite::CompareAction.new( - current_user, - @merge_request.target_project, - @merge_request.target_branch, - @merge_request.source_project, - @merge_request.source_branch - ) + @target_project = merge_request.target_project + @source_project = merge_request.source_project + @commits = @merge_request.compare_commits + @commit = @merge_request.compare_base_commit + @diffs = @merge_request.compare_diffs + @note_counts = Note.where(commit_id: @commits.map(&:id)). + group(:commit_id).count - @compare_failed = false - @commits = compare_action.commits - - if @commits - @commits.map! { |commit| Commit.new(commit) } - @commit = @commits.first - else - # false value because failed to get commits from satellite - @commits = [] - @compare_failed = true - end - - @note_counts = Note.where(commit_id: @commits.map(&:id)). - group(:commit_id).count - - begin - @diffs = compare_action.diffs - @merge_request.title = @merge_request.source_branch.titleize.humanize - @target_project = @merge_request.target_project - @target_repo = @target_project.repository - - diff_line_count = Commit::diff_line_count(@diffs) - @suppress_diff = Commit::diff_suppress?(@diffs, diff_line_count) - @force_suppress_diff = @suppress_diff - rescue Gitlab::Satellite::BranchesWithoutParent - @error = "Selected branches have no common commit so they cannot be merged." - end + if @diffs.any? + @diff_line_count = Commit::diff_line_count(@diffs) + @suppress_diff = Commit::diff_suppress?(@diffs, @diff_line_count) + @force_suppress_diff = @suppress_diff end end diff --git a/app/helpers/projects_helper.rb b/app/helpers/projects_helper.rb index ba4c7068e90..bc2ec84302d 100644 --- a/app/helpers/projects_helper.rb +++ b/app/helpers/projects_helper.rb @@ -221,4 +221,10 @@ module ProjectsHelper "Never" end end + + def contribution_guide_url(project) + if project && project.repository.contribution_guide + project_blob_path(project, tree_join(project.default_branch, project.repository.contribution_guide.name)) + end + end end diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index 597e02d498b..530e1add8e7 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -42,6 +42,10 @@ class MergeRequest < ActiveRecord::Base # It allows us to close or modify broken merge requests attr_accessor :allow_broken + # Temporary fields to store compare vars + # when creating new merge request + attr_accessor :can_be_created, :compare_failed, :compare_base_commit, :compare_commits, :compare_diffs + ActsAsTaggableOn.strict_case_match = true acts_as_taggable_on :labels diff --git a/app/services/merge_requests/build_service.rb b/app/services/merge_requests/build_service.rb new file mode 100644 index 00000000000..9c28d3d82d8 --- /dev/null +++ b/app/services/merge_requests/build_service.rb @@ -0,0 +1,69 @@ +module MergeRequests + class BuildService < MergeRequests::BaseService + def execute + merge_request = MergeRequest.new(params) + + # Set MR attributes + merge_request.can_be_created = false + merge_request.compare_failed = false + merge_request.compare_commits = [] + merge_request.compare_diffs = [] + merge_request.source_project = project unless merge_request.source_project + merge_request.target_project ||= (project.forked_from_project || project) + merge_request.target_branch ||= merge_request.target_project.default_branch + + unless merge_request.target_branch && merge_request.source_branch + return build_failed(merge_request, "You must select source and target branches") + end + + # Generate suggested MR title based on source branch name + merge_request.title = merge_request.source_branch.titleize.humanize + + # Try to compare branches to get commits list and diffs + compare_action = Gitlab::Satellite::CompareAction.new( + current_user, + merge_request.target_project, + merge_request.target_branch, + merge_request.source_project, + merge_request.source_branch + ) + + commits = compare_action.commits + + # At this point we decide if merge request can be created + # If we have at least one commit to merge -> creation allowed + if commits.present? + merge_request.compare_commits = Commit.decorate(commits) + merge_request.compare_base_commit = Commit.new(commits.first) + merge_request.can_be_created = true + merge_request.compare_failed = false + + # Try to collect diff for merge request. + # Note: even if diff is huge and we can't show it - we still should allow + # people to create MR. + diffs = compare_action.diffs + + if diffs.present? + merge_request.compare_diffs = diffs + end + else + merge_request.can_be_created = false + merge_request.compare_failed = true + end + + merge_request + + rescue Gitlab::Satellite::BranchesWithoutParent + return build_failed(merge_request, "Selected branches have no common commit so they cannot be merged.") + #rescue + #return build_failed(merge_request, "We cannot create merge request because of huge diff.") + end + + def build_failed(merge_request, message) + merge_request.errors.add(:base, message) + merge_request.compare_commits = [] + merge_request.can_be_created = false + merge_request + end + end +end diff --git a/app/views/projects/merge_requests/_new_compare.html.haml b/app/views/projects/merge_requests/_new_compare.html.haml index 18e3f419c72..76b5db419f7 100644 --- a/app/views/projects/merge_requests/_new_compare.html.haml +++ b/app/views/projects/merge_requests/_new_compare.html.haml @@ -27,12 +27,12 @@ .panel-footer .mr_target_commit - -if @merge_request.errors.any? + - if @merge_request.errors.any? .alert.alert-danger - @merge_request.errors.full_messages.each do |msg| %div= msg - - if @merge_request.source_branch.present? && @merge_request.target_branch.present? + - elsif @merge_request.source_branch.present? && @merge_request.target_branch.present? - if @compare_failed .alert.alert-danger %h4 Compare failed diff --git a/app/views/projects/merge_requests/_new_submit.html.haml b/app/views/projects/merge_requests/_new_submit.html.haml index 6050f20d763..73d364b4f93 100644 --- a/app/views/projects/merge_requests/_new_submit.html.haml +++ b/app/views/projects/merge_requests/_new_submit.html.haml @@ -10,75 +10,75 @@ %span.pull-right = link_to 'Change branches', new_project_merge_request_path(@project) -- if @error.present? - .centered-light-block - %h4 #{@error} -- else - = form_for [@project, @merge_request], html: { class: "merge-request-form" } do |f| - .panel.panel-default - .panel-body - .form-group - .light - = f.label :title do - = "Title *" - = f.text_field :title, class: "form-control input-lg js-gfm-input", maxlength: 255, rows: 5, required: true - .form-group - .light - = f.label :description, "Description" - = f.text_area :description, class: "form-control js-gfm-input markdown-area", rows: 10 - .clearfix.hint - .pull-left Description is parsed with #{link_to "GitLab Flavored Markdown", help_page_path("markdown", "markdown"), target: '_blank'}. - .pull-right Attach images (JPG, PNG, GIF) by dragging & dropping or #{link_to "selecting them", '#', class: 'markdown-selector' }. - .error-alert - .form-group - .issue-assignee - = f.label :assignee_id do - %i.icon-user - Assign to - %div - = project_users_select_tag('merge_request[assignee_id]', placeholder: 'Select a user', class: 'custom-form-control', selected: @merge_request.assignee_id, project_id: @merge_request.target_project_id) -   - = link_to 'Assign to me', '#', class: 'btn assign-to-me-link' - .form-group - .issue-milestone - = f.label :milestone_id do - %i.icon-time - Milestone - %div= f.select(:milestone_id, milestone_options(@merge_request), { include_blank: "Select milestone" }, {class: 'select2'}) - .panel-footer - - if @target_repo.contribution_guide - - contribution_guide_url = project_blob_path(@target_project, tree_join(@target_repo.root_ref, @target_repo.contribution_guide.name)) - %p - Please review the - %strong #{link_to "guidelines for contribution", contribution_guide_url} - to this repository. - = f.hidden_field :source_project_id - = f.hidden_field :target_project_id - = f.hidden_field :target_branch - = f.hidden_field :source_branch - = f.submit 'Submit merge request', class: "btn btn-create" += form_for [@project, @merge_request], html: { class: "merge-request-form" } do |f| + .panel.panel-default - .mr-compare - %div.panel.panel-default - .panel-heading - Commits (#{@commits.count}) - - if @commits.size > MergeRequestDiff::COMMITS_SAFE_SIZE - %ul.well-list - - Commit.decorate(@commits.first(MergeRequestDiff::COMMITS_SAFE_SIZE)).each do |commit| - = render "projects/commits/inline_commit", commit: commit, project: @project - %li.warning-row.unstyled - other #{@commits.size - MergeRequestDiff::COMMITS_SAFE_SIZE} commits hidden to prevent performance issues. - - else - %ul.well-list= render Commit.decorate(@commits), project: @project + .panel-body + .form-group + .light + = f.label :title do + = "Title *" + = f.text_field :title, class: "form-control input-lg js-gfm-input", maxlength: 255, rows: 5, required: true + .form-group + .light + = f.label :description, "Description" + = f.text_area :description, class: "form-control js-gfm-input markdown-area", rows: 10 + .clearfix.hint + .pull-left Description is parsed with #{link_to "GitLab Flavored Markdown", help_page_path("markdown", "markdown"), target: '_blank'}. + .pull-right Attach images (JPG, PNG, GIF) by dragging & dropping or #{link_to "selecting them", '#', class: 'markdown-selector' }. + .error-alert + .form-group + .issue-assignee + = f.label :assignee_id do + %i.icon-user + Assign to + %div + = project_users_select_tag('merge_request[assignee_id]', placeholder: 'Select a user', class: 'custom-form-control', selected: @merge_request.assignee_id, project_id: @merge_request.target_project_id) +   + = link_to 'Assign to me', '#', class: 'btn assign-to-me-link' + .form-group + .issue-milestone + = f.label :milestone_id do + %i.icon-time + Milestone + %div= f.select(:milestone_id, milestone_options(@merge_request), { include_blank: "Select milestone" }, {class: 'select2'}) + .panel-footer + - if contribution_guide_url(@target_project) + %p + Please review the + %strong #{link_to "guidelines for contribution", contribution_guide_url(@target_project)} + to this repository. + = f.hidden_field :source_project_id + = f.hidden_field :target_project_id + = f.hidden_field :target_branch + = f.hidden_field :source_branch + = f.submit 'Submit merge request', class: "btn btn-create" - %h4 Changes - - if @diffs.present? - = render "projects/commits/diffs", diffs: @diffs, project: @project - - elsif @commits.size > MergeRequestDiff::COMMITS_SAFE_SIZE - .bs-callout.bs-callout-danger - %h4 This comparison includes more than #{MergeRequestDiff::COMMITS_SAFE_SIZE} commits. - %p To preserve performance the line changes are not shown. +.mr-compare + %div.panel.panel-default + .panel-heading + Commits (#{@commits.count}) + - if @commits.size > MergeRequestDiff::COMMITS_SAFE_SIZE + %ul.well-list + - Commit.decorate(@commits.first(MergeRequestDiff::COMMITS_SAFE_SIZE)).each do |commit| + = render "projects/commits/inline_commit", commit: commit, project: @project + %li.warning-row.unstyled + other #{@commits.size - MergeRequestDiff::COMMITS_SAFE_SIZE} commits hidden to prevent performance issues. + - else + %ul.well-list= render Commit.decorate(@commits), project: @project + + %h4 Changes + - if @diffs.present? + = render "projects/commits/diffs", diffs: @diffs, project: @project + - elsif @commits.size > MergeRequestDiff::COMMITS_SAFE_SIZE + .bs-callout.bs-callout-danger + %h4 This comparison includes more than #{MergeRequestDiff::COMMITS_SAFE_SIZE} commits. + %p To preserve performance the line changes are not shown. + - else + .bs-callout.bs-callout-danger + %h4 This comparison includes huge diff. + %p To preserve performance the line changes are not shown. :javascript diff --git a/app/views/projects/merge_requests/new.html.haml b/app/views/projects/merge_requests/new.html.haml index c24e5916721..4756903d0e0 100644 --- a/app/views/projects/merge_requests/new.html.haml +++ b/app/views/projects/merge_requests/new.html.haml @@ -1,4 +1,4 @@ -- if @commits.present? +- if @merge_request.can_be_created = render 'new_submit' - else = render 'new_compare'