uros-b commented on code in PR #57764:
URL: https://github.com/apache/spark/pull/57764#discussion_r3719293523
##########
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:
Fixed in 5a8ae27. You are right that the comment contradicted the code --
`fetch_branches` passes `check=False` and warns-and-continues, so the refs are
refreshed best-effort, not guaranteed current. Reworded to say so and to carry
the same consequence the fetch warning already states (a recently merged PR
could look unmerged).
##########
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:
Fixed in 5a8ae27. Correct -- the file has a `__main__` doctest path that
calls `sys.exit(-1)`, so "import-only: it never exits" was wrong as written.
Now scoped to imported use, with the direct-execution behavior stated
explicitly:
```
When imported, nothing here exits, prints, or runs git: callers pass a
`run_git` callable and
so keep their own error-handling policy ... (Running this file directly
executes its doctests
and exits nonzero if any fail.)
```
##########
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:
Applied in 5a8ae27. For the record this wording is pre-existing on master,
not introduced here -- but this PR moves the line, so it is fair game to fix
while touching it.
--
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]
