diff --git a/dev/merge_spark_pr.py b/dev/merge_spark_pr.py index 454e390d2c019..193449564492b 100755 --- a/dev/merge_spark_pr.py +++ b/dev/merge_spark_pr.py @@ -48,6 +48,11 @@ from urllib.request import Request from urllib.error import HTTPError +# Shared with dev/pr_merge_status.py so the two committer tools agree on where a PR landed. +# Importable because Python puts this script's own directory first on sys.path. +from spark_merge_footer import branches_with_merge_footer as _branches_with_merge_footer +from spark_merge_footer import has_merge_footer + try: import jira.client @@ -415,6 +420,55 @@ def get_json(url): sys.exit(-1) +def merge_commit_candidates(pr_events): + """Split `pr_events` into (closed_commits, referenced_commits), each oldest-first. + + Ordered by time so that a PR reopened and merged again yields its latest merge last. + + >>> merge_commit_candidates([{"event": "closed", "commit_id": "a", "created_at": "t2"}, + ... {"event": "referenced", "commit_id": "b", "created_at": "t1"}]) + (['a'], ['b']) + >>> merge_commit_candidates([{"event": "closed", "commit_id": None, "created_at": "t1"}]) + ([], []) + >>> merge_commit_candidates([{"event": "referenced", "commit_id": "c", "created_at": "t2"}, + ... {"event": "referenced", "commit_id": "b", "created_at": "t1"}]) + ([], ['b', 'c']) + """ + + def commits_of(event_name): + matched = [e for e in pr_events if e["event"] == event_name and e["commit_id"] is not None] + return [e["commit_id"] for e in sorted(matched, key=lambda x: x["created_at"])] + + return commits_of("closed"), commits_of("referenced") + + +def find_merge_commit(pr_num, pr_events): + """Return (hash, message) of the commit that merged `pr_num`, or (None, None). + + GitHub attributes the merge commit to the `closed` event only when that commit lands + on the default branch (master), because the "Closes #N" keyword in the commit message + is what closes the PR and the keyword is honored only there. A PR merged into any + other branch -- e.g. one opened against a rolling branch-M.x -- is instead closed by + this script through the API, and that `closed` event carries no commit, so the merge + survives only as a `referenced` event. Prefer the `closed` commit, which GitHub itself + linked; otherwise fall back to `referenced` events, which are also raised by any commit + merely mentioning the PR, so confirm each against the merge footer `merge_pr` generates. + """ + + def message_of(commit_hash): + return get_json("%s/commits/%s" % (GITHUB_API_BASE, commit_hash))["commit"]["message"] + + closed_commits, referenced_commits = merge_commit_candidates(pr_events) + if closed_commits: + return closed_commits[-1], message_of(closed_commits[-1]) + + for commit_hash in reversed(referenced_commits): + message = message_of(commit_hash) + if has_merge_footer(message, pr_num): + return commit_hash, message + return None, None + + def close_pr(pr_num): url = "%s/pulls/%s" % (GITHUB_API_BASE, pr_num) data = json.dumps({"state": "closed"}).encode("utf-8") @@ -615,6 +669,45 @@ def _do_cherry_pick(pr_num, merge_hash, pick_ref): return pick_ref, pick_hash +def branches_with_merge_footer(pr_num, branch_names): + """Release branches from `branch_names` that already carry `pr_num`'s merge footer. + + Thin wrapper over the shared reader in `spark_merge_footer`, adding this script's own + policy: a git failure here must not abort a merge that may already have pushed, so it + warns and reports nothing rather than exiting. Per that module's refresh policy no fetch + is issued, so a backport not yet fetched into PUSH_REMOTE_NAME's tracking refs is simply + not reported -- the committer is still prompted and can type any branch. + """ + try: + landed = _branches_with_merge_footer( + pr_num, PUSH_REMOTE_NAME, lambda args: run_cmd(["git"] + args) + ) + except Exception as e: + print_error("Could not scan for existing backports of #%s (%s)." % (pr_num, e)) + return [] + # Keep branch_names' newest-first order, and drop anything not a known release branch. + return [b for b in branch_names if b in landed] + + +def default_pick_branch(branch_names, already_picked): + """Highest-ranked release branch that has not already received the change, or None. + + `branch_names` is ordered newest-first (see `semver_branch_rank`) and `already_picked` + holds the branches the change is known to be on, so the prompt never defaults to a + branch where the cherry-pick would come up empty. Returns None when every known branch + already has it, so callers can say so instead of offering an empty pick. + + >>> default_pick_branch(["branch-4.x", "branch-4.3", "branch-4.2"], ("branch-4.x",)) + 'branch-4.3' + >>> default_pick_branch(["branch-4.x", "branch-4.3"], ()) + 'branch-4.x' + >>> default_pick_branch(["branch-4.x"], ("branch-4.x",)) is None + True + """ + remaining = [b for b in branch_names if b not in already_picked] + return remaining[0] if remaining else None + + def _upstream_first_sibling(target_ref, pick_ref, branch_names, already_picked): """Return the sibling branch-M.x if Upstream-First should prompt, else None. @@ -1663,17 +1756,15 @@ def main(): # Merged pull requests don't appear as merged in the GitHub API; # Instead, they're closed by committers. - merge_commits = [e for e in pr_events if e["event"] == "closed" and e["commit_id"] is not None] - - if merge_commits and pr["state"] == "closed": - # A PR might have multiple merge commits, if it's reopened and merged again. We shall - # cherry-pick PRs in closed state with the latest merge hash. - # If the PR is still open(reopened), we shall not cherry-pick it but perform the normal - # merge as it could have been reverted earlier. - merge_commits = sorted(merge_commits, key=lambda x: x["created_at"]) - merge_hash = merge_commits[-1]["commit_id"] - message = get_json("%s/commits/%s" % (GITHUB_API_BASE, merge_hash))["commit"]["message"] - + # A PR might have multiple merge commits, if it's reopened and merged again. We shall + # cherry-pick PRs in closed state with the latest merge hash. + # If the PR is still open(reopened), we shall not cherry-pick it but perform the normal + # merge as it could have been reverted earlier. + merge_hash, message = (None, None) + if pr["state"] == "closed": + merge_hash, message = find_merge_commit(pr_num, pr_events) + + if merge_hash is not None: print("Pull request %s has already been merged, assuming you want to backport" % pr_num) commit_is_downloaded = ( run_cmd(["git", "rev-parse", "--quiet", "--verify", "%s^{commit}" % merge_hash]).strip() @@ -1683,11 +1774,41 @@ def main(): fail("Couldn't find any merge commit for #%s, you may need to update HEAD." % pr_num) print("Found commit %s:\n%s" % (merge_hash, message)) - default = branch_names[0] - picked = cherry_pick( - pr_num, merge_hash, default, branch_names, target_ref, already_picked=() - ) - post_merge_comment(pr_num, picked) + # The change is already on target_ref and on any branch a previous run backported it + # to, so exclude all of them: defaulting to one would cherry-pick an empty commit. + picked_refs = [target_ref] + [ + b for b in branches_with_merge_footer(pr_num, branch_names) if b != target_ref + ] + if len(picked_refs) > 1: + print("Already backported to: %s" % ", ".join(picked_refs[1:])) + # Loop so one invocation can reach several maintenance branches, as the merge path does. + picked_commits = [] + try: + while True: + default = default_pick_branch(branch_names, tuple(picked_refs)) + if default is None: + print( + "Every known release branch already contains #%s; nothing to pick." % pr_num + ) + break + picked = cherry_pick( + pr_num, + merge_hash, + default, + branch_names, + target_ref, + already_picked=tuple(picked_refs), + ) + picked_refs = picked_refs + [ref for ref, _ in picked] + picked_commits = picked_commits + picked + prompt = "Would you like to pick %s into another branch?" % merge_hash + if get_input(f"\n{prompt} (y/N): ", ["y", "n", ""]) != "y": + break + finally: + # Report whatever was pushed even if a later pick is aborted, since the earlier + # pushes have already landed. + if picked_commits: + post_merge_comment(pr_num, picked_commits) sys.exit(0) if not bool(pr["mergeable"]): @@ -1765,19 +1886,20 @@ def main(): # then each cherry-pick target as it is picked. merged_commits = [(target_ref, merge_hash)] - # Walk a mutable remaining-branches list so the next default correctly skips any - # branches already picked, including branches consumed by the Upstream-First two-branch - # path inside cherry_pick (e.g. picking branch-M.x + branch-M.N in a single prompt). - # merged_refs doubles as the already_picked set passed to cherry_pick: it starts with - # target_ref (the merge sink, never to be re-picked) and grows with every cherry-pick. - remaining_branches = [b for b in branch_names if b != target_ref] + # merged_refs drives both the next prompt default and the already_picked set passed to + # cherry_pick, so each grows with every cherry-pick -- including branches consumed by the + # Upstream-First two-branch path inside cherry_pick (e.g. picking branch-M.x + branch-M.N + # in a single prompt). It starts with target_ref, the merge sink, never to be re-picked. pick_prompt = "Would you like to pick %s into another branch?" % merge_hash # Always record the merge summary for what actually landed, even if a later # cherry-pick is aborted or cancelled: the merge into the target branch has # already been pushed, so cancelling a backport must not drop that line. try: while get_input(f"\n{pick_prompt} (y/N): ", ["y", "n", ""]) == "y": - default = remaining_branches[0] if remaining_branches else branch_names[0] + default = default_pick_branch(branch_names, tuple(merged_refs)) + if default is None: + print("Every known release branch already contains #%s; nothing to pick." % pr_num) + break picked = cherry_pick( pr_num, merge_hash, @@ -1786,12 +1908,8 @@ def main(): target_ref, already_picked=tuple(merged_refs), ) - picked_refs = [ref for ref, _ in picked] - merged_refs = merged_refs + picked_refs + merged_refs = merged_refs + [ref for ref, _ in picked] merged_commits = merged_commits + picked - for b in picked_refs: - if b in remaining_branches: - remaining_branches.remove(b) finally: if merged_commits: # The "Closes #N" keyword in the commit message only auto-closes the PR when the diff --git a/dev/pr_merge_status.py b/dev/pr_merge_status.py index 510409f22500d..0449280826688 100755 --- a/dev/pr_merge_status.py +++ b/dev/pr_merge_status.py @@ -60,6 +60,11 @@ import subprocess import sys +# Shared with dev/merge_spark_pr.py so the two committer tools cannot disagree about where a +# PR landed. Importable because Python puts this script's own directory first on sys.path. +from spark_merge_footer import branches_with_merge_footer +from spark_merge_footer import merge_footer_trailer + REPO = "apache/spark" @@ -177,38 +182,6 @@ def fetch_branches(remote): ) -def commits_with_trailer(trailer, remote): - """Returns the full SHAs of commits on `remote`'s branches whose message contains - `trailer`. Scoping to the one remote (rather than `--all`) keeps fork refs and tags - from adding noise or walk cost.""" - out = git("log", "--remotes=%s" % remote, "--fixed-strings", "--grep", trailer, "--format=%H") - return list(dict.fromkeys(out.split())) - - -def official_branches_containing(commit, remote): - """Returns the `remote` branch names (e.g. 'master', 'branch-4.x') that contain - `commit`, ignoring the remote's HEAD alias and any non-branch refs.""" - out = git( - "for-each-ref", - "--contains", - commit, - "--format=%(refname:short)", - "refs/remotes/%s/" % remote, - ) - prefix = remote + "/" - branches = set() - for ref in out.splitlines(): - # Real branches are "/"; the remote's HEAD symref shortens to the - # bare remote name (e.g. "upstream") -- skip anything without the "/" prefix, - # and the explicit "/HEAD" form for good measure. - if not ref.startswith(prefix): - continue - name = ref[len(prefix) :] - if name != "HEAD": - branches.add(name) - return branches - - def display_key(name): """Sorts `master` first, then branch-. ascending, with branch-.x (the active dev line for the next feature release) after its numeric siblings.""" @@ -257,19 +230,24 @@ def main(): # its merge there, since a merge always lands on the base branch. majors = {m for m in (latest_major(remote), branch_major(base)) if m is not None} - trailer = "Closes #%s from " % pr - landed = {} - for commit in commits_with_trailer(trailer, remote): - for branch in official_branches_containing(commit, remote): - if all_branches or is_relevant(branch, majors): - landed[branch] = commit[:11] + # fetch_branches above satisfies the shared reader's refresh policy: it reads local + # remote-tracking refs only, so they must already be current. + all_landed = branches_with_merge_footer(pr, remote, lambda args: git(*args)) + landed = { + branch: commit[:11] + for branch, commit in all_landed.items() + if all_branches or is_relevant(branch, majors) + } if landed: print("merged: yes") for branch in sorted(landed, key=display_key): print(" %-12s %s" % (branch, landed[branch])) else: - print('closed without merging -- no "%s" commit found (rejected or superseded).' % trailer) + print( + 'closed without merging -- no "%s" commit found (rejected or superseded).' + % merge_footer_trailer(pr) + ) if __name__ == "__main__": diff --git a/dev/spark_merge_footer.py b/dev/spark_merge_footer.py new file mode 100644 index 0000000000000..2177989b24355 --- /dev/null +++ b/dev/spark_merge_footer.py @@ -0,0 +1,222 @@ +#!/usr/bin/env python3 + +# +# Licensed to the Apache Software Foundation (ASF) under one or more +# contributor license agreements. See the NOTICE file distributed with +# this work for additional information regarding copyright ownership. +# The ASF licenses this file to You under the Apache License, Version 2.0 +# (the "License"); you may not use this file except in compliance with +# the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +# + +""" +Shared reader for the merge footer that `dev/merge_spark_pr.py` writes into every commit +it creates, so the committer tools cannot disagree about where a pull request landed. + +`merge_pr` ends each message it generates with + + Closes # from /. + + Authored-by: A + Signed-off-by: C + +and `git cherry-pick -x` copies that footer verbatim into every backport, appending its own +provenance lines after it. The footer is therefore the signal that identifies both a merge +and its backports -- `git ... --contains ` cannot, because a cherry-pick is a +new commit that no other branch contains. + +Two properties make reading it reliable, and both are easy to get wrong: + +- A PR body is passed through as its own `git commit -m` paragraph, so it may quote another + commit's footer in full, structure included. Only *position* distinguishes the generated + footer: `merge_pr` appends it last, so the generated one is the final "Closes" paragraph. +- `git log --grep` matches its pattern anywhere in a message, so it can only narrow the + walk; every candidate it returns must still be validated with `has_merge_footer`. + +This module is import-only: it never exits, prints, or runs git itself. Callers pass a +`run_git` callable and so keep their own error-handling policy -- `dev/pr_merge_status.py` +exits on a git failure, while `dev/merge_spark_pr.py` must not abort a merge in progress. + +Refresh policy: `branches_with_merge_footer` reads local remote-tracking refs only and +never fetches. A caller that needs current data fetches first (as `pr_merge_status.py` +does); a caller that must not touch the network mid-run simply accepts that a branch not +yet fetched goes unreported. +""" + +import re + +# The generated footer: a "Closes # from " line alone on its paragraph, followed by +# the authors paragraph. `\s*$` tolerates trailing whitespace. Requiring the blank line and +# the authors line rejects prose that merely mentions a PR; taking the *last* match (see +# `merge_footer_pr`) rejects a body that quotes a real footer. +_MERGE_FOOTER_RE = re.compile( + r"^Closes #(\d+) from \S+\s*$\n\n(?:Lead-authored-by|Authored-by):", + re.MULTILINE, +) + + +def merge_footer_trailer(pr_num): + """The literal fragment to pass to `git log --fixed-strings --grep`. + + Only a prefilter to narrow the walk: it matches anywhere in a message, so callers + validate each candidate with `has_merge_footer`. + + >>> merge_footer_trailer(1) + 'Closes #1 from ' + """ + return "Closes #%s from " % pr_num + + +def merge_footer_pr(message): + """The PR number in `message`'s generated merge footer, or None if it has none. + + Reads the *last* "Closes" paragraph, since a PR body copied into the message may quote + an earlier one. Cherry-pick provenance lines may follow the footer, but no later + "Closes" paragraph can. + + >>> footer = "Closes #1 from a/b.\\n\\nAuthored-by: A \\nSigned-off-by: C " + >>> merge_footer_pr("[SPARK-1][SQL] Title\\n\\nSome body.\\n\\n" + footer) + 1 + >>> merge_footer_pr("[SPARK-1][SQL] Title\\n\\n" + footer.replace("Authored", "Lead-authored")) + 1 + >>> merge_footer_pr("[SPARK-1][SQL] Title\\n\\nNo footer here.") is None + True + + A cherry-pick keeps the footer, with `-x` provenance appended after it: + + >>> pick = footer + "\\n(cherry picked from commit abc123)\\nSigned-off-by: C " + >>> merge_footer_pr("[SPARK-1][SQL] Title\\n\\n" + pick) + 1 + + A body quoting another PR's complete footer does not shadow the real one: + + >>> quoted = "Reverting:\\n\\n" + footer + "\\n\\nSee above." + >>> own = footer.replace("#1", "#2") + >>> merge_footer_pr("[SPARK-2][SQL] Later\\n\\n%s\\n\\n%s" % (quoted, own)) + 2 + """ + matches = _MERGE_FOOTER_RE.findall(message) + return int(matches[-1]) if matches else None + + +def has_merge_footer(message, pr_num): + """Whether `message`'s generated merge footer closes `pr_num`. See `merge_footer_pr`. + + `pr_num` may be an int or a string of digits: callers get the PR number from argv or from + the GitHub API, and comparing those two forms directly would silently never match. + + >>> footer = "Closes #1 from a/b.\\n\\nAuthored-by: A \\nSigned-off-by: C " + >>> has_merge_footer("[SPARK-1][SQL] Title\\n\\n" + footer, 1) + True + >>> has_merge_footer("[SPARK-1][SQL] Title\\n\\n" + footer, "1") + True + >>> has_merge_footer("[SPARK-1][SQL] Title\\n\\n" + footer, 2) + False + + A commit whose body quotes another PR's full footer is not taken for that PR's merge: + + >>> quoted = "Reverting:\\n\\n" + footer + "\\n\\nSee above." + >>> later = "[SPARK-2][SQL] Later\\n\\n%s\\n\\n%s" % (quoted, footer.replace("#1", "#2")) + >>> has_merge_footer(later, 1) + False + >>> has_merge_footer(later, 2) + True + """ + return merge_footer_pr(message) == int(pr_num) + + +def parse_commit_records(out): + """Parse `git log --format='%H %B%x00'` output into (commit_hash, message) pairs. + + A commit message spans lines, so records are NUL-delimited rather than newline-delimited. + + >>> parse_commit_records("abc first\\nline two\\x00def second\\x00") + [('abc', 'first\\nline two'), ('def', 'second')] + >>> parse_commit_records("") + [] + """ + records = [] + for record in out.split("\0"): + record = record.strip("\n") + if not record: + continue + commit_hash, _, message = record.partition(" ") + records.append((commit_hash, message)) + return records + + +def branch_names_from_refs(out, remote): + """Branch names in `git for-each-ref --format='%(refname:short)'` output for `remote`. + + Real branches are "/"; the remote's HEAD symref shortens to the bare + remote name, so anything without the "/" prefix is skipped, as is the explicit + "/HEAD" form. + + >>> sorted(branch_names_from_refs("up/master\\nup/branch-4.x\\nup\\nup/HEAD\\n", "up")) + ['branch-4.x', 'master'] + """ + prefix = remote + "/" + names = set() + for ref in out.splitlines(): + if not ref.startswith(prefix): + continue + name = ref[len(prefix) :] + if name != "HEAD": + names.add(name) + return names + + +def branches_with_merge_footer(pr_num, remote, run_git): + """Map each `remote` branch carrying `pr_num`'s merge footer to the commit that has it. + + `run_git(args)` runs `git` with `args` and returns its stdout; the caller supplies it so + this module imposes no error-handling or exit policy of its own. Reads local + remote-tracking refs only -- see this module's refresh policy. + + Scoping the walk to `--remotes=` keeps fork refs and tags from adding noise or + cost. Every commit `--grep` returns is validated before its branches count, so a commit + that merely quotes the trailer cannot make a branch look like it has the change. + """ + out = run_git( + [ + "log", + "--remotes=%s" % remote, + "--fixed-strings", + "--grep", + merge_footer_trailer(pr_num), + "--format=%H %B%x00", + ] + ) + landed = {} + for commit_hash, message in parse_commit_records(out): + if not has_merge_footer(message, pr_num): + continue + refs = run_git( + [ + "for-each-ref", + "--contains", + commit_hash, + "--format=%(refname:short)", + "refs/remotes/%s/" % remote, + ] + ) + for branch in branch_names_from_refs(refs, remote): + landed[branch] = commit_hash + return landed + + +if __name__ == "__main__": + import doctest + import sys + + failure_count, test_count = doctest.testmod() + if failure_count: + sys.exit(-1) diff --git a/dev/sparktestsupport/modules.py b/dev/sparktestsupport/modules.py index 79e2e5a7bc2e5..8c7c0a8ec4cc2 100644 --- a/dev/sparktestsupport/modules.py +++ b/dev/sparktestsupport/modules.py @@ -1760,6 +1760,7 @@ def __hash__(self): "dev/merge_spark_pr.py", "dev/requirements.txt", "dev/pr_merge_status.py", + "dev/spark_merge_footer.py", "dev/create_spark_jira.py", "dev/create-release/", ],