[feature] - Clear approvals if new commit is pushed #2874

@zopanix commented on GitHub (Feb 7, 2019):

I searched for a similar issue but could not find one

@zopanix commented on GitHub (Feb 7, 2019): I searched for a similar issue but could not find one

GiteaMirror commented

@pclermont commented on GitHub (Feb 7, 2019):

+1

@pclermont commented on GitHub (Feb 7, 2019): +1

GiteaMirror commented

@stale[bot] commented on GitHub (Apr 9, 2019):

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.

@stale[bot] commented on GitHub (Apr 9, 2019): 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.

GiteaMirror commented

@lunny commented on GitHub (May 5, 2019):

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

@lunny commented on GitHub (May 5, 2019): We should give an option on protected branch to allow reset or not.

GiteaMirror commented

@markusamshove commented on GitHub (May 29, 2019):

👮‍♂️ 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

@markusamshove commented on GitHub (May 29, 2019): :policeman: 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

GiteaMirror commented

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

@timothyqiu commented on GitHub (May 30, 2019):

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

@timothyqiu commented on GitHub (May 30, 2019): 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> ![Dismiss stale pull request approvals when new commits are pushed](https://help.github.com/assets/images/help/repository/PR-reviews-required-dismiss-stale.png)

GiteaMirror commented

@lunny commented on GitHub (May 30, 2019):

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

@lunny commented on GitHub (May 30, 2019): Yes, we also need an option on protected branch just like github.

GiteaMirror commented

@davidsvantesson commented on GitHub (Nov 26, 2019):

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.

@davidsvantesson commented on GitHub (Nov 26, 2019): 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.

GiteaMirror commented

@guillep2k commented on GitHub (Nov 26, 2019):

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.

@guillep2k commented on GitHub (Nov 26, 2019): > > > 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.

GiteaMirror commented

@lafriks commented on GitHub (Nov 27, 2019):

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

@lafriks commented on GitHub (Nov 27, 2019): But only merges from PR target branch can be skipped ;)

GiteaMirror commented

@davidsvantesson commented on GitHub (Nov 28, 2019):

@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.

@davidsvantesson commented on GitHub (Nov 28, 2019): @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. :scream: 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.

GiteaMirror commented

@markusamshove commented on GitHub (Nov 28, 2019):

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

@markusamshove commented on GitHub (Nov 28, 2019): 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

GiteaMirror commented

@davidsvantesson commented on GitHub (Nov 28, 2019):

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 commented on GitHub (Nov 28, 2019): 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.

GiteaMirror commented

@davidsvantesson commented on GitHub (Dec 14, 2019):

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.

@davidsvantesson commented on GitHub (Dec 14, 2019): 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.

GiteaMirror commented

@zeripath commented on GitHub (Dec 15, 2019):

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.

@zeripath commented on GitHub (Dec 15, 2019): 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.

GiteaMirror commented

@davidsvantesson commented on GitHub (Dec 26, 2019):

@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 commented on GitHub (Dec 26, 2019): @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.

GiteaMirror commented