dongjoon-hyun commented on code in PR #60031:
URL: https://github.com/apache/spark/pull/60031#discussion_r4472703355
##########
dev/merge_spark_pr.py:
##########
@@ -2658,61 +2876,7 @@ 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 = prepare_merge_title(title)
Review Comment:
**[22] `prepare_merge_title()` still runs, and prompts, on a backport.**
Thanks for the fix in [13]
([reply](https://github.com/apache/spark/pull/60031#discussion_r4471182607)). I
agree that a `Title.parse()` failure here predates this PR. What is new is
that, when the commit has the merge footer, the JIRA update now reads the ids,
the direction, and the components from the subject. So on a backport, the
result of this call only feeds the comparison in `backport_jira_title()` and
the no-footer fallback:
- A PR without a primary component, e.g. `[SPARK-1][TESTS] Fix`, still asks
for components on every backport run. On master that answer went to
`update_jira_for_pr` as `title_components`; now it is dropped.
- A current title that `Title.parse()` rejects, e.g.
`[SPARK-N][FOLLOWUP][MINOR] Fix` from an older merge, or a title edited after
the merge, still aborts a backport whose subject the JIRA update would have
used.
Could the backport path skip `prepare_merge_title()` when
`has_merge_footer()` holds, and run it only for the no-footer fallback?
##########
dev/merge_spark_pr.py:
##########
@@ -670,6 +670,49 @@ def message_of(commit_hash):
return None, None
+def backport_jira_title(pr_num, message: str, title: str) -> str:
+ """Return the title that the JIRA update of a backport run reads.
+
+ The JIRA update follows what was committed, so it reads the subject of the
merge commit
+ ``message`` rather than the current PR title, which may have been edited
since the
+ merge. A changed subject is printed next to the PR title. The subject is
used only when
+ the merge footer of ``message`` closes ``pr_num``, i.e. when ``merge_pr``
made the
+ commit for this PR. Otherwise the PR title is used as before, e.g. for a
commit made
+ with the GitHub squash button, whose subject ends with ``(#N)``.
+
+ >>> footer = "\\n\\nBody\\n\\nCloses #1 from a/b.\\n\\nAuthored-by: A
<[email protected]>"
+ >>> backport_jira_title(1, 'Revert "[SPARK-1][SQL] Fix"' + footer,
+ ... 'Revert "[SPARK-1][SQL] Fix"')
+ 'Revert "[SPARK-1][SQL] Fix"'
+ >>> backport_jira_title(1, 'Revert "Revert "[SPARK-1][SQL] Fix"" \\r' +
footer,
+ ... 'Revert "[SPARK-1][SQL] Fix"')
+ ... # doctest: +NORMALIZE_WHITESPACE
+ The JIRA update uses the merge commit subject: Revert "Revert
"[SPARK-1][SQL] Fix""
+ (current PR title: Revert "[SPARK-1][SQL] Fix"). If the PR title is the
right one, update
+ JIRA by hand, answering N if asked to update it.
+ 'Revert "Revert "[SPARK-1][SQL] Fix""'
+
+ A commit without this PR's merge footer, such as a squash-button merge or
a commit of
+ another PR, leaves the PR title:
+
+ >>> backport_jira_title(1, 'Revert "[SPARK-1][SQL] Fix" (#1)\\n\\nBody',
+ ... 'Revert "[SPARK-1][SQL] Fix"')
+ 'Revert "[SPARK-1][SQL] Fix"'
+ >>> backport_jira_title("1", "[SPARK-2][SQL] Other" + footer.replace("#1",
"#2"),
+ ... "[SPARK-1][SQL] Fix")
+ '[SPARK-1][SQL] Fix'
+ """
+ subject = message.split("\n", 1)[0].strip()
+ if subject == title or not has_merge_footer(message, pr_num):
Review Comment:
**[23] The subject check compares against the normalized title.**
`title` here is the result of `prepare_merge_title()` in this run, not the
GitHub PR title: its components are normalized, and include any entered at this
run's prompt. So the notice fires when only the normalization differs from the
one at merge time. For example, `65396d2f8d2` (`[SPARK-57923][SQL][SDP][TESTS]
Add CREATE STREAMING TABLE parser test coverage`, #56985) has the merge footer,
and its PR title is the same as the subject. But `prepare_merge_title()` now
turns `[TESTS]` into `[TEST]`, so a backport of it prints "update JIRA by hand"
although the ids, the direction, and the skip tags are the same.
Likewise, a PR merged as `[SPARK-1][CORE] Fix` after `CORE` was entered at
the merge prompt shows `(current PR title: [SPARK-1][SQL][CORE] Fix)` if
`SQL,CORE` is entered at the backport prompt, a title the PR never had.
Could this compare against `pr["title"].strip()` (which [22] would also
give), or only what the JIRA update reads: the leading ids,
`_is_net_revert(words)`, and the skip tags?
##########
dev/merge_spark_pr.py:
##########
@@ -670,6 +670,49 @@ def message_of(commit_hash):
return None, None
+def backport_jira_title(pr_num, message: str, title: str) -> str:
+ """Return the title that the JIRA update of a backport run reads.
+
+ The JIRA update follows what was committed, so it reads the subject of the
merge commit
+ ``message`` rather than the current PR title, which may have been edited
since the
+ merge. A changed subject is printed next to the PR title. The subject is
used only when
+ the merge footer of ``message`` closes ``pr_num``, i.e. when ``merge_pr``
made the
+ commit for this PR. Otherwise the PR title is used as before, e.g. for a
commit made
+ with the GitHub squash button, whose subject ends with ``(#N)``.
+
+ >>> footer = "\\n\\nBody\\n\\nCloses #1 from a/b.\\n\\nAuthored-by: A
<[email protected]>"
+ >>> backport_jira_title(1, 'Revert "[SPARK-1][SQL] Fix"' + footer,
+ ... 'Revert "[SPARK-1][SQL] Fix"')
+ 'Revert "[SPARK-1][SQL] Fix"'
+ >>> backport_jira_title(1, 'Revert "Revert "[SPARK-1][SQL] Fix"" \\r' +
footer,
+ ... 'Revert "[SPARK-1][SQL] Fix"')
+ ... # doctest: +NORMALIZE_WHITESPACE
+ The JIRA update uses the merge commit subject: Revert "Revert
"[SPARK-1][SQL] Fix""
+ (current PR title: Revert "[SPARK-1][SQL] Fix"). If the PR title is the
right one, update
+ JIRA by hand, answering N if asked to update it.
+ 'Revert "Revert "[SPARK-1][SQL] Fix""'
+
+ A commit without this PR's merge footer, such as a squash-button merge or
a commit of
+ another PR, leaves the PR title:
+
+ >>> backport_jira_title(1, 'Revert "[SPARK-1][SQL] Fix" (#1)\\n\\nBody',
+ ... 'Revert "[SPARK-1][SQL] Fix"')
+ 'Revert "[SPARK-1][SQL] Fix"'
+ >>> backport_jira_title("1", "[SPARK-2][SQL] Other" + footer.replace("#1",
"#2"),
+ ... "[SPARK-1][SQL] Fix")
+ '[SPARK-1][SQL] Fix'
+ """
+ subject = message.split("\n", 1)[0].strip()
+ if subject == title or not has_merge_footer(message, pr_num):
+ return title
+ print(
Review Comment:
**[24] The mismatch notice comes long before the JIRA prompt, and answering
N exits with -1.**
This prints right after `Found commit ...`, before the cherry-pick loop, so
the pick prompts, and maybe a conflict resolution, come between it and the
`Would you like to update an associated JIRA?` prompt it refers to. By then the
notice has likely scrolled away.
And if the committer answers N there, as the notice says, `continue_maybe()`
calls `fail("Okay, exiting")` from the `finally` at L2991. That prints a red
error and exits with -1 after a backport that succeeded, instead of reaching
`sys.exit(0)`. N has always worked that way, but this is the first message that
tells the committer to answer it.
Could the notice be printed just before `update_jira_for_pr` at L2991, and
either say that N ends the run there or come with a skip that does not fail?
##########
dev/merge_spark_pr.py:
##########
@@ -2823,9 +2988,11 @@ def main():
if picked_commits:
post_merge_comment(pr_num, picked_commits)
# Reconcile JIRA with every branch that contains the merge or
revert.
- update_jira_for_pr(pr_num, title, picked_refs, title_components,
is_revert=is_revert_pr)
+ update_jira_for_pr(pr_num, jira_title, picked_refs)
sys.exit(0)
+ check_wrapped_title(title)
Review Comment:
**[26] Minor: `check_wrapped_title()` runs after the body prompt and two API
calls.**
For an open PR titled `Revert ""`, the committer first sees the stripped PR
body and answers `Would you like to use the modified body? (y/N)`, and the
script fetches `users/<login>` and `pulls/<n>/commits`. Only then does this
fail with `Empty title inside the Revert/Reapply wrappers`. The `Malformed PR
title` failure next to it, in `prepare_merge_title()` at L2879, comes before
all of that.
Could the `find_merge_commit()` lookup move up next to L2879, so that
`check_wrapped_title()` runs right there whenever there is no merge commit?
##########
dev/merge_spark_pr.py:
##########
@@ -2307,17 +2393,53 @@ def _unwrap_title(title: str) -> str:
>>> _unwrap_title('Revert "')
'Revert "'
"""
- title = title.strip()
- while m := _WRAPPER_RE.fullmatch(title):
- title = m.group(1).strip()
- return title
+ return _unwrap_wrappers(title)[0]
Review Comment:
**[28] Nit: `_unwrap_title()` keeps the stripping examples.**
After [21], the description of the stripping rules lives in
`_unwrap_wrappers()`, and this is a one-line delegate. But the six examples
that pin those rules (whitespace in each layer, a bare `Revert "`) are still
here, while `_unwrap_wrappers()` has two. Its only callers are
`leading_jira_ids()` and `leading_title_tags()`.
Could the examples move to `_unwrap_wrappers()`, next to the description,
with the two callers calling `_unwrap_wrappers(title)[0]` directly?
##########
dev/merge_spark_pr.py:
##########
@@ -1971,10 +2026,12 @@ def update_jira_for_pr(pr_num, title, merge_branches,
title_components, is_rever
return
skip_jira_title_tags = ("MINOR", "TRIVIAL", "FOLLOWUP")
- jira_ids = leading_jira_ids(title)
- # Revert/Reapply PRs keep their title verbatim, so the tags are read
inside the wrappers,
+ # Unwrap once; the innermost title has no wrappers left for the helpers
below to strip.
Review Comment:
**[27] Nit: "Unwrap once" does not match what the helpers do.**
`leading_jira_ids()` and `leading_title_tags()` still call `_unwrap_title()`
on their argument, so the title is unwrapped three times here, and passing
`title` instead of `inner` gives the same result. That is the equivalent mutant
in the PR description. Your reply to [19]
([thread](https://github.com/apache/spark/pull/60031#discussion_r4471186106))
describes it the same way as this comment.
Could the comment say that the helpers unwrap again, or could this keep only
`words` and pass `title` to both helpers?
##########
dev/merge_spark_pr.py:
##########
@@ -2446,6 +2568,102 @@ def prompt_for_components():
return components
+def prepare_merge_title(raw_title: str) -> str:
+ """Return the title to merge for the PR title ``raw_title``.
+
+ 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
(see
+ :func:`check_wrapped_title` for what the merge path checks in it). 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, and written back into the title, from which
:func:`update_jira_for_pr` reads
+ them. A title that changes is printed.
+ """
+ title = raw_title.strip()
+ if not _unwrap_wrappers(title)[1]:
+ # 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 = str(parsed)
+ if title != raw_title:
+ print("Normalized title: %s" % title)
+ return title
+
+
+def check_wrapped_title(title: str) -> None:
+ """Check the ``Revert "..."`` / ``Reapply "..."`` wrappers of a PR title
before merging.
+
+ An empty innermost title fails, since the commit subject would say
nothing. An
+ innermost title that still starts with ``Revert`` or ``Reapply``, or a
form of either
+ such as ``Reverting`` (see ``_WRAPPER_WORD_RE``), is warned about and
needs a
+ confirmation. Such a title may have a dropped quote, e.g. ``Revert "Revert
"[SPARK-N]
+ ..."``, or wrap a subject that is not a wrapper, e.g. ``Revert to using
...``. The
+ shape does not tell which: the revert of an existing subject that GitHub
truncated
+ without its closing quote looks like a dropped quote. Either way, only the
outer
+ wrappers decide the direction, and no JIRA id or skip tag is read from it.
Other
+ titles pass silently.
+ """
+ inner, words = _unwrap_wrappers(title)
+ if not words:
+ return
+ if not inner:
+ fail("Empty title inside the Revert/Reapply wrappers of the PR title:
%s" % title)
+ if _WRAPPER_WORD_RE.match(inner):
Review Comment:
**[25] Some malformed nestings still pass `check_wrapped_title()` silently.**
Following up on [14]
([thread](https://github.com/apache/spark/pull/60031#discussion_r4470227669)):
the word check now catches `Reapplied` and `Re-apply`, but the innermost title
is still checked only for those spellings.
- With a doubled quote, `Revert ""[SPARK-1][SQL] X""` unwraps to
`"[SPARK-1][SQL] X"` with words `['Revert']`.
- With a misspelled inner wrapper, `Revert "Reaply "[SPARK-1][SQL] X""`
unwraps to `Reaply "[SPARK-1][SQL] X"`.
Neither matches `_WRAPPER_WORD_RE`, so there is no warning, and
`leading_jira_ids()` returns `[]`, since `_iter_bracket_tags()` only matches at
position 0. So the JIRA update takes the revert path with an empty id prompt,
the case this check is for. I checked these by hand with the regexes, not by
running the script.
Could the check also warn when the innermost title starts with `"`, or with
a single word followed by ` "` as a wrapper would be?
--
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]
