uros-b commented on code in PR #57764:
URL: https://github.com/apache/spark/pull/57764#discussion_r3718706660
##########
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:
Done in 3c5f8f6 -- extracted `dev/spark_merge_footer.py` and refactored both
tools onto it.
The module is deliberately import-only: it never exits, prints, or runs git
itself, and takes a `run_git` callable so each caller keeps its own error
policy. That mattered more than it looks -- `pr_merge_status.py` calls
`sys.exit()` from inside its `git()` helper, which is right for a read-only CLI
but would be wrong in the merge script, where a transient git failure must not
abort a merge that may already have pushed. Verified both policies hold:
```
pr_merge_status.py : exits on git failure (unchanged)
merge_spark_pr.py : warns, returns [], merge continues
```
The refresh policy you asked for is now stated in one place, in the module
docstring: the reader consumes local remote-tracking refs and never fetches, so
a caller that needs current data fetches first (`pr_merge_status.py` does, via
`fetch_branches`) and a caller that must not touch the network mid-run accepts
that an unfetched branch goes unreported.
Extracting this surfaced a real latent bug, which is a good argument for
your point: `merge_footer_pr` returns an `int`, but `pr_merge_status.py`
carries the PR number as a **string** from argv, so `==` never matched and
every PR looked unmerged. `has_merge_footer` now normalizes with `int(pr_num)`
and has a doctest for the string form. Had the two implementations stayed
separate, this class of divergence would have been exactly what the duplication
hid.
`dev/pr_merge_status.py` output is unchanged -- byte-identical to the
pre-refactor implementation across every case I could construct:
| case | PR | result |
|---|---|---|
| merged to master only | #57696 | identical |
| merged + backported | #57745 | identical |
| merged to `branch-4.x` only | #57713 | identical |
| multi-branch | #56356 | identical |
| `--all-branches` | #57713, #56356 | identical |
| still open | #57764 | identical |
| closed without merging | #57743 | identical |
| unknown PR (404) | #99999999 | identical |
Also registered the new file in `dev/sparktestsupport/modules.py` next to
its siblings, and its doctests run via `python3 dev/spark_merge_footer.py` (20
cases) as well as being importable for the merge script's startup
`doctest.testmod()`.
Remaining divergence, intentional and now documented rather than accidental:
`pr_merge_status.py` filters to relevant majors for display, while the merge
script filters to its known release-branch list. Both consume the same
`{branch: commit}` mapping.
--
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]
