uros-b commented on code in PR #57764:
URL: https://github.com/apache/spark/pull/57764#discussion_r3713762521
##########
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)
Review Comment:
Good catch, and the reasoning in my comment was simply wrong -- I claimed
line anchoring distinguished the generated trailer from copied body text, and
it does not. `merge_pr` passes the PR body through as its own `-m` paragraph,
so a body line beginning `Closes #<n> from ` stays at column 0. Reproduced:
```
old regex matched #57713: True <- false positive
new matcher matches #57713: False
new matcher matches real #58000: True
```
Fixed in 87c42a3 by validating the footer *structure* instead of a single
line. `merge_pr` always emits the `Closes` paragraph immediately followed by
the authors paragraph, which prose inside a body cannot reproduce:
```
Closes #<pr> from <author>/<branch>.
Authored-by: A <[email protected]>
Signed-off-by: C <[email protected]>
```
I checked that this really is invariant before keying on it: of the last 600
commits on master, **592 carry a `Closes` trailer and all 592 are immediately
followed by the authors paragraph** (`Authored-by:` or `Lead-authored-by:`).
After the change all 592 are still identified as merges, so the stricter check
costs no recall.
Regression coverage added as doctests on `has_merge_footer`, including
exactly this case -- a commit whose body quotes `Closes #1 from a/b.` while its
own footer closes #2: the matcher returns `False` for 1 and `True` for 2.
--
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]
