Block Github merge on changes requested?

I had a PR that had “changes requested”, but Github still allowed me to merge:

I get it that this is the same as used to be with Phab, but maybe we can start being a bit tighter and use the tool’s functionality fully?

We discussed protecting the main branch for tests passing, and as soon as we deem them stable enough, which I’m in strong favour. But I think we also ought to do the same (eventually) for approval and change requests.

But for now, if we allow people (with write access) to bypass the rules, we would still retain the same behaviour, but Github shows an unmistakable warning, avoiding basic mistakes:

Some reasonable interim compromise?

As long as it’s bypassable (which from your screenshot it seems it is), then having the extra barrier to avoid mistakenly merging while changes are still requested seems like a great idea to me.


As long as there is a bypass that seems fine to me (I’ve seen reviewer disappearing in the past after leaving some trivial comment marking a revision as “requiring changes”).



Yes, it is bypassable and indeed, it’s mostly to avoid pressing the big green button by accident (or prematurely).

I have merged the wrong PR before when reviewing multiple tabs and just pressing the wrong green button. With LLVM’s protection of no deletion, this creates a lot of noise.

Once we moved our projects to require approval and CI passing, I now only “force merge” when I’m pretty sure what I’m doing (like broken CI or the reviewer accepted offline and went on holiday).

The only etiquette I’d ask is to have a clear communication that this is happening, like pings and waits and other approvers acknowledging the bypass, but that’s no different than what we did in Phab, so not changing behaviour at all, just making it easier to follow the status quo.

1 Like

Could you please spell out precisely what ruleset you are proposing to add? Is it this?

  • Bypass list: Write
  • Require a pull request before merging
    • Required approvals: 1

I’m proposing:

  • Bypass list: Write
  • Require a pull request before merging
    • Required approvals: 1
  • Require status checks to pass before merging
    • Use the existing builders as list
  • Require conversation resolution before merging

The last point wasn’t explicit in the OP, but we use this in our project and is in line with LLVM’s review process.

Again, all of those can be bypassed by committers and would not change our behaviour other than make it a “conscious” step.

DId we actually reach a consensus on the last point? Would be nice to update this thread then.

I don’t think we have consenus on how to use the “conversation resolution” button, as @rovka raised. It also doesn’t work if the author can resolve all conversations themselves (I don’t know if there’s a setting to prevent this), since they could just mark the conversation as resolved and submit (I’ve already seen multiple occasions where people have marked a conversation as resolved, even though it wasn’t in my view).

Yeah, this is why I didn’t add originally. I don’t mind about this one, tbh. Happy to skip it.

Would this make it impossible to manually push commits to the repo?

No, direct push is a form of bypassing. If the writers have permission to bypass the rules, they can also push directly.

The only difference is that you get a “warning” on every manual push saying you’re bypassing the rules, which is fair, since you’re not getting a review not validating on the CI, so…

So what happens when someone in the bypass list creates a pull request, does it look any different than if we had no rules whatsoever?

I just tested this out and when you in the bypass list it makes you check an extra box in order to bypass the rule.

I also tested direct pushing and it works with the restrictions enabled and it still works (it just gives a warning like @rengolin said).

Overall, this looks like good news. We should be able to add restrictions on pull requests without impacting the ability to push directly.


The only difference in the PR is the red checkbox to bypass, which I think it’s a positive change.

In other projects, when I see a green merge button I usually assume CI is passing and someone approved it. So I’m always confused in LLVM. :slight_smile:

1 Like

I would be in favor of adding these checks right now. I think ‘Require conversation resolution before merging’ and the status checks need more discussion.

@tstellar would this also enables to turn on the “auto-merge” feature? That would be fantastic :slight_smile:

What is the downside of these? They are just printing a warning and requiring an extra click from the people who do the merging right?
One extra click when merging seems like extremely low extra burden to me.

Based on the documentation, its seems so.

For the conversation resolution, do we have agreement yet on who should be resolving these? I just think we should have some kind of official policy before we make it a required check. I don’t want clicking the bypass box to become routine if people are unsure what to do about the unresolved conversations.

For the status checks, do we have an agreed upon check that we want to be blocking?

I don’t quite see it as making anything “blocking” here, just raising awereness to avoid unintended mistakes :slight_smile:

Because I don’t see how the one you selected are different:

We don’t request a pull request and we don’t require approval on PR either, any more than any of the check we run on the PR right?

Right. I think the key here is to differentiate between mistakes and intentions. Github sees it as “requirement”, but with the ability to bypass, we see it as “an extra sanity check”. To be clear, my proposal is to avoid mistakes, not to change the culture of reviews.

“Requiring” PRs, approvals, CI passing, with the explicit configuration to bypass those rules for all writers, creates an extra step to merge something that may not be ready, so you only do it when you “know what you’re doing”.

Particularly on the “require PR” option, this is only “needed” here because the “require approvals” is nested inside it. I’d be fine with just requiring approvals from the PRs that are actually submitted, so we wouldn’t get warnings on direct pushes, but the “cost” of this is even less for direct pushes: there are no extra steps, just a warning after the push has finished.

The two main “issues” I can foresee in enabling these options are:

  • People get annoyed of the extra click on the PR. This would only “pile up” if too many people did that, and that’d mean our reviews and CI checks are being ignored, which to me, is a bad sign and we want to know if this is the case.
  • Users start to rely on approvals and CI to be green and get upset if someone else merges before approval and they have no chance to review. This is the main reason why we have “require approvals”, but it’s also not bullet proof. I think here is mostly about how the community moulds its behaviour around the rules.

I think enabling these options is so low cost and so easy to revert that it’s definitely worth a shot. At the very least it will give us a feeling for how the community has adapted to Github and help us make the experience better.

1 Like