Merge pull request #15032 from dduugg/rm-git-extend

Refactor GitRepositoryExtension to avoid monkey-patching
This commit is contained in:
Mike McQuaid 2023-04-17 12:21:11 +01:00 committed by GitHub
commit 775cddf6d9
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
10 changed files with 115 additions and 103 deletions

View File

@ -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.git_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, git_repo:, reason: "", verbose: false, resolve: false)
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.git_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, git_repo:, reason: "", verbose: false, resolve: false)
odebug "Squashing #{file}: #{commits.join " "}"
# Format commit messages into something similar to `git fmt-merge-message`.
@ -192,35 +192,36 @@ 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(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.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 = 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.pathname, 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 +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.path.git_head
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)
@ -268,13 +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, path: tap.path, 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, path: tap.path, 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.
@ -450,8 +454,9 @@ 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}?"
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)
@ -479,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.path, 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)

View File

@ -126,10 +126,11 @@ module Homebrew
EOS
end
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?
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
@ -155,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:
@ -168,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
@ -516,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
@ -528,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)) }

View File

@ -1,5 +0,0 @@
# typed: strict
module GitRepositoryExtension
requires_ancestor { Pathname }
end

View File

@ -4,100 +4,108 @@
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
module GitRepositoryExtension
class GitRepository
extend T::Sig
sig { returns(Pathname) }
attr_reader :pathname
sig { params(pathname: Pathname).void }
def initialize(pathname)
@pathname = pathname
end
sig { returns(T::Boolean) }
def git?
join(".git").exist?
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: self
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
@ -105,10 +113,10 @@ module GitRepositoryExtension
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: #{self}"
raise "Not a Git repository: #{pathname}"
end
unless Utils::Git.available?
@ -117,6 +125,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

View File

@ -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"

View File

@ -1,4 +1,4 @@
# typed: false
# typed: true
# frozen_string_literal: true
require "hardware"
@ -32,24 +32,24 @@ module SystemConfig
end
end
sig { returns(Pathname) }
sig { returns(GitRepository) }
def homebrew_repo
HOMEBREW_REPOSITORY.dup.extend(GitRepositoryExtension)
GitRepository.new(HOMEBREW_REPOSITORY)
end
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) }
@ -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 (?<curl_version>[\d.]+)/ =~ out
"#{curl_version} => #{curl_path}"
match_data = /^curl (?<curl_version>[\d.]+)/.match(out)
if match_data
"#{match_data[:curl_version]} => #{curl_path}"
else
"N/A"
end

View File

@ -93,8 +93,13 @@ class Tap
# The local path to this {Tap}.
# e.g. `/usr/local/Library/Taps/user/homebrew-repo`
sig { returns(Pathname) }
attr_reader :path
# The git repository of this {Tap}.
sig { returns(GitRepository) }
attr_reader :git_repo
# @private
def initialize(user, repo)
@user = user
@ -102,7 +107,7 @@ class Tap
@name = "#{@user}/#{@repo}".downcase
@full_name = "#{@user}/homebrew-#{@repo}"
@path = TAP_DIRECTORY/@full_name.downcase
@path.extend(GitRepositoryExtension)
@git_repo = GitRepository.new(@path)
@alias_table = nil
@alias_reverse_table = nil
end
@ -136,7 +141,7 @@ class Tap
def remote
return default_remote unless installed?
@remote ||= path.git_origin
@remote ||= git_repo.origin_url
end
# The remote repository name of this {Tap}.
@ -164,28 +169,28 @@ class Tap
# True if this {Tap} is a Git repository.
def git?
path.git?
git_repo.git_repo?
end
# git branch for this {Tap}.
def git_branch
raise TapUnavailableError, name unless installed?
path.git_branch
git_repo.branch_name
end
# git HEAD for this {Tap}.
def git_head
raise TapUnavailableError, name unless installed?
@git_head ||= path.git_head
@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.git_last_commit
git_repo.last_committed
end
# The issues URL of this {Tap}.
@ -386,20 +391,20 @@ class Tap
$stderr.ohai "#{name}: changed remote from #{remote} to #{requested_remote}" unless quiet
end
current_upstream_head = path.git_origin_branch
return if requested_remote.blank? && path.git_origin_has_branch?(current_upstream_head)
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]
args << "--quiet" if quiet
args << "origin"
safe_system "git", "-C", path, *args
path.git_origin_set_head_auto
git_repo.set_head_origin_auto
new_upstream_head = path.git_origin_branch
new_upstream_head = T.must(git_repo.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
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

View File

@ -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) { 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.git_commit_message).to include("foo 2.0")
expect(tap.path.git_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.git_commit_message).to include("food 2.0")
expect(path.git_commit_message).to include("Co-authored-by: #{secondary_author}")
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
end
end
@ -128,8 +129,8 @@ 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)
expect(tap.path.git_commit_message).to include("Signed-off-by:")
described_class.signoff!(tap.git_repo)
expect(tap.git_repo.commit_message).to include("Signed-off-by:")
(path/"Casks").mkpath
cask_file.write(cask)
@ -137,8 +138,8 @@ 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)
expect(tap.path.git_commit_message).to include("Signed-off-by:")
described_class.signoff!(tap.git_repo)
expect(tap.git_repo.commit_message).to include("Signed-off-by:")
end
end

View File

@ -4,7 +4,7 @@
module Utils
# Helper functions for querying Git information.
#
# @see GitRepositoryExtension
# @see GitRepository
# @api private
module Git
extend T::Sig

View File

@ -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 = Pathname(repo).extend(GitRepositoryExtension)
repo.git_head(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 = Pathname(repo).extend(GitRepositoryExtension)
repo.git_short_head(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 = Pathname(repo).extend(GitRepositoryExtension)
repo.git_branch(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 = Pathname(repo).extend(GitRepositoryExtension)
repo.git_commit_message(commit, safe: safe)
GitRepository.new(Pathname(repo)).commit_message(commit, safe: safe)
end
end