brew edit --print-path --formula src prints src not the full path

[gibfahn]

brew doctor output

❯ brew doctor
Please note that these warnings are just used to help the Homebrew maintainers
with debugging if you file an issue. If everything you use Homebrew for is
working fine: please don't worry or file an issue; just ignore this. Thanks!

Warning: The filesystem on /System/Volumes/Data appears to be case-sensitive.
The default macOS filesystem is case-insensitive. Please report any apparent problems.

Verification

• My "brew doctor output" above says Your system is ready to brew. and am still able to reproduce my issue.
• I ran brew update twice and am still able to reproduce my issue.
• This issue's title and/or description do not reference a single formula e.g. brew install wget. If they do, open an issue at https://github.com/Homebrew/homebrew-core/issues/new/choose instead.

brew config output

❯ brew config
HOMEBREW_VERSION: 4.1.11
ORIGIN: https://github.com/Homebrew/brew
HEAD: 1f80e82dd72fce6642e4ea360f7a1feef5d78470
Last commit: 3 hours ago
Core tap JSON: 15 Sep 19:55 UTC
HOMEBREW_PREFIX: /opt/homebrew
HOMEBREW_CASK_OPTS: []
HOMEBREW_MAKE_JOBS: 8
Homebrew Ruby: 2.6.10 => /System/Library/Frameworks/Ruby.framework/Versions/2.6/usr/bin/ruby
CPU: octa-core 64-bit arm_firestorm_icestorm
Clang: 15.0.0 build 1500
Git: 2.39.3 => /Library/Developer/CommandLineTools/usr/bin/git
Curl: 8.1.2 => /usr/bin/curl
macOS: 14.0-arm64
CLT: 15.0.0.0.1.1692336968
Xcode: N/A
Rosetta 2: false

What were you trying to do (and why)?

Get the path to the src core formula

What happened (include all command output)?

❯ brew edit --print-path --formula src
src

What did you expect to happen?

Same as if you fully specify the formula:

❯ brew edit -dv --print-path --formula homebrew/core/src
/opt/homebrew/Library/Taps/homebrew/homebrew-core/Formula/s/src.rb

Step-by-step reproduction instructions (by running brew commands)

brew edit --print-path --formula src

[MikeMcQuaid]

Can't reproduce with or without homebrew/core tapped. Do you have a src directory or file in your current working directory?

[gibfahn]

Oh wow, yes that's totally it, I have a src/ directory, and it works when I cd somewhere else. I would never have thought to check $PWD. Is this intended behaviour?

[Bo98]

It's an unintentional consequence of sharing code with brew style's intentional behaviour of allowing to take in any arbitrary path as an argument, rather than necessarily a valid package or tap name.

[apainintheneck]

This has come up before (#14895) and personally I think this behavior is confusing and unexpected. @MikeMcQuaid mentions in that issue that this is an intentional part of some users workflows though it seems like there are better options for opening files in the current directory. I think this is particularly confusing when the --formula or --cask options are passed to brew edit since that implies that relative files/directories should not be returned.

Other than deprecating this behavior entirely, we could always just print expanded paths whenever --print-path is set and at least then it would be clearer to the user what is happening.

[MikeMcQuaid]

Other than deprecating this behavior entirely, we could always just print expanded paths whenever --print-path is set and at least then it would be clearer to the user what is happening.

This seems like a good idea 👍🏻

[Bo98]

this is an intentional part

The behaviour was added to brew edit by accident when Cask support was added - the documentation still doesn't say passing arbitrary paths is supported here (brew style has an explicit named_args :file). What it's got going for it is just the length of time it's been like that (3 years). I don't really see the use case of it given it's longer than just doing code <path> or whatever.

I think this is particularly confusing when the --formula or --cask options are passed to brew edit since that implies that relative files/directories should not be returned

I agree with this - the logic should probably at least be skipped if only is set. --formula or --cask to me sounds like you are explicitly wanting to pass a formula or cask name.

[gibfahn]

Perhaps also this could default to a formula if one exists, so preferring src.rb over ./src/. I guess maybe that's a breaking change, but especially when --print-path is set, I struggle to see the use-case for wanting to do brew edit --print-path src to get src/, so 🤞 no-one started relying on that undocumented behaviour.

[abitrolly]

To add --formula parameter fix to edit.rb, I tried to simplify the flow in #16029, but run into incompatible rubocop requirements.

[apainintheneck]

I think we should deprecate the behavior of reading relative files/directories in brew edit.

This would mean adding a deprecation warning for users if local files exist that shadow formula/tap names.

It would also mean modifying Homebrew::CLI::NamedArgs#to_paths to add an option to skip local/relatives files/directories. This would allow us to keep the current behavior for brew style while updating brew edit to the expected behavior.

This might break some users' workflows. I believe that that is not a big deal though because there are other options when it comes to opening relative files and the brew edit command is not destructive so changing things should be low risk.

[apainintheneck]

@MikeMcQuaid You've mentioned multiple times that we should keep the current behavior because users will complain otherwise. Can you give use some examples of the users this change will negatively affect and why other approaches wouldn't work for them?

[MikeMcQuaid]

I think we should deprecate the behavior of reading relative files/directories in brew edit.
@MikeMcQuaid You've mentioned multiple times that we should keep the current behavior because users will complain otherwise. Can you give use some examples of the users this change will negatively affect and why other approaches wouldn't work for them?

I disagree. People use this and it's easier to resolve this confusion with messaging (i.e. always output a full, resolved path when editing or with errors) and then it allows them to continue to use these workflows.

An example workflow I've seen a few people do:

• git clone a tap outside of Library/Taps
• cd into that tap and brew edit Formula/something.rb

This neither seems harmful or undesirable, other than confusion like in this issue.

This would allow us to keep the current behavior for brew style while updating brew edit to the expected behavior.

I think it's also desirable that brew style Formula/something.rb || brew edit Formula/something.rb works as expected.

[Bo98]

I still don't think brew edit --cask Formula/something.rb working is desirable behaviour and path arguments should only be interpreted as such without --formula/--cask.

An example workflow I've seen a few people do

If this is intentional: can it be documented? It currently is not documented as supported for brew edit.

[MikeMcQuaid]

I still don't think brew edit --cask Formula/something.rb working is desirable behaviour and path arguments should only be interpreted as such without --formula/--cask.

I agree that that should fail. Whether brew edit --formula Formula/something.rb should work: I don't really care and could be convinced either way but mostly would be convinced by "the implementation is simpler this way".

[apainintheneck]

I disagree. People use this and it's easier to resolve this confusion with messaging (i.e. always output a full, resolved path when editing or with errors) and then it allows them to continue to use these workflows.

An example workflow I've seen a few people do:

• git clone a tap outside of Library/Taps
• cd into that tap and brew edit Formula/something.rb

This neither seems harmful or undesirable, other than confusion like in this issue.

It's harmful because this undocumented behavior has precedence over all of the documented and therefore expected behaviors of brew edit. Beyond that if someone is specifying the full path to a file, using brew edit <path> is not any more convenient than $EDITOR <path>. brew style <path> in contrast doesn't have any clear alternatives and this behavior is documented for that command.

The only argument for keeping the current behavior as I see it is momentum. It shouldn't be a big deal to remove support for undocumented behavior with clear alternatives; we do that regularly already.

[abitrolly]

After making https://github.com/Homebrew/brew/pull/16030/files I think there is also a structural problem - cli/named_args.rb:to_paths decides the user flow for brew edit and brew style. But it really should just expand the paths and let those commands handle the processing logic.

Maybe the path should be more complicated object after this function.

   path = "meh"
   path.formula = "Formula/meh.rb"
   path.cask = nil
   path.dir = "meh"

[MikeMcQuaid]

The only argument for keeping the current behavior as I see it is momentum.

For a package manager with millions of users and thousands of users: the "momentum" argument is non-trivial.

It shouldn't be a big deal to remove support for undocumented behavior with clear alternatives; we do that regularly already.

If we were talking about removing it from style and edit: I'd see the logic. Ultimately, though: this needs handled for both and edit is the only place there's confusion so we should handle it there.

Maybe the path should be more complicated object after this function.

I don't think the internals are the problem here.

[Bo98]

edit is the only place there's confusion so we should handle it there.

There is actually a wider question to be had here. This is how the following currently behave:

$ cd ~

$ brew formula hello
/usr/local/Homebrew/Library/Taps/homebrew/homebrew-core/Formula/h/hello.rb
$ brew formula Library
Library
$ brew formula /etc
/etc

$ brew log hello
(git log output)
$ brew log Library
[if not in git directory]
fatal: not a git repository (or any of the parent directories): .git
[if in git directory - and one that's not even a shallow clone like the warning suggests]
Warning: . is a shallow clone so only partial output will be shown.
To get a full clone, run:
  git -C "" fetch --unshallow
(git log output)

$ brew audit --installed Library
(runs `brew style Library`, not installed formulae, and then audits over the installed formulae, not Library)

The follow up question is how should each of the the previous behave?

[Bo98]

I still don't think brew edit --cask Formula/something.rb working is desirable behaviour and path arguments should only be interpreted as such without --formula/--cask.

I agree that that should fail. Whether brew edit --formula Formula/something.rb should work: I don't really care and could be convinced either way but mostly would be convinced by "the implementation is simpler this way".

The easiest way to deal with it is:

diff --git a/Library/Homebrew/cli/named_args.rb b/Library/Homebrew/cli/named_args.rb
index 98ab981c1..f0bf6ab74 100644
--- a/Library/Homebrew/cli/named_args.rb
+++ b/Library/Homebrew/cli/named_args.rb
@@ -230,7 +230,7 @@ module Homebrew
         @to_paths[only] ||= Homebrew.with_no_api_env_if_needed(@without_api) do
           downcased_unique_named.flat_map do |name|
             path = Pathname(name)
-            if File.exist?(name)
+            if only.nil? && File.exist?(name)
               path
             elsif name.count("/") == 1 && !name.start_with?("./", "/")
               tap = Tap.fetch(name)

This in turn fixes brew formula etc too.

---

There's perhaps also an priority argument to be had too. If you run brew style src, what should take priority - a src directory or a src formula?

[MikeMcQuaid]

The follow up question is how should each of the the previous behave?

Great question. Here's what I think the ideal behaviour/output would be:

$ cd ~

$ brew formula hello
/usr/local/Homebrew/Library/Taps/homebrew/homebrew-core/Formula/h/hello.rb

$ brew formula Library
/Users/mike/Library
$ brew formula /etc
/etc

# alternatively:

$ brew formula Library
Error: /Users/mike/Library is not a formula!
$ brew formula /etc
Error: /etc is not a formula!

$ brew log hello
Git log for /usr/local/Homebrew/Library/Taps/homebrew/homebrew-core/Formula/h/hello.rb:
(git log output)
$ brew log Library
[if not in git directory]
Error: no /Users/mike/Library/.git directory found!

$ brew log Library
[if in git directory - and one that's not even a shallow clone like the warning suggests]
Git log for /Users/mike/Library:
Warning: . is a shallow clone so only partial output will be shown.
To get a full clone, run:
  git -C "" fetch --unshallow
(git log output)

$ brew audit --installed Library
Error: no formulae or casks found in /Users/mike/Library!

The easiest way to deal with it is:

This makes sense to me 👍🏻

There's perhaps also an priority argument to be had too. If you run brew style src, what should take priority - a src directory or a src formula?

I think the src directory but the output should be clear that that's what it's looked at.

[apainintheneck]

It looks like we're already in agreement about the expanding paths in brew edit --print-path so that should be an easy thing to add. I'll make a PR for that.

The current eval order:

1. local file/dir
2. tap
   • This only works with tap/name and not tap name syntax.
3. formula/cask
   • This includes local files because of the path loaders so it includes ./formula.rb.
4. local file/dir
   • Included even if the file/directory doesn't exist as a catch-all.

I still think it's more intuitive to make local files/directories lower priority for this specific command but we should update the docs to mention it either way. The brew style man page includes this paragraph which we could use as a guide.

Check formulae or files for conformance to Homebrew style guidelines.

Lists of file, tap and formula may not be combined. If none are provided,
style will run style checks on the whole Homebrew library, including core code
and all formulae.

In comparison, this is what we have for brew edit right now.

Open a formula, cask or tap in the editor set by EDITOR or
HOMEBREW_EDITOR, or open the Homebrew repository for editing if no argument is
provided.

Edit: By keeping local files/directories as the highest precedence option we're essentially signaling that this is the most common expected use case of this command.

[apainintheneck]

I'd like to discuss other commands too if possible but maybe that'd be worth making another issue for since it's not directly related to this one. Just to make it easier to follow.

Edit: I'm certainly surprised that the brew formula and brew log commands work like that currently.

[MikeMcQuaid]

By keeping local files/directories as the highest precedence option we're essentially signaling that this is the most common expected use case of this command.

I don't agree with this take. It's almost certainly too much work but I think brew edit src should be lower precedence than brew edit ./src or brew edit src/foo or brew edit src.rb or brew edit src/ i.e. if it looks a lot more like a path: take that as the user requesting a path and if it's ambiguous: I don't mind as much how we resolve it.

As I keep saying, though: I think the main issue here is just communicating through the command output what is being edited.

I'd like to discuss other commands too if possible but maybe that'd be worth making another issue for since it's not directly related to this one. Just to make it easier to follow.

Yeh, happy to discuss that in another issue 👍🏻. I think ideally there'd be as much consistency between commands as possible.

[apainintheneck]

The ambiguous ones are brew edit src and brew edit user/repo/src for referencing formulae and brew edit user/repo for referencing taps.

It should be easy enough to add handling for the other cases though. Anything that starts or ends with a slash or includes a period should be treated as a file path by default. Strings with periods and strings that start with slashes are already unambiguous. Interesting that as currently written brew edit src/ is ambiguous unless it is a url that matches one of the formula/cask loaders. If there isn't a local directory that matches, we treat it as a tap which doesn't really make sense.

diff --git a/Library/Homebrew/cli/named_args.rb b/Library/Homebrew/cli/named_args.rb
index bfa013001..0fd56ae5d 100644
--- a/Library/Homebrew/cli/named_args.rb
+++ b/Library/Homebrew/cli/named_args.rb
@@ -230,9 +230,7 @@ module Homebrew
         @to_paths[only] ||= Homebrew.with_no_api_env_if_needed(@without_api) do
           downcased_unique_named.flat_map do |name|
             path = Pathname(name).expand_path
-            if only.nil? && File.exist?(name)
-              path
-            elsif name.count("/") == 1 && !name.start_with?("./", "/")
+            if name.match?(%r{^[^./]+/[^./]+$})
               tap = Tap.fetch(name)
 
               if recurse_tap

Something like this would work I think.

[MikeMcQuaid]

Something like this would work I think.

Yup, that seems like a good idea.