From 94c80a716935bc0926e8f89891c9dcbd7a08c959 Mon Sep 17 00:00:00 2001
From: "j.seto" <jseto@gitlab.com>
Date: Tue, 30 May 2023 15:41:44 -0400
Subject: [PATCH 1/8] Update redirect and commit lookup code for controllers

A ref is ambiguous if a ref_type is not specified and it could be
interpreted as a tag or a branch.  git would prefer to use the tag
when it exists so we redirect to the tag.

Contributes to https://gitlab.com/gitlab-org/gitlab/-/issues/20526
---
 .../redirects_for_missing_path_on_tree.rb     |   4 +-
 app/controllers/projects/blob_controller.rb   |  14 +-
 app/controllers/projects/tree_controller.rb   |  34 +++-
 app/controllers/projects_controller.rb        |   4 +-
 .../development/redirect_with_ref_type.yml    |   8 +
 lib/extracts_ref.rb                           |   2 +-
 lib/extracts_ref/requested_ref.rb             |  63 +++++++
 .../projects/blob_controller_spec.rb          |  46 ++++--
 .../projects/tree_controller_spec.rb          | 152 ++++++++++++++---
 spec/controllers/projects_controller_spec.rb  | 156 +++++++++---------
 spec/lib/extracts_ref/requested_ref_spec.rb   | 110 ++++++++++++
 11 files changed, 467 insertions(+), 126 deletions(-)
 create mode 100644 config/feature_flags/development/redirect_with_ref_type.yml
 create mode 100644 lib/extracts_ref/requested_ref.rb
 create mode 100644 spec/lib/extracts_ref/requested_ref_spec.rb

diff --git a/app/controllers/concerns/redirects_for_missing_path_on_tree.rb b/app/controllers/concerns/redirects_for_missing_path_on_tree.rb
index 92574dfade9f2d..97c23a2cf3c80c 100644
--- a/app/controllers/concerns/redirects_for_missing_path_on_tree.rb
+++ b/app/controllers/concerns/redirects_for_missing_path_on_tree.rb
@@ -1,8 +1,8 @@
 # frozen_string_literal: true
 
 module RedirectsForMissingPathOnTree
-  def redirect_to_tree_root_for_missing_path(project, ref, path)
-    redirect_to project_tree_path(project, ref), notice: missing_path_on_ref(path, ref)
+  def redirect_to_tree_root_for_missing_path(project, ref, path, ref_type: nil)
+    redirect_to project_tree_path(project, ref, ref_type: ref_type), notice: missing_path_on_ref(path, ref)
   end
 
   private
diff --git a/app/controllers/projects/blob_controller.rb b/app/controllers/projects/blob_controller.rb
index 28393e1f365e61..a60cc5301e2a4e 100644
--- a/app/controllers/projects/blob_controller.rb
+++ b/app/controllers/projects/blob_controller.rb
@@ -160,6 +160,8 @@ def blob
   end
 
   def check_for_ambiguous_ref
+    return if Feature.enabled?(:redirect_with_ref_type, @project)
+
     @ref_type = ref_type
 
     if @ref_type == ExtractsRef::BRANCH_REF_TYPE && ambiguous_ref?(@project, @ref)
@@ -169,7 +171,17 @@ def check_for_ambiguous_ref
   end
 
   def commit
-    @commit ||= @repository.commit(@ref)
+    if Feature.enabled?(:redirect_with_ref_type, @project)
+      response = ::ExtractsRef::RequestedRef.new(@repository, ref_type: ref_type, ref: @ref).find
+      @commit = response[:commit]
+      @ref_type = response[:ref_type]
+
+      if response[:ambiguous]
+        return redirect_to(project_blob_path(@project, File.join(@ref_type ? @ref : @commit.id, @path), ref_type: @ref_type))
+      end
+    else
+      @commit ||= @repository.commit(@ref)
+    end
 
     return render_404 unless @commit
   end
diff --git a/app/controllers/projects/tree_controller.rb b/app/controllers/projects/tree_controller.rb
index c8f698d6193559..d2a820d93b00bd 100644
--- a/app/controllers/projects/tree_controller.rb
+++ b/app/controllers/projects/tree_controller.rb
@@ -12,6 +12,7 @@ class Projects::TreeController < Projects::ApplicationController
 
   before_action :require_non_empty_project, except: [:new, :create]
   before_action :assign_ref_vars
+  before_action :find_requested_ref, only: [:show]
   before_action :assign_dir_vars, only: [:create_dir]
   before_action :authorize_read_code!
   before_action :authorize_edit_tree!, only: [:create_dir]
@@ -28,18 +29,20 @@ class Projects::TreeController < Projects::ApplicationController
   def show
     return render_404 unless @commit
 
-    @ref_type = ref_type
-    if @ref_type == BRANCH_REF_TYPE && ambiguous_ref?(@project, @ref)
-      branch = @project.repository.find_branch(@ref)
-      if branch
-        redirect_to project_tree_path(@project, branch.target)
-        return
+    unless Feature.enabled?(:redirect_with_ref_type, @project)
+      @ref_type = ref_type
+      if @ref_type == BRANCH_REF_TYPE && ambiguous_ref?(@project, @ref)
+        branch = @project.repository.find_branch(@ref)
+        if branch
+          redirect_to project_tree_path(@project, branch.target)
+          return
+        end
       end
     end
 
     if tree.entries.empty?
       if @repository.blob_at(@commit.id, @path)
-        redirect_to project_blob_path(@project, File.join(@ref, @path))
+        redirect_to project_blob_path(@project, File.join(@ref, @path), ref_type: @ref_type)
       elsif @path.present?
         redirect_to_tree_root_for_missing_path(@project, @ref, @path)
       end
@@ -59,6 +62,23 @@ def create_dir
 
   private
 
+  def find_requested_ref
+    return unless Feature.enabled?(:redirect_with_ref_type, @project)
+
+    @ref_type = ref_type
+    if @ref_type.present?
+      @tree = @repo.tree(@ref, @path, ref_type: @ref_type)
+    else
+      response = ExtractsPath::RequestedRef.new(@repository, ref_type: nil, ref: @ref).find
+      @ref_type = response[:ref_type]
+      @commit = response[:commit]
+
+      if response[:ambiguous]
+        redirect_to(project_tree_path(@project, File.join(@ref_type ? @ref : @commit.id, @path), ref_type: @ref_type))
+      end
+    end
+  end
+
   def redirect_renamed_default_branch?
     action_name == 'show'
   end
diff --git a/app/controllers/projects_controller.rb b/app/controllers/projects_controller.rb
index 71a34a40bd0724..bc4831d7772b6b 100644
--- a/app/controllers/projects_controller.rb
+++ b/app/controllers/projects_controller.rb
@@ -173,7 +173,9 @@ def show
       flash.now[:alert] = _("Project '%{project_name}' queued for deletion.") % { project_name: @project.name }
     end
 
-    if ambiguous_ref?(@project, @ref)
+    if Feature.enabled?(:redirect_with_ref_type, @project)
+      @ref_type = 'heads'
+    elsif ambiguous_ref?(@project, @ref)
       branch = @project.repository.find_branch(@ref)
 
       # The files view would render a ref other than the default branch
diff --git a/config/feature_flags/development/redirect_with_ref_type.yml b/config/feature_flags/development/redirect_with_ref_type.yml
new file mode 100644
index 00000000000000..54976df0b5b5bf
--- /dev/null
+++ b/config/feature_flags/development/redirect_with_ref_type.yml
@@ -0,0 +1,8 @@
+---
+name: redirect_with_ref_type
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/122237
+rollout_issue_url:
+milestone: '16.1'
+type: development
+group: group::source code
+default_enabled: false
diff --git a/lib/extracts_ref.rb b/lib/extracts_ref.rb
index 49ec564eb8d64a..2a48b66bb5c6e4 100644
--- a/lib/extracts_ref.rb
+++ b/lib/extracts_ref.rb
@@ -100,7 +100,7 @@ def assign_ref_vars
   # rubocop:enable Gitlab/ModuleWithInstanceVariables
 
   def tree
-    @tree ||= @repo.tree(@commit.id, @path) # rubocop:disable Gitlab/ModuleWithInstanceVariables
+    @tree ||= @repo.tree(@commit.id, @path, ref_type: ref_type) # rubocop:disable Gitlab/ModuleWithInstanceVariables
   end
 
   def extract_ref_path
diff --git a/lib/extracts_ref/requested_ref.rb b/lib/extracts_ref/requested_ref.rb
new file mode 100644
index 00000000000000..ff9ef9c0ff9a4e
--- /dev/null
+++ b/lib/extracts_ref/requested_ref.rb
@@ -0,0 +1,63 @@
+# frozen_string_literal: true
+
+module ExtractsRef
+  class RequestedRef
+    include Gitlab::Utils::StrongMemoize
+
+    SYMBOLIC_REF_PREFIX = %r{((refs/)?(heads|tags)/)+}
+    def initialize(repository, ref_type:, ref:)
+      @ref_type = ref_type
+      @ref = ref
+      @repository = repository
+    end
+
+    attr_reader :repository, :ref_type, :ref
+
+    def find
+      case ref_type
+      when 'tags'
+        { ref_type: ref_type, commit: tag }
+      when 'heads'
+        { ref_type: ref_type, commit: branch }
+      else
+        commit_without_ref_type
+      end
+    end
+
+    private
+
+    def commit_without_ref_type
+      return { ref_type: nil, commit: nil } if commit.nil?
+
+      # ref is probably complete 40 character sha
+      return { ref_type: nil, commit: commit } if commit.id == ref
+
+      if tag.present?
+        { ref_type: 'tags', commit: tag, ambiguous: branch.present? }
+      elsif branch.present?
+        { ref_type: 'heads', commit: branch }
+      elsif ref =~ SYMBOLIC_REF_PREFIX
+        { ref_type: nil, commit: commit, ambiguous: true }
+      else
+        { ref_type: nil, commit: commit }
+      end
+    end
+
+    def commit
+      repository.commit(ref)
+    end
+    strong_memoize_attr :commit
+
+    def tag
+      raw_commit = repository.find_tag(ref)&.dereferenced_target
+      ::Commit.new(raw_commit, repository.container) if raw_commit
+    end
+    strong_memoize_attr :tag
+
+    def branch
+      raw_commit = repository.find_branch(ref)&.dereferenced_target
+      ::Commit.new(raw_commit, repository.container) if raw_commit
+    end
+    strong_memoize_attr :branch
+  end
+end
diff --git a/spec/controllers/projects/blob_controller_spec.rb b/spec/controllers/projects/blob_controller_spec.rb
index b07cb7a228dbac..09bb2d698581b4 100644
--- a/spec/controllers/projects/blob_controller_spec.rb
+++ b/spec/controllers/projects/blob_controller_spec.rb
@@ -9,11 +9,18 @@
   let(:previous_default_branch) { nil }
 
   describe "GET show" do
-    let(:params) { { namespace_id: project.namespace, project_id: project, id: id } }
+    let(:params) { { namespace_id: project.namespace, project_id: project, id: id, ref_type: ref_type } }
+    let(:ref_type) { 'heads' }
     let(:request) do
       get(:show, params: params)
     end
 
+    let(:redirect_with_ref_type) { true }
+
+    before do
+      stub_feature_flags(redirect_with_ref_type: redirect_with_ref_type)
+    end
+
     render_views
 
     context 'with file path' do
@@ -28,21 +35,34 @@
         let(:ref) { 'ambiguous_ref' }
         let(:path) { 'README.md' }
         let(:id) { "#{ref}/#{path}" }
-        let(:params) { { namespace_id: project.namespace, project_id: project, id: id, ref_type: ref_type } }
 
-        context 'and explicitly requesting a branch' do
-          let(:ref_type) { 'heads' }
+        context 'and the redirect_with_ref_type flag is disabled' do
+          let(:redirect_with_ref_type) { false }
+
+          context 'and explicitly requesting a branch' do
+            let(:ref_type) { 'heads' }
+
+            it 'redirects to blob#show with sha for the branch' do
+              expect(response).to redirect_to(project_blob_path(project, "#{RepoHelpers.another_sample_commit.id}/#{path}"))
+            end
+          end
+
+          context 'and explicitly requesting a tag' do
+            let(:ref_type) { 'tags' }
 
-          it 'redirects to blob#show with sha for the branch' do
-            expect(response).to redirect_to(project_blob_path(project, "#{RepoHelpers.another_sample_commit.id}/#{path}"))
+            it 'responds with success' do
+              expect(response).to be_ok
+            end
           end
         end
 
-        context 'and explicitly requesting a tag' do
-          let(:ref_type) { 'tags' }
+        context 'and the redirect_with_ref_type flag is enabled' do
+          context 'when the ref_type is nil' do
+            let(:ref_type) { nil }
 
-          it 'responds with success' do
-            expect(response).to be_ok
+            it 'redirects to the tag' do
+              expect(response).to redirect_to(project_blob_path(project, id, ref_type: 'tags'))
+            end
           end
         end
       end
@@ -100,7 +120,7 @@
         let(:id) { 'master/README.md' }
 
         before do
-          get :show, params: { namespace_id: project.namespace, project_id: project, id: id }, format: :json
+          get :show, params: params, format: :json
         end
 
         it do
@@ -114,7 +134,7 @@
         let(:id) { 'master/README.md' }
 
         before do
-          get :show, params: { namespace_id: project.namespace, project_id: project, id: id, viewer: 'none' }, format: :json
+          get :show, params: { namespace_id: project.namespace, project_id: project, id: id, ref_type: 'heads', viewer: 'none' }, format: :json
         end
 
         it do
@@ -127,7 +147,7 @@
 
     context 'with tree path' do
       before do
-        get :show, params: { namespace_id: project.namespace, project_id: project, id: id }
+        get :show, params: params
 
         controller.instance_variable_set(:@blob, nil)
       end
diff --git a/spec/controllers/projects/tree_controller_spec.rb b/spec/controllers/projects/tree_controller_spec.rb
index 61998d516e8e71..5573ed9e1c876a 100644
--- a/spec/controllers/projects/tree_controller_spec.rb
+++ b/spec/controllers/projects/tree_controller_spec.rb
@@ -6,6 +6,7 @@
   let(:project) { create(:project, :repository, previous_default_branch: previous_default_branch) }
   let(:previous_default_branch) { nil }
   let(:user) { create(:user) }
+  let(:redirect_with_ref_type) { true }
 
   before do
     sign_in(user)
@@ -17,10 +18,14 @@
   describe "GET show" do
     let(:params) do
       {
-        namespace_id: project.namespace.to_param, project_id: project, id: id
+        namespace_id: project.namespace.to_param, project_id: project, id: id, ref_type: ref_type
       }
     end
 
+    let(:request) { get :show, params: params }
+
+    let(:ref_type) { 'heads' }
+
     # Make sure any errors accessing the tree in our views bubble up to this spec
     render_views
 
@@ -28,26 +33,77 @@
       expect(::Gitlab::GitalyClient).to receive(:allow_ref_name_caching).and_call_original
       project.repository.add_tag(project.creator, 'ambiguous_ref', RepoHelpers.sample_commit.id)
       project.repository.add_branch(project.creator, 'ambiguous_ref', RepoHelpers.another_sample_commit.id)
-      get :show, params: params
+
+      stub_feature_flags(redirect_with_ref_type: redirect_with_ref_type)
     end
 
-    context 'when the ref is ambiguous' do
-      let(:id) { 'ambiguous_ref' }
-      let(:params) { { namespace_id: project.namespace, project_id: project, id: id, ref_type: ref_type } }
+    context 'when the redirect_with_ref_type flag is disabled' do
+      let(:redirect_with_ref_type) { false }
+
+      context 'when there is a ref and tag with the same name' do
+        let(:id) { 'ambiguous_ref' }
+        let(:ref_type) { nil }
+        let(:params) { { namespace_id: project.namespace, project_id: project, id: id, ref_type: ref_type } }
 
-      context 'and explicitly requesting a branch' do
-        let(:ref_type) { 'heads' }
+        context 'and explicitly requesting a branch' do
+          let(:ref_type) { 'heads' }
 
-        it 'redirects to blob#show with sha for the branch' do
-          expect(response).to redirect_to(project_tree_path(project, RepoHelpers.another_sample_commit.id))
+          it 'redirects to blob#show with sha for the branch' do
+            request
+            expect(response).to redirect_to(project_tree_path(project, RepoHelpers.another_sample_commit.id))
+          end
+        end
+
+        context 'and explicitly requesting a tag' do
+          let(:ref_type) { 'tags' }
+
+          it 'responds with success' do
+            request
+            expect(response).to be_ok
+          end
         end
       end
+    end
 
-      context 'and explicitly requesting a tag' do
-        let(:ref_type) { 'tags' }
+    describe 'delegating to ExtractsRef::RequestedRef' do
+      context 'when there is a ref and tag with the same name' do
+        let(:id) { 'ambiguous_ref' }
+        let(:params) { { namespace_id: project.namespace, project_id: project, id: id, ref_type: ref_type } }
 
-        it 'responds with success' do
-          expect(response).to be_ok
+        let(:requested_ref_double) { ExtractsRef::RequestedRef.new(project.repository, ref_type: ref_type, ref: id) }
+
+        before do
+          allow(ExtractsRef::RequestedRef).to receive(:new).with(kind_of(Repository), ref_type: ref_type, ref: id).and_return(requested_ref_double)
+        end
+
+        context 'and explicitly requesting a branch' do
+          let(:ref_type) { 'heads' }
+
+          it 'finds the branch' do
+            expect(requested_ref_double).not_to receive(:find)
+            request
+            expect(response).to be_ok
+          end
+        end
+
+        context 'and explicitly requesting a tag' do
+          let(:ref_type) { 'tags' }
+
+          it 'finds the tag' do
+            expect(requested_ref_double).not_to receive(:find)
+            request
+            expect(response).to be_ok
+          end
+        end
+
+        context 'and not specifying a ref_type' do
+          let(:ref_type) { nil }
+
+          it 'finds the tags and redirects' do
+            expect(requested_ref_double).to receive(:find).and_call_original
+            request
+            expect(subject).to redirect_to("/#{project.full_path}/-/tree/#{id}/?ref_type=tags")
+          end
         end
       end
     end
@@ -55,19 +111,26 @@
     context "valid branch, no path" do
       let(:id) { 'master' }
 
-      it { is_expected.to respond_with(:success) }
+      it 'responds with success' do
+        request
+        expect(response).to be_ok
+      end
     end
 
     context "valid branch, valid path" do
       let(:id) { 'master/encoding/' }
 
-      it { is_expected.to respond_with(:success) }
+      it 'responds with success' do
+        request
+        expect(response).to be_ok
+      end
     end
 
     context "valid branch, invalid path" do
       let(:id) { 'master/invalid-path/' }
 
       it 'redirects' do
+        request
         expect(subject)
             .to redirect_to("/#{project.full_path}/-/tree/master")
       end
@@ -76,54 +139,91 @@
     context "invalid branch, valid path" do
       let(:id) { 'invalid-branch/encoding/' }
 
-      it { is_expected.to respond_with(:not_found) }
+      it 'responds with not_found' do
+        request
+        expect(subject).to respond_with(:not_found)
+      end
     end
 
     context "renamed default branch, valid file" do
       let(:id) { 'old-default-branch/encoding/' }
       let(:previous_default_branch) { 'old-default-branch' }
 
-      it { is_expected.to redirect_to("/#{project.full_path}/-/tree/#{project.default_branch}/encoding/") }
+      it 'redirects' do
+        request
+        expect(subject).to redirect_to("/#{project.full_path}/-/tree/#{project.default_branch}/encoding/")
+      end
     end
 
     context "renamed default branch, invalid file" do
       let(:id) { 'old-default-branch/invalid-path/' }
       let(:previous_default_branch) { 'old-default-branch' }
 
-      it { is_expected.to redirect_to("/#{project.full_path}/-/tree/#{project.default_branch}/invalid-path/") }
+      it 'redirects' do
+        request
+        expect(subject).to redirect_to("/#{project.full_path}/-/tree/#{project.default_branch}/invalid-path/")
+      end
     end
 
     context "valid empty branch, invalid path" do
       let(:id) { 'empty-branch/invalid-path/' }
 
       it 'redirects' do
-        expect(subject)
-            .to redirect_to("/#{project.full_path}/-/tree/empty-branch")
+        request
+        expect(subject).to redirect_to("/#{project.full_path}/-/tree/empty-branch")
       end
     end
 
     context "valid empty branch" do
       let(:id) { 'empty-branch' }
 
-      it { is_expected.to respond_with(:success) }
+      it 'responds with success' do
+        request
+        expect(response).to be_ok
+      end
     end
 
     context "invalid SHA commit ID" do
       let(:id) { 'ff39438/.gitignore' }
 
-      it { is_expected.to respond_with(:not_found) }
+      it 'responds with not_found' do
+        request
+        expect(subject).to respond_with(:not_found)
+      end
     end
 
     context "valid SHA commit ID" do
+      let(:ref_type) { nil }
       let(:id) { '6d39438' }
 
-      it { is_expected.to respond_with(:success) }
+      it 'responds with success' do
+        request
+        expect(response).to be_ok
+      end
+
+      context 'and there is a tag with the same name' do
+        before do
+          project.repository.add_tag(project.creator, id, RepoHelpers.sample_commit.id)
+        end
+
+        it 'responds with success' do
+          request
+
+          # This uses the tag
+          # TODO: Should we redirect in this case?
+          expect(response).to be_ok
+        end
+      end
     end
 
     context "valid SHA commit ID with path" do
+      let(:ref_type) { nil }
       let(:id) { '6d39438/.gitignore' }
 
-      it { expect(response).to have_gitlab_http_status(:found) }
+      it 'responds with found' do
+        request
+        expect(response).to have_gitlab_http_status(:found)
+      end
     end
   end
 
@@ -149,7 +249,7 @@
 
     before do
       get :show, params: {
-        namespace_id: project.namespace.to_param, project_id: project, id: id
+        namespace_id: project.namespace.to_param, project_id: project, id: id, ref_type: 'heads'
       }
     end
 
@@ -157,7 +257,7 @@
       let(:id) { 'master/README.md' }
 
       it 'redirects' do
-        redirect_url = "/#{project.full_path}/-/blob/master/README.md"
+        redirect_url = "/#{project.full_path}/-/blob/master/README.md?ref_type=heads"
         expect(subject).to redirect_to(redirect_url)
       end
     end
diff --git a/spec/controllers/projects_controller_spec.rb b/spec/controllers/projects_controller_spec.rb
index 6adddccfda7aed..46913cfa6496ca 100644
--- a/spec/controllers/projects_controller_spec.rb
+++ b/spec/controllers/projects_controller_spec.rb
@@ -164,107 +164,113 @@ def get_activity(project)
           end
         end
 
-        context 'when there is a tag with the same name as the default branch' do
-          let_it_be(:tagged_project) { create(:project, :public, :custom_repo, files: ['somefile']) }
-          let(:tree_with_default_branch) do
-            branch = tagged_project.repository.find_branch(tagged_project.default_branch)
-            project_tree_path(tagged_project, branch.target)
-          end
-
+        context 'when redirect_with_ref_type is disabled' do
           before do
-            tagged_project.repository.create_file(
-              tagged_project.creator,
-              'file_for_tag',
-              'content for file',
-              message: "Automatically created file",
-              branch_name: 'branch-to-tag'
-            )
-
-            tagged_project.repository.add_tag(
-              tagged_project.creator,
-              tagged_project.default_branch, # tag name
-              'branch-to-tag' # target
-            )
-          end
-
-          it 'redirects to tree view for the default branch' do
-            get :show, params: { namespace_id: tagged_project.namespace, id: tagged_project }
-            expect(response).to redirect_to(tree_with_default_branch)
-          end
-        end
-
-        context 'when the default branch name is ambiguous' do
-          let_it_be(:project_with_default_branch) do
-            create(:project, :public, :custom_repo, files: ['somefile'])
+            stub_feature_flags(redirect_with_ref_type: false)
           end
 
-          shared_examples 'ambiguous ref redirects' do
-            let(:project) { project_with_default_branch }
-            let(:branch_ref) { "refs/heads/#{ref}" }
-            let(:repo) { project.repository }
+          context 'when there is a tag with the same name as the default branch' do
+            let_it_be(:tagged_project) { create(:project, :public, :custom_repo, files: ['somefile']) }
+            let(:tree_with_default_branch) do
+              branch = tagged_project.repository.find_branch(tagged_project.default_branch)
+              project_tree_path(tagged_project, branch.target)
+            end
 
             before do
-              repo.create_branch(branch_ref, 'master')
-              repo.change_head(ref)
+              tagged_project.repository.create_file(
+                tagged_project.creator,
+                'file_for_tag',
+                'content for file',
+                message: "Automatically created file",
+                branch_name: 'branch-to-tag'
+              )
+
+              tagged_project.repository.add_tag(
+                tagged_project.creator,
+                tagged_project.default_branch, # tag name
+                'branch-to-tag' # target
+              )
             end
 
-            after do
-              repo.change_head('master')
-              repo.delete_branch(branch_ref)
+            it 'redirects to tree view for the default branch' do
+              get :show, params: { namespace_id: tagged_project.namespace, id: tagged_project }
+              expect(response).to redirect_to(tree_with_default_branch)
             end
+          end
 
-            subject do
-              get(
-                :show,
-                params: {
-                  namespace_id: project.namespace,
-                  id: project
-                }
-              )
+          context 'when the default branch name is ambiguous' do
+            let_it_be(:project_with_default_branch) do
+              create(:project, :public, :custom_repo, files: ['somefile'])
             end
 
-            context 'when there is no conflicting ref' do
-              let(:other_ref) { 'non-existent-ref' }
+            shared_examples 'ambiguous ref redirects' do
+              let(:project) { project_with_default_branch }
+              let(:branch_ref) { "refs/heads/#{ref}" }
+              let(:repo) { project.repository }
 
-              it { is_expected.to have_gitlab_http_status(:ok) }
-            end
+              before do
+                repo.create_branch(branch_ref, 'master')
+                repo.change_head(ref)
+              end
+
+              after do
+                repo.change_head('master')
+                repo.delete_branch(branch_ref)
+              end
 
-            context 'and that other ref exists' do
-              let(:other_ref) { 'master' }
+              subject do
+                get(
+                  :show,
+                  params: {
+                    namespace_id: project.namespace,
+                    id: project
+                  }
+                )
+              end
+
+              context 'when there is no conflicting ref' do
+                let(:other_ref) { 'non-existent-ref' }
 
-              let(:project_default_root_tree_path) do
-                sha = repo.find_branch(project.default_branch).target
-                project_tree_path(project, sha)
+                it { is_expected.to have_gitlab_http_status(:ok) }
               end
 
-              it 'redirects to tree view for the default branch' do
-                is_expected.to redirect_to(project_default_root_tree_path)
+              context 'and that other ref exists' do
+                let(:other_ref) { 'master' }
+
+                let(:project_default_root_tree_path) do
+                  sha = repo.find_branch(project.default_branch).target
+                  project_tree_path(project, sha)
+                end
+
+                it 'redirects to tree view for the default branch' do
+                  is_expected.to redirect_to(project_default_root_tree_path)
+                end
               end
             end
-          end
 
-          context 'when ref starts with ref/heads/' do
-            let(:ref) { "refs/heads/#{other_ref}" }
+            context 'when ref starts with ref/heads/' do
+              let(:ref) { "refs/heads/#{other_ref}" }
 
-            include_examples 'ambiguous ref redirects'
-          end
+              include_examples 'ambiguous ref redirects'
+            end
 
-          context 'when ref starts with ref/tags/' do
-            let(:ref) { "refs/tags/#{other_ref}" }
+            context 'when ref starts with ref/tags/' do
+              let(:ref) { "refs/tags/#{other_ref}" }
 
-            include_examples 'ambiguous ref redirects'
-          end
+              include_examples 'ambiguous ref redirects'
+            end
 
-          context 'when ref starts with heads/' do
-            let(:ref) { "heads/#{other_ref}" }
+            context 'when ref starts with heads/' do
+              let(:ref) { "heads/#{other_ref}" }
 
-            include_examples 'ambiguous ref redirects'
-          end
+              include_examples 'ambiguous ref redirects'
+            end
 
-          context 'when ref starts with tags/' do
-            let(:ref) { "tags/#{other_ref}" }
+            context 'when ref starts with tags/' do
+              let(:ref) { "tags/#{other_ref}" }
 
-            include_examples 'ambiguous ref redirects'
+              include_examples 'ambiguous ref redirects'
+            end
           end
         end
       end
diff --git a/spec/lib/extracts_ref/requested_ref_spec.rb b/spec/lib/extracts_ref/requested_ref_spec.rb
new file mode 100644
index 00000000000000..a6e77613f1ad8a
--- /dev/null
+++ b/spec/lib/extracts_ref/requested_ref_spec.rb
@@ -0,0 +1,110 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe ExtractsRef::RequestedRef, feature_category: :source_code_management do
+  describe '#find' do
+    subject { described_class.new(project.repository, ref_type: ref_type, ref: ref).find }
+
+    let_it_be(:project) { create(:project, :repository) }
+    let(:ref_type) { nil }
+    let(:existing_tag_name) { 'v1.0.0' }
+    let(:existing_branch_name) { 'feature' }
+
+    shared_examples 'RequestedRef when ref_type is specified' do
+      context 'when ref_type is heads' do
+        let(:ref_type) { 'heads' }
+
+        it 'returns the branch commit' do
+          expect(subject[:ref_type]).to eq('heads')
+          expect(subject[:commit].id).to eq(project.repository.find_branch(ref)&.dereferenced_target&.id)
+        end
+      end
+
+      context 'when ref_type is tags' do
+        let(:ref_type) { 'tags' }
+
+        it 'returns the tag commit' do
+          expect(subject[:ref_type]).to eq('tags')
+          expect(subject[:commit].id).to eq(project.repository.find_tag(ref)&.dereferenced_target&.id)
+        end
+      end
+    end
+
+    context 'when the ref is the sha for a commit' do
+      let(:ref) { '570e7b2abdd848b95f2f578043fc23bd6f6fd24d' }
+
+      before(:all) do
+        project.repository.create_branch('570e7b2abdd848b95f2f578043fc23bd6f6fd24d')
+        project.repository.add_tag(project.owner, '570e7b2abdd848b95f2f578043fc23bd6f6fd24d', 'master')
+      end
+
+      it_behaves_like 'RequestedRef when ref_type is specified'
+
+      it 'returns the commit' do
+        expect(subject[:ref_type]).to be_nil
+        expect(subject[:commit].id).to eq(project.repository.commit(ref).id)
+      end
+    end
+
+    context 'when ref is for a tag' do
+      let(:ref) { existing_tag_name }
+
+      it 'returns the tag commit' do
+        expect(subject[:ref_type]).to eq('tags')
+        expect(subject[:commit].id).to eq(project.repository.find_tag(ref)&.dereferenced_target&.id)
+      end
+
+      context 'and there is a branch with the same name' do
+        before do
+          project.repository.create_branch(existing_tag_name)
+        end
+
+        it_behaves_like 'RequestedRef when ref_type is specified'
+
+        it 'returns the tag commit' do
+          expect(subject[:ref_type]).to eq('tags')
+          expect(subject[:commit].id).to eq(project.repository.find_tag(ref)&.dereferenced_target&.id)
+          expect(subject[:ambiguous]).to be_truthy
+        end
+      end
+    end
+
+    context 'when ref is only for a branch' do
+      let(:ref) { existing_branch_name }
+
+      it 'returns the tag commit' do
+        expect(subject[:ref_type]).to eq('heads')
+        expect(subject[:commit].id).to eq(project.repository.find_branch(ref)&.dereferenced_target&.id)
+      end
+    end
+
+    context 'when ref is an abbreviated commit sha' do
+      let(:ref) { '570e7b2abdd848b95f2f578043fc23bd6f6fd24d'.first(8) }
+
+      it 'returns the commit' do
+        expect(subject[:ref_type]).to be_nil
+        expect(subject[:commit].id).to eq(project.repository.commit('570e7b2abdd848b95f2f578043fc23bd6f6fd24d').id)
+      end
+    end
+
+    context 'when ref does not exist' do
+      let(:ref) { SecureRandom.uuid }
+
+      it 'returns the commit' do
+        expect(subject[:ref_type]).to be_nil
+        expect(subject[:commit]).to be_nil
+      end
+    end
+
+    context 'when ref is symbolic' do
+      let(:ref) { "heads/#{existing_branch_name}" }
+
+      it 'returns the commit' do
+        expect(subject[:ref_type]).to be_nil
+        expect(subject[:commit].id).to eq(project.repository.find_branch(existing_branch_name)&.dereferenced_target&.id)
+        expect(subject[:ambiguous]).to be_truthy
+      end
+    end
+  end
+end
-- 
GitLab


From 641a29e66cbb10dedf2dd837dc4cbde09034fb6e Mon Sep 17 00:00:00 2001
From: "j.seto" <jseto@gitlab.com>
Date: Wed, 21 Jun 2023 11:25:27 -0400
Subject: [PATCH 2/8] Refactor controller tests

---
 .../projects/blob_controller_spec.rb          | 36 +++++++----
 .../projects/tree_controller_spec.rb          | 61 ++++++++++---------
 2 files changed, 56 insertions(+), 41 deletions(-)

diff --git a/spec/controllers/projects/blob_controller_spec.rb b/spec/controllers/projects/blob_controller_spec.rb
index 09bb2d698581b4..cbce8a9e14982a 100644
--- a/spec/controllers/projects/blob_controller_spec.rb
+++ b/spec/controllers/projects/blob_controller_spec.rb
@@ -5,12 +5,11 @@
 RSpec.describe Projects::BlobController, feature_category: :source_code_management do
   include ProjectForksHelper
 
-  let(:project) { create(:project, :public, :repository, previous_default_branch: previous_default_branch) }
-  let(:previous_default_branch) { nil }
+  let_it_be(:project) { create(:project, :public, :repository) }
 
   describe "GET show" do
     let(:params) { { namespace_id: project.namespace, project_id: project, id: id, ref_type: ref_type } }
-    let(:ref_type) { 'heads' }
+    let(:ref_type) { nil }
     let(:request) do
       get(:show, params: params)
     end
@@ -31,6 +30,11 @@
         request
       end
 
+      after do
+        project.repository.rm_tag(project.creator, 'ambiguous_ref')
+        project.repository.rm_branch(project.creator, 'ambiguous_ref')
+      end
+
       context 'when the ref is ambiguous' do
         let(:ref) { 'ambiguous_ref' }
         let(:path) { 'README.md' }
@@ -88,18 +92,22 @@
         it { is_expected.to respond_with(:not_found) }
       end
 
-      context "renamed default branch, valid file" do
-        let(:id) { 'old-default-branch/README.md' }
-        let(:previous_default_branch) { 'old-default-branch' }
+      context 'when default branch was renamed' do
+        let(:project) { create(:project, :public, :repository, previous_default_branch: previous_default_branch) }
 
-        it { is_expected.to redirect_to("/#{project.full_path}/-/blob/#{project.default_branch}/README.md") }
-      end
+        context "renamed default branch, valid file" do
+          let(:id) { 'old-default-branch/README.md' }
+          let(:previous_default_branch) { 'old-default-branch' }
+
+          it { is_expected.to redirect_to("/#{project.full_path}/-/blob/#{project.default_branch}/README.md") }
+        end
 
-      context "renamed default branch, invalid file" do
-        let(:id) { 'old-default-branch/invalid-path.rb' }
-        let(:previous_default_branch) { 'old-default-branch' }
+        context "renamed default branch, invalid file" do
+          let(:id) { 'old-default-branch/invalid-path.rb' }
+          let(:previous_default_branch) { 'old-default-branch' }
 
-        it { is_expected.to redirect_to("/#{project.full_path}/-/blob/#{project.default_branch}/invalid-path.rb") }
+          it { is_expected.to redirect_to("/#{project.full_path}/-/blob/#{project.default_branch}/invalid-path.rb") }
+        end
       end
 
       context "binary file" do
@@ -434,6 +442,10 @@ def blob_after_edit_path
       let(:after_delete_path) { project_tree_path(project, 'master/files') }
 
       it 'redirects to the sub directory' do
+        expect_next_instance_of(Files::DeleteService) do |instance|
+          expect(instance).to receive(:execute).and_return({ status: :success })
+        end
+
         delete :destroy, params: default_params
 
         expect(response).to redirect_to(after_delete_path)
diff --git a/spec/controllers/projects/tree_controller_spec.rb b/spec/controllers/projects/tree_controller_spec.rb
index 5573ed9e1c876a..b98fce30c1698f 100644
--- a/spec/controllers/projects/tree_controller_spec.rb
+++ b/spec/controllers/projects/tree_controller_spec.rb
@@ -3,8 +3,7 @@
 require 'spec_helper'
 
 RSpec.describe Projects::TreeController, feature_category: :source_code_management do
-  let(:project) { create(:project, :repository, previous_default_branch: previous_default_branch) }
-  let(:previous_default_branch) { nil }
+  let_it_be(:project) { create(:project, :repository) }
   let(:user) { create(:user) }
   let(:redirect_with_ref_type) { true }
 
@@ -24,7 +23,7 @@
 
     let(:request) { get :show, params: params }
 
-    let(:ref_type) { 'heads' }
+    let(:ref_type) { nil }
 
     # Make sure any errors accessing the tree in our views bubble up to this spec
     render_views
@@ -37,12 +36,16 @@
       stub_feature_flags(redirect_with_ref_type: redirect_with_ref_type)
     end
 
+    after do
+      project.repository.rm_tag(project.creator, 'ambiguous_ref')
+      project.repository.rm_branch(project.creator, 'ambiguous_ref')
+    end
+
     context 'when the redirect_with_ref_type flag is disabled' do
       let(:redirect_with_ref_type) { false }
 
       context 'when there is a ref and tag with the same name' do
         let(:id) { 'ambiguous_ref' }
-        let(:ref_type) { nil }
         let(:params) { { namespace_id: project.namespace, project_id: project, id: id, ref_type: ref_type } }
 
         context 'and explicitly requesting a branch' do
@@ -76,6 +79,14 @@
           allow(ExtractsRef::RequestedRef).to receive(:new).with(kind_of(Repository), ref_type: ref_type, ref: id).and_return(requested_ref_double)
         end
 
+        context 'and not specifying a ref_type' do
+          it 'finds the tags and redirects' do
+            expect(requested_ref_double).to receive(:find).and_call_original
+            request
+            expect(subject).to redirect_to("/#{project.full_path}/-/tree/#{id}/?ref_type=tags")
+          end
+        end
+
         context 'and explicitly requesting a branch' do
           let(:ref_type) { 'heads' }
 
@@ -95,16 +106,6 @@
             expect(response).to be_ok
           end
         end
-
-        context 'and not specifying a ref_type' do
-          let(:ref_type) { nil }
-
-          it 'finds the tags and redirects' do
-            expect(requested_ref_double).to receive(:find).and_call_original
-            request
-            expect(subject).to redirect_to("/#{project.full_path}/-/tree/#{id}/?ref_type=tags")
-          end
-        end
       end
     end
 
@@ -145,23 +146,27 @@
       end
     end
 
-    context "renamed default branch, valid file" do
-      let(:id) { 'old-default-branch/encoding/' }
-      let(:previous_default_branch) { 'old-default-branch' }
+    context 'when default branch was renamed' do
+      let(:project) { create(:project, :repository, previous_default_branch: previous_default_branch) }
 
-      it 'redirects' do
-        request
-        expect(subject).to redirect_to("/#{project.full_path}/-/tree/#{project.default_branch}/encoding/")
+      context "and the file is valid" do
+        let(:id) { 'old-default-branch/encoding/' }
+        let(:previous_default_branch) { 'old-default-branch' }
+
+        it 'redirects' do
+          request
+          expect(subject).to redirect_to("/#{project.full_path}/-/tree/#{project.default_branch}/encoding/")
+        end
       end
-    end
 
-    context "renamed default branch, invalid file" do
-      let(:id) { 'old-default-branch/invalid-path/' }
-      let(:previous_default_branch) { 'old-default-branch' }
+      context "and the file is invalid" do
+        let(:id) { 'old-default-branch/invalid-path/' }
+        let(:previous_default_branch) { 'old-default-branch' }
 
-      it 'redirects' do
-        request
-        expect(subject).to redirect_to("/#{project.full_path}/-/tree/#{project.default_branch}/invalid-path/")
+        it 'redirects' do
+          request
+          expect(subject).to redirect_to("/#{project.full_path}/-/tree/#{project.default_branch}/invalid-path/")
+        end
       end
     end
 
@@ -193,7 +198,6 @@
     end
 
     context "valid SHA commit ID" do
-      let(:ref_type) { nil }
       let(:id) { '6d39438' }
 
       it 'responds with success' do
@@ -217,7 +221,6 @@
     end
 
     context "valid SHA commit ID with path" do
-      let(:ref_type) { nil }
       let(:id) { '6d39438/.gitignore' }
 
       it 'responds with found' do
-- 
GitLab


From e53cec92bdae6f0dcb003a90110de5ffe55037f7 Mon Sep 17 00:00:00 2001
From: "j.seto" <jseto@gitlab.com>
Date: Wed, 21 Jun 2023 13:42:55 -0400
Subject: [PATCH 3/8] Refactor RequestedRef

---
 lib/extracts_ref/requested_ref.rb | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/lib/extracts_ref/requested_ref.rb b/lib/extracts_ref/requested_ref.rb
index ff9ef9c0ff9a4e..97f3e3fe869130 100644
--- a/lib/extracts_ref/requested_ref.rb
+++ b/lib/extracts_ref/requested_ref.rb
@@ -32,14 +32,16 @@ def commit_without_ref_type
       # ref is probably complete 40 character sha
       return { ref_type: nil, commit: commit } if commit.id == ref
 
+      return branch_or_tag_without_ref_type if branch_or_tag_without_ref_type
+
+      { ref_type: nil, commit: commit, ambiguous: ref.match?(SYMBOLIC_REF_PREFIX) }
+    end
+
+    def branch_or_tag_without_ref_type
       if tag.present?
         { ref_type: 'tags', commit: tag, ambiguous: branch.present? }
       elsif branch.present?
         { ref_type: 'heads', commit: branch }
-      elsif ref =~ SYMBOLIC_REF_PREFIX
-        { ref_type: nil, commit: commit, ambiguous: true }
-      else
-        { ref_type: nil, commit: commit }
       end
     end
 
-- 
GitLab


From 027763d10662d624242b94ec749a586f678a68da Mon Sep 17 00:00:00 2001
From: "j.seto" <jseto@gitlab.com>
Date: Wed, 21 Jun 2023 13:58:21 -0400
Subject: [PATCH 4/8] Refactor tests more

---
 spec/controllers/projects/blob_controller_spec.rb | 4 +---
 spec/controllers/projects/tree_controller_spec.rb | 4 +---
 2 files changed, 2 insertions(+), 6 deletions(-)

diff --git a/spec/controllers/projects/blob_controller_spec.rb b/spec/controllers/projects/blob_controller_spec.rb
index cbce8a9e14982a..49c1935c4a3c2e 100644
--- a/spec/controllers/projects/blob_controller_spec.rb
+++ b/spec/controllers/projects/blob_controller_spec.rb
@@ -93,18 +93,16 @@
       end
 
       context 'when default branch was renamed' do
-        let(:project) { create(:project, :public, :repository, previous_default_branch: previous_default_branch) }
+        let_it_be_with_reload(:project) { create(:project, :public, :repository, previous_default_branch: 'old-default-branch') }
 
         context "renamed default branch, valid file" do
           let(:id) { 'old-default-branch/README.md' }
-          let(:previous_default_branch) { 'old-default-branch' }
 
           it { is_expected.to redirect_to("/#{project.full_path}/-/blob/#{project.default_branch}/README.md") }
         end
 
         context "renamed default branch, invalid file" do
           let(:id) { 'old-default-branch/invalid-path.rb' }
-          let(:previous_default_branch) { 'old-default-branch' }
 
           it { is_expected.to redirect_to("/#{project.full_path}/-/blob/#{project.default_branch}/invalid-path.rb") }
         end
diff --git a/spec/controllers/projects/tree_controller_spec.rb b/spec/controllers/projects/tree_controller_spec.rb
index b98fce30c1698f..ffec670e97d097 100644
--- a/spec/controllers/projects/tree_controller_spec.rb
+++ b/spec/controllers/projects/tree_controller_spec.rb
@@ -147,11 +147,10 @@
     end
 
     context 'when default branch was renamed' do
-      let(:project) { create(:project, :repository, previous_default_branch: previous_default_branch) }
+      let_it_be_with_reload(:project) { create(:project, :repository, previous_default_branch: 'old-default-branch') }
 
       context "and the file is valid" do
         let(:id) { 'old-default-branch/encoding/' }
-        let(:previous_default_branch) { 'old-default-branch' }
 
         it 'redirects' do
           request
@@ -161,7 +160,6 @@
 
       context "and the file is invalid" do
         let(:id) { 'old-default-branch/invalid-path/' }
-        let(:previous_default_branch) { 'old-default-branch' }
 
         it 'redirects' do
           request
-- 
GitLab


From 60beae12251211167b31027e2d444a04c16019dc Mon Sep 17 00:00:00 2001
From: "j.seto" <jseto@gitlab.com>
Date: Wed, 21 Jun 2023 13:59:25 -0400
Subject: [PATCH 5/8] Update feature flag definition

---
 config/feature_flags/development/redirect_with_ref_type.yml | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/config/feature_flags/development/redirect_with_ref_type.yml b/config/feature_flags/development/redirect_with_ref_type.yml
index 54976df0b5b5bf..74a4d31eb2f786 100644
--- a/config/feature_flags/development/redirect_with_ref_type.yml
+++ b/config/feature_flags/development/redirect_with_ref_type.yml
@@ -2,7 +2,7 @@
 name: redirect_with_ref_type
 introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/122237
 rollout_issue_url:
-milestone: '16.1'
+milestone: '16.2'
 type: development
 group: group::source code
 default_enabled: false
-- 
GitLab


From 895432a07f9b90a1752a214a4b5e2327e011b985 Mon Sep 17 00:00:00 2001
From: "j.seto" <jseto@gitlab.com>
Date: Thu, 22 Jun 2023 13:18:52 -0400
Subject: [PATCH 6/8] Compare results with literal SHAs instead of result of
 code

---
 spec/lib/extracts_ref/requested_ref_spec.rb | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/spec/lib/extracts_ref/requested_ref_spec.rb b/spec/lib/extracts_ref/requested_ref_spec.rb
index a6e77613f1ad8a..51b3b7bf3c74c1 100644
--- a/spec/lib/extracts_ref/requested_ref_spec.rb
+++ b/spec/lib/extracts_ref/requested_ref_spec.rb
@@ -43,7 +43,7 @@
 
       it 'returns the commit' do
         expect(subject[:ref_type]).to be_nil
-        expect(subject[:commit].id).to eq(project.repository.commit(ref).id)
+        expect(subject[:commit].id).to eq(ref)
       end
     end
 
@@ -52,7 +52,7 @@
 
       it 'returns the tag commit' do
         expect(subject[:ref_type]).to eq('tags')
-        expect(subject[:commit].id).to eq(project.repository.find_tag(ref)&.dereferenced_target&.id)
+        expect(subject[:commit].id).to eq('6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9')
       end
 
       context 'and there is a branch with the same name' do
@@ -64,7 +64,7 @@
 
         it 'returns the tag commit' do
           expect(subject[:ref_type]).to eq('tags')
-          expect(subject[:commit].id).to eq(project.repository.find_tag(ref)&.dereferenced_target&.id)
+          expect(subject[:commit].id).to eq('6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9')
           expect(subject[:ambiguous]).to be_truthy
         end
       end
@@ -75,7 +75,7 @@
 
       it 'returns the tag commit' do
         expect(subject[:ref_type]).to eq('heads')
-        expect(subject[:commit].id).to eq(project.repository.find_branch(ref)&.dereferenced_target&.id)
+        expect(subject[:commit].id).to eq('0b4bc9a49b562e85de7cc9e834518ea6828729b9')
       end
     end
 
@@ -102,7 +102,7 @@
 
       it 'returns the commit' do
         expect(subject[:ref_type]).to be_nil
-        expect(subject[:commit].id).to eq(project.repository.find_branch(existing_branch_name)&.dereferenced_target&.id)
+        expect(subject[:commit].id).to eq('0b4bc9a49b562e85de7cc9e834518ea6828729b9')
         expect(subject[:ambiguous]).to be_truthy
       end
     end
-- 
GitLab


From dbd24e8f307d1202125282f938ac6247fa83066c Mon Sep 17 00:00:00 2001
From: "j.seto" <jseto@gitlab.com>
Date: Thu, 22 Jun 2023 13:20:12 -0400
Subject: [PATCH 7/8] Refactor commit_without_ref_type

---
 lib/extracts_ref/requested_ref.rb | 20 ++++++++------------
 1 file changed, 8 insertions(+), 12 deletions(-)

diff --git a/lib/extracts_ref/requested_ref.rb b/lib/extracts_ref/requested_ref.rb
index 97f3e3fe869130..f20018b5ef494e 100644
--- a/lib/extracts_ref/requested_ref.rb
+++ b/lib/extracts_ref/requested_ref.rb
@@ -27,21 +27,17 @@ def find
     private
 
     def commit_without_ref_type
-      return { ref_type: nil, commit: nil } if commit.nil?
-
-      # ref is probably complete 40 character sha
-      return { ref_type: nil, commit: commit } if commit.id == ref
-
-      return branch_or_tag_without_ref_type if branch_or_tag_without_ref_type
-
-      { ref_type: nil, commit: commit, ambiguous: ref.match?(SYMBOLIC_REF_PREFIX) }
-    end
-
-    def branch_or_tag_without_ref_type
-      if tag.present?
+      if commit.nil?
+        { ref_type: nil, commit: nil }
+      elsif commit.id == ref
+        # ref is probably complete 40 character sha
+        { ref_type: nil, commit: commit }
+      elsif tag.present?
         { ref_type: 'tags', commit: tag, ambiguous: branch.present? }
       elsif branch.present?
         { ref_type: 'heads', commit: branch }
+      else
+        { ref_type: nil, commit: commit, ambiguous: ref.match?(SYMBOLIC_REF_PREFIX) }
       end
     end
 
-- 
GitLab


From 103d15b1c7349ce1cbf86a60f7ad96807e372de7 Mon Sep 17 00:00:00 2001
From: "j.seto" <jseto@gitlab.com>
Date: Sat, 24 Jun 2023 13:44:49 -0400
Subject: [PATCH 8/8] Refactor spec to the sha being tested more clear

---
 spec/lib/extracts_ref/requested_ref_spec.rb | 97 +++++++++++++++------
 1 file changed, 70 insertions(+), 27 deletions(-)

diff --git a/spec/lib/extracts_ref/requested_ref_spec.rb b/spec/lib/extracts_ref/requested_ref_spec.rb
index 51b3b7bf3c74c1..80d3575b360aef 100644
--- a/spec/lib/extracts_ref/requested_ref_spec.rb
+++ b/spec/lib/extracts_ref/requested_ref_spec.rb
@@ -8,16 +8,38 @@
 
     let_it_be(:project) { create(:project, :repository) }
     let(:ref_type) { nil }
-    let(:existing_tag_name) { 'v1.0.0' }
-    let(:existing_branch_name) { 'feature' }
 
-    shared_examples 'RequestedRef when ref_type is specified' do
+    # Create branches and tags consistently with the same shas to make comparison easier to follow
+    let(:tag_sha) { RepoHelpers.sample_commit.id }
+    let(:branch_sha) { RepoHelpers.another_sample_commit.id }
+
+    shared_context 'when a branch exists' do
+      before do
+        project.repository.create_branch(branch_name, branch_sha)
+      end
+
+      after do
+        project.repository.rm_branch(project.owner, branch_name)
+      end
+    end
+
+    shared_context 'when a tag exists' do
+      before do
+        project.repository.add_tag(project.owner, tag_name, tag_sha)
+      end
+
+      after do
+        project.repository.rm_tag(project.owner, tag_name)
+      end
+    end
+
+    shared_examples 'RequestedRef when ref_type is specified' do |branch_sha, tag_sha|
       context 'when ref_type is heads' do
         let(:ref_type) { 'heads' }
 
         it 'returns the branch commit' do
           expect(subject[:ref_type]).to eq('heads')
-          expect(subject[:commit].id).to eq(project.repository.find_branch(ref)&.dereferenced_target&.id)
+          expect(subject[:commit].id).to eq(branch_sha)
         end
       end
 
@@ -26,65 +48,82 @@
 
         it 'returns the tag commit' do
           expect(subject[:ref_type]).to eq('tags')
-          expect(subject[:commit].id).to eq(project.repository.find_tag(ref)&.dereferenced_target&.id)
+          expect(subject[:commit].id).to eq(tag_sha)
         end
       end
     end
 
     context 'when the ref is the sha for a commit' do
-      let(:ref) { '570e7b2abdd848b95f2f578043fc23bd6f6fd24d' }
+      let(:ref) { branch_sha }
 
-      before(:all) do
-        project.repository.create_branch('570e7b2abdd848b95f2f578043fc23bd6f6fd24d')
-        project.repository.add_tag(project.owner, '570e7b2abdd848b95f2f578043fc23bd6f6fd24d', 'master')
-      end
+      context 'and a tag and branch with that sha as a name' do
+        include_context 'when a branch exists' do
+          let(:branch_name) { ref }
+        end
 
-      it_behaves_like 'RequestedRef when ref_type is specified'
+        include_context 'when a tag exists' do
+          let(:tag_name) { ref }
+        end
 
-      it 'returns the commit' do
-        expect(subject[:ref_type]).to be_nil
-        expect(subject[:commit].id).to eq(ref)
+        it_behaves_like 'RequestedRef when ref_type is specified',
+          RepoHelpers.another_sample_commit.id,
+          RepoHelpers.sample_commit.id
+
+        it 'returns the commit' do
+          expect(subject[:ref_type]).to be_nil
+          expect(subject[:commit].id).to eq(ref)
+        end
       end
     end
 
     context 'when ref is for a tag' do
-      let(:ref) { existing_tag_name }
+      include_context 'when a tag exists' do
+        let(:tag_name) { SecureRandom.uuid }
+      end
+
+      let(:ref) { tag_name }
 
       it 'returns the tag commit' do
         expect(subject[:ref_type]).to eq('tags')
-        expect(subject[:commit].id).to eq('6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9')
+        expect(subject[:commit].id).to eq(tag_sha)
       end
 
       context 'and there is a branch with the same name' do
-        before do
-          project.repository.create_branch(existing_tag_name)
+        include_context 'when a branch exists' do
+          let(:branch_name) { ref }
         end
 
-        it_behaves_like 'RequestedRef when ref_type is specified'
+        it_behaves_like 'RequestedRef when ref_type is specified',
+          RepoHelpers.another_sample_commit.id,
+          RepoHelpers.sample_commit.id
 
         it 'returns the tag commit' do
           expect(subject[:ref_type]).to eq('tags')
-          expect(subject[:commit].id).to eq('6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9')
+          expect(subject[:commit].id).to eq(tag_sha)
           expect(subject[:ambiguous]).to be_truthy
         end
       end
     end
 
     context 'when ref is only for a branch' do
-      let(:ref) { existing_branch_name }
+      let(:ref) { SecureRandom.uuid }
 
-      it 'returns the tag commit' do
+      include_context 'when a branch exists' do
+        let(:branch_name) { ref }
+      end
+
+      it 'returns the branch commit' do
         expect(subject[:ref_type]).to eq('heads')
-        expect(subject[:commit].id).to eq('0b4bc9a49b562e85de7cc9e834518ea6828729b9')
+        expect(subject[:commit].id).to eq(branch_sha)
       end
     end
 
     context 'when ref is an abbreviated commit sha' do
-      let(:ref) { '570e7b2abdd848b95f2f578043fc23bd6f6fd24d'.first(8) }
+      let(:ref) { branch_sha.first(8) }
 
       it 'returns the commit' do
         expect(subject[:ref_type]).to be_nil
-        expect(subject[:commit].id).to eq(project.repository.commit('570e7b2abdd848b95f2f578043fc23bd6f6fd24d').id)
+        expect(subject[:commit].id).to eq(branch_sha)
       end
     end
 
@@ -98,11 +137,15 @@
     end
 
     context 'when ref is symbolic' do
-      let(:ref) { "heads/#{existing_branch_name}" }
+      let(:ref) { "heads/#{branch_name}" }
+
+      include_context 'when a branch exists' do
+        let(:branch_name) { SecureRandom.uuid }
+      end
 
       it 'returns the commit' do
         expect(subject[:ref_type]).to be_nil
-        expect(subject[:commit].id).to eq('0b4bc9a49b562e85de7cc9e834518ea6828729b9')
+        expect(subject[:commit].id).to eq(branch_sha)
         expect(subject[:ambiguous]).to be_truthy
       end
     end
-- 
GitLab