From 543845f7efe0b70926ea699eaf1e413fa878b285 Mon Sep 17 00:00:00 2001
From: Pierre de La Morinerie <pierre@capitainetrain.com>
Date: Thu, 4 Feb 2016 19:23:58 +0100
Subject: [PATCH] Indicate how much an MR branch diverges from the target
branch
---
CHANGELOG | 1 +
app/models/merge_request.rb | 23 +++++++
.../projects/merge_requests/_show.html.haml | 2 +
features/project/merge_requests.feature | 10 +++
features/steps/project/merge_requests.rb | 29 ++++++++-
features/steps/shared/paths.rb | 8 +++
spec/factories/merge_requests.rb | 10 +++
spec/models/merge_request_spec.rb | 64 +++++++++++++++++++
8 files changed, 146 insertions(+), 1 deletion(-)
diff --git a/CHANGELOG b/CHANGELOG
index 69a1927b765..97ffbe67eec 100644
--- a/CHANGELOG
+++ b/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
diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb
index 1543ef311d7..f89bb6e9e4b 100644
--- a/app/models/merge_request.rb
+++ b/app/models/merge_request.rb
@@ -524,6 +524,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
diff --git a/app/views/projects/merge_requests/_show.html.haml b/app/views/projects/merge_requests/_show.html.haml
index d7bc26e24b9..44e25e034e4 100644
--- a/app/views/projects/merge_requests/_show.html.haml
+++ b/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"
diff --git a/features/project/merge_requests.feature b/features/project/merge_requests.feature
index 495f25f28e7..a69089f00c4 100644
--- a/features/project/merge_requests.feature
+++ b/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
diff --git a/features/steps/project/merge_requests.rb b/features/steps/project/merge_requests.rb
index dde864f5180..8bf423cc64b 100644
--- a/features/steps/project/merge_requests.rb
+++ b/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
diff --git a/features/steps/shared/paths.rb b/features/steps/shared/paths.rb
index f7812a707c4..da9d1503ebc 100644
--- a/features/steps/shared/paths.rb
+++ b/features/steps/shared/paths.rb
@@ -395,6 +395,14 @@ module SharedPaths
visit merge_request_path("Bug NS-05")
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
mr = MergeRequest.find_by(title: "Bug CO-01")
visit namespace_project_merge_request_path(mr.target_project.namespace, mr.target_project, mr)
diff --git a/spec/factories/merge_requests.rb b/spec/factories/merge_requests.rb
index 00de7bb5294..ca1c636fce4 100644
--- a/spec/factories/merge_requests.rb
+++ b/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
diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb
index c51f34034d7..59c40922abb 100644
--- a/spec/models/merge_request_spec.rb
+++ b/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) }
--
GitLab