From 6987ec29b084ca842e46601965a60519fe96dc33 Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Thu, 22 Sep 2016 17:01:01 +0800
Subject: [PATCH 1/3] For empty merge_request_diff st_commits would be nil

Closes #22438
---
 app/models/merge_request_diff.rb  |  4 +++-
 spec/models/merge_request_spec.rb | 27 +++++++++++++++++++++------
 2 files changed, 24 insertions(+), 7 deletions(-)

diff --git a/app/models/merge_request_diff.rb b/app/models/merge_request_diff.rb
index 7362886e9f5..afa3611d044 100644
--- a/app/models/merge_request_diff.rb
+++ b/app/models/merge_request_diff.rb
@@ -120,8 +120,10 @@ class MergeRequestDiff < ActiveRecord::Base
   def commits_sha
     if @commits
       commits.map(&:sha)
-    else
+    elsif st_commits
       st_commits.map { |commit| commit[:id] }
+    else
+      []
     end
   end
 
diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb
index 433aba7747b..12df6adde44 100644
--- a/spec/models/merge_request_spec.rb
+++ b/spec/models/merge_request_spec.rb
@@ -517,7 +517,7 @@ describe MergeRequest, models: true do
 
     context 'with multiple irrelevant merge_request_diffs' do
       before do
-        subject.update(target_branch: 'markdown')
+        subject.update(target_branch: 'v1.0.0')
       end
 
       it_behaves_like 'returning pipelines with proper ordering'
@@ -544,13 +544,28 @@ describe MergeRequest, models: true do
       subject.merge_request_diffs.flat_map(&:commits).map(&:sha).uniq
     end
 
-    before do
-      subject.update(target_branch: 'markdown')
+    shared_examples 'returning all SHA' do
+      it 'returns all SHA from all merge_request_diffs' do
+        expect(subject.merge_request_diffs.size).to eq(2)
+        expect(subject.all_commits_sha).to eq(all_commits_sha)
+      end
     end
 
-    it 'returns all SHA from all merge_request_diffs' do
-      expect(subject.merge_request_diffs.size).to eq(2)
-      expect(subject.all_commits_sha).to eq(all_commits_sha)
+    context 'with a completely different branch' do
+      before do
+        subject.update(target_branch: 'v1.0.0')
+      end
+
+      it_behaves_like 'returning all SHA'
+    end
+
+    context 'with a branch having no difference' do
+      before do
+        subject.update(target_branch: 'v1.1.0')
+        subject.reload # make sure commits were not cached
+      end
+
+      it_behaves_like 'returning all SHA'
     end
   end
 

From 4ed23a3a5700f52236a676559203d56e6a3d0835 Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Thu, 22 Sep 2016 17:28:36 +0800
Subject: [PATCH 2/3] So that st_commits could never be nil

---
 app/models/merge_request_diff.rb | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/app/models/merge_request_diff.rb b/app/models/merge_request_diff.rb
index afa3611d044..36b8b70870b 100644
--- a/app/models/merge_request_diff.rb
+++ b/app/models/merge_request_diff.rb
@@ -30,6 +30,10 @@ class MergeRequestDiff < ActiveRecord::Base
     select(column_names - ['st_diffs'])
   end
 
+  def st_commits
+    super || []
+  end
+
   # Collect information about commits and diff from repository
   # and save it to the database as serialized data
   def save_git_content
@@ -83,7 +87,7 @@ class MergeRequestDiff < ActiveRecord::Base
   end
 
   def commits
-    @commits ||= load_commits(st_commits || [])
+    @commits ||= load_commits(st_commits)
   end
 
   def reload_commits
@@ -120,10 +124,8 @@ class MergeRequestDiff < ActiveRecord::Base
   def commits_sha
     if @commits
       commits.map(&:sha)
-    elsif st_commits
-      st_commits.map { |commit| commit[:id] }
     else
-      []
+      st_commits.map { |commit| commit[:id] }
     end
   end
 

From b0ce4ca11012388b1e4fbc1d928cbf4c0da66104 Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Thu, 22 Sep 2016 17:34:46 +0800
Subject: [PATCH 3/3] Add test for MergeRequestDiff#commits_sha, feedback:

https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/6470#note_15856755
---
 spec/models/merge_request_diff_spec.rb | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/spec/models/merge_request_diff_spec.rb b/spec/models/merge_request_diff_spec.rb
index e5b185dc3f6..530a7def553 100644
--- a/spec/models/merge_request_diff_spec.rb
+++ b/spec/models/merge_request_diff_spec.rb
@@ -64,5 +64,27 @@ describe MergeRequestDiff, models: true do
         end
       end
     end
+
+    describe '#commits_sha' do
+      shared_examples 'returning all commits SHA' do
+        it 'returns all commits SHA' do
+          commits_sha = subject.commits_sha
+
+          expect(commits_sha).to eq(subject.commits.map(&:sha))
+        end
+      end
+
+      context 'when commits were loaded' do
+        before do
+          subject.commits
+        end
+
+        it_behaves_like 'returning all commits SHA'
+      end
+
+      context 'when commits were not loaded' do
+        it_behaves_like 'returning all commits SHA'
+      end
+    end
   end
 end