Merge branch 'ff-merge'

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

CHANGELOG-EE

 v 8.0.0 (unreleased)
   - Fix navigation issue when viewing Group Settings pages
   - Guests and Reporters can approve merge request as well
+  - Add fast-forward merge option in project settings 
+  - Separate rebase & fast-forward merge features
 
 v 7.14.0
   - Disable adding, updating and removing members from a group that is synced with LDAP

app/controllers/projects/merge_requests_controller.rb

@@ -2,7 +2,7 @@ class Projects::MergeRequestsController < Projects::ApplicationController
   before_action :module_enabled
   before_action :merge_request, only: [
     :edit, :update, :show, :diffs, :commits, :merge, :merge_check,
-    :ci_status, :toggle_subscription, :approve
+    :ci_status, :toggle_subscription, :approve, :ff_merge, :rebase
   ]
   before_action :closes_issues, only: [:edit, :update, :show, :diffs, :commits]
   before_action :validates_merge_request, only: [:show, :diffs, :commits]
@@ -218,6 +218,28 @@ class Projects::MergeRequestsController < Projects::ApplicationController
     redirect_to merge_request_path(@merge_request)
   end
 
+  def ff_merge
+    return access_denied! unless @merge_request.can_be_merged_by?(current_user)
+    return render_404 unless @merge_request.approved?
+
+    if @merge_request.ff_merge_possible?
+      MergeRequests::FfMergeService.new(merge_request.target_project, current_user).
+        execute(merge_request)
+      @status = true
+    else
+      @status = false
+    end
+  end
+
+  def rebase
+    return access_denied! unless @merge_request.can_be_merged_by?(current_user)
+    return render_404 unless @merge_request.approved?
+
+    RebaseWorker.perform_async(@merge_request.id, current_user.id)
+
+    redirect_to merge_request_path(@merge_request), notice: 'Rebase started. It will take some time'
+  end
+
   protected
 
   def selected_target_project

app/controllers/projects_controller.rb

@@ -194,8 +194,8 @@ class ProjectsController < ApplicationController
       :name, :path, :description, :issues_tracker, :tag_list,
       :issues_enabled, :merge_requests_enabled, :snippets_enabled, :issues_tracker_id, :default_branch,
       :wiki_enabled, :merge_requests_template, :visibility_level, :merge_requests_rebase_enabled,
-      :import_url, :last_activity_at, :namespace_id, :avatar, :merge_requests_rebase_default,
-      :approvals_before_merge, :approver_ids, :reset_approvals_on_push
+      :import_url, :last_activity_at, :namespace_id, :avatar,
+      :approvals_before_merge, :approver_ids, :reset_approvals_on_push, :merge_requests_ff_only_enabled
     )
   end
 

app/models/merge_request.rb

@@ -498,4 +498,20 @@ class MergeRequest < ActiveRecord::Base
       unlock_mr if locked?
     end
   end
+
+  def source_sha_parent
+    source_project.repository.commit(source_sha).parents.first.sha
+  end
+
+  def ff_merge_possible?
+    target_sha == source_sha_parent
+  end
+
+  def rebase_dir_path
+    Rails.root.join('tmp', 'rebase', source_project.id.to_s, id.to_s).to_s
+  end
+
+  def rebase_in_progress?
+    File.exist?(rebase_dir_path)
+  end
 end

app/models/repository.rb

@@ -435,6 +435,18 @@ class Repository
     end
   end
 
+  def ff_merge(user, source_sha, target_branch, options = {})
+    our_commit = rugged.branches[target_branch].target
+    their_commit = rugged.lookup(source_sha)
+
+    raise "Invalid merge target" if our_commit.nil?
+    raise "Invalid merge source" if their_commit.nil?
+
+    commit_with_hooks(user, target_branch) do |ref|
+      source_sha
+    end
+  end
+
   def merge(user, source_sha, target_branch, options = {})
     our_commit = rugged.branches[target_branch].target
     their_commit = rugged.lookup(source_sha)

app/services/merge_requests/ff_merge_service.rb

+module MergeRequests
+  # MergeService class
+  #
+  # Do git merge and in case of success
+  # mark merge request as merged and execute all hooks and notifications
+  # Executed when you do merge via GitLab UI
+  #
+  class FfMergeService < MergeRequests::BaseService
+    attr_reader :merge_request
+
+    def execute(merge_request)
+      @merge_request = merge_request
+
+      unless @merge_request.ff_merge_possible?
+        return error('Merge request is not mergeable')
+      end
+
+      merge_request.in_locked_state do
+        if update_head
+          after_merge
+          success
+        else
+          error('Can not merge changes')
+        end
+      end
+    end
+
+    private
+
+    def update_head
+      repository.ff_merge(current_user, merge_request.source_sha, merge_request.target_branch)
+    end
+
+    def after_merge
+      MergeRequests::PostMergeService.new(project, current_user).execute(merge_request)
+    end
+  end
+end

app/services/merge_requests/rebase_service.rb

+module MergeRequests
+  # MergeService class
+  #
+  # Do git merge and in case of success
+  # mark merge request as merged and execute all hooks and notifications
+  # Executed when you do merge via GitLab UI
+  #
+  class RebaseService < MergeRequests::BaseService
+    include Gitlab::Popen
+
+    attr_reader :merge_request
+
+    def execute(merge_request)
+      @merge_request = merge_request
+
+      if rebase
+        success
+      else
+        error('Failed to rebase. Should be done manually')
+      end
+    end
+
+    def rebase
+      Gitlab::ShellEnv.set_env(current_user)
+
+      if merge_request.rebase_in_progress?
+        log('Rebase task canceled: Another rebase is already in progress')
+        return false
+      end
+
+      # Clone
+      output, status = popen(%W(git clone -b #{merge_request.source_branch} -- #{source_project.repository.path_to_repo} #{tree_path}))
+
+      unless status.zero?
+        log('Failed to clone repository for rebase:')
+        log(output)
+        return false
+      end
+
+      # Rebase
+      output, status = popen(%W(git pull --rebase #{target_project.repository.path_to_repo} #{merge_request.target_branch}), tree_path)
+
+      unless status.zero?
+        log('Failed to rebase branch:')
+        log(output)
+        return false
+      end
+
+      # Push
+      output, status = popen(%W(git push -f origin #{merge_request.source_branch}), tree_path)
+
+      unless status.zero?
+        log('Failed to push rebased branch:')
+        log(output)
+        return false
+      end
+
+      true
+    rescue => ex
+      log('Failed to rebase branch:')
+      log(ex.message)
+    ensure
+      clean_dir
+      Gitlab::ShellEnv.reset_env
+    end
+
+    def source_project
+      @source_project ||= merge_request.source_project
+    end
+
+    def target_project
+      @target_project ||= merge_request.target_project
+    end
+
+    def tree_path
+      @tree_path ||= merge_request.rebase_dir_path
+    end
+
+    def log(message)
+      Gitlab::GitLogger.error(message)
+    end
+
+    def clean_dir
+      FileUtils.rm_rf(tree_path) if File.exist?(tree_path)
+    end
+  end
+end

app/views/projects/_merge_request_settings.html.haml

@@ -5,16 +5,20 @@
   .form-group
     .col-sm-offset-2.col-sm-10
       .checkbox
-        = f.label :merge_requests_rebase_enabled do
-          = f.check_box :merge_requests_rebase_enabled
-          %span.descr Allows rebasing of merge requests before merging.
+        = f.label :merge_requests_ff_only_enabled do
+          = f.check_box :merge_requests_ff_only_enabled
+          %strong Only fast-forward merging
+          %br
+          %span.descr The accept merge request button will only show when a merge without a merge commit is possible.
 
-  .form-group.rebase-default
+  .form-group.rebase-feature
     .col-sm-offset-2.col-sm-10
       .checkbox
-        = f.label :merge_requests_rebase_default do
-          = f.check_box :merge_requests_rebase_default
-          %span.descr Rebase enabled by default
+        = f.label :merge_requests_rebase_enabled do
+          = f.check_box :merge_requests_rebase_enabled
+          %strong Rebase button
+          %br
+          %span.descr Allows rebasing of merge requests before fast-forward merge.
 
   .form-group
     = f.label :merge_requests_template, class: 'control-label' do
@@ -65,3 +69,12 @@
 
 :coffeescript
   new UsersSelect()
+
+  mergeRequestsRebaseVisibilityCheck = ->
+    is_rebase_enabled = $("input#project_merge_requests_ff_only_enabled").prop("checked")
+    $(".rebase-feature").toggle(is_rebase_enabled)
+
+  mergeRequestsRebaseVisibilityCheck()
+
+  $("input#project_merge_requests_ff_only_enabled").change ->
+    mergeRequestsRebaseVisibilityCheck()

app/views/projects/edit.html.haml

@@ -213,16 +213,3 @@
 
 
 = render 'shared/confirm_modal', phrase: @project.path
-
-:coffeescript
-  $ ->
-    mergeRequestsRebaseVisibilityCheck = ->
-      is_rebase_enabled = $("input#project_merge_requests_rebase_enabled").prop("checked")
-      $(".rebase-default").toggle(is_rebase_enabled)
-
-    mergeRequestsRebaseVisibilityCheck()
-
-    $("input#project_merge_requests_rebase_enabled").change ->
-      mergeRequestsRebaseVisibilityCheck()
-
-

app/views/projects/merge_requests/ff_merge.js.haml

+- if @status
+  :plain
+    location.reload()
+- else
+  :plain
+    $('.mr-widget-body').html("#{escape_javascript(render('projects/merge_requests/widget/open/reload'))}");

app/views/projects/merge_requests/widget/_open.html.haml

@@ -17,6 +17,8 @@
       = render 'projects/merge_requests/widget/open/approve'
     - elsif !@merge_request.can_be_merged_by?(current_user)
       = render 'projects/merge_requests/widget/open/not_allowed'
+    - elsif @project.merge_requests_ff_only_enabled
+      = render 'projects/merge_requests/widget/open/ff_accept'
     - elsif @merge_request.can_be_merged?
       = render 'projects/merge_requests/widget/open/accept'
 

app/views/projects/merge_requests/widget/open/_accept.html.haml

@@ -9,11 +9,6 @@
         = label_tag :should_remove_source_branch, class: "remove_source_checkbox" do
           = check_box_tag :should_remove_source_branch
           Remove source branch
-    - if @merge_request.target_project.merge_requests_rebase_enabled && can_rebase?(@merge_request.target_project, @merge_request.target_branch)
-      .accept-control.remove_branch_holder.checkbox
-        = label_tag :should_rebase do
-          = check_box_tag :should_rebase, "1", @project.merge_requests_rebase_default
-          Rebase before merge
 
     .accept-control
       = link_to "#", class: "modify-merge-commit-link js-toggle-button" do

app/views/projects/merge_requests/widget/open/_ff_accept.html.haml

+- if @merge_request.ff_merge_possible?
+  = form_for [:ff_merge, @project.namespace.becomes(Namespace), @project, @merge_request], remote: true, method: :post, html: { class: 'accept-mr-form js-requires-input' } do |f|
+    = hidden_field_tag :authenticity_token, form_authenticity_token
+    .accept-merge-holder.clearfix.js-toggle-container
+      .accept-action
+        = f.button class: "btn btn-create accept-mr" do
+          Accept Merge Request
+
+      .accept-control
+        Fast-forward merge without creating merge commit
+- else
+  = form_for [:rebase, @project.namespace.becomes(Namespace), @project, @merge_request],
+    remote: false, method: :post, html: { class: 'accept-mr-form js-requires-input' } do |f|
+    = hidden_field_tag :authenticity_token, form_authenticity_token
+    .accept-merge-holder.clearfix.js-toggle-container
+      - if @merge_request.target_project.merge_requests_rebase_enabled && can_rebase?(@merge_request.target_project, @merge_request.target_branch)
+        - if @merge_request.rebase_in_progress?
+          %h4 Rebase in progress... It can take a while. Reload at will.
+        - else
+          .accept-action
+            = f.button class: "btn btn-reopen rebase-mr" do
+              Rebase from #{@merge_request.target_branch}
+      .accept-control
+        Fast-forward merge is not possible. Branch must be rebased first
+
+:coffeescript
+  $('.accept-mr-form').on 'ajax:before', ->
+    btn = $('.accept-mr')
+    btn.disable()
+    btn.html("<i class='fa fa-spinner fa-spin'></i> Merge in progress")
+
+  $('.rebase-mr-form').on 'ajax:before', ->
+    btn = $('.rebase-mr')
+    btn.disable()
+    btn.html("<i class='fa fa-spinner fa-spin'></i> Rebase in progress. It could take some time")

app/workers/rebase_worker.rb

+class RebaseWorker
+  include Sidekiq::Worker
+
+  sidekiq_options queue: :default
+
+  def perform(merge_request_id, current_user_id)
+    current_user = User.find(current_user_id)
+    merge_request = MergeRequest.find(merge_request_id)
+
+    MergeRequests::RebaseService.new(merge_request.target_project, current_user).
+      execute(merge_request)
+  end
+end

config/routes.rb

@@ -505,6 +505,8 @@ Gitlab::Application.routes.draw do
             get :ci_status
             post :toggle_subscription
             post :approve
+            post :rebase
+            post :ff_merge
           end
 
           collection do

db/migrate/20150827121444_add_fast_forward_option_to_project.rb

+class AddFastForwardOptionToProject < ActiveRecord::Migration
+  def change
+    add_column :projects, :merge_requests_ff_only_enabled, :boolean, default: false
+  end
+end

db/migrate/20150827144737_migrate_rebase_feature.rb

+class MigrateRebaseFeature < ActiveRecord::Migration
+  def up
+    execute %q{UPDATE projects SET merge_requests_ff_only_enabled = TRUE WHERE merge_requests_rebase_enabled IS TRUE}
+
+    remove_column :projects, :merge_requests_rebase_default
+  end
+
+  def down
+    add_column :projects, :merge_requests_rebase_default, :boolean, default: true
+  end
+end

db/schema.rb

@@ -11,7 +11,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema.define(version: 20150818213832) do
+ActiveRecord::Schema.define(version: 20150827144737) do
 
   # These are extensions that must be enabled in order to support this database
   enable_extension "plpgsql"
@@ -464,10 +464,10 @@ ActiveRecord::Schema.define(version: 20150818213832) do
     t.boolean  "merge_requests_rebase_enabled",  default: false
     t.string   "import_type"
     t.string   "import_source"
-    t.boolean  "merge_requests_rebase_default", default: true
     t.integer  "approvals_before_merge",         default: 0,        null: false
     t.boolean  "reset_approvals_on_push",        default: true
     t.integer  "commit_count",                   default: 0
+    t.boolean  "merge_requests_ff_only_enabled", default: false
   end
 
   add_index "projects", ["created_at", "id"], name: "index_projects_on_created_at_and_id", using: :btree

doc/workflow/README.md

@@ -15,6 +15,7 @@
 - [Project forking workflow](forking_workflow.md)
 - [Project users](add-user/add-user.md)
 - [Protected branches](protected_branches.md)
+- [Fast-forward merge](ff_merge.md)
 - [Rebase before merge](rebase_before_merge.md)
 - [Sharing a project with a group](share_with_group.md)
 - [Share projects with other groups](share_projects_with_other_groups.md)

doc/workflow/ff_merge.md

+# Fast-forward merge
+
+GitLab Enterprise Edition offers a way to accept merge request without creating merge commit.
+If you prefer linear git history - this might be a good feature for you.
+You can configure this per project basis by navigating to the project settings page and selecting `Only fast-forward merging` checkbox.
+
+![Merge request settings](ff_merge.png)
+
+Now when you visit merge request page you will be able to accept it only if fast-forward merge is possible. 
+If target branch is ahead of source branch - you need to rebase source branch before you will be able to do fast-forward merge.
+
+For simple rebase operations you can use [Rebase before merge](rebase_before_merge.md) feature. 

doc/workflow/rebase_before_merge.md

-#Rebase before merge
+# Rebase before merge
 
-GitLab Enterprise Edition offers a way to rebase before merging a merge request. You can configure this per project basis by navigating to the project settings page and selecting `Merge Requests Rebase` checkbox.
+GitLab Enterprise Edition offers a way to rebase source branch of merge request. 
+This feature is part of [Fast-forward merge](ff_merge.md) feature. 
+It allows you to rebase source branch of merge request in order to perform fast-forward merge.
+
+You can configure this per project basis by navigating to the project settings page and selecting `Rebase button` checkbox.
+This checkbox is visible only if you have `Only fast-forward merging` checkbox enabled.
 
 ![Merge request settings](merge_request_settings.png)
 
-Before accepting a merge request, select `rebase before merge`.
-![Merge request widget](merge_request_widget.png)
 
-GitLab will attempt to cleanly rebase before merging branches. If clean rebase is not possible, regular merge will be performed.
-If clean rebase is possible and history of the target branch will be altered with the merge.
+Now if fast-forward merge requires rebase - you will see rebase button:
+
+![Rebase request widget](rebase_request_widget.png)
+
+GitLab will attempt to rebase source branch. If rebase succeed you will see `Accept merge request` button.
+If clean rebase is not possible - you need to do rebase manually. 
+Possibly rebase requires some conflicts to be resolved by human.

features/project/ff_merge_requests.feature

+Feature: Project Ff Merge Requests
+  Background:
+    Given I sign in as a user
+    And I own project "Shop"
+    And project "Shop" have "Bug NS-05" open merge request with diffs inside
+    And ff merge enabled
+    And merge request "Bug NS-05" is mergeable
+
+  Scenario: I do ff-only merge
+    Given merge request "Bug NS-05" is rebased
+    When I visit merge request page "Bug NS-05"
+    Then I should see ff-only merge button
+
+  Scenario: I do rebase before ff-only merge
+    Given rebase before merge enabled
+    When I visit merge request page "Bug NS-05"
+    Then I should see rebase button
+    When I press rebase button
+    Then I should see rebase in progress message
+
+  Scenario: I should do rebase before ff-only merge
+    When I visit merge request page "Bug NS-05"
+    Then I should not see rebase button
+    And I should see rebase message

features/project/merge_requests.feature

@@ -278,12 +278,3 @@ Feature: Project Merge Requests
     And I select "fix" as source
     Then I see auto-suggested approver
     And I can add it to approver list
-
-
-  Scenario: I should see rebase checkbox
-    Given project "Shop" have "Bug NS-05" open merge request with diffs inside
-    And rebase before merge enabled
-    And merge request "Bug NS-05" is mergeable
-    And I visit merge request page "Bug NS-05"
-    And merge request is mergeable
-    Then I should see rebase checkbox

features/steps/project/ff_merge_requests.rb

+class Spinach::Features::ProjectFfMergeRequests < Spinach::FeatureSteps
+  include SharedAuthentication
+  include SharedIssuable
+  include SharedProject
+  include SharedNote
+  include SharedPaths
+  include SharedMarkdown
+  include SharedDiffNote
+  include SharedUser
+
+  step 'project "Shop" have "Bug NS-05" open merge request with diffs inside' do
+    create(:merge_request_with_diffs,
+           title: "Bug NS-05",
+           source_project: project,
+           target_project: project,
+           author: project.users.first)
+  end
+
+  step 'merge request is mergeable' do
+    expect(page).to have_button 'Accept Merge Request'
+  end
+
+  step 'I should see ff-only merge button' do
+    expect(page).to have_content "Fast-forward merge without creating merge commit"
+    expect(page).to have_button 'Accept Merge Request'
+  end
+
+  step 'merge request "Bug NS-05" is mergeable' do
+    merge_request.mark_as_mergeable
+  end
+
+  step 'I accept this merge request' do
+    page.within '.mr-state-widget' do
+      click_button "Accept Merge Request"
+    end
+  end
+
+  step 'I should see merged request' do
+    page.within '.issue-box' do
+      expect(page).to have_content "Merged"
+    end
+  end
+
+  step 'ff merge enabled' do
+    project = merge_request.target_project
+    project.merge_requests_ff_only_enabled = true
+    project.save!
+  end
+
+  step 'I should not see rebase button' do
+    expect(page).to_not have_button "Rebase"
+  end
+
+  step 'I should see rebase button' do
+    expect(page).to have_button "Rebase"
+  end
+
+  step 'I should see rebase message' do
+    expect(page).to have_content "Fast-forward merge is not possible. Branch must be rebased first"
+  end
+
+  step 'merge request "Bug NS-05" is rebased' do
+    merge_request.source_branch = 'flatten-dir'
+    merge_request.target_branch = 'improve/awesome'
+    merge_request.reload_code
+    merge_request.save!
+  end
+
+  step 'rebase before merge enabled' do
+    project = merge_request.target_project
+    project.merge_requests_rebase_enabled = true
+    project.save!
+  end
+
+  step 'I press rebase button' do
+    click_button "Rebase"
+  end
+
+  step "I should see rebase in progress message" do
+    expect(page).to have_content("Rebase started. It will take some time")
+  end
+
+  def merge_request
+    @merge_request ||= MergeRequest.find_by!(title: "Bug NS-05")
+  end
+end

features/steps/project/merge_requests.rb

@@ -438,16 +438,6 @@ class Spinach::Features::ProjectMergeRequests < Spinach::FeatureSteps
     end
   end
 
-  step 'rebase before merge enabled' do
-    project = merge_request.target_project
-    project.merge_requests_rebase_enabled = true
-    project.save!
-  end
-
-  step 'I should see rebase checkbox' do
-    expect(page).to have_content 'Rebase before merge'
-  end
-
   step 'I click on "Email Patches"' do
     click_link "Email Patches"
   end

spec/services/merge_requests/ff_merge_service_spec.rb

+require 'spec_helper'
+
+describe MergeRequests::FfMergeService do
+  let(:user) { create(:user) }
+  let(:user2) { create(:user) }
+  let(:merge_request) do
+    create(:merge_request,
+           source_branch: 'flatten-dir',
+           target_branch: 'improve/awesome',
+           assignee: user2)
+  end
+  let(:project) { merge_request.project }
+
+  before do
+    project.team << [user, :master]
+    project.team << [user2, :developer]
+  end
+
+  describe :execute do
+    context 'valid params' do
+      let(:service) { MergeRequests::FfMergeService.new(project, user, {}) }
+
+      before do
+        allow(service).to receive(:execute_hooks)
+
+        service.execute(merge_request)
+      end
+
+      it "should not create merge commit" do
+        source_branch_sha = merge_request.source_project.repository.commit(merge_request.source_branch).sha
+        target_branch_sha = merge_request.target_project.repository.commit(merge_request.target_branch).sha
+        expect(source_branch_sha).to eq(target_branch_sha)
+      end
+
+      it { expect(merge_request).to be_valid }
+      it { expect(merge_request).to be_merged }
+
+      it 'should send email to user2 about merge of new merge_request' do
+        email = ActionMailer::Base.deliveries.last
+        expect(email.to.first).to eq(user2.email)
+        expect(email.subject).to include(merge_request.title)
+      end
+
+      it 'should create system note about merge_request merge' do
+        note = merge_request.notes.last
+        expect(note.note).to include 'Status changed to merged'
+      end
+    end
+  end
+end

spec/services/merge_requests/rebase_service_spec.rb

+require 'spec_helper'
+
+describe MergeRequests::RebaseService do
+  let(:user) { create(:user) }
+  let(:merge_request) do
+    create(:merge_request,
+           source_branch: 'feature_conflict',
+           target_branch: 'master')
+  end
+  let(:project) { merge_request.project }
+
+  before do
+    project.team << [user, :master]
+  end
+
+  describe :execute do
+    context 'valid params' do
+      let(:service) { MergeRequests::RebaseService.new(project, user, {}) }
+
+      before do
+        service.execute(merge_request)
+      end
+
+      it "should rebase source branch" do
+        parent_sha = merge_request.source_project.repository.commit(merge_request.source_branch).parents.first.sha
+        target_branch_sha = merge_request.target_project.repository.commit(merge_request.target_branch).sha
+        expect(parent_sha).to eq(target_branch_sha)
+      end
+    end
+  end
+end