cloud-fan commented on code in PR #57764:
URL: https://github.com/apache/spark/pull/57764#discussion_r3719171555
##########
dev/pr_merge_status.py:
##########
@@ -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.
Review Comment:
Describe this as a best-effort refresh rather than saying the refs must be
current. `fetch_branches` uses `check=False` and continues after warning that a
failed fetch may leave the remote-tracking refs stale.
##########
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 #<pr> from <author>/<branch>.
+
+ Authored-by: A <[email protected]>
+ Signed-off-by: C <[email protected]>
+
+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 <merge_hash>` 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
Review Comment:
Narrow `never exits` to imported use. This file has a supported `__main__`
doctest path that calls `sys.exit(-1)` on failure, so the module as a whole is
not import-only and does exit when executed directly.
##########
dev/merge_spark_pr.py:
##########
@@ -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
Review Comment:
```suggestion
# If the PR is still open (reopened), we shall not cherry-pick it but
perform the normal
```
##########
dev/merge_spark_pr.py:
##########
@@ -615,6 +727,83 @@ def _do_cherry_pick(pr_num, merge_hash, pick_ref):
return pick_ref, pick_hash
+def branches_with_merge_footer(pr_num, branch_names):
Review Comment:
Confirmed: both tools now share the validated footer/branch reader, while
each caller keeps its own refresh and failure policy. This addresses the
consolidation concern.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
