diff --git a/eden/scm/sapling/ext/github/consts/query.py b/eden/scm/sapling/ext/github/consts/query.py index ef111962c8153..5b5a4882d1410 100644 --- a/eden/scm/sapling/ext/github/consts/query.py +++ b/eden/scm/sapling/ext/github/consts/query.py @@ -81,6 +81,22 @@ } """ +# Like GRAPHQL_UPDATE_PULL_REQUEST, but does not touch the base branch. +# GitHub rejects updatePullRequest mutations that include baseRefName for +# pull requests that are part of a native stack (the stack manages base +# branches itself), so this variant is used to update only the title/body. +GRAPHQL_UPDATE_PULL_REQUEST_NO_BASE = """ +mutation ($pullRequestId: ID!, $title: String!, $body: String!) { + updatePullRequest( + input: {pullRequestId: $pullRequestId, title: $title, body: $body} + ) { + pullRequest { + id + } + } +} +""" + GRAPHQL_CREATE_BRANCH = """ mutation ($repositoryId: ID!, $name: String!, $oid: GitObjectID!) { createRef(input: {repositoryId: $repositoryId, name: $name, oid: $oid}) { diff --git a/eden/scm/sapling/ext/github/gh_submit.py b/eden/scm/sapling/ext/github/gh_submit.py index 6004ef4959eec..12a36052b693c 100644 --- a/eden/scm/sapling/ext/github/gh_submit.py +++ b/eden/scm/sapling/ext/github/gh_submit.py @@ -12,14 +12,14 @@ import enum from dataclasses import dataclass -from typing import Dict, Optional, Tuple, Union +from typing import Dict, List, Optional, Tuple, Union from sapling.i18n import _ from sapling.result import Err, Ok, Result from . import github_gh_cli as gh_cli from .consts import query -from .github_gh_cli import JsonDict +from .github_gh_cli import JsonDict, ParamValue from .pullrequest import PullRequestId _Params = Union[str, int, bool] @@ -336,18 +336,28 @@ async def update_pull_request( node_id: str, title: str, body: str, - base: str, + base: Optional[str], ) -> Result[str, str]: """Returns an "ID!" for the pull request, which should match the node_id that was passed in. + + If base is None, the base branch is left untouched. This is required for + pull requests that are part of a native GitHub stack: GitHub rejects + updatePullRequest mutations that include baseRefName for such pull + requests, as the stack manages base branches itself. """ params: Dict[str, _Params] = { - "query": query.GRAPHQL_UPDATE_PULL_REQUEST, + "query": ( + query.GRAPHQL_UPDATE_PULL_REQUEST + if base is not None + else query.GRAPHQL_UPDATE_PULL_REQUEST_NO_BASE + ), "pullRequestId": node_id, "title": title, "body": body, - "base": base, } + if base is not None: + params["base"] = base result = await gh_cli.make_request(params, hostname=hostname) if result.is_err(): return Err(result.unwrap_err()) @@ -408,3 +418,159 @@ async def get_username(hostname: str) -> Result[str, str]: return Err(result.unwrap_err()) else: return Ok(result.unwrap()["data"]["viewer"]["login"]) + + +# Native GitHub "pull request stack" REST endpoints. The stacks API is in +# public preview and requires an explicit API version header: +# https://docs.github.com/en/rest/pulls/stacks +_STACKS_API_HEADERS = {"X-GitHub-Api-Version": "2026-03-10"} + + +@dataclass +class StackDetails: + """A native GitHub pull request stack. + + https://docs.github.com/en/rest/pulls/stacks + """ + + # Number that identifies the stack within the repo. Note that GitHub + # allocates stack numbers and pull request/issue numbers from disjoint + # ranges, so a stack number never collides with a pull request number. + number: int + # URL for the stack. + url: str + # True if the stack is still open. + is_open: bool + # Numbers of the *open* pull requests in the stack, ordered from the + # bottom of the stack (closest to the trunk) to the top. Merged and + # closed pull requests are excluded. + pull_requests: List[int] + + +def _parse_stack_from_dict(stack_obj: JsonDict) -> StackDetails: + """Parses a "Pull Request Stack" object from the REST API. + + Note that merged (and otherwise closed) pull requests are excluded from + `pull_requests`: + + >>> _parse_stack_from_dict({ + ... "id": 1, + ... "number": 7, + ... "node_id": "PRS_1", + ... "url": "https://api.github.com/repos/facebook/sapling/stacks/7", + ... "open": True, + ... "base": {"ref": "main"}, + ... "created_at": "2026-07-30T00:00:00Z", + ... "pull_requests": [ + ... {"number": 101, "state": "closed", + ... "merged_at": "2026-07-30T01:00:00Z", "draft": False, + ... "head": {"ref": "pr101", "sha": "0" * 40}}, + ... {"number": 102, "state": "open", "merged_at": None, + ... "draft": False, "head": {"ref": "pr102", "sha": "1" * 40}}, + ... {"number": 103, "state": "open", "merged_at": None, + ... "draft": True, "head": {"ref": "pr103", "sha": "2" * 40}}, + ... ], + ... }) + StackDetails(number=7, url='https://api.github.com/repos/facebook/sapling/stacks/7', is_open=True, pull_requests=[102, 103]) + """ + return StackDetails( + number=stack_obj["number"], + url=stack_obj["url"], + is_open=stack_obj["open"], + pull_requests=[ + pr["number"] for pr in stack_obj["pull_requests"] if pr["state"] == "open" + ], + ) + + +async def get_stack_for_pull_request( + hostname: str, owner: str, name: str, number: int +) -> Result[Optional[StackDetails], str]: + """Returns the stack containing the specified pull request, or None if the + pull request is not part of a stack. + """ + endpoint = f"repos/{owner}/{name}/stacks?pull_request={number}" + result = await gh_cli.make_request( + {}, hostname=hostname, endpoint=endpoint, headers=_STACKS_API_HEADERS + ) + if result.is_err(): + return Err(result.unwrap_err()) + + # The response is a JSON array of stacks. Because a pull request can be in + # at most one stack, the `pull_request` filter yields at most one entry. + stacks = result.unwrap() + if not stacks: + return Ok(None) + return Ok(_parse_stack_from_dict(stacks[0])) + + +async def create_stack( + hostname: str, owner: str, name: str, pr_numbers: List[int] +) -> Result[StackDetails, str]: + """Creates a native GitHub stack from the specified pull requests. + + `pr_numbers` must be ordered from the bottom of the stack to the top: the + bottom pull request's base must be the trunk, and each subsequent pull + request's base branch must match the head branch of the one below it. The + caller is responsible for having set up the base branches accordingly. + """ + endpoint = f"repos/{owner}/{name}/stacks" + params: Dict[str, ParamValue] = {"pull_requests": pr_numbers} + result = await gh_cli.make_request( + params, + hostname=hostname, + endpoint=endpoint, + method="POST", + headers=_STACKS_API_HEADERS, + ) + if result.is_err(): + return Err(result.unwrap_err()) + return Ok(_parse_stack_from_dict(result.unwrap())) + + +async def add_prs_to_stack( + hostname: str, owner: str, name: str, stack_number: int, pr_numbers: List[int] +) -> Result[StackDetails, str]: + """Appends pull requests onto the top of an existing stack. + + `pr_numbers` must contain only the pull requests to add, ordered from the + current top of the stack upward: the first one's base branch must match + the head branch of the stack's current top pull request. + """ + endpoint = f"repos/{owner}/{name}/stacks/{stack_number}/add" + params: Dict[str, ParamValue] = {"pull_requests": pr_numbers} + result = await gh_cli.make_request( + params, + hostname=hostname, + endpoint=endpoint, + method="POST", + headers=_STACKS_API_HEADERS, + ) + if result.is_err(): + return Err(result.unwrap_err()) + return Ok(_parse_stack_from_dict(result.unwrap())) + + +async def unstack( + hostname: str, owner: str, name: str, stack_number: int +) -> Result[Optional[StackDetails], str]: + """Removes the unmerged pull requests from a stack. + + Pull requests that cannot be unstacked (e.g., merged or queued for merge) + are left in place. Returns the updated stack if pull requests remain in + it; returns None if the stack was dissolved entirely (HTTP 204). + """ + endpoint = f"repos/{owner}/{name}/stacks/{stack_number}/unstack" + result = await gh_cli.make_request( + {}, + hostname=hostname, + endpoint=endpoint, + method="POST", + headers=_STACKS_API_HEADERS, + ) + if result.is_err(): + return Err(result.unwrap_err()) + data = result.unwrap() + if not data: + return Ok(None) + return Ok(_parse_stack_from_dict(data)) diff --git a/eden/scm/sapling/ext/github/github_gh_cli.py b/eden/scm/sapling/ext/github/github_gh_cli.py index d87248f6c8dea..a50fde86c81e6 100644 --- a/eden/scm/sapling/ext/github/github_gh_cli.py +++ b/eden/scm/sapling/ext/github/github_gh_cli.py @@ -16,32 +16,43 @@ JsonDict = Dict[str, Any] +# Scalar value that can be passed as a field to `gh api`. +_ScalarParam = Union[str, int, bool] +# `gh api` also supports array fields via repeated `key[]=value` args. +ParamValue = Union[_ScalarParam, List[_ScalarParam]] + async def make_request( - params: Dict[str, Union[str, int, bool]], + params: Dict[str, ParamValue], hostname: str, endpoint="graphql", method: Optional[str] = None, + headers: Optional[Dict[str, str]] = None, ) -> Result[JsonDict, str]: """If successful, returns a Result whose value is parsed JSON returned by the request. """ - return await _make_request(params, hostname, endpoint, method) + return await _make_request(params, hostname, endpoint, method, headers) # Unexported extension/mock point. async def _make_request( - params: Dict[str, Union[str, int, bool]], + params: Dict[str, ParamValue], hostname: str, endpoint: str, method: Optional[str], + headers: Optional[Dict[str, str]] = None, ) -> Result[JsonDict, str]: if method: endpoint_args = ["-X", method.upper(), endpoint] else: endpoint_args = [endpoint] + header_args = list( + itertools.chain(*[["-H", f"{k}: {v}"] for (k, v) in (headers or {}).items()]) + ) args = ( ["gh", "api", "--hostname", hostname] + + header_args + endpoint_args + list(itertools.chain(*[_format_param(k, v) for (k, v) in params.items()])) ) @@ -68,7 +79,13 @@ async def _make_request( response = None if proc.returncode == 0: - assert response is not None + if response is None: + # Some REST endpoints return "204 No Content" on success (e.g., + # dissolving a pull request stack), in which case `gh api` prints + # no JSON to parse. + if not stdout.strip(): + return Ok({}) + return Err(f"could not parse JSON from response: {stdout.decode()}") assert "errors" not in response return Ok(response) elif response is not None: @@ -82,7 +99,38 @@ async def _make_request( ) -def _format_param(key: str, value: Union[str, int, bool]) -> List[str]: +def _format_param(key: str, value: ParamValue) -> List[str]: + r"""Formats a param as a list of arguments to pass to `gh api`. + + >>> _format_param("body", "hello") + ['-f', 'body=hello'] + >>> _format_param("number", 42) + ['-F', 'number=42'] + >>> _format_param("draft", True) + ['-F', 'draft=true'] + + Array values use the `gh api` repeated-field syntax, e.g. + `-F "pull_requests[]=101" -F "pull_requests[]=102"`: + + >>> _format_param("pull_requests", [101, 102]) + ['-F', 'pull_requests[]=101', '-F', 'pull_requests[]=102'] + >>> _format_param("labels", ["bug", "help wanted"]) + ['-f', 'labels[]=bug', '-f', 'labels[]=help wanted'] + + An empty array is passed explicitly as `key[]` without a value (per the + `gh api` manual) rather than dropped: an empty array and a missing field + can have different meanings to an endpoint: + + >>> _format_param("empty", []) + ['-F', 'empty[]'] + """ + if isinstance(value, list): + if not value: + return ["-F", f"{key}[]"] + return list( + itertools.chain(*[_format_param(f"{key}[]", v) for v in value]) + ) + # In Python, bool is a subclass of int, so check it first. if isinstance(value, bool): opt = "-F" diff --git a/eden/scm/sapling/ext/github/mock_utils.py b/eden/scm/sapling/ext/github/mock_utils.py index added9869489b..06e31e5452888 100644 --- a/eden/scm/sapling/ext/github/mock_utils.py +++ b/eden/scm/sapling/ext/github/mock_utils.py @@ -28,8 +28,11 @@ REPO_ID = "R_test_github_repo" USER_NAME = "facebook_username" -ParamsType = Dict[str, Union[bool, int, str]] -MakeRequestType = Callable[[ParamsType, str, str, Optional[str]], Result[JsonDict, str]] +ParamsType = Dict[str, Union[bool, int, str, List[bool], List[int], List[str]]] +MakeRequestType = Callable[ + [ParamsType, str, str, Optional[str], Optional[Dict[str, str]]], + Result[JsonDict, str], +] RunGitCommandType = Callable[[List[str], str], bytes] @@ -75,6 +78,7 @@ class MockGitHubServer: def __init__(self, hostname: str = GITHUB_HOSTNAME): self.hostname: str = hostname self.requests: Dict[str, MockRequest] = {} + self._consumed_keys: set = set() async def make_request( self, @@ -83,10 +87,15 @@ async def make_request( hostname: str, endpoint: str = "graphql", method: Optional[str] = None, + headers: Optional[Dict[str, str]] = None, ) -> Result[JsonDict, str]: """Wrapper function for `github_gh_cli.make_request`. It reads mock data from `self.requests` instead of sending network requests. + + Note that `headers` is intentionally not part of the request key: the + headers we send (e.g., X-GitHub-Api-Version) do not affect which mock + response should be returned. """ assert real_make_request.__name__ == "_make_request", ( f"expected '_make_request', but got '{real_make_request.__name__}'" @@ -96,11 +105,45 @@ async def make_request( if key not in self.requests: raise MockRequestNotFound(key, self.requests) + self._consumed_keys.add(key) return self.requests[key].get_response() def _add_request(self, request_key: str, request: "MockRequest") -> None: self.requests[request_key] = request + def unconsumed_requests(self) -> List[str]: + """Keys of expectations that were never requested.""" + return sorted(k for k in self.requests if k not in self._consumed_keys) + + def report_unconsumed(self, ui) -> None: + """Prints a warning for every expectation that was never requested. + + Intended to be called after the command under test has finished (see + wrap_with_consumption_check). Tests that use this produce no extra + output when all expectations were consumed, so any warning makes the + test fail: this catches code paths that silently stopped making a + request the test author expected. + """ + for key in self.unconsumed_requests(): + first_line = key.splitlines()[0] + ui.status_err(f"warning, unconsumed mock request: {first_line}\n") + + +def wrap_with_consumption_check(server: "MockGitHubServer", module, funcname) -> None: + """Wraps `module.funcname` (a command function taking `ui` as its first + argument) so that unconsumed mock expectations are reported after the + command finishes, even if it aborts. + """ + from sapling import extensions + + def wrapped(orig, ui, *args, **kwargs): + try: + return orig(ui, *args, **kwargs) + finally: + server.report_unconsumed(ui) + + extensions.wrapfunction(module, funcname, wrapped) + def expect_get_repository_request( self, owner: str = OWNER, name: str = REPO_NAME ) -> "GetRepositoryRequest": diff --git a/eden/scm/sapling/ext/github/pull_request_body.py b/eden/scm/sapling/ext/github/pull_request_body.py index 46317b281e765..d0b0788b5e6a5 100644 --- a/eden/scm/sapling/ext/github/pull_request_body.py +++ b/eden/scm/sapling/ext/github/pull_request_body.py @@ -18,6 +18,7 @@ def create_pull_request_title_and_body( pr_numbers_index: int, repository: Repository, reviewstack: bool = True, + stack_list: bool = True, ) -> Tuple[str, str]: r"""Returns (title, body) for the pull request. @@ -84,6 +85,18 @@ def create_pull_request_title_and_body( * __->__ #42 * #4 + Disable the stack list entirely (used for the native "stacked" workflow, + where GitHub renders the stack in the pull request UI itself). Note that + this also suppresses the ReviewStack link: + >>> title, body = create_pull_request_title_and_body(commit_msg, pr_numbers_and_num_commits, + ... pr_numbers_index, contributor_repo, stack_list=False) + >>> print(title) + The original commit message. + >>> print(body) + Second line of message. + + + Single commit stack: >>> title, body = create_pull_request_title_and_body("Foo", [(1, 1)], 0, contributor_repo) >>> print(title) @@ -106,7 +119,7 @@ def create_pull_request_title_and_body( body = _strip_stack_information(body) extra = [] - if len(pr_numbers_and_num_commits) > 1: + if stack_list and len(pr_numbers_and_num_commits) > 1: if reviewstack: reviewstack_url = f"https://reviewstack.dev/{owner}/{name}/pull/{pr}" review_stack_message = f"Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack]({reviewstack_url})." diff --git a/eden/scm/sapling/ext/github/submit.py b/eden/scm/sapling/ext/github/submit.py index 423bdaff16bcf..89d838cccc2c0 100644 --- a/eden/scm/sapling/ext/github/submit.py +++ b/eden/scm/sapling/ext/github/submit.py @@ -70,6 +70,19 @@ class SubmitWorkflow(Enum): """ OVERLAP = "overlap" + """Like SINGLE, but additionally links the pull requests together using + GitHub's native "stacked pull requests" feature so GitHub renders the + stack natively and can merge/retarget it bottom-up: + https://docs.github.com/en/pull-requests/get-started/about-stacked-prs + """ + STACKED = "stacked" + + def uses_chained_bases(self) -> bool: + """Whether each PR in the stack uses the head branch of the PR below + it as its base branch (as opposed to all PRs sharing a common base). + """ + return self in (SubmitWorkflow.SINGLE, SubmitWorkflow.STACKED) + @staticmethod def from_config(ui) -> "SubmitWorkflow": workflow = ui.config( @@ -80,6 +93,8 @@ def from_config(ui) -> "SubmitWorkflow": return SubmitWorkflow.OVERLAP elif workflow == "single": return SubmitWorkflow.SINGLE + elif workflow == "stacked": + return SubmitWorkflow.STACKED else: # Note that "classic" is not recognized yet. ui.warn( @@ -212,13 +227,14 @@ def get_gitdir() -> str: repository = params.repository - # For the SINGLE workflow, we must update the base branch on existing PRs - # BEFORE pushing the new branch contents. Otherwise, when commits are - # reordered in the stack, GitHub may see that a PR's commits already exist - # in its (old) base branch and auto-close the PR as "merged". + # For workflows with chained base branches (SINGLE, STACKED), we must + # update the base branch on existing PRs BEFORE pushing the new branch + # contents. Otherwise, when commits are reordered in the stack, GitHub may + # see that a PR's commits already exist in its (old) base branch and + # auto-close the PR as "merged". # # See https://github.com/facebook/sapling/issues/1275 - if workflow == SubmitWorkflow.SINGLE: + if workflow.uses_chained_bases(): existing_prs = [ p for p in partitions if p[0].pr and p[0].pr.state == PullRequestState.OPEN ] @@ -235,8 +251,17 @@ def get_gitdir() -> str: if not pr or pr.state != PullRequestState.OPEN: continue base = repository.get_base_branch() - if index < len(partitions) - 1: - base = none_throws(partitions[index + 1][0].head_branch_name) + # Chain to the nearest partition below whose pull request is + # open (or that will get a new pull request). Closed/merged + # pull requests are skipped: using their head branches as + # bases would break the chain. + for below in partitions[index + 1 :]: + below_head = below[0] + below_pr = below_head.pr + if below_pr and below_pr.state != PullRequestState.OPEN: + continue + base = none_throws(below_head.head_branch_name) + break result = await gh_submit.update_pull_request( repository.hostname, pr.node_id, pr.title, pr.body, base ) @@ -327,8 +352,19 @@ async def rewrite_pull_request_body( # stack to the bottom. partition = partitions[index] base = repository.get_base_branch() - if workflow == SubmitWorkflow.SINGLE and index < len(partitions) - 1: - base = none_throws(partitions[index + 1][0].head_branch_name) + if workflow.uses_chained_bases() and not repository.is_fork: + # Chain to the nearest partition below whose pull request is open (or + # new). Closed/merged pull requests are skipped: using their head + # branches as bases would break the chain. For forks, chained bases + # are not possible at all (the head branches live on the fork, but a + # base branch must be a branch on the upstream repository), so the + # default base branch is kept. + for below in partitions[index + 1 :]: + below_pr = below[0].pr + if below_pr and below_pr.state != PullRequestState.OPEN: + continue + base = none_throws(below[0].head_branch_name) + break head_commit_data = partition[0] @@ -347,6 +383,10 @@ async def rewrite_pull_request_body( index, repository, reviewstack=ui.configbool("github", "pull-request-include-reviewstack"), + # For the native "stacked" workflow, GitHub renders the stack in the + # pull request UI itself, so the footer stack list (and ReviewStack + # link) would be redundant. + stack_list=workflow != SubmitWorkflow.STACKED, ) if pr.state != PullRequestState.OPEN: @@ -509,7 +549,7 @@ async def create_pull_requests_serially( parent = None for commit, branch_name in commits: base = repository.get_base_branch() - if workflow == SubmitWorkflow.SINGLE and parent: + if workflow.uses_chained_bases() and parent: base = none_throws(parent.head_branch_name) commit_msg = commit.get_msg() @@ -599,7 +639,11 @@ async def create_placeholder_strategy_params( commit=commit, parent=parent_commit ) commits_that_need_pull_requests.append(commit_needs_pr) - parent_commit = commit + if not pr or pr.state == PullRequestState.OPEN: + # Only open pull requests (or commits that will get a new pull + # request) can serve as the parent for chained bases: + # closed/merged head branches would break the chain. + parent_commit = commit # Reserve one GitHub issue number for each pull request (in parallel) and # then assign them in increasing order. Also ensure head_branch_name is set @@ -657,8 +701,11 @@ async def create_pull_request(params: PullRequestParams): issue_number = params.number # Note that "overlapping" pull requests will all share the same base. + # For forks, chained bases are not possible either: the head branches + # live on the fork, but the base branch of a pull request must be a + # branch on the upstream repository. base = base_branch_for_repo - if workflow == SubmitWorkflow.SINGLE: + if workflow.uses_chained_bases() and not repository.is_fork: parent = params.parent if parent: base = none_throws(parent.head_branch_name) diff --git a/eden/scm/tests/github/mock_closed_mid_stack.py b/eden/scm/tests/github/mock_closed_mid_stack.py new file mode 100644 index 0000000000000..284cb55beb0d6 --- /dev/null +++ b/eden/scm/tests/github/mock_closed_mid_stack.py @@ -0,0 +1,76 @@ +# Copyright (c) Meta Platforms, Inc. and affiliates. +# +# This software may be used and distributed according to the terms of the +# GNU General Public License version 2. + +from sapling import extensions, scmutil +from sapling.ext.github import github_gh_cli, submit +from sapling.ext.github.gh_submit import PullRequestState +from sapling.ext.github.mock_utils import ( + mock_run_git_command, + MockGitHubServer, + wrap_with_consumption_check, +) + +# An extension to mock network requests for `sl pr submit` with +# github.pr-workflow=single when the pull request at the bottom of the stack +# (#42) is closed: the base branches of the pull requests above it must skip +# the closed pull request's head branch (which would break the chain) and +# fall through to the default base branch. +# +# This mock is set up in `reposetup` so that expectations can be derived from +# the actual commit hashes in the test repo instead of hardcoding them. + + +def setup_mock_github_server(repo) -> MockGitHubServer: + github_server = MockGitHubServer() + + github_server.expect_get_repository_request().and_respond() + + # All three commits are linked to pull requests via "Pull Request + # resolved" lines in their commit messages. None of the head OIDs match + # the local commits, so all three head branches are pushed (pushing to a + # closed pull request's branch is harmless and preexisting behavior). + github_server.expect_get_pr_details_request(42).and_respond( + "PR_id_42", state=PullRequestState.CLOSED + ) + github_server.expect_get_pr_details_request(43).and_respond("PR_id_43") + github_server.expect_get_pr_details_request(44).and_respond("PR_id_44") + + # Base updates before the push: #44 chains to the open #43 below it, but + # #43 must NOT chain to the closed #42 below it: it falls through to the + # default base branch instead. (No base update is attempted for the + # closed #42 itself.) + github_server.expect_update_pr_request( + "PR_id_44", 44, "", base="pr43" + ).and_respond() + github_server.expect_update_pr_request( + "PR_id_43", 43, "", base="main" + ).and_respond() + + # Body rewrites follow the same base rules. The stack list footer still + # lists all three pull requests (including the closed one). + msg_two = "two\n\nPull Request resolved: https://github.com/facebook/test_github_repo/pull/43" + msg_three = "three\n\nPull Request resolved: https://github.com/facebook/test_github_repo/pull/44" + github_server.expect_update_pr_request( + "PR_id_44", 44, msg_three, base="pr43", stack_pr_ids=[42, 43, 44] + ).and_respond() + github_server.expect_update_pr_request( + "PR_id_43", 43, msg_two, base="main", stack_pr_ids=[42, 43, 44] + ).and_respond() + + github_server.expect_get_username_request().and_respond() + + tip = scmutil.revsingle(repo, "desc(three)").hex() + github_server.expect_merge_into_branch(tip).and_respond() + + return github_server + + +def reposetup(ui, repo): + github_server = setup_mock_github_server(repo) + extensions.wrapfunction( + github_gh_cli, "_make_request", github_server.make_request + ) + extensions.wrapfunction(submit, "run_git_command", mock_run_git_command) + wrap_with_consumption_check(github_server, submit, "submit") diff --git a/eden/scm/tests/github/mock_create_prs.py b/eden/scm/tests/github/mock_create_prs.py index 2bd5cd653b503..7979f4bd9d7f6 100644 --- a/eden/scm/tests/github/mock_create_prs.py +++ b/eden/scm/tests/github/mock_create_prs.py @@ -26,14 +26,19 @@ def setup_mock_github_server(ui) -> MockGitHubServer: (43, "two\n"), ] - single = ui.config("github", "pr-workflow") == "single" + # Both "single" and "stacked" chain each PR's base to the PR below it. + workflow = ui.config("github", "pr-workflow") + chained = workflow in ("single", "stacked") + # The "stacked" workflow omits the stack list footer from PR bodies + # because GitHub renders the stack natively. + stacked = workflow == "stacked" for idx, (num, msg) in enumerate(prs): title, body = title_and_body(msg) head = f"pr{num}" base = "main" - if single and idx > 0: + if chained and idx > 0: base = "pr%d" % prs[idx - 1][0] github_server.expect_create_pr_request( @@ -47,7 +52,11 @@ def setup_mock_github_server(ui) -> MockGitHubServer: github_server.expect_get_pr_details_request(num).and_respond(pr_id) github_server.expect_update_pr_request( - pr_id, num, msg, base=base, stack_pr_ids=[pr[0] for pr in prs] + pr_id, + num, + msg, + base=base, + stack_pr_ids=None if stacked else [pr[0] for pr in prs], ).and_respond() github_server.expect_get_username_request().and_respond() diff --git a/eden/scm/tests/github/mock_create_prs_placeholder_fork.py b/eden/scm/tests/github/mock_create_prs_placeholder_fork.py new file mode 100644 index 0000000000000..281263a3790a5 --- /dev/null +++ b/eden/scm/tests/github/mock_create_prs_placeholder_fork.py @@ -0,0 +1,89 @@ +# Copyright (c) Meta Platforms, Inc. and affiliates. +# +# This software may be used and distributed according to the terms of the +# GNU General Public License version 2. + +from sapling import extensions, scmutil +from sapling.ext.github import github_gh_cli, submit +from sapling.ext.github.mock_utils import ( + mock_run_git_command, + MockGitHubServer, + wrap_with_consumption_check, +) +from sapling.ext.github.pull_request_body import title_and_body + +# An extension to mock network requests for `sl pr submit` with +# github.placeholder-strategy=true and github.pr-workflow=single when the +# repo is a fork. Chained bases are not possible for forks (the head branches +# live on the fork, but a pull request's base branch must be a branch on the +# upstream repository), so both new pull requests must be created against the +# upstream default branch, and their body rewrites must not attempt to change +# the base to a fork branch. + +UPSTREAM = { + "id": "R_upstream_repo", + "owner": {"id": "upstream_id", "login": "upstream"}, + "name": "test_github_repo", + "isFork": False, + "defaultBranchRef": {"name": "main"}, +} + + +def setup_mock_github_server(repo) -> MockGitHubServer: + github_server = MockGitHubServer() + + github_server.expect_get_repository_request().and_respond( + is_fork=True, parent=UPSTREAM + ) + + # Placeholder issues are reserved on the upstream repository. + github_server.expect_create_pr_placeholder_request( + owner="upstream" + ).and_respond(start_number=42, num_times=2) + + prs = [ + (42, "one\n"), + (43, "two\n"), + ] + for num, msg in prs: + _title, body = title_and_body(msg) + # Despite the "single" workflow, both pull requests use the upstream + # default branch as the base: fork head branches cannot be bases. + github_server.expect_create_pr_using_placeholder_request( + body=body, + issue=num, + head=f"facebook:pr{num}", + base="main", + owner="upstream", + ).and_respond() + + pr_id = f"PR_id_{num}" + github_server.expect_get_pr_details_request( + num, owner="upstream" + ).and_respond(pr_id) + + # The body rewrite keeps the default base branch (no chaining). + github_server.expect_update_pr_request( + pr_id, + num, + msg, + base="main", + owner="upstream", + stack_pr_ids=[42, 43], + ).and_respond() + + github_server.expect_get_username_request().and_respond() + + tip = scmutil.revsingle(repo, "desc(two)").hex() + github_server.expect_merge_into_branch(tip).and_respond() + + return github_server + + +def reposetup(ui, repo): + github_server = setup_mock_github_server(repo) + extensions.wrapfunction( + github_gh_cli, "_make_request", github_server.make_request + ) + extensions.wrapfunction(submit, "run_git_command", mock_run_git_command) + wrap_with_consumption_check(github_server, submit, "submit") diff --git a/eden/scm/tests/github/mock_create_prs_with_open.py b/eden/scm/tests/github/mock_create_prs_with_open.py index 443871249aa37..4b853e23d6456 100644 --- a/eden/scm/tests/github/mock_create_prs_with_open.py +++ b/eden/scm/tests/github/mock_create_prs_with_open.py @@ -28,14 +28,19 @@ def setup_mock_github_server(ui) -> MockGitHubServer: (43, "two\n"), ] - single = ui.config("github", "pr-workflow") == "single" + # Both "single" and "stacked" chain each PR's base to the PR below it. + workflow = ui.config("github", "pr-workflow") + chained = workflow in ("single", "stacked") + # The "stacked" workflow omits the stack list footer from PR bodies + # because GitHub renders the stack natively. + stacked = workflow == "stacked" for idx, (num, msg) in enumerate(prs): title, body = title_and_body(msg) head = f"pr{num}" base = "main" - if single and idx > 0: + if chained and idx > 0: base = "pr%d" % prs[idx - 1][0] github_server.expect_create_pr_request( @@ -49,7 +54,11 @@ def setup_mock_github_server(ui) -> MockGitHubServer: github_server.expect_get_pr_details_request(num).and_respond(pr_id) github_server.expect_update_pr_request( - pr_id, num, msg, base=base, stack_pr_ids=[pr[0] for pr in prs] + pr_id, + num, + msg, + base=base, + stack_pr_ids=None if stacked else [pr[0] for pr in prs], ).and_respond() github_server.expect_get_username_request().and_respond() diff --git a/eden/scm/tests/test-doctest.py b/eden/scm/tests/test-doctest.py index e0f5d40b354df..d8e4ac5cb92f5 100644 --- a/eden/scm/tests/test-doctest.py +++ b/eden/scm/tests/test-doctest.py @@ -32,6 +32,8 @@ def testmod(name, optionflags=0, testtarget=None): testmod("sapling.pathlog") testmod("sapling.ext.github.archive_commit") +testmod("sapling.ext.github.gh_submit") +testmod("sapling.ext.github.github_gh_cli") testmod("sapling.ext.github.github_repo_util") testmod("sapling.ext.github.pr_parser") testmod("sapling.ext.github.pull_request_arg") diff --git a/eden/scm/tests/test-ext-github-pr-submit-closed.t b/eden/scm/tests/test-ext-github-pr-submit-closed.t index cf4244c465667..aad56fd40716b 100644 --- a/eden/scm/tests/test-ext-github-pr-submit-closed.t +++ b/eden/scm/tests/test-ext-github-pr-submit-closed.t @@ -19,3 +19,25 @@ test we don't try updating a closed pr: pushing 1 to https://github.com/facebook/test_github_repo.git warning, not updating #42 because it isn't open hint[unlink-closed-pr]: to create a new PR, disassociate commit(s) using 'sl pr unlink' then re-run 'sl pr submit' + +test chained bases skip a closed pull request: two open pull requests are +stacked on top of the closed #42. #43 must NOT use the closed #42's head +branch as its base (that would break the chain); it falls through to the +default base branch instead. + + $ echo b > b1 + $ sl ci -Aqm "two + > + > Pull Request resolved: https://github.com/facebook/test_github_repo/pull/43" + $ echo c > c1 + $ sl ci -Aqm "three + > + > Pull Request resolved: https://github.com/facebook/test_github_repo/pull/44" + $ sl pr submit --config extensions.pr_submit=$TESTDIR/github/mock_closed_mid_stack.py + updated base for https://github.com/facebook/test_github_repo/pull/44 + updated base for https://github.com/facebook/test_github_repo/pull/43 + pushing 3 to https://github.com/facebook/test_github_repo.git + updated body for https://github.com/facebook/test_github_repo/pull/44 + updated body for https://github.com/facebook/test_github_repo/pull/43 + warning, not updating #42 because it isn't open + hint[unlink-closed-pr]: to create a new PR, disassociate commit(s) using 'sl pr unlink' then re-run 'sl pr submit' diff --git a/eden/scm/tests/test-ext-github-pr-submit-placeholder-issue.t b/eden/scm/tests/test-ext-github-pr-submit-placeholder-issue.t index 8a60df92c32a3..115fa0f01fbb3 100644 --- a/eden/scm/tests/test-ext-github-pr-submit-placeholder-issue.t +++ b/eden/scm/tests/test-ext-github-pr-submit-placeholder-issue.t @@ -23,3 +23,23 @@ test sending pr pushing 1 to https://github.com/facebook/test_github_repo.git created new pull request: https://github.com/facebook/test_github_repo/pull/1 updated body for https://github.com/facebook/test_github_repo/pull/1 + +test the placeholder strategy with a chained-bases workflow on a fork: fork +head branches cannot be used as base branches on the upstream repository, so +both pull requests are created against the upstream default branch + + $ cd .. + $ sl init --git repo2 + $ cd repo2 + $ setconfig github.placeholder-strategy=True + $ setconfig github.pr-workflow=single + $ echo a > a1 + $ sl ci -Aqm one + $ echo a >> a1 + $ sl ci -Aqm two + $ sl pr submit --config extensions.pr_submit=$TESTDIR/github/mock_create_prs_placeholder_fork.py + pushing 2 to https://github.com/facebook/test_github_repo.git + created new pull request: https://github.com/upstream/test_github_repo/pull/42 + created new pull request: https://github.com/upstream/test_github_repo/pull/43 + updated body for https://github.com/upstream/test_github_repo/pull/43 + updated body for https://github.com/upstream/test_github_repo/pull/42 diff --git a/eden/scm/tests/test-ext-github-pr-submit-stacked.t b/eden/scm/tests/test-ext-github-pr-submit-stacked.t new file mode 100644 index 0000000000000..e993739e0f532 --- /dev/null +++ b/eden/scm/tests/test-ext-github-pr-submit-stacked.t @@ -0,0 +1,29 @@ +#require git no-eden no-windows + + $ eagerepo + $ enable github + $ export SL_TEST_GH_URL=https://github.com/facebook/test_github_repo.git + $ . $TESTDIR/git.sh + $ configure github.pr-workflow=stacked + +build up a github repo + + $ sl init --git repo1 + $ cd repo1 + $ echo a > a1 + $ sl ci -Aqm one + $ echo a >> a1 + $ sl ci -Aqm two + +confirm it is a 'github_repo' + $ sl log -r. -T '{github_repo}\n' + True + +test sending pr: each PR's base should be chained to the PR below it, same as +the "single" workflow (native stack linking is tested separately) + $ sl pr submit --config extensions.pr_submit=$TESTDIR/github/mock_create_prs.py + pushing 2 to https://github.com/facebook/test_github_repo.git + created new pull request: https://github.com/facebook/test_github_repo/pull/42 + created new pull request: https://github.com/facebook/test_github_repo/pull/43 + updated body for https://github.com/facebook/test_github_repo/pull/43 + updated body for https://github.com/facebook/test_github_repo/pull/42