sarutak opened a new pull request, #60031:
URL: https://github.com/apache/spark/pull/60031
### What changes were proposed in this pull request?
`dev/merge_spark_pr.py` decides from the PR title whether a PR is a revert,
and `update_jira_for_pr` uses that decision to choose the JIRA path: a revert
offers to remove fix versions (and to reopen the ticket if it is resolved or
closed), while anything else resolves the ticket. Since SPARK-59808 added the
revert path, `main()` has decided it from the outermost wrapper only
(`title.startswith('Revert "') and title.endswith('"')`) and passed the result
as `is_revert`. So nested wrappers get the net direction wrong:
- `Revert "Revert "[SPARK-N] ...""` re-lands the change, but takes the
revert path and never resolves SPARK-N.
- `Reapply "Revert "[SPARK-N] ...""` reverts the change again, but takes the
resolve path.
This PR adds `classify_wrapped_title()`, which returns `(is_wrapped,
is_revert)` from all the `Revert "..."` / `Reapply "..."` wrappers. Each
`Revert` flips the direction, and a `Reapply`, which is git's name for a revert
of a revert, flips it twice, so a title is a revert when it has an odd number
of `Revert` wrappers. The wrappers are stripped by the loop that
`_unwrap_title()` used, now `_unwrap_wrappers()`, which also returns the
wrapper words.
- `update_jira_for_pr` now decides the direction itself with
`classify_wrapped_title()`, from the same title it already reads the ids and
skip tags from, and its `is_revert` parameter is removed. Both callers pass the
title that `main()` produced: a wrapped title as is apart from surrounding
whitespace, or an ordinary title normalized by `Title.parse()`, which starts
with a bracket tag and so is never wrapped.
- `main()` uses `is_wrapped` to keep a wrapped title as is (apart from
surrounding whitespace, see below), replacing its own `startswith`/`endswith`
checks for `Revert` and `Reapply`.
Two edge cases change along with it, both to match `_unwrap_title()`:
- A title of just `Revert "` (or `Reapply "`) is no longer treated as
wrapped, so `main()` parses it as an ordinary title and fails with `Malformed
PR title` before merging.
- Surrounding whitespace is ignored, so `' Revert "[SPARK-N] ..." '` is a
revert. Previously it was not wrapped, and `Title.parse()` rejected it.
`main()` now strips the PR title, so such a title is also committed without the
whitespace, and the stripped title is printed as `Normalized title`.
This is the follow-up mentioned in the Out of scope note of #59708.
### Why are the changes needed?
A re-land titled `Revert "Revert "[SPARK-N] ...""` should resolve SPARK-N
again, but the script offers to remove fix versions instead, and the committer
has to notice and fix the ticket by hand. A reapply of a revert has the
opposite problem. master's history has 10 subjects with two `Revert` wrappers
(`Revert "Revert "...""`) and none with three, the latest ``Revert "Revert
"[SPARK-50227][CONNECT][PYTHON] Upgrade buf plugins to `v28.3`""`` (2024-11).
None of them was merged with this script, and the revert path only exists since
SPARK-59808, so the bug has not been hit yet; it is hit the first time a
committer merges such a PR.
### Does this PR introduce _any_ user-facing change?
No. This only affects the committer-side merge script.
### How was this patch tested?
- Doctests for `classify_wrapped_title()` covering a plain title, `Revert`,
`Reapply`, all four two-level nestings, two three-level nestings, surrounding
whitespace, and the bare `Revert "` and `Reapply "`, and for
`_unwrap_wrappers()`. The `update_jira_for_revert` doctest no longer passes
`is_revert`, so it now exercises the direction decision; it also checks that
`Revert "Revert "[SPARK-1234][SQL] Fix""` resolves SPARK-1234 and `Reapply
"Revert "[SPARK-1234][SQL] Fix""` reverts it. The doctests fail against mutants
that decide by the outermost wrapper only (in `classify_wrapped_title()` or in
`update_jira_for_pr`), never take the revert path, count a `Reapply` as one
revert, do not treat `Reapply` as a wrapper, treat a bare `Revert "` as a
revert or a bare `Reapply "` as wrapped, or do not strip whitespace. `main()`
itself has no test, so its `is_wrapped` check and its title stripping are not
covered. All doctests pass (`python3 -m doctest dev/merge_spark_pr.py`), and
`ruff chec
k` / `ruff format --check` pass.
- Replayed both rules over all 50,802 commit subjects of master at
1dc0839ca29. The old and new rules find the same 618 wrapped subjects (the
symmetric difference is empty, and no subject is a bare `Revert "` or `Reapply
"`). The only subjects whose direction changes are the 10 `Revert "Revert
"...""` re-lands, which now resolve.
### Was this patch authored or co-authored using generative AI tooling?
Generated-by: Kiro CLI
--
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]
