Don't let reordering a stack mark pull requests as merged

In direct mode a pull request's base is another pull request's head
branch.  Reordering a stack makes the branch a pull request used to be
based on merge that pull request's head in, and every branch is pushed
before any base is retargeted, so GitHub sees a head reachable from the
base it still has on file and closes that pull request as merged.  Merged
pull requests cannot be reopened.

Retargeting before pushing does not fix it: in a swap, the pull request
moving the other way is already reachable from the base it is about to be
given.  Park every pull request whose base is moving on the default
branch first instead, which no head branch is reachable from.  Costs one
extra API call and two base-change timeline entries per moved pull
request.

github_fake now closes a pull request as merged when its head becomes
reachable from its base, which is what lets reorder.py.test catch this.

Author: smeenai

src/ghstack/github_fake.py

@@ -125,13 +125,32 @@ def next_issue_comment_full_database_id(self, repo_id: GraphQLId) -> int:
         return r
 
     def push_hook(self, refs: Sequence[str]) -> None:
-        # updated_refs = set(refs)
-        # for pr in self.pull_requests:
-        #    # TODO: this assumes only origin repository
-        #    # if pr.headRefName in updated_refs:
-        #    #    pr.headRef =
-        #    pass
-        pass
+        self._refs_dirty = True
+
+    async def detect_merged(self) -> None:
+        """
+        GitHub closes a pull request as merged once its head becomes
+        reachable from its base, which in direct mode can happen through an
+        ordinary push to some other pull request's head branch.  Only scan
+        after something moved; a scan is a git call per open pull request.
+        """
+        if not self._refs_dirty or self.upstream_sh is None:
+            self._refs_dirty = False
+            return
+        self._refs_dirty = False
+        for pr in self.pull_requests.values():
+            if pr.closed:
+                continue
+            reachable = await self.upstream_sh.agit(
+                "merge-base",
+                "--is-ancestor",
+                pr.headRefName,
+                pr.baseRefName,
+                exitcode=True,
+            )
+            if reachable:
+                pr.closed = True
+                pr.merged = True
 
     def notify_merged(self, pr_resolved: ghstack.diff.PullRequestResolved) -> None:
         repo = self.repository(pr_resolved.owner, pr_resolved.repo)
@@ -140,6 +159,7 @@ def notify_merged(self, pr_resolved: ghstack.diff.PullRequestResolved) -> None:
         # TODO: model merged too
 
     def __init__(self, upstream_sh: Optional[ghstack.shell.Shell]) -> None:
+        self._refs_dirty = False
         self.repositories = {}
         self.pull_requests = {}
         self.issue_comments = {}
@@ -275,6 +295,7 @@ class PullRequest(Node):
     # state: PullRequestState
     title: str
     url: str
+    merged: bool = False
     reviewers: List[str] = dataclasses.field(default_factory=list)
     labels: List[str] = dataclasses.field(default_factory=list)
 
@@ -342,6 +363,7 @@ def __init__(self, upstream_sh: Optional[ghstack.shell.Shell] = None) -> None:
         self.state = GitHubState(upstream_sh)
 
     async def graphql(self, query: str, **kwargs: Any) -> Any:
+        await self.state.detect_merged()
         r = await graphql.graphql(
             schema=GITHUB_SCHEMA,
             source=query,
@@ -419,6 +441,7 @@ async def _update_pull_async(
         if "base" in input and input["base"] is not None:
             pr.baseRefName = input["base"]
             pr.baseRef = await repo._make_ref_async(state, pr.baseRefName)
+            state._refs_dirty = True
         if "body" in input and input["body"] is not None:
             pr.body = input["body"]
 
@@ -463,6 +486,7 @@ async def _set_default_branch_async(
         )
 
     async def arest(self, method: str, path: str, **kwargs: Any) -> Any:
+        await self.state.detect_merged()
         return await self._arest_impl(method, path, **kwargs)
 
     async def _arest_impl(self, method: str, path: str, **kwargs: Any) -> Any:

src/ghstack/submit.py

@@ -2005,6 +2005,25 @@ async def push_updates(
         # otherwise GitHub can spuriously think that the user pushed a number
         # of patches as part of the PR, when actually they were just from the
         # new upstream branch.
+        # In direct mode a pull request's base is another pull request's head
+        # branch, so a reorder can leave a pull request's head reachable from
+        # the base GitHub still has on file, and GitHub closes any pull request
+        # in that state as merged.  Park the ones whose base is moving on the
+        # default branch, which no head branch is ever reachable from, until
+        # their real base has been pushed.
+        if self.direct:
+            await _gather_ordered(
+                self.github.arest(
+                    "patch",
+                    "repos/{}/{}/pulls/{}".format(
+                        self.repo_owner, self.repo_name, s.number
+                    ),
+                    base=self.base,
+                )
+                for s in diffs_to_submit
+                if not s.closed and s.base != s.elab_diff.base_ref
+            )
+
         all_push_specs: List[str] = []
 
         for s in reversed(diffs_to_submit):

test/submit/reorder_repeatedly.py.test

@@ -0,0 +1,28 @@
+from ghstack.test_prelude import *
+
+await init_test()
+
+for n in ["1", "2", "3", "4"]:
+    await commit(n)
+await gh_submit("Initial")
+
+for rnd in range(3):
+    # swap the 2nd and 3rd commits
+    c1, c2, c3, c4 = (await git("rev-list", "--reverse", "origin/main..HEAD")).split()
+    await git("reset", "--hard", c1)
+    await git("cherry-pick", c3, c2, c4)
+    await gh_submit(f"Reorder {rnd}")
+
+r = await get_github().graphql(
+    """
+    query {
+      repository(name: "pytorch", owner: "pytorch") {
+        pullRequests { nodes { number closed } }
+      }
+    }
+    """
+)
+nodes = r["data"]["repository"]["pullRequests"]["nodes"]
+assert_eq([p["number"] for p in nodes if p["closed"]], [])
+
+ok()