Merge pull request #18167 from Homebrew/fix-cask-formula-file-validat…

…ion-v2
Homebrew · Aug 29, 2024 · 638a3dc · 638a3dc
2 parents 06fc224 + 60b8878
commit 638a3dc
Show file tree

Hide file tree

Showing 2 changed files with 126 additions and 23 deletions.
diff --git a/Library/Homebrew/tap.rb b/Library/Homebrew/tap.rb
@@ -753,34 +753,40 @@ def cask_files_by_name
     end
   end
 
-  # Check whether the file has a Ruby extension.
-  sig { params(file: Pathname).returns(T::Boolean) }
-  def ruby_file?(file)
-    file.extname == ".rb"
+  RUBY_FILE_NAME_REGEX = %r{[^/]+\.rb}
+  private_constant :RUBY_FILE_NAME_REGEX
+
+  ZERO_OR_MORE_SUBDIRECTORIES_REGEX = %r{(?:[^/]+/)*}
+  private_constant :ZERO_OR_MORE_SUBDIRECTORIES_REGEX
+
+  sig { returns(Regexp) }
+  def formula_file_regex
+    @formula_file_regex ||= case formula_dir
+    when path/"Formula"
+      %r{^Formula/#{ZERO_OR_MORE_SUBDIRECTORIES_REGEX.source}#{RUBY_FILE_NAME_REGEX.source}$}o
+    when path/"HomebrewFormula"
+      %r{^HomebrewFormula/#{ZERO_OR_MORE_SUBDIRECTORIES_REGEX.source}#{RUBY_FILE_NAME_REGEX.source}$}o
+    when path
+      /^#{RUBY_FILE_NAME_REGEX.source}$/o
+    else
+      raise ArgumentError, "Unexpected formula_dir: #{formula_dir}"
+    end
   end
-  private :ruby_file?
+  private :formula_file_regex
 
-  # Check whether the given path would present a {Formula} file in this {Tap}.
-  # Accepts either an absolute path or a path relative to this {Tap}'s path.
-  sig { params(file: T.any(String, Pathname)).returns(T::Boolean) }
+  # accepts the relative path of a file from {Tap}'s path
+  sig { params(file: String).returns(T::Boolean) }
   def formula_file?(file)
-    file = Pathname.new(file) unless file.is_a? Pathname
-    file = file.expand_path(path)
-    return false unless ruby_file?(file)
-    return false if cask_file?(file)
-
-    file.to_s.start_with?("#{formula_dir}/")
+    file.match?(formula_file_regex)
   end
 
-  # Check whether the given path would present a {Cask} file in this {Tap}.
-  # Accepts either an absolute path or a path relative to this {Tap}'s path.
-  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)
-    return false unless ruby_file?(file)
+  CASK_FILE_REGEX = %r{^Casks/#{ZERO_OR_MORE_SUBDIRECTORIES_REGEX.source}#{RUBY_FILE_NAME_REGEX.source}$}
+  private_constant :CASK_FILE_REGEX
 
-    file.to_s.start_with?("#{cask_dir}/")
+  # accepts the relative path of a file from {Tap}'s path
+  sig { params(file: String).returns(T::Boolean) }
+  def cask_file?(file)
+    file.match?(CASK_FILE_REGEX)
   end
 
   # An array of all {Formula} names of this {Tap}.

diff --git a/Library/Homebrew/test/tap_spec.rb b/Library/Homebrew/test/tap_spec.rb
@@ -3,6 +3,7 @@
 RSpec.describe Tap do
   include FileUtils
 
+  alias_matcher :have_cask_file, :be_cask_file
   alias_matcher :have_formula_file, :be_formula_file
   alias_matcher :have_custom_remote, :be_custom_remote
 
@@ -201,7 +202,6 @@ def setup_completion(link:)
     expect(homebrew_foo_tap.tap_migrations).to eq("removed-formula" => "homebrew/foo")
     expect(homebrew_foo_tap.command_files).to eq([cmd_file])
     expect(homebrew_foo_tap.to_hash).to be_a(Hash)
-    expect(homebrew_foo_tap).to have_formula_file(formula_file)
     expect(homebrew_foo_tap).to have_formula_file("Formula/foo.rb")
     expect(homebrew_foo_tap).not_to have_formula_file("bar.rb")
     expect(homebrew_foo_tap).not_to have_formula_file("Formula/baz.sh")
@@ -632,6 +632,103 @@ def setup_completion(link:)
         expect(homebrew_foo_tap.pypi_formula_mappings).to eq expected_result
       end
     end
+
+    describe "#formula_file?" do
+      it "matches files from Formula/" do
+        tap = described_class.fetch("hard/core")
+        FileUtils.mkdir_p(tap.path/"Formula")
+
+        %w[
+          kvazaar.rb
+          Casks/kvazaar.rb
+          Casks/k/kvazaar.rb
+          Formula/kvazaar.sh
+          HomebrewFormula/kvazaar.rb
+          HomebrewFormula/k/kvazaar.rb
+        ].each do |relative_path|
+          expect(tap).not_to have_formula_file(relative_path)
+        end
+
+        %w[
+          Formula/kvazaar.rb
+          Formula/k/kvazaar.rb
+        ].each do |relative_path|
+          expect(tap).to have_formula_file(relative_path)
+        end
+      ensure
+        FileUtils.rm_rf(tap.path.parent) if tap
+      end
+
+      it "matches files from HomebrewFormula/" do
+        tap = described_class.fetch("hard/core")
+        FileUtils.mkdir_p(tap.path/"HomebrewFormula")
+
+        %w[
+          kvazaar.rb
+          Casks/kvazaar.rb
+          Casks/k/kvazaar.rb
+          Formula/kvazaar.rb
+          Formula/k/kvazaar.rb
+          HomebrewFormula/kvazaar.sh
+        ].each do |relative_path|
+          expect(tap).not_to have_formula_file(relative_path)
+        end
+
+        %w[
+          HomebrewFormula/kvazaar.rb
+          HomebrewFormula/k/kvazaar.rb
+        ].each do |relative_path|
+          expect(tap).to have_formula_file(relative_path)
+        end
+      ensure
+        FileUtils.rm_rf(tap.path.parent) if tap
+      end
+
+      it "matches files from the top-level directory" do
+        tap = described_class.fetch("hard/core")
+        FileUtils.mkdir_p(tap.path)
+
+        %w[
+          kvazaar.sh
+          Casks/kvazaar.rb
+          Casks/k/kvazaar.rb
+          Formula/kvazaar.rb
+          Formula/k/kvazaar.rb
+          HomebrewFormula/kvazaar.rb
+          HomebrewFormula/k/kvazaar.rb
+        ].each do |relative_path|
+          expect(tap).not_to have_formula_file(relative_path)
+        end
+
+        expect(tap).to have_formula_file("kvazaar.rb")
+      ensure
+        FileUtils.rm_rf(tap.path.parent) if tap
+      end
+    end
+
+    describe "#cask_file?" do
+      it "matches files from Casks/" do
+        tap = described_class.fetch("hard/core")
+
+        %w[
+          kvazaar.rb
+          Casks/kvazaar.sh
+          Formula/kvazaar.rb
+          Formula/k/kvazaar.rb
+          HomebrewFormula/kvazaar.rb
+          HomebrewFormula/k/kvazaar.rb
+        ].each do |relative_path|
+          expect(tap).not_to have_cask_file(relative_path)
+        end
+
+        %w[
+          Casks/kvazaar.rb
+          Casks/k/kvazaar.rb
+        ].each do |relative_path|
+          expect(tap).to have_cask_file(relative_path)
+        end
+      end
+    end
   end
 
   describe CoreTap do