diff --git a/README.md b/README.md index 3e59e5b..b7a3200 100644 --- a/README.md +++ b/README.md @@ -41,6 +41,25 @@ patch update itself is included directly in the prompt. For existing PRs the patch update is an interdiff against the previously submitted PR patch, not the whole PR patch. Explicit `ghstack -m MESSAGE` still overrides this. +## Native GitHub Stacks + +If your repository has [GitHub Stacks](https://github.github.com/gh-stack/) +enabled (currently a private preview), ghstack can link the PRs it submits +into a native stack, which gets you the stack navigation UI, stack-aware +branch protection, and partial-stack merges. Enable it with: + +``` +ghstack config github_stacks true +``` + +or per-invocation with `ghstack --direct --github-stacks`. This requires +`--direct` mode, because GitHub requires each PR in a stack to target the +head branch of the PR below it, which the classic base-branch layout does +not satisfy. ghstack keeps the native stack in sync on resubmit: new +commits on top are appended, and restructures (inserts, reorders) dissolve +and recreate the stack, since the API has no reorder operation. Stack +linking failures are never fatal; the PRs themselves are always submitted. + ## How to use Make sure you have write permission to the repo you're opening PR with. diff --git a/src/ghstack/cli.py b/src/ghstack/cli.py index e2c196a..e3e98b1 100644 --- a/src/ghstack/cli.py +++ b/src/ghstack/cli.py @@ -110,6 +110,13 @@ def cli_context( default=None, help="Create stack that directly merges into main", ) +@click.option( + "--github-stacks/--no-github-stacks", + "github_stacks_opt", + is_flag=True, + default=None, + help="Link submitted PRs into a native GitHub stack (requires --direct)", +) @click.option( "--base", "-B", @@ -141,6 +148,7 @@ def main( short: bool, force: bool, direct_opt: Optional[bool], + github_stacks_opt: Optional[bool], no_skip: bool, draft: bool, base: Optional[str], @@ -166,6 +174,7 @@ def main( base=base, stack=stack, direct_opt=direct_opt, + github_stacks_opt=github_stacks_opt, reviewer=reviewer, label=label, ) @@ -505,6 +514,14 @@ def status(pull_request: str) -> None: is_flag=True, help="Create stack that directly merges into main", ) +@click.option( + "--github-stacks/--no-github-stacks", + "github_stacks_opt", + default=None, + is_flag=True, + help="Link submitted PRs into a native GitHub stack (requires --direct and " + "a repository with GitHub Stacks enabled; overrides .ghstackrc setting)", +) @click.option( "--no-fetch", is_flag=True, @@ -523,6 +540,7 @@ def submit( no_skip: bool, draft: bool, direct_opt: Optional[bool], + github_stacks_opt: Optional[bool], base: Optional[str], revs: Tuple[str, ...], stack: bool, @@ -553,6 +571,11 @@ def submit( revs=revs, stack=stack, direct_opt=direct_opt, + github_stacks=( + github_stacks_opt + if github_stacks_opt is not None + else config.github_stacks + ), reviewer=reviewer if reviewer is not None else config.reviewer, label=label if label is not None else config.label, no_fetch=no_fetch, diff --git a/src/ghstack/config.py b/src/ghstack/config.py index 9e031f1..2ded515 100644 --- a/src/ghstack/config.py +++ b/src/ghstack/config.py @@ -113,6 +113,9 @@ def get_gh_cli_credentials( ("label", Optional[str]), # Command to generate a per-PR update description from diff contents ("automsg", Optional[str]), + # Link submitted PRs into a native GitHub stack (requires direct mode + # and a repository with GitHub Stacks enabled) + ("github_stacks", bool), ], ) @@ -334,6 +337,11 @@ def read_config( else: automsg = None + if config.has_option("ghstack", "github_stacks"): + github_stacks = config.getboolean("ghstack", "github_stacks") + else: + github_stacks = False + if write_back: with open(config_path, "w") as f: config.write(f) @@ -352,6 +360,7 @@ def read_config( reviewer=reviewer, label=label, automsg=automsg, + github_stacks=github_stacks, ) logging.debug(f"conf = {conf}") return conf diff --git a/src/ghstack/github_fake.py b/src/ghstack/github_fake.py index 4871312..9eea89d 100644 --- a/src/ghstack/github_fake.py +++ b/src/ghstack/github_fake.py @@ -71,10 +71,20 @@ ) +# A native GitHub stack (the REST resource behind `gh stack`). Stack +# numbers are allocated from the same sequence as PR numbers. +@dataclass +class PullRequestStack: + number: GitHubNumber + _repository: GraphQLId + pull_request_numbers: List[GitHubNumber] + + # The "database" for our mock instance class GitHubState: repositories: Dict[GraphQLId, "Repository"] pull_requests: Dict[GraphQLId, "PullRequest"] + stacks: Dict[GitHubNumber, "PullRequestStack"] # This is very inefficient but whatever issue_comments: Dict[GraphQLId, "IssueComment"] _next_id: int @@ -142,6 +152,7 @@ def notify_merged(self, pr_resolved: ghstack.diff.PullRequestResolved) -> None: def __init__(self, upstream_sh: Optional[ghstack.shell.Shell]) -> None: self.repositories = {} self.pull_requests = {} + self.stacks = {} self.issue_comments = {} self._next_id = 5000 self._next_pull_request_number = {} @@ -417,6 +428,14 @@ async def _update_pull_async( if "title" in input and input["title"] is not None: pr.title = input["title"] if "base" in input and input["base"] is not None: + # Real GitHub rejects any PATCH with a base field on a PR that + # is part of a native stack, even if the value is unchanged + for stack in state.stacks.values(): + if number in stack.pull_request_numbers: + raise RuntimeError( + "Cannot change the base branch because the pull " + "request is part of a stack (HTTP 422)" + ) pr.baseRefName = input["base"] pr.baseRef = await repo._make_ref_async(state, pr.baseRefName) if "body" in input and input["body"] is not None: @@ -451,6 +470,79 @@ async def _update_issue_comment_async( if (r := input.get("body")) is not None: comment.body = r + def _dump_stack(self, owner: str, name: str, stack: PullRequestStack) -> Any: + state = self.state + repo = state.repository(owner, name) + prs = [state.pull_request(repo, n) for n in stack.pull_request_numbers] + return { + "number": stack.number, + "base": {"ref": prs[0].baseRefName}, + "open": True, + "pull_requests": [ + { + "number": pr.number, + "state": "closed" if pr.closed else "open", + "head": {"ref": pr.headRefName}, + "base": {"ref": pr.baseRefName}, + } + for pr in prs + ], + } + + # Mimics the base-chaining rule the real server enforces on both + # create and add: each PR's base ref must be the previous PR's head ref + def _validate_stack_chain( + self, repo: Repository, numbers: List[GitHubNumber], prev_head: Optional[str] + ) -> None: + state = self.state + for number in numbers: + for stack in state.stacks.values(): + if number in stack.pull_request_numbers: + raise RuntimeError( + f"Pull request #{number} is already in a stack (HTTP 422)" + ) + pr = state.pull_request(repo, number) + if prev_head is not None and pr.baseRefName != prev_head: + raise RuntimeError( + "Pull requests must form a stack, where each PR's base ref " + "is the previous PR's head ref (HTTP 422)" + ) + prev_head = pr.headRefName + + async def _create_stack_async( + self, owner: str, name: str, pull_requests: List[int] + ) -> Any: + state = self.state + repo = state.repository(owner, name) + numbers = [GitHubNumber(n) for n in pull_requests] + if len(numbers) < 2: + raise RuntimeError("A stack must contain at least two PRs (HTTP 422)") + self._validate_stack_chain(repo, numbers, None) + number = state.next_pull_request_number(repo.id) + stack = PullRequestStack( + number=number, + _repository=repo.id, + pull_request_numbers=numbers, + ) + state.stacks[number] = stack + return self._dump_stack(owner, name, stack) + + async def _add_to_stack_async( + self, + owner: str, + name: str, + stack_number: GitHubNumber, + pull_requests: List[int], + ) -> Any: + state = self.state + repo = state.repository(owner, name) + stack = state.stacks[stack_number] + numbers = [GitHubNumber(n) for n in pull_requests] + top = state.pull_request(repo, stack.pull_request_numbers[-1]) + self._validate_stack_chain(repo, numbers, top.headRefName) + stack.pull_request_numbers.extend(numbers) + return self._dump_stack(owner, name, stack) + # NB: This may have a payload, but we don't # use it so I didn't bother constructing it. async def _set_default_branch_async( @@ -495,6 +587,18 @@ async def _arest_impl(self, method: str, path: str, **kwargs: Any) -> Any: "id": comment.fullDatabaseId, "body": comment.body, } + if m := re.match( + r"^repos/([^/]+)/([^/]+)/stacks\?pull_request=(\d+)$", path + ): + pr_number = int(m.group(3)) + return [ + self._dump_stack(m.group(1), m.group(2), stack) + for stack in self.state.stacks.values() + if pr_number in stack.pull_request_numbers + ] + if m := re.match(r"^repos/([^/]+)/([^/]+)/stacks/(\d+)$", path): + stack = self.state.stacks[GitHubNumber(int(m.group(3)))] + return self._dump_stack(m.group(1), m.group(2), stack) elif method == "post": if m := re.match(r"^repos/([^/]+)/([^/]+)/pulls$", path): @@ -518,6 +622,20 @@ async def _arest_impl(self, method: str, path: str, **kwargs: Any) -> Any: reviewers = kwargs.get("reviewers", []) pr.reviewers.extend(reviewers) return {} + if m := re.match(r"^repos/([^/]+)/([^/]+)/stacks$", path): + return await self._create_stack_async( + m.group(1), m.group(2), kwargs["pull_requests"] + ) + if m := re.match(r"^repos/([^/]+)/([^/]+)/stacks/(\d+)/add$", path): + return await self._add_to_stack_async( + m.group(1), + m.group(2), + GitHubNumber(int(m.group(3))), + kwargs["pull_requests"], + ) + if m := re.match(r"^repos/([^/]+)/([^/]+)/stacks/(\d+)/unstack$", path): + del self.state.stacks[GitHubNumber(int(m.group(3)))] + return None if m := re.match(r"^repos/([^/]+)/([^/]+)/issues/([^/]+)/labels", path): # Handle adding labels state = self.state diff --git a/src/ghstack/github_real.py b/src/ghstack/github_real.py index 042d38a..43f9f62 100644 --- a/src/ghstack/github_real.py +++ b/src/ghstack/github_real.py @@ -224,6 +224,8 @@ async def arest(self, method: str, path: str, **kwargs: Any) -> Any: async with getattr(session, method)(url, **request_kwargs) as resp: logging.debug("%s response status: %s", log_prefix, resp.status) + if resp.status == 204: + return None try: r = await resp.json() except (aiohttp.ContentTypeError, ValueError): diff --git a/src/ghstack/submit.py b/src/ghstack/submit.py index a649f03..56597c9 100644 --- a/src/ghstack/submit.py +++ b/src/ghstack/submit.py @@ -445,6 +445,12 @@ class Submitter: # Command to generate a per-PR update description from diff contents. automsg: Optional[str] = None + # Link submitted PRs into a native GitHub stack. Only has an effect + # with --direct, because GitHub requires each PR's base ref to be the + # previous PR's head ref, which non-direct ghstack branches don't + # satisfy. Requires the repository to have GitHub Stacks enabled. + github_stacks: bool = False + # ~~~~~~~~~~~~~~~~~~~~~~~~ # Computed in post init @@ -2061,7 +2067,10 @@ def _update_pr_args(s: DiffMeta) -> Tuple[str, Dict[str, Any], Optional[str]]: ) base_kwargs = {} if self.direct: - base_kwargs["base"] = s.base + # Don't send no-op base updates: GitHub rejects any PATCH + # containing the base field on a PR in a native stack + if s.base != s.elab_diff.base_ref: + base_kwargs["base"] = s.base else: assert s.base == s.elab_diff.base_ref stack_desc = self._format_stack( @@ -2100,8 +2109,28 @@ async def _update_pr_async(s: DiffMeta) -> None: ), ) + # GitHub rejects base changes on PRs in a native stack, so any PR + # we are about to retarget must be unstacked first; + # _sync_github_stack will recreate the stack afterwards + if self.github_stacks and self.direct: + retarget_numbers = [ + s.number for s in diffs_to_submit if s.base != s.elab_diff.base_ref + ] + if retarget_numbers: + await self._unstack_prs(retarget_numbers) + await _gather_ordered(_update_pr_async(s) for s in reversed(diffs_to_submit)) + if self.github_stacks and diffs_to_submit: + if self.direct: + await self._sync_github_stack(all_diffs or diffs_to_submit) + else: + logging.warning( + "Not linking a GitHub stack: github_stacks requires --direct, " + "because GitHub requires each PR to target the head branch of " + "the PR below it" + ) + # Report what happened def format_url(s: DiffMeta) -> str: return "https://{github_url}/{owner}/{repo}/pull/{number}".format( @@ -2163,6 +2192,95 @@ def format_url(s: DiffMeta) -> str: "I did NOT close or update PRs previously associated with these commits." ) + # Dissolve any native GitHub stacks containing the given PRs. Errors + # are non-fatal here: if a PR is genuinely stacked and unstacking + # failed, the subsequent base-changing PATCH will fail loudly anyway. + async def _unstack_prs(self, numbers: List[GitHubNumber]) -> None: + prefix = f"repos/{self.repo_owner}/{self.repo_name}/stacks" + try: + memberships = await _gather_ordered( + self.github.aget(f"{prefix}?pull_request={n}") for n in numbers + ) + for stack_number in {s["number"] for r in memberships for s in r}: + await self.github.apost(f"{prefix}/{stack_number}/unstack") + logging.info( + "Dissolved GitHub stack #%s to allow retargeting; it will " + "be recreated after the update", + stack_number, + ) + except Exception: + logging.warning("Failed to unstack PRs", exc_info=True) + + # Sync the current chain of PRs to a native GitHub stack (the + # first-class REST resource behind `gh stack`). The server requires + # each PR's base ref to be the head ref of the PR below it, which is + # exactly what --direct submits produce. Stack linking failures are + # never fatal: the PRs themselves are already submitted correctly. + async def _sync_github_stack(self, all_diffs: List[DiffMeta]) -> None: + # all_diffs is in topo order (top of stack first); the stacks API + # wants bottom-to-top + chain = list(reversed(all_diffs)) + expected_base = self.base + for s in chain: + if s.base != expected_base: + logging.warning( + "Not linking a GitHub stack: PR #%s targets %s but the PR " + "below it ends at %s. This usually means the stack is " + "mid-restructure; resubmit the whole stack and try again.", + s.number, + s.base, + expected_base, + ) + return + expected_base = branch_head(s.username, s.ghnum) + numbers = [s.number for s in chain] + prefix = f"repos/{self.repo_owner}/{self.repo_name}/stacks" + try: + memberships = await _gather_ordered( + self.github.aget(f"{prefix}?pull_request={n}") for n in numbers + ) + existing = {s["number"]: s for r in memberships for s in r} + if len(existing) == 1: + (stack,) = existing.values() + open_numbers = [ + pr["number"] + for pr in stack["pull_requests"] + if pr["state"] == "open" + ] + if open_numbers[: len(numbers)] == numbers: + return + if numbers[: len(open_numbers)] == open_numbers: + added = numbers[len(open_numbers) :] + await self.github.apost( + f"{prefix}/{stack['number']}/add", pull_requests=added + ) + logging.info( + "Added %s to GitHub stack #%s", + ", ".join(f"#{n}" for n in added), + stack["number"], + ) + return + # The server has no reorder/remove endpoint, so any other + # restructure is unstack + recreate + for stack_number in existing: + await self.github.apost(f"{prefix}/{stack_number}/unstack") + if len(numbers) < 2: + return + r = await self.github.apost(prefix, pull_requests=numbers) + logging.info( + "Linked GitHub stack #%s: %s", + r["number"], + " <- ".join(f"#{n}" for n in numbers), + ) + except ghstack.github.NotFoundError: + logging.warning( + "Not linking a GitHub stack: the stacks API returned 404, which " + "usually means GitHub Stacks is not enabled on this repository " + "(it is in private preview; see https://github.github.com/gh-stack/)" + ) + except Exception: + logging.warning("Failed to link GitHub stack", exc_info=True) + async def check_invariants_for_diff( self, # the user diff is what the user actual sent us diff --git a/src/ghstack/test_prelude.py b/src/ghstack/test_prelude.py index 69ea201..20f7bc6 100644 --- a/src/ghstack/test_prelude.py +++ b/src/ghstack/test_prelude.py @@ -69,6 +69,7 @@ "get_sh", "get_upstream_sh", "get_github", + "get_github_stacks", "get_pr_reviewers", "get_pr_labels", "tick", @@ -224,6 +225,7 @@ async def gh_submit( reviewer: Optional[str] = None, label: Optional[str] = None, automsg: Optional[str] = None, + github_stacks: bool = False, ) -> List[ghstack.submit.DiffMeta]: self = CTX r = await ghstack.submit.main( @@ -247,6 +249,7 @@ async def gh_submit( reviewer=reviewer, label=label, automsg=automsg, + github_stacks=github_stacks, ) await self.check_global_github_invariants(self.direct) return r @@ -469,6 +472,11 @@ def get_pr_reviewers(pr_number: int) -> List[str]: return pr.reviewers +def get_github_stacks() -> List[List[int]]: + github = get_github() + return [list(stack.pull_request_numbers) for stack in github.state.stacks.values()] + + def get_pr_labels(pr_number: int) -> List[str]: """Get the labels for a PR number.""" github = get_github() diff --git a/test/submit/github_stacks.py.test b/test/submit/github_stacks.py.test new file mode 100644 index 0000000..1abd397 --- /dev/null +++ b/test/submit/github_stacks.py.test @@ -0,0 +1,57 @@ +from ghstack.test_prelude import * + +await init_test() + +await commit("A") +await commit("B") +A, B = await gh_submit("Initial", github_stacks=True) + +# Native GitHub stacks require direct mode (each PR must target the head +# branch of the PR below it) +if is_direct(): + assert_eq(get_github_stacks(), [[500, 501]]) +else: + assert_eq(get_github_stacks(), []) + +# Resubmitting an unchanged stack is idempotent +await amend("B") +await gh_submit("Update B", github_stacks=True) + +if is_direct(): + assert_eq(get_github_stacks(), [[500, 501]]) +else: + assert_eq(get_github_stacks(), []) + +# A new commit on top is appended to the existing stack +# (in direct mode, PR number 502 was consumed by the stack itself) +await commit("C") +await gh_submit("Add C", github_stacks=True) + +if is_direct(): + assert_eq(get_github_stacks(), [[500, 501, 503]]) +else: + assert_eq(get_github_stacks(), []) + +# Submitting only a prefix of the stack leaves the existing stack alone +await gh_submit("Update A only", github_stacks=True, revs=["HEAD~~"]) + +if is_direct(): + assert_eq(get_github_stacks(), [[500, 501, 503]]) +else: + assert_eq(get_github_stacks(), []) + +# Inserting a commit mid-stack retargets the PR above it, which requires +# dissolving and recreating the native stack +top = GitCommitHash(await git("rev-parse", "HEAD")) +await checkout(GitCommitHash(await git("rev-parse", "HEAD~"))) +await commit("M") +await cherry_pick(top) +await gh_submit("Insert M", github_stacks=True) + +if is_direct(): + # 504 is M's PR (505 was consumed by the recreated stack) + assert_eq(get_github_stacks(), [[500, 501, 504, 503]]) +else: + assert_eq(get_github_stacks(), []) + +ok()