From 1ffb77f82154c3480478522eb9a4eadcce78589d Mon Sep 17 00:00:00 2001 From: Douglas Eichelberger Date: Sat, 15 Apr 2023 18:13:38 -0700 Subject: [PATCH] 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