Allow disabling "Dismiss stale pull request approvals when new commits are pushed" even when enabling "Whether the most recent reviewable push must be approved by someone other than the person who pushed it."

[jdimatteo]

Select Topic Area

Bug

Body

In my repo's "Protect matching branches" settings (at https://github.com///settings/branch_protection_rules/) I have:

1. not checked / disabled: "Dismiss stale pull request approvals when new commits are pushed. New reviewable commits pushed to a matching branch will dismiss pull request review approvals."
2. checked / enabled: "Require approval of the most recent reviewable push. Whether the most recent reviewable push must be approved by someone other than the person who pushed it."

In my pr (https://github.com///pull/) my coworker @darist approved the PR. At the bottom of the PR it showed "Changes approved" and the button "Squash and merge" was present:

Then I pushed a commit, and the bottom of the PR changed to "Review required. New changes require approval from someone other than the last pusher."

After unchecking (2), it changed back to "Changes approved" and I successfully merged the PR.

How do I configure my repo to respect disabling (1) "Dismiss stale" even when (2) "must be approved by someone other than the person who pushed it" is checked?

These are two separate settings and arguably shouldn't be intertwined like this -- this is arguably a bug and GH behavior should change so that these settings do result in this PR remaining approved and mergable.

[jdimatteo]

Here is an example in a public repo:

1. I configured https://github.com/jdimatteo/test-repo-settings with these settings:
   1. not checked / disabled: "Dismiss stale pull request approvals when new commits are pushed. New reviewable commits pushed to a matching branch will dismiss pull request review approvals."
   2. checked / enabled: "Require approval of the most recent reviewable push. Whether the most recent reviewable push must be approved by someone other than the person who pushed it."

2. Created Test Repo Settings "Dismiss Stale" disabled and "approved by someone other" enabled jdimatteo/test-repo-settings#26 with author jdimatteo@ with single commit jdimatteo/test-repo-settings@b8bae98

3. foo1111@ approved with a comment requesting a change to punctuation, showing "All checks have passed":

4. jdimatteo@ addressed the punctuation review feedback pushing commit jdimatteo/test-repo-settings@6ccb8b2

expected behavior: PR remains showing "All checks have passed"
actual behavior: PR shows "Review required" and "New changes require approval from someone other than the last pusher."

This behavior is not correct because foo1111@ approved and jdimatteo@ is the last pusher, so the PR already has approval from someone other than the last pusher.

[ftruzzi]

@jdimatteo I completely agree with you. I think this feature should be named, as it initially was, "Require approval from someone other than the last pusher". The purpose would be to avoid someone hijacking your PR by pushing a bad commit and then approving it themselves.

Otherwise, as you said, there is a lot of overlap with the "Dismiss stale pull request approvals when new commits are pushed" feature and it's quite confusing.

I've opened a ticket to GitHub pointing this out and asking them to look at this discussion, but received no reply yet.

[jdimatteo]

Prior example was possibly wrong because approver was not in CODEOWNERS -- here is a correct example:

1. I configured https://github.com/jdimatteo/test-repo-settings with these settings:
   1. not checked / disabled: "Dismiss stale pull request approvals when new commits are pushed. New reviewable commits pushed to a matching branch will dismiss pull request review approvals."
   2. checked / enabled: "Require approval of the most recent reviewable push. Whether the most recent reviewable push must be approved by someone other than the person who pushed it."

2. Created Retry: Test Repo Settings "Dismiss Stale" disabled and "approved by someone other" enabled jdimatteo/test-repo-settings#27 with author foo1111@ with single commit jdimatteo/test-repo-settings@630aedd

3. jdimatteo@ approved with a comment requesting a change to punctuation, showing "All checks have passed":

4. foo1111@ addressed the punctuation review feedback pushing commit jdimatteo/test-repo-settings@8d04e95

expected behavior: PR remains showing "All checks have passed"
actual behavior: PR shows "Review required" and "New changes require approval from someone other than the last pusher."

This behavior is not correct because jdimatteo@ approved and foo1111@ is the last pusher, so the PR already has approval from someone other than the last pusher.

[jdimatteo]

FYI after discussing with GitHub enterprise support they said this is working as designed and they have no intention of changing the behavior nor clarifying the documentation / UI messaging. This is disappointing.