upload: Push a dummy commit to work around reordering issues

We've had a long running issue where github may mark prs as merged
or closed when trying to reorder relative series. This is caused
by push + update not being atomic (see comments for more details).

To work around this we'll

- detect it in a dumb way, if 2 or more prs had their base changed
this upload. this will result in some false positives, but the only
cost is an additional ~1s push that people likely won't notice, and
will save us from needing to write more complex logic to detect it
correctly.
- push a dummy commit to every single branch initially that prevents
github from marking any of the prs as merged.
- update as normal
- do an extra push to remove the dummy commit

Topic: reorder4
Reviewers: aaron, brian-k
Fixes: #170

Author: jerry-skydio

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.
@@ -699,6 +703,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.
@@ -744,6 +749,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
@@ -1003,7 +1021,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 that 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(

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, create_local_branches=False)
     finally:
         topics.print(not args.verbose)
     return 0