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

[Mark Reinhold]

2024/6/18 12:00:00 -0400, iris.clark at oracle.com:
> 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

Iris -- thanks for this proposal.

To summarize the key points of the discussion:

  - This change will require a bit more work on the part of Reviewers
    when a re-review is required.  They can minimize that work, however,
    by first looking at the diff of changes since their last review; if
    that diff is small (e.g., just a copyright-year update) then they
    need not re-review the entire PR [1].

  - This change might motivate contributors to squash their commits
    together or push them in bulk so that Reviewers aren't prompted to
    re-review PRs too frequently [2].

  - When a re-review is required then only the number of Reviewers
    required by Skara will need to re-review.  The default number of
    Reviewers required is one, but for a particular PR this can be
    increased via the `/reviewers` command [3].  A Reviewer can later
    decrease that via the same command, so if a contributor commits a
    late but trivial change to a PR then the first Reviewer who examines
    it can use `/reviewers 1` to enable immediate integration [4].

On balance, the benefit of this change appears to justify the cost.
Let’s adopt it effective tomorrow, as Iris proposed.  If it proves more
onerous than we expect then we’ll re-evaluate it.

- Mark


[1] https://mail.openjdk.org/pipermail/jdk-dev/2024-June/009139.html
[2] https://mail.openjdk.org/pipermail/jdk-dev/2024-June/009143.html
[3] https://mail.openjdk.org/pipermail/jdk-dev/2024-June/009146.html
[4] https://mail.openjdk.org/pipermail/jdk-dev/2024-June/009156.html