-
-
Notifications
You must be signed in to change notification settings - Fork 10.1k
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.
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 untap cmd bugs #16875
Fix untap cmd bugs #16875
Conversation
This wasn't working before for a few reasons. 1. It never got past the installed name check because the installed name sets had short names and the tap names were long names including the tap namespace too. Now we just trim the long name before comparing it to the installed name set. Before: ``` ["name"].include?("tap/full/name") # always false ``` After: ``` ["name"].include("tap/full/name".split("/").last) # sometimes true ``` 2. The names we were trying to load formulae and casks with were incorrect. Before: ``` tap = Tap.fetch("homebrew/cask-versions") token = "homebrew/cask-versions/token" cask = Cask::CaskLoader.load("#{tap}/#{token}") ``` After: ``` token = "homebrew/cask-versions/token" cask = CaskCaskLoader.load(token) ```
These are regression tests to make sure that this logic is reproducible. If this logic is not working, it might mean that someone removes a tap accidentally that still includes a formula or cask that they currently have installed. The tests are extravagant and over-engineered but I'm not sure that there's an easier way to do this without massive integration tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense to me, thanks @apainintheneck!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, please feel free to self-merge when you consider this ready!
I'm kind of surprised it's really that slow. Before:
After:
FYI, I'm using the following command to profile the tests. $ brew tests --only=untap --profile=10 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @apainintheneck. Fine to ship as-is for fixing user bugs but feels like this makes sense to live in another file 🔜 that I'd love in a follow-up PR.
require "extend/cachable" | ||
|
||
module Homebrew | ||
# Helpers for the `brew untap` command. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If these are only used by the untap
command: can they live in cmd/untap
instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that its not the best location but it does make the logic and testing simpler. It'll be easy to move back into the command once we're using the new command interface described in #16815.
brew style
with your changes locally?brew typecheck
with your changes locally?brew tests
with your changes locally?This bug was found when reviewing #16867.
The warning for installed formulae or casks in
brew untap
wasn't working before for a few reasons.installed name sets had short names and the tap names were
long names including the tap namespace too. Now we just trim the
long name before comparing it to the installed name set.
Before:
After:
were incorrect.
Before:
After:
I tried to keep the commits organized so they're probably best to review in order. The change itself is pretty simple (see the first commit) but adding tests is quite a feat. Let me know if there's some way to simplify the tests without turning them into a bunch of integration tests.
It is not that tough to test that things are working correctly now.
If you have any casks installed, the following line should fail for you.
Beyond that I installed the candy formula from a third-party tap and made sure that it gave me the appropriate warning when trying to untap the tap with that formula.