From 428943c2197864a5d2ac9e13caa1eb2f05d13112 Mon Sep 17 00:00:00 2001 From: Shoaib Meenai Date: Wed, 2 Sep 2026 10:20:23 -0700 Subject: [PATCH] Park only the pull requests a reorder actually endangers Parking every pull request whose base ref changed also catches ones that were never at risk, such as when a commit is inserted below them: their base moves onto a different pull request, but nothing becomes reachable from the base they had. Each of those costs an API call and two base-change entries in the pull request's timeline. Decide by reachability instead, against the commit each old base is about to point at, which is known locally before anything is pushed. github_fake counts base changes so a test can pin this down. --- src/ghstack/github_fake.py | 3 + src/ghstack/submit.py | 61 +++++++++++++------- test/submit/insert_middle_base_churn.py.test | 27 +++++++++ 3 files changed, 70 insertions(+), 21 deletions(-) create mode 100644 test/submit/insert_middle_base_churn.py.test diff --git a/src/ghstack/github_fake.py b/src/ghstack/github_fake.py index 2427336..1ad01d3 100644 --- a/src/ghstack/github_fake.py +++ b/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 diff --git a/src/ghstack/submit.py b/src/ghstack/submit.py index be45b58..6c52ff0 100644 --- a/src/ghstack/submit.py +++ b/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) diff --git a/test/submit/insert_middle_base_churn.py.test b/test/submit/insert_middle_base_churn.py.test new file mode 100644 index 0000000..0f12969 --- /dev/null +++ b/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()