From 429f23dcc605776a2e2660f3516b8d465e6dc0d7 Mon Sep 17 00:00:00 2001 From: Douglas Eichelberger Date: Sat, 15 Apr 2023 15:06:58 -0700 Subject: [PATCH 1/7] Create GitRepoPath --- Library/Homebrew/dev-cmd/extract.rb | 2 +- Library/Homebrew/diagnostic.rb | 1 + .../{git_repository.rb => git_repo_path.rb} | 12 +++++++----- Library/Homebrew/extend/git_repo_path.rbi | 17 +++++++++++++++++ Library/Homebrew/global.rb | 2 +- Library/Homebrew/system_config.rb | 13 +++++++------ Library/Homebrew/tap.rb | 18 ++++++++++-------- Library/Homebrew/test/dev-cmd/pr-pull_spec.rb | 2 +- Library/Homebrew/utils/git.rb | 2 +- Library/Homebrew/utils/git_repository.rb | 8 ++++---- 10 files changed, 50 insertions(+), 27 deletions(-) rename Library/Homebrew/extend/{git_repository.rb => git_repo_path.rb} (93%) create mode 100644 Library/Homebrew/extend/git_repo_path.rbi diff --git a/Library/Homebrew/dev-cmd/extract.rb b/Library/Homebrew/dev-cmd/extract.rb index ac002199cc..96542a3cdb 100644 --- a/Library/Homebrew/dev-cmd/extract.rb +++ b/Library/Homebrew/dev-cmd/extract.rb @@ -113,7 +113,7 @@ module Homebrew end destination_tap.install unless destination_tap.installed? - repo = source_tap.path + repo = source_tap.git_repo.pathname pattern = if source_tap.core_tap? [repo/"Formula/#{name}.rb"] else diff --git a/Library/Homebrew/diagnostic.rb b/Library/Homebrew/diagnostic.rb index 9b88e6ef7b..79f28088a3 100644 --- a/Library/Homebrew/diagnostic.rb +++ b/Library/Homebrew/diagnostic.rb @@ -126,6 +126,7 @@ module Homebrew EOS end + sig { params(repository_path: GitRepoPath, desired_origin: String).returns(T.nilable(String)) } def examine_git_origin(repository_path, desired_origin) return if !Utils::Git.available? || !repository_path.git? diff --git a/Library/Homebrew/extend/git_repository.rb b/Library/Homebrew/extend/git_repo_path.rb similarity index 93% rename from Library/Homebrew/extend/git_repository.rb rename to Library/Homebrew/extend/git_repo_path.rb index 6e21970e31..a02b108542 100644 --- a/Library/Homebrew/extend/git_repository.rb +++ b/Library/Homebrew/extend/git_repo_path.rb @@ -7,12 +7,14 @@ require "utils/popen" # Extensions to {Pathname} for querying Git repository information. # @see Utils::Git # @api private -module GitRepositoryExtension +class GitRepoPath < SimpleDelegator extend T::Sig + alias pathname __getobj__ + sig { returns(T::Boolean) } def git? - join(".git").exist? + pathname.join(".git").exist? end # Gets the URL of the Git origin remote. @@ -26,7 +28,7 @@ module GitRepositoryExtension def git_origin=(origin) return if !git? || !Utils::Git.available? - safe_system Utils::Git.git, "remote", "set-url", "origin", origin, chdir: self + safe_system Utils::Git.git, "remote", "set-url", "origin", origin, chdir: pathname end # Gets the full commit hash of the HEAD commit. @@ -108,7 +110,7 @@ module GitRepositoryExtension unless git? return unless safe - raise "Not a Git repository: #{self}" + raise "Not a Git repository: #{pathname}" end unless Utils::Git.available? @@ -117,6 +119,6 @@ module GitRepositoryExtension raise "Git is unavailable" end - Utils.popen_read(Utils::Git.git, *args, safe: safe, chdir: self, err: err).chomp.presence + Utils.popen_read(Utils::Git.git, *args, safe: safe, chdir: pathname, err: err).chomp.presence end end diff --git a/Library/Homebrew/extend/git_repo_path.rbi b/Library/Homebrew/extend/git_repo_path.rbi new file mode 100644 index 0000000000..b1fb6031dd --- /dev/null +++ b/Library/Homebrew/extend/git_repo_path.rbi @@ -0,0 +1,17 @@ +# typed: strict + +class GitRepoPath < SimpleDelegator + include Kernel + + # This is a workaround to enable `alias pathname __getobj__` + # @see https://github.com/sorbet/sorbet/issues/2378#issuecomment-569474238 + sig { returns(Pathname) } + def __getobj__; end + + def /(_arg0); end + def parent; end + def abv; end + def rmtree; end + def cd; end + def directory?; end +end diff --git a/Library/Homebrew/global.rb b/Library/Homebrew/global.rb index 3c3cecdb2d..7e451d952a 100644 --- a/Library/Homebrew/global.rb +++ b/Library/Homebrew/global.rb @@ -131,7 +131,7 @@ end require "context" require "extend/array" -require "extend/git_repository" +require "extend/git_repo_path" require "extend/pathname" require "extend/predicable" require "extend/module" diff --git a/Library/Homebrew/system_config.rb b/Library/Homebrew/system_config.rb index 349b1e1288..7b4c1f4018 100644 --- a/Library/Homebrew/system_config.rb +++ b/Library/Homebrew/system_config.rb @@ -1,4 +1,4 @@ -# typed: false +# typed: true # frozen_string_literal: true require "hardware" @@ -32,9 +32,9 @@ module SystemConfig end end - sig { returns(Pathname) } + sig { returns(GitRepoPath) } def homebrew_repo - HOMEBREW_REPOSITORY.dup.extend(GitRepositoryExtension) + GitRepoPath.new(HOMEBREW_REPOSITORY) end sig { returns(String) } @@ -69,7 +69,7 @@ module SystemConfig sig { returns(String) } def core_tap_origin - CoreTap.instance.remote || "(none)" + CoreTap.instance.remote end sig { returns(String) } @@ -132,8 +132,9 @@ module SystemConfig def describe_curl out, = system_command(curl_executable, args: ["--version"], verbose: false) - if /^curl (?[\d.]+)/ =~ out - "#{curl_version} => #{curl_path}" + match_data = /^curl (?[\d.]+)/.match(out) + if match_data + "#{match_data[:curl_version]} => #{curl_path}" else "N/A" end diff --git a/Library/Homebrew/tap.rb b/Library/Homebrew/tap.rb index f263af6053..45aa7a618f 100644 --- a/Library/Homebrew/tap.rb +++ b/Library/Homebrew/tap.rb @@ -93,16 +93,18 @@ class Tap # The local path to this {Tap}. # e.g. `/usr/local/Library/Taps/user/homebrew-repo` + sig { returns(GitRepoPath) } attr_reader :path + alias git_repo path + # @private def initialize(user, repo) @user = user @repo = repo @name = "#{@user}/#{@repo}".downcase @full_name = "#{@user}/homebrew-#{@repo}" - @path = TAP_DIRECTORY/@full_name.downcase - @path.extend(GitRepositoryExtension) + @path = GitRepoPath.new(TAP_DIRECTORY/@full_name.downcase) @alias_table = nil @alias_reverse_table = nil end @@ -315,7 +317,7 @@ class Tap ignore_interrupts do # wait for git to possibly cleanup the top directory when interrupt happens. sleep 0.1 - FileUtils.rm_rf path + FileUtils.rm_rf git_repo.pathname path.parent.rmdir_if_possible end raise @@ -386,7 +388,7 @@ class Tap $stderr.ohai "#{name}: changed remote from #{remote} to #{requested_remote}" unless quiet end - current_upstream_head = path.git_origin_branch + current_upstream_head = T.must(path.git_origin_branch) return if requested_remote.blank? && path.git_origin_has_branch?(current_upstream_head) args = %w[fetch] @@ -395,7 +397,7 @@ class Tap safe_system "git", "-C", path, *args path.git_origin_set_head_auto - new_upstream_head = path.git_origin_branch + new_upstream_head = T.must(path.git_origin_branch) return if new_upstream_head == current_upstream_head path.git_rename_branch old: current_upstream_head, new: new_upstream_head @@ -462,7 +464,7 @@ class Tap sig { returns(T::Array[Pathname]) } def potential_formula_dirs - @potential_formula_dirs ||= [path/"Formula", path/"HomebrewFormula", path].freeze + @potential_formula_dirs ||= [path/"Formula", path/"HomebrewFormula", git_repo.pathname].freeze end # Path to the directory of all {Cask} files for this {Tap}. @@ -567,7 +569,7 @@ class Tap sig { params(file: T.any(String, Pathname)).returns(T::Boolean) } def formula_file?(file) file = Pathname.new(file) unless file.is_a? Pathname - file = file.expand_path(path) + file = file.expand_path(git_repo.pathname) return false unless ruby_file?(file) return false if cask_file?(file) @@ -580,7 +582,7 @@ class Tap sig { params(file: T.any(String, Pathname)).returns(T::Boolean) } def cask_file?(file) file = Pathname.new(file) unless file.is_a? Pathname - file = file.expand_path(path) + file = file.expand_path(git_repo.pathname) return false unless ruby_file?(file) file.to_s.start_with?("#{cask_dir}/") diff --git a/Library/Homebrew/test/dev-cmd/pr-pull_spec.rb b/Library/Homebrew/test/dev-cmd/pr-pull_spec.rb index 9366befe34..058fa99b22 100644 --- a/Library/Homebrew/test/dev-cmd/pr-pull_spec.rb +++ b/Library/Homebrew/test/dev-cmd/pr-pull_spec.rb @@ -81,7 +81,7 @@ describe "brew pr-pull" do let(:tap) { Tap.fetch("Homebrew", "foo") } let(:formula_file) { tap.path/"Formula/foo.rb" } let(:cask_file) { tap.cask_dir/"food.rb" } - let(:path) { (Tap::TAP_DIRECTORY/"homebrew/homebrew-foo").extend(GitRepositoryExtension) } + let(:path) { GitRepoPath.new(Tap::TAP_DIRECTORY/"homebrew/homebrew-foo") } describe "#autosquash!" do it "squashes a formula or cask correctly" do diff --git a/Library/Homebrew/utils/git.rb b/Library/Homebrew/utils/git.rb index 54007e24b8..8b88654a12 100644 --- a/Library/Homebrew/utils/git.rb +++ b/Library/Homebrew/utils/git.rb @@ -4,7 +4,7 @@ module Utils # Helper functions for querying Git information. # - # @see GitRepositoryExtension + # @see GitRepoPath # @api private module Git extend T::Sig diff --git a/Library/Homebrew/utils/git_repository.rb b/Library/Homebrew/utils/git_repository.rb index 6c0e983a89..5ab0275ff1 100644 --- a/Library/Homebrew/utils/git_repository.rb +++ b/Library/Homebrew/utils/git_repository.rb @@ -15,7 +15,7 @@ module Utils def self.git_head(repo = Pathname.pwd, length: nil, safe: true) return git_short_head(repo, length: length) if length.present? - repo = Pathname(repo).extend(GitRepositoryExtension) + repo = GitRepoPath.new(Pathname(repo)) repo.git_head(safe: safe) end @@ -28,7 +28,7 @@ module Utils ).returns(T.nilable(String)) } def self.git_short_head(repo = Pathname.pwd, length: nil, safe: true) - repo = Pathname(repo).extend(GitRepositoryExtension) + repo = GitRepoPath.new(Pathname(repo)) repo.git_short_head(length: length, safe: safe) end @@ -40,7 +40,7 @@ module Utils ).returns(T.nilable(String)) } def self.git_branch(repo = Pathname.pwd, safe: true) - repo = Pathname(repo).extend(GitRepositoryExtension) + repo = GitRepoPath.new(Pathname(repo)) repo.git_branch(safe: safe) end @@ -53,7 +53,7 @@ module Utils ).returns(T.nilable(String)) } def self.git_commit_message(repo = Pathname.pwd, commit: "HEAD", safe: true) - repo = Pathname(repo).extend(GitRepositoryExtension) + repo = GitRepoPath.new(Pathname(repo)) repo.git_commit_message(commit, safe: safe) end end From 3d1232eeace4ce4230fa47613739f53e3520bd10 Mon Sep 17 00:00:00 2001 From: Douglas Eichelberger Date: Sat, 15 Apr 2023 16:29:33 -0700 Subject: [PATCH 2/7] rename methods --- Library/Homebrew/extend/git_repo_path.rb | 36 ++++++++++++------------ 1 file changed, 18 insertions(+), 18 deletions(-) diff --git a/Library/Homebrew/extend/git_repo_path.rb b/Library/Homebrew/extend/git_repo_path.rb index a02b108542..23eaf913eb 100644 --- a/Library/Homebrew/extend/git_repo_path.rb +++ b/Library/Homebrew/extend/git_repo_path.rb @@ -13,93 +13,93 @@ class GitRepoPath < SimpleDelegator alias pathname __getobj__ sig { returns(T::Boolean) } - def git? + def git_repo? pathname.join(".git").exist? end # Gets the URL of the Git origin remote. sig { returns(T.nilable(String)) } - def git_origin + def origin_url popen_git("config", "--get", "remote.origin.url") end # Sets the URL of the Git origin remote. sig { params(origin: String).returns(T.nilable(T::Boolean)) } - def git_origin=(origin) - return if !git? || !Utils::Git.available? + def origin_url=(origin) + return if !git_repo? || !Utils::Git.available? safe_system Utils::Git.git, "remote", "set-url", "origin", origin, chdir: pathname end # Gets the full commit hash of the HEAD commit. sig { params(safe: T::Boolean).returns(T.nilable(String)) } - def git_head(safe: false) + def head_ref(safe: false) popen_git("rev-parse", "--verify", "--quiet", "HEAD", safe: safe) end # Gets a short commit hash of the HEAD commit. sig { params(length: T.nilable(Integer), safe: T::Boolean).returns(T.nilable(String)) } - def git_short_head(length: nil, safe: false) + def short_head_ref(length: nil, safe: false) short_arg = length.present? ? "--short=#{length}" : "--short" popen_git("rev-parse", short_arg, "--verify", "--quiet", "HEAD", safe: safe) end # Gets the relative date of the last commit, e.g. "1 hour ago" sig { returns(T.nilable(String)) } - def git_last_commit + def last_committed popen_git("show", "-s", "--format=%cr", "HEAD") end # Gets the name of the currently checked-out branch, or HEAD if the repository is in a detached HEAD state. sig { params(safe: T::Boolean).returns(T.nilable(String)) } - def git_branch(safe: false) + def branch_name(safe: false) popen_git("rev-parse", "--abbrev-ref", "HEAD", safe: safe) end # Change the name of a local branch sig { params(old: String, new: String).void } - def git_rename_branch(old:, new:) + def rename_branch(old:, new:) popen_git("branch", "-m", old, new) end # Set an upstream branch for a local branch to track sig { params(local: String, origin: String).void } - def git_branch_set_upstream(local:, origin:) + def set_upstream_branch(local:, origin:) popen_git("branch", "-u", "origin/#{origin}", local) end # Gets the name of the default origin HEAD branch. sig { returns(T.nilable(String)) } - def git_origin_branch + def origin_branch_name popen_git("symbolic-ref", "-q", "--short", "refs/remotes/origin/HEAD")&.split("/")&.last end # Returns true if the repository's current branch matches the default origin branch. sig { returns(T.nilable(T::Boolean)) } - def git_default_origin_branch? - git_origin_branch == git_branch + def default_origin_branch? + origin_branch_name == branch_name end # Returns the date of the last commit, in YYYY-MM-DD format. sig { returns(T.nilable(String)) } - def git_last_commit_date + def last_commit_date popen_git("show", "-s", "--format=%cd", "--date=short", "HEAD") end # Returns true if the given branch exists on origin sig { params(branch: String).returns(T::Boolean) } - def git_origin_has_branch?(branch) + def origin_has_branch?(branch) popen_git("ls-remote", "--heads", "origin", branch).present? end sig { void } - def git_origin_set_head_auto + def set_head_origin_auto popen_git("remote", "set-head", "origin", "--auto") end # Gets the full commit message of the specified commit, or of the HEAD commit if unspecified. sig { params(commit: String, safe: T::Boolean).returns(T.nilable(String)) } - def git_commit_message(commit = "HEAD", safe: false) + def commit_message(commit = "HEAD", safe: false) popen_git("log", "-1", "--pretty=%B", commit, "--", safe: safe, err: :out)&.strip end @@ -107,7 +107,7 @@ class GitRepoPath < SimpleDelegator sig { params(args: T.untyped, safe: T::Boolean, err: T.nilable(Symbol)).returns(T.nilable(String)) } def popen_git(*args, safe: false, err: nil) - unless git? + unless git_repo? return unless safe raise "Not a Git repository: #{pathname}" From 8307255ce863a7decde4b92c1e679ce375952ae0 Mon Sep 17 00:00:00 2001 From: Douglas Eichelberger Date: Sat, 15 Apr 2023 16:46:13 -0700 Subject: [PATCH 3/7] Update call sites --- Library/Homebrew/dev-cmd/pr-pull.rb | 14 ++++++------ Library/Homebrew/diagnostic.rb | 4 ++-- Library/Homebrew/system_config.rb | 6 ++--- Library/Homebrew/tap.rb | 22 +++++++++---------- Library/Homebrew/test/dev-cmd/pr-pull_spec.rb | 12 +++++----- Library/Homebrew/utils/git_repository.rb | 8 +++---- 6 files changed, 33 insertions(+), 33 deletions(-) diff --git a/Library/Homebrew/dev-cmd/pr-pull.rb b/Library/Homebrew/dev-cmd/pr-pull.rb index f37f993463..8b03ddbc55 100644 --- a/Library/Homebrew/dev-cmd/pr-pull.rb +++ b/Library/Homebrew/dev-cmd/pr-pull.rb @@ -86,7 +86,7 @@ module Homebrew end def self.signoff!(path, pull_request: nil, dry_run: false) - subject, body, trailers = separate_commit_message(path.git_commit_message) + subject, body, trailers = separate_commit_message(path.commit_message) if pull_request # This is a tap pull request and approving reviewers should also sign-off. @@ -167,7 +167,7 @@ module Homebrew new_package = Utils::Git.file_at_commit(path, file, "HEAD") bump_subject = determine_bump_subject(old_package, new_package, package_file, reason: reason).strip - subject, body, trailers = separate_commit_message(path.git_commit_message) + subject, body, trailers = separate_commit_message(path.commit_message) if subject != bump_subject && !subject.start_with?("#{package_name}:") safe_system("git", "-C", path, "commit", "--amend", "-q", @@ -192,7 +192,7 @@ module Homebrew messages = [] trailers = [] commits.each do |commit| - subject, body, trailer = separate_commit_message(path.git_commit_message(commit)) + subject, body, trailer = separate_commit_message(path.commit_message(commit)) body = body.lines.map { |line| " #{line.strip}" }.join("\n") messages << "* #{subject}\n#{body}".strip trailers << trailer @@ -228,7 +228,7 @@ module Homebrew # TODO: fix test in `test/dev-cmd/pr-pull_spec.rb` and assume `cherry_picked: false`. def self.autosquash!(original_commit, tap:, reason: "", verbose: false, resolve: false, cherry_picked: true) - original_head = tap.path.git_head + original_head = tap.path.head_ref commits = Utils.safe_popen_read("git", "-C", tap.path, "rev-list", "--reverse", "#{original_commit}..HEAD").lines.map(&:strip) @@ -450,8 +450,8 @@ module Homebrew _, user, repo, pr = *url_match odie "Not a GitHub pull request: #{arg}" unless pr - if !tap.path.git_default_origin_branch? || args.branch_okay? || args.clean? - opoo "Current branch is #{tap.path.git_branch}: do you need to pull inside #{tap.path.git_origin_branch}?" + if !tap.path.default_origin_branch? || args.branch_okay? || args.clean? + opoo "Current branch is #{tap.path.branch_name}: do you need to pull inside #{tap.path.origin_branch_name}?" end pr_labels = GitHub.pull_request_labels(user, repo, pr) @@ -464,7 +464,7 @@ module Homebrew ohai "Fetching #{tap} pull request ##{pr}" Dir.mktmpdir pr do |dir| cd dir do - current_branch_head = ENV["GITHUB_SHA"] || tap.git_head + current_branch_head = ENV["GITHUB_SHA"] || tap.head_ref original_commit = if args.no_cherry_pick? # TODO: Handle the case where `merge-base` returns multiple commits. Utils.safe_popen_read("git", "-C", tap.path, "merge-base", "origin/HEAD", current_branch_head).strip diff --git a/Library/Homebrew/diagnostic.rb b/Library/Homebrew/diagnostic.rb index 79f28088a3..85264aa8f1 100644 --- a/Library/Homebrew/diagnostic.rb +++ b/Library/Homebrew/diagnostic.rb @@ -128,9 +128,9 @@ module Homebrew sig { params(repository_path: GitRepoPath, desired_origin: String).returns(T.nilable(String)) } def examine_git_origin(repository_path, desired_origin) - return if !Utils::Git.available? || !repository_path.git? + return if !Utils::Git.available? || !repository_path.git_repo? - current_origin = repository_path.git_origin + current_origin = repository_path.origin_url if current_origin.nil? <<~EOS diff --git a/Library/Homebrew/system_config.rb b/Library/Homebrew/system_config.rb index 7b4c1f4018..0350aad322 100644 --- a/Library/Homebrew/system_config.rb +++ b/Library/Homebrew/system_config.rb @@ -39,17 +39,17 @@ module SystemConfig sig { returns(String) } def head - homebrew_repo.git_head || "(none)" + homebrew_repo.head_ref || "(none)" end sig { returns(String) } def last_commit - homebrew_repo.git_last_commit || "never" + homebrew_repo.last_committed || "never" end sig { returns(String) } def origin - homebrew_repo.git_origin || "(none)" + homebrew_repo.origin_url || "(none)" end sig { returns(String) } diff --git a/Library/Homebrew/tap.rb b/Library/Homebrew/tap.rb index 45aa7a618f..5db8ba329c 100644 --- a/Library/Homebrew/tap.rb +++ b/Library/Homebrew/tap.rb @@ -138,7 +138,7 @@ class Tap def remote return default_remote unless installed? - @remote ||= path.git_origin + @remote ||= path.origin_url end # The remote repository name of this {Tap}. @@ -166,28 +166,28 @@ class Tap # True if this {Tap} is a Git repository. def git? - path.git? + path.git_repo? end # git branch for this {Tap}. def git_branch raise TapUnavailableError, name unless installed? - path.git_branch + path.branch_name end # git HEAD for this {Tap}. def git_head raise TapUnavailableError, name unless installed? - @git_head ||= path.git_head + @git_head ||= path.head_ref end # Time since last git commit for this {Tap}. def git_last_commit raise TapUnavailableError, name unless installed? - path.git_last_commit + path.last_committed end # The issues URL of this {Tap}. @@ -388,20 +388,20 @@ class Tap $stderr.ohai "#{name}: changed remote from #{remote} to #{requested_remote}" unless quiet end - current_upstream_head = T.must(path.git_origin_branch) - return if requested_remote.blank? && path.git_origin_has_branch?(current_upstream_head) + current_upstream_head = T.must(path.origin_branch_name) + return if requested_remote.blank? && path.origin_has_branch?(current_upstream_head) args = %w[fetch] args << "--quiet" if quiet args << "origin" safe_system "git", "-C", path, *args - path.git_origin_set_head_auto + path.set_head_origin_auto - new_upstream_head = T.must(path.git_origin_branch) + new_upstream_head = T.must(path.origin_branch_name) return if new_upstream_head == current_upstream_head - path.git_rename_branch old: current_upstream_head, new: new_upstream_head - path.git_branch_set_upstream local: new_upstream_head, origin: new_upstream_head + path.rename_branch old: current_upstream_head, new: new_upstream_head + path.set_upstream_branch local: new_upstream_head, origin: new_upstream_head return if quiet diff --git a/Library/Homebrew/test/dev-cmd/pr-pull_spec.rb b/Library/Homebrew/test/dev-cmd/pr-pull_spec.rb index 058fa99b22..88b8b9992a 100644 --- a/Library/Homebrew/test/dev-cmd/pr-pull_spec.rb +++ b/Library/Homebrew/test/dev-cmd/pr-pull_spec.rb @@ -98,8 +98,8 @@ describe "brew pr-pull" do File.write(formula_file, formula_version) safe_system Utils::Git.git, "commit", formula_file, "-m", "version", "--author=#{secondary_author}" described_class.autosquash!(original_hash, tap: tap) - expect(tap.path.git_commit_message).to include("foo 2.0") - expect(tap.path.git_commit_message).to include("Co-authored-by: #{secondary_author}") + expect(tap.path.commit_message).to include("foo 2.0") + expect(tap.path.commit_message).to include("Co-authored-by: #{secondary_author}") end (path/"Casks").mkpath @@ -113,8 +113,8 @@ describe "brew pr-pull" do File.write(cask_file, cask_version) safe_system Utils::Git.git, "commit", cask_file, "-m", "version", "--author=#{secondary_author}" described_class.autosquash!(original_hash, tap: tap) - expect(path.git_commit_message).to include("food 2.0") - expect(path.git_commit_message).to include("Co-authored-by: #{secondary_author}") + expect(path.commit_message).to include("food 2.0") + expect(path.commit_message).to include("Co-authored-by: #{secondary_author}") end end end @@ -129,7 +129,7 @@ describe "brew pr-pull" do safe_system Utils::Git.git, "commit", "-m", "foo 1.0 (new formula)" end described_class.signoff!(tap.path) - expect(tap.path.git_commit_message).to include("Signed-off-by:") + expect(tap.path.commit_message).to include("Signed-off-by:") (path/"Casks").mkpath cask_file.write(cask) @@ -138,7 +138,7 @@ describe "brew pr-pull" do safe_system Utils::Git.git, "commit", "-m", "food 1.0 (new cask)" end described_class.signoff!(tap.path) - expect(tap.path.git_commit_message).to include("Signed-off-by:") + expect(tap.path.commit_message).to include("Signed-off-by:") end end diff --git a/Library/Homebrew/utils/git_repository.rb b/Library/Homebrew/utils/git_repository.rb index 5ab0275ff1..f9f9f29d09 100644 --- a/Library/Homebrew/utils/git_repository.rb +++ b/Library/Homebrew/utils/git_repository.rb @@ -16,7 +16,7 @@ module Utils return git_short_head(repo, length: length) if length.present? repo = GitRepoPath.new(Pathname(repo)) - repo.git_head(safe: safe) + repo.head_ref(safe: safe) end # Gets a short commit hash of the HEAD commit. @@ -29,7 +29,7 @@ module Utils } def self.git_short_head(repo = Pathname.pwd, length: nil, safe: true) repo = GitRepoPath.new(Pathname(repo)) - repo.git_short_head(length: length, safe: safe) + repo.short_head_ref(length: length, safe: safe) end # Gets the name of the currently checked-out branch, or HEAD if the repository is in a detached HEAD state. @@ -41,7 +41,7 @@ module Utils } def self.git_branch(repo = Pathname.pwd, safe: true) repo = GitRepoPath.new(Pathname(repo)) - repo.git_branch(safe: safe) + repo.branch_name(safe: safe) end # Gets the full commit message of the specified commit, or of the HEAD commit if unspecified. @@ -54,6 +54,6 @@ module Utils } def self.git_commit_message(repo = Pathname.pwd, commit: "HEAD", safe: true) repo = GitRepoPath.new(Pathname(repo)) - repo.git_commit_message(commit, safe: safe) + repo.commit_message(commit, safe: safe) end end From 1ffb77f82154c3480478522eb9a4eadcce78589d Mon Sep 17 00:00:00 2001 From: Douglas Eichelberger Date: Sat, 15 Apr 2023 18:13:38 -0700 Subject: [PATCH 4/7] cleanup --- Library/Homebrew/dev-cmd/pr-pull.rb | 51 ++++++++++--------- Library/Homebrew/extend/git_repo_path.rbi | 7 --- Library/Homebrew/tap.rb | 29 +++++------ Library/Homebrew/test/dev-cmd/pr-pull_spec.rb | 15 +++--- 4 files changed, 48 insertions(+), 54 deletions(-) diff --git a/Library/Homebrew/dev-cmd/pr-pull.rb b/Library/Homebrew/dev-cmd/pr-pull.rb index 8b03ddbc55..9a3e0df9be 100644 --- a/Library/Homebrew/dev-cmd/pr-pull.rb +++ b/Library/Homebrew/dev-cmd/pr-pull.rb @@ -85,12 +85,12 @@ module Homebrew [subject, body, trailers] end - def self.signoff!(path, pull_request: nil, dry_run: false) - subject, body, trailers = separate_commit_message(path.commit_message) + def self.signoff!(git_repo, pull_request: nil, dry_run: false) + subject, body, trailers = separate_commit_message(git_repo.commit_message) if pull_request # This is a tap pull request and approving reviewers should also sign-off. - tap = Tap.from_path(path) + tap = Tap.from_path(git_repo.pathname) review_trailers = GitHub.approved_reviews(tap.user, tap.full_name.split("/").last, pull_request).map do |r| "Signed-off-by: #{r["name"]} <#{r["email"]}>" end @@ -101,7 +101,7 @@ module Homebrew body += "\n\n#{close_message}" unless body.include? close_message end - git_args = Utils::Git.git, "-C", path, "commit", "--amend", "--signoff", "--allow-empty", "--quiet", + git_args = Utils::Git.git, "-C", git_repo.pathname, "commit", "--amend", "--signoff", "--allow-empty", "--quiet", "--message", subject, "--message", body, "--message", trailers if dry_run @@ -156,21 +156,21 @@ module Homebrew # Cherry picks a single commit that modifies a single file. # Potentially rewords this commit using {determine_bump_subject}. - def self.reword_package_commit(commit, file, reason: "", verbose: false, resolve: false, path: ".") - package_file = Pathname.new(path) / file + def self.reword_package_commit(commit, file, reason: "", verbose: false, resolve: false, git_repo:) + package_file = git_repo.pathname / file package_name = package_file.basename.to_s.chomp(".rb") odebug "Cherry-picking #{package_file}: #{commit}" - Utils::Git.cherry_pick!(path, commit, verbose: verbose, resolve: resolve) + Utils::Git.cherry_pick!(git_repo, commit, verbose: verbose, resolve: resolve) - old_package = Utils::Git.file_at_commit(path, file, "HEAD^") - new_package = Utils::Git.file_at_commit(path, file, "HEAD") + old_package = Utils::Git.file_at_commit(git_repo, file, "HEAD^") + new_package = Utils::Git.file_at_commit(git_repo, file, "HEAD") bump_subject = determine_bump_subject(old_package, new_package, package_file, reason: reason).strip - subject, body, trailers = separate_commit_message(path.commit_message) + subject, body, trailers = separate_commit_message(git_repo.commit_message) if subject != bump_subject && !subject.start_with?("#{package_name}:") - safe_system("git", "-C", path, "commit", "--amend", "-q", + safe_system("git", "-C", git_repo.pathname, "commit", "--amend", "-q", "-m", bump_subject, "-m", subject, "-m", body, "-m", trailers) ohai bump_subject else @@ -181,7 +181,7 @@ module Homebrew # Cherry picks multiple commits that each modify a single file. # Words the commit according to {determine_bump_subject} with the body # corresponding to all the original commit messages combined. - def self.squash_package_commits(commits, file, reason: "", verbose: false, resolve: false, path: ".") + def self.squash_package_commits(commits, file, reason: "", verbose: false, resolve: false, git_repo:) odebug "Squashing #{file}: #{commits.join " "}" # Format commit messages into something similar to `git fmt-merge-message`. @@ -192,35 +192,35 @@ module Homebrew messages = [] trailers = [] commits.each do |commit| - subject, body, trailer = separate_commit_message(path.commit_message(commit)) + subject, body, trailer = separate_commit_message(git_repo.commit_message(commit)) body = body.lines.map { |line| " #{line.strip}" }.join("\n") messages << "* #{subject}\n#{body}".strip trailers << trailer end # Get the set of authors in this series. - authors = Utils.safe_popen_read("git", "-C", path, "show", + authors = Utils.safe_popen_read("git", "-C", git_repo.pathname, "show", "--no-patch", "--pretty=%an <%ae>", *commits).lines.map(&:strip).uniq.compact # Get the author and date of the first commit of this series, which we use for the squashed commit. original_author = authors.shift - original_date = Utils.safe_popen_read "git", "-C", path, "show", "--no-patch", "--pretty=%ad", commits.first + original_date = Utils.safe_popen_read "git", "-C", git_repo.pathname, "show", "--no-patch", "--pretty=%ad", commits.first # Generate trailers for coauthors and combine them with the existing trailers. co_author_trailers = authors.map { |au| "Co-authored-by: #{au}" } trailers = [trailers + co_author_trailers].flatten.uniq.compact # Apply the patch series but don't commit anything yet. - Utils::Git.cherry_pick!(path, "--no-commit", *commits, verbose: verbose, resolve: resolve) + Utils::Git.cherry_pick!(git_repo, "--no-commit", *commits, verbose: verbose, resolve: resolve) # Determine the bump subject by comparing the original state of the tree to its current state. - package_file = Pathname.new(path) / file - old_package = Utils::Git.file_at_commit(path, file, "#{commits.first}^") + package_file = git_repo.pathname / file + old_package = Utils::Git.file_at_commit(git_repo, file, "#{commits.first}^") new_package = package_file.read bump_subject = determine_bump_subject(old_package, new_package, package_file, reason: reason) # Commit with the new subject, body, and trailers. - safe_system("git", "-C", path, "commit", "--quiet", + safe_system("git", "-C", git_repo.pathname, "commit", "--quiet", "-m", bump_subject, "-m", messages.join("\n"), "-m", trailers.join("\n"), "--author", original_author, "--date", original_date, "--", file) ohai bump_subject @@ -228,7 +228,7 @@ module Homebrew # TODO: fix test in `test/dev-cmd/pr-pull_spec.rb` and assume `cherry_picked: false`. def self.autosquash!(original_commit, tap:, reason: "", verbose: false, resolve: false, cherry_picked: true) - original_head = tap.path.head_ref + original_head = tap.git_repo.head_ref commits = Utils.safe_popen_read("git", "-C", tap.path, "rev-list", "--reverse", "#{original_commit}..HEAD").lines.map(&:strip) @@ -268,13 +268,13 @@ module Homebrew files = commits_to_files[commit] if files.length == 1 && files_to_commits[files.first].length == 1 # If there's a 1:1 mapping of commits to files, just cherry pick and (maybe) reword. - reword_package_commit(commit, files.first, path: tap.path, reason: reason, verbose: verbose, resolve: resolve) + reword_package_commit(commit, files.first, git_repo: tap.git_repo, reason: reason, verbose: verbose, resolve: resolve) processed_commits << commit elsif files.length == 1 && files_to_commits[files.first].length > 1 # If multiple commits modify a single file, squash them down into a single commit. file = files.first commits = files_to_commits[file] - squash_package_commits(commits, file, path: tap.path, reason: reason, verbose: verbose, resolve: resolve) + squash_package_commits(commits, file, git_repo: tap.git_repo, reason: reason, verbose: verbose, resolve: resolve) processed_commits += commits else # We can't split commits (yet) so just raise an error. @@ -450,8 +450,9 @@ module Homebrew _, user, repo, pr = *url_match odie "Not a GitHub pull request: #{arg}" unless pr - if !tap.path.default_origin_branch? || args.branch_okay? || args.clean? - opoo "Current branch is #{tap.path.branch_name}: do you need to pull inside #{tap.path.origin_branch_name}?" + git_repo = tap.git_repo + if !git_repo.default_origin_branch? || args.branch_okay? || args.clean? + opoo "Current branch is #{git_repo.branch_name}: do you need to pull inside #{git_repo.origin_branch_name}?" end pr_labels = GitHub.pull_request_labels(user, repo, pr) @@ -464,7 +465,7 @@ module Homebrew ohai "Fetching #{tap} pull request ##{pr}" Dir.mktmpdir pr do |dir| cd dir do - current_branch_head = ENV["GITHUB_SHA"] || tap.head_ref + current_branch_head = ENV["GITHUB_SHA"] || tap.git_head original_commit = if args.no_cherry_pick? # TODO: Handle the case where `merge-base` returns multiple commits. Utils.safe_popen_read("git", "-C", tap.path, "merge-base", "origin/HEAD", current_branch_head).strip diff --git a/Library/Homebrew/extend/git_repo_path.rbi b/Library/Homebrew/extend/git_repo_path.rbi index b1fb6031dd..b94fb13774 100644 --- a/Library/Homebrew/extend/git_repo_path.rbi +++ b/Library/Homebrew/extend/git_repo_path.rbi @@ -7,11 +7,4 @@ class GitRepoPath < SimpleDelegator # @see https://github.com/sorbet/sorbet/issues/2378#issuecomment-569474238 sig { returns(Pathname) } def __getobj__; end - - def /(_arg0); end - def parent; end - def abv; end - def rmtree; end - def cd; end - def directory?; end end diff --git a/Library/Homebrew/tap.rb b/Library/Homebrew/tap.rb index 5db8ba329c..ad5fab96d0 100644 --- a/Library/Homebrew/tap.rb +++ b/Library/Homebrew/tap.rb @@ -93,7 +93,6 @@ class Tap # The local path to this {Tap}. # e.g. `/usr/local/Library/Taps/user/homebrew-repo` - sig { returns(GitRepoPath) } attr_reader :path alias git_repo path @@ -138,7 +137,7 @@ class Tap def remote return default_remote unless installed? - @remote ||= path.origin_url + @remote ||= git_repo.origin_url end # The remote repository name of this {Tap}. @@ -173,21 +172,21 @@ class Tap def git_branch raise TapUnavailableError, name unless installed? - path.branch_name + git_repo.branch_name end # git HEAD for this {Tap}. def git_head raise TapUnavailableError, name unless installed? - @git_head ||= path.head_ref + @git_head ||= git_repo.head_ref end # Time since last git commit for this {Tap}. def git_last_commit raise TapUnavailableError, name unless installed? - path.last_committed + git_repo.last_committed end # The issues URL of this {Tap}. @@ -317,7 +316,7 @@ class Tap ignore_interrupts do # wait for git to possibly cleanup the top directory when interrupt happens. sleep 0.1 - FileUtils.rm_rf git_repo.pathname + FileUtils.rm_rf path path.parent.rmdir_if_possible end raise @@ -388,20 +387,20 @@ class Tap $stderr.ohai "#{name}: changed remote from #{remote} to #{requested_remote}" unless quiet end - current_upstream_head = T.must(path.origin_branch_name) - return if requested_remote.blank? && path.origin_has_branch?(current_upstream_head) + current_upstream_head = git_repo.origin_branch_name + return if requested_remote.blank? && git_repo.origin_has_branch?(current_upstream_head) args = %w[fetch] args << "--quiet" if quiet args << "origin" safe_system "git", "-C", path, *args - path.set_head_origin_auto + git_repo.set_head_origin_auto - new_upstream_head = T.must(path.origin_branch_name) + new_upstream_head = git_repo.origin_branch_name return if new_upstream_head == current_upstream_head - path.rename_branch old: current_upstream_head, new: new_upstream_head - path.set_upstream_branch local: new_upstream_head, origin: new_upstream_head + git_repo.rename_branch old: current_upstream_head, new: new_upstream_head + git_repo.set_upstream_branch local: new_upstream_head, origin: new_upstream_head return if quiet @@ -464,7 +463,7 @@ class Tap sig { returns(T::Array[Pathname]) } def potential_formula_dirs - @potential_formula_dirs ||= [path/"Formula", path/"HomebrewFormula", git_repo.pathname].freeze + @potential_formula_dirs ||= [path/"Formula", path/"HomebrewFormula", path].freeze end # Path to the directory of all {Cask} files for this {Tap}. @@ -569,7 +568,7 @@ class Tap sig { params(file: T.any(String, Pathname)).returns(T::Boolean) } def formula_file?(file) file = Pathname.new(file) unless file.is_a? Pathname - file = file.expand_path(git_repo.pathname) + file = file.expand_path(path) return false unless ruby_file?(file) return false if cask_file?(file) @@ -582,7 +581,7 @@ class Tap sig { params(file: T.any(String, Pathname)).returns(T::Boolean) } def cask_file?(file) file = Pathname.new(file) unless file.is_a? Pathname - file = file.expand_path(git_repo.pathname) + file = file.expand_path(path) return false unless ruby_file?(file) file.to_s.start_with?("#{cask_dir}/") diff --git a/Library/Homebrew/test/dev-cmd/pr-pull_spec.rb b/Library/Homebrew/test/dev-cmd/pr-pull_spec.rb index 88b8b9992a..8cc627e162 100644 --- a/Library/Homebrew/test/dev-cmd/pr-pull_spec.rb +++ b/Library/Homebrew/test/dev-cmd/pr-pull_spec.rb @@ -81,7 +81,7 @@ describe "brew pr-pull" do let(:tap) { Tap.fetch("Homebrew", "foo") } let(:formula_file) { tap.path/"Formula/foo.rb" } let(:cask_file) { tap.cask_dir/"food.rb" } - let(:path) { GitRepoPath.new(Tap::TAP_DIRECTORY/"homebrew/homebrew-foo") } + let(:path) { Pathname(Tap::TAP_DIRECTORY/"homebrew/homebrew-foo") } describe "#autosquash!" do it "squashes a formula or cask correctly" do @@ -98,8 +98,8 @@ describe "brew pr-pull" do File.write(formula_file, formula_version) safe_system Utils::Git.git, "commit", formula_file, "-m", "version", "--author=#{secondary_author}" described_class.autosquash!(original_hash, tap: tap) - expect(tap.path.commit_message).to include("foo 2.0") - expect(tap.path.commit_message).to include("Co-authored-by: #{secondary_author}") + expect(tap.git_repo.commit_message).to include("foo 2.0") + expect(tap.git_repo.commit_message).to include("Co-authored-by: #{secondary_author}") end (path/"Casks").mkpath @@ -113,8 +113,9 @@ describe "brew pr-pull" do File.write(cask_file, cask_version) safe_system Utils::Git.git, "commit", cask_file, "-m", "version", "--author=#{secondary_author}" described_class.autosquash!(original_hash, tap: tap) - expect(path.commit_message).to include("food 2.0") - expect(path.commit_message).to include("Co-authored-by: #{secondary_author}") + git_repo = GitRepoPath.new(path) + expect(git_repo.commit_message).to include("food 2.0") + expect(git_repo.commit_message).to include("Co-authored-by: #{secondary_author}") end end end @@ -129,7 +130,7 @@ describe "brew pr-pull" do safe_system Utils::Git.git, "commit", "-m", "foo 1.0 (new formula)" end described_class.signoff!(tap.path) - expect(tap.path.commit_message).to include("Signed-off-by:") + expect(tap.git_repo.commit_message).to include("Signed-off-by:") (path/"Casks").mkpath cask_file.write(cask) @@ -138,7 +139,7 @@ describe "brew pr-pull" do safe_system Utils::Git.git, "commit", "-m", "food 1.0 (new cask)" end described_class.signoff!(tap.path) - expect(tap.path.commit_message).to include("Signed-off-by:") + expect(tap.git_repo.commit_message).to include("Signed-off-by:") end end From a6fbf5f1ac79a0d0bc08b18c96ca07a537bf6db6 Mon Sep 17 00:00:00 2001 From: Douglas Eichelberger Date: Sat, 15 Apr 2023 18:35:43 -0700 Subject: [PATCH 5/7] brew style --fix --- Library/Homebrew/dev-cmd/pr-pull.rb | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/Library/Homebrew/dev-cmd/pr-pull.rb b/Library/Homebrew/dev-cmd/pr-pull.rb index 9a3e0df9be..89065577cd 100644 --- a/Library/Homebrew/dev-cmd/pr-pull.rb +++ b/Library/Homebrew/dev-cmd/pr-pull.rb @@ -156,7 +156,7 @@ module Homebrew # Cherry picks a single commit that modifies a single file. # Potentially rewords this commit using {determine_bump_subject}. - def self.reword_package_commit(commit, file, reason: "", verbose: false, resolve: false, git_repo:) + def self.reword_package_commit(commit, file, git_repo:, reason: "", verbose: false, resolve: false) package_file = git_repo.pathname / file package_name = package_file.basename.to_s.chomp(".rb") @@ -181,7 +181,7 @@ module Homebrew # Cherry picks multiple commits that each modify a single file. # Words the commit according to {determine_bump_subject} with the body # corresponding to all the original commit messages combined. - def self.squash_package_commits(commits, file, reason: "", verbose: false, resolve: false, git_repo:) + def self.squash_package_commits(commits, file, git_repo:, reason: "", verbose: false, resolve: false) odebug "Squashing #{file}: #{commits.join " "}" # Format commit messages into something similar to `git fmt-merge-message`. @@ -204,7 +204,8 @@ module Homebrew # Get the author and date of the first commit of this series, which we use for the squashed commit. original_author = authors.shift - original_date = Utils.safe_popen_read "git", "-C", git_repo.pathname, "show", "--no-patch", "--pretty=%ad", commits.first + original_date = Utils.safe_popen_read "git", "-C", git_repo.pathname, "show", "--no-patch", "--pretty=%ad", + commits.first # Generate trailers for coauthors and combine them with the existing trailers. co_author_trailers = authors.map { |au| "Co-authored-by: #{au}" } @@ -268,13 +269,15 @@ module Homebrew files = commits_to_files[commit] if files.length == 1 && files_to_commits[files.first].length == 1 # If there's a 1:1 mapping of commits to files, just cherry pick and (maybe) reword. - reword_package_commit(commit, files.first, git_repo: tap.git_repo, reason: reason, verbose: verbose, resolve: resolve) + reword_package_commit(commit, files.first, git_repo: tap.git_repo, reason: reason, verbose: verbose, +resolve: resolve) processed_commits << commit elsif files.length == 1 && files_to_commits[files.first].length > 1 # If multiple commits modify a single file, squash them down into a single commit. file = files.first commits = files_to_commits[file] - squash_package_commits(commits, file, git_repo: tap.git_repo, reason: reason, verbose: verbose, resolve: resolve) + squash_package_commits(commits, file, git_repo: tap.git_repo, reason: reason, verbose: verbose, +resolve: resolve) processed_commits += commits else # We can't split commits (yet) so just raise an error. From b90897e28051b2eac44927c31efd076daf2fbb12 Mon Sep 17 00:00:00 2001 From: Douglas Eichelberger Date: Sat, 15 Apr 2023 19:06:40 -0700 Subject: [PATCH 6/7] Create git_repo attr --- Library/Homebrew/dev-cmd/extract.rb | 2 +- Library/Homebrew/dev-cmd/pr-pull.rb | 6 +++--- Library/Homebrew/diagnostic.rb | 14 +++++++------- Library/Homebrew/extend/git_repo_path.rbi | 10 ---------- .../extend/{git_repo_path.rb => git_repository.rb} | 2 +- Library/Homebrew/extend/git_repository.rbi | 9 +++++++-- Library/Homebrew/global.rb | 2 +- Library/Homebrew/system_config.rb | 4 ++-- Library/Homebrew/tap.rb | 10 +++++++--- Library/Homebrew/test/dev-cmd/pr-pull_spec.rb | 6 +++--- Library/Homebrew/utils/git.rb | 2 +- Library/Homebrew/utils/git_repository.rb | 8 ++++---- 12 files changed, 37 insertions(+), 38 deletions(-) delete mode 100644 Library/Homebrew/extend/git_repo_path.rbi rename Library/Homebrew/extend/{git_repo_path.rb => git_repository.rb} (99%) diff --git a/Library/Homebrew/dev-cmd/extract.rb b/Library/Homebrew/dev-cmd/extract.rb index 96542a3cdb..ac002199cc 100644 --- a/Library/Homebrew/dev-cmd/extract.rb +++ b/Library/Homebrew/dev-cmd/extract.rb @@ -113,7 +113,7 @@ module Homebrew end destination_tap.install unless destination_tap.installed? - repo = source_tap.git_repo.pathname + repo = source_tap.path pattern = if source_tap.core_tap? [repo/"Formula/#{name}.rb"] else diff --git a/Library/Homebrew/dev-cmd/pr-pull.rb b/Library/Homebrew/dev-cmd/pr-pull.rb index 89065577cd..a191bf2aa8 100644 --- a/Library/Homebrew/dev-cmd/pr-pull.rb +++ b/Library/Homebrew/dev-cmd/pr-pull.rb @@ -270,14 +270,14 @@ module Homebrew if files.length == 1 && files_to_commits[files.first].length == 1 # If there's a 1:1 mapping of commits to files, just cherry pick and (maybe) reword. reword_package_commit(commit, files.first, git_repo: tap.git_repo, reason: reason, verbose: verbose, -resolve: resolve) + resolve: resolve) processed_commits << commit elsif files.length == 1 && files_to_commits[files.first].length > 1 # If multiple commits modify a single file, squash them down into a single commit. file = files.first commits = files_to_commits[file] squash_package_commits(commits, file, git_repo: tap.git_repo, reason: reason, verbose: verbose, -resolve: resolve) + resolve: resolve) processed_commits += commits else # We can't split commits (yet) so just raise an error. @@ -483,7 +483,7 @@ resolve: resolve) autosquash!(original_commit, tap: tap, cherry_picked: !args.no_cherry_pick?, verbose: args.verbose?, resolve: args.resolve?, reason: args.message) end - signoff!(tap.path, pull_request: pr, dry_run: args.dry_run?) unless args.clean? + signoff!(tap.git_repo, pull_request: pr, dry_run: args.dry_run?) unless args.clean? end unless formulae_need_bottles?(tap, original_commit, pr_labels, args: args) diff --git a/Library/Homebrew/diagnostic.rb b/Library/Homebrew/diagnostic.rb index 85264aa8f1..070ba8dd61 100644 --- a/Library/Homebrew/diagnostic.rb +++ b/Library/Homebrew/diagnostic.rb @@ -126,7 +126,7 @@ module Homebrew EOS end - sig { params(repository_path: GitRepoPath, desired_origin: String).returns(T.nilable(String)) } + sig { params(repository_path: GitRepository, desired_origin: String).returns(T.nilable(String)) } def examine_git_origin(repository_path, desired_origin) return if !Utils::Git.available? || !repository_path.git_repo? @@ -156,8 +156,8 @@ module Homebrew def broken_tap(tap) return unless Utils::Git.available? - repo = HOMEBREW_REPOSITORY.dup.extend(GitRepositoryExtension) - return unless repo.git? + repo = GitRepository.new(HOMEBREW_REPOSITORY) + return unless repo.git_repo? message = <<~EOS #{tap.full_name} was not tapped properly! Run: @@ -169,7 +169,7 @@ module Homebrew tap_head = tap.git_head return message if tap_head.blank? - return if tap_head != repo.git_head + return if tap_head != repo.head_ref message end @@ -517,7 +517,7 @@ module Homebrew end def check_brew_git_origin - repo = HOMEBREW_REPOSITORY.dup.extend(GitRepositoryExtension) + repo = GitRepository.new(HOMEBREW_REPOSITORY) examine_git_origin(repo, Homebrew::EnvConfig.brew_git_remote) end @@ -529,14 +529,14 @@ module Homebrew CoreTap.ensure_installed! end - broken_tap(coretap) || examine_git_origin(coretap.path, Homebrew::EnvConfig.core_git_remote) + broken_tap(coretap) || examine_git_origin(coretap.git_repo, Homebrew::EnvConfig.core_git_remote) end def check_casktap_integrity default_cask_tap = Tap.default_cask_tap return unless default_cask_tap.installed? - broken_tap(default_cask_tap) || examine_git_origin(default_cask_tap.path, default_cask_tap.remote) + broken_tap(default_cask_tap) || examine_git_origin(default_cask_tap.git_repo, default_cask_tap.remote) end sig { returns(T.nilable(String)) } diff --git a/Library/Homebrew/extend/git_repo_path.rbi b/Library/Homebrew/extend/git_repo_path.rbi deleted file mode 100644 index b94fb13774..0000000000 --- a/Library/Homebrew/extend/git_repo_path.rbi +++ /dev/null @@ -1,10 +0,0 @@ -# typed: strict - -class GitRepoPath < SimpleDelegator - include Kernel - - # This is a workaround to enable `alias pathname __getobj__` - # @see https://github.com/sorbet/sorbet/issues/2378#issuecomment-569474238 - sig { returns(Pathname) } - def __getobj__; end -end diff --git a/Library/Homebrew/extend/git_repo_path.rb b/Library/Homebrew/extend/git_repository.rb similarity index 99% rename from Library/Homebrew/extend/git_repo_path.rb rename to Library/Homebrew/extend/git_repository.rb index 23eaf913eb..9a33202c78 100644 --- a/Library/Homebrew/extend/git_repo_path.rb +++ b/Library/Homebrew/extend/git_repository.rb @@ -7,7 +7,7 @@ require "utils/popen" # Extensions to {Pathname} for querying Git repository information. # @see Utils::Git # @api private -class GitRepoPath < SimpleDelegator +class GitRepository < SimpleDelegator extend T::Sig alias pathname __getobj__ diff --git a/Library/Homebrew/extend/git_repository.rbi b/Library/Homebrew/extend/git_repository.rbi index 9ffee241cf..002eda5ea8 100644 --- a/Library/Homebrew/extend/git_repository.rbi +++ b/Library/Homebrew/extend/git_repository.rbi @@ -1,5 +1,10 @@ # typed: strict -module GitRepositoryExtension - requires_ancestor { Pathname } +class GitRepository < SimpleDelegator + include Kernel + + # This is a workaround to enable `alias pathname __getobj__` + # @see https://github.com/sorbet/sorbet/issues/2378#issuecomment-569474238 + sig { returns(Pathname) } + def __getobj__; end end diff --git a/Library/Homebrew/global.rb b/Library/Homebrew/global.rb index 7e451d952a..3c3cecdb2d 100644 --- a/Library/Homebrew/global.rb +++ b/Library/Homebrew/global.rb @@ -131,7 +131,7 @@ end require "context" require "extend/array" -require "extend/git_repo_path" +require "extend/git_repository" require "extend/pathname" require "extend/predicable" require "extend/module" diff --git a/Library/Homebrew/system_config.rb b/Library/Homebrew/system_config.rb index 0350aad322..a5a1e48198 100644 --- a/Library/Homebrew/system_config.rb +++ b/Library/Homebrew/system_config.rb @@ -32,9 +32,9 @@ module SystemConfig end end - sig { returns(GitRepoPath) } + sig { returns(GitRepository) } def homebrew_repo - GitRepoPath.new(HOMEBREW_REPOSITORY) + GitRepository.new(HOMEBREW_REPOSITORY) end sig { returns(String) } diff --git a/Library/Homebrew/tap.rb b/Library/Homebrew/tap.rb index ad5fab96d0..ce98f055ab 100644 --- a/Library/Homebrew/tap.rb +++ b/Library/Homebrew/tap.rb @@ -93,9 +93,12 @@ class Tap # The local path to this {Tap}. # e.g. `/usr/local/Library/Taps/user/homebrew-repo` + sig { returns(Pathname) } attr_reader :path - alias git_repo path + # The git repository of this {Tap}. + sig { returns(GitRepository) } + attr_reader :git_repo # @private def initialize(user, repo) @@ -103,7 +106,8 @@ class Tap @repo = repo @name = "#{@user}/#{@repo}".downcase @full_name = "#{@user}/homebrew-#{@repo}" - @path = GitRepoPath.new(TAP_DIRECTORY/@full_name.downcase) + @path = TAP_DIRECTORY/@full_name.downcase + @git_repo = GitRepository.new(@path) @alias_table = nil @alias_reverse_table = nil end @@ -165,7 +169,7 @@ class Tap # True if this {Tap} is a Git repository. def git? - path.git_repo? + git_repo.git_repo? end # git branch for this {Tap}. diff --git a/Library/Homebrew/test/dev-cmd/pr-pull_spec.rb b/Library/Homebrew/test/dev-cmd/pr-pull_spec.rb index 8cc627e162..4465fc4a30 100644 --- a/Library/Homebrew/test/dev-cmd/pr-pull_spec.rb +++ b/Library/Homebrew/test/dev-cmd/pr-pull_spec.rb @@ -113,7 +113,7 @@ describe "brew pr-pull" do File.write(cask_file, cask_version) safe_system Utils::Git.git, "commit", cask_file, "-m", "version", "--author=#{secondary_author}" described_class.autosquash!(original_hash, tap: tap) - git_repo = GitRepoPath.new(path) + git_repo = GitRepository.new(path) expect(git_repo.commit_message).to include("food 2.0") expect(git_repo.commit_message).to include("Co-authored-by: #{secondary_author}") end @@ -129,7 +129,7 @@ describe "brew pr-pull" do safe_system Utils::Git.git, "add", formula_file safe_system Utils::Git.git, "commit", "-m", "foo 1.0 (new formula)" end - described_class.signoff!(tap.path) + described_class.signoff!(tap.git_repo) expect(tap.git_repo.commit_message).to include("Signed-off-by:") (path/"Casks").mkpath @@ -138,7 +138,7 @@ describe "brew pr-pull" do safe_system Utils::Git.git, "add", cask_file safe_system Utils::Git.git, "commit", "-m", "food 1.0 (new cask)" end - described_class.signoff!(tap.path) + described_class.signoff!(tap.git_repo) expect(tap.git_repo.commit_message).to include("Signed-off-by:") end end diff --git a/Library/Homebrew/utils/git.rb b/Library/Homebrew/utils/git.rb index 8b88654a12..10c36f2896 100644 --- a/Library/Homebrew/utils/git.rb +++ b/Library/Homebrew/utils/git.rb @@ -4,7 +4,7 @@ module Utils # Helper functions for querying Git information. # - # @see GitRepoPath + # @see GitRepository # @api private module Git extend T::Sig diff --git a/Library/Homebrew/utils/git_repository.rb b/Library/Homebrew/utils/git_repository.rb index f9f9f29d09..1e4c1bedbb 100644 --- a/Library/Homebrew/utils/git_repository.rb +++ b/Library/Homebrew/utils/git_repository.rb @@ -15,7 +15,7 @@ module Utils def self.git_head(repo = Pathname.pwd, length: nil, safe: true) return git_short_head(repo, length: length) if length.present? - repo = GitRepoPath.new(Pathname(repo)) + repo = GitRepository.new(Pathname(repo)) repo.head_ref(safe: safe) end @@ -28,7 +28,7 @@ module Utils ).returns(T.nilable(String)) } def self.git_short_head(repo = Pathname.pwd, length: nil, safe: true) - repo = GitRepoPath.new(Pathname(repo)) + repo = GitRepository.new(Pathname(repo)) repo.short_head_ref(length: length, safe: safe) end @@ -40,7 +40,7 @@ module Utils ).returns(T.nilable(String)) } def self.git_branch(repo = Pathname.pwd, safe: true) - repo = GitRepoPath.new(Pathname(repo)) + repo = GitRepository.new(Pathname(repo)) repo.branch_name(safe: safe) end @@ -53,7 +53,7 @@ module Utils ).returns(T.nilable(String)) } def self.git_commit_message(repo = Pathname.pwd, commit: "HEAD", safe: true) - repo = GitRepoPath.new(Pathname(repo)) + repo = GitRepository.new(Pathname(repo)) repo.commit_message(commit, safe: safe) end end From 403f08db8be3a943d443df0261cb122ca84f42d5 Mon Sep 17 00:00:00 2001 From: Douglas Eichelberger Date: Sat, 15 Apr 2023 19:35:00 -0700 Subject: [PATCH 7/7] Remove delegation --- Library/Homebrew/dev-cmd/pr-pull.rb | 17 +++++++++-------- Library/Homebrew/extend/git_repository.rbi | 10 ---------- Library/Homebrew/{extend => }/git_repository.rb | 12 +++++++++--- Library/Homebrew/global.rb | 2 +- Library/Homebrew/tap.rb | 4 ++-- Library/Homebrew/utils/git_repository.rb | 14 +++++--------- 6 files changed, 26 insertions(+), 33 deletions(-) delete mode 100644 Library/Homebrew/extend/git_repository.rbi rename Library/Homebrew/{extend => }/git_repository.rb (93%) diff --git a/Library/Homebrew/dev-cmd/pr-pull.rb b/Library/Homebrew/dev-cmd/pr-pull.rb index a191bf2aa8..80a75e5703 100644 --- a/Library/Homebrew/dev-cmd/pr-pull.rb +++ b/Library/Homebrew/dev-cmd/pr-pull.rb @@ -212,11 +212,11 @@ module Homebrew trailers = [trailers + co_author_trailers].flatten.uniq.compact # Apply the patch series but don't commit anything yet. - Utils::Git.cherry_pick!(git_repo, "--no-commit", *commits, verbose: verbose, resolve: resolve) + Utils::Git.cherry_pick!(git_repo.pathname, "--no-commit", *commits, verbose: verbose, resolve: resolve) # Determine the bump subject by comparing the original state of the tree to its current state. package_file = git_repo.pathname / file - old_package = Utils::Git.file_at_commit(git_repo, file, "#{commits.first}^") + old_package = Utils::Git.file_at_commit(git_repo.pathname, file, "#{commits.first}^") new_package = package_file.read bump_subject = determine_bump_subject(old_package, new_package, package_file, reason: reason) @@ -229,7 +229,8 @@ module Homebrew # TODO: fix test in `test/dev-cmd/pr-pull_spec.rb` and assume `cherry_picked: false`. def self.autosquash!(original_commit, tap:, reason: "", verbose: false, resolve: false, cherry_picked: true) - original_head = tap.git_repo.head_ref + git_repo = tap.git_repo + original_head = git_repo.head_ref commits = Utils.safe_popen_read("git", "-C", tap.path, "rev-list", "--reverse", "#{original_commit}..HEAD").lines.map(&:strip) @@ -269,15 +270,15 @@ module Homebrew files = commits_to_files[commit] if files.length == 1 && files_to_commits[files.first].length == 1 # If there's a 1:1 mapping of commits to files, just cherry pick and (maybe) reword. - reword_package_commit(commit, files.first, git_repo: tap.git_repo, reason: reason, verbose: verbose, - resolve: resolve) + reword_package_commit( + commit, files.first, git_repo: git_repo, reason: reason, verbose: verbose, resolve: resolve + ) processed_commits << commit elsif files.length == 1 && files_to_commits[files.first].length > 1 # If multiple commits modify a single file, squash them down into a single commit. file = files.first commits = files_to_commits[file] - squash_package_commits(commits, file, git_repo: tap.git_repo, reason: reason, verbose: verbose, - resolve: resolve) + squash_package_commits(commits, file, git_repo: git_repo, reason: reason, verbose: verbose, resolve: resolve) processed_commits += commits else # We can't split commits (yet) so just raise an error. @@ -483,7 +484,7 @@ module Homebrew autosquash!(original_commit, tap: tap, cherry_picked: !args.no_cherry_pick?, verbose: args.verbose?, resolve: args.resolve?, reason: args.message) end - signoff!(tap.git_repo, pull_request: pr, dry_run: args.dry_run?) unless args.clean? + signoff!(git_repo, pull_request: pr, dry_run: args.dry_run?) unless args.clean? end unless formulae_need_bottles?(tap, original_commit, pr_labels, args: args) diff --git a/Library/Homebrew/extend/git_repository.rbi b/Library/Homebrew/extend/git_repository.rbi deleted file mode 100644 index 002eda5ea8..0000000000 --- a/Library/Homebrew/extend/git_repository.rbi +++ /dev/null @@ -1,10 +0,0 @@ -# typed: strict - -class GitRepository < SimpleDelegator - include Kernel - - # This is a workaround to enable `alias pathname __getobj__` - # @see https://github.com/sorbet/sorbet/issues/2378#issuecomment-569474238 - sig { returns(Pathname) } - def __getobj__; end -end diff --git a/Library/Homebrew/extend/git_repository.rb b/Library/Homebrew/git_repository.rb similarity index 93% rename from Library/Homebrew/extend/git_repository.rb rename to Library/Homebrew/git_repository.rb index 9a33202c78..ba5aab4024 100644 --- a/Library/Homebrew/extend/git_repository.rb +++ b/Library/Homebrew/git_repository.rb @@ -4,13 +4,19 @@ require "utils/git" require "utils/popen" -# Extensions to {Pathname} for querying Git repository information. +# Given a {Pathname}, provides methods for querying Git repository information. # @see Utils::Git # @api private -class GitRepository < SimpleDelegator +class GitRepository extend T::Sig - alias pathname __getobj__ + sig { returns(Pathname) } + attr_reader :pathname + + sig { params(pathname: Pathname).void } + def initialize(pathname) + @pathname = pathname + end sig { returns(T::Boolean) } def git_repo? diff --git a/Library/Homebrew/global.rb b/Library/Homebrew/global.rb index 3c3cecdb2d..071eba618a 100644 --- a/Library/Homebrew/global.rb +++ b/Library/Homebrew/global.rb @@ -131,7 +131,7 @@ end require "context" require "extend/array" -require "extend/git_repository" +require "git_repository" require "extend/pathname" require "extend/predicable" require "extend/module" diff --git a/Library/Homebrew/tap.rb b/Library/Homebrew/tap.rb index ce98f055ab..cb4f3b080a 100644 --- a/Library/Homebrew/tap.rb +++ b/Library/Homebrew/tap.rb @@ -391,7 +391,7 @@ class Tap $stderr.ohai "#{name}: changed remote from #{remote} to #{requested_remote}" unless quiet end - current_upstream_head = git_repo.origin_branch_name + current_upstream_head = T.must(git_repo.origin_branch_name) return if requested_remote.blank? && git_repo.origin_has_branch?(current_upstream_head) args = %w[fetch] @@ -400,7 +400,7 @@ class Tap safe_system "git", "-C", path, *args git_repo.set_head_origin_auto - new_upstream_head = git_repo.origin_branch_name + new_upstream_head = T.must(git_repo.origin_branch_name) return if new_upstream_head == current_upstream_head git_repo.rename_branch old: current_upstream_head, new: new_upstream_head diff --git a/Library/Homebrew/utils/git_repository.rb b/Library/Homebrew/utils/git_repository.rb index 1e4c1bedbb..2af21e2989 100644 --- a/Library/Homebrew/utils/git_repository.rb +++ b/Library/Homebrew/utils/git_repository.rb @@ -13,10 +13,9 @@ module Utils ).returns(T.nilable(String)) } def self.git_head(repo = Pathname.pwd, length: nil, safe: true) - return git_short_head(repo, length: length) if length.present? + return git_short_head(repo, length: length) if length - repo = GitRepository.new(Pathname(repo)) - repo.head_ref(safe: safe) + GitRepository.new(Pathname(repo)).head_ref(safe: safe) end # Gets a short commit hash of the HEAD commit. @@ -28,8 +27,7 @@ module Utils ).returns(T.nilable(String)) } def self.git_short_head(repo = Pathname.pwd, length: nil, safe: true) - repo = GitRepository.new(Pathname(repo)) - repo.short_head_ref(length: length, safe: safe) + GitRepository.new(Pathname(repo)).short_head_ref(length: length, safe: safe) end # Gets the name of the currently checked-out branch, or HEAD if the repository is in a detached HEAD state. @@ -40,8 +38,7 @@ module Utils ).returns(T.nilable(String)) } def self.git_branch(repo = Pathname.pwd, safe: true) - repo = GitRepository.new(Pathname(repo)) - repo.branch_name(safe: safe) + GitRepository.new(Pathname(repo)).branch_name(safe: safe) end # Gets the full commit message of the specified commit, or of the HEAD commit if unspecified. @@ -53,7 +50,6 @@ module Utils ).returns(T.nilable(String)) } def self.git_commit_message(repo = Pathname.pwd, commit: "HEAD", safe: true) - repo = GitRepository.new(Pathname(repo)) - repo.commit_message(commit, safe: safe) + GitRepository.new(Pathname(repo)).commit_message(commit, safe: safe) end end