<div dir="ltr"><div>One question, do merges with master cause the Skara reviewer check to fail? </div><div><br></div><div>We try to encourage authors to merge often, especially before pushing. Patches should be based on a reasonably fresh copy of master. I believe the contribution guidelines state that too, and it is just good practice.</div><div><br></div><div>Cheers, Thomas</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, Jun 18, 2024 at 6:00 PM Iris Clark <<a href="mailto:iris.clark@oracle.com">iris.clark@oracle.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi.<br>
<br>
Every PR must be reviewed by at least one Reviewer [0] before it's integrated<br>
into the main-line repository [1].  While we recommend that the PR contain the<br>
final version of the change at creation time, this is not always feasible,<br>
particularly for large or complex changes.  The integrated version of the PR<br>
may evolve significantly from when the review was done.  Thus the final<br>
commit's "Reviewed-by" field may give false confidence that the complete set<br>
of changes has been reviewed before it was integrated into the repository.<br>
This could, in the worst case, allow an adversary to commit malicious code.<br>
<br>
I propose that reviews be automatically marked as stale when the PR is<br>
updated, and re-review be required before integration.  Stale reviews do not<br>
count towards the minimum number of reviews required for integration.  The<br>
final commit's "Reviewed-by" field will continue to list all reviewers,<br>
regardless of whether they evaluated an older version of the PR.  When a PR is<br>
updated, instead of simply noting that the PR applies to a particular version,<br>
the review will be noted as stale, indicating that the PR does not meet<br>
integration requirements.  This re-review requirement has been in effect for<br>
the OpenJFX repos [2] since October 2019, when OpenJFX moved to GitHub using<br>
the Skara tooling.<br>
<br>
I suggest that we adopt this policy two weeks hence, on Tue 3 July.<br>
<br>
Concerns?<br>
<br>
[0] <a href="https://openjdk.org/guide/#fixing-a-bug" rel="noreferrer" target="_blank">https://openjdk.org/guide/#fixing-a-bug</a> (step 12)<br>
[1] <a href="https://github.com/openjdk/jdk" rel="noreferrer" target="_blank">https://github.com/openjdk/jdk</a><br>
[2] <a href="https://github.com/openjdk/jfx" rel="noreferrer" target="_blank">https://github.com/openjdk/jfx</a><br>
</blockquote></div>