<!DOCTYPE html><html><head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
</head>
<body>
<div class="moz-cite-prefix">On 6/24/24 06:38, Thomas Stüfe wrote:<br>
</div>
<blockquote type="cite" cite="mid:CAA-vtUy3mcp-sx9D=HP8KTBS0e8SCEp7TohqvWZ5eB2Xs8Da5g@mail.gmail.com">
<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>
</blockquote>
<p>Skara does not have a way to distinguish clean merges from the
target branch from other changes when determining if a review is
stale. It simple compares the hash the review was performed at
with the current head hash of the PR source branch.</p>
<p>Implementing such a check may be possible in the future, but it's
not something we can promise to happen.<br>
</p>
<p>/Erik<br>
</p>
<blockquote type="cite" cite="mid:CAA-vtUy3mcp-sx9D=HP8KTBS0e8SCEp7TohqvWZ5eB2Xs8Da5g@mail.gmail.com">
<div dir="ltr">
<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" moz-do-not-send="true" class="moz-txt-link-freetext">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" moz-do-not-send="true" class="moz-txt-link-freetext">https://openjdk.org/guide/#fixing-a-bug</a>
(step 12)<br>
[1] <a href="https://github.com/openjdk/jdk" rel="noreferrer" target="_blank" moz-do-not-send="true" class="moz-txt-link-freetext">https://github.com/openjdk/jdk</a><br>
[2] <a href="https://github.com/openjdk/jfx" rel="noreferrer" target="_blank" moz-do-not-send="true" class="moz-txt-link-freetext">https://github.com/openjdk/jfx</a><br>
</blockquote>
</div>
</blockquote>
</body>
</html>