sarutak commented on code in PR #60031:
URL: https://github.com/apache/spark/pull/60031#discussion_r4480577157
##########
dev/merge_spark_pr.py:
##########
@@ -670,6 +670,32 @@ def message_of(commit_hash):
return None, None
+def committed_title(pr_num: Union[int, str], message: Optional[str]) ->
Optional[str]:
+ """Return the subject of the merge commit ``message`` if ``merge_pr`` made
it for this PR.
+
+ That is the title that was committed, and a backport run reads it for the
JIRA update
+ instead of the current PR title, which may have been edited since the
merge. It is
+ used only when the merge footer of ``message`` closes ``pr_num``;
otherwise this
+ returns None, and the PR title is used as before, e.g. for a commit made
with the GitHub
+ squash button, whose subject ends with ``(#N)``, or a commit of another PR.
+
+ >>> footer = "\\n\\nBody\\n\\nCloses #1 from a/b.\\n\\nAuthored-by: A
<[email protected]>"
+ >>> committed_title(1, 'Revert "Revert "[SPARK-1][SQL] Fix"" \\r' + footer)
+ 'Revert "Revert "[SPARK-1][SQL] Fix""'
+ >>> committed_title("1", "[SPARK-1][SQL] Fix" + footer)
+ '[SPARK-1][SQL] Fix'
+ >>> committed_title(1, 'Revert "[SPARK-1][SQL] Fix" (#1)\\n\\nBody') is
None
+ True
+ >>> committed_title(1, "[SPARK-2][SQL] Other" + footer.replace("#1",
"#2")) is None
+ True
+ >>> committed_title(1, None) is None
+ True
+ """
+ if message is None or not has_merge_footer(message, pr_num):
Review Comment:
Thanks. I checked the assumption in the Risks section, and it does not hold:
GitHub links a squash-button merge commit to the PR's `merged` event, and the
PR's `closed` event has no commit, e.g. #52460 (6bae835ccbc), #47309, and
#43188. `merge_commit_candidates()` reads only `closed` and `referenced`
events, and the squash commit has no merge footer, so `find_merge_commit()`
returns `(None, None)` for such a PR, on master and with this PR; I ran it on
the real events of #52460. A squash-merged PR therefore never reaches the
backport path; a run on it takes the merge path, as on master. So there is no
squash-button backport for `committed_title()` to handle, and I left it as is.
`find_merge_commit()` now has a doctest with these events: `merged` with the
commit, `closed` without it, and a `referenced` event for the commit, as #47309
has. Making such PRs backportable would mean reading the `merged` event in
`find_merge_commit()`, which is outside this PR.
The docstrings and doctests were wrong about this, though: they used a
squash-button message as the example of a backport commit without the merge
footer. `committed_title()` now says why such a commit does not get there, and
the no-footer examples in `title_for_run()`, `jira_title_for_backport()`, and
the new `main()` doctest use another PR's merge commit linked to the `closed`
event. This also corrects my replies to [13] and [22], which named a
squash-button commit as the fallback case, and the description.
##########
dev/merge_spark_pr.py:
##########
@@ -2658,61 +3091,25 @@ 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)
+ # Merged pull requests don't appear as merged in the GitHub API;
+ # Instead, they're closed by committers.
+ # A PR might have multiple merge commits, if it's reopened and merged
again. We shall
+ # cherry-pick PRs in closed state with the latest merge hash.
+ # If the PR is still open (reopened), we shall not cherry-pick it but
perform the normal
+ # merge as it could have been reverted earlier.
+ merge_hash, message = (None, None)
+ if pr["state"] == "closed":
+ merge_hash, message = find_merge_commit(pr_num, pr_events)
+ if merge_hash is not None:
+ print("Pull request %s has already been merged, assuming you want to
backport" % pr_num)
+ commit_is_downloaded = (
+ git.run(["git", "rev-parse", "--quiet", "--verify", "%s^{commit}"
% merge_hash]).strip()
+ != ""
+ )
+ if not commit_is_downloaded:
+ fail("Couldn't find any merge commit for #%s, you may need to
update HEAD." % pr_num)
+ print("Found commit %s:\n%s" % (merge_hash, message))
+ title = title_for_run(pr_num, title, message)
Review Comment:
Done. A new `main_titles` doctest drives `main()` with every GitHub, git,
and JIRA call mocked. It checks the title that a merge commits, checks against
JIRA, and passes to `update_jira_for_pr`, with a component entered at the
prompt; and the title that a backport passes to `update_jira_for_pr`, for a
commit with the PR's merge footer whose PR title is the same, differs only in
how a tag is spelled, also lacks the component entered at the merge prompt (no
prompt), or was edited to another id, answered N and y, and for another PR's
commit. Each run also returns its exit code, and prints a line for each
cherry-pick, the pick-again prompt, and the JIRA update, so the doctest pins
that the notice and the question come after the cherry-picks, right before the
JIRA update.
The mutants you named fail it: passing `None` instead of `message` to
`title_for_run()`, dropping the `jira_title_for_backport()` call, and swapping
the two title arguments of `main()`'s call to it. So do the four `main()`
mutants the description listed as surviving: passing the raw PR title to
`update_jira_for_pr` on a merge or on a backport, not offering the PR title,
and offering it compared with the subject itself. Four new ones fail it too:
committing or checking the raw PR title on a merge, and calling
`jira_title_for_backport()` before the cherry-picks or inside their loop. The
only surviving mutant is an equivalent one in `committed_title()`. The doctest
sets `DRY_RUN_ENV` to False, so it also passes with `DRY_RUN` exported.
This updates my replies to [5], [16], and [22], which said that no doctest
reaches `main()` and that only the uncommitted mocked run covers its call sites.
##########
dev/merge_spark_pr.py:
##########
@@ -2012,6 +2088,150 @@ def update_jira_for_pr(pr_num, title, merge_branches,
title_components, is_rever
)
+def _bracketed(tags):
+ """Format tags as in a title, e.g. ``[MINOR], [FOLLOWUP]``, for
messages."""
+ return ", ".join("[%s]" % tag for tag in tags)
+
+
+def committed_title_notice(subject: str, pr_title: str) -> Optional[str]:
+ """Say that ``subject`` and ``pr_title`` would update JIRA differently, or
return None.
+
+ Both are compared by :func:`jira_action`: whether JIRA is skipped, and if
not, which
+ tickets and in which direction. The components are not compared: they
differ whenever
+ one was entered at the merge prompt, and are only reconciled with an
unresolved ticket.
+ A resolve of a ticket that is already resolved as Fixed only adds fix
versions, and a
Review Comment:
Done. The docstring now says that resolving a ticket already resolved as
Fixed offers to add fix versions and may offer to correct the Affects
Version/s, without reconciling components (see `resolve_jira_issue()`), and
still says that a ticket resolved other than as Fixed is left as is.
##########
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:
Update: my reply above named a squash-button commit as the case without the
merge footer. That was wrong: `find_merge_commit()` never returns such a
commit, since GitHub links it to the PR's `merged` event and the `closed` event
has no commit, so a squash-merged PR takes the merge path, as on master. The
fallback without the footer is another PR's merge commit linked to the `closed`
event. Details in the reply to the spark-test review.
--
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]
