From a20167292627e1799b79031bf3314e65802d5417 Mon Sep 17 00:00:00 2001 From: Ray Liu Date: Fri, 31 Jul 2026 13:13:38 -0400 Subject: [PATCH 1/7] [1/n][sl][github][gh stacks] support headers and list params ### ctx github added native stacked pull requests, it would be nice to add native support to it for sapling! this stack lets `sl pr submit` and `sl pull` via a new `github.pr.workflow = stacked` mode ### changes made - `github_gh_cli.make_request` accepts custom headers (`-H`, needed for the `X-GitHub-Api-Version` preview header) - `_format_param` supports list values using the `gh api` repeated-field syntax (`pull_requests[]=101`) - an empty list is passed explicitly as `key[]` without a value (per the gh api manual) rather than dropped, since an empty array and a missing field can mean different things to an endpoint - mock_utils: `MockGitHubServer` now tracks which expectations were consumed; tests can opt in via `wrap_with_consumption_check` to fail when an expected request silently stops happening (closes the old TODO) ### test plan doctests in `_format_param` (registered in test-doctest.py, including the empty-array case), existing github .t suite. verified the empty-array wire format empirically: `gh api -F "pull_requests[]" --verbose` sends `{"pull_requests": []}` --- eden/scm/sapling/ext/github/github_gh_cli.py | 50 ++++++++++++++++++-- eden/scm/sapling/ext/github/mock_utils.py | 47 +++++++++++++++++- eden/scm/tests/test-doctest.py | 1 + 3 files changed, 92 insertions(+), 6 deletions(-) diff --git a/eden/scm/sapling/ext/github/github_gh_cli.py b/eden/scm/sapling/ext/github/github_gh_cli.py index d87248f6c8dea..593493bf06e1c 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()])) ) @@ -82,7 +93,38 @@ async def _make_request( ) -def _format_param(key: str, value: Union[str, int, bool]) -> List[str]: +def _format_param(key: str, value: ParamValue) -> List[str]: + r"""Formats a param as a list of arguments to pass to `gh api`. + + >>> _format_param("body", "hello") + ['-f', 'body=hello'] + >>> _format_param("number", 42) + ['-F', 'number=42'] + >>> _format_param("draft", True) + ['-F', 'draft=true'] + + Array values use the `gh api` repeated-field syntax, e.g. + `-F "pull_requests[]=101" -F "pull_requests[]=102"`: + + >>> _format_param("pull_requests", [101, 102]) + ['-F', 'pull_requests[]=101', '-F', 'pull_requests[]=102'] + >>> _format_param("labels", ["bug", "help wanted"]) + ['-f', 'labels[]=bug', '-f', 'labels[]=help wanted'] + + An empty array is passed explicitly as `key[]` without a value (per the + `gh api` manual) rather than dropped: an empty array and a missing field + can have different meanings to an endpoint: + + >>> _format_param("empty", []) + ['-F', 'empty[]'] + """ + if isinstance(value, list): + if not value: + return ["-F", f"{key}[]"] + return list( + itertools.chain(*[_format_param(f"{key}[]", v) for v in value]) + ) + # In Python, bool is a subclass of int, so check it first. if isinstance(value, bool): opt = "-F" diff --git a/eden/scm/sapling/ext/github/mock_utils.py b/eden/scm/sapling/ext/github/mock_utils.py index added9869489b..06e31e5452888 100644 --- a/eden/scm/sapling/ext/github/mock_utils.py +++ b/eden/scm/sapling/ext/github/mock_utils.py @@ -28,8 +28,11 @@ REPO_ID = "R_test_github_repo" USER_NAME = "facebook_username" -ParamsType = Dict[str, Union[bool, int, str]] -MakeRequestType = Callable[[ParamsType, str, str, Optional[str]], Result[JsonDict, str]] +ParamsType = Dict[str, Union[bool, int, str, List[bool], List[int], List[str]]] +MakeRequestType = Callable[ + [ParamsType, str, str, Optional[str], Optional[Dict[str, str]]], + Result[JsonDict, str], +] RunGitCommandType = Callable[[List[str], str], bytes] @@ -75,6 +78,7 @@ class MockGitHubServer: def __init__(self, hostname: str = GITHUB_HOSTNAME): self.hostname: str = hostname self.requests: Dict[str, MockRequest] = {} + self._consumed_keys: set = set() async def make_request( self, @@ -83,10 +87,15 @@ async def make_request( hostname: str, endpoint: str = "graphql", method: Optional[str] = None, + headers: Optional[Dict[str, str]] = None, ) -> Result[JsonDict, str]: """Wrapper function for `github_gh_cli.make_request`. It reads mock data from `self.requests` instead of sending network requests. + + Note that `headers` is intentionally not part of the request key: the + headers we send (e.g., X-GitHub-Api-Version) do not affect which mock + response should be returned. """ assert real_make_request.__name__ == "_make_request", ( f"expected '_make_request', but got '{real_make_request.__name__}'" @@ -96,11 +105,45 @@ async def make_request( if key not in self.requests: raise MockRequestNotFound(key, self.requests) + self._consumed_keys.add(key) return self.requests[key].get_response() def _add_request(self, request_key: str, request: "MockRequest") -> None: self.requests[request_key] = request + def unconsumed_requests(self) -> List[str]: + """Keys of expectations that were never requested.""" + return sorted(k for k in self.requests if k not in self._consumed_keys) + + def report_unconsumed(self, ui) -> None: + """Prints a warning for every expectation that was never requested. + + Intended to be called after the command under test has finished (see + wrap_with_consumption_check). Tests that use this produce no extra + output when all expectations were consumed, so any warning makes the + test fail: this catches code paths that silently stopped making a + request the test author expected. + """ + for key in self.unconsumed_requests(): + first_line = key.splitlines()[0] + ui.status_err(f"warning, unconsumed mock request: {first_line}\n") + + +def wrap_with_consumption_check(server: "MockGitHubServer", module, funcname) -> None: + """Wraps `module.funcname` (a command function taking `ui` as its first + argument) so that unconsumed mock expectations are reported after the + command finishes, even if it aborts. + """ + from sapling import extensions + + def wrapped(orig, ui, *args, **kwargs): + try: + return orig(ui, *args, **kwargs) + finally: + server.report_unconsumed(ui) + + extensions.wrapfunction(module, funcname, wrapped) + def expect_get_repository_request( self, owner: str = OWNER, name: str = REPO_NAME ) -> "GetRepositoryRequest": diff --git a/eden/scm/tests/test-doctest.py b/eden/scm/tests/test-doctest.py index e0f5d40b354df..90d1726ff9426 100644 --- a/eden/scm/tests/test-doctest.py +++ b/eden/scm/tests/test-doctest.py @@ -32,6 +32,7 @@ def testmod(name, optionflags=0, testtarget=None): testmod("sapling.pathlog") testmod("sapling.ext.github.archive_commit") +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") From 39097a05f7a003a64f4cea1497a1c47c065a53d6 Mon Sep 17 00:00:00 2001 From: Ray Liu Date: Thu, 13 Aug 2026 12:02:29 -0400 Subject: [PATCH 2/7] [2/n][sl][github][gh stacks] add support for stack endpoint ### ctx basic plumbing for github native stacks REST API ### changes made - `gh_submit`: `StackDetails` dataclass + `get_stack_for_pull_request`, `create_stack`, `add_prs_to_stack`, `unstack`, all pinned to the `2026-03-10` preview API version - `unstack` returns the remaining stack when dissolution is partial (merged/queued diffs cannot be unstacked) so callers can react instead of assuming success - `update_pull_request` now takes `base: Optional[str]`: github rejects `updatePullRequest` mutations that include `baseRefName` for PRs in a native stack, so `base=None` uses a new mutation variant (`GRAPHQL_UPDATE_PULL_REQUEST_NO_BASE`) that only touches title/body ### test plan CI (doctest on `_parse_stack_from_dict`), and dogfooded on my own computer via a patch https://github.com/raydatray/rusty-mcrouter/pull/198 Screenshot 2026-07-31 at 3 20 04 PM --- eden/scm/sapling/ext/github/consts/query.py | 16 ++ eden/scm/sapling/ext/github/gh_submit.py | 176 ++++++++++++++++++- eden/scm/sapling/ext/github/github_gh_cli.py | 8 +- eden/scm/tests/test-doctest.py | 1 + 4 files changed, 195 insertions(+), 6 deletions(-) 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 593493bf06e1c..a50fde86c81e6 100644 --- a/eden/scm/sapling/ext/github/github_gh_cli.py +++ b/eden/scm/sapling/ext/github/github_gh_cli.py @@ -79,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: diff --git a/eden/scm/tests/test-doctest.py b/eden/scm/tests/test-doctest.py index 90d1726ff9426..d8e4ac5cb92f5 100644 --- a/eden/scm/tests/test-doctest.py +++ b/eden/scm/tests/test-doctest.py @@ -32,6 +32,7 @@ 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") From f79b0f3a0c92653014077722cd73de5ca4e4328f Mon Sep 17 00:00:00 2001 From: Ray Liu Date: Thu, 13 Aug 2026 12:02:29 -0400 Subject: [PATCH 3/7] [3/n][sl][github][gh stacks] add stacked workflow for submit ### ctx introduces the new native github stack workflow ### changes made - new `SubmitWorkflow.STACKED` variant, selected via `github.pr-workflow=stacked` - chains each PR's base to the head branch of the PR below it (like `single`), factored into `SubmitWorkflow.uses_chained_bases()` - hardened chained-base selection everywhere it happens (base-update loop, body rewrites, serial and placeholder creation): closed/merged diffs are skipped when picking the base below (their head branches would break the chain), and forks never chain (fork head branches cannot be bases on the upstream repo, so fall back to the default branch) ### test plan - new test-ext-github-pr-submit-stacked.t (initial submit) - test-ext-github-pr-submit-closed.t: open diffs stacked on a closed one chain past it to main - test-ext-github-pr-submit-placeholder-issue.t: placeholder strategy on a fork creates diffs against the upstream default branch --- eden/scm/sapling/ext/github/submit.py | 67 +++++++++++--- .../scm/tests/github/mock_closed_mid_stack.py | 76 ++++++++++++++++ eden/scm/tests/github/mock_create_prs.py | 5 +- .../mock_create_prs_placeholder_fork.py | 89 +++++++++++++++++++ .../tests/github/mock_create_prs_with_open.py | 5 +- .../tests/test-ext-github-pr-submit-closed.t | 22 +++++ ...t-ext-github-pr-submit-placeholder-issue.t | 20 +++++ .../tests/test-ext-github-pr-submit-stacked.t | 29 ++++++ 8 files changed, 297 insertions(+), 16 deletions(-) create mode 100644 eden/scm/tests/github/mock_closed_mid_stack.py create mode 100644 eden/scm/tests/github/mock_create_prs_placeholder_fork.py create mode 100644 eden/scm/tests/test-ext-github-pr-submit-stacked.t diff --git a/eden/scm/sapling/ext/github/submit.py b/eden/scm/sapling/ext/github/submit.py index 423bdaff16bcf..6a84aefdc3fa0 100644 --- a/eden/scm/sapling/ext/github/submit.py +++ b/eden/scm/sapling/ext/github/submit.py @@ -70,6 +70,19 @@ class SubmitWorkflow(Enum): """ OVERLAP = "overlap" + """Like SINGLE, but additionally links the pull requests together using + GitHub's native "stacked pull requests" feature so GitHub renders the + stack natively and can merge/retarget it bottom-up: + https://docs.github.com/en/pull-requests/get-started/about-stacked-prs + """ + STACKED = "stacked" + + def uses_chained_bases(self) -> bool: + """Whether each PR in the stack uses the head branch of the PR below + it as its base branch (as opposed to all PRs sharing a common base). + """ + return self in (SubmitWorkflow.SINGLE, SubmitWorkflow.STACKED) + @staticmethod def from_config(ui) -> "SubmitWorkflow": workflow = ui.config( @@ -80,6 +93,8 @@ def from_config(ui) -> "SubmitWorkflow": return SubmitWorkflow.OVERLAP elif workflow == "single": return SubmitWorkflow.SINGLE + elif workflow == "stacked": + return SubmitWorkflow.STACKED else: # Note that "classic" is not recognized yet. ui.warn( @@ -212,13 +227,14 @@ def get_gitdir() -> str: repository = params.repository - # For the SINGLE workflow, we must update the base branch on existing PRs - # BEFORE pushing the new branch contents. Otherwise, when commits are - # reordered in the stack, GitHub may see that a PR's commits already exist - # in its (old) base branch and auto-close the PR as "merged". + # For workflows with chained base branches (SINGLE, STACKED), we must + # update the base branch on existing PRs BEFORE pushing the new branch + # contents. Otherwise, when commits are reordered in the stack, GitHub may + # see that a PR's commits already exist in its (old) base branch and + # auto-close the PR as "merged". # # See https://github.com/facebook/sapling/issues/1275 - if workflow == SubmitWorkflow.SINGLE: + if workflow.uses_chained_bases(): existing_prs = [ p for p in partitions if p[0].pr and p[0].pr.state == PullRequestState.OPEN ] @@ -235,8 +251,17 @@ def get_gitdir() -> str: if not pr or pr.state != PullRequestState.OPEN: continue base = repository.get_base_branch() - if index < len(partitions) - 1: - base = none_throws(partitions[index + 1][0].head_branch_name) + # Chain to the nearest partition below whose pull request is + # open (or that will get a new pull request). Closed/merged + # pull requests are skipped: using their head branches as + # bases would break the chain. + for below in partitions[index + 1 :]: + below_head = below[0] + below_pr = below_head.pr + if below_pr and below_pr.state != PullRequestState.OPEN: + continue + base = none_throws(below_head.head_branch_name) + break result = await gh_submit.update_pull_request( repository.hostname, pr.node_id, pr.title, pr.body, base ) @@ -327,8 +352,19 @@ async def rewrite_pull_request_body( # stack to the bottom. partition = partitions[index] base = repository.get_base_branch() - if workflow == SubmitWorkflow.SINGLE and index < len(partitions) - 1: - base = none_throws(partitions[index + 1][0].head_branch_name) + if workflow.uses_chained_bases() and not repository.is_fork: + # Chain to the nearest partition below whose pull request is open (or + # new). Closed/merged pull requests are skipped: using their head + # branches as bases would break the chain. For forks, chained bases + # are not possible at all (the head branches live on the fork, but a + # base branch must be a branch on the upstream repository), so the + # default base branch is kept. + for below in partitions[index + 1 :]: + below_pr = below[0].pr + if below_pr and below_pr.state != PullRequestState.OPEN: + continue + base = none_throws(below[0].head_branch_name) + break head_commit_data = partition[0] @@ -509,7 +545,7 @@ async def create_pull_requests_serially( parent = None for commit, branch_name in commits: base = repository.get_base_branch() - if workflow == SubmitWorkflow.SINGLE and parent: + if workflow.uses_chained_bases() and parent: base = none_throws(parent.head_branch_name) commit_msg = commit.get_msg() @@ -599,7 +635,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 +697,11 @@ async def create_pull_request(params: PullRequestParams): issue_number = params.number # Note that "overlapping" pull requests will all share the same base. + # For forks, chained bases are not possible either: the head branches + # live on the fork, but the base branch of a pull request must be a + # branch on the upstream repository. base = base_branch_for_repo - if workflow == SubmitWorkflow.SINGLE: + if workflow.uses_chained_bases() and not repository.is_fork: parent = params.parent if parent: base = none_throws(parent.head_branch_name) diff --git a/eden/scm/tests/github/mock_closed_mid_stack.py b/eden/scm/tests/github/mock_closed_mid_stack.py new file mode 100644 index 0000000000000..284cb55beb0d6 --- /dev/null +++ b/eden/scm/tests/github/mock_closed_mid_stack.py @@ -0,0 +1,76 @@ +# Copyright (c) Meta Platforms, Inc. and affiliates. +# +# This software may be used and distributed according to the terms of the +# GNU General Public License version 2. + +from sapling import extensions, scmutil +from sapling.ext.github import github_gh_cli, submit +from sapling.ext.github.gh_submit import PullRequestState +from sapling.ext.github.mock_utils import ( + mock_run_git_command, + MockGitHubServer, + wrap_with_consumption_check, +) + +# An extension to mock network requests for `sl pr submit` with +# github.pr-workflow=single when the pull request at the bottom of the stack +# (#42) is closed: the base branches of the pull requests above it must skip +# the closed pull request's head branch (which would break the chain) and +# fall through to the default base branch. +# +# This mock is set up in `reposetup` so that expectations can be derived from +# the actual commit hashes in the test repo instead of hardcoding them. + + +def setup_mock_github_server(repo) -> MockGitHubServer: + github_server = MockGitHubServer() + + github_server.expect_get_repository_request().and_respond() + + # All three commits are linked to pull requests via "Pull Request + # resolved" lines in their commit messages. None of the head OIDs match + # the local commits, so all three head branches are pushed (pushing to a + # closed pull request's branch is harmless and preexisting behavior). + github_server.expect_get_pr_details_request(42).and_respond( + "PR_id_42", state=PullRequestState.CLOSED + ) + github_server.expect_get_pr_details_request(43).and_respond("PR_id_43") + github_server.expect_get_pr_details_request(44).and_respond("PR_id_44") + + # Base updates before the push: #44 chains to the open #43 below it, but + # #43 must NOT chain to the closed #42 below it: it falls through to the + # default base branch instead. (No base update is attempted for the + # closed #42 itself.) + github_server.expect_update_pr_request( + "PR_id_44", 44, "", base="pr43" + ).and_respond() + github_server.expect_update_pr_request( + "PR_id_43", 43, "", base="main" + ).and_respond() + + # Body rewrites follow the same base rules. The stack list footer still + # lists all three pull requests (including the closed one). + msg_two = "two\n\nPull Request resolved: https://github.com/facebook/test_github_repo/pull/43" + msg_three = "three\n\nPull Request resolved: https://github.com/facebook/test_github_repo/pull/44" + github_server.expect_update_pr_request( + "PR_id_44", 44, msg_three, base="pr43", stack_pr_ids=[42, 43, 44] + ).and_respond() + github_server.expect_update_pr_request( + "PR_id_43", 43, msg_two, base="main", stack_pr_ids=[42, 43, 44] + ).and_respond() + + github_server.expect_get_username_request().and_respond() + + tip = scmutil.revsingle(repo, "desc(three)").hex() + github_server.expect_merge_into_branch(tip).and_respond() + + return github_server + + +def reposetup(ui, repo): + github_server = setup_mock_github_server(repo) + extensions.wrapfunction( + github_gh_cli, "_make_request", github_server.make_request + ) + extensions.wrapfunction(submit, "run_git_command", mock_run_git_command) + wrap_with_consumption_check(github_server, submit, "submit") diff --git a/eden/scm/tests/github/mock_create_prs.py b/eden/scm/tests/github/mock_create_prs.py index 2bd5cd653b503..b438db76fde0b 100644 --- a/eden/scm/tests/github/mock_create_prs.py +++ b/eden/scm/tests/github/mock_create_prs.py @@ -26,14 +26,15 @@ 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. + chained = ui.config("github", "pr-workflow") in ("single", "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( 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..78fdf93d393c1 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,15 @@ 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. + chained = ui.config("github", "pr-workflow") in ("single", "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( diff --git a/eden/scm/tests/test-ext-github-pr-submit-closed.t b/eden/scm/tests/test-ext-github-pr-submit-closed.t index cf4244c465667..aad56fd40716b 100644 --- a/eden/scm/tests/test-ext-github-pr-submit-closed.t +++ b/eden/scm/tests/test-ext-github-pr-submit-closed.t @@ -19,3 +19,25 @@ test we don't try updating a closed pr: pushing 1 to https://github.com/facebook/test_github_repo.git warning, not updating #42 because it isn't open hint[unlink-closed-pr]: to create a new PR, disassociate commit(s) using 'sl pr unlink' then re-run 'sl pr submit' + +test chained bases skip a closed pull request: two open pull requests are +stacked on top of the closed #42. #43 must NOT use the closed #42's head +branch as its base (that would break the chain); it falls through to the +default base branch instead. + + $ echo b > b1 + $ sl ci -Aqm "two + > + > Pull Request resolved: https://github.com/facebook/test_github_repo/pull/43" + $ echo c > c1 + $ sl ci -Aqm "three + > + > Pull Request resolved: https://github.com/facebook/test_github_repo/pull/44" + $ sl pr submit --config extensions.pr_submit=$TESTDIR/github/mock_closed_mid_stack.py + updated base for https://github.com/facebook/test_github_repo/pull/44 + updated base for https://github.com/facebook/test_github_repo/pull/43 + pushing 3 to https://github.com/facebook/test_github_repo.git + updated body for https://github.com/facebook/test_github_repo/pull/44 + updated body for https://github.com/facebook/test_github_repo/pull/43 + warning, not updating #42 because it isn't open + hint[unlink-closed-pr]: to create a new PR, disassociate commit(s) using 'sl pr unlink' then re-run 'sl pr submit' diff --git a/eden/scm/tests/test-ext-github-pr-submit-placeholder-issue.t b/eden/scm/tests/test-ext-github-pr-submit-placeholder-issue.t index 8a60df92c32a3..115fa0f01fbb3 100644 --- a/eden/scm/tests/test-ext-github-pr-submit-placeholder-issue.t +++ b/eden/scm/tests/test-ext-github-pr-submit-placeholder-issue.t @@ -23,3 +23,23 @@ test sending pr pushing 1 to https://github.com/facebook/test_github_repo.git created new pull request: https://github.com/facebook/test_github_repo/pull/1 updated body for https://github.com/facebook/test_github_repo/pull/1 + +test the placeholder strategy with a chained-bases workflow on a fork: fork +head branches cannot be used as base branches on the upstream repository, so +both pull requests are created against the upstream default branch + + $ cd .. + $ sl init --git repo2 + $ cd repo2 + $ setconfig github.placeholder-strategy=True + $ setconfig github.pr-workflow=single + $ echo a > a1 + $ sl ci -Aqm one + $ echo a >> a1 + $ sl ci -Aqm two + $ sl pr submit --config extensions.pr_submit=$TESTDIR/github/mock_create_prs_placeholder_fork.py + pushing 2 to https://github.com/facebook/test_github_repo.git + created new pull request: https://github.com/upstream/test_github_repo/pull/42 + created new pull request: https://github.com/upstream/test_github_repo/pull/43 + updated body for https://github.com/upstream/test_github_repo/pull/43 + updated body for https://github.com/upstream/test_github_repo/pull/42 diff --git a/eden/scm/tests/test-ext-github-pr-submit-stacked.t b/eden/scm/tests/test-ext-github-pr-submit-stacked.t new file mode 100644 index 0000000000000..e993739e0f532 --- /dev/null +++ b/eden/scm/tests/test-ext-github-pr-submit-stacked.t @@ -0,0 +1,29 @@ +#require git no-eden no-windows + + $ eagerepo + $ enable github + $ export SL_TEST_GH_URL=https://github.com/facebook/test_github_repo.git + $ . $TESTDIR/git.sh + $ configure github.pr-workflow=stacked + +build up a github repo + + $ sl init --git repo1 + $ cd repo1 + $ echo a > a1 + $ sl ci -Aqm one + $ echo a >> a1 + $ sl ci -Aqm two + +confirm it is a 'github_repo' + $ sl log -r. -T '{github_repo}\n' + True + +test sending pr: each PR's base should be chained to the PR below it, same as +the "single" workflow (native stack linking is tested separately) + $ sl pr submit --config extensions.pr_submit=$TESTDIR/github/mock_create_prs.py + pushing 2 to https://github.com/facebook/test_github_repo.git + created new pull request: https://github.com/facebook/test_github_repo/pull/42 + created new pull request: https://github.com/facebook/test_github_repo/pull/43 + updated body for https://github.com/facebook/test_github_repo/pull/43 + updated body for https://github.com/facebook/test_github_repo/pull/42 From 6d6375d9c3051666b5c55f0ec38826cef93deabc Mon Sep 17 00:00:00 2001 From: Ray Liu Date: Thu, 13 Aug 2026 12:38:19 -0400 Subject: [PATCH 4/7] [4/n][sl][github][gh stacks] update submit body for stacks ### ctx github renders native stacks in the PR UI itself so the sapling footer would be redundant - lets remove it when submitting via native stacks ### changes made - `create_pull_request_title_and_body` takes a `stack_list` flag; the stacked workflow omits the footer, other workflows are unchanged ### test plan updated mocks + test-ext-github-pr-submit-stacked.t --- eden/scm/sapling/ext/github/pull_request_body.py | 15 ++++++++++++++- eden/scm/sapling/ext/github/submit.py | 4 ++++ eden/scm/tests/github/mock_create_prs.py | 12 ++++++++++-- .../scm/tests/github/mock_create_prs_with_open.py | 12 ++++++++++-- 4 files changed, 38 insertions(+), 5 deletions(-) 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 6a84aefdc3fa0..89d838cccc2c0 100644 --- a/eden/scm/sapling/ext/github/submit.py +++ b/eden/scm/sapling/ext/github/submit.py @@ -383,6 +383,10 @@ async def rewrite_pull_request_body( index, repository, reviewstack=ui.configbool("github", "pull-request-include-reviewstack"), + # For the native "stacked" workflow, GitHub renders the stack in the + # pull request UI itself, so the footer stack list (and ReviewStack + # link) would be redundant. + stack_list=workflow != SubmitWorkflow.STACKED, ) if pr.state != PullRequestState.OPEN: diff --git a/eden/scm/tests/github/mock_create_prs.py b/eden/scm/tests/github/mock_create_prs.py index b438db76fde0b..7979f4bd9d7f6 100644 --- a/eden/scm/tests/github/mock_create_prs.py +++ b/eden/scm/tests/github/mock_create_prs.py @@ -27,7 +27,11 @@ def setup_mock_github_server(ui) -> MockGitHubServer: ] # Both "single" and "stacked" chain each PR's base to the PR below it. - chained = ui.config("github", "pr-workflow") in ("single", "stacked") + 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) @@ -48,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_with_open.py b/eden/scm/tests/github/mock_create_prs_with_open.py index 78fdf93d393c1..4b853e23d6456 100644 --- a/eden/scm/tests/github/mock_create_prs_with_open.py +++ b/eden/scm/tests/github/mock_create_prs_with_open.py @@ -29,7 +29,11 @@ def setup_mock_github_server(ui) -> MockGitHubServer: ] # Both "single" and "stacked" chain each PR's base to the PR below it. - chained = ui.config("github", "pr-workflow") in ("single", "stacked") + 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) @@ -50,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() From 28ddf2ec74956179192659f831426a6dc8227eda Mon Sep 17 00:00:00 2001 From: Ray Liu Date: Thu, 13 Aug 2026 12:38:52 -0400 Subject: [PATCH 5/7] [5/n][sl][github][gh stacks] wire up gh stacks to sl pr submit ### ctx actually links the submitted PRs into a native github stack and deals with the various constraints github puts on stacked diffs ### changes made - after submit, diffs are linked via the stacks API: create the stack on first submit, then append new diffs when the local stack extends it at the top - the remote stack is queried up front, before any mutations, and submit fails closed: - if the query fails, abort (pushing blind against an unknown stack state could corrupt it) - if it diverged from the local stack (reorder, dropped diffs, or a new diff inserted below the top - stacks can only grow at the top), abort before updating bases or pushing. new `--restack` flag dissolves and recreates it (since the gh API has no reorder operation). tell the user to do this as a ui hint - `--restack` refuses to dissolve a stack it could not recreate (fewer than 2 diffs), and aborts if dissolution is only partial (merged/queued diffs stay behind) - closed stacks are treated as no stack - base branches of diffs already in a stack are never updated via the API (since gh rejects that, the stack manages bases on its own). body/title rewrites use the "base-less" mutation from #1393 - new diffs are created directly against the head branch below them instead of being created against main and re-based afterwards (this is impossible to do once diffs are stacked on gh). falls back to default base for forks, where gh native stacks are not supported and a warning is printed (i wanted to submit this stack with my patch to show it works but alas i cant :( ) ### test plan - test-ext-github-pr-submit-stacked.t covers: the initial submit, extending an existing stack, diverged stack with/without --restack and with/without local changes to push, mid-stack insertion with/without --restack, stacks API query failure, partial unstack (abort up front / warn during sync), --restack with fewer than 2 diffs, closed stack, fork fallback - test-ext-github-pr-submit-stacked-placeholder.t: placeholder strategy + stacked - new mocks derive commit hashes from the test repo at runtime (reposetup) instead of hardcoding them, and use the consumption check from #1392 so unexercised expectations fail the test - tested live on my own machine/repo ``` sl config --local extensions.github=/Users/ray/sapling/eden/scm/sapling/ext/github/__init__.py sl config --local extensions.signing_shim=/Users/ray/.sl-signing-shim.py sl config --local github.pr-workflow=stacked ``` - create a stack here https://github.com/raydatray/rusty-mcrouter/pull/197 - submit 197 and 198 - it creates a stack - then add 200 locally and submit again - it extends the stack - then add 202 locally and submit again - it extends the stack - go down to 198 and amend the diff locally. restack locally and resubmit - modifies only 198 and the stack is updated Screenshot 2026-07-31 at 3 42 08 PM --- eden/scm/sapling/ext/github/__init__.py | 16 + eden/scm/sapling/ext/github/mock_utils.py | 215 +++++++++- eden/scm/sapling/ext/github/submit.py | 400 +++++++++++++++++- eden/scm/tests/github/mock_closed_stack.py | 59 +++ .../tests/github/mock_create_stacked_prs.py | 77 ++++ .../mock_create_stacked_prs_placeholder.py | 73 ++++ eden/scm/tests/github/mock_diverged_stack.py | 56 +++ .../tests/github/mock_diverged_stack_dirty.py | 88 ++++ .../tests/github/mock_extend_stacked_prs.py | 87 ++++ eden/scm/tests/github/mock_fork_stacked.py | 80 ++++ .../scm/tests/github/mock_insert_mid_stack.py | 59 +++ .../github/mock_insert_mid_stack_restack.py | 96 +++++ .../tests/github/mock_restack_too_small.py | 44 ++ .../tests/github/mock_stack_query_failure.py | 46 ++ eden/scm/tests/github/mock_unstack_remnant.py | 55 +++ .../tests/github/mock_unstack_remnant_sync.py | 51 +++ ...ext-github-pr-submit-stacked-placeholder.t | 28 ++ .../tests/test-ext-github-pr-submit-stacked.t | 178 +++++++- 18 files changed, 1686 insertions(+), 22 deletions(-) create mode 100644 eden/scm/tests/github/mock_closed_stack.py create mode 100644 eden/scm/tests/github/mock_create_stacked_prs.py create mode 100644 eden/scm/tests/github/mock_create_stacked_prs_placeholder.py create mode 100644 eden/scm/tests/github/mock_diverged_stack.py create mode 100644 eden/scm/tests/github/mock_diverged_stack_dirty.py create mode 100644 eden/scm/tests/github/mock_extend_stacked_prs.py create mode 100644 eden/scm/tests/github/mock_fork_stacked.py create mode 100644 eden/scm/tests/github/mock_insert_mid_stack.py create mode 100644 eden/scm/tests/github/mock_insert_mid_stack_restack.py create mode 100644 eden/scm/tests/github/mock_restack_too_small.py create mode 100644 eden/scm/tests/github/mock_stack_query_failure.py create mode 100644 eden/scm/tests/github/mock_unstack_remnant.py create mode 100644 eden/scm/tests/github/mock_unstack_remnant_sync.py create mode 100644 eden/scm/tests/test-ext-github-pr-submit-stacked-placeholder.t diff --git a/eden/scm/sapling/ext/github/__init__.py b/eden/scm/sapling/ext/github/__init__.py index fb2ca7a802722..31e994e35cd77 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): diff --git a/eden/scm/sapling/ext/github/mock_utils.py b/eden/scm/sapling/ext/github/mock_utils.py index 06e31e5452888..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 @@ -322,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, @@ -345,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) @@ -384,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 @@ -621,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/submit.py b/eden/scm/sapling/ext/github/submit.py index 89d838cccc2c0..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, ) ) @@ -166,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: @@ -223,10 +234,137 @@ 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 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 @@ -250,6 +388,11 @@ 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() # Chain to the nearest partition below whose pull request is # open (or that will get a new pull request). Closed/merged @@ -329,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] @@ -339,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, @@ -351,8 +696,14 @@ 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.uses_chained_bases() and not repository.is_fork: + 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 @@ -413,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] @@ -457,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 @@ -466,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 @@ -506,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) @@ -527,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, @@ -546,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.uses_chained_bases() 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) @@ -585,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. 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_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_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-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 index e993739e0f532..7429e0c92a5a8 100644 --- a/eden/scm/tests/test-ext-github-pr-submit-stacked.t +++ b/eden/scm/tests/test-ext-github-pr-submit-stacked.t @@ -19,11 +19,183 @@ confirm it is a 'github_repo' $ sl log -r. -T '{github_repo}\n' True -test sending pr: each PR's base should be chained to the PR below it, same as -the "single" workflow (native stack linking is tested separately) - $ sl pr submit --config extensions.pr_submit=$TESTDIR/github/mock_create_prs.py +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 From 4e46548895b25bd768103d4c5640fb5a11d6fe6b Mon Sep 17 00:00:00 2001 From: Ray Liu Date: Thu, 13 Aug 2026 12:38:52 -0400 Subject: [PATCH 6/7] [6/n][sl][github][gh stacks] wire up gh stacks to sl pull ### ctx `sl pr pull` links a diff's ancestors by parsing the sapling stack list footer, which the stacked workflow omits in #1395 ### changes made - when the body has no footer, query the native stacks API and link the ancestors below the diff from the stack instead - warns "no stack information" only if the diff is in no stack at all; a diff that is in a stack but no longer an open member of it (e.g. merged - the API only lists open members, so its position is unknown) gets a specific warning instead of the misleading one - pulling the bottom of a stack links nothing and warns nothing ### test plan - new test-ext-github-pr-pull-stacked.t with mocked stack responses: top of stack, bottom of stack, merged member - dogfooded on my own computer ``` sl config --local extensions.github=/Users/ray/sapling/eden/scm/sapling/ext/github/__init__.py sl config --local extensions.signing_shim=/Users/ray/.sl-signing-shim.py sl config --local github.pr-workflow=stacked ``` - create a stack here https://github.com/raydatray/rusty-mcrouter/pull/197 - rebase and merge as stack 197 and 198 - they merge as a stack and github doenst ask you to rebase (like it normally would in the previous pr workflow) Screenshot 2026-07-31 at 3 49 38 PM - go back to local and run sl pull - we see what those two are marked as landed Screenshot 2026-07-31 at 3 50 14 PM - rebase those two remaining diffs on main, and then resubmit, we see that the stack remains preserved on github Screenshot 2026-07-31 at 3 51 26 PM --- .../sapling/ext/github/import_pull_request.py | 75 +++++++++++++++++-- eden/scm/tests/github/mock_pull_stacked_pr.py | 49 ++++++++++++ .../github/mock_pull_stacked_pr_bottom.py | 40 ++++++++++ .../github/mock_pull_stacked_pr_merged.py | 44 +++++++++++ .../tests/test-ext-github-pr-pull-stacked.t | 47 ++++++++++++ 5 files changed, 248 insertions(+), 7 deletions(-) create mode 100644 eden/scm/tests/github/mock_pull_stacked_pr.py create mode 100644 eden/scm/tests/github/mock_pull_stacked_pr_bottom.py create mode 100644 eden/scm/tests/github/mock_pull_stacked_pr_merged.py create mode 100644 eden/scm/tests/test-ext-github-pr-pull-stacked.t 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/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/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. From 00202e912d836ce4f82cfef4444ca91b27eaa783 Mon Sep 17 00:00:00 2001 From: Ray Liu Date: Thu, 13 Aug 2026 12:38:52 -0400 Subject: [PATCH 7/7] [7/7][sl][github][gh stacks] update some documentation ### ctx documentation updates for the new native stacked workflow ### changes made - extension help text for `github.pr-workflow` (overlap/single/stacked) and `pr submit --restack` - website: github.md + sapling-stack.md updated to describe the stacked workflow and its tradeoffs ### test plan docs only --- eden/scm/sapling/ext/github/__init__.py | 19 +++++++++++++++++++ website/docs/git/github.md | 3 ++- website/docs/git/sapling-stack.md | 22 +++++++++++++++++++++- 3 files changed, 42 insertions(+), 2 deletions(-) diff --git a/eden/scm/sapling/ext/github/__init__.py b/eden/scm/sapling/ext/github/__init__.py index 31e994e35cd77..0df030b205b79 100644 --- a/eden/scm/sapling/ext/github/__init__.py +++ b/eden/scm/sapling/ext/github/__init__.py @@ -127,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/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.