Merge branch 'rebase_on_merge_request' into 'master'

Rebase on merge request

Introduces merge request option to rebase before merging.

Issue #185

See merge request !254

CHANGELOG-EE

 v 7.6.0
   - Added Audit events related to membership changes for groups and projects
+  - Added option to attempt a rebase before merging merge request
 
 v 7.5.0
   - Added an ability to check each author commit's email by regex

app/assets/javascripts/project_new.js.coffee

@@ -23,3 +23,8 @@ class @ProjectNew
         $('#project_issues_tracker_id').attr('disabled', 'disabled')
       else
         $('#project_issues_tracker_id').removeAttr('disabled')
+
+    $("#project_merge_requests_enabled").change ->
+      checked = $(this).prop("checked")
+      $("#project_merge_requests_template").prop "disabled", not checked
+      $("#project_merge_requests_rebase_enabled").prop "disabled", not checked

app/controllers/projects/merge_requests_controller.rb

@@ -122,6 +122,7 @@ class Projects::MergeRequestsController < Projects::ApplicationController
 
     if @merge_request.open? && @merge_request.can_be_merged?
       @merge_request.should_remove_source_branch = params[:should_remove_source_branch]
+      @merge_request.should_rebase = params[:should_rebase]
       @merge_request.automerge!(current_user, params[:commit_message])
       @status = true
     else

app/controllers/projects_controller.rb

@@ -202,7 +202,8 @@ class ProjectsController < ApplicationController
     params.require(:project).permit(
       :name, :path, :description, :issues_tracker, :tag_list,
       :issues_enabled, :merge_requests_enabled, :snippets_enabled, :issues_tracker_id, :default_branch,
-      :wiki_enabled, :visibility_level, :import_url, :last_activity_at, :namespace_id, :merge_requests_template
+      :wiki_enabled, :visibility_level, :import_url, :last_activity_at, :namespace_id, :merge_requests_template,
+      :merge_requests_rebase_enabled
     )
   end
 end

app/helpers/branches_helper.rb

@@ -19,4 +19,14 @@ module BranchesHelper
 
     current_user.can?(action, project)
   end
+
+  def can_rebase?(project, branch_name)
+    if project.protected_branch? branch_name
+      can?(current_user, :push_code_to_protected_branches, project)
+    elsif can?(current_user, :push_code, project)
+      true
+    else
+      false
+    end
+  end
 end

app/models/merge_request.rb

@@ -40,6 +40,9 @@ class MergeRequest < ActiveRecord::Base
 
   attr_accessor :should_remove_source_branch
 
+  # If true, merge request should rebase before merging
+  attr_accessor :should_rebase
+
   # When this attribute is true some MR validation is ignored
   # It allows us to close or modify broken merge requests
   attr_accessor :allow_broken

app/views/projects/edit.html.haml

@@ -66,6 +66,13 @@
                   = f.check_box :merge_requests_enabled
                   %span.descr Submit changes to be merged upstream.
 
+            .form-group
+              = f.label :merge_requests_rebase_enabled, "Merge Requests Rebase", class: 'control-label'
+              .col-sm-10
+                .checkbox
+                  = f.check_box :merge_requests_rebase_enabled
+                  %span.descr Allows rebasing of merge requests before merging.
+
             .form-group
               = f.label :merge_requests_template, class: 'control-label' do
                 Merge request template

app/views/projects/merge_requests/show/_mr_accept.html.haml

@@ -24,6 +24,11 @@
                 = label_tag :should_remove_source_branch, class: "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)
+              .remove_branch_holder.pull-left
+                = label_tag :should_rebase, class: "checkbox" do
+                  = check_box_tag :should_rebase
+                  Rebase before merge
             .js-toggle-container
               %label
                 %i.fa.fa-edit

db/migrate/20141126120926_add_merge_request_rebase_enabled_to_projects.rb

+class AddMergeRequestRebaseEnabledToProjects < ActiveRecord::Migration
+  def change
+    add_column :projects, :merge_requests_rebase_enabled, :boolean, default: false
+  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: 20141118150935) do
+ActiveRecord::Schema.define(version: 20141126120926) do
 
   # These are extensions that must be enabled in order to support this database
   enable_extension "plpgsql"
@@ -302,22 +302,23 @@ ActiveRecord::Schema.define(version: 20141118150935) do
     t.datetime "created_at"
     t.datetime "updated_at"
     t.integer  "creator_id"
-    t.boolean  "issues_enabled",          default: true,     null: false
-    t.boolean  "wall_enabled",            default: true,     null: false
-    t.boolean  "merge_requests_enabled",  default: true,     null: false
-    t.boolean  "wiki_enabled",            default: true,     null: false
+    t.boolean  "issues_enabled",                default: true,     null: false
+    t.boolean  "wall_enabled",                  default: true,     null: false
+    t.boolean  "merge_requests_enabled",        default: true,     null: false
+    t.boolean  "wiki_enabled",                  default: true,     null: false
     t.integer  "namespace_id"
-    t.string   "issues_tracker",          default: "gitlab", null: false
+    t.string   "issues_tracker",                default: "gitlab", null: false
     t.string   "issues_tracker_id"
-    t.boolean  "snippets_enabled",        default: true,     null: false
+    t.boolean  "snippets_enabled",              default: true,     null: false
     t.datetime "last_activity_at"
     t.string   "import_url"
-    t.integer  "visibility_level",        default: 0,        null: false
-    t.boolean  "archived",                default: false,    null: false
+    t.integer  "visibility_level",              default: 0,        null: false
+    t.boolean  "archived",                      default: false,    null: false
     t.string   "import_status"
-    t.float    "repository_size",         default: 0.0
-    t.integer  "star_count",              default: 0,        null: false
+    t.float    "repository_size",               default: 0.0
+    t.integer  "star_count",                    default: 0,        null: false
     t.text     "merge_requests_template"
+    t.boolean  "merge_requests_rebase_enabled", default: false
   end
 
   add_index "projects", ["creator_id"], name: "index_projects_on_creator_id", using: :btree

doc/workflow/gitlab_flow.md

@@ -238,6 +238,16 @@ In a CI strategy you can merge in master at the start of the day to prevent pain
 In a synchronization point strategy you only merge in from well defined points in time, for example a tagged release.
 This strategy is [advocated by Linus Torvalds](https://www.mail-archive.com/dri-devel@lists.sourceforge.net/msg39091.html) because the state of the code at these points is better known.
 
+GitLab Enterprise Edition offers a way to rebase before merging a merge request. You can configure this per project bassis by navigating to the project settings page and selecting `Merge Requests Rebase` checkbox.
+
+![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 traget branch will be altered with the the merge.
+
 In conclusion, we can say that you should try to prevent merge commits, but not eliminate them.
 Your codebase should be clean but your history should represent what actually happened.
 Developing software happen in small messy steps and it is OK to have your history reflect this.

lib/gitlab/satellite/merge_action.rb

@@ -2,6 +2,8 @@ module Gitlab
   module Satellite
     # GitLab server-side merge
     class MergeAction < Action
+      include Gitlab::Popen
+
       attr_accessor :merge_request
 
       def initialize(user, merge_request)
@@ -27,16 +29,24 @@ module Gitlab
       def merge!(merge_commit_message = nil)
         in_locked_and_timed_satellite do |merge_repo|
           prepare_satellite!(merge_repo)
+
+          # If rebase before merge
+          # Attempt a rebase and return if succesful.
+          # If unsuccesful, continue with regular merge.
+          if merge_request.should_rebase
+            if rebase_in_satellite!(merge_repo)
+              remove_source_branch(merge_repo)
+              return true
+            end
+          end
+
           if merge_in_satellite!(merge_repo, merge_commit_message)
             # push merge back to bare repo
             # will raise CommandFailed when push fails
             merge_repo.git.push(default_options, :origin, merge_request.target_branch)
 
             # remove source branch
-            if merge_request.should_remove_source_branch && !project.root_ref?(merge_request.source_branch) && !merge_request.for_fork?
-              # will raise CommandFailed when push fails
-              merge_repo.git.push(default_options, :origin, ":#{merge_request.source_branch}")
-            end
+            remove_source_branch(merge_repo)
             # merge, push and branch removal successful
             true
           end
@@ -133,6 +143,40 @@ module Gitlab
         handle_exception(ex)
       end
 
+      # Rebases before merging the source_branch into the target_branch in the satellite.
+      # Before starting the rebase, clears out the satellite.
+      # Returns false if rebase cannot be done cleanly and resets the satellite.
+      # Returns true otherwise.
+
+      # Eg. of clean rebase
+      # source_branch: feature
+      # target_branch: master
+      #
+      # git checkout feature
+      # git pull --rebase origin master
+      # git push origin feature:master
+      # git merge feature
+      # git push remote master
+      def rebase_in_satellite!(repo)
+        update_satellite_source_and_target!(repo)
+        repo.git.checkout(default_options({b: true}), merge_request.source_branch, "source/#{merge_request.source_branch}")
+
+        output, status = popen(%W(git pull --rebase origin #{merge_request.target_branch}), repo.working_dir)
+        if status == 0
+          Gitlab::AppLogger.info "Rebasing before merge in #{merge_request.source_project.path_with_namespace} MR!#{merge_request.id}: #{output}."
+          if merge_request.source_branch && merge_request.target_branch
+            repo.git.push(default_options, "origin", "#{merge_request.source_branch}:#{merge_request.target_branch}")
+          end
+        else
+          repo.git.rebase(default_options, "--abort")
+          Gitlab::AppLogger.info "Rebasing in in #{merge_request.source_project.path_with_namespace} MR!#{merge_request.id} aborted, rebase manually."
+          prepare_satellite!(repo)
+          false
+        end
+      rescue Grit::Git::CommandFailed => ex
+        handle_exception(ex)
+      end
+
       # Assumes a satellite exists that is a fresh clone of the projects repo, prepares satellite for merges, diffs etc
       def update_satellite_source_and_target!(repo)
         repo.remote_add('source', merge_request.source_project.repository.path_to_repo)
@@ -141,6 +185,15 @@ module Gitlab
       rescue Grit::Git::CommandFailed => ex
         handle_exception(ex)
       end
+
+      def remove_source_branch(repo)
+        if merge_request.should_remove_source_branch && !project.root_ref?(merge_request.source_branch) && !merge_request.for_fork?
+          # will raise CommandFailed when push fails
+          repo.git.push(default_options, :origin, ":#{merge_request.source_branch}")
+        end
+      rescue Grit::Git::CommandFailed => ex
+        handle_exception(ex)
+      end
     end
   end
 end

spec/lib/gitlab/satellite/merge_action_spec.rb

@@ -101,4 +101,36 @@ describe 'Gitlab::Satellite::MergeAction' do
         merge_request_with_conflict).can_be_merged?.should be_false }
     end
   end
+
+
+  describe '#can_be_rebased?' do
+    context 'on fork' do
+      before(:each) do
+        merge_request.stub(:should_rebase).and_return(true)
+      end
+
+      it { Gitlab::Satellite::MergeAction.new(
+        merge_request_fork.author,
+        merge_request_fork).merge!.should be_true }
+
+      it { Gitlab::Satellite::MergeAction.new(
+        merge_request_fork_with_conflict.author,
+        merge_request_fork_with_conflict).merge!.should be_false }
+    end
+
+    context 'between branches' do
+      before(:each) do
+        merge_request.stub(:should_rebase).and_return(true)
+      end
+
+      it { Gitlab::Satellite::MergeAction.new(
+        merge_request.author,
+        merge_request).merge!.should be_true }
+
+      it { Gitlab::Satellite::MergeAction.new(
+        merge_request_with_conflict.author,
+        merge_request_with_conflict).merge!.should be_false }
+    end
+  end
 end
+