From a20167292627e1799b79031bf3314e65802d5417 Mon Sep 17 00:00:00 2001 From: Ray Liu Date: Fri, 31 Jul 2026 13:13:38 -0400 Subject: [PATCH 1/4] [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/4] [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/4] [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/4] [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()