Push to PR cannot always reset reviews

[aspiers]

If a pull request branch is updated via a push (force or otherwise), it should invalidate any approvals submitted via the new review system, because the update could introduce new changes which the previous approvers might not approve. Currently this invalidation does not happen, which means that there is a high risk of these new changes getting merged even though they were never reviewed. This significantly weakens the reliability of the review system. Gerrit is an example of a code review system which handles this correctly: only the reviews on the latest patch set are taken into account.

As a result of this approach, Gerrit additionally invalidates disapprovals as well as approvals. There are arguments both for and against that behaviour (and hence whether it should be replicated on GitHub):

If someone submits a review of an early version of a PR which requires changes (i.e. a -1 vote), then if that disapproval is not reset when a new version of the PR is pushed, it means that a follow-up re-review from that specific person is required before the PR can be merged. That has one advantage of ensuring that the concerns they expressed are fixed by the new version; however there is a risk that that person may go AWOL and leave the PR blocked forever. In that case, a workaround might be to close the PR and resubmit it. This might seem like an overly noisy approach, but OTOH it might be good to make the process of effectively manually overriding someone's disapproval a noisy operation.

[aspiers]

I've also submitted this via GitHub's Contact a human button.

[aspiers]

Reply from GitHub:

Hi Adam -

Thanks for getting in touch and sharing your thoughts on the new pull request reviews. We're glad you like them! I've passed your idea of reseting a review whenever new pushes are made on to the team working on this so they can see it, though I can't promise if/when this will be implemented.

If there's anything else you'd like to see improved, please let us know!

All the best,
GitHub Staff

[aspiers]

In the case that the PR assignee feature is used to denote a responsibility to review the PR, this issue becomes related to #778, because of the following workflow:

1. PR is submitted
2. Person X is assigned to the PR to request a review from them
3. Person X reviews the PR and removes their assignment
4. PR is updated according to their feedback
5. At this point, as per this issue X's review should be reset, but ideally they would be automatically reassigned to the event, indicating "please review the new version". Currently this reassignment has to be done manually after the PR has been updated.

[aspiers]

GitHub has just added support for dismissing reviews on pull requests, which means that reviews can now be reset as per above if they are out of date. However, this can only be done manually, not automatically when a review is updated, so this issue is still valid.

[aspiers]

Wow, apparently GitHub quietly added this feature at some point recently - I can't find any mention of it in their blog about new features, but it's missing from the screenshot in their blog announcing restriction of who can dismiss reviews, yet included in their User Documentation page Enabling required reviews for pull requests.

So it looks like we can close this now! 🎉 🎆 🍾 😎

[aspiers]

Worth noting that this feature only dismisses approvals, not reviews which request changes.

[GlennPiper]

WOO HOO! I was hoping there was a way to fix this, and just came across your update indicating it was added. Thank you for adding that comment @aspiers !

[megatux]

I do not understand how this solves the issue. I do not want it for a specific "protected" branch, but for any PR review on any branch. Any new code should be reviewed. The tool should help with that.

[aspiers]

@megatux That's a very good point. I believe that the most commonly used development workflow is that PRs only get submitted to protected branches, and GitHub have partially solved this aspect of reviewing for that scenario. However, if your development workflow involves a less typical branching strategy where PRs are sent to a constantly changing set of branches then their current solution is incomplete.

Additionally, my observation in the comment above that the feature only dismisses approvals lends further weight to the notion that this problem is not fully solved; some people may also want it to dismiss -1 reviews which request changes. Therefore I'm reopening this.