diff --git a/revup/topic_stack.py b/revup/topic_stack.py index 335d540..cf15e37 100644 --- a/revup/topic_stack.py +++ b/revup/topic_stack.py @@ -18,6 +18,7 @@ from revup.types import ( GitCommitHash, GitConflictException, + GitTreeHash, RevupConflictException, RevupUsageException, ) @@ -256,6 +257,9 @@ class TopicStack: # Whether populate() was successfully called populated: bool = False + # Whether to work around github issues with reordering by pushing a dummy commit + use_reordering_workaround = False + def all_reviews_iter(self) -> Iterator[Tuple[str, Topic, str, Review]]: """ One liner for common iteration pattern to reduce indentation a bit. @@ -709,6 +713,7 @@ async def mark_rebases(self, skip_rebase: bool) -> None: changes that are already merged, or where push can be skipped due to being rebases or being identical. """ + num_reordered_changes = 0 for _, topic, base_branch, review in self.all_reviews_iter(): # If the relative branch already merged, reset the remote base directly to the base # branch. @@ -754,6 +759,19 @@ async def mark_rebases(self, skip_rebase: bool) -> None: # recreate the pr (but warn anyway). review.status = PrStatus.NEW + if review.pr_info is not None and review.remote_base != review.pr_info.baseRef: + logging.debug( + f"Retargeting pr {review.remote_head} from {review.pr_info.baseRef}" + f" to {review.remote_base}" + ) + num_reordered_changes += 1 + if num_reordered_changes > 1: + # The logic for correctly detecting whether changes have been reordered can be + # complex. To simplify, we'll apply the workaround if at least 2 PRs had a base + # change. This is relatively rare, and the only cost is another 1s spent on an + # extra git push operation. + self.use_reordering_workaround = True + if review.pr_info is None: # This is a new pr, no need to check patch ids review.is_pure_rebase = False @@ -1013,7 +1031,23 @@ async def push_git_refs(self, uploader: str, create_local_branches: bool) -> Non if review.push_status != PushStatus.PUSHED or review.status == PrStatus.MERGED: continue - push_targets.append(f"{review.new_commits[-1]}:refs/heads/{review.remote_head}") + commit_to_push = review.new_commits[-1] + if self.use_reordering_workaround: + # When reordering a relative series of PRs, github isn't able to handle the push, + # which happens via git, atomically with the api update, which happens through + # http. As a result github always sees the push happen first with the old relative + # structure. When reordering, a PR that is being moved forward might briefly look + # like it contains no new commits, causing github to either mark it merged or + # closed. To prevent this, we add an empty dummy commit onto the branch before + # pushing, do the update, then push once more to remove the dummy commit. This + # only happens when we detect the it is needed, so does not add much overhead. + dummy_commit = git.CommitHeader( + GitTreeHash(f"{commit_to_push}^{{tree}}"), [commit_to_push] + ) + dummy_commit.commit_msg = "Revup dummy commit to work around reordering issues" + commit_to_push = await self.git_ctx.commit_tree(dummy_commit) + + push_targets.append(f"{commit_to_push}:refs/heads/{review.remote_head}") if create_local_branches: await self.git_ctx.git( diff --git a/revup/upload.py b/revup/upload.py index 39e8217..a8a0571 100644 --- a/revup/upload.py +++ b/revup/upload.py @@ -110,6 +110,11 @@ async def main( # Review graph requires populated PR urls from creation topics.populate_review_graph() await topics.update_prs() + + if topics.use_reordering_workaround: + topics.use_reordering_workaround = False + with get_console().status("Pushing again to work around reordering issues…"): + await topics.push_git_refs(git_ctx.author, False) finally: topics.print(not args.verbose) return 0