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

Jump to bottom

Fix installing from API with tap names #12340

Merged
merged 1 commit into from Oct 28, 2021
Merged

Fix installing from API with tap names #12340

merged 1 commit into from Oct 28, 2021

Conversation

Rylan12
Copy link
Member

@Rylan12 Rylan12 commented Oct 28, 2021

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same change?
  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes? Here's an example.
  • Have you successfully run brew style with your changes locally?
  • Have you successfully run brew typecheck with your changes locally?
  • Have you successfully run brew tests with your changes locally?

Follow-up to #12333 (also there's a chance that this fixes #12326)

This PR fixes brew install with HOMEBREW_INSTALL_FROM_API set so that it will work for brew install homebrew/core/foo and brew install homebrew/cask/foo.

I marked this as critical because it fixes a bug that was introduced in #12333.

@Rylan12 Rylan12 added the critical Critical change which should be shipped as soon as possible. label Oct 28, 2021
@BrewTestBot
Copy link
Member

Review period skipped due to critical label.

@Rylan12 Rylan12 mentioned this pull request Oct 28, 2021
Merged
7 tasks
Rylan12
Rylan12 commented Oct 28, 2021
View reviewed changes
Library/Homebrew/cmd/install.rb
next if File.exist?(name)
next if name !~ HOMEBREW_TAP_FORMULA_REGEX && name !~ HOMEBREW_CASK_TAP_CASK_REGEX
next unless name =~ HOMEBREW_TAP_FORMULA_REGEX
Copy link
Member Author

Choose a reason for hiding this comment

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

I don't really understand this check (mainly why there's a separate check for HOMEBREW_TAP_FORMULA_REGEX and HOMEBREW_CASK_TAP_CASK_REGEX.

As far as I can tell, there is no situation where name would not match HOMEBREW_TAP_FORMULA_REGEX but would match HOMEBREW_CASK_TAP_CASK_REGEX so I removed the second check. Let me know if there's a case I'm missing.

Copy link
Member

@carlocab carlocab Oct 28, 2021 •
edited

Choose a reason for hiding this comment

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

What if it does match HOMEBREW_TAP_FORMULA_REGEX but does not match HOMEBREW_CASK_TAP_CASK_REGEX? Is that possible? Or is this case not relevant? I hate conditionals.

Copy link
Member Author

Choose a reason for hiding this comment

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

If it matches HOMEBREW_TAP_FORMULA_REGEX, then the first part of the conditional (name !~ HOMEBREW_TAP_FORMULA_REGEX) is false so the entire conditional is false. So it doesn't matter if it matches HOMEBREW_CASK_TAP_CASK_REGEX or not.

Yeah, I had to go over this like 50 times to understand what it meant

Copy link
Member

Choose a reason for hiding this comment

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

This may have been for when these regexes were different. I'm fine with them being removed.

@Rylan12 Rylan12 mentioned this pull request Oct 28, 2021
Closed
MikeMcQuaid
MikeMcQuaid approved these changes Oct 28, 2021
View reviewed changes
Library/Homebrew/cmd/install.rb
next if File.exist?(name)
next if name !~ HOMEBREW_TAP_FORMULA_REGEX && name !~ HOMEBREW_CASK_TAP_CASK_REGEX
next unless name =~ HOMEBREW_TAP_FORMULA_REGEX
Copy link
Member

Choose a reason for hiding this comment

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

This may have been for when these regexes were different. I'm fine with them being removed.

@MikeMcQuaid MikeMcQuaid merged commit 241dd2c into Homebrew:master Oct 28, 2021
@MikeMcQuaid
Copy link
Member

Thanks again @Rylan12!

@Rylan12 Rylan12 deleted the install-from-api-tap-names branch October 28, 2021 17:09
@github-actions github-actions bot added the outdated PR was locked due to age label Nov 28, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 28, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@carlocab carlocab carlocab left review comments

@MikeMcQuaid MikeMcQuaid MikeMcQuaid approved these changes

@BrewTestBot BrewTestBot BrewTestBot approved these changes

Assignees
No one assigned
Labels
critical Critical change which should be shipped as soon as possible. outdated PR was locked due to age
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Apply HOMEBREW_INSTALL_FROM_API env to brew upgrade operations
4 participants
@Rylan12 @BrewTestBot @MikeMcQuaid @carlocab