Make `brew tap-info` show commit hash for taps that don't use the JSON API (_i.e._, are cloned locally)

benknoble commented 1 month ago

Verification

[X] 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.

Provide a detailed description of the proposed feature

Currently, brew tap-info <tap> provides a small amount of information about a tap; using --verbose does not provide more information, but using --json does.

In none of the above modes are tap versions shown, which can be helpful when troubleshooting an issue with a custom tap and formula that revolves around the version of both tap and formula.

The proposed feature is to add a version field to the JSON and text-based outputs of tap-info that indicates the Git commit hash of the currently checked-out tap, just as if git -C $(brew --repository)/Library/Taps/<tap> rev-parse HEAD were run.

What is the motivation for the feature?

As stated in the proposal, this provides useful debugging information:

it indicates whether or not a tap is up-to-date
it indicates the precise version of a tap being used to execute formula from that tap, which may reveal important differences

How will the feature be relevant to at least 90% of Homebrew users?

This feature can lead to quicker resolution of issues with formula from 3rd-party taps. For taps that use the JSON API, like homebrew/core, this will be less relevant (but brew tap-info homebrew/core reports "Not Installed" now, so I don't think that's a concern).

What alternatives to the feature have been considered?

I mentioned git -C $(brew --repository)/Library/Taps/<tap> rev-parse HEAD earlier: this is one way to request the relevant information. It adds an extra step for everyone involved in troubleshooting issues, however. Placing the information in brew tap-info makes the tap more self-describing and seems like a natural fit.

MikeMcQuaid commented 1 month ago

Thanks @benknoble, this loosely makes sense to me. Which would have been more useful to solve the problem you faced: the actual SHA of the commit, the date of the commit or both/neither?

benknoble commented 1 month ago

I think the SHA; I can get the date from that easily enough with access to the corresponding repo. (A way to include the result of git show -s <sha> would be nice but not necessary.)

MikeMcQuaid commented 1 month ago

I asked because in brew config we include:

ORIGIN: https://github.com/Homebrew/brew
HEAD: bd3c7f80530d21e791ccc01b02234b299941dd29
Last commit: 7 hours ago
Core tap HEAD: ae0af7c512095a35ffc933184d2c5108e6dfaaf1
Core tap last commit: 3 days ago
Core tap JSON: 20 Sep 11:55 UTC
Core cask tap HEAD: f5b6c9217e78b8f7b9e650bf1744027791b7052b
Core cask tap last commit: 3 days ago
Core cask tap JSON: 20 Sep 11:23 UTC

so the code is already there to do similar stuff so it may make sense for both consistency and given the existing helpers to do that

MikeMcQuaid commented 1 month ago

Will review PR for this. This code may be useful: https://github.com/Homebrew/brew/blob/bd3c7f80530d21e791ccc01b02234b299941dd29/Library/Homebrew/system_config.rb#L110-L133

Would suggest we do something consistently here both for JSON and Git taps.

benknoble commented 1 month ago

I asked because in brew config we include:
ORIGIN: https://github.com/Homebrew/brew
HEAD: bd3c7f80530d21e791ccc01b02234b299941dd29
Last commit: 7 hours ago
Core tap HEAD: ae0af7c512095a35ffc933184d2c5108e6dfaaf1
Core tap last commit: 3 days ago
Core tap JSON: 20 Sep 11:55 UTC
Core cask tap HEAD: f5b6c9217e78b8f7b9e650bf1744027791b7052b
Core cask tap last commit: 3 days ago
Core cask tap JSON: 20 Sep 11:23 UTC
so the code is already there to do similar stuff so it may make sense for both consistency and given the existing helpers to do that

Yep, if the date's already available easily I don't see any problems including it.

Will review PR for this. This code may be useful:

https://github.com/Homebrew/brew/blob/bd3c7f80530d21e791ccc01b02234b299941dd29/Library/Homebrew/system_config.rb#L110-L133

Would suggest we do something consistently here both for JSON and Git taps.

Great! I'll find some time, hopefully soon, to work on this.

benknoble commented 1 month ago

So far, this seems to do something like what I want for both brew tap-info --installed and brew tap-info --installed --json (the latter optionally with | jq 'map({name, remote, HEAD, last_commit, branch})'):

diff --git i/Library/Homebrew/cmd/tap-info.rb w/Library/Homebrew/cmd/tap-info.rb
index 2d00a70cb..bca548660 100644
--- i/Library/Homebrew/cmd/tap-info.rb
+++ w/Library/Homebrew/cmd/tap-info.rb
@@ -76,6 +76,10 @@ def print_tap_info(taps)
               info += "\nPrivate" if tap.private?
               info += "\n#{tap.path} (#{tap.path.abv})"
               info += "\nFrom: #{tap.remote.presence || "N/A"}"
+              info += "\norigin: #{tap.remote}" if tap.remote != tap.default_remote
+              info += "\nHEAD: #{tap.git_head || "(none)"}"
+              info += "\nlast commit: #{tap.git_last_commit || "never"}"
+              info += "\nbranch: #{tap.git_branch || "(none)"}" if tap.git_branch != "master"
             else
               info += "Not installed"
             end
diff --git i/Library/Homebrew/tap.rb w/Library/Homebrew/tap.rb
index dd78d0a97..4bf635058 100644
--- i/Library/Homebrew/tap.rb
+++ w/Library/Homebrew/tap.rb
@@ -881,6 +881,9 @@ def to_hash
       hash["remote"] = remote
       hash["custom_remote"] = custom_remote?
       hash["private"] = private?
+      hash["HEAD"] = git_head || "(none)"
+      hash["last_commit"] = git_last_commit || "never"
+      hash["branch"] = git_branch || "(none)"
     end

     hash

Thoughts? It would be nice to avoid the duplicated code spread across 3 files, of course, but I'm not Ruby expert :) I did attempt a version of StringIO.open {|s| SystemConfig.dump_tap_config(tap, s); s.string } for the tap-info command, but (naturally) it errors when given non-Core taps. So any common code should be called by SystemConfig, not the other way around.

MikeMcQuaid commented 1 month ago

@benknoble That makes sense to me. I think the duplication here is fine. Can you open a PR? Thanks!

Homebrew / brew