diff --git a/Library/Homebrew/dev-cmd/pr-pull.rb b/Library/Homebrew/dev-cmd/pr-pull.rb index 6d57517098..bb8ac10e9d 100644 --- a/Library/Homebrew/dev-cmd/pr-pull.rb +++ b/Library/Homebrew/dev-cmd/pr-pull.rb @@ -98,22 +98,28 @@ module Homebrew [subject, body, trailers] end - def signoff!(pr, tap:, args:) - subject, body, trailers = separate_commit_message(Utils::Git.commit_message(tap.path)) + 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 @@ -392,7 +398,7 @@ module Homebrew original_commit = Utils.popen_read("git", "-C", tap.path, "rev-parse", "HEAD").chomp cherry_pick_pr!(user, repo, 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? + 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 604a32c238..7fb6578bd7 100644 --- a/Library/Homebrew/test/dev-cmd/pr-pull_spec.rb +++ b/Library/Homebrew/test/dev-cmd/pr-pull_spec.rb @@ -1,68 +1,83 @@ # frozen_string_literal: true require "dev-cmd/pr-pull" +require "utils/git" +require "tap" require "cmd/shared_examples/args_parse" 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 "#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 - let(:formula) do - <<~EOS - class Foo < Formula - url "https://brew.sh/foo-1.0.tgz" - end - EOS - end - - let(:formula_version) do - <<~EOS - class Foo < Formula - url "https://brew.sh/foo-2.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_rebuild) do - <<~EOS - class Foo < Formula - desc "Helpful description" - url "https://brew.sh/foo-1.0.tgz" - end - EOS - end - it "correctly bumps a new formula" do - expect(described_class.determine_bump_subject("", formula, "foo.rb")).to eq("foo 1.0 (new formula)") + 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, "foo.rb")).to eq("foo 2.0") + 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, "foo.rb", reason: "for fun" + 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, "foo.rb")).to eq("foo: rebuild") + 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, "", "foo.rb")).to eq("foo: delete") + expect(described_class.determine_bump_subject(formula, "", formula_file)).to eq("foo: delete") end end end