Merge branch 'indicate-mr-diverged-from-target' into 'master'

Indicate when an MR diverged from the target branch

This adds an indicator to the "Merge MR" box, to tell if and how much an MR diverged from its target branch.

For instance, consider an MR to merge the branch `feature` into `master`. Some other commits were added to `master` since `feature` was created, and the two branches diverged.

```text
o master
|
o    o feature
|    |
o    o
|  /
o
```

In this case, there will be a label in the MR Merge box stating:

> This MR is by 3 commits behind the target branch `master`.

## Screenshots

### The branch diverged from the target (UI Proposal)

![UI_suggestion_1](/uploads/cd5bee3959e68026ec7d5097259d53f4/UI_suggestion_1.png)

### The branch diverged from the target (alternative UI Proposal)

![UI_suggestion_2](/uploads/f36977101b59a610850e129837dfbc83/UI_suggestion_2.png)

## How is this useful?

- In a _rebase-workflow_ (MR are preferably rebased before being merged), the reviewer wants to know if an MR is rebased on the target branch before merging it. 
    
    _With this indicator, the reviewer knows immediately if the branch is rebased, or if she needs to ask the committer to rebase its branch._

<br>

- To keep the git history readable, a team prefers to avoid merging branches that really lag a lot behind the target branch. Merging an MR that is 10 commits behind is fine, but 200 is too much.

    _With this indicator, the reviewer can see on the MR page if the branch is really far behind the target – or only a few commits behind._

## Open questions

We've been using this at @captaintrain for a few months now, and found it quite useful.

I guess the open-questions are mostly: what UI would be the more adequate? Any thoughts on this, on the general usefulness and/or on the code?

See merge request !2217

CHANGELOG

@@ -6,6 +6,7 @@ v 8.6.0 (unreleased)
   - Fix issue when pushing to projects ending in .wiki
   - Fix avatar stretching by providing a cropping feature (Johann Pardanaud)
   - Don't load all of GitLab in mail_room
+  - Indicate how much an MR diverged from the target branch (Pierre de La Morinerie)
   - Strip leading and trailing spaces in URL validator (evuez)
   - Return empty array instead of 404 when commit has no statuses in commit status API
   - Update documentation to reflect Guest role not being enforced on internal projects

app/models/merge_request.rb

@@ -537,6 +537,29 @@ class MergeRequest < ActiveRecord::Base
     end
   end
 
+  def diverged_commits_count
+    cache = Rails.cache.read(:"merge_request_#{id}_diverged_commits")
+
+    if cache.blank? || cache[:source_sha] != source_sha || cache[:target_sha] != target_sha
+      cache = {
+        source_sha: source_sha,
+        target_sha: target_sha,
+        diverged_commits_count: compute_diverged_commits_count
+      }
+      Rails.cache.write(:"merge_request_#{id}_diverged_commits", cache)
+    end
+
+    cache[:diverged_commits_count]
+  end
+
+  def compute_diverged_commits_count
+    Gitlab::Git::Commit.between(target_project.repository.raw_repository, source_sha, target_sha).size
+  end
+
+  def diverged_from_target_branch?
+    diverged_commits_count > 0
+  end
+
   def ci_commit
     @ci_commit ||= source_project.ci_commit(last_commit.id) if last_commit && source_project
   end

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

@@ -34,6 +34,8 @@
         %span into
         = link_to namespace_project_commits_path(@project.namespace, @project, @merge_request.target_branch), class: "label-branch" do
           = @merge_request.target_branch
+        - if @merge_request.open? && @merge_request.diverged_from_target_branch?
+          %span (#{pluralize(@merge_request.diverged_commits_count, 'commit')} behind)
 
     = render "projects/merge_requests/show/how_to_merge"
     = render "projects/merge_requests/widget/show.html.haml"

features/project/merge_requests.feature

@@ -26,6 +26,16 @@ Feature: Project Merge Requests
     When I visit project "Shop" merge requests page
     Then I should see "other_branch" branch
 
+  Scenario: I should not see the numbers of diverged commits if the branch is rebased on the target
+    Given project "Shop" have "Bug NS-07" open merge request with rebased branch
+    When I visit merge request page "Bug NS-07"
+    Then I should not see the diverged commits count
+
+  Scenario: I should see the numbers of diverged commits if the branch diverged from the target
+    Given project "Shop" have "Bug NS-08" open merge request with diverged branch
+    When I visit merge request page "Bug NS-08"
+    Then I should see the diverged commits count
+
   Scenario: I should see rejected merge requests
     Given I click link "Closed"
     Then I should see "Feature NS-03" in merge requests

features/steps/project/merge_requests.rb

@@ -60,7 +60,6 @@ class Spinach::Features::ProjectMergeRequests < Spinach::FeatureSteps
     expect(page).not_to have_content "Feature NS-03"
   end
 
-
   step 'I should not see "Bug NS-04" in merge requests' do
     expect(page).not_to have_content "Bug NS-04"
   end
@@ -121,6 +120,22 @@ class Spinach::Features::ProjectMergeRequests < Spinach::FeatureSteps
            author: project.users.first)
   end
 
+  step 'project "Shop" have "Bug NS-07" open merge request with rebased branch' do
+    create(:merge_request, :rebased,
+      title: "Bug NS-07",
+      source_project: project,
+      target_project: project,
+      author: project.users.first)
+  end
+
+  step 'project "Shop" have "Bug NS-08" open merge request with diverged branch' do
+    create(:merge_request, :diverged,
+      title: "Bug NS-08",
+      source_project: project,
+      target_project: project,
+      author: project.users.first)
+  end
+
   step 'project "Shop" have "Feature NS-03" closed merge request' do
     create(:closed_merge_request,
            title: "Feature NS-03",
@@ -490,6 +505,18 @@ class Spinach::Features::ProjectMergeRequests < Spinach::FeatureSteps
     end
   end
 
+  step 'I should see the diverged commits count' do
+    page.within ".mr-source-target" do
+      expect(page).to have_content /([0-9]+ commits behind)/
+    end
+  end
+
+  step 'I should not see the diverged commits count' do
+    page.within ".mr-source-target" do
+      expect(page).not_to have_content /([0-9]+ commit[s]? behind)/
+    end
+  end
+
   step 'I should see "Bug NS-05" at the top' do
     expect(page.find('ul.content-list.mr-list li.merge-request:first-child')).to have_content("Bug NS-05")
   end

features/steps/shared/paths.rb

@@ -388,13 +388,19 @@ module SharedPaths
   end
 
   step 'I visit merge request page "Bug NS-04"' do
-    mr = MergeRequest.find_by(title: "Bug NS-04")
+    visit merge_request_path("Bug NS-04")
-    visit namespace_project_merge_request_path(mr.target_project.namespace, mr.target_project, mr)
   end
 
   step 'I visit merge request page "Bug NS-05"' do
-    mr = MergeRequest.find_by(title: "Bug NS-05")
+    visit merge_request_path("Bug NS-05")
-    visit namespace_project_merge_request_path(mr.target_project.namespace, mr.target_project, mr)
+  end
+
+  step 'I visit merge request page "Bug NS-07"' do
+    visit merge_request_path("Bug NS-07")
+  end
+
+  step 'I visit merge request page "Bug NS-08"' do
+    visit merge_request_path("Bug NS-08")
   end
 
   step 'I visit merge request page "Bug CO-01"' do
@@ -503,6 +509,11 @@ module SharedPaths
     Project.find_by!(name: 'Shop')
   end
 
+  def merge_request_path(title)
+    mr = MergeRequest.find_by(title: title)
+    namespace_project_merge_request_path(mr.target_project.namespace, mr.target_project, mr)
+  end
+
   # ----------------------------------------
   # Errors
   # ----------------------------------------

spec/factories/merge_requests.rb

@@ -69,6 +69,16 @@ FactoryGirl.define do
       target_branch "master"
     end
 
+    trait :rebased do
+      source_branch "markdown"
+      target_branch "improve/awesome"
+    end
+
+    trait :diverged do
+      source_branch "feature"
+      target_branch "master"
+    end
+
     trait :merge_when_build_succeeds do
       merge_when_build_succeeds true
       merge_user author

spec/models/merge_request_spec.rb

@@ -274,6 +274,70 @@ describe MergeRequest, models: true do
     end
   end
 
+  describe '#diverged_commits_count' do
+    let(:project)      { create(:project) }
+    let(:fork_project) { create(:project, forked_from_project: project) }
+
+    context 'diverged on same repository' do
+      subject(:merge_request_with_divergence) { create(:merge_request, :diverged, source_project: project, target_project: project) }
+
+      it 'counts commits that are on target branch but not on source branch' do
+        expect(subject.diverged_commits_count).to eq(5)
+      end
+    end
+
+    context 'diverged on fork' do
+      subject(:merge_request_fork_with_divergence) { create(:merge_request, :diverged, source_project: fork_project, target_project: project) }
+
+      it 'counts commits that are on target branch but not on source branch' do
+        expect(subject.diverged_commits_count).to eq(5)
+      end
+    end
+
+    context 'rebased on fork' do
+      subject(:merge_request_rebased) { create(:merge_request, :rebased, source_project: fork_project, target_project: project) }
+
+      it 'counts commits that are on target branch but not on source branch' do
+        expect(subject.diverged_commits_count).to eq(0)
+      end
+    end
+
+    describe 'caching' do
+      before(:example) do
+        allow(Rails).to receive(:cache).and_return(ActiveSupport::Cache::MemoryStore.new)
+      end
+
+      it 'caches the output' do
+        expect(subject).to receive(:compute_diverged_commits_count).
+          once.
+          and_return(2)
+
+        subject.diverged_commits_count
+        subject.diverged_commits_count
+      end
+
+      it 'invalidates the cache when the source sha changes' do
+        expect(subject).to receive(:compute_diverged_commits_count).
+          twice.
+          and_return(2)
+
+        subject.diverged_commits_count
+        allow(subject).to receive(:source_sha).and_return('123abc')
+        subject.diverged_commits_count
+      end
+
+      it 'invalidates the cache when the target sha changes' do
+        expect(subject).to receive(:compute_diverged_commits_count).
+          twice.
+          and_return(2)
+
+        subject.diverged_commits_count
+        allow(subject).to receive(:target_sha).and_return('123abc')
+        subject.diverged_commits_count
+      end
+    end
+  end
+
   it_behaves_like 'an editable mentionable' do
     subject { create(:merge_request) }