Support loading formulae/casks from subdirectories

Previously, we required all formulae and casks to be in a specific
formula or cask directory but did not check any subdirectories.

This commit allows using subdirectories for official taps, the only
ones likely to be big enough to warrant sharding in this way and to
avoid potentially breaking backwards compatibility for existing taps.

This was inspired by the most recent issues with homebrew-cask.

Library/Homebrew/cask/cask_loader.rb

@@ -167,7 +167,9 @@ module Cask
 
       def initialize(tapped_name)
         user, repo, token = tapped_name.split("/", 3)
-        super Tap.fetch(user, repo).cask_dir/"#{token}.rb"
+        tap = Tap.fetch(user, repo)
+        cask = CaskLoader.find_cask_in_tap(token, tap)
+        super cask
       end
 
       def load(config:)
@@ -421,8 +423,15 @@ module Cask
     end
 
     def self.tap_paths(token)
-      Tap.map { |t| t.cask_dir/"#{token.to_s.downcase}.rb" }
-         .select(&:exist?)
+      Tap.map do |tap|
+        find_cask_in_tap(token.to_s.downcase, tap)
+      end.select(&:exist?)
+    end
+
+    def self.find_cask_in_tap(token, tap)
+      filename = "#{token}.rb"
+
+      Tap.cask_files_by_name(tap).fetch(filename, tap.cask_dir/filename)
     end
   end
 end

Library/Homebrew/cmd/tap-info.rb

@@ -50,17 +50,14 @@ module Homebrew
       tap_count = 0
       formula_count = 0
       command_count = 0
-      pinned_count = 0
       private_count = 0
       Tap.each do |tap|
         tap_count += 1
         formula_count += tap.formula_files.size
         command_count += tap.command_files.size
-        pinned_count += 1 if tap.pinned?
         private_count += 1 if tap.private?
       end
       info = "#{tap_count} #{"tap".pluralize(tap_count)}"
-      info += ", #{pinned_count} pinned"
       info += ", #{private_count} private"
       info += ", #{formula_count} #{"formula".pluralize(formula_count)}"
       info += ", #{command_count} #{"command".pluralize(command_count)}"

Library/Homebrew/formulary.rb

@@ -516,15 +516,14 @@ module Formulary
     def formula_name_path(tapped_name, warn: true)
       user, repo, name = tapped_name.split("/", 3).map(&:downcase)
       @tap = Tap.fetch user, repo
-      formula_dir = @tap.formula_dir || @tap.path
-      path = formula_dir/"#{name}.rb"
+      path = find_formula_from_name(name)
 
       unless path.file?
         if (possible_alias = @tap.alias_dir/name).file?
           path = possible_alias.resolved_path
           name = path.basename(".rb").to_s
         elsif (new_name = @tap.formula_renames[name]) &&
-              (new_path = formula_dir/"#{new_name}.rb").file?
+              (new_path = find_formula_from_name(new_name)).file?
           old_name = name
           path = new_path
           name = new_name
@@ -562,6 +561,12 @@ module Formulary
       e.issues_url = tap.issues_url || tap.to_s
       raise
     end
+
+    private
+
+    def find_formula_from_name(name)
+      Formulary.find_formula_in_tap(name, @tap)
+    end
   end
 
   # Pseudo-loader which will raise a {FormulaUnavailableError} when trying to load the corresponding formula.
@@ -800,22 +805,28 @@ module Formulary
   end
 
   def self.core_path(name)
-    CoreTap.instance.formula_dir/"#{name.to_s.downcase}.rb"
+    find_formula_in_tap(name.to_s.downcase, CoreTap.instance)
   end
 
   def self.core_alias_path(name)
     CoreTap.instance.alias_dir/name.to_s.downcase
   end
 
-  def self.tap_paths(name, taps = Dir[HOMEBREW_LIBRARY/"Taps/*/*/"])
+  def self.tap_paths(name, taps = Tap)
     name = name.to_s.downcase
     taps.map do |tap|
-      Pathname.glob([
-        "#{tap}Formula/#{name}.rb",
-        "#{tap}HomebrewFormula/#{name}.rb",
-        "#{tap}#{name}.rb",
-        "#{tap}Aliases/#{name}",
-      ]).find(&:file?)
-    end.compact
+      formula_path = find_formula_in_tap(name, tap)
+
+      alias_path = tap.alias_dir/name
+      next alias_path if !formula_path.exist? && alias_path.exist?
+
+      formula_path
+    end.select(&:file?)
+  end
+
+  def self.find_formula_in_tap(name, tap)
+    filename = "#{name}.rb"
+
+    Tap.formula_files_by_name(tap).fetch(filename, tap.formula_dir/filename)
   end
 end

Library/Homebrew/tap.rb

@@ -1,4 +1,4 @@
-# typed: false
+# typed: true
 # frozen_string_literal: true
 
 require "commands"
@@ -416,7 +416,6 @@ class Tap
     abv = path.abv
     formatted_contents = contents.presence&.to_sentence&.dup&.prepend(" ")
 
-    unpin if pinned?
     CacheStoreDatabase.use(:descriptions) do |db|
       DescriptionCacheStore.new(db)
                            .delete_from_formula_names!(formula_names)
@@ -451,15 +450,23 @@ class Tap
   end
 
   # Path to the directory of all {Formula} files for this {Tap}.
+  sig { returns(Pathname) }
   def formula_dir
-    @formula_dir ||= potential_formula_dirs.find(&:directory?) || (path/"Formula")
+    # Official formulae taps always use this directory, saves time to hardcode.
+    @formula_dir ||= if official?
+      path/"Formula"
+    else
+      potential_formula_dirs.find(&:directory?) || (path/"Formula")
+    end
   end
 
+  sig { returns(T::Array[Pathname]) }
   def potential_formula_dirs
     @potential_formula_dirs ||= [path/"Formula", path/"HomebrewFormula", path].freeze
   end
 
   # Path to the directory of all {Cask} files for this {Tap}.
+  sig { returns(Pathname) }
   def cask_dir
     @cask_dir ||= path/"Casks"
   end
@@ -483,15 +490,35 @@ class Tap
   end
 
   # An array of all {Formula} files of this {Tap}.
+  sig { returns(T::Array[Pathname]) }
   def formula_files
     @formula_files ||= if formula_dir.directory?
-      formula_dir.children.select(&method(:ruby_file?))
+      # TODO: odeprecate the non-official/old logic with a new minor release somehow?
+      if official?
+        formula_dir.find
+      else
+        formula_dir.children
+      end.select(&method(:ruby_file?))
     else
       []
     end
   end
 
+  # A cached hash of {Formula} basenames to {Formula} file pathnames for a {Tap}
+  sig { params(tap: Tap).returns(T::Hash[String, Pathname]) }
+  def self.formula_files_by_name(tap)
+    cache_key = "formula_files_by_name_#{tap}"
+    cache.fetch(cache_key) do |key|
+      cache[key] = tap.formula_files.each_with_object({}) do |file, hash|
+        # If there's more than one file with the same basename: intentionally
+        # ignore the later ones here.
+        hash[file.basename.to_s] ||= file
+      end
+    end
+  end
+
   # An array of all versioned {Formula} files of this {Tap}.
+  sig { returns(T::Array[Pathname]) }
   def versioned_formula_files
     @versioned_formula_files ||= formula_files.select do |file|
       file.basename(".rb").to_s =~ /@[\d.]+$/
@@ -499,16 +526,37 @@ class Tap
   end
 
   # An array of all {Cask} files of this {Tap}.
+  sig { returns(T::Array[Pathname]) }
   def cask_files
     @cask_files ||= if cask_dir.directory?
-      cask_dir.children.select(&method(:ruby_file?))
+      # TODO: odeprecate the non-official/old logic with a new minor release somehow?
+      if official?
+        cask_dir.find
+      else
+        cask_dir.children
+      end.select(&method(:ruby_file?))
     else
       []
     end
   end
 
+  # A cached hash of {Cask} basenames to {Cask} file pathnames for a {Tap}
+  sig { params(tap: Tap).returns(T::Hash[String, Pathname]) }
+  def self.cask_files_by_name(tap)
+    cache_key = "cask_files_by_name_#{tap}"
+
+    cache.fetch(cache_key) do |key|
+      cache[key] = tap.cask_files.each_with_object({}) do |file, hash|
+        # If there's more than one file with the same basename: intentionally
+        # ignore the later ones here.
+        hash[file.basename.to_s] ||= file
+      end
+    end
+  end
+
   # returns true if the file has a Ruby extension
   # @private
+  sig { params(file: Pathname).returns(T::Boolean) }
   def ruby_file?(file)
     file.extname == ".rb"
   end
@@ -516,45 +564,56 @@ class Tap
   # return true if given path would present a {Formula} file in this {Tap}.
   # accepts both absolute path and relative path (relative to this {Tap}'s path)
   # @private
+  sig { params(file: T.any(String, Pathname)).returns(T::Boolean) }
   def formula_file?(file)
     file = Pathname.new(file) unless file.is_a? Pathname
     file = file.expand_path(path)
-    ruby_file?(file) && file.parent == formula_dir
+    return false unless ruby_file?(file)
+
+    file.to_s.start_with?("#{formula_dir}/")
   end
 
   # return true if given path would present a {Cask} file in this {Tap}.
   # accepts both absolute path and relative path (relative to this {Tap}'s path)
   # @private
+  sig { params(file: T.any(String, Pathname)).returns(T::Boolean) }
   def cask_file?(file)
     file = Pathname.new(file) unless file.is_a? Pathname
     file = file.expand_path(path)
-    ruby_file?(file) && file.parent == cask_dir
+    return false unless ruby_file?(file)
+
+    file.to_s.start_with?("#{cask_dir}/")
   end
 
   # An array of all {Formula} names of this {Tap}.
+  sig { returns(T::Array[String]) }
   def formula_names
     @formula_names ||= formula_files.map(&method(:formula_file_to_name))
   end
 
   # An array of all {Cask} tokens of this {Tap}.
+  sig { returns(T::Array[String]) }
   def cask_tokens
     @cask_tokens ||= cask_files.map(&method(:formula_file_to_name))
   end
 
   # path to the directory of all alias files for this {Tap}.
   # @private
+  sig { returns(Pathname) }
   def alias_dir
     @alias_dir ||= path/"Aliases"
   end
 
   # an array of all alias files of this {Tap}.
   # @private
+  sig { returns(T::Array[Pathname]) }
   def alias_files
     @alias_files ||= Pathname.glob("#{alias_dir}/*").select(&:file?)
   end
 
   # an array of all aliases of this {Tap}.
   # @private
+  sig { returns(T::Array[String]) }
   def aliases
     @aliases ||= alias_files.map { |f| alias_file_to_name(f) }
   end
@@ -584,11 +643,13 @@ class Tap
     @alias_reverse_table
   end
 
+  sig { returns(Pathname) }
   def command_dir
     @command_dir ||= path/"cmd"
   end
 
   # An array of all commands files of this {Tap}.
+  sig { returns(T::Array[Pathname]) }
   def command_files
     @command_files ||= if command_dir.directory?
       Commands.find_commands(command_dir)
@@ -597,19 +658,7 @@ class Tap
     end
   end
 
-  # path to the pin record for this {Tap}.
-  # @private
-  def pinned_symlink_path
-    HOMEBREW_LIBRARY/"PinnedTaps/#{name}"
-  end
-
-  # True if this {Tap} has been pinned.
-  def pinned?
-    return @pinned if instance_variable_defined?(:@pinned)
-
-    @pinned = pinned_symlink_path.directory?
-  end
-
+  sig { returns(Hash) }
   def to_hash
     hash = {
       "name"          => name,
@@ -635,6 +684,7 @@ class Tap
   end
 
   # Hash with tap formula renames.
+  sig { returns(Hash) }
   def formula_renames
     @formula_renames ||= if (rename_file = path/HOMEBREW_TAP_FORMULA_RENAMES_FILE).file?
       JSON.parse(rename_file.read)
@@ -644,6 +694,7 @@ class Tap
   end
 
   # Hash with tap migrations.
+  sig { returns(Hash) }
   def tap_migrations
     @tap_migrations ||= if (migration_file = path/HOMEBREW_TAP_MIGRATIONS_FILE).file?
       JSON.parse(migration_file.read)
@@ -665,18 +716,19 @@ class Tap
   end
 
   # Hash with pypi formula mappings
-  sig { returns(Hash) }
   def pypi_formula_mappings
     @pypi_formula_mappings = read_formula_list path/HOMEBREW_TAP_PYPI_FORMULA_MAPPINGS
   end
 
   # @private
+  sig { returns(T::Boolean) }
   def should_report_analytics?
     return Homebrew::EnvConfig.install_from_api? && official? unless installed?
 
     !private?
   end
 
+  sig { params(other: T.nilable(T.any(String, Tap))).returns(T::Boolean) }
   def ==(other)
     other = Tap.fetch(other) if other.is_a?(String)
     self.class == other.class && name == other.name
@@ -695,6 +747,7 @@ class Tap
   end
 
   # An array of all installed {Tap} names.
+  sig { returns(T::Array[String]) }
   def self.names
     map(&:name).sort
   end
@@ -712,11 +765,13 @@ class Tap
   end
 
   # @private
+  sig { params(file: Pathname).returns(String) }
   def formula_file_to_name(file)
     "#{name}/#{file.basename(".rb")}"
   end
 
   # @private
+  sig { params(file: Pathname).returns(String) }
   def alias_file_to_name(file)
     "#{name}/#{file.basename}"
   end
@@ -796,10 +851,12 @@ class CoreTap < Tap
     super "Homebrew", "core"
   end
 
+  sig { returns(CoreTap) }
   def self.instance
     @instance ||= new
   end
 
+  sig { void }
   def self.ensure_installed!
     return if instance.installed?
     return if Homebrew::EnvConfig.install_from_api?
@@ -811,6 +868,7 @@ class CoreTap < Tap
     instance.install
   end
 
+  sig { returns(String) }
   def remote
     super if installed? || !Homebrew::EnvConfig.install_from_api?
 
@@ -840,24 +898,6 @@ class CoreTap < Tap
     super
   end
 
-  # @private
-  sig { void }
-  def pin
-    raise "Tap#pin is not available for CoreTap"
-  end
-
-  # @private
-  sig { void }
-  def unpin
-    raise "Tap#unpin is not available for CoreTap"
-  end
-
-  # @private
-  sig { returns(T::Boolean) }
-  def pinned?
-    false
-  end
-
   # @private
   sig { returns(T::Boolean) }
   def core_tap?
@@ -871,6 +911,7 @@ class CoreTap < Tap
   end
 
   # @private
+  sig { returns(Pathname) }
   def formula_dir
     @formula_dir ||= begin
       self.class.ensure_installed!
@@ -879,6 +920,7 @@ class CoreTap < Tap
   end
 
   # @private
+  sig { returns(Pathname) }
   def alias_dir
     @alias_dir ||= begin
       self.class.ensure_installed!
@@ -887,6 +929,7 @@ class CoreTap < Tap
   end
 
   # @private
+  sig { returns(Hash) }
   def formula_renames
     @formula_renames ||= begin
       self.class.ensure_installed!
@@ -895,6 +938,7 @@ class CoreTap < Tap
   end
 
   # @private
+  sig { returns(Hash) }
   def tap_migrations
     @tap_migrations ||= begin
       self.class.ensure_installed!
@@ -903,6 +947,7 @@ class CoreTap < Tap
   end
 
   # @private
+  sig { returns(Hash) }
   def audit_exceptions
     @audit_exceptions ||= begin
       self.class.ensure_installed!
@@ -911,6 +956,7 @@ class CoreTap < Tap
   end
 
   # @private
+  sig { returns(Hash) }
   def style_exceptions
     @style_exceptions ||= begin
       self.class.ensure_installed!
@@ -919,6 +965,7 @@ class CoreTap < Tap
   end
 
   # @private
+  sig { returns(Hash) }
   def pypi_formula_mappings
     @pypi_formula_mappings ||= begin
       self.class.ensure_installed!
@@ -927,16 +974,19 @@ class CoreTap < Tap
   end
 
   # @private
+  sig { params(file: Pathname).returns(String) }
   def formula_file_to_name(file)
     file.basename(".rb").to_s
   end
 
   # @private
+  sig { params(file: Pathname).returns(String) }
   def alias_file_to_name(file)
     file.basename.to_s
   end
 
   # @private
+  sig { returns(T::Array[String]) }
   def aliases
     return super if installed? || !Homebrew::EnvConfig.install_from_api?
 
@@ -944,6 +994,7 @@ class CoreTap < Tap
   end
 
   # @private
+  sig { returns(T::Array[String]) }
   def formula_names
     return super if installed? || !Homebrew::EnvConfig.install_from_api?
 
@@ -953,8 +1004,11 @@ end
 
 # Permanent configuration per {Tap} using `git-config(1)`.
 class TapConfig
+  extend T::Sig
+
   attr_reader :tap
 
+  sig { params(tap: Tap).void }
   def initialize(tap)
     @tap = tap
   end

Library/Homebrew/test/cask/cask_loader/from_tap_loader_spec.rb

@@ -0,0 +1,35 @@
+# typed: false
+# frozen_string_literal: true
+
+describe Cask::CaskLoader::FromTapLoader do
+  let(:cask_name) { "testball" }
+  let(:cask_full_name) { "homebrew/cask/#{cask_name}" }
+  let(:cask_path) { Tap.default_cask_tap.cask_dir/"#{cask_name}.rb" }
+
+  describe "#load" do
+    before do
+      cask_path.parent.mkpath
+      cask_path.write <<~RUBY
+        cask '#{cask_name}' do
+          url 'https://brew.sh/'
+        end
+      RUBY
+    end
+
+    it "returns a Cask" do
+      expect(described_class.new(cask_full_name).load(config: nil)).to be_a(Cask::Cask)
+    end
+
+    it "raises an error if the Cask cannot be found" do
+      expect { described_class.new("foo/bar/baz").load(config: nil) }.to raise_error(Cask::CaskUnavailableError)
+    end
+
+    context "with sharded Cask directory" do
+      let(:cask_path) { Tap.default_cask_tap.cask_dir/cask_name[0]/"#{cask_name}.rb" }
+
+      it "returns a Cask" do
+        expect(described_class.new(cask_full_name).load(config: nil)).to be_a(Cask::Cask)
+      end
+    end
+  end
+end

Library/Homebrew/test/formulary_spec.rb

@@ -81,6 +81,21 @@ describe Formulary do
       }.to raise_error(ArgumentError)
     end
 
+    context "with sharded Formula directory" do
+      before { CoreTap.instance.clear_cache }
+
+      let(:formula_name) { "testball_sharded" }
+      let(:formula_path) { CoreTap.new.formula_dir/formula_name[0]/"#{formula_name}.rb" }
+
+      it "returns a Formula" do
+        expect(described_class.factory(formula_name)).to be_a(Formula)
+      end
+
+      it "returns a Formula when given a fully qualified name" do
+        expect(described_class.factory("homebrew/core/#{formula_name}")).to be_a(Formula)
+      end
+    end
+
     context "when the Formula has the wrong class" do
       let(:formula_name) { "giraffe" }
       let(:formula_content) do
@@ -171,7 +186,7 @@ describe Formulary do
     context "when loading from Tap" do
       let(:tap) { Tap.new("homebrew", "foo") }
       let(:another_tap) { Tap.new("homebrew", "bar") }
-      let(:formula_path) { tap.path/"#{formula_name}.rb" }
+      let(:formula_path) { tap.path/"Formula/#{formula_name}.rb" }
 
       it "returns a Formula when given a name" do
         expect(described_class.factory(formula_name)).to be_a(Formula)
@@ -195,8 +210,8 @@ describe Formulary do
       end
 
       it "raises an error if a Formula is in multiple Taps" do
-        another_tap.path.mkpath
-        (another_tap.path/"#{formula_name}.rb").write formula_content
+        (another_tap.path/"Formula").mkpath
+        (another_tap.path/"Formula/#{formula_name}.rb").write formula_content
 
         expect {
           described_class.factory(formula_name)

Library/Homebrew/test/missing_formula_spec.rb

@@ -61,8 +61,8 @@ describe Homebrew::MissingFormula do
 
     before do
       tap_path = Tap::TAP_DIRECTORY/"homebrew/homebrew-foo"
-      tap_path.mkpath
-      (tap_path/"deleted-formula.rb").write "placeholder"
+      (tap_path/"Formula").mkpath
+      (tap_path/"Formula/deleted-formula.rb").write "placeholder"
       ENV.delete "GIT_AUTHOR_DATE"
       ENV.delete "GIT_COMMITTER_DATE"
 
@@ -70,7 +70,7 @@ describe Homebrew::MissingFormula do
         system "git", "init"
         system "git", "add", "--all"
         system "git", "commit", "-m", "initial state"
-        system "git", "rm", "deleted-formula.rb"
+        system "git", "rm", "Formula/deleted-formula.rb"
         system "git", "commit", "-m", "delete formula 'deleted-formula'"
       end
     end

Library/Homebrew/test/tap_spec.rb

@@ -528,15 +528,12 @@ describe Tap do
       expect(core_tap.name).to eq("homebrew/core")
       expect(core_tap.command_files).to eq([])
       expect(core_tap).to be_installed
-      expect(core_tap).not_to be_pinned
       expect(core_tap).to be_official
       expect(core_tap).to be_a_core_tap
     end
 
     specify "forbidden operations" do
       expect { core_tap.uninstall }.to raise_error(RuntimeError)
-      expect { core_tap.pin }.to raise_error(RuntimeError)
-      expect { core_tap.unpin }.to raise_error(RuntimeError)
     end
 
     specify "files" do