Refactor MR build process

Signed-off-by: Dmitriy Zaporozhets <dmitriy.zaporozhets@gmail.com>

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
-    )
-
-    @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
-
-    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
-      )
+    params[:merge_request] ||= ActionController::Parameters.new(source_project: @project)
+    @merge_request = MergeRequests::BuildService.new(project, current_user, merge_request_params).execute
 
-      @compare_failed = false
-      @commits = compare_action.commits
-
-      if @commits
-        @commits.map! { |commit| Commit.new(commit) }
-        @commit = @commits.first
+    @target_branches = if @merge_request.target_project
+                         @merge_request.target_project.repository.branch_names
                        else
-        # false value because failed to get commits from satellite
-        @commits = []
-        @compare_failed = true
+                         []
                        end
 
+    @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
 
-      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)
+    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
-      rescue Gitlab::Satellite::BranchesWithoutParent
-        @error = "Selected branches have no common commit so they cannot be merged."
-      end
     end
   end
 

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

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
 

app/services/merge_requests/build_service.rb

+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

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

app/views/projects/merge_requests/_new_submit.html.haml

@@ -10,11 +10,8 @@
 
   %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|
+
+= form_for [@project, @merge_request], html: { class: "merge-request-form" } do |f|
   .panel.panel-default
 
     .panel-body
@@ -47,11 +44,10 @@
             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))
+      - if contribution_guide_url(@target_project)
         %p
           Please review the
-            %strong #{link_to "guidelines for contribution", contribution_guide_url}
+          %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
@@ -59,7 +55,7 @@
       = f.hidden_field :source_branch
       = f.submit 'Submit merge request', class: "btn btn-create"
 
-  .mr-compare
+.mr-compare
   %div.panel.panel-default
     .panel-heading
       Commits (#{@commits.count})
@@ -79,6 +75,10 @@
     .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

app/views/projects/merge_requests/new.html.haml

-- if @commits.present?
+- if @merge_request.can_be_created
   = render 'new_submit'
 - else
   = render 'new_compare'