Merge pull request #8724 from jonchang/pr-pull-autosquash
pr-pull: implement autosquash option
This commit is contained in:
Jonathan Chang
GitHub
commit
164f0f0ee0
@ -31,6 +31,9 @@ module Homebrew
|
||||
switch "--keep-old",
|
||||
description: "If the formula specifies a rebuild version, " \
|
||||
"attempt to preserve its value in the generated DSL."
|
||||
switch "--autosquash",
|
||||
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)."
|
||||
switch "--resolve",
|
||||
@ -39,6 +42,9 @@ module Homebrew
|
||||
switch "--warn-on-upload-failure",
|
||||
description: "Warn instead of raising an error if the bottle upload fails. "\
|
||||
"Useful for repairing bottle uploads that previously failed."
|
||||
flag "--message=",
|
||||
depends_on: "--autosquash",
|
||||
description: "Message to include when autosquashing revision bumps, deletions, and rebuilds."
|
||||
flag "--workflow=",
|
||||
description: "Retrieve artifacts from the specified workflow (default: `tests.yml`)."
|
||||
flag "--artifact=",
|
||||
@ -52,8 +58,8 @@ module Homebrew
|
||||
flag "--bintray-mirror=",
|
||||
description: "Use the specified Bintray repository to automatically mirror stable URLs "\
|
||||
"defined in the formulae (default: `mirror`)."
|
||||
|
||||
min_named 1
|
||||
conflicts "--clean", "--autosquash"
|
||||
end
|
||||
end
|
||||
|
||||
@ -78,21 +84,29 @@ module Homebrew
|
||||
end
|
||||
end
|
||||
|
||||
def signoff!(pr, tap:, args:)
|
||||
message = Utils.popen_read "git", "-C", tap.path, "log", "-1", "--pretty=%B"
|
||||
# Separates a commit message into subject, body, and trailers.
|
||||
def separate_commit_message(message)
|
||||
subject = message.lines.first.strip
|
||||
|
||||
# Skip the subject and separate lines that look like trailers (e.g. "Co-authored-by")
|
||||
# from lines that look like regular body text.
|
||||
trailers, body = message.lines.drop(1).partition { |s| s.match?(/^[a-z-]+-by:/i) }
|
||||
|
||||
# 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"]}>\n"
|
||||
end
|
||||
trailers = trailers.uniq.join.strip
|
||||
body = body.join.strip.gsub(/\n{3,}/, "\n\n")
|
||||
|
||||
[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)
|
||||
|
||||
# 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")
|
||||
|
||||
close_message = "Closes ##{pr}."
|
||||
body += "\n\n#{close_message}" unless body.include? close_message
|
||||
new_message = [subject, body, trailers].join("\n\n").strip
|
||||
@ -104,6 +118,163 @@ module Homebrew
|
||||
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"))
|
||||
|
||||
new_formula = begin
|
||||
Formulary::FormulaLoader.new(formula_name, full_path).get_formula(:stable)
|
||||
rescue FormulaUnavailableError
|
||||
return "#{formula_name}: delete #{reason}"
|
||||
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)
|
||||
rescue FormulaUnavailableError
|
||||
return "#{formula_name} #{new_formula.stable.version} (new formula)"
|
||||
end
|
||||
|
||||
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}"
|
||||
else
|
||||
"#{formula_name}: #{reason || "rebuild"}"
|
||||
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?)
|
||||
|
||||
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)
|
||||
|
||||
if subject != bump_subject && !subject.start_with?("#{formula_name}:")
|
||||
safe_system("git", "-C", path, "commit", "--amend", "-q",
|
||||
"-m", bump_subject, "-m", subject, "-m", body, "-m", trailers)
|
||||
ohai bump_subject
|
||||
else
|
||||
ohai subject
|
||||
end
|
||||
end
|
||||
|
||||
# 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: ".")
|
||||
odebug "Squashing #{file}: #{commits.join " "}"
|
||||
|
||||
# Format commit messages into something similar to `git fmt-merge-message`.
|
||||
# * subject 1
|
||||
# * subject 2
|
||||
# optional body
|
||||
# * subject 3
|
||||
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)
|
||||
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",
|
||||
"--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
|
||||
|
||||
# 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: args.verbose?, resolve: args.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
|
||||
|
||||
# Commit with the new subject, body, and trailers.
|
||||
safe_system("git", "-C", path, "commit", "--quiet",
|
||||
"-m", bump_subject, "-m", messages.join("\n"), "-m", trailers.join("\n"),
|
||||
"--author", original_author, "--date", original_date, "--", file)
|
||||
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?
|
||||
|
||||
original_head = Utils.safe_popen_read("git", "-C", path, "rev-parse", "HEAD").strip
|
||||
|
||||
commits = Utils.safe_popen_read("git", "-C", path, "rev-list",
|
||||
"--reverse", "#{original_commit}..HEAD").lines.map(&:strip)
|
||||
|
||||
# Generate a bidirectional mapping of commits <=> formula files.
|
||||
files_to_commits = {}
|
||||
commits_to_files = commits.map do |commit|
|
||||
files = Utils.safe_popen_read("git", "-C", path, "diff-tree", "--diff-filter=AMD",
|
||||
"-r", "--name-only", "#{commit}^", commit).lines.map(&:strip)
|
||||
files.each do |file|
|
||||
files_to_commits[file] ||= []
|
||||
files_to_commits[file] << commit
|
||||
next if %r{^Formula/.*\.rb$}.match?(file)
|
||||
|
||||
odie <<~EOS
|
||||
Autosquash can't squash commits that modify non-formula files.
|
||||
File: #{file}
|
||||
Commit: #{commit}
|
||||
EOS
|
||||
end
|
||||
[commit, files]
|
||||
end.to_h
|
||||
|
||||
# Reset to state before cherry-picking.
|
||||
safe_system "git", "-C", path, "reset", "--hard", original_commit
|
||||
|
||||
# Iterate over every commit in the pull request series, but if we have to squash
|
||||
# multiple commits into one, ensure that we skip over commits we've already squashed.
|
||||
processed_commits = []
|
||||
commits.each do |commit|
|
||||
next if processed_commits.include? commit
|
||||
|
||||
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)
|
||||
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)
|
||||
processed_commits += commits
|
||||
else
|
||||
# We can't split commits (yet) so just raise an error.
|
||||
odie <<~EOS
|
||||
Autosquash can't split commits that modify multiple files.
|
||||
Commit: #{commit}
|
||||
Files: #{files.join " "}
|
||||
EOS
|
||||
end
|
||||
end
|
||||
rescue
|
||||
opoo "Autosquash encountered an error; resetting to original cherry-picked state at #{original_head}"
|
||||
system "git", "-C", path, "reset", "--hard", original_head
|
||||
system "git", "-C", path, "cherry-pick", "--abort"
|
||||
raise
|
||||
end
|
||||
|
||||
def cherry_pick_pr!(pr, args:, path: ".")
|
||||
if args.dry_run?
|
||||
puts <<~EOS
|
||||
@ -116,19 +287,9 @@ module Homebrew
|
||||
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
|
||||
|
||||
# git cherry-pick unfortunately has no quiet option
|
||||
ohai "Cherry-picking #{commit_count} commit#{"s" unless commit_count == 1} from ##{pr}"
|
||||
cherry_pick_args = "git", "-C", path, "cherry-pick", "--ff", "--allow-empty", "#{merge_base}..FETCH_HEAD"
|
||||
result = args.verbose? ? system(*cherry_pick_args) : quiet_system(*cherry_pick_args)
|
||||
|
||||
unless result
|
||||
if args.resolve?
|
||||
odie "Cherry-pick failed: try to resolve it."
|
||||
else
|
||||
system "git", "-C", path, "cherry-pick", "--abort"
|
||||
odie "Cherry-pick failed!"
|
||||
end
|
||||
end
|
||||
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?)
|
||||
end
|
||||
end
|
||||
|
||||
@ -231,6 +392,7 @@ module Homebrew
|
||||
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?
|
||||
|
||||
unless args.no_upload?
|
||||
|
||||
@ -44,6 +44,14 @@ describe Utils::Git do
|
||||
let(:files_hash1) { [@h3[0..6], ["LICENSE.txt"]] }
|
||||
let(:files_hash2) { [@h2[0..6], ["README.md"]] }
|
||||
|
||||
describe "#cherry_pick!" do
|
||||
it "aborts when cherry picking an existing hash" do
|
||||
expect {
|
||||
described_class.cherry_pick!(HOMEBREW_CACHE, file_hash1)
|
||||
}.to raise_error(ErrorDuringExecution, /Merge conflict in README.md/)
|
||||
end
|
||||
end
|
||||
|
||||
describe "#last_revision_commit_of_file" do
|
||||
it "gives last revision commit when before_commit is nil" do
|
||||
expect(
|
||||
|
||||
@ -1,7 +1,5 @@
|
||||
# frozen_string_literal: true
|
||||
|
||||
require "open3"
|
||||
|
||||
module Utils
|
||||
# Helper functions for querying Git information.
|
||||
#
|
||||
@ -16,7 +14,7 @@ module Utils
|
||||
def version
|
||||
return @version if defined?(@version)
|
||||
|
||||
stdout, _, status = system_command(HOMEBREW_SHIMS_PATH/"scm/git", args: ["--version"], print_stderr: false)
|
||||
stdout, _, status = system_command(git, args: ["--version"], print_stderr: false)
|
||||
@version = status.success? ? stdout.chomp[/git version (\d+(?:\.\d+)*)/, 1] : nil
|
||||
end
|
||||
|
||||
@ -24,7 +22,13 @@ module Utils
|
||||
return unless available?
|
||||
return @path if defined?(@path)
|
||||
|
||||
@path = Utils.popen_read(HOMEBREW_SHIMS_PATH/"scm/git", "--homebrew=print-path").chomp.presence
|
||||
@path = Utils.popen_read(git, "--homebrew=print-path").chomp.presence
|
||||
end
|
||||
|
||||
def git
|
||||
return @git if defined?(@git)
|
||||
|
||||
@git = HOMEBREW_SHIMS_PATH/"scm/git"
|
||||
end
|
||||
|
||||
def remote_exists?(url)
|
||||
@ -36,6 +40,7 @@ module Utils
|
||||
def clear_available_cache
|
||||
remove_instance_variable(:@version) if defined?(@version)
|
||||
remove_instance_variable(:@path) if defined?(@path)
|
||||
remove_instance_variable(:@git) if defined?(@git)
|
||||
end
|
||||
|
||||
def last_revision_commit_of_file(repo, file, before_commit: nil)
|
||||
@ -45,12 +50,7 @@ module Utils
|
||||
[before_commit.split("..").first]
|
||||
end
|
||||
|
||||
out, = Open3.capture3(
|
||||
HOMEBREW_SHIMS_PATH/"scm/git", "-C", repo,
|
||||
"log", "--format=%h", "--abbrev=7", "--max-count=1",
|
||||
*args, "--", file
|
||||
)
|
||||
out.chomp
|
||||
Utils.popen_read(git, "-C", repo, "log", "--format=%h", "--abbrev=7", "--max-count=1", *args, "--", file).chomp
|
||||
end
|
||||
|
||||
def last_revision_commit_of_files(repo, files, before_commit: nil)
|
||||
@ -66,12 +66,11 @@ module Utils
|
||||
# <file_path2>
|
||||
# ...
|
||||
# return [<commit_hash>, [file_path1, file_path2, ...]]
|
||||
out, = Open3.capture3(
|
||||
HOMEBREW_SHIMS_PATH/"scm/git", "-C", repo, "log",
|
||||
rev, *paths = Utils.popen_read(
|
||||
git, "-C", repo, "log",
|
||||
"--pretty=format:%h", "--abbrev=7", "--max-count=1",
|
||||
"--diff-filter=d", "--name-only", *args, "--", *files
|
||||
)
|
||||
rev, *paths = out.chomp.split(/\n/).reject(&:empty?)
|
||||
).lines.map(&:chomp).reject(&:empty?)
|
||||
[rev, paths]
|
||||
end
|
||||
|
||||
@ -79,11 +78,7 @@ module Utils
|
||||
relative_file = Pathname(file).relative_path_from(repo)
|
||||
|
||||
commit_hash = last_revision_commit_of_file(repo, relative_file, before_commit: before_commit)
|
||||
out, = Open3.capture3(
|
||||
HOMEBREW_SHIMS_PATH/"scm/git", "-C", repo,
|
||||
"show", "#{commit_hash}:#{relative_file}"
|
||||
)
|
||||
out
|
||||
Utils.popen_read(git, "-C", repo, "show", "#{commit_hash}:#{relative_file}")
|
||||
end
|
||||
|
||||
def ensure_installed!
|
||||
@ -121,8 +116,22 @@ module Utils
|
||||
end
|
||||
|
||||
def origin_branch(repo)
|
||||
Utils.popen_read("git", "-C", repo, "symbolic-ref", "-q", "--short",
|
||||
Utils.popen_read(git, "-C", repo, "symbolic-ref", "-q", "--short",
|
||||
"refs/remotes/origin/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)
|
||||
cmd = [git, "-C", repo, "cherry-pick"] + args
|
||||
output = Utils.popen_read(*cmd, err: :out)
|
||||
if $CHILD_STATUS.success?
|
||||
puts output if verbose
|
||||
output
|
||||
else
|
||||
system git, "-C", repo, "cherry-pick", "--abort" unless resolve
|
||||
raise ErrorDuringExecution.new(cmd, status: $CHILD_STATUS, output: [[:stdout, output]])
|
||||
end
|
||||
end
|
||||
end
|
||||
end
|
||||
|
||||
@ -1096,12 +1096,16 @@ Requires write access to the repository.
|
||||
Do not amend the commits from pull requests.
|
||||
* `--keep-old`:
|
||||
If the formula specifies a rebuild version, attempt to preserve its value in the generated DSL.
|
||||
* `--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).
|
||||
* `--resolve`:
|
||||
When a patch fails to apply, leave in progress and allow user to resolve, instead of aborting.
|
||||
* `--warn-on-upload-failure`:
|
||||
Warn instead of raising an error if the bottle upload fails. Useful for repairing bottle uploads that previously failed.
|
||||
* `--message`:
|
||||
Message to include when autosquashing revision bumps, deletions, and rebuilds.
|
||||
* `--workflow`:
|
||||
Retrieve artifacts from the specified workflow (default: `tests.yml`).
|
||||
* `--artifact`:
|
||||
|
||||
@ -1519,6 +1519,10 @@ Do not amend the commits from pull requests\.
|
||||
If the formula specifies a rebuild version, attempt to preserve its value in the generated DSL\.
|
||||
.
|
||||
.TP
|
||||
\fB\-\-autosquash\fR
|
||||
Automatically reformat and reword commits in the pull request to our preferred format\.
|
||||
.
|
||||
.TP
|
||||
\fB\-\-branch\-okay\fR
|
||||
Do not warn if pulling to a branch besides master (useful for testing)\.
|
||||
.
|
||||
@ -1531,6 +1535,10 @@ When a patch fails to apply, leave in progress and allow user to resolve, instea
|
||||
Warn instead of raising an error if the bottle upload fails\. Useful for repairing bottle uploads that previously failed\.
|
||||
.
|
||||
.TP
|
||||
\fB\-\-message\fR
|
||||
Message to include when autosquashing revision bumps, deletions, and rebuilds\.
|
||||
.
|
||||
.TP
|
||||
\fB\-\-workflow\fR
|
||||
Retrieve artifacts from the specified workflow (default: \fBtests\.yml\fR)\.
|
||||
.
|
||||
|
||||
Loading…
x
Reference in New Issue
Block a user