spark-test commented on code in PR #59899:
URL: https://github.com/apache/spark/pull/59899#discussion_r4418977115
##########
dev/merge_spark_pr.py:
##########
@@ -901,12 +1516,312 @@ def merge_pr(pr_num, target_ref, title, body,
pr_repo_desc, pr_author, co_author
return merge_hash
-def _do_cherry_pick(pr_num, merge_hash, pick_ref):
- """Cherry-pick `merge_hash` onto `pick_ref` and push.
+def _backport_pr_branch(pr_num, pick_ref):
+ """Fork branch name for a backport PR: stable, PR-traceable,
branch-suffixed.
- Returns the (pushed ref, pushed commit hash) pair. Raises `SkipCherryPick`
if the
- cherry-pick conflicts and the committer declines to resolve it, after
attempting to
- abort the cherry-pick and restore the working tree.
+ >>> _backport_pr_branch(123, "branch-4.2")
+ 'backport-pr123-branch-4.2'
+ >>> _backport_pr_branch(9, "branch-3.5")
+ 'backport-pr9-branch-3.5'
+ """
+ return "backport-pr%s-%s" % (pr_num, pick_ref)
+
+
+def _backport_pr_title(pick_ref, title):
+ """PR title for a backport PR: the merged title with its version tag set
to the target.
+
+ The tag goes where Title.parse expects one, so the script can still merge
the result.
+ Revert/Reapply and other unparseable titles stay verbatim.
+
+ >>> _backport_pr_title("branch-4.2", "[SPARK-1234][SQL] Fix a thing")
+ '[SPARK-1234][4.2][SQL] Fix a thing'
+ >>> _backport_pr_title("branch-4.x", "[SPARK-1234][4.X][SQL] Fix a thing")
+ '[SPARK-1234][4.X][SQL] Fix a thing'
+ >>> _backport_pr_title("branch-4.2", "[SPARK-1234][4.X][SQL] Fix a thing")
+ '[SPARK-1234][4.2][SQL] Fix a thing'
+ >>> _backport_pr_title("branch-4.2", "[MINOR] Fix typo")
+ '[MINOR][4.2] Fix typo'
+ >>> _backport_pr_title("branch-4.2-preview", "[MINOR] Fix typo")
+ '[MINOR] Fix typo'
+ >>> _backport_pr_title("branch-4.2", 'Revert "[SPARK-1234][SQL] Fix"')
+ 'Revert "[SPARK-1234][SQL] Fix"'
+ >>> all(
+ ... Title.parse(_backport_pr_title("branch-4.2", t))
+ ... for t in ("[SPARK-1234][SQL] Fix", "[MINOR] Fix typo")
+ ... )
+ True
+ """
+ try:
+ parsed = Title.parse(title)
+ except ValueError:
+ return title
+ version = pick_ref[len("branch-") :].upper()
+ if not _VERSION_TAG_RE.match(version):
+ return title
+ others = [c for c in parsed.components if not _VERSION_TAG_RE.match(c)]
+ return str(Title(parsed.leading, [version] + others, parsed.text))
+
+
+def _remote_repo(url):
+ """`owner/repo` named by a git remote URL, or None.
+
+ >>> _remote_repo("[email protected]:me/spark.git")
+ 'me/spark'
+ >>> _remote_repo("https://github.com/apache/spark.git";)
+ 'apache/spark'
+ >>> _remote_repo("ssh://[email protected]:443/me/spark.git")
+ 'me/spark'
+ >>> _remote_repo("git@github-work:me/spark")
+ 'me/spark'
+ >>> _remote_repo("https://gitbox.apache.org/repos/asf/spark.git";)
+ 'asf/spark'
+ >>> _remote_repo("") is None
+ True
+ """
+ m = re.search(r"[:/]([^/:]+)/([^/:]+?)(?:\.git)?/?$", url)
+ return "%s/%s" % m.groups() if m else None
+
+
+def _remote_url(name):
+ try:
+ return git.run(["git", "remote", "get-url", name]).strip()
Review Comment:
**Nit (P3):** `git remote get-url <name>` returns the remote's fetch URL,
but `git push <name>` goes to `remote.<name>.pushurl` when one is set, and
applies `url.*.pushInsteadOf`. I checked with git 2.47.3 and two local bare
repos. With `origin` fetching from A and `pushurl` set to B, `get-url origin`
printed A, `git push origin +HEAD:refs/heads/backport-pr7-branch-4.2` created
the branch only in B, and `get-url --push origin` printed B.
So with a fork fetch URL and an Apache push URL, `_fork_owner` passes, and
line 1741 force-creates `backport-pr<N>-<branch>` in the Apache repository.
That starts `build_main.yml` there (it runs on pushes to every branch) and
mails commits@, which is exactly what the guard and the docstring's 'only after
_fork_owner vetted the destination' promise to prevent. PR creation then fails,
because the head owner came from the fetch URL.
The configuration is unusual, but the fix is small: vet every URL from `git
remote get-url --push --all <fork remote>` with the same refusal, and refuse
when they name a different repository than the fetch URL.
**Verification:**
- **Regression:** With a fork fetch URL and an Apache push URL configured on
the fork remote, the guard refuses and no push is attempted.
- **Compatibility:** A fork remote without a separate push URL, and a
host-alias URL with FORK_OWNER set, resolve to the same owner as today.
##########
dev/merge_spark_pr.py:
##########
@@ -846,41 +1440,62 @@ def merge_pr(pr_num, target_ref, title, body,
pr_repo_desc, pr_author, co_author
continue_maybe(msg)
had_conflicts = True
- # Use PR author as the primary author for proper GitHub attribution
- primary_author = bold_input(
- 'Enter primary author in the format of "name <email>" [%s]: ' %
pr_author
- )
- if primary_author == "":
- primary_author = pr_author
-
- merge_message_flags = []
-
- merge_message_flags += ["-m", title]
- if body is not None:
- # We remove @ symbols from the body to avoid triggering e-mails
- # to people every time someone creates a public fork of Spark.
- merge_message_flags += ["-m", body.replace("@", "")]
+ backport = _backport_of(body, pr_repo_desc.split("/", 1)[-1], target_ref)
+ if backport:
+ # The footer already names the author; a different --author would
contradict it.
+ print("Backport of #%s: keeping its commit message and author." %
backport[0])
+ primary_author = backport[2]
+ else:
+ # Use PR author as the primary author for proper GitHub attribution
+ primary_author = bold_input(
+ 'Enter primary author in the format of "name <email>" [%s]: ' %
pr_author
+ )
+ if primary_author == "":
+ primary_author = pr_author
committer_name = git.run("git config --get user.name").strip()
committer_email = git.run("git config --get user.email").strip()
+ signoff = "Signed-off-by: %s <%s>" % (committer_name, committer_email)
+ conflicts = "This patch had conflicts when merged, resolved by\nCommitter:
%s <%s>" % (
+ committer_name,
+ committer_email,
+ )
- if had_conflicts:
- message = "This patch had conflicts when merged, resolved
by\nCommitter: %s <%s>" % (
- committer_name,
- committer_email,
- )
- merge_message_flags += ["-m", message]
+ merge_message_flags = ["-m", title]
+ if backport:
+ # The original message already ends with #N's footer and authors (its
@s were stripped
+ # at the original merge). No "Closes #<this PR>": that would become
the last footer and
+ # hide the branch from #N's merge status. main() closes this PR
through the API.
+ merge_message_flags += ["-m", backport[1]]
+ if had_conflicts:
+ merge_message_flags += ["-m", conflicts]
+ # Credit anyone who pushed fixups to the backport PR itself. Match on
the "<email>":
+ # the pick's own commit is the original author's, maybe under another
display name.
+ trailers = ["Backport-PR: #%s" % pr_num]
+ trailers += [
+ "Co-authored-by: %s" % a
+ for a in co_authors
+ if "<" + a.split("<")[-1] not in backport[1]
+ ]
+ merge_message_flags += ["-m", "\n".join(trailers + [signoff])]
Review Comment:
**Nit (P3):** Each `-m` becomes its own paragraph, and git reads trailers
only from the last one. I ran `git interpret-trailers --parse` on both shapes.
The direct-pick message yields the original `Lead-authored-by`,
`Co-authored-by` and both `Signed-off-by` lines. The message this arm builds
yields only `Backport-PR: #9` and the merger's `Signed-off-by`. Likewise, `git
shortlog --group=trailer:co-authored-by` credits the original co-author for the
first shape and nobody for the second. I couldn't check offline whether
GitHub's co-author display follows the same last-paragraph rule. This arm's own
fixup `Co-authored-by` lines rely on that rule too.
Joining these lines onto the reused message's final paragraph with a single
newline keeps one trailer block. I checked that the result still parses with
the original authorship lines, and that `merge_footer_pr` still returns #N.
Emitting the conflicts note before the reused message keeps its `Committer:`
line out of that block.
**Verification:**
- **Behavior:** The message merge_pr builds for a recognized backport PR,
parsed as git trailers, contains the original authorship lines as well as the
new Backport-PR and Signed-off-by lines.
- **Compatibility:** With and without the conflicts note, the original PR's
footer remains the last generated footer that the shared reader finds.
##########
dev/merge_spark_pr.py:
##########
@@ -946,9 +1861,34 @@ def _do_cherry_pick(pr_num, merge_hash, pick_ref):
else:
print("Cherry-pick already completed manually; continuing with the
backport.")
- continue_maybe(
- "Pick complete (local ref %s). Push to %s?" % (pick_branch_name,
PUSH_REMOTE_NAME)
- )
+ maybe_binding_policy_warning(pick_ref, pick_head)
+ maybe_compile_check(pick_branch_name)
+
+ while True:
+ choice = _landing_choice(pick_branch_name, pick_ref)
+ if choice == "s":
+ # Restore the original checkout and drop the temp PICK_PR_*
branches, as the
+ # continue_maybe decline path does; otherwise a later pick of the
same ref fails
+ # when `git fetch` tries to update the checked-out branch.
+ print("Skipping this branch; the merge continues.")
+ clean_up()
+ raise SkipCherryPick()
+ if choice == "p":
Review Comment:
**Nit (P3):** This is what remains of the earlier comment on the untested
[p] path. `_land_via_fork`'s outcomes, the Enter default and FORK_OWNER
precedence are now pinned; the caller in this loop is not. The only doctest
that executes `_do_cherry_pick` (in `cherry_pick`'s docstring) answers `d`,
declines, then `s`, and mocks both pre-push helpers without asserting their
calls. Each of these mutations of a scratch copy kept all 406 doctests passing:
- `continue` at 1882 replaced by `pass`. A failed fork landing then falls
through to `git push <apache> PICK:<branch>`, a direct push of a pick the
committer chose to gate on CI.
- `return pick_ref, marker` returning a commit hash. The open backport PR
then counts as landed for JIRA and Upstream-First.
- The `maybe_binding_policy_warning` call deleted, or its arguments swapped.
The swap fails open through `semver_branch_rank`'s sentinel.
A few `_do_cherry_pick` doctests would cover this:
- [p] success returns the marker with no direct push.
- [p] failure loops back, and [s] then skips with no push.
- Each pre-push helper is called once with the right arguments (the
binding-policy check with the pre-pick head).
This extends an [existing
discussion](https://github.com/apache/spark/pull/59899#discussion_r4408045794).
**Verification:**
- **Regression:** Each of those landing-loop regressions makes a doctest
fail, while the current loop passes.
##########
dev/merge_spark_pr.py:
##########
@@ -2595,20 +3678,26 @@ def main():
default,
branch_names,
target_ref,
- already_picked=tuple(picked_refs),
+ already_picked=tuple(landed_refs),
Review Comment:
**Nit (P3):** Only `main()` keeps pending picks out of `already_picked` and
out of the JIRA input: here, in the merge-path loop, and in both
`update_jira_for_pr` calls. No doctest reaches it. I changed this line to
`already_picked=tuple(picked_refs)` in a scratch copy, which is the shape of
the earlier Upstream-First bypass, and all 406 doctests still passed. `main()`
had no doctest seam before this PR either. One fix: move the per-pick
bookkeeping that both loops repeat into a helper and doctest it with a direct
push, a PR link, a `fork:` marker and an earlier-run pending branch. That pins
the routing without mocking all of `main()`.
**Verification:**
- **Regression:** Given a direct push, a backport PR link, a fork-only
marker and an earlier-run pending branch, the helper reports only the direct
push as landed, the other three as pending, and all four as excluded from the
prompt default. Treating every pick as landed makes the doctest fail.
##########
dev/merge_spark_pr.py:
##########
@@ -901,12 +1516,312 @@ def merge_pr(pr_num, target_ref, title, body,
pr_repo_desc, pr_author, co_author
return merge_hash
-def _do_cherry_pick(pr_num, merge_hash, pick_ref):
- """Cherry-pick `merge_hash` onto `pick_ref` and push.
+def _backport_pr_branch(pr_num, pick_ref):
+ """Fork branch name for a backport PR: stable, PR-traceable,
branch-suffixed.
- Returns the (pushed ref, pushed commit hash) pair. Raises `SkipCherryPick`
if the
- cherry-pick conflicts and the committer declines to resolve it, after
attempting to
- abort the cherry-pick and restore the working tree.
+ >>> _backport_pr_branch(123, "branch-4.2")
+ 'backport-pr123-branch-4.2'
+ >>> _backport_pr_branch(9, "branch-3.5")
+ 'backport-pr9-branch-3.5'
+ """
+ return "backport-pr%s-%s" % (pr_num, pick_ref)
+
+
+def _backport_pr_title(pick_ref, title):
+ """PR title for a backport PR: the merged title with its version tag set
to the target.
+
+ The tag goes where Title.parse expects one, so the script can still merge
the result.
+ Revert/Reapply and other unparseable titles stay verbatim.
+
+ >>> _backport_pr_title("branch-4.2", "[SPARK-1234][SQL] Fix a thing")
+ '[SPARK-1234][4.2][SQL] Fix a thing'
+ >>> _backport_pr_title("branch-4.x", "[SPARK-1234][4.X][SQL] Fix a thing")
+ '[SPARK-1234][4.X][SQL] Fix a thing'
+ >>> _backport_pr_title("branch-4.2", "[SPARK-1234][4.X][SQL] Fix a thing")
+ '[SPARK-1234][4.2][SQL] Fix a thing'
+ >>> _backport_pr_title("branch-4.2", "[MINOR] Fix typo")
+ '[MINOR][4.2] Fix typo'
+ >>> _backport_pr_title("branch-4.2-preview", "[MINOR] Fix typo")
+ '[MINOR] Fix typo'
+ >>> _backport_pr_title("branch-4.2", 'Revert "[SPARK-1234][SQL] Fix"')
+ 'Revert "[SPARK-1234][SQL] Fix"'
+ >>> all(
+ ... Title.parse(_backport_pr_title("branch-4.2", t))
+ ... for t in ("[SPARK-1234][SQL] Fix", "[MINOR] Fix typo")
+ ... )
+ True
+ """
+ try:
+ parsed = Title.parse(title)
+ except ValueError:
+ return title
+ version = pick_ref[len("branch-") :].upper()
+ if not _VERSION_TAG_RE.match(version):
+ return title
+ others = [c for c in parsed.components if not _VERSION_TAG_RE.match(c)]
+ return str(Title(parsed.leading, [version] + others, parsed.text))
+
+
+def _remote_repo(url):
+ """`owner/repo` named by a git remote URL, or None.
+
+ >>> _remote_repo("[email protected]:me/spark.git")
+ 'me/spark'
+ >>> _remote_repo("https://github.com/apache/spark.git";)
+ 'apache/spark'
+ >>> _remote_repo("ssh://[email protected]:443/me/spark.git")
+ 'me/spark'
+ >>> _remote_repo("git@github-work:me/spark")
+ 'me/spark'
+ >>> _remote_repo("https://gitbox.apache.org/repos/asf/spark.git";)
+ 'asf/spark'
+ >>> _remote_repo("") is None
+ True
+ """
+ m = re.search(r"[:/]([^/:]+)/([^/:]+?)(?:\.git)?/?$", url)
+ return "%s/%s" % m.groups() if m else None
+
+
+def _remote_url(name):
+ try:
+ return git.run(["git", "remote", "get-url", name]).strip()
+ except Exception:
+ return ""
+
+
+def _fork_owner():
+ """GitHub owner of the fork remote, for a backport PR's `head`. ValueError
if unusable.
+
+ The fork is checked by the repository its URL names, not the remote name:
an `origin` that
+ was never repointed off apache/spark would otherwise get the backport
branch and the PR.
+
+ The env knobs are pinned so a committer's own settings cannot fail these
at startup:
+
+ >>> from unittest.mock import patch
+ >>> def remotes(fork, owner=""):
+ ... urls = {"apache": "https://gitbox.apache.org/repos/asf/spark.git";,
+ ... "apache-github": "https://github.com/apache/spark.git";,
"origin": fork}
+ ... return patch.multiple(sys.modules[__name__], _remote_url=urls.get,
FORK_OWNER=owner,
+ ... FORK_REMOTE_NAME="origin",
PUSH_REMOTE_NAME="apache",
+ ... PR_REMOTE_NAME="apache-github")
+ >>> def owner_of(fork, owner=""):
+ ... with remotes(fork, owner):
+ ... try:
+ ... return _fork_owner()
+ ... except ValueError as e:
+ ... return "refused" if "is the Apache repo" in str(e) else
str(e)
+ >>> owner_of("ssh://[email protected]:443/me/spark.git")
+ 'me'
+ >>> [owner_of(f, owner="me") for f in (
+ ... "[email protected]:apache/spark.git",
+ ... "https://gitbox.apache.org/repos/asf/spark.git";,
+ ... "git@gh:Apache/Spark",
+ ... )]
+ ['refused', 'refused', 'refused']
+
+ A github.com URL names its owner; FORK_OWNER only fills in for a host
alias:
+
+ >>> owner_of("[email protected]:me/spark.git", owner="someone-else")
+ 'me'
+ >>> owner_of("git@gh-alias:me/spark.git", owner="Me")
+ 'Me'
+ >>> owner_of("git@gh-alias:me/spark.git").startswith("Cannot tell the
GitHub owner")
+ True
+ """
+ url = _remote_url(FORK_REMOTE_NAME)
+ repo = _remote_repo(url)
+ if repo is None:
+ raise ValueError(
+ "No usable %s remote (%r). Set FORK_REMOTE_NAME to your fork's
remote."
+ % (FORK_REMOTE_NAME, url)
+ )
+ apache = {"apache/spark"} | {
+ (_remote_repo(_remote_url(r)) or "").lower() for r in
(PUSH_REMOTE_NAME, PR_REMOTE_NAME)
+ }
+ if repo.lower() in apache:
+ raise ValueError(
+ "%s remote (%s) is the Apache repo, not a fork. Set
FORK_REMOTE_NAME to your "
+ "fork's remote." % (FORK_REMOTE_NAME, url)
+ )
+ if "github.com" in url:
+ return repo.split("/")[0]
+ if FORK_OWNER:
+ return FORK_OWNER
+ raise ValueError(
+ "Cannot tell the GitHub owner of %s remote (%s); set FORK_OWNER to
your GitHub login."
+ % (FORK_REMOTE_NAME, url)
+ )
+
+
+def _landing_choice(pick_branch_name, pick_ref):
+ """Ask how a finished pick lands: [d]irect push, [p] fork + draft PR, or
[s]kip (default).
+
+ >>> from contextlib import redirect_stdout
+ >>> from io import StringIO
+ >>> from unittest.mock import patch
+ >>> with patch("builtins.input", return_value=""),
redirect_stdout(StringIO()):
+ ... choice = _landing_choice("PR_TOOL_PICK_PR_1_BRANCH-4.2",
"branch-4.2")
+ >>> choice
+ 's'
+ """
+ return get_input(
+ "Pick complete (local ref %s). How should it land?\n"
+ " [d] Push directly to %s/%s\n"
+ " [p] Push to your fork (%s) and open a draft backport PR against %s
(for CI)\n"
+ " [s] Skip this branch (default)\n"
+ "Choose [d/p/s]: "
+ % (pick_branch_name, PUSH_REMOTE_NAME, pick_ref, FORK_REMOTE_NAME,
pick_ref),
+ {"d": ["d", "direct"], "p": ["p", "pr"], "s": ["s", "skip", ""]},
+ )
+
+
+def _land_via_fork(pr_num, pick_ref, pick_branch_name, title):
+ """Push the pick to the fork and open a draft backport PR against
`pick_ref`.
+
+ Returns the PR URL, or a `fork:<owner>:<branch>` marker if the push worked
but the PR did
+ not open. Either failure offers _manual_backport_pr, whose pasted URL
wins. Returns None,
+ having pushed nothing, when the fork is unusable or the push fails and no
URL was pasted,
+ so the caller can retry, push directly, or skip. The fork branch is
script-owned and
+ force-pushed (only after _fork_owner vetted the destination), so a retry
updates it.
+
+ >>> from contextlib import redirect_stdout
+ >>> from io import StringIO
+ >>> from unittest.mock import Mock, patch
+ >>> def land(owner, created, push_error=None, pasted=""):
+ ... with (
+ ... patch("builtins.input", return_value=pasted),
+ ... patch.multiple(
+ ... sys.modules[__name__],
_fork_owner=Mock(side_effect=[owner]),
+ ... FORK_REMOTE_NAME="origin",
+ ... ),
+ ... patch.object(git, "push", side_effect=push_error) as push,
+ ... patch.object(git, "run", return_value="body\\n"),
+ ... patch.object(github, "create_pr", return_value=created) as
create,
+ ... redirect_stdout(StringIO()),
+ ... ):
+ ... marker = _land_via_fork(7, "branch-4.2", "PICK",
"[SPARK-1][SQL] Fix")
+ ... return marker, push.call_args_list, create.call_args_list
+ >>> marker, pushes, creates = land("me", {"html_url":
"https://github.com/apache/spark/pull/9"})
+ >>> marker
+ 'https://github.com/apache/spark/pull/9'
+ >>> pushes
+ [call('origin', 'PICK', 'backport-pr7-branch-4.2', force=True)]
+ >>> creates[0].args[:3]
+ ('branch-4.2', 'me:backport-pr7-branch-4.2', '[SPARK-1][4.2][SQL] Fix')
+ >>> creates[0].args[3]
+ 'Backport-of: #7\\n\\nbody'
+ >>> land("me", None)[0]
+ 'fork:me:backport-pr7-branch-4.2'
+ >>> land("me", None, push_error=OSError("rejected"))
+ (None, [call('origin', 'PICK', 'backport-pr7-branch-4.2', force=True)], [])
+ >>> land(ValueError("origin is the Apache repo"), None)
+ (None, [], [])
+ >>> land("me", None, pasted="https://github.com/apache/spark/pull/10";)[0]
+ 'https://github.com/apache/spark/pull/10'
+
+ The dry-run seam fakes both effects, so a dry run takes the success branch:
+
+ >>> with redirect_stdout(StringIO()) as out:
+ ... DryRunGit().push("origin", "PICK", "backport-pr7-branch-4.2",
force=True)
+ ... created = DryRunGitHub().create_pr("branch-4.2", "me:b", "t", "b")
+ >>> created["html_url"]
+ 'https://github.com/apache/spark/pull/DRY-RUN'
+ >>> "would push PICK to origin:backport-pr7-branch-4.2 (forced)" in
out.getvalue()
+ True
+ """
+ backport_branch = _backport_pr_branch(pr_num, pick_ref)
+ pr_title = _backport_pr_title(pick_ref, title)
+ # The pick's own message is what GitHub would prefill for a single-commit
branch. Its @s
+ # were stripped when the merge commit was built, so it fires no mention
mails. The marker
+ # lets merge_pr keep the original message, footer and author when this PR
is merged.
+ pr_body = git.run(["git", "log", "-1", "--format=%b",
pick_branch_name]).strip()
+ pr_body = "Backport-of: #%s\n\n%s" % (pr_num, pr_body)
Review Comment:
**Non-blocking (P2):** `pr_num` here is the PR the current run is
processing. `_backport_of`, however, accepts the marker only when it matches
the pick message's last merge footer (1416-1420), and the two differ when the
run merged a script-opened backport PR. I checked with the head module. A
branch-4.1 pick made from the run that merged #9 (the backport of #7) has
`merge_footer_pr(...) == 7`. Its body `Backport-of: #9` with head
`backport-pr9-branch-4.1` is rejected, while the same body as `Backport-of: #7`
on `backport-pr7-branch-4.1` is accepted with the original author.
After the rejection, merging that PR takes the normal arm. The author prompt
defaults to the backport PR's opener, the contributor drops to
`Co-authored-by`, and `Closes #<new PR> from ...` becomes the last footer.
`dev/pr_merge_status.py 7` and #7's already-backported scan then miss
branch-4.1, and `pending_fork_backports(7)` never sees the `backport-pr9-*`
branch.
The likeliest trigger is the Upstream-First-friendly order: merge the
branch-4.x backport PR first, then continue to branch-4.2 with [p] from that
same run. Taking N from `merge_footer_pr` of the pick's message, and falling
back to `pr_num`, would keep one identity per original PR and branch.
**Verification:**
- **Regression:** A pick whose message carries the original PR's footer,
landed with [p] from a run on a different PR, gets a fork branch and
Backport-of marker naming the original PR, and Backport-of recognition accepts
that body for that head branch.
- **Compatibility:** A pick from a run on the original PR still uses that
PR's number, and a pick whose message has no merge footer falls back to the
run's PR.
##########
dev/merge_spark_pr.py:
##########
@@ -846,41 +1440,62 @@ def merge_pr(pr_num, target_ref, title, body,
pr_repo_desc, pr_author, co_author
continue_maybe(msg)
had_conflicts = True
- # Use PR author as the primary author for proper GitHub attribution
- primary_author = bold_input(
- 'Enter primary author in the format of "name <email>" [%s]: ' %
pr_author
- )
- if primary_author == "":
- primary_author = pr_author
-
- merge_message_flags = []
-
- merge_message_flags += ["-m", title]
- if body is not None:
- # We remove @ symbols from the body to avoid triggering e-mails
- # to people every time someone creates a public fork of Spark.
- merge_message_flags += ["-m", body.replace("@", "")]
+ backport = _backport_of(body, pr_repo_desc.split("/", 1)[-1], target_ref)
+ if backport:
+ # The footer already names the author; a different --author would
contradict it.
+ print("Backport of #%s: keeping its commit message and author." %
backport[0])
+ primary_author = backport[2]
+ else:
+ # Use PR author as the primary author for proper GitHub attribution
+ primary_author = bold_input(
+ 'Enter primary author in the format of "name <email>" [%s]: ' %
pr_author
+ )
+ if primary_author == "":
+ primary_author = pr_author
committer_name = git.run("git config --get user.name").strip()
committer_email = git.run("git config --get user.email").strip()
+ signoff = "Signed-off-by: %s <%s>" % (committer_name, committer_email)
+ conflicts = "This patch had conflicts when merged, resolved by\nCommitter:
%s <%s>" % (
+ committer_name,
+ committer_email,
+ )
- if had_conflicts:
- message = "This patch had conflicts when merged, resolved
by\nCommitter: %s <%s>" % (
- committer_name,
- committer_email,
- )
- merge_message_flags += ["-m", message]
+ merge_message_flags = ["-m", title]
+ if backport:
+ # The original message already ends with #N's footer and authors (its
@s were stripped
+ # at the original merge). No "Closes #<this PR>": that would become
the last footer and
+ # hide the branch from #N's merge status. main() closes this PR
through the API.
+ merge_message_flags += ["-m", backport[1]]
+ if had_conflicts:
+ merge_message_flags += ["-m", conflicts]
+ # Credit anyone who pushed fixups to the backport PR itself. Match on
the "<email>":
+ # the pick's own commit is the original author's, maybe under another
display name.
+ trailers = ["Backport-PR: #%s" % pr_num]
Review Comment:
**Non-blocking (P2):** This arm records the backport PR only as
`Backport-PR: #<pr>` and deliberately drops `Closes #<pr> from ...`. But
`dev/spark_merge_footer.py` recognizes a merge only by a `Closes #<pr> from`
line followed by an authors paragraph (`_MERGE_FOOTER_RE`, `has_merge_footer`,
and the `git log --grep` prefilter). I ran the head reader on the message this
arm builds for backport PR #9 of #7: `has_merge_footer(msg, 9)` is False and
`has_merge_footer(msg, 7)` is True.
So once #9 lands on branch-4.2:
- `dev/pr_merge_status.py 9` prints 'closed without merging ... (rejected or
superseded)'. AGENTS.md sends readers to that tool for exactly this question,
and the answer contradicts the script's own '- merged into branch-4.2' comment
on #9.
- `dev/merge_spark_pr.py 9` gets `(None, None)` from `find_merge_commit`.
main() closed #9 through the API, so its closed event carries no commit, and
the referenced commit fails the footer check. The script then offers a fresh
merge of an already-applied change instead of entering backport-only mode.
#7's own status is unaffected, so only the backport PR's own merge signal is
lost.
**Recommended change:** Teach the shared footer reader that a backport merge
commit belongs to two PRs: the original PR, whose 'Closes #<N> from' footer
comes last, and the backport PR named by a 'Backport-PR: #<M>' line in the
final trailer paragraph. has_merge_footer should answer true for either number,
branches_with_merge_footer's git-log prefilter should match either form, and
the module docstring should describe the backport format. merge_footer_pr keeps
returning the original PR.
**Why this works:** With one definition of the backport format in the shared
module, dev/pr_merge_status.py and the merge script's find_merge_commit see a
merged backport PR through the functions they already call. Every lookup for
the original PR still keys on the last 'Closes' footer and is unchanged.
**Scope:** Extend the shared footer reader so it recognizes a merged
backport PR by its Backport-PR trailer, and update the contract the module
documents.
**Compatibility:** The original PR's merge status and its already-backported
branch scan stay exactly as they are today.
**Risks:** A match for 'Backport-PR: #<M>' anywhere in the message would let
a PR body that quotes the trailer claim a merge, so the reader must apply the
same position rule it applies to footers and accept the trailer only in the
final paragraph after the last generated footer.
**Constraints:** merge_footer_pr must keep returning the original PR for
backport commits, so Backport-of recognition and every status lookup for the
original PR stay unchanged.
**Exclusions:**
- The producer's paragraph layout is changed by the separate trailer-block
fix; this reader change only has to accept the layout that fix produces.
**Success:** dev/pr_merge_status.py reports a merged backport PR as merged
into its target branch. Re-running the merge script on a merged backport PR
finds its merge commit and enters backport-only mode. Status lookups for the
original PR return the same branches as before.
**Verification:**
- **Behavior:** For a message that carries the original PR's footer followed
by a final paragraph containing the backport PR's trailer, the shared reader
reports a merge for both PR numbers and still names the original PR as the
footer's PR.
- **Regression:** A PR body that quotes a backport trailer before the
generated footer does not make the reader report a merge for the quoted PR
number.
- **Compatibility:** Messages without a backport trailer, cherry-picked
messages with provenance lines, and bodies that quote another PR's footer keep
the reader results they have today.
##########
dev/merge_spark_pr.py:
##########
@@ -673,9 +711,9 @@ def message_of(commit_hash):
class GitHub:
"""GitHub REST writes used by the merge script -- the single seam for PR
mutations.
- Reads go through the module-level get_json(); only the two mutations
(closing the PR and
- posting a comment) live here, so DryRunGitHub can override them without
touching any read
- path. main() constructs GitHub() normally and DryRunGitHub() for a dry run.
+ Reads go through the module-level get_json(); the mutations (closing the
PR, posting a
Review Comment:
**Nit (P3):** The texts the earlier comment listed are updated, but this
docstring now misdescribes code added since then. `open_pr` (759-767) is a
read, yet it goes through the new `_api()` rather than `get_json()`, and it
returns None on any error instead of giving `get_json()`'s rate-limit/401
diagnostics and exiting. `create_pr` also PATCHes an existing open backport
PR's title and body (776-780), which the mutation list doesn't include, so
checking DryRunGitHub's coverage against this inventory would miss the PATCH.
Naming the best-effort open-PR lookup and the update-or-create behavior here
would close the gap.
This extends an [existing
discussion](https://github.com/apache/spark/pull/59899#discussion_r4408045797).
**Verification:**
- **Inspection:** The docstring names every request the GitHub class issues
(closing a PR, commenting, looking up an open PR, updating a PR, creating a PR)
and classifies each correctly as a read or a write.
##########
dev/merge_spark_pr.py:
##########
@@ -946,9 +1861,34 @@ def _do_cherry_pick(pr_num, merge_hash, pick_ref):
else:
print("Cherry-pick already completed manually; continuing with the
backport.")
- continue_maybe(
- "Pick complete (local ref %s). Push to %s?" % (pick_branch_name,
PUSH_REMOTE_NAME)
- )
+ maybe_binding_policy_warning(pick_ref, pick_head)
+ maybe_compile_check(pick_branch_name)
Review Comment:
**Nit (P3):** The opt-in compile runs between the fetch at 1828 and the
non-forced `git push <apache> PICK:<branch>` at 1894, and nothing re-checks the
remote tip in between. If another committer pushes to the branch while a clean
compile of the default modules plus Hive runs, the push is rejected.
`fail('Exception while pushing: ...')` at 1896 then runs `clean_up()`, which
force-deletes the pick branch with any hand-resolved conflicts and exits the
run. Inside `[b]oth`, the already-landed branch-M.x also drops out of the merge
comment and the JIRA update, because only `SkipCherryPick` is caught there.
The `fail()` itself predates this PR; the long build in front of it is what
makes the race realistic. `maybe_compile_check` already avoids the same loss on
Ctrl-C (1175-1177). A failed direct push could be handled the way a failed fork
push already is: print the error and go back to the landing prompt with the
pick intact, where [p] and [s] remain available.
**Verification:**
- **Regression:** When the direct push fails, the cherry-pick step keeps the
pick branch, does not exit, and honors the committer's next landing answer.
##########
dev/merge_spark_pr.py:
##########
@@ -983,6 +1923,59 @@ def branches_with_merge_footer(pr_num, branch_names):
return [b for b in branch_names if b in landed]
+def pending_fork_backports(pr_num, branch_names):
+ """{branch: PR URL} for release branches with an open backport PR from an
earlier run.
+
+ That PR leaves no merge footer, only its backport-pr<N>-<branch> branch on
the fork, so look
+ there first and then confirm the PR is still open: a closed one's branch
often lingers.
+ Best-effort like branches_with_merge_footer: an unusable fork or a failure
reports nothing.
+
+ >>> from contextlib import redirect_stdout
+ >>> from io import StringIO
+ >>> from unittest.mock import patch
+ >>> heads = "abc\\trefs/heads/backport-pr7-branch-4.x\\n"
+ >>> heads += "abc\\trefs/heads/backport-pr7-branch-4.1\\n"
+ >>> def open_pr(base, head):
+ ... return {"html_url": "https://x/pull/9"} if base == "branch-4.x"
else None
+ >>> m = sys.modules[__name__]
+ >>> with (
+ ... patch.multiple(m, FORK_REMOTE_NAME="origin", _fork_owner=lambda:
"me"),
+ ... patch.object(git, "run", return_value=heads) as run,
+ ... patch.object(github, "open_pr", side_effect=open_pr) as gh,
+ ... ):
+ ... pending_fork_backports(7, ["branch-4.x", "branch-4.2",
"branch-4.1"])
+ {'branch-4.x': 'https://x/pull/9'}
+ >>> run.call_args[0][0]
+ ['git', 'ls-remote', '--heads', 'origin', 'refs/heads/backport-pr7-*']
+ >>> [c.args for c in gh.call_args_list]
+ [('branch-4.x', 'me:backport-pr7-branch-4.x'), ('branch-4.1',
'me:backport-pr7-branch-4.1')]
+ >>> with (
+ ... patch.object(m, "_fork_owner", side_effect=ValueError("origin is
the Apache repo")),
+ ... patch.object(git, "run", side_effect=AssertionError("should not
query")),
Review Comment:
**Nit (P3):** `git.run` is inside the same `try ... except Exception` as
`_fork_owner()` (1962-1967). An unexpected `ls-remote` therefore raises this
`AssertionError` into the handler, which prints it to the redirected stdout and
returns `{}`, and `{}` is all this example checks. I moved the `ls-remote`
ahead of `_fork_owner()` in a scratch copy and all 406 doctests still passed.
(Deleting the `_fork_owner()` call outright is caught, but only incidentally:
the first example then fails on the undefined `owner`.) Recording the git calls
and asserting there were none, or asserting the printed refusal reason, would
make the guard observable.
**Verification:**
- **Regression:** With the ls-remote call moved ahead of the fork vetting,
the doctest fails; with the current order, it passes.
##########
dev/merge_spark_pr.py:
##########
@@ -828,6 +1031,397 @@ def clean_up():
git.run("git branch -D %s" % branch)
+def maybe_compile_check(ref_name: str) -> None:
+ """Offer to run a clean SBT compile of main and test sources before
pushing `ref_name`.
+
+ `ref_name` is the checked-out cherry-pick, so a clean compile here catches
a mis-resolved
+ conflict, or a PR that bit-rotted against the target branch, before it
lands. It is a smoke
+ test (see COMPILE_CHECK_CMD), not CI: modules behind other profiles are
not compiled.
+ Declining skips the check. A red build asks rather than aborts, because a
non-zero sbt is
+ as easily a JDK the branch rejects or a failed download as it is the
backport; declining
+ there skips this branch (see continue_maybe). Output streams so a long
compile does not
+ look like a hang.
+
+ What is compiled is the working tree, not the commit: a file re-edited
after the merge
+ commit, or an untracked source file left over from a manual conflict
resolution, compiles
+ here and is absent from the push. (A resolved-but-never-`git add`ed
conflict is NOT the
+ case -- the commit itself fails while files are unmerged, so the check is
never reached.)
+ So warn when the tree is dirty rather than report a green that is about
something else,
+ and name the remedy (`git add` + `git commit --amend`) since the fix is
the commit, not
+ the build.
+
+ Declining runs no build and no git -- the git.run mock raises if a
regression moves a
+ git call above the prompt, which otherwise would run real git in the
committer's repo on
+ every startup (doctest.testmod() runs on invocation):
+
+ >>> from contextlib import redirect_stdout
+ >>> from io import StringIO
+ >>> from unittest.mock import call, patch
+ >>> with (
+ ... patch("builtins.input", return_value=""),
+ ... patch.object(git, "run", side_effect=AssertionError("should not
run git")),
+ ... patch("subprocess.call") as run,
+ ... redirect_stdout(StringIO()),
+ ... ):
+ ... maybe_compile_check("PR_TOOL_PICK_PR_1_BRANCH-4.1")
+ >>> run.call_count
+ 0
+
+ Accepting runs the SBT compile once, from the repo root, with an explicit
-P so SparkBuild
+ does not fall back to `-Psbt`; a green build returns quietly. The two git
calls are pinned
+ too -- transposing them or dropping --porcelain would otherwise stay green:
+
+ >>> with (
+ ... patch("builtins.input", return_value="y"),
+ ... patch.object(git, "run", side_effect=["/repo\\n", ""]) as grun,
+ ... patch("subprocess.call", return_value=0) as run,
+ ... redirect_stdout(StringIO()),
+ ... ):
+ ... maybe_compile_check("PR_TOOL_PICK_PR_1_BRANCH-4.1")
+ >>> grun.call_args_list[0]
+ call(['git', 'rev-parse', '--show-toplevel'])
+ >>> grun.call_args_list[1]
+ call(['git', 'status', '--porcelain'])
+ >>> run.call_count
+ 1
+ >>> run.call_args[0][0][0], run.call_args[0][0][-1]
+ ('build/sbt', ';clean;compile;Test/compile')
+ >>> [a for a in run.call_args[0][0] if a.startswith("-P")]
+ ['-Phive']
+ >>> run.call_args[1]["cwd"]
+ '/repo'
+
+ A dirty tree still builds, but warns first -- and the warning names the
remedy
+ (`git add` + `git commit --amend`) alongside the porcelain output:
+
+ >>> with (
+ ... patch("builtins.input", return_value="y"),
+ ... patch.object(git, "run", side_effect=["/repo\\n", " M
foo.scala\\n"]),
+ ... patch("subprocess.call", return_value=0),
+ ... redirect_stdout(StringIO()) as out,
+ ... ):
+ ... maybe_compile_check("PR_TOOL_PICK_PR_1_BRANCH-4.1")
+ >>> "git commit --amend" in out.getvalue()
+ True
+ >>> "M foo.scala" in out.getvalue() # porcelain output is strip()ed
before printing
+ True
+ >>> "Compile check passed." in out.getvalue()
+ True
+
+ The experts-only override continues past a red build:
+
+ >>> with (
+ ... patch("builtins.input", side_effect=["y", "y"]),
+ ... patch.object(git, "run", side_effect=["/repo\\n", ""]),
+ ... patch("subprocess.call", return_value=1),
+ ... redirect_stdout(StringIO()),
+ ... ):
+ ... maybe_compile_check("PR_TOOL_PICK_PR_1_BRANCH-4.1")
+
+ Ctrl-C during the build, and a build/sbt that cannot be run at all, are
"did not pass" and
+ get the same override prompt -- never a traceback, which would reach the
top-level handler
+ and let clean_up() delete the unpushed pick:
+
+ >>> for boom in (KeyboardInterrupt(), OSError("no build/sbt")):
+ ... with (
+ ... patch("builtins.input", side_effect=["y", "y"]),
+ ... patch.object(git, "run", side_effect=["/repo\\n", ""]),
+ ... patch("subprocess.call", side_effect=boom),
+ ... redirect_stdout(StringIO()),
+ ... ):
+ ... maybe_compile_check("PR_TOOL_PICK_PR_1_BRANCH-4.1")
+
+ Declining the override skips just this branch instead of exiting:
+
+ >>> with (
+ ... patch("builtins.input", side_effect=["y", ""]),
+ ... patch.object(
+ ... git, "run", side_effect=["/repo\\n", "",
subprocess.CalledProcessError(1, "g")]
+ ... ),
+ ... patch("subprocess.call", return_value=1),
+ ... redirect_stdout(StringIO()),
+ ... ):
+ ... try:
+ ... maybe_compile_check("PR_TOOL_PICK_PR_1_BRANCH-4.1")
+ ... skipped = False
+ ... except SkipCherryPick:
+ ... skipped = True
+ >>> skipped
+ True
+ """
+ # The command is too long to read inside a prompt, so print it on its own
line first.
+ # shlex.join quotes the ";clean;..." argument, so the printed line can be
copy-pasted
+ # into a shell without the semicolons splitting it into four commands.
+ print("Optional pre-push check: %s" % shlex.join(COMPILE_CHECK_CMD))
+ print("Note: 'clean' deletes every module's build output, including under
--dry-run.")
+ prompt = "Run an optional clean compile on %s before pushing? (y/N): " %
ref_name
+ if get_input(prompt, ["y", "n", ""]) != "y":
+ return
+ # build/sbt is repo-relative and SPARK_HOME defaults to the cwd, which is
dev/ when the
+ # script is run as ./merge_spark_pr.py per its own usage line. Ask git for
the root instead.
+ root = git.run(["git", "rev-parse", "--show-toplevel"]).strip()
+ dirty = git.run(["git", "status", "--porcelain"]).strip()
+ if dirty:
+ print_error(
+ "Working tree is not clean, so what compiles below is not exactly
the commit that "
+ "would be pushed -- the uncommitted or untracked changes below
compile here but "
+ "are not in the push. If any of them belong in the pick, 'git add'
them and "
+ "'git commit --amend' before pushing; if they are unrelated
scratch, ignore this "
+ "and carry on:\n%s" % dirty
+ )
+ try:
+ status = subprocess.call(COMPILE_CHECK_CMD, cwd=root)
+ except KeyboardInterrupt:
+ # Interrupting the build must not unwind into the top-level handler:
clean_up() there
+ # force-deletes the PR_TOOL_* branch holding the unpushed commit and
any hand-resolved
+ # conflicts. Treat it as "the check did not finish" and let the
committer decide.
+ print_error("Compile check interrupted.")
+ except OSError as e:
+ print_error("Could not run %s in %s: %s" % (COMPILE_CHECK_CMD[0],
root, e))
+ else:
+ if status == 0:
+ print("Compile check passed.")
+ return
+ print_error(
+ "Compile check exited %d. That is the backport, or your toolchain
-- a JDK this "
+ "branch rejects and a failed dependency download look the same
here." % status
+ )
+ continue_maybe(
+ "Compile check did not pass on %s. Push anyway? (experts only!)" %
ref_name, True
+ )
+
+
+def _is_pre_binding_policy_branch(ref: str) -> bool:
+ """True if `ref` is a Spark branch that predates the ConfigBindingPolicy
API (4.2+).
+
+ master and branch-M.x (M >= 4) carry the API; branch-4.0/branch-4.1 and
any branch-3.x
+ do not. Ranking is semver_branch_rank's job -- branch-M.x sorts above
every branch-M.N,
+ and a ref it cannot classify as a maintenance branch gets its (-1, -1)
sentinel. Those
+ refs are treated as carrying the API: fail-open, so master, a tag, or a
custom ref never
+ raises a false warning -- none of them is a pre-4.2 branch anyway.
+
+ >>> _is_pre_binding_policy_branch("master")
+ False
+ >>> _is_pre_binding_policy_branch("branch-4.x")
+ False
+ >>> _is_pre_binding_policy_branch("branch-4.2")
+ False
+ >>> [_is_pre_binding_policy_branch(b) for b in ("branch-4.1",
"branch-4.0", "branch-3.x")]
+ [True, True, True]
+ >>> _is_pre_binding_policy_branch("branch-3.5")
+ True
+ >>> _is_pre_binding_policy_branch("HEAD")
+ False
+ >>> _is_pre_binding_policy_branch("my-custom-ref")
+ False
+ """
+ rank = semver_branch_rank(ref)
+ return rank != (-1, -1) and rank < (4, 2)
+
+
+def _added_binding_policy_tokens(diff: str) -> list[str]:
+ """Return the sorted subset of BINDING_POLICY_PATTERNS matched on added
(``+``) lines.
+
+ Only scans lines the diff added (a leading ``+`` that is not the ``+++ ``
file header --
+ the trailing space keeps an added line whose own text starts with ``++``
counted, apart
+ from the ``++ <space>`` case no real Scala/Java line starts with), so a
cherry-pick that
+ *removes* a stray .withBindingPolicy is not flagged. Matching uses the
+ precise patterns from BINDING_POLICY_PATTERNS, so a comment that merely
mentions
+ ``withBindingPolicy`` (no leading dot, no call) is not flagged;
``ConfigBindingPolicy`` in
+ a comment still can match, which is fine -- the prompt is advisory.
+
+ >>> _added_binding_policy_tokens(
+ ... "+++ b/foo\\n"
+ ... "+.withBindingPolicy(X)\\n"
+ ... "-.withBindingPolicy(Y)\\n"
+ ... "+import ...ConfigBindingPolicy\\n"
+ ... "+// see withBindingPolicy usage\\n"
+ ... " context\\n"
+ ... )
+ ['ConfigBindingPolicy', 'withBindingPolicy']
+ >>> _added_binding_policy_tokens("-.withBindingPolicy(X)\\n")
+ []
+ >>> _added_binding_policy_tokens("+// mentions ConfigBindingPolicy in a
comment\\n")
+ ['ConfigBindingPolicy']
+ >>> _added_binding_policy_tokens("")
+ []
+ """
+ found = set()
+ for line in diff.splitlines():
+ if line.startswith("+") and not line.startswith("+++ "):
+ for token, pattern in BINDING_POLICY_PATTERNS:
+ if pattern.search(line):
+ found.add(token)
+ return sorted(found)
+
+
+def maybe_binding_policy_warning(ref: str, base_head: str) -> None:
+ """Warn before pushing to a pre-4.2 `ref` a cherry-pick that adds
ConfigBindingPolicy.
+
+ A cherry-pick onto branch-4.1/4.0/3.x that introduces
``.withBindingPolicy(...)`` or
+ ``ConfigBindingPolicy`` will not compile there (the API arrived in
SPARK-55928, 4.2+).
+ This is a git-diff + grep pre-push check: it diffs `base_head` (the branch
tip before the
+ pick) against HEAD restricted to Scala/Java sources, scans only the added
lines for the
+ precise patterns in BINDING_POLICY_PATTERNS, and on a hit prints the
remedy and asks the
+ committer to confirm the push; declining skips this branch (see
continue_maybe). No-op on
+ branches that carry the API. Only backports are checked: a PR opened
straight against a
+ maintenance branch already ran that branch's CI.
+
+ The diff is forced plain: without --no-color a committer with ``color.ui =
always`` gets
+ added lines behind an ANSI escape, no line starts with "+", and the check
reports nothing
+ while looking like it ran. Like branches_with_merge_footer, a git failure
only warns: this
+ runs after the merge has already been pushed.
+
+ `never` below is how these tests assert "does not prompt": a bare input
mock would return a
+ MagicMock that get_input's loop never accepts, so a regressed guard would
hang the script at
+ startup -- doctest.testmod() runs on every invocation -- instead of
failing.
+
+ >>> from contextlib import redirect_stdout
+ >>> from io import StringIO
+ >>> from unittest.mock import patch
+ >>> never = AssertionError("should not prompt")
+
+ No-op on a branch that carries the API (no diff fetched, no prompt):
+
+ >>> with (
+ ... patch.object(git, "run") as run,
+ ... patch("builtins.input", side_effect=never),
+ ... redirect_stdout(StringIO()),
+ ... ):
+ ... maybe_binding_policy_warning("branch-4.2", "deadbeef")
+ >>> run.call_count
+ 0
+
+ No-op when the change added none of the tokens. The git command is pinned
here because
+ nothing else in these tests would notice it being wrong -- transposing the
two endpoints
+ inverts the whole check, and dropping the pathspec or --no-color silently
defeats it:
+
+ >>> with (
+ ... patch.object(git, "run", return_value="+ unrelated change\\n") as
run,
+ ... patch("builtins.input", side_effect=never),
+ ... redirect_stdout(StringIO()),
+ ... ):
+ ... maybe_binding_policy_warning("branch-4.1", "deadbeef")
+ >>> run.call_args[0][0][:6]
+ ['git', 'diff', '--no-color', '--no-ext-diff', 'deadbeef', 'HEAD']
+ >>> run.call_args[0][0][6:]
+ ['--', ':(top)*.scala', ':(top)*.java']
+
+ Warns and prompts when the change adds a token on a pre-4.2 branch;
accepting continues
+ (no abort), so input is called once by the continue_maybe prompt:
+
+ >>> with (
+ ... patch.object(git, "run", return_value="+.withBindingPolicy(X)\\n"),
+ ... patch("builtins.input", return_value="y") as inp,
+ ... redirect_stdout(StringIO()),
+ ... ):
+ ... maybe_binding_policy_warning("branch-4.1", "deadbeef")
+ >>> inp.call_count
+ 1
+
+ Declining skips just that branch, so an already-pushed branch-M.x pick
+ and the JIRA resolution survive (the second git.run is the
CHERRY_PICK_HEAD probe):
+
+ >>> with (
+ ... patch.object(
+ ... git,
+ ... "run",
+ ... side_effect=[
+ ... "+.withBindingPolicy(X)\\n",
+ ... subprocess.CalledProcessError(1, "git"),
+ ... ],
+ ... ),
+ ... patch("builtins.input", return_value=""),
+ ... redirect_stdout(StringIO()),
+ ... ):
+ ... try:
+ ... maybe_binding_policy_warning("branch-4.1", "deadbeef")
+ ... skipped = False
+ ... except SkipCherryPick:
+ ... skipped = True
+ >>> skipped
+ True
+
+ A git failure warns and scans nothing rather than aborting a half-pushed
merge:
+
+ >>> with (
+ ... patch.object(git, "run", side_effect=OSError("git is sad")),
+ ... patch("builtins.input", side_effect=never),
+ ... redirect_stdout(StringIO()) as out,
+ ... ):
+ ... maybe_binding_policy_warning("branch-4.1", "deadbeef")
+ >>> "git is sad" in out.getvalue()
+ True
+ """
+ if not _is_pre_binding_policy_branch(ref):
+ return
+ try:
+ diff = git.run(
+ ["git", "diff", "--no-color", "--no-ext-diff", base_head, "HEAD",
"--"]
+ + list(BINDING_POLICY_PATHSPECS)
+ )
+ except Exception as e:
+ print_error(
+ "Could not diff %s..HEAD; skipping the ConfigBindingPolicy check:
%s" % (base_head, e)
+ )
+ return
+ tokens = _added_binding_policy_tokens(diff)
+ if not tokens:
+ return
+ print_error(
+ "Warning: what would be pushed to %s adds %s, which does not exist
before branch-4.2 "
+ "(SPARK-55928). Drop the .withBindingPolicy(...) call and the
ConfigBindingPolicy "
+ "import. Leave .version(...) alone -- it names the release the config
shipped in -- "
+ "unless the config itself is new on this branch, in which case
retarget it to this "
+ "branch's next release." % (ref, " / ".join(tokens))
+ )
+ continue_maybe("Push to %s anyway? (experts only!)" % ref, True)
+
+
+# First line of a backport PR body opened by _land_via_fork; the rest is the
pick's message.
+_BACKPORT_MARKER_RE = re.compile(r"^Backport-of: #(\d+)\n\n")
+
+
+def _backport_of(body, head_ref, target_ref):
+ """(original PR, original message, primary author) if this is a backport
PR, else None.
+
+ Needs the marker, a merge footer for that same PR, and the script's own
head branch for
+ this target, so a body that merely quotes a footer, or pastes one into a
PR against
+ master, is not taken for a backport. Mentions are stripped as merge_pr
does for any
+ body, keeping the @ inside the footer's e-mail addresses.
+
+ >>> footer = "Closes #7 from a/b.\\n\\nAuthored-by: A
<[email protected]>\\nSigned-off-by: C <[email protected]>"
+ >>> message = "Fix it.\\n\\n" + footer + "\\n(cherry picked from commit
abc)"
+ >>> crlf = "Backport-of: #7\\r\\n\\r\\n" + message.replace("\\n", "\\r\\n")
+ >>> _backport_of(crlf, "backport-pr7-branch-4.2", "branch-4.2")[::2]
+ (7, 'A <[email protected]>')
+ >>> _backport_of("Backport-of: #7\\n\\nHi @bob.\\n\\n" + message,
"backport-pr7-branch-4.2",
+ ... "branch-4.2")[1].splitlines()[0]
+ 'Hi bob.'
+ >>> [_backport_of(b, h, "branch-4.2") for b, h in (
+ ... ("Backport-of: #8\\n\\n" + message, "backport-pr8-branch-4.2"), #
footer is #7's
+ ... ("Backport-of: #7\\n\\n" + message, "my-feature"), # not the
script's branch
+ ... (message, "backport-pr7-branch-4.2"),
+ ... (None, "backport-pr7-branch-4.2"),
+ ... )]
+ [None, None, None, None]
+
+ The merged commit keeps #7 as its last footer, so #7's merge status finds
the branch:
+
+ >>> merge_footer_pr("[SPARK-1][4.2][SQL] Fix\\n\\n%s\\n\\nBackport-PR: #9"
% message)
Review Comment:
**Nit (P3):** This doctest checks a hand-assembled string, not the message
`merge_pr` writes, and no doctest calls `merge_pr` (its only caller is
`main()`). I tried two mutations on a scratch copy of the head file, and all
406 doctests still passed with each. One appended a `Closes #<pr> from ...`
paragraph after the trailers, which hides the branch from #N's merge status.
The other used `pr_author` instead of `backport[2]` as `--author`, which
misattributes the commit.
A doctest that drives `merge_pr` for a recognized backport PR, with git and
the author prompt mocked, and asserts the `git commit` invocation would pin the
author, the absence of this PR's own footer, and the final trailer block. It
should pin whatever layout the trailer-block fix settles on.
**Verification:**
- **Regression:** Each of those three regressions to the Backport-of arm
makes the new merge_pr doctest fail, and the unmodified arm passes it.
--
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]
