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.

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

Downcase internal commands #18705

Merged
merged 2 commits into from
Nov 4, 2024
Merged

Conversation

cocateh
Copy link
Contributor

@cocateh cocateh commented Nov 3, 2024

Since APFS is case-insensitive by default, Ruby will load the command file if user passes it mixed-case, but invoking it later will fail.

  • 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?

Since APFS is case-insensitive by default, Ruby will load the command file
if user passes it mixed-case, but invoking it later will fail.
@ZhongRuoyu
Copy link
Member

ZhongRuoyu commented Nov 3, 2024

Thanks for the PR @cocateh, but can you please elaborate what your intended use case is? Is it something like brew INSTALL? Why not invoke it with the correct letter case?

This does matter in case-sensitive filesystems where you can technically have brew INSTALL as an external command. I don't think we make assumptions on the filesystem like this.

@cocateh
Copy link
Contributor Author

cocateh commented Nov 3, 2024

Hi!

That's not the point of the PR, if you have a mixed-case command then brew crashes saying that it can't find an internal command.

This is because of what I described, because APFS is by default case-insensitive, if you pass something like upgrAde, then valid_internal_cmd? passes because it checks whether there is such command in brew's load path by doing require.

Because of the case-insensitivity, this returns true, so it loaded the command file, however, brew won't be able to find a proper handler in AbstractCommand nor do Command.method_name (it crashes on this one), because cmd is still mixed-case.

Since all of brew's internal commands are lower-case, I don't see a reason why shouldn't it be downcased by default for the sole convenience of a user :-)

@cocateh
Copy link
Contributor Author

cocateh commented Nov 3, 2024

This is a minor issue, though, I just saw my Homebrew crash when I miss-spelled my command so that was a quick fix I think is sensible.

@cocateh
Copy link
Contributor Author

cocateh commented Nov 3, 2024

This does matter in case-sensitive filesystems where you can technically have brew INSTALL as an external command. I don't think we make assumptions on the filesystem like this.

This change applies only to internal commands. It wouldn't make much sense to make two internal commands, both with different capitalisation.

@MikeMcQuaid
Copy link
Member

@cocateh Thanks for the PR! I like the idea but not the implementation. I've adjusted it to improve the error messaging instead.

@MikeMcQuaid MikeMcQuaid merged commit 614678f into Homebrew:master Nov 4, 2024
27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants