From aa2303ef91141e55e6c7a6afed802d881ef97840 Mon Sep 17 00:00:00 2001
From: Eulyeon Ko <eko@gitlab.com>
Date: Thu, 14 Sep 2023 13:57:59 +0900
Subject: [PATCH 1/7] Create RefExtractor class

RefExtractor is a refactor of ExtractsRef module.

ExtractsRef was mostly used as a Rails concern in controllers
to extract git related strings from request parameters.
The module utilized instance variables that made the code
hard to follow and difficult to examine for side effect.

As of this commit, ExtractsRef cannot be removed yet because
another related module ExtractsPath depends on ExtractsRef.
---
 ee/app/finders/llm/extra_resource_finder.rb   |  11 +-
 lib/extracts_ref.rb                           |  35 ++--
 lib/extracts_ref/ref_extractor.rb             | 183 ++++++++++++++++++
 spec/lib/extracts_path_spec.rb                |   2 +-
 spec/lib/extracts_ref/ref_extractor_spec.rb   | 125 ++++++++++++
 .../ref_extraction_shared_examples.rb         | 169 ++++++++++++++++
 6 files changed, 494 insertions(+), 31 deletions(-)
 create mode 100644 lib/extracts_ref/ref_extractor.rb
 create mode 100644 spec/lib/extracts_ref/ref_extractor_spec.rb
 create mode 100644 spec/support/shared_examples/ref_extraction_shared_examples.rb

diff --git a/ee/app/finders/llm/extra_resource_finder.rb b/ee/app/finders/llm/extra_resource_finder.rb
index 856b01df0a071c..8d54f4b1e656f2 100644
--- a/ee/app/finders/llm/extra_resource_finder.rb
+++ b/ee/app/finders/llm/extra_resource_finder.rb
@@ -6,10 +6,6 @@ module Llm
   # Since the finder does not deal with DB resources, it's been added to spec/support/finder_collection_allowlist.yml.
   # As more resource types need to be supported (potentially), appropriate abstractions should be designed and added.
   class ExtraResourceFinder
-    # TODO: https://gitlab.com/gitlab-org/gitlab/-/issues/422133
-    # The module is meant to be used in controllers.
-    include ::ExtractsRef
-
     def initialize(current_user, referer_url)
       @current_user = current_user
       @referer_url = referer_url
@@ -68,12 +64,7 @@ def extract_blob_ref_and_path(resource_path)
                         .first.tap { |blob_path| blob_path || "" }
       return if resource_path.empty?
 
-      extract_ref(resource_path)
-    end
-
-    # Required to use the method `extract_ref` from ExtractsRef
-    def repository_container
-      @project
+      ExtractsRef::RefExtractor.new(@project, {}).extract_ref(resource_path)
     end
   end
 end
diff --git a/lib/extracts_ref.rb b/lib/extracts_ref.rb
index fa3c1fe7307d09..88d95855f4a1ac 100644
--- a/lib/extracts_ref.rb
+++ b/lib/extracts_ref.rb
@@ -1,32 +1,31 @@
 # frozen_string_literal: true
 
+# TODO:
+# THIS MODULE'S BEEN DEPRECATED.
+# You should not be using this module directly anymore.
+# Use `ExtractsRef::RefExtractor` class instead. The class is a refactor of this module.
+# This module was intended to be used as a controller concern and utilized many instance variables.
+# This module still exists because another concern-like lib module `ExtractsPath` is depending on it.
+#
 # Module providing methods for dealing with separating a tree-ish string and a
 # file path string when combined in a request parameter
 # Can be extended for different types of repository object, e.g. Project or Snippet
 module ExtractsRef
-  InvalidPathError = Class.new(StandardError)
-  BRANCH_REF_TYPE = 'heads'
-  TAG_REF_TYPE = 'tags'
-  REF_TYPES = [BRANCH_REF_TYPE, TAG_REF_TYPE].freeze
+  InvalidPathError = ExtractsRef::RefExtractor::InvalidPathError
+  BRANCH_REF_TYPE = ExtractsRef::RefExtractor::BRANCH_REF_TYPE
+  TAG_REF_TYPE = ExtractsRef::RefExtractor::TAG_REF_TYPE
+  REF_TYPES = ExtractsRef::RefExtractor::REF_TYPES
 
   def self.ref_type(type)
-    return unless REF_TYPES.include?(type&.downcase)
-
-    type.downcase
+    ExtractsRef::RefExtractor.ref_type(type)
   end
 
   def self.qualify_ref(ref, type)
-    validated_type = ref_type(type)
-    return ref unless validated_type
-
-    %(refs/#{validated_type}/#{ref})
+    ExtractsRef::RefExtractor.qualify_ref(ref, type)
   end
 
   def self.unqualify_ref(ref, type)
-    validated_type = ref_type(type)
-    return ref unless validated_type
-
-    ref.sub(%r{^refs/#{validated_type}/}, '')
+    ExtractsRef::RefExtractor.unqualify_ref(ref, type)
   end
 
   # Given a string containing both a Git tree-ish, such as a branch or tag, and
@@ -91,7 +90,7 @@ def assign_ref_vars
     return unless @ref.present?
 
     @commit = if ref_type
-                @fully_qualified_ref = ExtractsRef.qualify_ref(@ref, ref_type)
+                @fully_qualified_ref = ExtractsRef::RefExtractor.qualify_ref(@ref, ref_type)
                 @repo.commit(@fully_qualified_ref)
               else
                 @repo.commit(@ref)
@@ -171,10 +170,6 @@ def ref_names
     @ref_names ||= repository_container.repository.ref_names # rubocop:disable Gitlab/ModuleWithInstanceVariables
   end
 
-  def repository_container
-    raise NotImplementedError
-  end
-
   # deprecated in favor of ExtractsRef::RequestedRef
   def ambiguous_ref?(project, ref)
     return false unless ref
diff --git a/lib/extracts_ref/ref_extractor.rb b/lib/extracts_ref/ref_extractor.rb
new file mode 100644
index 00000000000000..ffdc9f1db9cc68
--- /dev/null
+++ b/lib/extracts_ref/ref_extractor.rb
@@ -0,0 +1,183 @@
+# frozen_string_literal: true
+
+# Module providing methods for dealing with separating a tree-ish string and a
+# file path string when combined in a request parameter
+# Can be extended for different types of repository object, e.g. Project or Snippet
+module ExtractsRef
+  class RefExtractor
+    InvalidPathError = Class.new(StandardError)
+    BRANCH_REF_TYPE = 'heads'
+    TAG_REF_TYPE = 'tags'
+    REF_TYPES = [BRANCH_REF_TYPE, TAG_REF_TYPE].freeze
+
+    attr_reader :repository_container, :params
+    attr_accessor :id, :ref, :commit, :path, :fully_qualified_ref, :repo
+
+    def initialize(repository_container, params, override_id: nil)
+      @repository_container = repository_container
+      @params = params.extract!(:id, :ref, :path, :ref_type)
+      @override_id = override_id
+    end
+
+    # Extracts common variables for views working with Git tree-ish objects
+    #
+    # Assignments are:
+    #
+    # - @id     - A string representing the joined ref and path
+    #             Assigns @override_id if it is present.
+    # - @ref    - A string representing the ref (e.g., the branch, tag, or commit SHA)
+    # - @path   - A string representing the filesystem path
+    # - @commit - A Commit representing the commit from the given ref
+    # - @fully_qualified_ref - A string representing the fully qualifed ref (e.g., refs/tags/v1.1)
+    # - @repo
+    #
+    # If the :id parameter appears to be requesting a specific response format,
+    # that will be handled as well.
+    def extract!
+      qualified_id, @ref, @path = extract_ref_path
+      @id = @override_id || qualified_id
+      @repo = repository_container.repository
+      raise InvalidPathError if @ref.match?(/\s/)
+
+      return unless @ref.present?
+
+      @commit = if ref_type
+                  @fully_qualified_ref = self.class.qualify_ref(@ref, ref_type)
+                  @repo.commit(@fully_qualified_ref)
+                else
+                  @repo.commit(@ref)
+                end
+    end
+
+    def self.ref_type(type)
+      return unless REF_TYPES.include?(type&.downcase)
+
+      type.downcase
+    end
+
+    def self.qualify_ref(ref, type)
+      validated_type = ref_type(type)
+      return ref unless validated_type
+
+      %(refs/#{validated_type}/#{ref})
+    end
+
+    def self.unqualify_ref(ref, type)
+      validated_type = ref_type(type)
+      return ref unless validated_type
+
+      ref.sub(%r{^refs/#{validated_type}/}, '')
+    end
+
+    # Given a string containing both a Git tree-ish, such as a branch or tag, and
+    # a filesystem path joined by forward slashes, attempts to separate the two.
+    #
+    # Expects a repository_container method that returns the active repository object. This is
+    # used to check the input against a list of valid repository refs.
+    #
+    # Examples
+    #
+    #   # No repository_container available
+    #   extract_ref('master')
+    #   # => ['', '']
+    #
+    #   extract_ref('master')
+    #   # => ['master', '']
+    #
+    #   extract_ref("f4b14494ef6abf3d144c28e4af0c20143383e062/CHANGELOG")
+    #   # => ['f4b14494ef6abf3d144c28e4af0c20143383e062', 'CHANGELOG']
+    #
+    #   extract_ref("v2.0.0/README.md")
+    #   # => ['v2.0.0', 'README.md']
+    #
+    #   extract_ref('master/app/models/project.rb')
+    #   # => ['master', 'app/models/project.rb']
+    #
+    #   extract_ref('issues/1234/app/models/project.rb')
+    #   # => ['issues/1234', 'app/models/project.rb']
+    #
+    #   # Given an invalid branch, we fall back to just splitting on the first slash
+    #   extract_ref('non/existent/branch/README.md')
+    #   # => ['non', 'existent/branch/README.md']
+    #
+    # Returns an Array where the first value is the tree-ish and the second is the
+    # path
+    def extract_ref(id)
+      pair = extract_raw_ref(id)
+
+      [
+        pair[0].strip,
+        pair[1].delete_prefix('/').delete_suffix('/')
+      ]
+    end
+
+    def tree
+      @tree ||= @repo.tree(@commit.id, @path) # rubocop:disable Gitlab/ModuleWithInstanceVariables
+    end
+
+    def extract_ref_path
+      id = extract_id_from_params
+      ref, path = extract_ref(id)
+
+      [id, ref, path]
+    end
+
+    def ref_type
+      self.class.ref_type(params[:ref_type])
+    end
+
+    private
+
+    def extract_raw_ref(id)
+      return ['', ''] unless repository_container
+
+      # If the ref appears to be a SHA, we're done, just split the string
+      return $~.captures if id =~ /^(\h{40})(.+)/
+
+      # No slash means we must have a ref and no path
+      return [id, ''] unless id.include?('/')
+
+      # Otherwise, attempt to detect the ref using a list of the
+      # repository_container's branches and tags
+
+      # Append a trailing slash if we only get a ref and no file path
+      id = [id, '/'].join unless id.ends_with?('/')
+      first_path_segment, rest = id.split('/', 2)
+
+      return [first_path_segment, rest] if use_first_path_segment?(first_path_segment)
+
+      valid_refs = ref_names.select { |v| id.start_with?("#{v}/") }
+
+      # No exact ref match, so just try our best
+      return id.match(%r{([^/]+)(.*)}).captures if valid_refs.empty?
+
+      # There is a distinct possibility that multiple refs prefix the ID.
+      # Use the longest match to maximize the chance that we have the
+      # right ref.
+      best_match = valid_refs.max_by(&:length)
+
+      # Partition the string into the ref and the path, ignoring the empty first value
+      id.partition(best_match)[1..]
+    end
+
+    def use_first_path_segment?(ref)
+      return false unless repository_container
+      return false if repository_container.repository.has_ambiguous_refs?
+
+      repository_container.repository.branch_names_include?(ref) ||
+        repository_container.repository.tag_names_include?(ref)
+    end
+
+    def extract_id_from_params
+      id = [params[:id] || params[:ref]]
+      id << ("/#{params[:path]}") unless params[:path].blank?
+      id.join
+    end
+
+    def ref_names
+      return [] unless repository_container
+
+      @ref_names ||= repository_container.repository.ref_names
+    end
+  end
+end
diff --git a/spec/lib/extracts_path_spec.rb b/spec/lib/extracts_path_spec.rb
index 5db2fbd923e575..793ed10d29673d 100644
--- a/spec/lib/extracts_path_spec.rb
+++ b/spec/lib/extracts_path_spec.rb
@@ -215,7 +215,7 @@ def default_url_options
     end
 
     it 'raises an error if there are no matching refs' do
-      expect { extract_ref_without_atom('foo.atom') }.to raise_error(ExtractsRef::InvalidPathError)
+      expect { extract_ref_without_atom('foo.atom') }.to raise_error(ExtractsRef::RefExtractor::InvalidPathError)
     end
   end
 end
diff --git a/spec/lib/extracts_ref/ref_extractor_spec.rb b/spec/lib/extracts_ref/ref_extractor_spec.rb
new file mode 100644
index 00000000000000..23b283967cac77
--- /dev/null
+++ b/spec/lib/extracts_ref/ref_extractor_spec.rb
@@ -0,0 +1,125 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe ExtractsRef::RefExtractor, feature_category: :source_code_management do
+  include RepoHelpers
+
+  let_it_be(:owner) { create(:user) }
+  let_it_be(:container) { create(:snippet, :repository, author: owner) }
+
+  let(:ref) { sample_commit[:id] }
+  let(:path) { sample_commit[:line_code_path] }
+  let(:params) { { path: path, ref: ref } }
+
+  let(:ref_extractor) { described_class.new(container, params) }
+
+  before do
+    ref_names = ['master', 'foo/bar/baz', 'v1.0.0', 'v2.0.0', 'release/app', 'release/app/v1.0.0']
+
+    allow(container.repository).to receive(:ref_names).and_return(ref_names)
+  end
+
+  describe '#extract_vars!' do
+    it_behaves_like 'extracts ref vars'
+
+    context 'when ref contains trailing space' do
+      let(:ref) { 'master ' }
+
+      it 'strips surrounding space' do
+        ref_extractor.extract!
+
+        expect(ref_extractor.ref).to eq('master')
+      end
+    end
+
+    context 'when ref and path are nil' do
+      let(:ref) { nil }
+      let(:path) { nil }
+
+      it 'does not set commit' do
+        expect(container.repository).not_to receive(:commit).with('')
+
+        ref_extractor.extract!
+
+        expect(ref_extractor.commit).to be_nil
+      end
+    end
+
+    context 'when a ref_type parameter is provided' do
+      let(:params) { { path: path, ref: ref, ref_type: 'tags' } }
+
+      it 'sets a fully_qualified_ref variable' do
+        fully_qualified_ref = "refs/tags/#{ref}"
+
+        expect(container.repository).to receive(:commit).with(fully_qualified_ref)
+
+        ref_extractor.extract!
+
+        expect(ref_extractor.fully_qualified_ref).to eq(fully_qualified_ref)
+      end
+    end
+  end
+
+  describe '#ref_type' do
+    let(:params) { { ref_type: 'heads' } }
+
+    it 'delegates to .ref_type' do
+      expect(described_class).to receive(:ref_type).with('heads')
+
+      ref_extractor.ref_type
+    end
+  end
+
+  describe '.ref_type' do
+    subject { described_class.ref_type(ref_type) }
+
+    context 'when ref_type is nil' do
+      let(:ref_type) { nil }
+
+      it { is_expected.to eq(nil) }
+    end
+
+    context 'when ref_type is heads' do
+      let(:ref_type) { 'heads' }
+
+      it { is_expected.to eq('heads') }
+    end
+
+    context 'when ref_type is tags' do
+      let(:ref_type) { 'tags' }
+
+      it { is_expected.to eq('tags') }
+    end
+
+    context 'when ref_type is invalid' do
+      let(:ref_type) { 'invalid' }
+
+      it { is_expected.to eq(nil) }
+    end
+  end
+
+  describe '.qualify_ref' do
+    subject { described_class.qualify_ref(ref, ref_type) }
+
+    context 'when ref_type is nil' do
+      let(:ref_type) { nil }
+
+      it { is_expected.to eq(ref) }
+    end
+
+    context 'when ref_type valid' do
+      let(:ref_type) { 'heads' }
+
+      it { is_expected.to eq("refs/#{ref_type}/#{ref}") }
+    end
+
+    context 'when ref_type is invalid' do
+      let(:ref_type) { 'invalid' }
+
+      it { is_expected.to eq(ref) }
+    end
+  end
+
+  it_behaves_like 'extracts ref method'
+end
diff --git a/spec/support/shared_examples/ref_extraction_shared_examples.rb b/spec/support/shared_examples/ref_extraction_shared_examples.rb
new file mode 100644
index 00000000000000..4b0b3efd0b7a7b
--- /dev/null
+++ b/spec/support/shared_examples/ref_extraction_shared_examples.rb
@@ -0,0 +1,169 @@
+# frozen_string_literal: true
+
+RSpec.shared_examples 'extracts ref vars' do
+  it 'extracts the repository var' do
+    ref_extractor.extract!
+
+    expect(ref_extractor.repo).to eq container.repository
+  end
+
+  context 'when ref contains %20' do
+    let(:ref) { 'foo%20bar' }
+
+    it 'is not converted to a space in @id' do
+      container.repository.add_branch(owner, 'foo%20bar', 'master')
+
+      ref_extractor.extract!
+
+      expect(ref_extractor.id).to start_with('foo%20bar/')
+    end
+  end
+
+  context 'when ref contains trailing space' do
+    let(:ref) { 'master ' }
+
+    it 'strips surrounding space' do
+      ref_extractor.extract!
+
+      expect(ref_extractor.ref).to eq('master')
+    end
+  end
+
+  context 'when ref contains leading space' do
+    let(:ref) { ' master ' }
+
+    it 'strips surrounding space' do
+      ref_extractor.extract!
+
+      expect(ref_extractor.ref).to eq('master')
+    end
+  end
+
+  context 'when path contains space' do
+    let(:ref) { '38008cb17ce1466d8fec2dfa6f6ab8dcfe5cf49e' }
+    let(:path) { 'with space' }
+
+    it 'is not converted to %20 in @path' do
+      ref_extractor.extract!
+
+      expect(ref_extractor.path).to eq(path)
+    end
+  end
+
+  context 'when override_id is given' do
+    let(:ref_extractor) do
+      described_class.new(container, params, override_id: '38008cb17ce1466d8fec2dfa6f6ab8dcfe5cf49e')
+    end
+
+    it 'uses override_id' do
+      ref_extractor.extract!
+
+      expect(ref_extractor.id).to eq('38008cb17ce1466d8fec2dfa6f6ab8dcfe5cf49e')
+    end
+  end
+end
+
+RSpec.shared_examples 'extracts ref method' do
+  describe '#extract_ref' do
+    it 'returns an empty pair when no repository_container is set' do
+      allow_next_instance_of(described_class) do |instance|
+        allow(instance).to receive(:repository_container).and_return(nil)
+      end
+      expect(ref_extractor.extract_ref('master/CHANGELOG')).to eq(['', ''])
+    end
+
+    context 'without a path' do
+      it 'extracts a valid branch' do
+        expect(ref_extractor.extract_ref('master')).to eq(['master', ''])
+      end
+
+      it 'extracts a valid tag' do
+        expect(ref_extractor.extract_ref('v2.0.0')).to eq(['v2.0.0', ''])
+      end
+
+      it 'extracts a valid commit ref without a path' do
+        expect(ref_extractor.extract_ref('f4b14494ef6abf3d144c28e4af0c20143383e062')).to eq(
+          ['f4b14494ef6abf3d144c28e4af0c20143383e062', '']
+        )
+      end
+
+      it 'falls back to a primitive split for an invalid ref' do
+        expect(ref_extractor.extract_ref('stable')).to eq(['stable', ''])
+      end
+
+      it 'does not fetch ref names when there is no slash' do
+        expect(ref_extractor).not_to receive(:ref_names)
+
+        ref_extractor.extract_ref('master')
+      end
+
+      it 'fetches ref names when there is a slash' do
+        expect(ref_extractor).to receive(:ref_names).and_call_original
+
+        ref_extractor.extract_ref('release/app/v1.0.0')
+      end
+    end
+
+    context 'with a path' do
+      it 'extracts a valid branch' do
+        expect(ref_extractor.extract_ref('foo/bar/baz/CHANGELOG')).to eq(
+          ['foo/bar/baz', 'CHANGELOG'])
+      end
+
+      it 'extracts a valid tag' do
+        expect(ref_extractor.extract_ref('v2.0.0/CHANGELOG')).to eq(['v2.0.0', 'CHANGELOG'])
+      end
+
+      it 'extracts a valid commit SHA' do
+        expect(ref_extractor.extract_ref('f4b14494ef6abf3d144c28e4af0c20143383e062/CHANGELOG')).to eq(
+          %w[f4b14494ef6abf3d144c28e4af0c20143383e062 CHANGELOG]
+        )
+      end
+
+      it 'falls back to a primitive split for an invalid ref' do
+        expect(ref_extractor.extract_ref('stable/CHANGELOG')).to eq(%w[stable CHANGELOG])
+      end
+
+      it 'extracts the longest matching ref' do
+        expect(ref_extractor.extract_ref('release/app/v1.0.0/README.md')).to eq(
+          ['release/app/v1.0.0', 'README.md'])
+      end
+
+      context 'when the repository does not have ambiguous refs' do
+        before do
+          allow(container.repository).to receive(:has_ambiguous_refs?).and_return(false)
+        end
+
+        it 'does not fetch all ref names when the first path component is a ref' do
+          expect(ref_extractor).not_to receive(:ref_names)
+          expect(container.repository).to receive(:branch_names_include?).with('v1.0.0').and_return(false)
+          expect(container.repository).to receive(:tag_names_include?).with('v1.0.0').and_return(true)
+
+          expect(ref_extractor.extract_ref('v1.0.0/doc/README.md')).to eq(['v1.0.0', 'doc/README.md'])
+        end
+
+        it 'fetches all ref names when the first path component is not a ref' do
+          expect(ref_extractor).to receive(:ref_names).and_call_original
+          expect(container.repository).to receive(:branch_names_include?).with('release').and_return(false)
+          expect(container.repository).to receive(:tag_names_include?).with('release').and_return(false)
+
+          expect(ref_extractor.extract_ref('release/app/doc/README.md')).to eq(['release/app', 'doc/README.md'])
+        end
+      end
+
+      context 'when the repository has ambiguous refs' do
+        before do
+          allow(container.repository).to receive(:has_ambiguous_refs?).and_return(true)
+        end
+
+        it 'always fetches all ref names' do
+          expect(ref_extractor).to receive(:ref_names).and_call_original
+          expect(container.repository).not_to receive(:branch_names_include?)
+          expect(container.repository).not_to receive(:tag_names_include?)
+
+          expect(ref_extractor.extract_ref('v1.0.0/doc/README.md')).to eq(['v1.0.0', 'doc/README.md'])
+        end
+      end
+    end
+  end
+end
-- 
GitLab


From 2a69cba73e84825265a5ee7b1e9dfa02b39097d4 Mon Sep 17 00:00:00 2001
From: Eulyeon Ko <eko@gitlab.com>
Date: Thu, 14 Sep 2023 17:24:54 +0900
Subject: [PATCH 2/7] Swap out ExtractsRef with RefExtractor

RefExtractor class is a refactor of ExtractsRef module.
---
 app/controllers/concerns/snippets/blobs_actions.rb | 11 +++++------
 app/controllers/projects/blob_controller.rb        |  2 +-
 app/graphql/resolvers/blobs_resolver.rb            |  2 +-
 app/graphql/resolvers/last_commit_resolver.rb      |  2 +-
 app/models/tree.rb                                 |  4 ++--
 app/presenters/blob_presenter.rb                   |  2 +-
 app/presenters/tree_entry_presenter.rb             |  2 +-
 7 files changed, 12 insertions(+), 13 deletions(-)

diff --git a/app/controllers/concerns/snippets/blobs_actions.rb b/app/controllers/concerns/snippets/blobs_actions.rb
index 2a0491b4df8b64..fded6faf8b6aad 100644
--- a/app/controllers/concerns/snippets/blobs_actions.rb
+++ b/app/controllers/concerns/snippets/blobs_actions.rb
@@ -4,7 +4,6 @@ module Snippets::BlobsActions
   extend ActiveSupport::Concern
 
   include Gitlab::Utils::StrongMemoize
-  include ExtractsRef
   include Snippets::SendBlob
 
   included do
@@ -19,17 +18,17 @@ def raw
 
   private
 
-  def repository_container
-    snippet
+  def ref_extractor
+    @ref_extractor ||= ExtractsRef::RefExtractor.new(snippet, params.permit(:id, :ref, :path, :ref_type))
   end
 
   # rubocop:disable Gitlab/ModuleWithInstanceVariables
   def blob
-    assign_ref_vars
+    ref_extractor.extract!
 
-    return unless @commit
+    return unless ref_extractor.commit
 
-    @repo.blob_at(@commit.id, @path)
+    ref_extractor.repo.blob_at(ref_extractor.commit.id, ref_extractor.path)
   end
   strong_memoize_attr :blob
   # rubocop:enable Gitlab/ModuleWithInstanceVariables
diff --git a/app/controllers/projects/blob_controller.rb b/app/controllers/projects/blob_controller.rb
index 18ed617e20cdf0..015e56db012f66 100644
--- a/app/controllers/projects/blob_controller.rb
+++ b/app/controllers/projects/blob_controller.rb
@@ -165,7 +165,7 @@ def check_for_ambiguous_ref
 
     @ref_type = ref_type
 
-    if @ref_type == ExtractsRef::BRANCH_REF_TYPE && ambiguous_ref?(@project, @ref)
+    if @ref_type == ExtractsRef::RefExtractor::BRANCH_REF_TYPE && ambiguous_ref?(@project, @ref)
       branch = @project.repository.find_branch(@ref)
       redirect_to project_blob_path(@project, File.join(branch.target, @path))
     end
diff --git a/app/graphql/resolvers/blobs_resolver.rb b/app/graphql/resolvers/blobs_resolver.rb
index 546eeb76ff53f4..27a15381b435f5 100644
--- a/app/graphql/resolvers/blobs_resolver.rb
+++ b/app/graphql/resolvers/blobs_resolver.rb
@@ -36,7 +36,7 @@ def resolve(paths:, ref:, ref_type:)
       ref ||= repository.root_ref
       validate_ref(ref)
 
-      ref = ExtractsRef.qualify_ref(ref, ref_type)
+      ref = ExtractsRef::RefExtractor.qualify_ref(ref, ref_type)
 
       repository.blobs_at(paths.map { |path| [ref, path] }).tap do |blobs|
         blobs.each do |blob|
diff --git a/app/graphql/resolvers/last_commit_resolver.rb b/app/graphql/resolvers/last_commit_resolver.rb
index acf7826ab13880..ff5701ede8cd97 100644
--- a/app/graphql/resolvers/last_commit_resolver.rb
+++ b/app/graphql/resolvers/last_commit_resolver.rb
@@ -12,7 +12,7 @@ def resolve(**args)
       # Ensure merge commits can be returned by sending nil to Gitaly instead of '/'
       path = tree.path == '/' ? nil : tree.path
       commit = Gitlab::Git::Commit.last_for_path(tree.repository,
-        ExtractsRef.qualify_ref(tree.sha, tree.ref_type), path, literal_pathspec: true)
+        ExtractsRef::RefExtractor.qualify_ref(tree.sha, tree.ref_type), path, literal_pathspec: true)
 
       ::Commit.new(commit, tree.repository.project) if commit
     end
diff --git a/app/models/tree.rb b/app/models/tree.rb
index 4d62334800deed..030e7d9e85fb55 100644
--- a/app/models/tree.rb
+++ b/app/models/tree.rb
@@ -13,10 +13,10 @@ def initialize(
     @repository = repository
     @sha = sha
     @path = path
-    @ref_type = ExtractsRef.ref_type(ref_type)
+    @ref_type = ExtractsRef::RefExtractor.ref_type(ref_type)
     git_repo = @repository.raw_repository
 
-    ref = ExtractsRef.qualify_ref(@sha, ref_type)
+    ref = ExtractsRef::RefExtractor.qualify_ref(@sha, ref_type)
 
     @entries, @cursor = Gitlab::Git::Tree.where(git_repo, ref, @path, recursive, skip_flat_paths, rescue_not_found,
       pagination_params)
diff --git a/app/presenters/blob_presenter.rb b/app/presenters/blob_presenter.rb
index 6f32f4de62cac2..e8886f8c21277c 100644
--- a/app/presenters/blob_presenter.rb
+++ b/app/presenters/blob_presenter.rb
@@ -193,7 +193,7 @@ def project
   def commit_id
     # If `ref_type` is present the commit_id will include the ref qualifier e.g. `refs/heads/`.
     # We only accept/return unqualified refs so we need to remove the qualifier from the `commit_id`.
-    ExtractsRef.unqualify_ref(blob.commit_id, ref_type)
+    ExtractsRef::RefExtractor.unqualify_ref(blob.commit_id, ref_type)
   end
 
   def ref_qualified_path
diff --git a/app/presenters/tree_entry_presenter.rb b/app/presenters/tree_entry_presenter.rb
index 3f4a9f13c36338..674fc3ee32208e 100644
--- a/app/presenters/tree_entry_presenter.rb
+++ b/app/presenters/tree_entry_presenter.rb
@@ -19,7 +19,7 @@ def ref_qualified_path
     # If `ref_type` is present the commit_id will include the ref qualifier e.g. `refs/heads/`.
     # We only accept/return unqualified refs so we need to remove the qualifier from the `commit_id`.
 
-    commit_id = ExtractsRef.unqualify_ref(tree.commit_id, ref_type)
+    commit_id = ExtractsRef::RefExtractor.unqualify_ref(tree.commit_id, ref_type)
 
     File.join(commit_id, tree.path)
   end
-- 
GitLab


From 7ee98769d8ad127010db7512f7113a580e4810d0 Mon Sep 17 00:00:00 2001
From: Eulyeon Ko <eko@gitlab.com>
Date: Sun, 17 Sep 2023 14:54:27 +0900
Subject: [PATCH 3/7] Refactor ExtractsPath and ExtractsRef

`ExtractsRef` module is now deprecated
and there's a new refactored class version `ExtractsRef::RefExtractor`.

- Add a warning to discourage using `ExtractsRef`
- Refactor `ExtractsRef` to use `ExtractsRef::RefExtractor`
  to the extent possible to avoid divergence.
- Remove some class methods from `ExtractsRef`
  - `ExtractsPath` includes `ExtractsRef` and the consumers of `ExtractsPath`
    does not use the removed class methods.

At some point `ExtractsPath` should also be refactored into a standalone class.
https://gitlab.com/gitlab-org/gitlab/-/issues/425379
---
 lib/extracts_ref.rb            | 23 +++++++++--------------
 spec/lib/extracts_path_spec.rb |  4 ++--
 spec/lib/extracts_ref_spec.rb  | 22 ----------------------
 3 files changed, 11 insertions(+), 38 deletions(-)

diff --git a/lib/extracts_ref.rb b/lib/extracts_ref.rb
index 88d95855f4a1ac..af3f841ea6edc9 100644
--- a/lib/extracts_ref.rb
+++ b/lib/extracts_ref.rb
@@ -1,11 +1,10 @@
 # frozen_string_literal: true
 
-# TODO:
-# THIS MODULE'S BEEN DEPRECATED.
-# You should not be using this module directly anymore.
-# Use `ExtractsRef::RefExtractor` class instead. The class is a refactor of this module.
-# This module was intended to be used as a controller concern and utilized many instance variables.
-# This module still exists because another concern-like lib module `ExtractsPath` is depending on it.
+# TOOD: https://gitlab.com/gitlab-org/gitlab/-/issues/425379
+# WARNING: This module has been deprecated.
+# The module solely exists because ExtractsPath depends on this module (ExtractsPath is the only user.)
+# ExtractsRef::RefExtractor class is a refactored version of this module and provides
+# the same functionalities. You should use the class instead.
 #
 # Module providing methods for dealing with separating a tree-ish string and a
 # file path string when combined in a request parameter
@@ -20,14 +19,6 @@ def self.ref_type(type)
     ExtractsRef::RefExtractor.ref_type(type)
   end
 
-  def self.qualify_ref(ref, type)
-    ExtractsRef::RefExtractor.qualify_ref(ref, type)
-  end
-
-  def self.unqualify_ref(ref, type)
-    ExtractsRef::RefExtractor.unqualify_ref(ref, type)
-  end
-
   # Given a string containing both a Git tree-ish, such as a branch or tag, and
   # a filesystem path joined by forward slashes, attempts to separate the two.
   #
@@ -170,6 +161,10 @@ def ref_names
     @ref_names ||= repository_container.repository.ref_names # rubocop:disable Gitlab/ModuleWithInstanceVariables
   end
 
+  def repository_container
+    raise NotImplementedError
+  end
+
   # deprecated in favor of ExtractsRef::RequestedRef
   def ambiguous_ref?(project, ref)
     return false unless ref
diff --git a/spec/lib/extracts_path_spec.rb b/spec/lib/extracts_path_spec.rb
index 793ed10d29673d..a10ff60a24915d 100644
--- a/spec/lib/extracts_path_spec.rb
+++ b/spec/lib/extracts_path_spec.rb
@@ -2,7 +2,7 @@
 
 require 'spec_helper'
 
-RSpec.describe ExtractsPath do
+RSpec.describe ExtractsPath, feature_category: :source_code_management do
   include described_class
   include RepoHelpers
   include Gitlab::Routing
@@ -215,7 +215,7 @@ def default_url_options
     end
 
     it 'raises an error if there are no matching refs' do
-      expect { extract_ref_without_atom('foo.atom') }.to raise_error(ExtractsRef::RefExtractor::InvalidPathError)
+      expect { extract_ref_without_atom('foo.atom') }.to raise_error(ExtractsPath::InvalidPathError)
     end
   end
 end
diff --git a/spec/lib/extracts_ref_spec.rb b/spec/lib/extracts_ref_spec.rb
index bf33c8b95f1213..9ff11899e89faa 100644
--- a/spec/lib/extracts_ref_spec.rb
+++ b/spec/lib/extracts_ref_spec.rb
@@ -100,27 +100,5 @@
     end
   end
 
-  describe '.qualify_ref' do
-    subject { described_class.qualify_ref(ref, ref_type) }
-
-    context 'when ref_type is nil' do
-      let(:ref_type) { nil }
-
-      it { is_expected.to eq(ref) }
-    end
-
-    context 'when ref_type valid' do
-      let(:ref_type) { 'heads' }
-
-      it { is_expected.to eq("refs/#{ref_type}/#{ref}") }
-    end
-
-    context 'when ref_type is invalid' do
-      let(:ref_type) { 'invalid' }
-
-      it { is_expected.to eq(ref) }
-    end
-  end
-
   it_behaves_like 'extracts refs'
 end
-- 
GitLab


From fc5e9c8be99d70282a116863b7d1cb08e2208c87 Mon Sep 17 00:00:00 2001
From: Eulyeon Ko <eko@gitlab.com>
Date: Mon, 18 Sep 2023 10:36:09 +0900
Subject: [PATCH 4/7] Remove unused method

---
 lib/extracts_ref/ref_extractor.rb | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/lib/extracts_ref/ref_extractor.rb b/lib/extracts_ref/ref_extractor.rb
index ffdc9f1db9cc68..03d1eadcaadfe7 100644
--- a/lib/extracts_ref/ref_extractor.rb
+++ b/lib/extracts_ref/ref_extractor.rb
@@ -111,10 +111,6 @@ def extract_ref(id)
       ]
     end
 
-    def tree
-      @tree ||= @repo.tree(@commit.id, @path) # rubocop:disable Gitlab/ModuleWithInstanceVariables
-    end
-
     def extract_ref_path
       id = extract_id_from_params
       ref, path = extract_ref(id)
-- 
GitLab


From 9a56875adb343eafee924e0f56947bea942c76a2 Mon Sep 17 00:00:00 2001
From: Eulyeon Ko <eko@gitlab.com>
Date: Wed, 20 Sep 2023 15:47:46 +0900
Subject: [PATCH 5/7] Remove repo accessor

---
 app/controllers/concerns/snippets/blobs_actions.rb     | 10 ++--------
 lib/extracts_ref/ref_extractor.rb                      |  3 +--
 .../shared_examples/ref_extraction_shared_examples.rb  |  6 ------
 3 files changed, 3 insertions(+), 16 deletions(-)

diff --git a/app/controllers/concerns/snippets/blobs_actions.rb b/app/controllers/concerns/snippets/blobs_actions.rb
index fded6faf8b6aad..955debfc209e32 100644
--- a/app/controllers/concerns/snippets/blobs_actions.rb
+++ b/app/controllers/concerns/snippets/blobs_actions.rb
@@ -18,20 +18,14 @@ def raw
 
   private
 
-  def ref_extractor
-    @ref_extractor ||= ExtractsRef::RefExtractor.new(snippet, params.permit(:id, :ref, :path, :ref_type))
-  end
-
-  # rubocop:disable Gitlab/ModuleWithInstanceVariables
   def blob
+    ref_extractor = ExtractsRef::RefExtractor.new(snippet, params.permit(:id, :ref, :path, :ref_type))
     ref_extractor.extract!
-
     return unless ref_extractor.commit
 
-    ref_extractor.repo.blob_at(ref_extractor.commit.id, ref_extractor.path)
+    snippet.repository.blob_at(ref_extractor.commit.id, ref_extractor.path)
   end
   strong_memoize_attr :blob
-  # rubocop:enable Gitlab/ModuleWithInstanceVariables
 
   def ensure_blob
     render_404 unless blob
diff --git a/lib/extracts_ref/ref_extractor.rb b/lib/extracts_ref/ref_extractor.rb
index 03d1eadcaadfe7..ff632d49873602 100644
--- a/lib/extracts_ref/ref_extractor.rb
+++ b/lib/extracts_ref/ref_extractor.rb
@@ -11,7 +11,7 @@ class RefExtractor
     REF_TYPES = [BRANCH_REF_TYPE, TAG_REF_TYPE].freeze
 
     attr_reader :repository_container, :params
-    attr_accessor :id, :ref, :commit, :path, :fully_qualified_ref, :repo
+    attr_accessor :id, :ref, :commit, :path, :fully_qualified_ref
 
     def initialize(repository_container, params, override_id: nil)
       @repository_container = repository_container
@@ -29,7 +29,6 @@ def initialize(repository_container, params, override_id: nil)
     # - @path   - A string representing the filesystem path
     # - @commit - A Commit representing the commit from the given ref
     # - @fully_qualified_ref - A string representing the fully qualifed ref (e.g., refs/tags/v1.1)
-    # - @repo
     #
     # If the :id parameter appears to be requesting a specific response format,
     # that will be handled as well.
diff --git a/spec/support/shared_examples/ref_extraction_shared_examples.rb b/spec/support/shared_examples/ref_extraction_shared_examples.rb
index 4b0b3efd0b7a7b..6e2ef3ec591089 100644
--- a/spec/support/shared_examples/ref_extraction_shared_examples.rb
+++ b/spec/support/shared_examples/ref_extraction_shared_examples.rb
@@ -1,12 +1,6 @@
 # frozen_string_literal: true
 
 RSpec.shared_examples 'extracts ref vars' do
-  it 'extracts the repository var' do
-    ref_extractor.extract!
-
-    expect(ref_extractor.repo).to eq container.repository
-  end
-
   context 'when ref contains %20' do
     let(:ref) { 'foo%20bar' }
 
-- 
GitLab


From 354b189baaa79c898707fe9867062cc89b168844 Mon Sep 17 00:00:00 2001
From: Eulyeon Ko <eko@gitlab.com>
Date: Wed, 20 Sep 2023 16:03:25 +0900
Subject: [PATCH 6/7] Relocate class methods to the top section

---
 lib/extracts_ref/ref_extractor.rb | 42 ++++++++++++++++---------------
 1 file changed, 22 insertions(+), 20 deletions(-)

diff --git a/lib/extracts_ref/ref_extractor.rb b/lib/extracts_ref/ref_extractor.rb
index ff632d49873602..ac9b0ebb7afc19 100644
--- a/lib/extracts_ref/ref_extractor.rb
+++ b/lib/extracts_ref/ref_extractor.rb
@@ -13,6 +13,28 @@ class RefExtractor
     attr_reader :repository_container, :params
     attr_accessor :id, :ref, :commit, :path, :fully_qualified_ref
 
+    class << self
+      def ref_type(type)
+        return unless REF_TYPES.include?(type&.downcase)
+
+        type.downcase
+      end
+
+      def qualify_ref(ref, type)
+        validated_type = ref_type(type)
+        return ref unless validated_type
+
+        %(refs/#{validated_type}/#{ref})
+      end
+
+      def unqualify_ref(ref, type)
+        validated_type = ref_type(type)
+        return ref unless validated_type
+
+        ref.sub(%r{^refs/#{validated_type}/}, '')
+      end
+    end
+
     def initialize(repository_container, params, override_id: nil)
       @repository_container = repository_container
       @params = params.extract!(:id, :ref, :path, :ref_type)
@@ -48,26 +70,6 @@ def extract!
                 end
     end
 
-    def self.ref_type(type)
-      return unless REF_TYPES.include?(type&.downcase)
-
-      type.downcase
-    end
-
-    def self.qualify_ref(ref, type)
-      validated_type = ref_type(type)
-      return ref unless validated_type
-
-      %(refs/#{validated_type}/#{ref})
-    end
-
-    def self.unqualify_ref(ref, type)
-      validated_type = ref_type(type)
-      return ref unless validated_type
-
-      ref.sub(%r{^refs/#{validated_type}/}, '')
-    end
-
     # Given a string containing both a Git tree-ish, such as a branch or tag, and
     # a filesystem path joined by forward slashes, attempts to separate the two.
     #
-- 
GitLab


From 7de57f32beead00677a27b19c12287f46582fc0b Mon Sep 17 00:00:00 2001
From: Eulyeon Ko <eko@gitlab.com>
Date: Wed, 20 Sep 2023 16:06:33 +0900
Subject: [PATCH 7/7] Apply maintainer suggestions

---
 .../ref_extraction_shared_examples.rb         | 70 ++++++++++---------
 1 file changed, 36 insertions(+), 34 deletions(-)

diff --git a/spec/support/shared_examples/ref_extraction_shared_examples.rb b/spec/support/shared_examples/ref_extraction_shared_examples.rb
index 6e2ef3ec591089..f51c3a16406411 100644
--- a/spec/support/shared_examples/ref_extraction_shared_examples.rb
+++ b/spec/support/shared_examples/ref_extraction_shared_examples.rb
@@ -1,58 +1,60 @@
 # frozen_string_literal: true
 
 RSpec.shared_examples 'extracts ref vars' do
-  context 'when ref contains %20' do
-    let(:ref) { 'foo%20bar' }
+  describe '#extract!' do
+    context 'when ref contains %20' do
+      let(:ref) { 'foo%20bar' }
 
-    it 'is not converted to a space in @id' do
-      container.repository.add_branch(owner, 'foo%20bar', 'master')
+      it 'is not converted to a space in @id' do
+        container.repository.add_branch(owner, 'foo%20bar', 'master')
 
-      ref_extractor.extract!
+        ref_extractor.extract!
 
-      expect(ref_extractor.id).to start_with('foo%20bar/')
+        expect(ref_extractor.id).to start_with('foo%20bar/')
+      end
     end
-  end
 
-  context 'when ref contains trailing space' do
-    let(:ref) { 'master ' }
+    context 'when ref contains trailing space' do
+      let(:ref) { 'master ' }
 
-    it 'strips surrounding space' do
-      ref_extractor.extract!
+      it 'strips surrounding space' do
+        ref_extractor.extract!
 
-      expect(ref_extractor.ref).to eq('master')
+        expect(ref_extractor.ref).to eq('master')
+      end
     end
-  end
 
-  context 'when ref contains leading space' do
-    let(:ref) { ' master ' }
+    context 'when ref contains leading space' do
+      let(:ref) { ' master ' }
 
-    it 'strips surrounding space' do
-      ref_extractor.extract!
+      it 'strips surrounding space' do
+        ref_extractor.extract!
 
-      expect(ref_extractor.ref).to eq('master')
+        expect(ref_extractor.ref).to eq('master')
+      end
     end
-  end
 
-  context 'when path contains space' do
-    let(:ref) { '38008cb17ce1466d8fec2dfa6f6ab8dcfe5cf49e' }
-    let(:path) { 'with space' }
+    context 'when path contains space' do
+      let(:ref) { '38008cb17ce1466d8fec2dfa6f6ab8dcfe5cf49e' }
+      let(:path) { 'with space' }
 
-    it 'is not converted to %20 in @path' do
-      ref_extractor.extract!
+      it 'is not converted to %20 in @path' do
+        ref_extractor.extract!
 
-      expect(ref_extractor.path).to eq(path)
+        expect(ref_extractor.path).to eq(path)
+      end
     end
-  end
 
-  context 'when override_id is given' do
-    let(:ref_extractor) do
-      described_class.new(container, params, override_id: '38008cb17ce1466d8fec2dfa6f6ab8dcfe5cf49e')
-    end
+    context 'when override_id is given' do
+      let(:ref_extractor) do
+        described_class.new(container, params, override_id: '38008cb17ce1466d8fec2dfa6f6ab8dcfe5cf49e')
+      end
 
-    it 'uses override_id' do
-      ref_extractor.extract!
+      it 'uses override_id' do
+        ref_extractor.extract!
 
-      expect(ref_extractor.id).to eq('38008cb17ce1466d8fec2dfa6f6ab8dcfe5cf49e')
+        expect(ref_extractor.id).to eq('38008cb17ce1466d8fec2dfa6f6ab8dcfe5cf49e')
+      end
     end
   end
 end
@@ -75,7 +77,7 @@
         expect(ref_extractor.extract_ref('v2.0.0')).to eq(['v2.0.0', ''])
       end
 
-      it 'extracts a valid commit ref without a path' do
+      it 'extracts a valid commit ref' do
         expect(ref_extractor.extract_ref('f4b14494ef6abf3d144c28e4af0c20143383e062')).to eq(
           ['f4b14494ef6abf3d144c28e4af0c20143383e062', '']
         )
-- 
GitLab