Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix support of formulae aliases in taps #16637

Merged
merged 4 commits into from
Feb 11, 2024
+11 −4
Merged

Fix support of formulae aliases in taps #16637

Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
1570a4b
Fix support of formulae aliases in taps
sjorek Feb 10, 2024
863654e
remove accidentally added newline
sjorek Feb 11, 2024
d01de75
refactor bugfix to satify brew audit
sjorek Feb 11, 2024
70ffc31
formulary: update logic and add regression test
apainintheneck Feb 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion Library/Homebrew/formulary.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -906,8 +906,9 @@ def self.tap_formula_name_type(tapped_name, warn:)
user, repo, name = tapped_name.split("/", 3).map(&:downcase)
tap = Tap.fetch user, repo
type = nil
alias_name = tap.core_tap? ? name : "#{tap}/#{name}"

if (possible_alias = tap.alias_table[name].presence)
if (possible_alias = tap.alias_table[alias_name].presence)
name = possible_alias
type = :alias
elsif (new_name = tap.formula_renames[name].presence)
Expand Down Expand Up @@ -949,6 +950,7 @@ def self.tap_loader_for(tapped_name, warn:)
end
end

name = name.split("/").last if name != :nil && type == :alias
Copy link
Member

@reitermarkus reitermarkus Feb 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As mentioned in #16623 (comment), this is the exact same fix. Can you move the split into the conditional above to avoid checking another condition here?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, sorry, your suggestion is simpler than what I proposed earlier. Using String#split should avoid the nil type errors entirely too.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@apainintheneck I'm not sure who is targeted by @reitermarkus's wish to move the split - shall I do it?
And if yes; I'm not sure where exactly to move it to?

So like this?

diff --git a/Library/Homebrew/formulary.rb b/Library/Homebrew/formulary.rb
index 9a1a7cc2c..cabc1f74a 100644
--- a/Library/Homebrew/formulary.rb
+++ b/Library/Homebrew/formulary.rb
@@ -909,7 +909,7 @@ module Formulary
     alias_name = tap.core_tap? ? name : "#{tap}/#{name}"
 
     if (possible_alias = tap.alias_table[alias_name].presence)
-      name = possible_alias
+      name = tap.core_tap? ? possible_alias : possible_alias.split("/").last
       type = :alias
     elsif (new_name = tap.formula_renames[name].presence)
       old_name = name
@@ -950,7 +950,6 @@ module Formulary
       end
     end
 
-    name = name.split("/").last if name != :nil && type == :alias
     path = find_formula_in_tap(name, tap)
     TapLoader.new(name, path, tap: tap)
   end

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or like this?

diff --git a/Library/Homebrew/formulary.rb b/Library/Homebrew/formulary.rb
index 9a1a7cc2c..952ee50fd 100644
--- a/Library/Homebrew/formulary.rb
+++ b/Library/Homebrew/formulary.rb
@@ -948,9 +948,10 @@ module Formulary
       elsif Homebrew::API::Formula.all_formulae.key?(name)
         return FormulaAPILoader.new(name)
       end
+    elsif name != :nil && type == :alias
+      name = name.split("/").last
     end
 
-    name = name.split("/").last if name != :nil && type == :alias
     path = find_formula_in_tap(name, tap)
     TapLoader.new(name, path, tap: tap)
   end

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The latter seems to be error-prone … eg. when tap.core_tap? && !Homebrew::EnvConfig.no_install_from_api? is true and the API-lookup Homebrew::API::Formula.all_formulae.key?(name) is false.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@apainintheneck I'm not sure who is targeted by @reitermarkus's wish to move the split - shall I do it? And if yes; I'm not sure where exactly to move it to?

So like this?

diff --git a/Library/Homebrew/formulary.rb b/Library/Homebrew/formulary.rb
index 9a1a7cc2c..cabc1f74a 100644
--- a/Library/Homebrew/formulary.rb
+++ b/Library/Homebrew/formulary.rb
@@ -909,7 +909,7 @@ module Formulary
     alias_name = tap.core_tap? ? name : "#{tap}/#{name}"
 
     if (possible_alias = tap.alias_table[alias_name].presence)
-      name = possible_alias
+      name = tap.core_tap? ? possible_alias : possible_alias.split("/").last
       type = :alias
     elsif (new_name = tap.formula_renames[name].presence)
       old_name = name
@@ -950,7 +950,6 @@ module Formulary
       end
     end
 
-    name = name.split("/").last if name != :nil && type == :alias
     path = find_formula_in_tap(name, tap)
     TapLoader.new(name, path, tap: tap)
   end

This is pretty much what I was thinking. I'll update it now and test it locally to make sure it works.

path = find_formula_in_tap(name, tap)
TapLoader.new(name, path, tap: tap)
end
Expand Down
Loading