Merge branch 'fix-issue-2593' into 'master'

Fix Error 500 when comparing non-existing branches

Closes #2593

See merge request !1355

3 files changed, 36 insertions, 5 deletions

diff --git a/app/controllers/projects/compare_controller.rb b/app/controllers/projects/compare_controller.rb
index d9b3adae95b..d15004f93a6 100644
--- a/app/controllers/projects/compare_controller.rb
+++ b/app/controllers/projects/compare_controller.rb
@@ -16,10 +16,12 @@ class Projects::CompareController < Projects::ApplicationController
     compare_result = CompareService.new.
       execute(@project, head_ref, @project, base_ref)
 
-    @commits = compare_result.commits
-    @diffs = compare_result.diffs
-    @commit = @commits.last
-    @line_notes = []
+    if compare_result
+      @commits = compare_result.commits
+      @diffs = compare_result.diffs
+      @commit = @commits.last
+      @line_notes = []
+    end
   end
 
   def create
diff --git a/app/services/compare_service.rb b/app/services/compare_service.rb
index 70f642baaaa..bfe6a3dc4be 100644
--- a/app/services/compare_service.rb
+++ b/app/services/compare_service.rb
@@ -4,7 +4,10 @@ require 'securerandom'
 # and return Gitlab::CompareResult object that responds to commits and diffs
 class CompareService
   def execute(source_project, source_branch, target_project, target_branch)
-    source_sha = source_project.commit(source_branch).sha
+    source_commit = source_project.commit(source_branch)
+    return unless source_commit
+
+    source_sha = source_commit.sha
 
     # If compare with other project we need to fetch ref first
     unless target_project == source_project
diff --git a/spec/controllers/projects/compare_controller_spec.rb b/spec/controllers/projects/compare_controller_spec.rb
index b643b354073..2a447248b70 100644
--- a/spec/controllers/projects/compare_controller_spec.rb
+++ b/spec/controllers/projects/compare_controller_spec.rb
@@ -22,4 +22,30 @@ describe Projects::CompareController do
     expect(assigns(:diffs).length).to be >= 1
     expect(assigns(:commits).length).to be >= 1
   end
+
+  describe 'non-existent refs' do
+    it 'invalid source ref' do
+      get(:show,
+          namespace_id: project.namespace.to_param,
+          project_id: project.to_param,
+          from: 'non-existent',
+          to: ref_to)
+
+      expect(response).to be_success
+      expect(assigns(:diffs)).to eq([])
+      expect(assigns(:commits)).to eq([])
+    end
+
+    it 'invalid target ref' do
+      get(:show,
+          namespace_id: project.namespace.to_param,
+          project_id: project.to_param,
+          from: ref_from,
+          to: 'non-existent')
+
+      expect(response).to be_success
+      expect(assigns(:diffs)).to eq(nil)
+      expect(assigns(:commits)).to eq(nil)
+    end
+  end
 end