diff options
author | Douwe Maan <douwe@gitlab.com> | 2016-03-18 12:28:43 +0000 |
---|---|---|
committer | Rémy Coutable <remy@rymai.me> | 2016-03-18 21:22:25 +0100 |
commit | 5b82e15bbc4b4c4d0c9e7c2597b368d369c13a60 (patch) | |
tree | f495c1af17aa39e9899e993caeabf1aab1e026d8 | |
parent | ddb2de0957b3cf4c82a0e3a53e969e8b74e3b3dc (diff) | |
download | gitlab-ce-5b82e15bbc4b4c4d0c9e7c2597b368d369c13a60.tar.gz |
Merge branch 'fix-mr-source-sha' into 'master'
Fix MergeRequest#source_sha when there is no diff
Closes https://gitlab.com/gitlab-org/gitlab-ce/issues/14170
### Overview
This MR fixes an unhandled Exception when visiting the page of an open Merge Request without diff.
### Description
`MergeRequest#source_sha` is expected to return the sha of the source branch last commit. But when an open Merge Request has no diff (e.g. all commits have already been merged to the target branch), `merge_request.source_sha` incorrectly returns `nil`.
This was without consequences before – but since !2217 was merged (a few days ago), it makes `Gitlab::Git::Commit.between` raise an "Unexpected nil argument" exception. This can be reproduced when visiting the http://localhost:3000/gitlab-org/gitlab-test/merge_requests/2 page on a fresh local Gitlab setup.
This MR fixes the crash, by making sure that `source_sha` returns a
correct result even when there is no diff available. I also added tests.
@DouweM I believe you wrote most of this code in the first place ; does this looks correct to you, or is there a better way to resolve this issue maybe?
See merge request !3135
-rw-r--r-- | CHANGELOG | 1 | ||||
-rw-r--r-- | app/models/merge_request.rb | 6 | ||||
-rw-r--r-- | spec/factories/merge_requests.rb | 5 | ||||
-rw-r--r-- | spec/models/merge_request_spec.rb | 25 |
4 files changed, 36 insertions, 1 deletions
diff --git a/CHANGELOG b/CHANGELOG index 06a4555b801..74217e80bfe 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -39,6 +39,7 @@ v 8.6.0 (unreleased) - Fix bug where Bitbucket `closed` issues were imported as `opened` (Iuri de Silvio) - Don't show Issues/MRs from archived projects in Groups view - Fix wrong "iid of max iid" in Issuable sidebar for some merged MRs + - Fix empty source_sha on Merge Request when there is no diff (Pierre de La Morinerie) - 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 diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index 188325045e2..30a7bd47be7 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -520,7 +520,11 @@ class MergeRequest < ActiveRecord::Base end def source_sha - last_commit.try(:sha) + last_commit.try(:sha) || source_tip.try(:sha) + end + + def source_tip + source_branch && source_project.repository.commit(source_branch) end def fetch_ref diff --git a/spec/factories/merge_requests.rb b/spec/factories/merge_requests.rb index a9df5fa1d3a..e281e2f227b 100644 --- a/spec/factories/merge_requests.rb +++ b/spec/factories/merge_requests.rb @@ -51,6 +51,11 @@ FactoryGirl.define do trait :with_diffs do end + trait :without_diffs do + source_branch "improve/awesome" + target_branch "master" + end + trait :conflict do source_branch "feature_conflict" target_branch "feature" diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb index 633a16b59c2..654c71b6825 100644 --- a/spec/models/merge_request_spec.rb +++ b/spec/models/merge_request_spec.rb @@ -86,6 +86,31 @@ describe MergeRequest, models: true do end end + describe '#source_sha' do + let(:last_branch_commit) { subject.source_project.repository.commit(subject.source_branch) } + + context 'with diffs' do + subject { create(:merge_request, :with_diffs) } + it 'returns the sha of the source branch last commit' do + expect(subject.source_sha).to eq(last_branch_commit.sha) + end + end + + context 'without diffs' do + subject { create(:merge_request, :without_diffs) } + it 'returns the sha of the source branch last commit' do + expect(subject.source_sha).to eq(last_branch_commit.sha) + end + end + + context 'when the merge request is being created' do + subject { build(:merge_request, source_branch: nil, compare_commits: []) } + it 'returns nil' do + expect(subject.source_sha).to be_nil + end + end + end + describe '#to_reference' do it 'returns a String reference to the object' do expect(subject.to_reference).to eq "!#{subject.iid}" |