• she/her

Principal engineer at Mercury. I've authored the Dhall configuration language, the Haskell for all blog, and countless packages and keynote presentations.

I'm a midwife to the hidden beauty in everything.

💖 @wiredaemon


discord
Gabriella439
discord server
discord.gg/XS5ZDZ8nnp
location
bay area
private page
cohost.org/newmoon

fullmoon
@fullmoon

One of the things I've been working on recently is fixing bors-ng to mark pull requests "merged" instead of "closed" when squash merging a batch of them. The conventional wisdom is that this is Not Possible™️ for third-party merge queues using GitHub's API but I think it actually is possible.

To provide some context, GitHub does natively provide its own merge queue feature which is currently in beta, but we trialed this at work a few months ago and ran into this issue so that's why I'm pushing pretty hard on getting bors-ng working in the interim until that is fixed.

Plus I kinda just want to see if I can fix this longstanding UX wart in bors-ng once and for all since proving that it's possible will improve the design of other third-party merge automation solutions (like Mergify).


fullmoon
@fullmoon

So there are basically two approaches to getting this to work: one that's simpler with a worse UX and one that's more complicated with a better UX. To explain things, though, I need to provide some background first.


Background

GitHub has some support for automatically marking PRs as merged if you merge and push the branch via the command line. The way this works is that if GitHub detects that all of the PR's commits are present in the base branch then it automatically marks the PR as merged.

HOWEVER, this does not work with squash merges because squash merges generate a new commit that is distinct from the commits present in the original PR. So if you squash merge and push the branch from the command line then GitHub has no way of knowing that the commit you just pushed to the base branch corresponds to the original PR.

This is the reason why a lot of merge automation tools (e.g. bors-ng) don't properly mark a PR as merged if you configure them to use squash merges because what they will do is test a branch containing a batch of squashed merge PRs and then when tests pass they'll just force push the branch they tested to the base branch.

Instead, what all of the merge automation solutions do is that they'll close the PR instead of marking it merged (since GitHub does not provide a way to mark a PR as merged if GitHub doesn't believe it was truly merged), which messes with a lot of integrations. For example, any issues linked to the PR won't be properly marked solved because the issue tracker will think that the PR wasn't merged.

Simple solution with poor UX

HOWEVER × 2, there actually is one way you can get GitHub to auto-close PRs when you squash merge them. This approach is fairly simple to implement.

Suppose that the tip of your base branch is a commit named start and you have N PRs that you want to test in a batch against that base branch. The first thing that all merge automation solutions will do is create N commits representing the N PRs squash merged onto the start commit (which we'll denote squash₀ through squashₙ), like this:

start → squash₀ → squash₁ → … → squashₙ

Then they'll run CI on the tip of that staging branch (e.g. commit squashₙ) and check if tests pass. If tests do pass, then here is the simple solution to merge those N squashed commits into the base branch in such a way that GitHub thinks the PRs were actually merged:

  • for each squashed commit (i.e. squash₀ through squashₙ) in order:
    • push the parent commit of the squashed commit to the base branch
    • force push the squashed commit to the original PR it was derived from
      • NOTE: this will clobber the original PR's commit history!
  • push squashₙ to the base branch

If you do this, then GitHub will automatically mark each PR as merged because it will recognize that the (new, overridden) commit history of each PR is present in the base branch.

The downside of this approach is that you lose the original commits on the PR, which is why this approach has a worse UX even though it is very simple to implement.

More complex solution with a better UX

The other way to get GitHub to recognize that a squash-merged PR is actually merged is to use GitHub to squash merge the PR (instead of using the command line). This is effectively the same thing as if you had clicked the green merge button in the GitHub UI to squash merge the PR. Since you go through the GitHub API then GitHub can recognize that the PR should be marked merged since GitHub was the one that merged it. This also preserves the original PR's commit history.

So the solution with a better UX is:

  • before testing the batch:
    • remember the original commit that each squash-merged commit was derived from
      • i.e. record the HEAD commit for each PR
  • after testing the batch:
    • for each PR that was tested as part of that batch:
      • if the tip of the PR no longer matches the original commit:
        • force push the original commit to reset the PR back to where it was
          • this ensures that you don't merge untested code into the base branch
      • use GitHub's API to squash merge the PR into the base branch

The only limitation of this approach is: if the author of the PR pushes new commits after their PR was queued for testing then those new commits will be lost if the PR is accepted and merged. (And, honestly, that's kind of their fault for adding new code to the branch after they already submitted it for testing as part of a larger batch of PRs, so the punishment fits the crime)

Conclusion

Anyway, I managed to implement the latter approach for bors-ng and I'm going to open source it soon. To be honest, it won't provide that much value to open source it (because once GitHub finally irons out the bugs in their native merge queue then it's useless code), but I'll still open source it anyway.


You must log in to comment.

in reply to @fullmoon's post:

wouldn't it be better UX if a push to an approved PR cancels the approve, instead of overriding commits?

it still should be rare, so this probably does not matter, but cancelling would be far less surprising imo