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

[Kevin Rushforth]

When I do a re-review of a PR I've already approved, I only looks at the 
diffs since my last review. So the amount of additional effort for a 
trivial change (e.g., fixing a typo or updating copyright years) that is 
done after I've approved it is very small.

And if it isn't a trivial change, I'd want a closer look anyway.

-- Kevin


On 6/18/2024 1:11 PM, Gibbons, Scott wrote:
> Just a thought - won't this essentially double the load on reviewers?  
> I understand the idea, but are the reviewers able to handle such an 
> increase in workload?
>
> Thanks,
>
> *--Scott Gibbons*
>
> Software Development Engineer, Runtime Engineering
>
> DEVELOPER SOFTWARE ENGINEERING
> Ph: 1-503-456-7756
>
> Cell: 1-469-450-8390
>
> Intel JF1, 2111 NE 25th Ave
>
> Hillsboro, OR 97124
> Intel Corporation | www.intel.com 
> <https://webmail.intel.com/owa/redir.aspx?SURL=WYr7qZDpIv3m1SKFmeHJuzsfCBuGN-jwkBYQUSRR6yrupkscpgzUCGgAdAB0AHAAOgAvAC8AdwB3AHcALgBpAG4AdABlAGwALgBjAG8AbQA.&URL=http%3a%2f%2fwww.intel.com>
>
> ------------------------------------------------------------------------
> *From:* jdk-dev <jdk-dev-retn at openjdk.org> on behalf of Kevin 
> Rushforth <kevin.rushforth at oracle.com>
> *Sent:* Tuesday, June 18, 2024 10:31 AM
> *To:* jdk-dev at openjdk.org <jdk-dev at openjdk.org>
> *Subject:* Re: Proposal: Require re-review before integration if the 
> PR is modified
> Yes, it will. At least the minimum number of reviewers will need to
> re-review.
>
> -- Kevin
>
>
> On 6/18/2024 10:27 AM, Daniel Fuchs wrote:
> > Hi Iris,
> >
> > I would welcome this change. But would it block integration
> > if the new changeset that was pushed is only a merge changeset,
> > merging changes from the mainline into the PR for a last round
> > of testing?
> >
> > best regards,
> >
> > -- daniel
> >
> > On 18/06/2024 17:00, Iris Clark 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
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/jdk-dev/attachments/20240618/11467739/attachment.htm>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: Outlook-z2fj5dpz
Type: image/png
Size: 1250 bytes
Desc: not available
URL: <https://mail.openjdk.org/pipermail/jdk-dev/attachments/20240618/11467739/Outlook-z2fj5dpz.png>