Implement Repository#ambiguous_ref?

This implements Repository#ambiguous_ref? and checks if a ref is
ambiguous before trying to resolve the ref in Project#protected_for?

5 files changed, 67 insertions, 45 deletions

diff --git a/app/models/project.rb b/app/models/project.rb
index 2ba273c6f98..800c8afb49f 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -1737,10 +1737,10 @@ class Project < ActiveRecord::Base
   end
 
   def protected_for?(ref)
-    resolved_ref = repository.resolve_ref(ref)
-    return false unless resolved_ref
+    return false unless ref && !repository.ambiguous_ref?(ref)
 
-    ref_name = Gitlab::Git.ref_name(resolved_ref)
+    resolved_ref = repository.resolve_ref(ref)
+    ref_name = resolved_ref == ref ? Gitlab::Git.ref_name(resolved_ref) : ref
 
     if Gitlab::Git.branch_ref?(resolved_ref)
       ProtectedBranch.protected?(self, ref_name)
diff --git a/app/models/repository.rb b/app/models/repository.rb
index 221d01c5b44..9e01f7f0df5 100644
--- a/app/models/repository.rb
+++ b/app/models/repository.rb
@@ -182,15 +182,14 @@ class Repository
     tags.find { |tag| tag.name == name }
   end
 
-  def resolve_ref(ref)
-    tag_exists = tag_exists?(ref)
-    branch_exists = branch_exists?(ref)
+  def ambiguous_ref?(ref)
+    tag_exists?(ref) && branch_exists?(ref)
+  end
 
-    if tag_exists && branch_exists
-      nil
-    elsif tag_exists
+  def resolve_ref(ref)
+    if tag_exists?(ref)
       Gitlab::Git::TAG_REF_PREFIX + ref
-    elsif branch_exists
+    elsif branch_exists?(ref)
       Gitlab::Git::BRANCH_REF_PREFIX + ref
     else
       ref
diff --git a/lib/gitlab/ci/pipeline/chain/validate/repository.rb b/lib/gitlab/ci/pipeline/chain/validate/repository.rb
index 6e95c0717c6..09b3187569c 100644
--- a/lib/gitlab/ci/pipeline/chain/validate/repository.rb
+++ b/lib/gitlab/ci/pipeline/chain/validate/repository.rb
@@ -17,7 +17,7 @@ module Gitlab
                 return error('Commit not found')
               end
 
-              unless @command.project.repository.resolve_ref(@command.origin_ref)
+              if @command.project.repository.ambiguous_ref?(@command.origin_ref)
                 return error('Ref is ambiguous')
               end
             end
diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb
index 25560ddea8d..18ad69a1bda 100644
--- a/spec/models/project_spec.rb
+++ b/spec/models/project_spec.rb
@@ -2565,14 +2565,23 @@ describe Project do
   describe '#protected_for?' do
     let(:project) { create(:project, :repository) }
 
-    subject { project.protected_for?('ref') }
+    subject { project.protected_for?(ref) }
 
-    before do
-      allow(project.repository).to receive(:resolve_ref).and_return(ref)
+    context 'when ref is nil' do
+      let(:ref) { nil }
+
+      it 'returns false' do
+        is_expected.to be_falsey
+      end
     end
 
     context 'when ref is ambiguous' do
-      let(:ref) { nil }
+      let(:ref) { 'ref' }
+
+      before do
+        project.repository.add_branch(project.creator, ref, 'master')
+        project.repository.add_tag(project.creator, ref, 'master')
+      end
 
       it 'returns false' do
         is_expected.to be_falsey
@@ -2580,7 +2589,7 @@ describe Project do
     end
 
     context 'when the ref is not protected' do
-      let(:ref) { 'refs/heads/master' }
+      let(:ref) { 'master' }
 
       before do
         stub_application_setting(
@@ -2593,7 +2602,7 @@ describe Project do
     end
 
     context 'when the ref is a protected branch' do
-      let(:ref) { 'refs/heads/master' }
+      let(:ref) { 'master' }
 
       before do
         create(:protected_branch, name: 'master', project: project)
@@ -2605,7 +2614,7 @@ describe Project do
     end
 
     context 'when the ref is a protected tag' do
-      let(:ref) { 'refs/tags/v1.0.0' }
+      let(:ref) { 'v1.0.0' }
 
       before do
         create(:protected_tag, name: 'v1.0.0', project: project)
diff --git a/spec/models/repository_spec.rb b/spec/models/repository_spec.rb
index 3d6cf2cbc19..cdececd49e0 100644
--- a/spec/models/repository_spec.rb
+++ b/spec/models/repository_spec.rb
@@ -1005,7 +1005,36 @@ describe Repository do
     end
   end
 
+  describe '#ambiguous_ref?' do
+    let(:ref) { 'ref' }
+
+    subject { repository.ambiguous_ref?(ref) }
+
+    context 'when ref is ambiguous' do
+      before do
+        repository.add_tag(project.creator, ref, 'master')
+        repository.add_branch(project.creator, ref, 'master')
+      end
+
+      it 'should be true' do
+        is_expected.to eq(true)
+      end
+    end
+
+    context 'when ref is not ambiguous' do
+      before do
+        repository.add_tag(project.creator, ref, 'master')
+      end
+
+      it 'should be false' do
+        is_expected.to eq(false)
+      end
+    end
+  end
+
   describe '#resolve_ref' do
+    let(:ref) { 'ref' }
+
     subject { repository.resolve_ref(ref) }
 
     context 'when ref is full ref' do
@@ -1016,38 +1045,23 @@ describe Repository do
       end
     end
 
-    context 'when ref is a tag or branch name' do
-      let(:ref) { 'ref' }
-
-      context 'when ref is ambiguous' do
-        before do
-          repository.add_tag(project.creator, ref, 'master')
-          repository.add_branch(project.creator, ref, 'master')
-        end
-
-        it 'returns nil' do
-          is_expected.to eq(nil)
-        end
+    context 'when ref is tag name' do
+      before do
+        repository.add_tag(project.creator, ref, 'master')
       end
 
-      context 'when ref is tag name' do
-        before do
-          repository.add_tag(project.creator, ref, 'master')
-        end
-
-        it 'returns the tag ref' do
-          is_expected.to eq("refs/tags/#{ref}")
-        end
+      it 'returns the tag ref' do
+        is_expected.to eq("refs/tags/#{ref}")
       end
+    end
 
-      context 'when ref is branch name' do
-        before do
-          repository.add_branch(project.creator, ref, 'master')
-        end
+    context 'when ref is branch name' do
+      before do
+        repository.add_branch(project.creator, ref, 'master')
+      end
 
-        it 'returns the branch ref' do
-          is_expected.to eq("refs/heads/#{ref}")
-        end
+      it 'returns the branch ref' do
+        is_expected.to eq("refs/heads/#{ref}")
       end
     end
   end