Merge branch 'check-ff-more-precisely' into 'master'

Check fast-forward more precisely

The original issue is that (#260), some people want to be able to merge
a fast-forward MR regardless it's rebased or not. The previously
check for this: `target_sha == source_sha_parent`, assumes that the
questioning MR has a linear history (i.e. it's rebased), therefore
we could do a fast-forward merge. However, we could also do fast-forward
merge if the MR has already merged target branch (i.e. it's merged).

This MR would allow them to merge any fast-forward MR regardless rebased
or not.

But actually this breaks some assumption. In the option of:

> Merge commit with semi-linear history
>> A merge commit is created for every merge, but merging is only
>> allowed if the branch has been rebased. This way you get a history
>> that reads linearly (as with fast-forward merges), with the addition
>> of merge commits.
>> When the branch has not been rebased, the user is given the option to do
>> so.

It clearly says: `merging is only allowed if the branch has been rebased`
This MR would break this assumption. The same applies to:

> Fast-forward merge
>> No merge commits are created and all merges are fast-forwarded, which
>> means that merging is only allowed if the branch has been rebased.
>> When the branch has not been rebased, the user is given the option to do
>> so.

This means that rebase and FF are two separated concept and we're
mixing them here. We should probably update the wordings and perhaps
provide another option if we want to keep all the workflows.

My own thought is actually, allowing only FF merge might make little
sense. Allowing only FF *and* rebased (linear history) would make more
sense. That means I think the original behaviour actually makes more
sense, just that the wordings are wrong. i.e. They could be FF merged,
but that's not the point, we still want you to rebase this branch!

On the other hand, I am not against adding a new option for this
behaviour either. People have different workflows, and I think it makes
sense to allow any workflows which Git provided.

Either way, I think some wordings should be changed.

By the way, could we write a test for this?

Fixes #260

/cc @DouweM 

See merge request !454

CHANGELOG-EE

@@ -31,6 +31,7 @@ v 8.9.0
   - Send notification email when merge request is approved
   - Distribute RepositoryUpdateMirror jobs in time and add exclusive lease on them by project_id
   - [Elastic] Move ES settings to application settings
+  - Always allow merging a merge request whenever fast-forward is possible. !454
   - Disable mirror flag for projects without import_url
   - UpdateMirror service return an error status when no mirror
   - Don't reset approvals when rebasing an MR from the UI

app/models/merge_request.rb

@@ -294,7 +294,7 @@ class MergeRequest < ActiveRecord::Base
 
     check_if_can_be_merged
 
-    can_be_merged? && !must_be_rebased?
+    can_be_merged? && !should_be_rebased?
   end
 
   def mergeable_state?(skip_ci_check: false)
@@ -633,10 +633,10 @@ class MergeRequest < ActiveRecord::Base
   end
 
   def ff_merge_possible?
-    target_sha == source_sha_parent
+    project.repository.is_ancestor?(target_sha, source_sha)
   end
 
-  def must_be_rebased?
+  def should_be_rebased?
     self.project.ff_merge_must_be_possible? && !ff_merge_possible?
   end
 

app/views/projects/_merge_request_settings.html.haml

@@ -19,11 +19,11 @@
         %strong Merge commit with semi-linear history
         %br
         %span.descr
-          A merge commit is created for every merge, but merging is only allowed if the branch has been rebased.
-          This way you get a history that reads linearly (as with fast-forward merges), with the addition of merge commits.
+          A merge commit is created for every merge, but merging is only allowed if fast-forward merge is possible.
+          This way you could make sure that if this merge request would build, after merging to target branch it would also build.
         %br
         %span.descr
-          When the branch has not been rebased, the user is given the option to do so.
+          When fast-forward merge is not possible, the user is given the option to rebase.
 
     .radio
       = label_tag :project_merge_method_ff do
@@ -31,10 +31,10 @@
         %strong Fast-forward merge
         %br
         %span.descr
-          No merge commits are created and all merges are fast-forwarded, which means that merging is only allowed if the branch has been rebased.
+          No merge commits are created and all merges are fast-forwarded, which means that merging is only allowed if the branch could be fast-forwarded.
         %br
         %span.descr
-          When the branch has not been rebased, the user is given the option to do so.
+          When fast-forward merge is not possible, the user is given the option to rebase.
 
   .form-group
     = f.label :merge_requests_template, class: 'label-light' do

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

@@ -23,7 +23,7 @@
       = render 'projects/merge_requests/widget/open/not_allowed'
     - elsif !@merge_request.mergeable_ci_state? && @pipeline && @pipeline.failed?
       = render 'projects/merge_requests/widget/open/build_failed'
-    - elsif @merge_request.must_be_rebased?
+    - elsif @merge_request.should_be_rebased?
       = render 'projects/merge_requests/widget/open/rebase'
     - else
       = render 'projects/merge_requests/widget/open/accept'

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

@@ -25,7 +25,7 @@
         = f.button class: "btn btn-reopen js-rebase-button" do
           Rebase onto #{@merge_request.target_branch}
       .accept-control
-        Fast-forward merge is not possible. Rebase the source branch onto the target branch to allow this merge request to be merged.
+        Fast-forward merge is not possible. Rebase the source branch onto the target branch or merge target branch into source branch to allow this merge request to be merged.
 
   :javascript
     $('.rebase-mr-form').on('ajax:send', function() {

features/project/ff_merge_requests.feature

@@ -6,7 +6,7 @@ Feature: Project Ff Merge Requests
     And merge request "Bug NS-05" is mergeable
 
   @javascript
-  Scenario: I do ff-only merge
+  Scenario: I do ff-only merge for rebased branch
     Given ff merge enabled
     And merge request "Bug NS-05" is rebased
     When I visit merge request page "Bug NS-05"
@@ -14,6 +14,15 @@ Feature: Project Ff Merge Requests
     When I accept this merge request
     Then I should see merged request
 
+  @javascript
+  Scenario: I do ff-only merge for merged branch
+    Given ff merge enabled
+    And merge request "Bug NS-05" merged target
+    When I visit merge request page "Bug NS-05"
+    Then I should see ff-only merge button
+    When I accept this merge request
+    Then I should see merged request
+
   @javascript
   Scenario: I do rebase before ff-only merge
     Given ff merge enabled

features/steps/project/ff_merge_requests.rb

@@ -58,6 +58,13 @@ class Spinach::Features::ProjectFfMergeRequests < Spinach::FeatureSteps
     merge_request.save!
   end
 
+  step 'merge request "Bug NS-05" merged target' do
+    merge_request.source_branch = 'merged-target'
+    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

spec/support/test_env.rb

@@ -11,6 +11,7 @@ module TestEnv
     'feature_conflict' => 'bb5206f',
     'fix'              => '48f0be4',
     'improve/awesome'  => '5937ac0',
+    'merged-target'    => '21751bf',
     'markdown'         => '0ed8c6c',
     'lfs'              => 'be93687',
     'master'           => '5937ac0',