sarutak commented on code in PR #60031:
URL: https://github.com/apache/spark/pull/60031#discussion_r4471182082
##########
dev/merge_spark_pr.py:
##########
@@ -2446,6 +2636,79 @@ def prompt_for_components():
return components
+def prepare_merge_title(raw_title: str) -> tuple[str, list[str], bool]:
+ """Return ``(title, title_components, is_revert)`` for the title of the PR
to merge.
+
+ Surrounding whitespace is stripped. A ``Revert "..."`` / ``Reapply "..."``
title is
+ otherwise kept as is: the wrapped title is an existing commit subject,
which may
+ predate the current title rules, so it is not parsed and has no
components. It is
+ rejected when the innermost title still starts with ``Revert`` or
``Reapply``, e.g.
+ with a quote dropped: that wrapper would not be counted when deciding the
direction
+ (see :func:`_is_net_revert`), and its JIRA id and skip tags would not be
found
+ either. Any other title is parsed with :meth:`Title.parse`, which
+ fails on a malformed title, and its component tags are normalized, with a
prompt for
+ a primary component when there is none. A title that changes is printed.
+ """
+ title = raw_title.strip()
+ inner, words = _unwrap_wrappers(title)
+ if words:
+ if _WRAPPER_WORD_RE.match(inner):
Review Comment:
Agreed, it is now a warning with a confirmation, on the merge path only (see
[13]). `check_wrapped_title()` prints the warning and asks `Merge with this
title?` (`continue_maybe`). The warning names the direction the JIRA update
takes from the outer wrappers. It asks to retitle, with the `Reapply "[SPARK-N]
..."` example from [9], only if a quote was dropped or the innermost title is
the subject of a revert commit, e.g. `Revert [SPARK-N] ...`. If the title only
starts with the word, as in `Revert to using ...`, it says to keep the title
and enter the JIRA id at the prompt or leave it blank. For `Revert "Revert to
using comma-delimited split"` the direction it names is a revert, which is
right for that title.
I did not narrow it to the dropped-quote case. Of the 56 master subjects
whose revert is caught, 16 look like a dropped quote, mostly subjects that are
missing their closing quote, some truncated by GitHub, e.g. `Revert
"[SPARK-54922][PYTHON] Remove ignore[arg-type] for np.set_prin…`. Reverting
most of those is a real re-land, so the narrowed check would still block them.
The other 40 would pass silently, e.g. the revert of `Revert [SPARK-54758][SQL]
...`, which re-lands the change but takes the revert path with no JIRA id. With
a confirmation, a title like yours costs one prompt.
This updates my earlier replies: for [1], the check is now a warning, in
`check_wrapped_title()`, and does not run on a backport; for [5], the
unbalanced-title doctests moved to `check_wrapped_title`, and
`prepare_merge_title()` now returns only the title (see [17]); for [9], the
message is the new warning; and for [11], `prepare_merge_title()` no longer
decides the direction; `check_wrapped_title()` passes the words it already has
to `_is_net_revert`, and `update_jira_for_pr` unwraps its title itself, since
after [17] it takes only the title.
##########
dev/merge_spark_pr.py:
##########
@@ -2658,61 +2921,9 @@ def main():
if pr.get("draft", False) or "[WIP]" in title or "[DO-NOT-MERGE]" in title:
fail("Cannot merge a draft PR #%s: %s" % (pr_num, title))
- # e.g. 'Revert "[SPARK-56357][BUILD] Upgrade sbt to 1.12.8"'
- is_revert_pr = title.startswith('Revert "') and title.endswith('"')
- # e.g. 'Reapply "[SPARK-56357][BUILD] Upgrade sbt to 1.12.8"'
- is_reapply_pr = title.startswith('Reapply "') and title.endswith('"')
-
- # Normalized PR-title component tags, used later to reconcile JIRA
components. Empty for
- # Revert/Reapply PRs, whose titles are kept verbatim and not parsed for
components.
- title_components: List[str] = []
-
- # Revert and Reapply PRs keep their title verbatim.
- if not (is_revert_pr or is_reapply_pr):
- # Parse; fail on a malformed title.
- try:
- parsed = Title.parse(title)
- except ValueError as e:
- fail("Malformed PR title: %s" % e)
-
- # Normalize component tags via the registry and track primary.
- components = []
- has_primary = False
- for tag in parsed.components:
- c = Component.find(tag)
- if c is not None and c.primary:
- has_primary = True
- components.append(Component.normalize(tag))
- if not has_primary:
- new_tags = prompt_for_components()
- components = new_tags + components
-
- # Deduplicate tags in insertion order.
- components = list(dict.fromkeys(components))
-
- # Move version tags (e.g. [4.X], [4.2]) to the head of components.
- versions = [t for t in components if _VERSION_TAG_RE.match(t)]
- if versions:
- others = [t for t in components if not _VERSION_TAG_RE.match(t)]
- components = versions + others
-
- # Move FOLLOWUP to the last tag.
- non_followup = [t for t in components if t != "FOLLOWUP"]
- if len(non_followup) < len(components):
- components = non_followup + ["FOLLOWUP"]
-
- # Warn about tags that are neither known components nor version tags.
- unknown = [
- t for t in components if Component.find(t) is None and not
_VERSION_TAG_RE.match(t)
- ]
- if unknown:
- print_error("Title has unknown tag(s): %s" % ", ".join("[%s]" % t
for t in unknown))
-
- parsed.components = components
- title_components = list(parsed.components)
- title = str(parsed)
- if title != pr["title"]:
- print("Normalized title: %s" % title)
+ # title_components are the normalized PR-title component tags, used later
to reconcile
+ # JIRA components.
+ title, title_components, is_revert_pr = prepare_merge_title(title)
Review Comment:
Done, both. On a backport, `backport_jira_title()` reads the subject of the
merge commit, the title `merge_pr` committed, and `main()` passes it to
`update_jira_for_pr`, so the direction, the ids, and the skip tags follow what
was committed. `update_jira_for_pr` now reads the components from its title too
(see [17]), so on a backport they come from the same title as the ids. The
subject is used only when the commit has this PR's merge footer. A commit made
with the GitHub squash button has no footer, and its subject ends with `(#N)`,
which would hide the wrappers, and with them the id of a Revert/Reapply title
(6 master subjects end that way). For such a commit the PR title is used, as
before.
The trade-off is a PR title corrected after the merge, e.g. a wrong SPARK id
fixed in the title: the backport now uses the committed subject instead. The
line it prints shows both, e.g. `The JIRA update uses the merge commit subject:
... (current PR title: ...). If the PR title is the right one, update JIRA by
hand, answering N if asked to update it.`
`check_wrapped_title()` runs only on the merge path, after the backport
branch, so a backport never stops on it. `prepare_merge_title()` still runs
before the backport check, as the title handling did before this PR. So a
current title that `Title.parse()` rejects still stops a backport, as before; I
left that as is.
##########
dev/merge_spark_pr.py:
##########
@@ -2137,8 +2268,12 @@ def normalize(cls, token):
_LEADING_TAGS = frozenset({"MINOR", "TRIVIAL"})
# A SPARK id that opens a title without brackets, e.g. the old subject
"SPARK-49004".
_BARE_SPARK_ID_RE = re.compile(r"SPARK-[0-9]+\b", re.IGNORECASE | re.ASCII)
-# A Revert "..." / Reapply "..." wrapper, which main() keeps verbatim in the
PR title.
-_WRAPPER_RE = re.compile(r'(?:Revert|Reapply) "(.*)"', re.DOTALL)
+# A Revert "..." / Reapply "..." wrapper, which prepare_merge_title() keeps in
the PR title.
+_WRAPPER_RE = re.compile(r'(Revert|Reapply) "(.*)"', re.DOTALL)
+# Revert or Reapply, or a form of either such as Reverting, still opening a
title after
+# unwrapping, e.g. 'Revert [SPARK-N] ...'. Case is ignored, as older subjects
include
+# 'revert [SPARK-N] ...' and 'Reverting ...'.
+_WRAPPER_WORD_RE = re.compile(r"Revert|Reapply", re.IGNORECASE)
Review Comment:
Done, with your regex: `_WRAPPER_WORD_RE` is now `Revert|Re-?appl`,
case-insensitive. The `check_wrapped_title` doctest covers `Reapplied` and
`Re-apply`. No master subject opens with either form, so the history counts do
not change.
--
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]
