Park only the pull requests a reorder actually endangers

src/ghstack/github_fake.py

@@ -296,6 +296,7 @@ class PullRequest(Node):
     title: str
     url: str
     merged: bool = False
+    base_changes: int = 0
     reviewers: List[str] = dataclasses.field(default_factory=list)
     labels: List[str] = dataclasses.field(default_factory=list)
 
@@ -439,6 +440,8 @@ 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:
+            if input["base"] != pr.baseRefName:
+                pr.base_changes += 1
             pr.baseRefName = input["base"]
             pr.baseRef = await repo._make_ref_async(state, pr.baseRefName)
             state._refs_dirty = True

src/ghstack/submit.py

@@ -2005,26 +2005,8 @@ 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] = []
+        new_tips: Dict[str, GitCommitHash] = {}
 
         for s in reversed(diffs_to_submit):
             for diff, b in s.push_branches:
@@ -2035,9 +2017,46 @@ async def push_updates(
                     force = False
                 else:
                     force = self.force
-                all_push_specs.append(
-                    push_spec(diff, branch(s.username, s.ghnum, b), force=force)
+                branch_name = branch(s.username, s.ghnum, b)
+                new_tips[branch_name] = diff
+                all_push_specs.append(push_spec(diff, branch_name, force=force))
+
+        # In direct mode a pull request's base is another pull request's head
+        # branch, so moving a commit earlier in the stack can leave its head
+        # reachable from the base GitHub still has on file, and GitHub closes
+        # any pull request in that state as merged.  Park those on the default
+        # branch, which no head branch is ever reachable from, until their real
+        # base has been pushed.  Only reachability matters: a pull request
+        # whose base merely got renumbered, as when a commit is inserted below
+        # it, is not at risk and retargeting it twice would be pure noise.
+        parked = []
+        if self.direct:
+            for s in diffs_to_submit:
+                if s.closed or s.base == s.elab_diff.base_ref:
+                    continue
+                old_base = s.elab_diff.base_ref
+                old_base_tip = new_tips.get(
+                    old_base, GitCommitHash(f"{self.remote_name}/{old_base}")
                 )
+                if await self.sh.agit(
+                    "merge-base",
+                    "--is-ancestor",
+                    s.head,
+                    old_base_tip,
+                    exitcode=True,
+                ):
+                    parked.append(s)
+        await _gather_ordered(
+            self.github.arest(
+                "patch",
+                "repos/{}/{}/pulls/{}".format(
+                    self.repo_owner, self.repo_name, s.number
+                ),
+                base=self.base,
+            )
+            for s in parked
+        )
+
         if all_push_specs:
             await self._git_push(all_push_specs)
 

test/submit/insert_middle_base_churn.py.test

@@ -0,0 +1,27 @@
+from ghstack.test_prelude import *
+
+await init_test()
+
+await commit("A")
+await commit("B")
+await commit("C")
+A, B, C = await gh_submit("Initial")
+
+# Insert a commit below B.  B's base moves onto the new pull request, but
+# nothing became reachable from B's old base, so B should be retargeted once
+# and not parked on main first.
+await checkout(A)
+await commit("X")
+await cherry_pick(B)
+await cherry_pick(C)
+await gh_submit("Insert")
+
+github = get_github()
+repo = github.state.repository("pytorch", "pytorch")
+changes = {
+    n: github.state.pull_request(repo, ghstack.github_fake.GitHubNumber(n)).base_changes
+    for n in (500, 501, 502)
+}
+assert_eq([n for n, c in changes.items() if c > 1], [])
+
+ok()