Proposal: Require re-review before integration if the PR is modified

[Pavel Rappo]

A few comments on concerns I've seen in this thread and elsewhere so far.

- If this proposal is accepted, some Committers and Authors will have to change their habits. For example, they might start to lump their commits and/or push them in bulk so that Reviewers aren't prompted to re-review a PR too often.

- Maybe, just maybe, if Skara can be taught to reliably recognise a genuine, uninteresting merge with the branch a PR is targeting, it can allow integration without re-review? Would this ease the Reviewers' burden and not penalise the "merge to test with GitHub Actions" use case?

-Pavel

> On 18 Jun 2024, at 17:00, Iris Clark <iris.clark at oracle.com> wrote:
> 
> Hi.
> 
> Every PR must be reviewed by at least one Reviewer [0] before it's integrated
> into the main-line repository [1].  While we recommend that the PR contain the
> final version of the change at creation time, this is not always feasible,
> particularly for large or complex changes.  The integrated version of the PR
> may evolve significantly from when the review was done.  Thus the final
> commit's "Reviewed-by" field may give false confidence that the complete set
> of changes has been reviewed before it was integrated into the repository.
> This could, in the worst case, allow an adversary to commit malicious code.
> 
> I propose that reviews be automatically marked as stale when the PR is
> updated, and re-review be required before integration.  Stale reviews do not
> count towards the minimum number of reviews required for integration.  The
> final commit's "Reviewed-by" field will continue to list all reviewers,
> regardless of whether they evaluated an older version of the PR.  When a PR is
> updated, instead of simply noting that the PR applies to a particular version,
> the review will be noted as stale, indicating that the PR does not meet
> integration requirements.  This re-review requirement has been in effect for
> the OpenJFX repos [2] since October 2019, when OpenJFX moved to GitHub using
> the Skara tooling.
> 
> I suggest that we adopt this policy two weeks hence, on Tue 3 July.
> 
> Concerns?
> 
> [0] https://openjdk.org/guide/#fixing-a-bug (step 12)
> [1] https://github.com/openjdk/jdk
> [2] https://github.com/openjdk/jfx