From 4a8a8282d93fa8a486fdd3adc67a40fee737861d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9my=20Coutable?= Date: Fri, 18 Mar 2016 09:28:41 +0100 Subject: [PATCH 1/2] Fix an issue when the target branch of a MR had been deleted Before displaying the "diverged commits" note, we're checking if the MR is open, but we should check if it's mergeable instead because this check ensure the source and target branches exist. This was introduced by !2217 and fixes #14388. --- CHANGELOG | 1 + app/views/projects/merge_requests/_show.html.haml | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/CHANGELOG b/CHANGELOG index 28c1c30e20..ca360c6706 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -43,6 +43,7 @@ v 8.6.0 (unreleased) - Increase the notes polling timeout over time (Roberto Dip) - Add shortcut to toggle markdown preview (Florent Baldino) - Show labels in dashboard and group milestone views + - Fix an issue when the target branch of a MR had been deleted - Add main language of a project in the list of projects (Tiago Botelho) - Add #upcoming filter to Milestone filter (Tiago Botelho) - Add ability to show archived projects on dashboard, explore and group pages diff --git a/app/views/projects/merge_requests/_show.html.haml b/app/views/projects/merge_requests/_show.html.haml index ee5b9fd95a..b9695157f6 100644 --- a/app/views/projects/merge_requests/_show.html.haml +++ b/app/views/projects/merge_requests/_show.html.haml @@ -34,7 +34,7 @@ %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? + - if @merge_request.mergeable? && @merge_request.diverged_from_target_branch? %span (#{pluralize(@merge_request.diverged_commits_count, 'commit')} behind) = render "projects/merge_requests/show/how_to_merge" From bc92de8f038012b284ea1cbfbb2f0950943ebd88 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9my=20Coutable?= Date: Fri, 18 Mar 2016 18:16:04 +0100 Subject: [PATCH 2/2] Add a safeguard in MergeRequest#compute_diverged_commits_count We have to ensure source_sha and target_sha are not nil before calling Gitlab::Git::Commit.between. --- app/models/merge_request.rb | 5 ++++- .../projects/merge_requests/_show.html.haml | 2 +- spec/models/merge_request_spec.rb | 22 +++++++++++++++++++ 3 files changed, 27 insertions(+), 2 deletions(-) diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index 30a7bd47be..a6140b5b04 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -516,7 +516,7 @@ class MergeRequest < ActiveRecord::Base end def target_sha - @target_sha ||= target_project.repository.commit(target_branch).sha + @target_sha ||= target_project.repository.commit(target_branch).try(:sha) end def source_sha @@ -572,8 +572,11 @@ class MergeRequest < ActiveRecord::Base end def compute_diverged_commits_count + return 0 unless source_sha && target_sha + Gitlab::Git::Commit.between(target_project.repository.raw_repository, source_sha, target_sha).size end + private :compute_diverged_commits_count def diverged_from_target_branch? diverged_commits_count > 0 diff --git a/app/views/projects/merge_requests/_show.html.haml b/app/views/projects/merge_requests/_show.html.haml index b9695157f6..ee5b9fd95a 100644 --- a/app/views/projects/merge_requests/_show.html.haml +++ b/app/views/projects/merge_requests/_show.html.haml @@ -34,7 +34,7 @@ %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.mergeable? && @merge_request.diverged_from_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" diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb index 654c71b682..2165cfb7a3 100644 --- a/spec/models/merge_request_spec.rb +++ b/spec/models/merge_request_spec.rb @@ -86,6 +86,16 @@ describe MergeRequest, models: true do end end + describe '#target_sha' do + context 'when the target branch does not exist anymore' do + subject { create(:merge_request).tap { |mr| mr.update_attribute(:target_branch, 'deleted') } } + + it 'returns nil' do + expect(subject.target_sha).to be_nil + end + end + end + describe '#source_sha' do let(:last_branch_commit) { subject.source_project.repository.commit(subject.source_branch) } @@ -310,6 +320,18 @@ describe MergeRequest, models: true do let(:project) { create(:project) } let(:fork_project) { create(:project, forked_from_project: project) } + context 'when the target branch does not exist anymore' do + subject { create(:merge_request).tap { |mr| mr.update_attribute(:target_branch, 'deleted') } } + + it 'does not crash' do + expect{ subject.diverged_commits_count }.not_to raise_error + end + + it 'returns 0' do + expect(subject.diverged_commits_count).to eq(0) + end + end + context 'diverged on same repository' do subject(:merge_request_with_divergence) { create(:merge_request, :diverged, source_project: project, target_project: project) }