Intelligent 'Dismiss stale approvals when new commits are pushed'

[sandstrom]

There is a Github branch protection setting called "Dismiss stale pull request approvals when new commits are pushed". Unfortunately, this one won't allow a workflow with fixup and squash commits.

Feature Request

During a PR review with it's common that 1-2 fixup commits are added to address reviewer concerns. It would be good if the PR approvals were not dismissed, in the scenario where commits are squashed, but no actual files has changed, i.e. there are no code changes, only commits being moved around or merged.

Example Scenario

1. A developer adds a PR with the following commits:

- Add first name to user model (a931bb)
- Update user icon (72ce49)
- Adjust user profile wording (eff2aa)

2. During the review phase, the author adds two fixup commits, to address concerns that came up. The commits are now:

- Add first name to user model (a931bb)
- Update user icon (72ce49)
- Adjust user profile wording (eff2aa)
- fixup! Add first name (1457e3)
- fixup! Adjust user profile (bb2445)

3. The reviewer adds approval to this PR, everything is good!

4. Now, the problem is, if the author would merge the two fixup commits, the approval would be dismissed as stale, even though there is no difference in the code, only two merged PRs.

Solution: Would be great if a SHA-hash of the entire diff (or each individual touched file in PR) was compared instead, and if they are identical the existing approval is not considered stale, even if commits have changed.

---

[giner]

While this may seem to be a minor feature it would improve development flows a lot at low cost

[sandstrom]

Write directly to Github (their product feedback form) and let them know. They rarely read this forum.

[singhprincejeet]

Rebasing my changes with latest commit without any code change should not lead to dismissal of review either. This promotes people to push stale code v/s getting their code reviewed again.

[bewuethr]

I'm pretty sure that as of today, an "update with rebase" (at least via the UI) does not require re-review, even if "dismiss stale approvals when new commits are pushed" is enabled.

[sandstrom]

That's not what this is about.

It's about the scenario where you have a few "fixup! My other commit" commits present when the PR was approved.

But as a last step before merging, you want to squash them in (but the actual file diff across the full PR is unchanged).

Doing so will invalidate the review.

[bewuethr]

I know that it doesn't fix your issue with fixup/squash, but the rebase issue has been fixed.

[rwultsch]

This request fixes an incessant annoyance for me.

[sandstrom]

Write directly to Github (their product feedback form) and let them know. They rarely read this forum.

[jimbali]

If even Bitbucket can do it then surely GitHub can? 🤔

[nslusher-sf]

I also found this surprising as GitHub's feature set is far superior in every other aspect.

[shlomiken]

The problem i see is this , our branch is defined to be up-to-date , so if someone approved my PR and i need to rebase the approval goes away, this is very annoying as it might happen couple of times a day.

[katherine-black]

If you use the rebase branch button in GitHub, the approval does not get dismissed, even with 'Dismiss stale...' checked.

[sandstrom]

That's not what this issue is about. See the text under "Example Scenario" above. It's about squashing commits together, even though the overall diff for the PR remains unchanged.

[DarylCantrell]

@sandstrom @shlomiken

Thanks for the feedback. As part of the security changes we made to review policies (see this blog post), we also enabled the use-cases you are asking about:

• rebasing locally and pushing the rebased source branch
• merging the target into source branch without conflicts
• splitting commits apart without making code changes
• squashing commits together without making code changes

...or any combination of those operations.

Regards, Daryl

[sandstrom]

Wow! Alright, sounds like you're the best advocate we can have for it! 😄

[ZacSweers]

@DarylCantrell are you saying those were added in June or are being added now in addition to the changes made in June?

[willsmythe]

These changes landed in June, but we neglected to highlight them in the June changelog post.

[dalmolinz]

A very obvious use case that's missing is when applying suggestions. I.e.:

1. A opens PR
2. B approves with suggestions
3. A applies suggestions, say directly from the GH UI
4. The review is dismissed (!?)

[Wind010]

It would be nice to have a manifest of files that would require re-approval if changed. Would also be nice to have some sort of configuration for Levenshtein distance threshold between files in last commit vs the latest commit.