diff --git a/Library/Homebrew/dev-cmd/pr-pull.rb b/Library/Homebrew/dev-cmd/pr-pull.rb index 61eeae1ac683be053a0f9e6d6c60537b5167ae22..88cd9532793b9b268d320edcfe478eb8f1cf0b5f 100644 --- a/Library/Homebrew/dev-cmd/pr-pull.rb +++ b/Library/Homebrew/dev-cmd/pr-pull.rb @@ -35,7 +35,7 @@ module Homebrew description: "Automatically reformat and reword commits in the pull request to our "\ "preferred format." switch "--branch-okay", - description: "Do not warn if pulling to a branch besides master (useful for testing)." + description: "Do not warn if pulling to a branch besides the repository default (useful for testing)." switch "--resolve", description: "When a patch fails to apply, leave in progress and allow user to resolve, "\ "instead of aborting." @@ -98,39 +98,43 @@ module Homebrew [subject, body, trailers] end - def signoff!(pr, tap:, args:) - message = Utils.popen_read "git", "-C", tap.path, "log", "-1", "--pretty=%B" - subject, body, trailers = separate_commit_message(message) + def signoff!(path, pr: nil, dry_run: false) + subject, body, trailers = separate_commit_message(Utils::Git.commit_message(path)) - # Approving reviewers also sign-off on merge. - trailers += GitHub.approved_reviews(tap.user, "homebrew-#{tap.repo}", pr).map do |r| - "Signed-off-by: #{r["name"]} <#{r["email"]}>" - end.join("\n") + if pr + # This is a tap pull request and approving reviewers should also sign-off. + tap = Tap.from_path(path) + trailers += GitHub.approved_reviews(tap.user, "homebrew-#{tap.repo}", pr).map do |r| + "Signed-off-by: #{r["name"]} <#{r["email"]}>" + end.join("\n") - close_message = "Closes ##{pr}." - body += "\n\n#{close_message}" unless body.include? close_message - new_message = [subject, body, trailers].join("\n\n").strip + # Append the close message as well, unless the commit body already includes it. + close_message = "Closes ##{pr}." + body += "\n\n#{close_message}" unless body.include? close_message + end - if args.dry_run? - puts "git commit --amend --signoff -m $message" + git_args = Utils::Git.git, "-C", path, "commit", "--amend", "--signoff", "--allow-empty", "--quiet", + "--message", subject, "--message", body, "--message", trailers + + if dry_run + puts(*git_args) else - safe_system "git", "-C", tap.path, "commit", "--amend", "--signoff", "--allow-empty", "-q", "-m", new_message + safe_system(*git_args) end end - def determine_bump_subject(file, original_commit, path: ".", reason: nil) - full_path = Pathname.new(path)/file - formula_name = File.basename(file.chomp(".rb")) + def determine_bump_subject(old_contents, new_contents, formula_path, reason: nil) + formula_path = Pathname(formula_path) + formula_name = formula_path.basename.to_s.chomp(".rb") new_formula = begin - Formulary::FormulaLoader.new(formula_name, full_path).get_formula(:stable) + Formulary.from_contents(formula_name, formula_path, new_contents, :stable) rescue FormulaUnavailableError - return "#{formula_name}: delete #{reason}" + return "#{formula_name}: delete #{reason}".strip end old_formula = begin - old_file = Utils.popen_read "git", "-C", path, "show", "#{original_commit}:#{file}" - Formulary.from_contents(formula_name, full_path, old_file, :stable) + Formulary.from_contents(formula_name, formula_path, old_contents, :stable) rescue FormulaUnavailableError return "#{formula_name} #{new_formula.stable.version} (new formula)" end @@ -138,22 +142,26 @@ module Homebrew if old_formula.stable.version != new_formula.stable.version "#{formula_name} #{new_formula.stable.version}" elsif old_formula.revision != new_formula.revision - "#{formula_name}: revision #{reason}" + "#{formula_name}: revision #{reason}".strip else - "#{formula_name}: #{reason || "rebuild"}" + "#{formula_name}: #{reason || "rebuild"}".strip end end # Cherry picks a single commit that modifies a single file. # Potentially rewords this commit using `determine_bump_subject`. - def reword_formula_commit(commit, file, args:, path: ".") - formula_name = File.basename(file.chomp(".rb")) - odebug "Cherry-picking #{file}: #{commit}" - Utils::Git.cherry_pick!(path, commit, verbose: args.verbose?, resolve: args.resolve?) + def reword_formula_commit(commit, file, reason: "", verbose: false, resolve: false, path: ".") + formula_file = Pathname.new(path) / file + formula_name = formula_file.basename.to_s.chomp(".rb") + + odebug "Cherry-picking #{formula_file}: #{commit}" + Utils::Git.cherry_pick!(path, commit, verbose: verbose, resolve: resolve) - bump_subject = determine_bump_subject(file, "HEAD^", path: path, reason: args.message).strip - message = Utils.popen_read("git", "-C", path, "log", "-1", "--pretty=%B") - subject, body, trailers = separate_commit_message(message) + old_formula = Utils::Git.file_at_commit(path, file, "HEAD^") + new_formula = Utils::Git.file_at_commit(path, file, "HEAD") + + bump_subject = determine_bump_subject(old_formula, new_formula, formula_file, reason: reason).strip + subject, body, trailers = separate_commit_message(Utils::Git.commit_message(path)) if subject != bump_subject && !subject.start_with?("#{formula_name}:") safe_system("git", "-C", path, "commit", "--amend", "-q", @@ -167,7 +175,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 squash_formula_commits(commits, file, args:, path: ".") + def squash_formula_commits(commits, file, reason: "", verbose: false, resolve: false, path: ".") odebug "Squashing #{file}: #{commits.join " "}" # Format commit messages into something similar to `git fmt-merge-message`. @@ -178,8 +186,7 @@ module Homebrew messages = [] trailers = [] commits.each do |commit| - original_message = Utils.safe_popen_read("git", "-C", path, "show", "--no-patch", "--pretty=%B", commit) - subject, body, trailer = separate_commit_message(original_message) + subject, body, trailer = separate_commit_message(Utils::Git.commit_message(path, commit)) body = body.lines.map { |line| " #{line.strip}" }.join("\n") messages << "* #{subject}\n#{body}".strip trailers << trailer @@ -198,10 +205,13 @@ module Homebrew 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: args.verbose?, resolve: args.resolve?) + Utils::Git.cherry_pick!(path, "--no-commit", *commits, verbose: verbose, resolve: resolve) # Determine the bump subject by comparing the original state of the tree to its current state. - bump_subject = determine_bump_subject(file, "#{commits.first}^", path: path, reason: args.message).strip + formula_file = Pathname.new(path) / file + old_formula = Utils::Git.file_at_commit(path, file, "#{commits.first}^") + new_formula = File.read(formula_file) + bump_subject = determine_bump_subject(old_formula, new_formula, formula_file, reason: reason) # Commit with the new subject, body, and trailers. safe_system("git", "-C", path, "commit", "--quiet", @@ -210,11 +220,7 @@ module Homebrew ohai bump_subject end - def autosquash!(original_commit, path: ".", args: nil) - # Autosquash assumes we've already modified the current state of the git repository, - # so just exit early if we're in a dry run. - return if args.dry_run? - + def autosquash!(original_commit, path: ".", reason: "", verbose: false, resolve: false) original_head = Utils.safe_popen_read("git", "-C", path, "rev-parse", "HEAD").strip commits = Utils.safe_popen_read("git", "-C", path, "rev-list", @@ -251,13 +257,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_formula_commit(commit, files.first, path: path, args: args) + reword_formula_commit(commit, files.first, path: path, 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_formula_commits(commits, file, path: path, args: args) + squash_formula_commits(commits, file, path: path, reason: reason, verbose: verbose, resolve: resolve) processed_commits += commits else # We can't split commits (yet) so just raise an error. @@ -275,30 +281,20 @@ module Homebrew raise end - def cherry_pick_pr!(pr, args:, path: ".") + def cherry_pick_pr!(user, repo, pr, args:, path: ".") if args.dry_run? puts <<~EOS git fetch --force origin +refs/pull/#{pr}/head git merge-base HEAD FETCH_HEAD git cherry-pick --ff --allow-empty $merge_base..FETCH_HEAD EOS - else - safe_system "git", "-C", path, "fetch", "--quiet", "--force", "origin", "+refs/pull/#{pr}/head" - merge_base = Utils.popen_read("git", "-C", path, "merge-base", "HEAD", "FETCH_HEAD").strip - commit_count = Utils.popen_read("git", "-C", path, "rev-list", "#{merge_base}..FETCH_HEAD").lines.count - - ohai "Using #{commit_count} commit#{"s" unless commit_count == 1} from ##{pr}" - Utils::Git.cherry_pick!(path, "--ff", "--allow-empty", "#{merge_base}..FETCH_HEAD", - verbose: args.verbose?, resolve: args.resolve?) + return end - end - - def check_branch(path, ref, args:) - branch = Utils.popen_read("git", "-C", path, "symbolic-ref", "--short", "HEAD").strip - return if branch == ref || args.clean? || args.branch_okay? - - opoo "Current branch is #{branch}: do you need to pull inside #{ref}?" + commits = GitHub.pull_request_commits(user, repo, pr) + safe_system "git", "-C", path, "fetch", "--quiet", "--force", "origin", commits.last + ohai "Using #{commits.count} commit#{"s" unless commits.count == 1} from ##{pr}" + Utils::Git.cherry_pick!(path, "--ff", "--allow-empty", *commits, verbose: args.verbose?, resolve: args.resolve?) end def formulae_need_bottles?(tap, original_commit, args:) @@ -385,15 +381,23 @@ module Homebrew _, user, repo, pr = *url_match odie "Not a GitHub pull request: #{arg}" unless pr - check_branch tap.path, "master", args: args + current_branch = Utils::Git.current_branch(tap.path) + origin_branch = Utils::Git.origin_branch(tap.path).split("/").last + + if current_branch != origin_branch || args.branch_okay? || args.clean? + opoo "Current branch is #{current_branch}: do you need to pull inside #{origin_branch}?" + end ohai "Fetching #{tap} pull request ##{pr}" Dir.mktmpdir pr do |dir| cd dir do original_commit = Utils.popen_read("git", "-C", tap.path, "rev-parse", "HEAD").chomp - cherry_pick_pr!(pr, path: tap.path, args: args) - autosquash!(original_commit, path: tap.path, args: args) if args.autosquash? - signoff!(pr, tap: tap, args: args) unless args.clean? + cherry_pick_pr!(user, repo, pr, path: tap.path, args: args) + if args.autosquash? && !args.dry_run? + autosquash!(original_commit, path: tap.path, + verbose: args.verbose?, resolve: args.resolve?, reason: args.message) + end + signoff!(tap.path, pr: pr, dry_run: args.dry_run?) unless args.clean? unless args.no_upload? mirror_formulae(tap, original_commit, diff --git a/Library/Homebrew/test/dev-cmd/pr-pull_spec.rb b/Library/Homebrew/test/dev-cmd/pr-pull_spec.rb index 99dfa041e9c5ecf60a41c0c4b99ce3a204ea08bb..f11823c52c4d33738f9249590a3dedc308f865e4 100644 --- a/Library/Homebrew/test/dev-cmd/pr-pull_spec.rb +++ b/Library/Homebrew/test/dev-cmd/pr-pull_spec.rb @@ -1,7 +1,104 @@ # frozen_string_literal: true +require "dev-cmd/pr-pull" +require "utils/git" +require "tap" require "cmd/shared_examples/args_parse" -describe "Homebrew.pr_pull_args" do - it_behaves_like "parseable arguments" +describe Homebrew do + let(:formula_rebuild) do + <<~EOS + class Foo < Formula + desc "Helpful description" + url "https://brew.sh/foo-1.0.tgz" + end + EOS + end + let(:formula_revision) do + <<~EOS + class Foo < Formula + url "https://brew.sh/foo-1.0.tgz" + revision 1 + end + EOS + end + let(:formula_version) do + <<~EOS + class Foo < Formula + url "https://brew.sh/foo-2.0.tgz" + end + EOS + end + let(:formula) do + <<~EOS + class Foo < Formula + url "https://brew.sh/foo-1.0.tgz" + end + EOS + end + let(:formula_file) { path/"Formula/foo.rb" } + let(:path) { Tap::TAP_DIRECTORY/"homebrew/homebrew-foo" } + + describe "Homebrew.pr_pull_args" do + it_behaves_like "parseable arguments" + end + + describe "#autosquash!" do + it "squashes a formula correctly" do + secondary_author = "Someone Else <me@example.com>" + (path/"Formula").mkpath + formula_file.write(formula) + cd path do + safe_system Utils::Git.git, "init" + safe_system Utils::Git.git, "add", formula_file + safe_system Utils::Git.git, "commit", "-m", "foo 1.0 (new formula)" + original_hash = `git rev-parse HEAD`.chomp + File.open(formula_file, "w") { |f| f.write(formula_revision) } + safe_system Utils::Git.git, "commit", formula_file, "-m", "revision" + File.open(formula_file, "w") { |f| f.write(formula_version) } + safe_system Utils::Git.git, "commit", formula_file, "-m", "version", "--author=#{secondary_author}" + described_class.autosquash!(original_hash, path: ".") + expect(Utils::Git.commit_message(path)).to include("foo 2.0") + expect(Utils::Git.commit_message(path)).to include("Co-authored-by: #{secondary_author}") + end + end + end + + describe "#signoff!" do + it "signs off a formula" do + (path/"Formula").mkpath + formula_file.write(formula) + cd path do + safe_system Utils::Git.git, "init" + safe_system Utils::Git.git, "add", formula_file + safe_system Utils::Git.git, "commit", "-m", "foo 1.0 (new formula)" + end + described_class.signoff!(path) + expect(Utils::Git.commit_message(path)).to include("Signed-off-by:") + end + end + + 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)") + end + + it "correctly bumps a formula version" do + expect(described_class.determine_bump_subject(formula, formula_version, formula_file)).to eq("foo 2.0") + end + + it "correctly bumps a formula revision with reason" do + expect(described_class.determine_bump_subject( + formula, formula_revision, formula_file, 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") + end + + it "correctly bumps a formula deletion" do + expect(described_class.determine_bump_subject(formula, "", formula_file)).to eq("foo: delete") + end + end end diff --git a/Library/Homebrew/test/utils/git_spec.rb b/Library/Homebrew/test/utils/git_spec.rb index 09bac00fe2e6f7c709e62083f98874124230a25f..f03f6143794032f5a504d0eda5c0750baf910f48 100644 --- a/Library/Homebrew/test/utils/git_spec.rb +++ b/Library/Homebrew/test/utils/git_spec.rb @@ -18,22 +18,28 @@ describe Utils::Git do File.open("README.md", "w") { |f| f.write("README") } system git, "add", HOMEBREW_CACHE/"README.md" - system git, "commit", "-m", "'File added'" + system git, "commit", "-m", "File added" @h1 = `git rev-parse HEAD` File.open("README.md", "w") { |f| f.write("# README") } system git, "add", HOMEBREW_CACHE/"README.md" - system git, "commit", "-m", "'written to File'" + system git, "commit", "-m", "written to File" @h2 = `git rev-parse HEAD` File.open("LICENSE.txt", "w") { |f| f.write("LICENCE") } system git, "add", HOMEBREW_CACHE/"LICENSE.txt" - system git, "commit", "-m", "'File added'" + system git, "commit", "-m", "File added" @h3 = `git rev-parse HEAD` File.open("LICENSE.txt", "w") { |f| f.write("LICENSE") } system git, "add", HOMEBREW_CACHE/"LICENSE.txt" - system git, "commit", "-m", "'written to File'" + system git, "commit", "-m", "written to File" + + File.open("LICENSE.txt", "w") { |f| f.write("test") } + system git, "add", HOMEBREW_CACHE/"LICENSE.txt" + system git, "commit", "-m", "written to File" + @cherry_pick_commit = `git rev-parse HEAD` + system git, "reset", "--hard", "HEAD^" end end @@ -43,8 +49,26 @@ describe Utils::Git do let(:files) { ["README.md", "LICENSE.txt"] } let(:files_hash1) { [@h3[0..6], ["LICENSE.txt"]] } let(:files_hash2) { [@h2[0..6], ["README.md"]] } + let(:cherry_pick_commit) { @cherry_pick_commit[0..6] } + + describe "#commit_message" do + it "returns the commit message" do + expect(described_class.commit_message(HOMEBREW_CACHE, file_hash1)).to eq("File added") + expect(described_class.commit_message(HOMEBREW_CACHE, file_hash2)).to eq("written to File") + end + + it "errors when commit doesn't exist" do + expect { + described_class.commit_message(HOMEBREW_CACHE, "bad_refspec") + }.to raise_error(ErrorDuringExecution, /bad revision/) + end + end describe "#cherry_pick!" do + it "can cherry pick a commit" do + expect(described_class.cherry_pick!(HOMEBREW_CACHE, cherry_pick_commit)).to be_truthy + end + it "aborts when cherry picking an existing hash" do expect { described_class.cherry_pick!(HOMEBREW_CACHE, file_hash1) @@ -68,6 +92,17 @@ describe Utils::Git do end end + describe "#file_at_commit" do + it "returns file contents when file exists" do + expect(described_class.file_at_commit(HOMEBREW_CACHE, file, file_hash1)).to eq("README") + end + + it "returns empty when file doesn't exist" do + expect(described_class.file_at_commit(HOMEBREW_CACHE, "foo.txt", file_hash1)).to eq("") + expect(described_class.file_at_commit(HOMEBREW_CACHE, "LICENSE.txt", file_hash1)).to eq("") + end + end + describe "#last_revision_commit_of_files" do context "when before_commit is nil" do it "gives last revision commit" do diff --git a/Library/Homebrew/test/utils/github_spec.rb b/Library/Homebrew/test/utils/github_spec.rb index 37f2fd04839551a36aaa6b2ab178009d16ffcaba..6c7739ff7100d89a6e0c7b1b9e8688f2aeb2a1d3 100644 --- a/Library/Homebrew/test/utils/github_spec.rb +++ b/Library/Homebrew/test/utils/github_spec.rb @@ -75,4 +75,11 @@ describe GitHub do expect(url).to eq("https://api.github.com/repos/Homebrew/homebrew-core/actions/artifacts/3557392/zip") end end + + describe "::pull_request_commits", :needs_network do + it "gets the correct commits hashes for a pull request" do + hashes = %w[188606a4a9587365d930b02c98ad6857b1d00150 25a71fe1ea1558415d6496d23834dc70778ddee5] + expect(subject.pull_request_commits("Homebrew", "legacy-homebrew", 50678)).to eq(hashes) + end + end end diff --git a/Library/Homebrew/utils/git.rb b/Library/Homebrew/utils/git.rb index 168c9ce439d67fb8be1b5249e87d2ca89266ffa2..ad1ebe7744569b8f2913d7cd213cec6b89d61d6d 100644 --- a/Library/Homebrew/utils/git.rb +++ b/Library/Homebrew/utils/git.rb @@ -76,9 +76,19 @@ module Utils def last_revision_of_file(repo, file, before_commit: nil) relative_file = Pathname(file).relative_path_from(repo) - commit_hash = last_revision_commit_of_file(repo, relative_file, before_commit: before_commit) - Utils.popen_read(git, "-C", repo, "show", "#{commit_hash}:#{relative_file}") + file_at_commit(repo, file, commit_hash) + end + + def file_at_commit(repo, file, commit) + relative_file = Pathname(file) + relative_file = relative_file.relative_path_from(repo) if relative_file.absolute? + Utils.popen_read(git, "-C", repo, "show", "#{commit}:#{relative_file}") + end + + def commit_message(repo, commit = nil) + commit ||= "HEAD" + Utils.safe_popen_read(git, "-C", repo, "log", "-1", "--pretty=%B", commit, "--", err: :out).strip end def ensure_installed! @@ -120,6 +130,10 @@ module Utils "refs/remotes/origin/HEAD").chomp.presence end + def current_branch(repo) + Utils.popen_read("git", "-C", repo, "symbolic-ref", "--short", "HEAD").chomp.presence + end + # Special case of `git cherry-pick` that permits non-verbose output and # optional resolution on merge conflict. def cherry_pick!(repo, *args, resolve: false, verbose: false) diff --git a/Library/Homebrew/utils/github.rb b/Library/Homebrew/utils/github.rb index 2ca8dc31985e13b6256bab78b5a0b779570d6581..ab34ee32d7169b00abf5c6a5db021ccb8c8e3347 100644 --- a/Library/Homebrew/utils/github.rb +++ b/Library/Homebrew/utils/github.rb @@ -747,4 +747,8 @@ module GitHub end end end + + def pull_request_commits(user, repo, pr) + open_api(url_to("repos", user, repo, "pulls", pr, "commits?per_page=100")).map { |c| c["sha"] } + end end diff --git a/docs/Manpage.md b/docs/Manpage.md index 9e3920301fdc2426987206a13ad13d66884bbdbe..143cf6ca4a9712263d11bf209be4f7acd6c6d534 100644 --- a/docs/Manpage.md +++ b/docs/Manpage.md @@ -1101,7 +1101,7 @@ Requires write access to the repository. * `--autosquash`: Automatically reformat and reword commits in the pull request to our preferred format. * `--branch-okay`: - Do not warn if pulling to a branch besides master (useful for testing). + Do not warn if pulling to a branch besides the repository default (useful for testing). * `--resolve`: When a patch fails to apply, leave in progress and allow user to resolve, instead of aborting. * `--warn-on-upload-failure`: diff --git a/manpages/brew.1 b/manpages/brew.1 index 8415cedd6e3adee0584e2ee4a6a4ffd963fdd27b..a392f009eef69d38b33f1af1e12489d22806c680 100644 --- a/manpages/brew.1 +++ b/manpages/brew.1 @@ -1528,7 +1528,7 @@ Automatically reformat and reword commits in the pull request to our preferred f . .TP \fB\-\-branch\-okay\fR -Do not warn if pulling to a branch besides master (useful for testing)\. +Do not warn if pulling to a branch besides the repository default (useful for testing)\. . .TP \fB\-\-resolve\fR