[feature] - Clear approvals if new commit is pushed

[zopanix]

• Gitea version (or commit ref): 1.7.1
• Git version: n/a
• Operating system: docker gitea/gitea:1.7.1
• Database (use [x]):
  • PostgreSQL
  • MySQL
  • MSSQL
  • SQLite
• Can you reproduce the bug at https://try.gitea.io:
  • Yes (provide example URL)
  • No
  • Not relevant
• Log gist: N/A

Description

As a user, I want to be able to discard approvals automatically on my PR if a new commit has been pushed on my branch.

In our current workflow, we would like to be able to reset approvals completely if someone pushes a new commit on the PR. We've had some ninja pushes to repo's like that in the past and this feature would enforce our workflow.

I would imagine this feature being located in the branch section of the repository, when I enable protected branches, it would be an additional checkbox titled :
[x] Reset approvals on push

I know github doesn't have this feature (last time I checked), but bitbucket and gitlab do have it and can be very useful for larger teams with a lot of juniors.

On the technical side of things, I would imagine it being some sort of pre-commit hook that would check if there is a pull request, and on that pull request if there were approvals, and remove them if there were.

[zopanix]

I searched for a similar issue but could not find one

[pclermont]

+1

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs during the next 2 weeks. Thank you for your contributions.

[lunny]

We should give an option on protected branch to allow reset or not.

[MarkusAmshove]

👮‍♂️ In our current workflow, we would like to be able to reset approvals completely if someone pushes a new commit on the PR. We've had some ninja pushes to repo's like that in the past and this feature would enforce our workflow

[timothyqiu]

FYI, GitHub has this feature as "Dismiss stale pull request approvals when new commits are pushed".

https://help.github.com/assets/images/help/repository/PR-reviews-required.png

[lunny]

Yes, we also need an option on protected branch just like github.

[davidsvantesson]

Should there be commits which do not trigger stale approvals? I am mainly thinking of merges with the target branch, if the diff has not changed the review would still be valid? I think that will be the hardest part of this issue to solve, if needed.

[guillep2k]

Should there be commits which do not trigger stale approvals? I am mainly thinking of merges with the target branch, if the diff has not changed the review would still be valid? I think that will be the hardest part of this issue to solve, if needed.

I agree. That shouldn't mean a rollback in approvals.

[lafriks]

But only merges from PR target branch can be skipped ;)

[davidsvantesson]

@lafriks Yeah, but is that sufficient condition? A normal case is to solve merge conflicts when doing merges, meaning the diff to the target branch will not be the same. Would old approvals still count in that case? There are even people doing completely unrelated changes in a merge commit. 😱

Thus I was more thinking it was necessary to check if the diff to the target branch was changed (diff the diff 😄 ). I did some initial tests how GitHub is handling this, and they don't stale approvals on merges. However I have not tested these scenarios.

[MarkusAmshove]

In my opinion every commit (except empty ones) should invalidate approvals.
This includes merge commits (no matter if from target or source), because they may change the content and introduce unreviewed changes

[davidsvantesson]

If the diff to the target branch doesn't change by a commit (this includes clean merge commits), I don't think the content of the PR has changed and thus invalidating approvals should preferably not be done. But implementation-wise it will be much easier to just invalidate at every commit, so if acceptable that could be an initial approach.

[davidsvantesson]

I looked a bit on this. To know if a new push will affect the PR diff (to target branch), it is necessary to compare the two diffs (diff between merge base and old commit and new commit respectively). Additionaly the git diff includes which two commits a file differ, this has to be ignored because it can update even if the diff will not (see line starting with_index_ below).

diff --git a/README.md b/README.md
index 68fc835..00a9243 100644
--- a/README.md
+++ b/README.md

As there was recently a change to not store the patch on file (PR #9302), the old diff will have to be calculated again.

[zeripath]

Ok. How can we uniquely define a patch? Essentially it the difference between two shas.

So when we approve we need to store the current head and current base SHAs. Head SHA is simple, it's the pulls ref for this PR. Base branch SHA is a bit more difficult but could be set as another ref.

How do we account for force pushes? here's where things are more difficult. We will have to check the differences using the base repo pulls heads before these are updated. It's also here that we should generate the diffs to send to watchers.

Now how do we check if these are different diffs? Well we could just generate the patches to a temporary file or perhaps better we could apply the old patch to an index from the new base SHA and then ensure there is no diff on the new head SHA - returning the diff between these if there is one.

Hopefully that makes some sense.

[davidsvantesson]

@zeripath I don't think we need to store anything at approve time. If a push causes approvals to become stale it would make all approvals for that PR stale. For sure someone might force-push the old SHA back and make the approval valid again, but IMO we don't need to consider that case.

So my idea is either

• Check every new commit to see if it changes the diff with the merge branch
• Assume new commits make approvals stale except if it is a merge commit. Then we could do a test merge to see if the merge was "clean" (resulting in same SHA or content as the actual commit).
• Ignoring this case for now and always mark old reviews as stale at new pushes. But I think It would be a bit annoying.

[davidsvantesson]

Pushes to base branch: In my opinion this should not make approvals stale / require re-review. If there are merge conflicts that would require re-review after that has been fixed.