diff --git a/eden/scm/sapling/ext/github/__init__.py b/eden/scm/sapling/ext/github/__init__.py index fb2ca7a802722..0df030b205b79 100644 --- a/eden/scm/sapling/ext/github/__init__.py +++ b/eden/scm/sapling/ext/github/__init__.py @@ -50,6 +50,13 @@ def unlink_closed_pr_hint() -> str: ) +@hint("pr-submit-restack") +def pr_submit_restack_hint() -> str: + return _( + "use '@prog@ pr submit --restack' to dissolve the stack on GitHub and recreate it to match your local stack" + ) + + def reposetup(ui, repo): ui.setconfig("hooks", "post-pull.prmarker", pr_marker.cleanup_landed_pr_hook) @@ -99,6 +106,15 @@ def pull_request_command(ui, repo, *args, **opts): ("m", "message", None, _("message describing changes to updated commits")), ("d", "draft", False, _("mark new pull requests as draft")), ("o", "open", False, _("open pull requests in browser after creation")), + ( + "", + "restack", + False, + _( + "with github.pr-workflow=stacked, dissolve and recreate the " + "stack on GitHub if it has diverged from the local stack" + ), + ), ], ) def submit_cmd(ui, repo, *args, **opts): @@ -111,6 +127,25 @@ def submit_cmd(ui, repo, *args, **opts): ``default`` is a fork, they will be created against default's upstream repository. + The ``github.pr-workflow`` config option controls how a stack of + commits is mapped onto pull requests: + + - ``overlap`` (default): every pull request targets the repository's + default branch, so each pull request contains its commit plus all + commits below it in the stack. + + - ``single``: each pull request contains a single commit and targets + the head branch of the pull request below it in the stack. + + - ``stacked``: like ``single``, but the pull requests are also linked + together using GitHub's native stacked pull requests feature, so + GitHub renders the stack in the pull request UI and can merge it + from the bottom up. If the stack on GitHub has diverged from your + local stack (e.g., commits were reordered or removed), it is not + modified unless ``--restack`` is passed, which dissolves the stack + on GitHub and recreates it to match your local stack. Note that + GitHub does not support stacks across forks. + Returns 0 on success. """ return submit.submit(ui, repo, *args, **opts) 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/import_pull_request.py b/eden/scm/sapling/ext/github/import_pull_request.py index 324a348abf04b..a3240bed18cc6 100644 --- a/eden/scm/sapling/ext/github/import_pull_request.py +++ b/eden/scm/sapling/ext/github/import_pull_request.py @@ -4,7 +4,7 @@ # GNU General Public License version 2. import asyncio -from typing import Optional +from typing import List, Optional from sapling import error from sapling.hg import updatetotally @@ -12,7 +12,7 @@ from sapling.node import bin from .consts.stackheader import STACK_HEADER_PREFIX as GHSTACK_HEADER_PREFIX -from .gh_submit import get_pull_request_details +from .gh_submit import get_pull_request_details, get_stack_for_pull_request from .pull_request_arg import parse_pull_request_arg from .pull_request_body import parse_stack_information from .pullrequest import PullRequestId @@ -102,12 +102,31 @@ async def _get_pr(ui, repo, pr_id: PullRequestId, is_goto: bool) -> None: ) else: - ui.warn( - _( - "No stack information found in the pull request body.\n" - "Ancestors will not be linked to pull requests.\n" + # The body has no Sapling stack list footer, but the pull request may + # be part of a native GitHub stack (e.g., if it was created with + # github.pr-workflow=stacked, which omits the footer). + ancestor_numbers = await _get_stack_ancestors_from_github(ui, pr_id) + if ancestor_numbers is None: + ui.warn( + _( + "No stack information found in the pull request body.\n" + "Ancestors will not be linked to pull requests.\n" + ) ) - ) + elif ancestor_numbers: + unlinked_ancestors = list( + filter( + lambda pr: pr is not None, + await asyncio.gather( + *[ + _link_pull_request(store, pr_id, number) + for number in ancestor_numbers + ] + ), + ) + ) + # else: pr_id is the bottom of a native stack, so there are no + # ancestors to link. if is_goto: updatetotally(ui, repo, head_oid_node, None) @@ -125,6 +144,48 @@ async def _get_pr(ui, repo, pr_id: PullRequestId, is_goto: bool) -> None: ) +async def _get_stack_ancestors_from_github( + ui, pr_id: PullRequestId +) -> Optional[List[int]]: + """Queries GitHub's native "stacked pull requests" API for the stack + containing pr_id. + + Returns None if the pull request is not part of a native stack. + Otherwise, returns the numbers of the pull requests below pr_id in the + stack, ordered from the one directly below pr_id downward (which may be + empty if pr_id is the bottom of the stack). + """ + result = await get_stack_for_pull_request( + pr_id.get_hostname(), pr_id.owner, pr_id.name, pr_id.number + ) + if result.is_err(): + ui.warn( + _("warning, could not query stacks for #%d: %s\n") + % (pr_id.number, result.unwrap_err()) + ) + return None + stack = result.unwrap() + if stack is None: + return None + if pr_id.number not in stack.pull_requests: + # The pull request is associated with the stack but is not one of its + # open members (stack.pull_requests excludes merged/closed pull + # requests), so its position within the stack is unknown. Warn here + # (rather than returning None, which would produce a misleading "no + # stack information" message) and link nothing. + ui.warn( + _( + "#%d is no longer an open member of stack #%d.\n" + "Ancestors will not be linked to pull requests.\n" + ) + % (pr_id.number, stack.number) + ) + return [] + index = stack.pull_requests.index(pr_id.number) + # stack.pull_requests is ordered from the bottom of the stack to the top. + return list(reversed(stack.pull_requests[:index])) + + async def _link_pull_request( store: PullRequestStore, original: PullRequestId, number: int ) -> Optional[PullRequestId]: diff --git a/eden/scm/sapling/ext/github/mock_utils.py b/eden/scm/sapling/ext/github/mock_utils.py index added9869489b..e6ea85e985c0d 100644 --- a/eden/scm/sapling/ext/github/mock_utils.py +++ b/eden/scm/sapling/ext/github/mock_utils.py @@ -11,7 +11,7 @@ from sapling.ext.github.consts import query from sapling.ext.github.gh_submit import PullRequestState from sapling.ext.github.pull_request_body import title_and_body -from sapling.result import Ok, Result +from sapling.result import Err, Ok, Result from .consts import GITHUB_HOSTNAME from .github_gh_cli import JsonDict @@ -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": @@ -279,7 +322,9 @@ def expect_update_pr_request( pr_id: str, pr_number: int, commit_msg: str, - base: str = "main", + # None means the update does not touch the base branch (used for the + # "stacked" workflow, where the native GitHub stack manages bases). + base: Optional[str] = "main", owner: str = OWNER, name: str = REPO_NAME, stack_pr_ids: Optional[List[int]] = None, @@ -302,12 +347,17 @@ def expect_update_pr_request( title, body = title_and_body(commit_msg) params: ParamsType = { - "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": pr_id, "title": title, "body": body, - "base": base, } + if base is not None: + params["base"] = base key = create_request_key(params, self.hostname) request = UpdatePrRequest(key, pr_id) self._add_request(key, request) @@ -341,6 +391,61 @@ def expect_merge_into_branch( self._add_request(key, request) return request + def expect_get_stack_request( + self, + pr_number: int, + owner: str = OWNER, + name: str = REPO_NAME, + ) -> "GetStackRequest": + endpoint = f"repos/{owner}/{name}/stacks?pull_request={pr_number}" + key = create_request_key({}, self.hostname, endpoint=endpoint) + request = GetStackRequest(key, owner, name) + self._add_request(key, request) + return request + + def expect_create_stack_request( + self, + pr_numbers: List[int], + owner: str = OWNER, + name: str = REPO_NAME, + ) -> "CreateStackRequest": + endpoint = f"repos/{owner}/{name}/stacks" + params: ParamsType = {"pull_requests": pr_numbers} + key = create_request_key( + params, self.hostname, endpoint=endpoint, method="POST" + ) + request = CreateStackRequest(key, owner, name, pr_numbers) + self._add_request(key, request) + return request + + def expect_add_to_stack_request( + self, + stack_number: int, + pr_numbers: List[int], + owner: str = OWNER, + name: str = REPO_NAME, + ) -> "AddToStackRequest": + endpoint = f"repos/{owner}/{name}/stacks/{stack_number}/add" + params: ParamsType = {"pull_requests": pr_numbers} + key = create_request_key( + params, self.hostname, endpoint=endpoint, method="POST" + ) + request = AddToStackRequest(key, owner, name, stack_number) + self._add_request(key, request) + return request + + def expect_unstack_request( + self, + stack_number: int, + owner: str = OWNER, + name: str = REPO_NAME, + ) -> "UnstackRequest": + endpoint = f"repos/{owner}/{name}/stacks/{stack_number}/unstack" + key = create_request_key({}, self.hostname, endpoint=endpoint, method="POST") + request = UnstackRequest(key, owner, name, stack_number) + self._add_request(key, request) + return request + class MockRequest: @abstractmethod @@ -578,6 +683,151 @@ def get_response(self) -> Result[JsonDict, str]: return self._response +def create_stack_response( + owner: str, name: str, stack_number: int, pr_numbers: List[int], is_open: bool = True +) -> JsonDict: + """Builds a "Pull Request Stack" REST API response object. + + `pr_numbers` is ordered from the bottom of the stack to the top. + """ + return { + "id": stack_number, + "number": stack_number, + "node_id": f"PRS_id_{stack_number}", + "url": f"https://github.com/{owner}/{name}/stacks/{stack_number}", + "open": is_open, + "base": {"ref": "main"}, + "created_at": "2026-07-30T00:00:00Z", + "pull_requests": [ + { + "number": n, + "state": "open", + "draft": False, + "merged_at": None, + "head": {"ref": f"pr{n}", "sha": gen_hash_hexdigest(f"pr{n}")}, + } + for n in pr_numbers + ], + } + + +class GetStackRequest(MockRequest): + def __init__(self, key: str, owner: str, name: str) -> None: + self._key = key + self._response: Optional[Result[JsonDict, str]] = None + + self._owner = owner + self._name = name + + def and_respond( + self, + stack_number: Optional[int] = None, + pr_numbers: Optional[List[int]] = None, + is_open: bool = True, + ): + """Responds with the stack containing the pull request, or an empty + list (meaning "not part of any stack") if stack_number is None. + """ + if stack_number is None: + self._response = Ok([]) + else: + self._response = Ok( + [ + create_stack_response( + self._owner, + self._name, + stack_number, + pr_numbers or [], + is_open=is_open, + ) + ] + ) + + def and_respond_error(self, message: str): + """Responds with an error, as if the stacks API request failed.""" + self._response = Err(message) + + def get_response(self) -> Result[JsonDict, str]: + if self._response is None: + raise MockResponseNotSet(self._key) + return self._response + + +class CreateStackRequest(MockRequest): + def __init__(self, key: str, owner: str, name: str, pr_numbers: List[int]) -> None: + self._key = key + self._response: Optional[Result[JsonDict, str]] = None + + self._owner = owner + self._name = name + self._pr_numbers = pr_numbers + + def and_respond(self, stack_number: int): + self._response = Ok( + create_stack_response( + self._owner, self._name, stack_number, self._pr_numbers + ) + ) + + def get_response(self) -> Result[JsonDict, str]: + if self._response is None: + raise MockResponseNotSet(self._key) + return self._response + + +class AddToStackRequest(MockRequest): + def __init__(self, key: str, owner: str, name: str, stack_number: int) -> None: + self._key = key + self._response: Optional[Result[JsonDict, str]] = None + + self._owner = owner + self._name = name + self._stack_number = stack_number + + def and_respond(self, pr_numbers: List[int]): + """`pr_numbers` is the full list of pull requests in the stack after + the addition, ordered from the bottom to the top. + """ + self._response = Ok( + create_stack_response( + self._owner, self._name, self._stack_number, pr_numbers + ) + ) + + def get_response(self) -> Result[JsonDict, str]: + if self._response is None: + raise MockResponseNotSet(self._key) + return self._response + + +class UnstackRequest(MockRequest): + def __init__(self, key: str, owner: str, name: str, stack_number: int) -> None: + self._key = key + self._response: Optional[Result[JsonDict, str]] = None + + self._owner = owner + self._name = name + self._stack_number = stack_number + + def and_respond(self, remaining_pr_numbers: Optional[List[int]] = None): + """Responds with the updated stack if pull requests remain in it, or + with an empty response (HTTP 204: stack dissolved) by default. + """ + if remaining_pr_numbers is None: + self._response = Ok({}) + else: + self._response = Ok( + create_stack_response( + self._owner, self._name, self._stack_number, remaining_pr_numbers + ) + ) + + def get_response(self) -> Result[JsonDict, str]: + if self._response is None: + raise MockResponseNotSet(self._key) + return self._response + + class MockRequestNotFound(error.Abort): def __init__(self, key: str, requests: Dict[str, MockRequest]) -> None: import textwrap 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..ee3d9c018309c 100644 --- a/eden/scm/sapling/ext/github/submit.py +++ b/eden/scm/sapling/ext/github/submit.py @@ -34,9 +34,15 @@ def submit(ui, repo, *args, **opts) -> int: github_repo = check_github_repo(repo) is_draft = opts.get("draft") is_open = opts.get("open") + is_restack = opts.get("restack") return asyncio.run( update_commits_in_stack( - ui, repo, github_repo, is_draft=is_draft, is_open=is_open + ui, + repo, + github_repo, + is_draft=is_draft, + is_open=is_open, + restack=is_restack, ) ) @@ -70,6 +76,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 +99,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( @@ -151,7 +172,12 @@ async def get_partitions(ui, repo, store, filter) -> List[List[CommitData]]: async def update_commits_in_stack( - ui, repo, github_repo: GitHubRepo, is_draft: bool, is_open: bool = False + ui, + repo, + github_repo: GitHubRepo, + is_draft: bool, + is_open: bool = False, + restack: bool = False, ) -> int: parents = repo.dirstate.parents() if parents[0] == nullid: @@ -208,17 +234,145 @@ def get_gitdir() -> str: if not refs_to_update: ui.status_err(_("no pull requests to update\n")) + if workflow == SubmitWorkflow.STACKED: + # Even when there is nothing to push, ensure the pull requests are + # linked into a native GitHub stack. This makes stack creation + # idempotent: if it failed on a previous run (or the stack was + # modified on GitHub), re-running `sl pr submit` reconciles it. + repository = params.repository + if not repository: + repository = await get_repository_for_origin( + origin, github_repo.hostname + ) + await sync_github_stack(ui, partitions, repository, restack=restack) return 0 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 the STACKED workflow, consult the native GitHub stack BEFORE making + # any changes: + # + # - If the stack on GitHub has diverged from the local stack and --restack + # was not passed, abort now, before any base updates or pushes: partial + # updates against a diverged stack can corrupt it. + # - With --restack, dissolve the diverged stack now, so that subsequent + # base branch updates are not rejected by GitHub (GitHub does not allow + # changing the base branch of a pull request that is in a stack). + # - If the stack matches (or is extended by) the local stack, remember its + # members: their base branches are managed by the stack itself and must + # not be updated via the API. + github_stack: Optional[gh_submit.StackDetails] = None + github_stack_fetched = False + prs_in_github_stack = set() + if workflow == SubmitWorkflow.STACKED: + if not repository: + repository = await get_repository_for_origin(origin, github_repo.hostname) + if not repository.is_fork: + local_stack = local_open_pr_numbers(partitions) + if local_stack: + fetched, github_stack = await query_github_stack( + ui, repository, local_stack + ) + if not fetched: + # Fail closed: without knowing the state of the stack on + # GitHub, base updates and pushes could corrupt it. (The + # warning with the underlying error was already printed.) + raise error.Abort( + _( + "could not determine the state of the stack on " + "GitHub; re-run 'pr submit' to retry" + ) + ) + github_stack_fetched = True + if github_stack: + stack_prs = github_stack.pull_requests + new_pr_inserted = _has_new_pr_below_stack_top( + partitions, set(stack_prs) + ) + if stack_prs == local_stack[: len(stack_prs)] and not new_pr_inserted: + # The stack matches the local stack (possibly extended + # at the top with new pull requests). + prs_in_github_stack = set(stack_prs) + elif restack: + # Dissolving the stack is only useful if it can be + # recreated afterwards, which requires at least two + # open pull requests in the local stack. + num_new_prs = sum(1 for p in partitions if not p[0].pr) + if len(local_stack) + num_new_prs < 2: + raise error.Abort( + _( + "--restack would leave stack #%d with " + "fewer than two pull requests; dissolve " + "it on GitHub instead if that is intended" + ) + % github_stack.number + ) + # The stacks API has no "reorder" operation, so + # dissolve the diverged stack now; it is recreated + # from the local stack at the end of the submit. + unstack_result = await gh_submit.unstack( + repository.hostname, + *repository.get_upstream_owner_and_name(), + github_stack.number, + ) + if unstack_result.is_err(): + raise error.Abort( + _("failed to dissolve stack #%d: %s") + % (github_stack.number, unstack_result.unwrap_err()) + ) + # Merged or queued pull requests cannot be unstacked + # and are left in place; recreating the stack is not + # possible while they remain in the old one. + remnant = unstack_result.unwrap() + if remnant and remnant.pull_requests: + raise error.Abort( + _( + "stack #%d was only partially dissolved: " + "%s could not be unstacked (merged or " + "queued pull requests are left in place)" + ) + % (remnant.number, _pr_list(remnant.pull_requests)) + ) + github_stack = None + else: + if new_pr_inserted: + ui.status_err( + _( + "new pull requests would be inserted below " + "the top of stack #%d on GitHub (%s), which " + "requires recreating the stack; not " + "updating it\n" + ) + % ( + github_stack.number, + _pr_list(github_stack.pull_requests), + ) + ) + else: + ui.status_err( + _( + "stack #%d on GitHub (%s) does not match " + "your local stack (%s); not updating it\n" + ) + % ( + github_stack.number, + _pr_list(github_stack.pull_requests), + _pr_list(local_stack), + ) + ) + hintutil.triggershow(ui, "pr-submit-restack") + raise error.Abort( + _("stack on GitHub has diverged from your local stack") + ) + + # 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 ] @@ -234,9 +388,23 @@ def get_gitdir() -> str: pr = partition[0].pr if not pr or pr.state != PullRequestState.OPEN: continue + if pr.number in prs_in_github_stack: + # GitHub rejects base branch changes for pull requests + # that are in a native stack; the stack manages base + # branches itself. + 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 ) @@ -304,6 +472,16 @@ def get_gitdir() -> str: ] await asyncio.gather(*rewrite_and_archive_requests) + if workflow == SubmitWorkflow.STACKED: + await sync_github_stack( + ui, + partitions, + repository, + restack=restack, + stack=github_stack, + stack_fetched=github_stack_fetched, + ) + # Open pull requests in browser if --open flag was specified if is_open: pr_urls = [none_throws(p[0].pr).url for p in partitions if p[0].pr] @@ -314,6 +492,198 @@ def get_gitdir() -> str: return 0 +def local_open_pr_numbers(partitions: List[List[CommitData]]) -> List[int]: + """Numbers of the open pull requests in the local stack, ordered from the + bottom of the stack to the top (as expected by the GitHub stacks API). + Note that `partitions` is ordered from the top of the stack to the bottom. + """ + return [ + none_throws(p[0].pr).number + for p in reversed(partitions) + if p[0].pr and p[0].pr.state == PullRequestState.OPEN + ] + + +def _pr_list(numbers: List[int]) -> str: + return ", ".join(f"#{n}" for n in numbers) + + +def _has_new_pr_below_stack_top( + partitions: List[List[CommitData]], stack_prs: set +) -> bool: + """True if a commit without a pull request (i.e. one that will get a new + pull request) sits below a member of the native GitHub stack. Appending + to a stack is only possible at the top, so this requires recreating the + stack even when the existing members are otherwise in order. + """ + seen_new = False + # `partitions` is ordered from the top of the stack to the bottom. + for p in reversed(partitions): + head = p[0] + pr = head.pr + if pr is None: + seen_new = True + elif ( + seen_new + and pr.state == PullRequestState.OPEN + and pr.number in stack_prs + ): + return True + return False + + +async def query_github_stack( + ui, repository: Repository, local: List[int] +) -> Tuple[bool, Optional["gh_submit.StackDetails"]]: + """Queries GitHub for the native stack containing the local stack's pull + requests, if any. Returns (fetched, stack) where `fetched` is False if the + query failed (a warning is printed in that case). + """ + owner, name = repository.get_upstream_owner_and_name() + hostname = repository.hostname + + # Query with the bottom pull request first: it is the most stable member + # of an existing stack. Also try the top one to catch the case where the + # local stack was extended (or reordered) at the bottom. + stack = None + for number in dict.fromkeys([local[0], local[-1]]): + result = await gh_submit.get_stack_for_pull_request( + hostname, owner, name, number + ) + if result.is_err(): + ui.status_err( + _("warning, could not query stacks for #%d: %s\n") + % (number, result.unwrap_err()) + ) + return False, None + stack = result.unwrap() + if stack: + break + if stack and not stack.is_open: + # A closed stack cannot be appended to or dissolved; treat it the + # same as no stack. + stack = None + return True, stack + + +async def sync_github_stack( + ui, + partitions: List[List[CommitData]], + repository: Repository, + restack: bool = False, + stack: Optional["gh_submit.StackDetails"] = None, + stack_fetched: bool = False, +) -> None: + """Links the pull requests together using GitHub's native "stacked pull + requests" feature. + + Creates the stack if it does not exist and appends new pull requests to + the top of an existing one. If the stack on GitHub has diverged from the + local stack (e.g., commits were reordered or removed), no changes are made + to it unless `restack` is True, in which case the stack on GitHub is + dissolved and recreated to match the local stack. + + Failures to sync the stack are reported as warnings rather than aborting: + at this point, the pull requests themselves have already been created or + updated successfully, and re-running `sl pr submit` will retry linking. + """ + if repository.is_fork: + # GitHub requires all branches of a stack to be in the same + # repository, so stacks are not supported across forks. + ui.status_err( + _( + "warning: GitHub does not support stacks across forks; " + "pull requests were submitted without a stack\n" + ) + ) + return + + local = local_open_pr_numbers(partitions) + if len(local) < 2: + # A stack must contain at least two pull requests. Note that GitHub + # automatically removes merged pull requests from existing stacks, so + # there is nothing to clean up here as the stack shrinks. + return + + owner, name = repository.get_upstream_owner_and_name() + hostname = repository.hostname + pr_list = _pr_list + + if not stack_fetched: + fetched, stack = await query_github_stack(ui, repository, local) + if not fetched: + return + + if stack is None: + result = await gh_submit.create_stack(hostname, owner, name, local) + if result.is_err(): + ui.status_err( + _("warning, failed to create stack for %s: %s\n") + % (pr_list(local), result.unwrap_err()) + ) + else: + ui.status_err(_("created stack: %s\n") % result.unwrap().url) + elif stack.pull_requests == local: + ui.status_err(_("stack #%d is up-to-date\n") % stack.number) + elif stack.pull_requests == local[: len(stack.pull_requests)]: + # The local stack extends the stack on GitHub at the top, so the new + # pull requests can simply be appended. + to_add = local[len(stack.pull_requests) :] + result = await gh_submit.add_prs_to_stack( + hostname, owner, name, stack.number, to_add + ) + if result.is_err(): + ui.status_err( + _("warning, failed to add %s to stack #%d: %s\n") + % (pr_list(to_add), stack.number, result.unwrap_err()) + ) + else: + ui.status_err( + _("added %s to stack #%d\n") % (pr_list(to_add), stack.number) + ) + elif restack: + # The stacks API has no "reorder" operation, so dissolve the stack and + # recreate it from the local stack. + unstack_result = await gh_submit.unstack(hostname, owner, name, stack.number) + if unstack_result.is_err(): + ui.status_err( + _("warning, failed to dissolve stack #%d: %s\n") + % (stack.number, unstack_result.unwrap_err()) + ) + return + # Merged or queued pull requests cannot be unstacked and are left in + # place; recreating the stack is not possible while they remain in + # the old one. + remnant = unstack_result.unwrap() + if remnant and remnant.pull_requests: + ui.status_err( + _( + "warning, stack #%d was only partially dissolved: %s " + "could not be unstacked (merged or queued pull requests " + "are left in place)\n" + ) + % (remnant.number, _pr_list(remnant.pull_requests)) + ) + return + result = await gh_submit.create_stack(hostname, owner, name, local) + if result.is_err(): + ui.status_err( + _("warning, failed to recreate stack for %s: %s\n") + % (pr_list(local), result.unwrap_err()) + ) + else: + ui.status_err(_("recreated stack: %s\n") % result.unwrap().url) + else: + ui.status_err( + _( + "warning: stack #%d on GitHub (%s) does not match your local " + "stack (%s); not updating it\n" + ) + % (stack.number, pr_list(stack.pull_requests), pr_list(local)) + ) + hintutil.triggershow(ui, "pr-submit-restack") + + async def rewrite_pull_request_body( partitions: List[List[CommitData]], index: int, @@ -326,9 +696,26 @@ async def rewrite_pull_request_body( # of this branch. Recall that partitions is ordered from the top of the # 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) + base: Optional[str] = repository.get_base_branch() + if workflow == SubmitWorkflow.STACKED: + # GitHub rejects base branch changes for pull requests that are in a + # native stack (the stack manages base branches itself), so leave the + # base untouched when updating the title/body. New pull requests get + # the correct base at creation time. + base = None + elif 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 +734,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: @@ -373,8 +764,11 @@ class SerialStrategyParams: # git push --force any heads that need updating, creating new branch names, # if necessary. refs_to_update: List[str] - # The str in the Tuple is the head branch name for the commit. - pull_requests_to_create: List[Tuple[CommitData, str]] + # The first str in the Tuple is the head branch name for the commit. The + # second is the head branch name of the partition below it in the stack + # (None if it is at the bottom), to be used as the base branch in + # workflows with chained bases. + pull_requests_to_create: List[Tuple[CommitData, str, Optional[str]]] repository: Optional[Repository] @@ -417,7 +811,7 @@ async def create_serial_strategy_params( # git push --force any heads that need updating, creating new branch names, # if necessary. refs_to_update = [] - pull_requests_to_create: List[Tuple[CommitData, str]] = [] + pull_requests_to_create: List[Tuple[CommitData, str, Optional[str]]] = [] # These are set lazily because they require GraphQL calls. next_pull_request_number = None @@ -426,6 +820,9 @@ async def create_serial_strategy_params( # Note that `partitions` is ordered from the top of the stack to the bottom, # but we want to create PRs from the bottom to the top so the PR numbers are # created in ascending order. + # Head branch of the partition below the current one, i.e. the base branch + # to use for a new pull request in workflows with chained bases. + below_branch: Optional[str] = None for partition in reversed(partitions): top = partition[0] pr = top.pr @@ -466,7 +863,12 @@ async def create_serial_strategy_params( ) refs_to_update.append(f"{hex(top.node)}:refs/heads/{branch_name}") top.head_branch_name = branch_name - pull_requests_to_create.append((top, branch_name)) + pull_requests_to_create.append((top, branch_name, below_branch)) + 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 base for the partition above: + # closed/merged head branches would break the chain. + below_branch = top.head_branch_name return SerialStrategyParams(refs_to_update, pull_requests_to_create, repository) @@ -487,7 +889,7 @@ def get_pull_request_template(commit: CommitData) -> None | str: async def create_pull_requests_serially( - commits: List[Tuple[CommitData, str]], + commits: List[Tuple[CommitData, str, Optional[str]]], workflow: SubmitWorkflow, repository: Repository, store: PullRequestStore, @@ -506,11 +908,21 @@ async def create_pull_requests_serially( # Create the pull requests in order serially to give us the best chance of # the number in the branch name matching that of the actual pull request. commits_to_update = [] - parent = None - for commit, branch_name in commits: + for commit, branch_name, below_branch in commits: base = repository.get_base_branch() - if workflow == SubmitWorkflow.SINGLE and parent: - base = none_throws(parent.head_branch_name) + if workflow.uses_chained_bases() and below_branch and not repository.is_fork: + # Use the head branch of the partition below this commit as the + # base branch, even if that pull request already existed. This is + # required for the STACKED workflow (the base branch cannot be + # corrected afterwards: GitHub rejects base branch changes for + # pull requests in a native stack) and avoids a redundant base + # update for the SINGLE workflow. + # + # For forks this is not possible: the head branches live on the + # fork, but the base branch of a pull request must be a branch on + # the upstream repository, so fall back to the default base + # branch. + base = below_branch commit_msg = commit.get_msg() title, body = title_and_body(commit_msg) @@ -545,8 +957,6 @@ async def create_pull_requests_serially( store.map_commit_to_pull_request(commit.node, pr_id) commits_to_update.append((commit, pr_id)) - parent = commit - # Now that all of the pull requests have been created, update the .pr field # on each CommitData. We prioritize the create_pull_request() calls to try # to get the pull request numbers to match up. @@ -599,7 +1009,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 +1071,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_closed_stack.py b/eden/scm/tests/github/mock_closed_stack.py new file mode 100644 index 0000000000000..42b021f53a8dd --- /dev/null +++ b/eden/scm/tests/github/mock_closed_stack.py @@ -0,0 +1,59 @@ +# 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, +) + +# An extension to mock network requests for `sl pr submit` with +# github.pr-workflow=stacked when the pull request's stack on GitHub is +# closed. A closed stack cannot be appended to or dissolved, so it is treated +# the same as no stack: the base branch is updated normally and no stack +# operations are attempted (a single open pull request cannot form a new +# stack). + + +def setup_mock_github_server(repo) -> MockGitHubServer: + github_server = MockGitHubServer() + + github_server.expect_get_repository_request().and_respond() + + # #42's head is stale (the commit was amended), so it is pushed. + github_server.expect_get_pr_details_request(42).and_respond("PR_id_42") + + # The stack containing #42 is closed. + github_server.expect_get_stack_request(42).and_respond( + stack_number=100, pr_numbers=[42, 43], is_open=False + ) + + # Since the closed stack does not manage #42 anymore, its base branch is + # updated via the API. + github_server.expect_update_pr_request( + "PR_id_42", 42, "", base="main" + ).and_respond() + + msg = "one\n\nPull Request resolved: https://github.com/facebook/test_github_repo/pull/42" + github_server.expect_update_pr_request( + "PR_id_42", 42, msg, base=None + ).and_respond() + + github_server.expect_get_username_request().and_respond() + tip = scmutil.revsingle(repo, "desc(one)").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/github/mock_create_stacked_prs.py b/eden/scm/tests/github/mock_create_stacked_prs.py new file mode 100644 index 0000000000000..68b5d0b55dabc --- /dev/null +++ b/eden/scm/tests/github/mock_create_stacked_prs.py @@ -0,0 +1,77 @@ +# 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 +from sapling.ext.github import github_gh_cli, submit +from sapling.ext.github.mock_utils import mock_run_git_command, MockGitHubServer +from sapling.ext.github.pull_request_body import title_and_body + +# An extension to mock network requests for the initial `sl pr submit` of a +# stack of two commits with github.pr-workflow=stacked. It replaces +# `github_gh_cli.make_request` and `submit.run_git_command` with the +# corresponding mock functions. Check the `uisetup` function for how the mock +# functions are registered. + + +def setup_mock_github_server(ui) -> MockGitHubServer: + """Setup mock GitHub Server for testing happy case of `sl pr submit` with + the "stacked" workflow. + """ + github_server = MockGitHubServer() + + github_server.expect_get_repository_request().and_respond() + + github_server.expect_guess_next_pull_request_number().and_respond() + + prs = [ + (42, "one\n"), + (43, "two\n"), + ] + + for idx, (num, msg) in enumerate(prs): + title, body = title_and_body(msg) + head = f"pr{num}" + + # Each PR's base is chained to the head branch of the PR below it. + base = "main" if idx == 0 else "pr%d" % prs[idx - 1][0] + + github_server.expect_create_pr_request( + body=body, + title=title, + head=head, + base=base, + ).and_respond(number=num) + + pr_id = f"PR_id_{num}" + github_server.expect_get_pr_details_request(num).and_respond(pr_id) + + # The "stacked" workflow omits the stack list footer from PR bodies + # (GitHub renders the stack natively), so stack_pr_ids is left unset. + # It also leaves the base branch untouched when rewriting the body + # (base=None), since the native stack manages base branches. + github_server.expect_update_pr_request( + pr_id, num, msg, base=None + ).and_respond() + + github_server.expect_get_username_request().and_respond() + + head = "1a67244b0a776bfcc3be6bf811e98c993d78ce47" + github_server.expect_merge_into_branch(head).and_respond() + + # Neither the bottom (#42) nor the top (#43) pull request is part of a + # stack yet, so a new stack is created. + github_server.expect_get_stack_request(42).and_respond() + github_server.expect_get_stack_request(43).and_respond() + github_server.expect_create_stack_request([42, 43]).and_respond(stack_number=100) + + return github_server + + +def uisetup(ui): + mock_github_server = setup_mock_github_server(ui) + extensions.wrapfunction( + github_gh_cli, "_make_request", mock_github_server.make_request + ) + extensions.wrapfunction(submit, "run_git_command", mock_run_git_command) diff --git a/eden/scm/tests/github/mock_create_stacked_prs_placeholder.py b/eden/scm/tests/github/mock_create_stacked_prs_placeholder.py new file mode 100644 index 0000000000000..5780cdde3c67c --- /dev/null +++ b/eden/scm/tests/github/mock_create_stacked_prs_placeholder.py @@ -0,0 +1,73 @@ +# 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 the initial `sl pr submit` of a +# stack of two commits with github.pr-workflow=stacked and +# github.placeholder-strategy=true: pull request numbers are reserved via +# placeholder issues, the pull requests are created with chained bases, and +# they are linked into a native GitHub stack. + + +def setup_mock_github_server(repo) -> MockGitHubServer: + github_server = MockGitHubServer() + + github_server.expect_get_repository_request().and_respond() + + github_server.expect_create_pr_placeholder_request().and_respond( + start_number=42, num_times=2 + ) + + prs = [ + (42, "one\n"), + (43, "two\n"), + ] + for idx, (num, msg) in enumerate(prs): + _title, body = title_and_body(msg) + + # Each PR's base is chained to the head branch of the PR below it. + base = "main" if idx == 0 else "pr%d" % prs[idx - 1][0] + github_server.expect_create_pr_using_placeholder_request( + body=body, issue=num, base=base + ).and_respond() + + pr_id = f"PR_id_{num}" + github_server.expect_get_pr_details_request(num).and_respond(pr_id) + + # The stacked workflow leaves the base untouched when rewriting the + # body, and omits the stack list footer. + github_server.expect_update_pr_request( + pr_id, num, msg, base=None + ).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() + + # Neither pull request is part of a stack yet, so a new stack is created. + github_server.expect_get_stack_request(42).and_respond() + github_server.expect_get_stack_request(43).and_respond() + github_server.expect_create_stack_request([42, 43]).and_respond( + stack_number=100 + ) + + 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_diverged_stack.py b/eden/scm/tests/github/mock_diverged_stack.py new file mode 100644 index 0000000000000..6ee075c72a9cc --- /dev/null +++ b/eden/scm/tests/github/mock_diverged_stack.py @@ -0,0 +1,56 @@ +# 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 +from sapling.ext.github import github_gh_cli, submit +from sapling.ext.github.mock_utils import mock_run_git_command, MockGitHubServer + +# An extension to mock network requests for `sl pr submit` with +# github.pr-workflow=stacked when the stack on GitHub has diverged from the +# local stack (e.g., it was reordered on GitHub). All local commits are +# up-to-date, so nothing is pushed. +# +# Without --restack, the diverged stack must not be modified (a warning is +# printed instead). With --restack, the stack is dissolved and recreated, so +# this mock also includes the unstack/create expectations (they are simply +# unused in the no---restack case). + +COMMIT_ONE = "ebe5b8faff36687becb7bdbca1e6a61dac428834" +COMMIT_TWO = "1a67244b0a776bfcc3be6bf811e98c993d78ce47" +COMMIT_THREE = "f4185fef85f10d46b859c30076243068b0f59245" + + +def setup_mock_github_server(ui) -> MockGitHubServer: + github_server = MockGitHubServer() + + github_server.expect_get_repository_request().and_respond() + + for num, oid in [(42, COMMIT_ONE), (43, COMMIT_TWO), (44, COMMIT_THREE)]: + github_server.expect_get_pr_details_request(num).and_respond( + f"PR_id_{num}", head_ref_oid=oid + ) + + # The stack on GitHub has a different order than the local stack + # (#42, #43, #44). + github_server.expect_get_stack_request(42).and_respond( + stack_number=100, pr_numbers=[43, 42, 44] + ) + + # Only used with --restack: the diverged stack is dissolved (204) and + # recreated to match the local stack. + github_server.expect_unstack_request(100).and_respond() + github_server.expect_create_stack_request([42, 43, 44]).and_respond( + stack_number=101 + ) + + return github_server + + +def uisetup(ui): + mock_github_server = setup_mock_github_server(ui) + extensions.wrapfunction( + github_gh_cli, "_make_request", mock_github_server.make_request + ) + extensions.wrapfunction(submit, "run_git_command", mock_run_git_command) diff --git a/eden/scm/tests/github/mock_diverged_stack_dirty.py b/eden/scm/tests/github/mock_diverged_stack_dirty.py new file mode 100644 index 0000000000000..c74a1b5d395a9 --- /dev/null +++ b/eden/scm/tests/github/mock_diverged_stack_dirty.py @@ -0,0 +1,88 @@ +# 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, +) + +# An extension to mock network requests for `sl pr submit` with +# github.pr-workflow=stacked when the stack on GitHub has diverged from the +# local stack AND there are local changes to push (the top commit was +# amended). +# +# Without --restack, the submit must abort BEFORE updating any base branches +# or pushing anything: partial updates against a diverged stack can corrupt +# it. With --restack, the diverged stack is dissolved up front, the bases and +# branches are updated, and the stack is recreated from the local stack. +# +# 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() + + commit_one = scmutil.revsingle(repo, "desc(one)").hex() + commit_two = scmutil.revsingle(repo, "desc(two)").hex() + + github_server.expect_get_pr_details_request(42).and_respond( + "PR_id_42", head_ref_oid=commit_one + ) + github_server.expect_get_pr_details_request(43).and_respond( + "PR_id_43", head_ref_oid=commit_two + ) + # PR #44's head is stale (the commit was amended), so it needs a push. + github_server.expect_get_pr_details_request(44).and_respond("PR_id_44") + + # The stack on GitHub has a different order than the local stack + # (#42, #43, #44). It is queried up front, before any mutations. + github_server.expect_get_stack_request(42).and_respond( + stack_number=100, pr_numbers=[43, 42, 44] + ) + + # Only used with --restack: the diverged stack is dissolved up front, + # base branches are updated, the amended commit is pushed, and the stack + # is recreated from the local stack. + github_server.expect_unstack_request(100).and_respond() + 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="pr42" + ).and_respond() + github_server.expect_update_pr_request( + "PR_id_42", 42, "", base="main" + ).and_respond() + github_server.expect_update_pr_request( + "PR_id_44", 44, "three\n", base=None + ).and_respond() + github_server.expect_update_pr_request( + "PR_id_43", 43, "two\n", base=None + ).and_respond() + github_server.expect_update_pr_request( + "PR_id_42", 42, "one\n", base=None + ).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() + github_server.expect_create_stack_request([42, 43, 44]).and_respond( + stack_number=101 + ) + + 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) diff --git a/eden/scm/tests/github/mock_extend_stacked_prs.py b/eden/scm/tests/github/mock_extend_stacked_prs.py new file mode 100644 index 0000000000000..540d9fbb5b7ef --- /dev/null +++ b/eden/scm/tests/github/mock_extend_stacked_prs.py @@ -0,0 +1,87 @@ +# 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 +from sapling.ext.github import github_gh_cli, submit +from sapling.ext.github.mock_utils import mock_run_git_command, MockGitHubServer + +# An extension to mock network requests for `sl pr submit` with +# github.pr-workflow=stacked when a new commit is added on top of a stack +# whose pull requests (#42, #43) were created by a previous submit (see +# mock_create_stacked_prs.py): the new PR #44 should be appended to the +# existing stack on GitHub. + +COMMIT_ONE = "ebe5b8faff36687becb7bdbca1e6a61dac428834" +COMMIT_TWO = "1a67244b0a776bfcc3be6bf811e98c993d78ce47" +COMMIT_THREE = "f4185fef85f10d46b859c30076243068b0f59245" + + +def setup_mock_github_server(ui) -> MockGitHubServer: + github_server = MockGitHubServer() + + github_server.expect_get_repository_request().and_respond() + + # PRs #42 and #43 already exist and are up-to-date: their head OIDs match + # the local commits, so only the new commit is pushed. + github_server.expect_get_pr_details_request(42).and_respond( + "PR_id_42", head_ref_oid=COMMIT_ONE + ) + github_server.expect_get_pr_details_request(43).and_respond( + "PR_id_43", head_ref_oid=COMMIT_TWO + ) + + # The next PR number should be 44. + github_server.expect_guess_next_pull_request_number().and_respond( + latest_issue_num=40, latest_pr_num=43 + ) + + # Existing PRs #42 and #43 are already members of native stack #100 (see + # the expect_get_stack_request below), so their base branches must NOT be + # updated before pushing: GitHub rejects base branch changes for pull + # requests that are in a stack (the stack manages bases itself). Hence + # there are no base-update expectations here. + + # The new PR is created directly against the head branch of the pull + # request below it in the stack: the base cannot be corrected afterwards, + # since GitHub rejects base branch changes for PRs in a native stack. + github_server.expect_create_pr_request( + body="", title="three", head="pr44", base="pr43" + ).and_respond(number=44) + github_server.expect_get_pr_details_request(44).and_respond("PR_id_44") + + # Body rewrites leave the base branch untouched (base=None) in the + # "stacked" workflow. + github_server.expect_update_pr_request( + "PR_id_44", 44, "three\n", base=None + ).and_respond() + github_server.expect_update_pr_request( + "PR_id_43", 43, "two\n", base=None + ).and_respond() + github_server.expect_update_pr_request( + "PR_id_42", 42, "one\n", base=None + ).and_respond() + + github_server.expect_get_username_request().and_respond() + github_server.expect_merge_into_branch(COMMIT_THREE).and_respond() + + # #42 is already the bottom of stack #100, and the local stack extends it + # at the top, so #44 is appended to the existing stack. The stack is + # queried up front (before any base updates or pushes). + github_server.expect_get_stack_request(42).and_respond( + stack_number=100, pr_numbers=[42, 43] + ) + github_server.expect_add_to_stack_request(100, [44]).and_respond( + pr_numbers=[42, 43, 44] + ) + + return github_server + + +def uisetup(ui): + mock_github_server = setup_mock_github_server(ui) + extensions.wrapfunction( + github_gh_cli, "_make_request", mock_github_server.make_request + ) + extensions.wrapfunction(submit, "run_git_command", mock_run_git_command) diff --git a/eden/scm/tests/github/mock_fork_stacked.py b/eden/scm/tests/github/mock_fork_stacked.py new file mode 100644 index 0000000000000..33bea4d63cf97 --- /dev/null +++ b/eden/scm/tests/github/mock_fork_stacked.py @@ -0,0 +1,80 @@ +# 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.pr-workflow=stacked when the repo is a fork. GitHub requires all +# branches of a stack to be in the same repository, so no native stack is +# created (a warning is printed instead), and chained bases are not possible +# (fork head branches cannot be bases on the upstream repository), so both +# pull requests are created against the upstream default 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 + ) + + github_server.expect_guess_next_pull_request_number( + owner="upstream" + ).and_respond() + + prs = [ + (42, "one\n"), + (43, "two\n"), + ] + for num, msg in prs: + title, body = title_and_body(msg) + github_server.expect_create_pr_request( + body=body, + title=title, + head=f"facebook:pr{num}", + base="main", + owner="upstream", + ).and_respond(number=num) + + pr_id = f"PR_id_{num}" + github_server.expect_get_pr_details_request( + num, owner="upstream" + ).and_respond(pr_id) + + # The stacked workflow leaves the base untouched when rewriting the + # body, and omits the stack list footer. + github_server.expect_update_pr_request( + pr_id, num, msg, base=None, owner="upstream" + ).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_insert_mid_stack.py b/eden/scm/tests/github/mock_insert_mid_stack.py new file mode 100644 index 0000000000000..2124719a5ec54 --- /dev/null +++ b/eden/scm/tests/github/mock_insert_mid_stack.py @@ -0,0 +1,59 @@ +# 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, +) + +# An extension to mock network requests for `sl pr submit` with +# github.pr-workflow=stacked when a new commit was inserted into the middle +# of a stack whose existing pull requests (#42, #43, #44) are linked into +# native stack #100 on GitHub in the same order. +# +# Even though the existing pull requests match the stack on GitHub, the new +# pull request cannot be appended (stacks can only grow at the top), so +# without --restack the submit must abort before updating or pushing +# anything. (See mock_insert_mid_stack_restack.py for the --restack case.) + + +def setup_mock_github_server(repo) -> MockGitHubServer: + github_server = MockGitHubServer() + + github_server.expect_get_repository_request().and_respond() + + commit_one = scmutil.revsingle(repo, "desc(one)").hex() + github_server.expect_get_pr_details_request(42).and_respond( + "PR_id_42", head_ref_oid=commit_one + ) + # #43 and #44 are stale: their commits were rebased on top of the + # inserted commit. + 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") + + # The next PR number would be 45 (for the inserted commit). + github_server.expect_guess_next_pull_request_number().and_respond( + latest_issue_num=40, latest_pr_num=44 + ) + + # The stack matches the existing pull requests, but the new pull request + # would be inserted below its top, which requires recreating the stack. + github_server.expect_get_stack_request(42).and_respond( + stack_number=100, pr_numbers=[42, 43, 44] + ) + + 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_insert_mid_stack_restack.py b/eden/scm/tests/github/mock_insert_mid_stack_restack.py new file mode 100644 index 0000000000000..e84b4e3ee5a27 --- /dev/null +++ b/eden/scm/tests/github/mock_insert_mid_stack_restack.py @@ -0,0 +1,96 @@ +# 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, +) + +# The --restack counterpart of mock_insert_mid_stack.py: the diverged stack +# (#42, #43, #44 with a new commit inserted between #42 and #43) is dissolved +# up front, the base branches are re-chained (including through the new pull +# request #45), everything is pushed, and the stack is recreated from the +# local stack. + + +def setup_mock_github_server(repo) -> MockGitHubServer: + github_server = MockGitHubServer() + + github_server.expect_get_repository_request().and_respond() + + commit_one = scmutil.revsingle(repo, "desc(one)").hex() + github_server.expect_get_pr_details_request(42).and_respond( + "PR_id_42", head_ref_oid=commit_one + ) + 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") + + github_server.expect_guess_next_pull_request_number().and_respond( + latest_issue_num=40, latest_pr_num=44 + ) + + github_server.expect_get_stack_request(42).and_respond( + stack_number=100, pr_numbers=[42, 43, 44] + ) + + # The diverged stack is dissolved before any base updates. + github_server.expect_unstack_request(100).and_respond() + + # Base branches are re-chained through the new pull request: #43's base + # becomes the inserted commit's head branch (pr45). + 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="pr45" + ).and_respond() + github_server.expect_update_pr_request( + "PR_id_42", 42, "", base="main" + ).and_respond() + + # The inserted commit's pull request is created directly against the head + # branch of the pull request below it. + github_server.expect_create_pr_request( + body="", title="insert", head="pr45", base="pr42" + ).and_respond(number=45) + github_server.expect_get_pr_details_request(45).and_respond("PR_id_45") + + # Body rewrites leave the base branch untouched in the stacked workflow. + github_server.expect_update_pr_request( + "PR_id_44", 44, "three\n", base=None + ).and_respond() + github_server.expect_update_pr_request( + "PR_id_43", 43, "two\n", base=None + ).and_respond() + github_server.expect_update_pr_request( + "PR_id_45", 45, "insert\n", base=None + ).and_respond() + github_server.expect_update_pr_request( + "PR_id_42", 42, "one\n", base=None + ).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() + + # The stack is recreated to match the local stack, including the inserted + # pull request. + github_server.expect_create_stack_request([42, 45, 43, 44]).and_respond( + stack_number=101 + ) + + 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_pull_stacked_pr.py b/eden/scm/tests/github/mock_pull_stacked_pr.py new file mode 100644 index 0000000000000..0d003182a59a3 --- /dev/null +++ b/eden/scm/tests/github/mock_pull_stacked_pr.py @@ -0,0 +1,49 @@ +# 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 +from sapling.ext.github import github_gh_cli +from sapling.ext.github.mock_utils import MockGitHubServer + +# An extension to mock network requests for `sl pr pull` of a pull request +# that is part of a native GitHub stack: its body contains no Sapling stack +# list footer (github.pr-workflow=stacked omits it), so the ancestors must be +# discovered via the stacks API. + +COMMIT_ONE = "ebe5b8faff36687becb7bdbca1e6a61dac428834" +COMMIT_TWO = "1a67244b0a776bfcc3be6bf811e98c993d78ce47" +COMMIT_THREE = "f4185fef85f10d46b859c30076243068b0f59245" + + +def setup_mock_github_server(ui) -> MockGitHubServer: + github_server = MockGitHubServer() + + # Details for the pulled PR (#44). Its body is empty: no stack list + # footer. + github_server.expect_get_pr_details_request(44).and_respond( + "PR_id_44", head_ref_oid=COMMIT_THREE + ) + + # #44 is the top of native stack #100, so #43 and #42 are its ancestors. + github_server.expect_get_stack_request(44).and_respond( + stack_number=100, pr_numbers=[42, 43, 44] + ) + + # The ancestors are linked by fetching their details. + github_server.expect_get_pr_details_request(43).and_respond( + "PR_id_43", head_ref_oid=COMMIT_TWO + ) + github_server.expect_get_pr_details_request(42).and_respond( + "PR_id_42", head_ref_oid=COMMIT_ONE + ) + + return github_server + + +def uisetup(ui): + mock_github_server = setup_mock_github_server(ui) + extensions.wrapfunction( + github_gh_cli, "_make_request", mock_github_server.make_request + ) diff --git a/eden/scm/tests/github/mock_pull_stacked_pr_bottom.py b/eden/scm/tests/github/mock_pull_stacked_pr_bottom.py new file mode 100644 index 0000000000000..cb494fdf59b8e --- /dev/null +++ b/eden/scm/tests/github/mock_pull_stacked_pr_bottom.py @@ -0,0 +1,40 @@ +# 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 +from sapling.ext.github import github_gh_cli, import_pull_request +from sapling.ext.github.mock_utils import ( + MockGitHubServer, + wrap_with_consumption_check, +) + +# An extension to mock network requests for `sl pr pull` of the pull request +# at the bottom of a native GitHub stack: there are no ancestors to link, so +# no warning is printed and no ancestor details are fetched. + +COMMIT_ONE = "ebe5b8faff36687becb7bdbca1e6a61dac428834" + + +def setup_mock_github_server(ui) -> MockGitHubServer: + github_server = MockGitHubServer() + + github_server.expect_get_pr_details_request(42).and_respond( + "PR_id_42", head_ref_oid=COMMIT_ONE + ) + + # #42 is the bottom of native stack #100. + github_server.expect_get_stack_request(42).and_respond( + stack_number=100, pr_numbers=[42, 43, 44] + ) + + return github_server + + +def uisetup(ui): + github_server = setup_mock_github_server(ui) + extensions.wrapfunction( + github_gh_cli, "_make_request", github_server.make_request + ) + wrap_with_consumption_check(github_server, import_pull_request, "get_pr") diff --git a/eden/scm/tests/github/mock_pull_stacked_pr_merged.py b/eden/scm/tests/github/mock_pull_stacked_pr_merged.py new file mode 100644 index 0000000000000..f823c6eb9dd8c --- /dev/null +++ b/eden/scm/tests/github/mock_pull_stacked_pr_merged.py @@ -0,0 +1,44 @@ +# 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 +from sapling.ext.github import github_gh_cli, import_pull_request +from sapling.ext.github.gh_submit import PullRequestState +from sapling.ext.github.mock_utils import ( + MockGitHubServer, + wrap_with_consumption_check, +) + +# An extension to mock network requests for `sl pr pull` of a pull request +# that is associated with a native GitHub stack but is no longer one of its +# open members (it was merged): its position within the stack is unknown, so +# ancestors cannot be linked, and a specific warning is printed (rather than +# the misleading "No stack information found in the pull request body"). + +COMMIT_THREE = "f4185fef85f10d46b859c30076243068b0f59245" + + +def setup_mock_github_server(ui) -> MockGitHubServer: + github_server = MockGitHubServer() + + github_server.expect_get_pr_details_request(45).and_respond( + "PR_id_45", head_ref_oid=COMMIT_THREE, state=PullRequestState.MERGED + ) + + # The stack query returns the stack, but #45 is not among its open + # members (merged pull requests are excluded). + github_server.expect_get_stack_request(45).and_respond( + stack_number=100, pr_numbers=[42, 43] + ) + + return github_server + + +def uisetup(ui): + github_server = setup_mock_github_server(ui) + extensions.wrapfunction( + github_gh_cli, "_make_request", github_server.make_request + ) + wrap_with_consumption_check(github_server, import_pull_request, "get_pr") diff --git a/eden/scm/tests/github/mock_restack_too_small.py b/eden/scm/tests/github/mock_restack_too_small.py new file mode 100644 index 0000000000000..d90113eaf5c1a --- /dev/null +++ b/eden/scm/tests/github/mock_restack_too_small.py @@ -0,0 +1,44 @@ +# 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 +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, +) + +# An extension to mock network requests for `sl pr submit` with +# github.pr-workflow=stacked when the local stack contains a single open pull +# request (#42) while the stack on GitHub contains two (#42, #43): the local +# and GitHub stacks have diverged, and --restack must refuse to dissolve the +# stack because it could not be recreated afterwards (a stack requires at +# least two pull requests). + + +def setup_mock_github_server(repo) -> MockGitHubServer: + github_server = MockGitHubServer() + + github_server.expect_get_repository_request().and_respond() + + # #42's head is stale (the commit was amended), so there is something to + # push, which makes this a "mutating" submit. + github_server.expect_get_pr_details_request(42).and_respond("PR_id_42") + + github_server.expect_get_stack_request(42).and_respond( + stack_number=100, pr_numbers=[42, 43] + ) + + 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_stack_query_failure.py b/eden/scm/tests/github/mock_stack_query_failure.py new file mode 100644 index 0000000000000..841c17d0b1aa1 --- /dev/null +++ b/eden/scm/tests/github/mock_stack_query_failure.py @@ -0,0 +1,46 @@ +# 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, +) + +# An extension to mock network requests for `sl pr submit` with +# github.pr-workflow=stacked when the stacks API query fails while there are +# local changes to push. The submit must fail closed: without knowing the +# state of the stack on GitHub, base updates and pushes could corrupt it. + + +def setup_mock_github_server(repo) -> MockGitHubServer: + github_server = MockGitHubServer() + + github_server.expect_get_repository_request().and_respond() + + for desc, num in [("one", 42), ("insert", 45), ("two", 43)]: + commit = scmutil.revsingle(repo, f"desc({desc})").hex() + github_server.expect_get_pr_details_request(num).and_respond( + f"PR_id_{num}", head_ref_oid=commit + ) + # #44 is stale: its commit was amended. + github_server.expect_get_pr_details_request(44).and_respond("PR_id_44") + + github_server.expect_get_stack_request(42).and_respond_error( + "mock stacks API failure" + ) + + 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_unstack_remnant.py b/eden/scm/tests/github/mock_unstack_remnant.py new file mode 100644 index 0000000000000..ae6054d05c03b --- /dev/null +++ b/eden/scm/tests/github/mock_unstack_remnant.py @@ -0,0 +1,55 @@ +# 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, +) + +# An extension to mock network requests for `sl pr submit --restack` with +# github.pr-workflow=stacked when dissolving the diverged stack only +# partially succeeds: pull requests that are merged or queued for merge +# cannot be unstacked and are left in place. Recreating the stack is not +# possible while they remain in the old one, so the submit must abort before +# updating or pushing anything. + + +def setup_mock_github_server(repo) -> MockGitHubServer: + github_server = MockGitHubServer() + + github_server.expect_get_repository_request().and_respond() + + for desc, num in [("one", 42), ("insert", 45), ("two", 43)]: + commit = scmutil.revsingle(repo, f"desc({desc})").hex() + github_server.expect_get_pr_details_request(num).and_respond( + f"PR_id_{num}", head_ref_oid=commit + ) + # #44 is stale: its commit was amended. + github_server.expect_get_pr_details_request(44).and_respond("PR_id_44") + + # The stack on GitHub has a different order than the local stack. + github_server.expect_get_stack_request(42).and_respond( + stack_number=100, pr_numbers=[43, 42, 45, 44] + ) + + # Dissolving the stack leaves #43 in place (e.g., it is queued for + # merge). + github_server.expect_unstack_request(100).and_respond( + remaining_pr_numbers=[43] + ) + + 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_unstack_remnant_sync.py b/eden/scm/tests/github/mock_unstack_remnant_sync.py new file mode 100644 index 0000000000000..220879e4dce74 --- /dev/null +++ b/eden/scm/tests/github/mock_unstack_remnant_sync.py @@ -0,0 +1,51 @@ +# 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, +) + +# Like mock_unstack_remnant.py, but for the case where all local commits are +# up-to-date (nothing to push): dissolving the diverged stack happens during +# the stack sync at the end of the submit, and a partial dissolution is +# reported as a warning (the stack is not recreated). + + +def setup_mock_github_server(repo) -> MockGitHubServer: + github_server = MockGitHubServer() + + github_server.expect_get_repository_request().and_respond() + + for desc, num in [("one", 42), ("two", 43)]: + commit = scmutil.revsingle(repo, f"desc({desc})").hex() + github_server.expect_get_pr_details_request(num).and_respond( + f"PR_id_{num}", head_ref_oid=commit + ) + + # The stack on GitHub has a different order than the local stack. + github_server.expect_get_stack_request(42).and_respond( + stack_number=100, pr_numbers=[43, 42] + ) + + # Dissolving the stack leaves #43 in place (e.g., it is queued for + # merge), so the stack cannot be recreated. + github_server.expect_unstack_request(100).and_respond( + remaining_pr_numbers=[43] + ) + + 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/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-pull-stacked.t b/eden/scm/tests/test-ext-github-pr-pull-stacked.t new file mode 100644 index 0000000000000..e01e57ee518b6 --- /dev/null +++ b/eden/scm/tests/test-ext-github-pr-pull-stacked.t @@ -0,0 +1,47 @@ +#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 + +build up a github repo whose commits correspond to the pull requests of a +native GitHub stack (the commits are already present locally, so no actual +network pull is necessary) + + $ sl init --git repo1 + $ cd repo1 + $ echo a > a1 + $ sl ci -Aqm one + $ echo a >> a1 + $ sl ci -Aqm two + $ echo b > b1 + $ sl ci -Aqm three + +no commits are linked to pull requests yet + $ sl log -T '{desc} {github_pull_request_number}\n' + three + two + one + +pulling a PR whose body has no stack list footer, but which is part of a +native GitHub stack, links its ancestors using the stacks API + $ sl pr pull 44 --config extensions.pr_pull=$TESTDIR/github/mock_pull_stacked_pr.py + imported #44 as f4185fef85f10d46b859c30076243068b0f59245 + $ sl log -T '{desc} {github_pull_request_number}\n' + three 44 + two 43 + one 42 + +pulling the bottom of a native stack has no ancestors to link: no warning is +printed + $ sl pr pull 42 --config extensions.pr_pull=$TESTDIR/github/mock_pull_stacked_pr_bottom.py + imported #42 as ebe5b8faff36687becb7bdbca1e6a61dac428834 + +pulling a PR that is associated with a native stack but is no longer one of +its open members (e.g., it was merged) warns specifically instead of +pretending there is no stack information; ancestors are not linked + $ sl pr pull 45 --config extensions.pr_pull=$TESTDIR/github/mock_pull_stacked_pr_merged.py + imported #45 as f4185fef85f10d46b859c30076243068b0f59245 + #45 is no longer an open member of stack #100. + Ancestors will not be linked to pull requests. 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-placeholder.t b/eden/scm/tests/test-ext-github-pr-submit-stacked-placeholder.t new file mode 100644 index 0000000000000..1695ac966c80b --- /dev/null +++ b/eden/scm/tests/test-ext-github-pr-submit-stacked-placeholder.t @@ -0,0 +1,28 @@ +#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 + $ setconfig github.placeholder-strategy=True + $ echo a > a1 + $ sl ci -Aqm one + $ echo a >> a1 + $ sl ci -Aqm two + +submitting a stack of 2 commits with the placeholder strategy creates the PRs +(with chained bases) and links them into a native GitHub stack + + $ sl pr submit --config extensions.pr_submit=$TESTDIR/github/mock_create_stacked_prs_placeholder.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 + created stack: https://github.com/facebook/test_github_repo/stacks/100 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..7429e0c92a5a8 --- /dev/null +++ b/eden/scm/tests/test-ext-github-pr-submit-stacked.t @@ -0,0 +1,201 @@ +#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 + +submitting a stack of 2 commits creates the PRs (with chained bases, like the +"single" workflow, and without the stack list footer) and links them into a +native GitHub stack + $ sl pr submit --config extensions.pr_submit=$TESTDIR/github/mock_create_stacked_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 + created stack: https://github.com/facebook/test_github_repo/stacks/100 + +adding a commit on top and resubmitting appends the new PR to the existing +stack; the bases of #42 and #43 are NOT updated via the API (GitHub rejects +base changes for PRs in a native stack) + $ echo b > b1 + $ sl ci -Aqm three + $ sl pr submit --config extensions.pr_submit=$TESTDIR/github/mock_extend_stacked_prs.py + #42 is up-to-date + #43 is up-to-date + pushing 1 to https://github.com/facebook/test_github_repo.git + created new pull request: https://github.com/facebook/test_github_repo/pull/44 + updated body for https://github.com/facebook/test_github_repo/pull/44 + updated body for https://github.com/facebook/test_github_repo/pull/43 + updated body for https://github.com/facebook/test_github_repo/pull/42 + added #44 to stack #100 + +if the stack on GitHub has diverged from the local stack, warn without +modifying it + $ sl pr submit --config extensions.pr_submit=$TESTDIR/github/mock_diverged_stack.py + #42 is up-to-date + #43 is up-to-date + #44 is up-to-date + no pull requests to update + warning: stack #100 on GitHub (#43, #42, #44) does not match your local stack (#42, #43, #44); not updating it + hint[pr-submit-restack]: use 'sl pr submit --restack' to dissolve the stack on GitHub and recreate it to match your local stack + +with --restack, the diverged stack is dissolved and recreated to match the +local stack + $ sl pr submit --restack --config extensions.pr_submit=$TESTDIR/github/mock_diverged_stack.py + #42 is up-to-date + #43 is up-to-date + #44 is up-to-date + no pull requests to update + recreated stack: https://github.com/facebook/test_github_repo/stacks/101 + +if the stack on GitHub has diverged AND there are local changes to push, +abort before updating any bases or pushing anything + $ echo b >> b1 + $ sl amend + $ sl pr submit --config extensions.pr_submit=$TESTDIR/github/mock_diverged_stack_dirty.py + #42 is up-to-date + #43 is up-to-date + stack #100 on GitHub (#43, #42, #44) does not match your local stack (#42, #43, #44); not updating it + hint[pr-submit-restack]: use 'sl pr submit --restack' to dissolve the stack on GitHub and recreate it to match your local stack + abort: stack on GitHub has diverged from your local stack + [255] + +with --restack, the diverged stack is dissolved up front so that base +branches can be updated, then recreated from the local stack + $ sl pr submit --restack --config extensions.pr_submit=$TESTDIR/github/mock_diverged_stack_dirty.py + #42 is up-to-date + #43 is up-to-date + updated base for https://github.com/facebook/test_github_repo/pull/44 + updated base for https://github.com/facebook/test_github_repo/pull/43 + updated base for https://github.com/facebook/test_github_repo/pull/42 + pushing 1 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 + updated body for https://github.com/facebook/test_github_repo/pull/42 + created stack: https://github.com/facebook/test_github_repo/stacks/101 + +inserting a new commit into the middle of the stack diverges from the stack +on GitHub even though the existing PRs are in the same order (a stack can +only grow at the top): without --restack, abort before updating or pushing +anything + $ sl goto -q 'desc(one)' + $ echo c > c1 + $ sl ci -Aqm insert + $ sl rebase -qs 'desc(two)' -d . + $ sl pr submit --config extensions.pr_submit=$TESTDIR/github/mock_insert_mid_stack.py + #42 is up-to-date + new pull requests would be inserted below the top of stack #100 on GitHub (#42, #43, #44), which requires recreating the stack; not updating it + hint[pr-submit-restack]: use 'sl pr submit --restack' to dissolve the stack on GitHub and recreate it to match your local stack + abort: stack on GitHub has diverged from your local stack + [255] + +with --restack, the stack is dissolved up front, the new PR is created with +its base chained into the stack, and the stack is recreated in the new order + $ sl pr submit --restack --config extensions.pr_submit=$TESTDIR/github/mock_insert_mid_stack_restack.py + #42 is up-to-date + updated base for https://github.com/facebook/test_github_repo/pull/44 + updated base for https://github.com/facebook/test_github_repo/pull/43 + updated base for https://github.com/facebook/test_github_repo/pull/42 + pushing 3 to https://github.com/facebook/test_github_repo.git + created new pull request: https://github.com/facebook/test_github_repo/pull/45 + updated body for https://github.com/facebook/test_github_repo/pull/44 + updated body for https://github.com/facebook/test_github_repo/pull/43 + updated body for https://github.com/facebook/test_github_repo/pull/45 + updated body for https://github.com/facebook/test_github_repo/pull/42 + created stack: https://github.com/facebook/test_github_repo/stacks/101 + +if the stacks API query fails while there are local changes to push, fail +closed: without knowing the state of the stack, pushing could corrupt it + $ sl goto -q 'desc(three)' + $ echo b >> b1 + $ sl amend + $ sl pr submit --config extensions.pr_submit=$TESTDIR/github/mock_stack_query_failure.py + #42 is up-to-date + #45 is up-to-date + #43 is up-to-date + warning, could not query stacks for #42: mock stacks API failure + abort: could not determine the state of the stack on GitHub; re-run 'pr submit' to retry + [255] + +if dissolving a diverged stack only partially succeeds (merged or queued +PRs cannot be unstacked), abort rather than proceed against the remnant + $ sl pr submit --restack --config extensions.pr_submit=$TESTDIR/github/mock_unstack_remnant.py + #42 is up-to-date + #45 is up-to-date + #43 is up-to-date + abort: stack #100 was only partially dissolved: #43 could not be unstacked (merged or queued pull requests are left in place) + [255] + +a single open pull request whose stack on GitHub has more members is a +divergence; --restack refuses to dissolve the stack since it could not be +recreated with fewer than two pull requests + $ cd .. + $ sl init --git repo2 + $ cd repo2 + $ echo a > a1 + $ sl ci -Aqm "one + > + > Pull Request resolved: https://github.com/facebook/test_github_repo/pull/42" + $ echo a >> a1 + $ sl amend + $ sl pr submit --config extensions.pr_submit=$TESTDIR/github/mock_restack_too_small.py + stack #100 on GitHub (#42, #43) does not match your local stack (#42); not updating it + hint[pr-submit-restack]: use 'sl pr submit --restack' to dissolve the stack on GitHub and recreate it to match your local stack + abort: stack on GitHub has diverged from your local stack + [255] + $ sl pr submit --restack --config extensions.pr_submit=$TESTDIR/github/mock_restack_too_small.py + abort: --restack would leave stack #100 with fewer than two pull requests; dissolve it on GitHub instead if that is intended + [255] + +a closed stack is treated the same as no stack: the base branch is updated +via the API and no stack operations are attempted + $ sl pr submit --config extensions.pr_submit=$TESTDIR/github/mock_closed_stack.py + updated base for https://github.com/facebook/test_github_repo/pull/42 + pushing 1 to https://github.com/facebook/test_github_repo.git + updated body for https://github.com/facebook/test_github_repo/pull/42 + +if dissolving the stack during the post-submit sync partially fails, warn +without recreating the stack (the pull requests themselves were already +submitted) + $ echo b > b1 + $ sl ci -Aqm "two + > + > Pull Request resolved: https://github.com/facebook/test_github_repo/pull/43" + $ sl pr submit --restack --config extensions.pr_submit=$TESTDIR/github/mock_unstack_remnant_sync.py + #42 is up-to-date + #43 is up-to-date + no pull requests to update + warning, stack #100 was only partially dissolved: #43 could not be unstacked (merged or queued pull requests are left in place) + +stacks are not supported across forks: pull requests are created against the +upstream default branch and a warning is printed instead of creating a stack + $ cd .. + $ sl init --git repo3 + $ cd repo3 + $ echo a > a1 + $ sl ci -Aqm one + $ echo a >> a1 + $ sl ci -Aqm two + $ sl pr submit --config extensions.pr_submit=$TESTDIR/github/mock_fork_stacked.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 + warning: GitHub does not support stacks across forks; pull requests were submitted without a stack diff --git a/website/docs/git/github.md b/website/docs/git/github.md index 084548012ec3b..e3576aabb1baa 100644 --- a/website/docs/git/github.md +++ b/website/docs/git/github.md @@ -48,10 +48,11 @@ See the dedicated [Sapling Stack](/docs/git/sapling-stack.md) page for more info **Pros:** - Works with any GitHub repo. +- With `github.pr-workflow=stacked`, integrates with GitHub's native [stacked pull requests](https://docs.github.com/en/pull-requests/get-started/about-stacked-prs) feature, so GitHub renders the stack in the pull request UI and can merge it from the bottom up. See [Pull request workflows](/docs/git/sapling-stack.md#pull-request-workflows). **Cons:** -- Creates "overlapping" pull requests that may be confusing to reviewers using the GitHub pull request UI. Reviewers are strongly encouraged to use [ReviewStack](/docs/addons/reviewstack.md) for code review instead of GitHub. +- By default, creates "overlapping" pull requests that may be confusing to reviewers using the GitHub pull request UI. Reviewers are strongly encouraged to use [ReviewStack](/docs/addons/reviewstack.md) for code review instead of GitHub, or to switch to the `stacked` workflow. :::tip diff --git a/website/docs/git/sapling-stack.md b/website/docs/git/sapling-stack.md index d5a1d50c5fff0..12e9eceb5df92 100644 --- a/website/docs/git/sapling-stack.md +++ b/website/docs/git/sapling-stack.md @@ -14,8 +14,28 @@ Make sure you have followed the instructions to [authenticate with GitHub using :::caution -`sl pr submit` creates _overlapping_ commits where each pull request contains the commit that is intended to be reviewed as part of the pull request as well as all commits below it in the stack. This will not "look right" on GitHub, so collaborators who use this command are encouraged to use [ReviewStack](/docs/addons/reviewstack.md) to review these pull requests, as ReviewStack will present only the commit that is intended to be reviewed for each pull request. +By default, `sl pr submit` creates _overlapping_ pull requests where each pull request contains the commit that is intended to be reviewed as part of the pull request as well as all commits below it in the stack. This will not "look right" on GitHub, so collaborators who use this command are encouraged to use [ReviewStack](/docs/addons/reviewstack.md) to review these pull requests, as ReviewStack will present only the commit that is intended to be reviewed for each pull request. Alternatively, see [Pull request workflows](#pull-request-workflows) below for the `stacked` workflow, which uses GitHub's native support for stacked pull requests. ::: +## Pull request workflows + +The `github.pr-workflow` config option controls how `sl pr submit` maps a stack of commits onto pull requests: + +- `overlap` (default): every pull request targets the repository's default branch, so each pull request contains its commit plus all commits below it in the stack. Best reviewed with [ReviewStack](/docs/addons/reviewstack.md). +- `single`: each pull request contains exactly one commit and targets the head branch of the pull request below it in the stack, so each pull request shows only the diff for its own commit. +- `stacked`: like `single`, but the pull requests are also linked together using GitHub's native [stacked pull requests](https://docs.github.com/en/pull-requests/get-started/about-stacked-prs) feature (public preview), so GitHub renders the stack in the pull request UI and can merge and retarget it from the bottom up. + +To enable the native stacked workflow: + +``` +sl config --user github.pr-workflow stacked +``` + +Notes on the `stacked` workflow: + +- GitHub requires all branches of a stack to be in the same repository, so native stacks are not available when contributing from a fork. Pull requests are still created; they are just not linked into a stack. +- If the stack on GitHub no longer matches your local stack (for example, because you reordered or removed commits, or the stack was modified on GitHub), `sl pr submit` warns without modifying it. Run `sl pr submit --restack` to dissolve the stack on GitHub and recreate it to match your local stack. +- Pull requests created with this workflow omit the "Stack created with Sapling" footer from their descriptions, since GitHub displays the stack natively. + If you get into a funny state, try using `sl pr link` or `sl pr unlink` to add or remove associations between commits and pull requests, as appropriate.