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()