From 29ecbf08bcc943229b352d0664df450cec087ba0 Mon Sep 17 00:00:00 2001 From: Uros Bojanic <221401595+uros-b@users.noreply.github.com> Date: Tue, 4 Aug 2026 14:24:43 +0000 Subject: [PATCH 1/5] [SPARK-58561][INFRA] Support backporting a PR merged into a non-default branch in `merge_spark_pr.py` Detect an already-merged PR from `referenced` events when the `closed` event carries no commit, so a PR merged into a non-default branch (e.g. one opened against a rolling `branch-M.x`) can still be cherry-picked with the script. Default the backport prompt to the highest branch that has not already received the change, instead of the highest branch overall, which was the branch the PR had just merged into and produced an empty cherry-pick. --- dev/merge_spark_pr.py | 117 ++++++++++++++++++++++++++++++++---------- 1 file changed, 91 insertions(+), 26 deletions(-) diff --git a/dev/merge_spark_pr.py b/dev/merge_spark_pr.py index 454e390d2c01..43ca0aa8ea8b 100755 --- a/dev/merge_spark_pr.py +++ b/dev/merge_spark_pr.py @@ -415,6 +415,59 @@ 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, confirming each against the + "Closes #N from " line that `merge_pr` writes so that an unrelated commit merely + mentioning the PR is not mistaken for its merge. + """ + + 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]) + + # Anchored to line start: a PR body quoting "Closes #N from ..." is copied into the merge + # commit message too, and only the script's own trailer sits at the start of a line. + marker = re.compile(r"^Closes #%s from " % pr_num, re.MULTILINE) + for commit_hash in reversed(referenced_commits): + message = message_of(commit_hash) + if marker.search(message): + 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 +668,25 @@ def _do_cherry_pick(pr_num, merge_hash, pick_ref): return pick_ref, pick_hash +def default_pick_branch(branch_names, already_picked): + """Highest-ranked release branch that has not already received the change. + + `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. Falls back to the newest branch when + every known branch is accounted for, leaving the committer to type a target. + + >>> 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",)) + 'branch-4.x' + """ + remaining = [b for b in branch_names if b not in already_picked] + return remaining[0] if remaining else branch_names[0] + + 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 +1735,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,9 +1753,11 @@ 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] + # The change is already on target_ref, so default to the next branch down and mark + # target_ref as picked: defaulting to it would cherry-pick an empty commit. + default = default_pick_branch(branch_names, (target_ref,)) picked = cherry_pick( - pr_num, merge_hash, default, branch_names, target_ref, already_picked=() + pr_num, merge_hash, default, branch_names, target_ref, already_picked=(target_ref,) ) post_merge_comment(pr_num, picked) sys.exit(0) @@ -1765,33 +1837,26 @@ 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] picked = cherry_pick( pr_num, merge_hash, - default, + default_pick_branch(branch_names, tuple(merged_refs)), branch_names, 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 From 87c42a3b482ea46ec58822a7077bf4f13aa58091 Mon Sep 17 00:00:00 2001 From: Uros Bojanic <221401595+uros-b@users.noreply.github.com> Date: Tue, 4 Aug 2026 15:06:50 +0000 Subject: [PATCH 2/5] [SPARK-58561][INFRA][FOLLOWUP] Validate the merge footer and skip already-backported branches Address review feedback on the backport path: - Identify a merge commit by the footer structure `merge_pr` generates (a "Closes" paragraph followed immediately by the authors paragraph) rather than by the "Closes #N from " line alone. `merge_pr` passes the PR body through as its own commit-message paragraph, so a body quoting another PR's closing line kept that line at the start of a line and satisfied the old check. - Derive the already-picked set from the release branches that carry the merge footer, so a repeated invocation no longer defaults to a branch a previous run already backported to. `git branch --contains ` cannot see this: a cherry-pick is a new commit, and the footer is what `cherry-pick -x` copies, the same signal `dev/pr_merge_status.py` reads. - Loop in backport mode so one invocation can reach several maintenance branches, as the normal merge path already does. --- dev/merge_spark_pr.py | 134 +++++++++++++++++++++++++++++++++++++----- 1 file changed, 120 insertions(+), 14 deletions(-) diff --git a/dev/merge_spark_pr.py b/dev/merge_spark_pr.py index 43ca0aa8ea8b..4cba5b740c05 100755 --- a/dev/merge_spark_pr.py +++ b/dev/merge_spark_pr.py @@ -415,6 +415,47 @@ def get_json(url): sys.exit(-1) +def has_merge_footer(message, pr_num): + """Whether `message` carries the merge footer `merge_pr` generates for `pr_num`. + + Matching the "Closes #N from " line alone is not enough to identify a merge commit: + `merge_pr` passes the PR body through as its own `git commit -m` paragraph, so a body + that quotes another PR's closing line keeps that line at the start of a line in the + resulting message. What a body cannot fake is the footer *structure*, which `merge_pr` + always emits as a "Closes" paragraph followed immediately by the authors paragraph: + + Closes # from /. + + Authored-by: A + Signed-off-by: C + + >>> footer = "Closes #1 from a/b.\\n\\nAuthored-by: A \\nSigned-off-by: C " + >>> has_merge_footer("[SPARK-1][SQL] Title\\n\\nSome body.\\n\\n" + footer, 1) + True + >>> lead = footer.replace("Authored", "Lead-authored") + >>> has_merge_footer("[SPARK-1][SQL] Title\\n\\n" + lead, 1) + True + >>> has_merge_footer("[SPARK-1][SQL] Title\\n\\n" + footer, 2) + False + + A body quoting another PR's closing line is not mistaken for that PR's merge: + + >>> body = "Reverts:\\nCloses #1 from a/b.\\n\\nSee that commit." + >>> quoting = "[SPARK-2][SQL] Later\\n\\n%s\\n\\n%s" % (body, footer.replace("#1", "#2")) + >>> has_merge_footer(quoting, 1) + False + >>> has_merge_footer(quoting, 2) + True + """ + # \s*$ tolerates trailing whitespace; the blank line then the authors paragraph is what + # distinguishes the generated footer from the same text quoted inside a PR body. + footer = re.compile( + r"^Closes #%s from \S+\s*$\n\n(Lead-authored-by|Authored-by):" % pr_num, + re.MULTILINE, + ) + return footer.search(message) is not None + + def merge_commit_candidates(pr_events): """Split `pr_events` into (closed_commits, referenced_commits), each oldest-first. @@ -446,9 +487,8 @@ def find_merge_commit(pr_num, pr_events): 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, confirming each against the - "Closes #N from " line that `merge_pr` writes so that an unrelated commit merely - mentioning the PR is not mistaken for its merge. + 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): @@ -458,12 +498,9 @@ def message_of(commit_hash): if closed_commits: return closed_commits[-1], message_of(closed_commits[-1]) - # Anchored to line start: a PR body quoting "Closes #N from ..." is copied into the merge - # commit message too, and only the script's own trailer sits at the start of a line. - marker = re.compile(r"^Closes #%s from " % pr_num, re.MULTILINE) for commit_hash in reversed(referenced_commits): message = message_of(commit_hash) - if marker.search(message): + if has_merge_footer(message, pr_num): return commit_hash, message return None, None @@ -668,6 +705,53 @@ 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. + + A cherry-pick is a new commit, so `git branch --contains ` finds only the + branch the change was merged into; what identifies a backport is the footer, which + `cherry-pick -x` copies verbatim (the same signal `dev/pr_merge_status.py` reads). + Best-effort: this only sees branches already fetched into PUSH_REMOTE_NAME's tracking + refs, so a backport pushed from elsewhere and not yet fetched is simply not reported -- + the committer is still prompted and can type any branch. + """ + trailer = "Closes #%s from " % pr_num + try: + out = run_cmd( + [ + "git", + "log", + "--remotes=%s" % PUSH_REMOTE_NAME, + "--fixed-strings", + "--grep", + trailer, + "--format=%H", + ] + ) + except Exception as e: + print_error("Could not scan for existing backports of #%s (%s)." % (pr_num, e)) + return [] + + landed = set() + prefix = "%s/" % PUSH_REMOTE_NAME + for commit_hash in out.split(): + refs = run_cmd( + [ + "git", + "for-each-ref", + "--contains", + commit_hash, + "--format=%(refname:short)", + "refs/remotes/%s/" % PUSH_REMOTE_NAME, + ] + ) + for ref in refs.splitlines(): + if ref.startswith(prefix): + landed.add(ref[len(prefix) :]) + # 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. @@ -1753,13 +1837,35 @@ 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)) - # The change is already on target_ref, so default to the next branch down and mark - # target_ref as picked: defaulting to it would cherry-pick an empty commit. - default = default_pick_branch(branch_names, (target_ref,)) - picked = cherry_pick( - pr_num, merge_hash, default, branch_names, target_ref, already_picked=(target_ref,) - ) - 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: + picked = cherry_pick( + pr_num, + merge_hash, + default_pick_branch(branch_names, tuple(picked_refs)), + 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"]): From 0853820f582d184457fdc50f17efdd744cdf333f Mon Sep 17 00:00:00 2001 From: Uros Bojanic <221401595+uros-b@users.noreply.github.com> Date: Tue, 4 Aug 2026 16:26:00 +0000 Subject: [PATCH 3/5] [SPARK-58561][INFRA][FOLLOWUP] Identify the merge footer by position and stop when no branch remains Address the second round of review feedback: - Identify the generated footer by position rather than structure. A PR body is copied verbatim into the merge commit, so it can quote another commit's entire footer, authors paragraph included; matching the structure anywhere in the message therefore still selected the wrong commit. `merge_pr` appends the footer last, so `merge_footer_pr` reads the final "Closes" paragraph and compares its number. Cherry-pick provenance lines may follow it, but no later "Closes" paragraph can. - Validate the release-branch scan with the same matcher. `git log --grep` matches the fragment anywhere in a message, so a commit merely quoting the trailer made every containing branch look already backported; candidates are now confirmed before their branches count. - Return None from `default_pick_branch` when every known branch already has the change, and have both call sites report that instead of defaulting to a branch whose cherry-pick would be empty. --- dev/merge_spark_pr.py | 114 +++++++++++++++++++++++++++++------------- 1 file changed, 79 insertions(+), 35 deletions(-) diff --git a/dev/merge_spark_pr.py b/dev/merge_spark_pr.py index 4cba5b740c05..9f4aaf1b4ed3 100755 --- a/dev/merge_spark_pr.py +++ b/dev/merge_spark_pr.py @@ -415,45 +415,67 @@ def get_json(url): sys.exit(-1) -def has_merge_footer(message, pr_num): - """Whether `message` carries the merge footer `merge_pr` generates for `pr_num`. +def merge_footer_pr(message): + """The PR number in `message`'s generated merge footer, or None if it has none. - Matching the "Closes #N from " line alone is not enough to identify a merge commit: + A commit message cannot be searched for "Closes #N from " to identify the merge of N: `merge_pr` passes the PR body through as its own `git commit -m` paragraph, so a body - that quotes another PR's closing line keeps that line at the start of a line in the - resulting message. What a body cannot fake is the footer *structure*, which `merge_pr` - always emits as a "Closes" paragraph followed immediately by the authors paragraph: + discussing or reverting another commit can quote that commit's entire footer, structure + included. What is unforgeable is *position*: `merge_pr` appends the footer last, so the + generated one is the final "Closes" paragraph in the message. Later `cherry-pick -x` + lines may follow it, but no further "Closes" paragraph can. So read the last one and + compare its number, rather than searching for a number anywhere. + + >>> 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 + """ + # \s*$ tolerates trailing whitespace. The blank line and authors paragraph reject prose + # that merely mentions a PR; taking the LAST match rejects a body quoting a real footer. + footer = re.compile( + r"^Closes #(\d+) from \S+\s*$\n\n(?:Lead-authored-by|Authored-by):", + re.MULTILINE, + ) + matches = footer.findall(message) + return int(matches[-1]) if matches else None - Closes # from /. - Authored-by: A - Signed-off-by: C +def has_merge_footer(message, pr_num): + """Whether `message`'s generated merge footer closes `pr_num`. See `merge_footer_pr`. >>> footer = "Closes #1 from a/b.\\n\\nAuthored-by: A \\nSigned-off-by: C " - >>> has_merge_footer("[SPARK-1][SQL] Title\\n\\nSome body.\\n\\n" + footer, 1) - True - >>> lead = footer.replace("Authored", "Lead-authored") - >>> has_merge_footer("[SPARK-1][SQL] Title\\n\\n" + lead, 1) + >>> 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 body quoting another PR's closing line is not mistaken for that PR's merge: + A commit whose body quotes another PR's full footer is not taken for that PR's merge: - >>> body = "Reverts:\\nCloses #1 from a/b.\\n\\nSee that commit." - >>> quoting = "[SPARK-2][SQL] Later\\n\\n%s\\n\\n%s" % (body, footer.replace("#1", "#2")) - >>> has_merge_footer(quoting, 1) + >>> 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(quoting, 2) + >>> has_merge_footer(later, 2) True """ - # \s*$ tolerates trailing whitespace; the blank line then the authors paragraph is what - # distinguishes the generated footer from the same text quoted inside a PR body. - footer = re.compile( - r"^Closes #%s from \S+\s*$\n\n(Lead-authored-by|Authored-by):" % pr_num, - re.MULTILINE, - ) - return footer.search(message) is not None + return merge_footer_pr(message) == pr_num def merge_commit_candidates(pr_events): @@ -711,12 +733,16 @@ def branches_with_merge_footer(pr_num, branch_names): A cherry-pick is a new commit, so `git branch --contains ` finds only the branch the change was merged into; what identifies a backport is the footer, which `cherry-pick -x` copies verbatim (the same signal `dev/pr_merge_status.py` reads). + `git log --grep` only narrows the walk -- it matches the fragment anywhere in a message, + including inside a copied PR body -- so every candidate is confirmed with + `has_merge_footer` before its branches count as already backported. Best-effort: this only sees branches already fetched into PUSH_REMOTE_NAME's tracking refs, so a backport pushed from elsewhere and not yet fetched is simply not reported -- the committer is still prompted and can type any branch. """ trailer = "Closes #%s from " % pr_num try: + # %x00 delimits records so a commit message (which spans lines) stays one field. out = run_cmd( [ "git", @@ -725,7 +751,7 @@ def branches_with_merge_footer(pr_num, branch_names): "--fixed-strings", "--grep", trailer, - "--format=%H", + "--format=%H %B%x00", ] ) except Exception as e: @@ -734,7 +760,14 @@ def branches_with_merge_footer(pr_num, branch_names): landed = set() prefix = "%s/" % PUSH_REMOTE_NAME - for commit_hash in out.split(): + for record in out.split("\0"): + record = record.strip("\n") + if not record: + continue + commit_hash, _, message = record.partition(" ") + # --grep matched somewhere in the message; only the generated footer counts. + if not has_merge_footer(message, pr_num): + continue refs = run_cmd( [ "git", @@ -753,22 +786,22 @@ def branches_with_merge_footer(pr_num, branch_names): def default_pick_branch(branch_names, already_picked): - """Highest-ranked release branch that has not already received the change. + """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. Falls back to the newest branch when - every known branch is accounted for, leaving the committer to type a target. + 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",)) - '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 branch_names[0] + return remaining[0] if remaining else None def _upstream_first_sibling(target_ref, pick_ref, branch_names, already_picked): @@ -1848,10 +1881,17 @@ def main(): 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_pick_branch(branch_names, tuple(picked_refs)), + default, branch_names, target_ref, already_picked=tuple(picked_refs), @@ -1953,10 +1993,14 @@ def main(): # 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 = 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, - default_pick_branch(branch_names, tuple(merged_refs)), + default, branch_names, target_ref, already_picked=tuple(merged_refs), From 1f41a2dfc186cbfc7f3c2a455ca89dd15fb4407d Mon Sep 17 00:00:00 2001 From: Uros Bojanic <221401595+uros-b@users.noreply.github.com> Date: Tue, 4 Aug 2026 18:18:18 +0000 Subject: [PATCH 4/5] [SPARK-58561][INFRA][FOLLOWUP] Apply ruff format to the exhausted-branch message `ruff format` collapses the string and its `%` operand onto one line, which fits within the 100-character limit. Matches the sibling call site in the merge path. --- dev/merge_spark_pr.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/dev/merge_spark_pr.py b/dev/merge_spark_pr.py index 9f4aaf1b4ed3..4e6277ee09a4 100755 --- a/dev/merge_spark_pr.py +++ b/dev/merge_spark_pr.py @@ -1884,8 +1884,7 @@ def main(): 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 + "Every known release branch already contains #%s; nothing to pick." % pr_num ) break picked = cherry_pick( From 3c5f8f69396ae450ea771dd6113ef32d63806e3c Mon Sep 17 00:00:00 2001 From: Uros Bojanic <221401595+uros-b@users.noreply.github.com> Date: Wed, 5 Aug 2026 07:20:16 +0000 Subject: [PATCH 5/5] [SPARK-58561][INFRA][FOLLOWUP] Share one merge-footer reader between the committer tools `dev/merge_spark_pr.py` and `dev/pr_merge_status.py` both located trailer-bearing commits and mapped them to containing branches, but only the former validated the generated footer and only the latter refreshed tracking refs, so the two tools could disagree about where a PR landed. Extract that flow into `dev/spark_merge_footer.py` and use it from both. The module is import-only: it never exits, prints, or runs git itself, and takes a `run_git` callable so each caller keeps its own error policy -- `pr_merge_status.py` exits on a git failure, while `merge_spark_pr.py` must not abort a merge that may already have pushed. The refresh policy is stated in one place: the reader consumes local remote-tracking refs and never fetches, so a caller needing current data fetches first (as `pr_merge_status.py` does). `has_merge_footer` now accepts the PR number as an int or a string of digits, since one caller takes it from argv and the other from the GitHub API; comparing those two forms directly matched nothing. `dev/pr_merge_status.py` output is unchanged: verified byte-identical to the previous implementation across merged, backported, multi-branch, open, rejected, 404, and --all-branches cases. --- dev/merge_spark_pr.py | 120 ++--------------- dev/pr_merge_status.py | 56 +++----- dev/spark_merge_footer.py | 222 ++++++++++++++++++++++++++++++++ dev/sparktestsupport/modules.py | 1 + 4 files changed, 252 insertions(+), 147 deletions(-) create mode 100644 dev/spark_merge_footer.py diff --git a/dev/merge_spark_pr.py b/dev/merge_spark_pr.py index 4e6277ee09a4..193449564492 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,69 +420,6 @@ def get_json(url): sys.exit(-1) -def merge_footer_pr(message): - """The PR number in `message`'s generated merge footer, or None if it has none. - - A commit message cannot be searched for "Closes #N from " to identify the merge of N: - `merge_pr` passes the PR body through as its own `git commit -m` paragraph, so a body - discussing or reverting another commit can quote that commit's entire footer, structure - included. What is unforgeable is *position*: `merge_pr` appends the footer last, so the - generated one is the final "Closes" paragraph in the message. Later `cherry-pick -x` - lines may follow it, but no further "Closes" paragraph can. So read the last one and - compare its number, rather than searching for a number anywhere. - - >>> 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 - """ - # \s*$ tolerates trailing whitespace. The blank line and authors paragraph reject prose - # that merely mentions a PR; taking the LAST match rejects a body quoting a real footer. - footer = re.compile( - r"^Closes #(\d+) from \S+\s*$\n\n(?:Lead-authored-by|Authored-by):", - re.MULTILINE, - ) - matches = footer.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`. - - >>> 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, 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) == pr_num - - def merge_commit_candidates(pr_events): """Split `pr_events` into (closed_commits, referenced_commits), each oldest-first. @@ -730,57 +672,19 @@ def _do_cherry_pick(pr_num, merge_hash, pick_ref): def branches_with_merge_footer(pr_num, branch_names): """Release branches from `branch_names` that already carry `pr_num`'s merge footer. - A cherry-pick is a new commit, so `git branch --contains ` finds only the - branch the change was merged into; what identifies a backport is the footer, which - `cherry-pick -x` copies verbatim (the same signal `dev/pr_merge_status.py` reads). - `git log --grep` only narrows the walk -- it matches the fragment anywhere in a message, - including inside a copied PR body -- so every candidate is confirmed with - `has_merge_footer` before its branches count as already backported. - Best-effort: this only sees branches already fetched into PUSH_REMOTE_NAME's tracking - refs, so a backport pushed from elsewhere and not yet fetched is simply not reported -- - the committer is still prompted and can type any branch. + 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. """ - trailer = "Closes #%s from " % pr_num try: - # %x00 delimits records so a commit message (which spans lines) stays one field. - out = run_cmd( - [ - "git", - "log", - "--remotes=%s" % PUSH_REMOTE_NAME, - "--fixed-strings", - "--grep", - trailer, - "--format=%H %B%x00", - ] + 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 [] - - landed = set() - prefix = "%s/" % PUSH_REMOTE_NAME - for record in out.split("\0"): - record = record.strip("\n") - if not record: - continue - commit_hash, _, message = record.partition(" ") - # --grep matched somewhere in the message; only the generated footer counts. - if not has_merge_footer(message, pr_num): - continue - refs = run_cmd( - [ - "git", - "for-each-ref", - "--contains", - commit_hash, - "--format=%(refname:short)", - "refs/remotes/%s/" % PUSH_REMOTE_NAME, - ] - ) - for ref in refs.splitlines(): - if ref.startswith(prefix): - landed.add(ref[len(prefix) :]) # 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] diff --git a/dev/pr_merge_status.py b/dev/pr_merge_status.py index 510409f22500..044928082668 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 000000000000..2177989b2435 --- /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 79e2e5a7bc2e..8c7c0a8ec4cc 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/", ],