Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Clean up Tap. #16730

Closed
wants to merge 1 commit into from
Closed

Clean up Tap. #16730

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
42 changes: 22 additions & 20 deletions Library/Homebrew/dev-cmd/pr-pull.rb
View file
Open in desktop
Expand Up @@ -87,12 +87,12 @@
[subject, body, trailers]
end

def self.signoff!(git_repo, pull_request: nil, dry_run: false)
def self.signoff!(tap, pull_request: nil, dry_run: false)
git_repo = tap.git_repo
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(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
Expand Down Expand Up @@ -130,26 +130,24 @@
end
end

def self.determine_bump_subject(old_contents, new_contents, subject_path, reason: nil)
def self.determine_bump_subject(old_contents, new_contents, subject_path, tap:, reason: nil)
subject_path = Pathname(subject_path)
tap = Tap.from_path(subject_path)
subject_name = subject_path.basename.to_s.chomp(".rb")
is_cask = subject_path.to_s.start_with?("#{tap.cask_dir}/")
name = is_cask ? "cask" : "formula"

new_package = get_package(tap, subject_name, subject_path, new_contents)
type = "cask" if tap.cask_file?(subject_path)
type = "formula" if tap.formula_file?(subject_path)

return "#{subject_name}: delete #{reason}".strip if new_package.blank?
new_package = get_package(tap, subject_name, subject_path, new_contents)
return "#{subject_name}: delete#{reason&.prepend(" ")}" if new_package.blank?

old_package = get_package(tap, subject_name, subject_path, old_contents)
return "#{subject_name} #{new_package.version} (new #{type})" if old_package.blank?

if old_package.blank?
"#{subject_name} #{new_package.version} (new #{name})"
elsif old_package.version != new_package.version
if old_package.version != new_package.version
"#{subject_name} #{new_package.version}"
elsif !is_cask && old_package.revision != new_package.revision
elsif type == "formula" && old_package.revision != new_package.revision
"#{subject_name}: revision #{reason}".strip
elsif is_cask && old_package.sha256 != new_package.sha256
elsif type == "cask" && old_package.sha256 != new_package.sha256
"#{subject_name}: checksum update #{reason}".strip
else
"#{subject_name}: #{reason || "rebuild"}".strip
Expand All @@ -158,7 +156,9 @@

# 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, git_repo:, reason: "", verbose: false, resolve: false)
def self.reword_package_commit(commit, file, tap:, reason: "", verbose: false, resolve: false)
git_repo = tap.git_repo

Check warning on line 160 in Library/Homebrew/dev-cmd/pr-pull.rb

View check run for this annotation

Codecov / codecov/patch

Library/Homebrew/dev-cmd/pr-pull.rb#L160 

Added line #L160 was not covered by tests

package_file = git_repo.pathname / file
package_name = package_file.basename.to_s.chomp(".rb")

Expand All @@ -168,7 +168,7 @@
old_package = Utils::Git.file_at_commit(git_repo.to_s, file, "HEAD^")
new_package = Utils::Git.file_at_commit(git_repo.to_s, file, "HEAD")

bump_subject = determine_bump_subject(old_package, new_package, package_file, reason:).strip
bump_subject = determine_bump_subject(old_package, new_package, package_file, reason:, tap:).strip

Check warning on line 171 in Library/Homebrew/dev-cmd/pr-pull.rb

View check run for this annotation

Codecov / codecov/patch

Library/Homebrew/dev-cmd/pr-pull.rb#L171 

Added line #L171 was not covered by tests
subject, body, trailers = separate_commit_message(git_repo.commit_message)

if subject != bump_subject && !subject.start_with?("#{package_name}:")
Expand All @@ -183,9 +183,11 @@
# 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, git_repo:, reason: "", verbose: false, resolve: false)
def self.squash_package_commits(commits, file, tap:, reason: "", verbose: false, resolve: false)
odebug "Squashing #{file}: #{commits.join " "}"

git_repo = tap.git_repo

# Format commit messages into something similar to `git fmt-merge-message`.
# * subject 1
# * subject 2
Expand Down Expand Up @@ -220,7 +222,7 @@
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:)
bump_subject = determine_bump_subject(old_package, new_package, package_file, tap:, reason:)

# Commit with the new subject, body, and trailers.
safe_system("git", "-C", git_repo.pathname, "commit", "--quiet",
Expand Down Expand Up @@ -273,14 +275,14 @@
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:, reason:, verbose:, resolve:
commit, files.first, tap:, reason:, verbose:, 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:, reason:, verbose:, resolve:)
squash_package_commits(commits, file, tap:, reason:, verbose:, resolve:)
processed_commits += commits
else
# We can't split commits (yet) so just raise an error.
Expand Down Expand Up @@ -464,7 +466,7 @@
autosquash!(original_commit, tap:, cherry_picked: !args.no_cherry_pick?,
verbose: args.verbose?, resolve: args.resolve?, reason: args.message)
end
signoff!(git_repo, pull_request: pr, dry_run: args.dry_run?) unless args.clean?
signoff!(tap, pull_request: pr, dry_run: args.dry_run?) unless args.clean?
end

unless formulae_need_bottles?(tap, original_commit, pr_labels, args:)
Expand Down
2 changes: 1 addition & 1 deletion Library/Homebrew/diagnostic.rb
View file
Open in desktop
Expand Up @@ -537,7 +537,7 @@
core_cask_tap = CoreCaskTap.instance
return unless core_cask_tap.installed?

broken_tap(core_cask_tap) || examine_git_origin(core_cask_tap.git_repo, core_cask_tap.remote)
broken_tap(core_cask_tap) || examine_git_origin(core_cask_tap.git_repo, T.must(core_cask_tap.remote))

Check warning on line 540 in Library/Homebrew/diagnostic.rb

View check run for this annotation

Codecov / codecov/patch

Library/Homebrew/diagnostic.rb#L540 

Added line #L540 was not covered by tests
end

sig { returns(T.nilable(String)) }
Expand Down
26 changes: 15 additions & 11 deletions Library/Homebrew/test/dev-cmd/pr-pull_spec.rb
View file
Open in desktop
Expand Up @@ -128,7 +128,7 @@ class Foo < Formula
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.git_repo)
described_class.signoff!(tap)
expect(tap.git_repo.commit_message).to include("Signed-off-by:")

(path/"Casks").mkpath
Expand All @@ -137,7 +137,7 @@ class Foo < Formula
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.git_repo)
described_class.signoff!(tap)
expect(tap.git_repo.commit_message).to include("Signed-off-by:")
end
end
Expand All @@ -162,41 +162,45 @@ class Foo < Formula

describe "#determine_bump_subject" do
it "correctly bumps a new formula" do
expect(described_class.determine_bump_subject("", formula, formula_file)).to eq("foo 1.0 (new formula)")
expect(described_class.determine_bump_subject("", formula, formula_file,
tap:)).to eq("foo 1.0 (new formula)")
end

it "correctly bumps a new cask" do
expect(described_class.determine_bump_subject("", cask, cask_file)).to eq("food 1.0 (new cask)")
expect(described_class.determine_bump_subject("", cask, cask_file, tap:)).to eq("food 1.0 (new cask)")
end

it "correctly bumps a formula version" do
expect(described_class.determine_bump_subject(formula, formula_version, formula_file)).to eq("foo 2.0")
expect(described_class.determine_bump_subject(formula, formula_version, formula_file,
tap:)).to eq("foo 2.0")
end

it "correctly bumps a cask version" do
expect(described_class.determine_bump_subject(cask, cask_version, cask_file)).to eq("food 2.0")
expect(described_class.determine_bump_subject(cask, cask_version, cask_file, tap:)).to eq("food 2.0")
end

it "correctly bumps a cask checksum" do
expect(described_class.determine_bump_subject(cask, cask_checksum, cask_file)).to eq("food: checksum update")
expect(described_class.determine_bump_subject(cask, cask_checksum, cask_file,
tap:)).to eq("food: checksum update")
end

it "correctly bumps a formula revision with reason" do
expect(described_class.determine_bump_subject(
formula, formula_revision, formula_file, reason: "for fun"
formula, formula_revision, formula_file, tap:, reason: "for fun"
)).to eq("foo: revision for fun")
end

it "correctly bumps a formula rebuild" do
expect(described_class.determine_bump_subject(formula, formula_rebuild, formula_file)).to eq("foo: rebuild")
expect(described_class.determine_bump_subject(formula, formula_rebuild, formula_file,
tap:)).to eq("foo: rebuild")
end

it "correctly bumps a formula deletion" do
expect(described_class.determine_bump_subject(formula, "", formula_file)).to eq("foo: delete")
expect(described_class.determine_bump_subject(formula, "", formula_file, tap:)).to eq("foo: delete")
end

it "correctly bumps a cask deletion" do
expect(described_class.determine_bump_subject(cask, "", cask_file)).to eq("food: delete")
expect(described_class.determine_bump_subject(cask, "", cask_file, tap:)).to eq("food: delete")
end
end
end
Expand Down