Block pull request merge if changes are requested

[davidsvantesson]

• Gitea version (or commit ref): 1.11.0+dev-311-g675f27523

Description

There should be a branch protection setting to Block merge if changes are requested by a reviewer. A reviewer here means someone that would count to the required approvals (with current implementation that is the same as users in the whitelist).

For discussion: I would actually think blocking should be the default behavior and instead you can change the setting to Allow merging even if there are changes requested.

[guillep2k]

I think that such setting would be definitively useful, and I'd make that part of the branch protection settings (i.e. not at app.ini or repo levels).

If it should be the default, however, is debatable. The thing with Gitea is that it serves two very different scenarios: public sites with open source projects like Gitea itself, and private sites with maybe enterprise projects.

For open source projects it's problematic to tie a person to anything; people come and go, have new priorities, etc. Reviewers in this case may even be passersby that simply leave an opinion.

For the enterprise environment it's unlikely that issues will be hanging forever because of a person not being available, and reviewers will probably be people with authority that should not be bypassed, so it makes sense to have a "do not ignore" default.

It's an easy setting to check/uncheck when protecting a branch, anyway. We should be sure to make it available in the API so users can change it in bulk if they need to.

[davidsvantesson]

Yes, definitely a branch protection setting.

I think Github will always block merge if there are changes requested (i.e. not even possible to disable by setting)? That is why I though it should maybe be the default setting. But it might be simpler to let the default (checkbox unticked) be to not block merging.

In any case I think there should be a way to "force merge" for certain users (maybe administrators or specific whitelist), to be able to ignore e.g. a reviewer. But I think that is out of scope for this issue.

[zeripath]

Yeah I find the Gitea behaviour a little surprising - request changes from appropriate team members should at least be addressed even if it just means dismissed not totally ignored.

[guillep2k]

In any case I think there should be a way to "force merge" for certain users (maybe administrators or specific whitelist), to be able to ignore e.g. a reviewer. But I think that is out of scope for this issue.

Admins/Owners should be able to override this without a whitelist because they can simply go and change the setting if they like, so it's useless to block them. We can add a warning ("There are pending reviews!") if we like. I think that should be good enough for starters.

[davidsvantesson]

@guillep2k For merging in the GUI we can add clear warnings that it overrides the branch protections rules, so it makes sense admins can override there.

When it comes to pushing and manual merging I don't think it is a good idea to allow admins to override. For me branch protection is both about authorization and preventing mistakes. I can trust someone in administrating the repository, but people can easily do mistakes and push or even force push to the wrong branch. GitHub has a special setting to Include administrators. I think a similar setting is needed if we should allow administrators to push to the branch (but then you can just add them in the existing settings). If we only consider the force/override merge of pull request in the UI, then I think it can be acceptable without a whitelist.

[guillep2k]

@guillep2k For merging in the GUI we can add clear warnings that it overrides the branch protections rules, so it makes sense admins can override there.

Yes, I meant in the UI.