Proposal: Optimize "Require approval of the most recent reviewable push" for Directly Committed Approver Suggestions

[dillonstreator]

Select Topic Area

Product Feedback

Body

Problem:

The current behavior of "Require pull request reviews before merging," specifically the "Require approval of the most recent reviewable push" option, may lead to unnecessary re-review requests when the pull request owner (A) directly commits the suggested changes made by an authorized reviewer (B). This situation occurs when B approves the PR along with a suggested change, and A incorporates that change directly without introducing any additional modifications.

Scenario:

Consider a team scenario with members A and B. Member A opens a pull request, and member B approves it with a suggested change. If A directly applies that suggested change without introducing any further modifications, the system should recognize that B has effectively reviewed their own suggested change. In this case, triggering a re-review requirement is redundant.

Proposed Solution:

To address this, I suggest refining the "Require approval of the most recent reviewable push" functionality to exempt cases where the PR owner directly commits the suggested changes made by an authorized reviewer. In such instances, where the only change made is the suggested modification already approved by the reviewer, the system should intelligently recognize this and avoid triggering a re-review request.

Benefits:

1. Efficiency: Eliminates unnecessary re-review requests when the PR owner directly applies approved suggestions without additional changes.

2. Maintains Security: Ensures that the pull request review process remains secure and does not allow unapproved changes to bypass approval.

Implementation Details:

1. Check for Direct Commit of Approved Suggestions:

   • Introduce a mechanism to identify when the only changes in a PR are direct commits of previously approved suggestions.
   • Implement logic to exempt such cases from triggering re-review requirements.

2. Documentation Update:

   • Clearly document the refined behavior in the GitHub documentation to provide transparency to users. Specifically the documentation under Require pull request reviews before merging:

     • Optionally, you can require that the most recent reviewable push must be approved by someone other than the person who pushed it. This means at least one other authorized reviewer has approved any changes. For example, the "last reviewer" can check that the latest set of changes incorporates feedback from other reviews, and does not add new, unreviewed content.

       For complex pull requests that require many reviews, requiring an approval from someone other than the last person to push can be a compromise that avoids the need to dismiss all stale reviews: with this option, "stale" reviews are not dismissed, and the pull request remains approved as long as someone other than the person who made the most recent changes approves it. Users who have already reviewed a pull request can reapprove after the most recent push to meet this requirement. If you are concerned about pull requests being "hijacked" (where unapproved content is added to approved pull requests), it is safer to dismiss stale reviews.

Conclusion:

This proposal aims to enhance the pull request review process by making it more nuanced and efficient. By optimizing the "Require approval of the most recent reviewable push" feature to recognize scenarios where the PR owner directly commits approved suggestions, we can streamline the process without compromising the integrity of the review workflow.

[davidnewhall]

This seems entirely sane and was the behavior I expected. Our enterprise and all 12 organizations need this. Re-approvals on already-approved suggestions is a huge time suck. The alternative is to allow unreviewed code into a protected branch. Can we get another option?

[jpgpuyo]

+1000